Bug 86440

Summary:

[Chromium] Implement WebViewImpl::textInputInfo() for Android

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

New Bugs

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, eric, fishd, jamesr, tkent+wkapi, tomz, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

86709

Bug Blocks:

66687

Attachments:

Description	Flags
work in progress	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none
Patch for landing	none

Adam Barth

Reported 2012-05-14 22:06:27 PDT

[Chromium] Implement WebViewImpl::textInputInfo() for Android

Attachments
work in progress (10.80 KB, patch) 2012-05-14 22:07 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch (12.85 KB, patch) 2012-05-14 23:53 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch (14.26 KB, patch) 2012-05-15 14:47 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch (16.51 KB, patch) 2012-05-15 18:36 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch (16.50 KB, patch) 2012-05-15 19:04 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch for landing (16.52 KB, patch) 2012-05-16 19:31 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch for landing (15.82 KB, patch) 2012-05-20 23:05 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 2012-05-14 22:07:15 PDT

Created attachment 141853 [details] work in progress

Adam Barth

Comment 2 2012-05-14 23:53:06 PDT

Created attachment 141864 [details] Patch

WebKit Review Bot

Comment 3 2012-05-14 23:55:32 PDT

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Eric Seidel (no email)

Comment 4 2012-05-14 23:58:10 PDT

Comment on attachment 141864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141864&action=review This is really a patch for darin. > Source/WebKit/chromium/public/WebTextInputInfo.h:2 > + * Copyright (C) 2011 Google Inc. All rights reserved. Back to the future!

Darin Fisher (:fishd, Google)

Comment 5 2012-05-15 12:21:07 PDT

Comment on attachment 141864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141864&action=review > Source/WebKit/chromium/public/WebTextInputInfo.h:35 > + nit: no newline > Source/WebKit/chromium/public/WebTextInputInfo.h:67 > + return a.type == b.type this is a fair bit of code. i recommend making this non-inline by creating an equals method. see WebNode.h for example. > Source/WebKit/chromium/public/WebWidget.h:36 > #include "WebTextInputType.h" nit: delete this include since it comes in through WebTextInputInfo.h > Source/WebKit/chromium/src/WebViewImpl.cpp:1903 > + info.value = static_cast<HTMLTextAreaElement*>(node)->value(); is this really the best way to do run-time type detection and downcasting?

Adam Barth

Comment 6 2012-05-15 14:00:22 PDT

Comment on attachment 141864 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141864&action=review Thanks for the review. Will fix. >> Source/WebKit/chromium/src/WebViewImpl.cpp:1903 >> + info.value = static_cast<HTMLTextAreaElement*>(node)->value(); > > is this really the best way to do run-time type detection and downcasting? Yeah, this is a relatively common idiom in WebCore, especially in form-related code. When I first discovered it, I tried for a while to break it by searching for a way to disassociate the tag name from the C++ type, but it doesn't appear to be possible.

Adam Barth

Comment 7 2012-05-15 14:47:27 PDT

Created attachment 142064 [details] Patch

WebKit Review Bot

Comment 8 2012-05-15 17:34:32 PDT

Comment on attachment 142064 [details] Patch Attachment 142064 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12706485

Adam Barth

Comment 9 2012-05-15 18:36:04 PDT

Created attachment 142119 [details] Patch

Adam Barth

Comment 10 2012-05-15 19:04:13 PDT

Created attachment 142125 [details] Patch

Tom Zakrajsek

Comment 11 2012-05-15 21:17:51 PDT

Comment on attachment 142125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142125&action=review I was just perusing for my own edification, but did come up with two nits/questions. > Source/WebKit/chromium/public/WebTextInputInfo.h:2 > + * Copyright (C) 2011 Google Inc. All rights reserved. Should this be 2012? > Source/WebKit/chromium/public/WebTextInputInfo.h:73 > + return !(a == b); Why not just return (a != b)? Just curious if I'm missing an idiom I should know about.

Adam Barth

Comment 12 2012-05-15 21:20:05 PDT

Comment on attachment 142125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142125&action=review >> Source/WebKit/chromium/public/WebTextInputInfo.h:2 >> + * Copyright (C) 2011 Google Inc. All rights reserved. > > Should this be 2012? I believe this code was written in 2011. >> Source/WebKit/chromium/public/WebTextInputInfo.h:73 >> + return !(a == b); > > Why not just return (a != b)? Just curious if I'm missing an idiom I should know about. That would just end up calling operator!= recursively. The idea here is to have operator!= call operator== instead.

Tom Zakrajsek

Comment 13 2012-05-15 21:31:36 PDT

Wow, that's embarrassing. Of course that's why it was using !(a==b). It looked so odd to me that i completely missed the context. ValueAdd went from 0 to -1 :(

Darin Fisher (:fishd, Google)

Comment 14 2012-05-15 22:55:35 PDT

Comment on attachment 142125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142125&action=review > Source/WebKit/chromium/src/WebTextInputInfo.cpp:44 > + nit: extra new line

Adam Barth

Comment 15 2012-05-16 17:22:32 PDT

No worries Tom. I appreciate your looking at these patches and providing feedback. I should dig up some of the bonehead webkit-dev posts I've written so you can have a good laugh. :)

Adam Barth

Comment 16 2012-05-16 19:31:21 PDT

Created attachment 142396 [details] Patch for landing

WebKit Review Bot

Comment 17 2012-05-16 22:03:15 PDT

Comment on attachment 142396 [details] Patch for landing Clearing flags on attachment: 142396 Committed r117396: <http://trac.webkit.org/changeset/117396>

WebKit Review Bot

Comment 18 2012-05-16 22:03:21 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 19 2012-05-16 23:29:17 PDT

Re-opened since this is blocked by 86709

Adam Barth

Comment 20 2012-05-20 22:31:13 PDT

// Check WebKit::WebTextInputType and ui::TextInputType is kept in sync. Looks like we need to keep these in sync, which is going to be tricky given that they're in different repos. I'm going to skip adding the new enum values in this patch and tackle that problem in the future.

Adam Barth

Comment 21 2012-05-20 23:05:39 PDT

Created attachment 142948 [details] Patch for landing

WebKit Review Bot

Comment 22 2012-05-21 01:27:00 PDT

Comment on attachment 142948 [details] Patch for landing Clearing flags on attachment: 142948 Committed r117745: <http://trac.webkit.org/changeset/117745>

WebKit Review Bot

Comment 23 2012-05-21 01:27:05 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.