[Chromium] Implement WebViewImpl::textInputInfo() for Android
Created attachment 141853 [details] work in progress
Created attachment 141864 [details] Patch
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.
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!
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?
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.
Created attachment 142064 [details] Patch
Comment on attachment 142064 [details] Patch Attachment 142064 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12706485
Created attachment 142119 [details] Patch
Created attachment 142125 [details] Patch
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.
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.
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 :(
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
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. :)
Created attachment 142396 [details] Patch for landing
Comment on attachment 142396 [details] Patch for landing Clearing flags on attachment: 142396 Committed r117396: <http://trac.webkit.org/changeset/117396>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 86709
// 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.
Created attachment 142948 [details] Patch for landing
Comment on attachment 142948 [details] Patch for landing Clearing flags on attachment: 142948 Committed r117745: <http://trac.webkit.org/changeset/117745>