Bug 86440 – [Chromium] Implement WebViewImpl::textInputInfo() for Android

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

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

[Adam Barth]

Created attachment 141853 [details]
work in progress

[Adam Barth]

Created attachment 141864 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] 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 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 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 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]

Created attachment 142064 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 142119 [details]
Patch

[Adam Barth]

Created attachment 142125 [details]
Patch

[Tom Zakrajsek]

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 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]

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 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]

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]

Created attachment 142396 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 142396 [details]
Patch for landing

Clearing flags on attachment: 142396

Committed r117396: <http://trac.webkit.org/changeset/117396>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 86709

[Adam Barth]

// 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]

Created attachment 142948 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 142948 [details]
Patch for landing

Clearing flags on attachment: 142948

Committed r117745: <http://trac.webkit.org/changeset/117745>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.