Bug 68162 – [chromium]The focus of an input field inside an Iframe doesn't get cleared even though clearFocusedNode is called.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[chandra shekar vallala]

call of  WebKit::WebViewImpl::clearFocusedNode is not clearing the focus of input node inside a iframe.

[chandra shekar vallala]

Created attachment 107498 [details]
fix +unit test patch

In WebKit::WebViewImpl::clearFocusedNode
the focused frame is focusedWebCoreFrame() not m_page->mainFrame().

I attached a patch for this bug along with unit test.

am not sure that should I write the unit test in a new class 'WebViewTest'.

[WebKit Review Bot]

Attachment 107498 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 107498 [details]
fix +unit test patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107498&action=review

Thanks for the patch.  This change looks beneficial, there are just a couple process-oriented nits to clear up and then we'll be all set.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1783
> +    // Get the focused frame.

This comment doesn't explain the "why" of any code, just the "what", which we can read on the next line anyway.  It's probably best to remove.

> Source/WebKit/chromium/ChangeLog:3
> +        Need a short description and bug URL (OOPS!)

Please fill out this information.  The Tools/Scripts/prepare-Changelog script can do that for you if you use the "-b" option.  Alternatively, "webkit-patch upload" can also help make this easier.

> Source/WebKit/chromium/ChangeLog:6
> +

It's a good idea to add some text to the ChangeLog explaining why you're making this change.  That helps folks reviewing your patch understand that problem you're solving and it helps other folks looking at this change in the future understand what you're accomplishing.

[chandra shekar vallala]

Created attachment 107608 [details]
updated patch

Updated the patch as per comments.
I included the description and bug Id in ChangeLog file.

[Adam Barth]

Comment on attachment 107608 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107608&action=review

> Source/WebKit/chromium/tests/WebFrameTest.cpp:239
> +class TestWebViewClient : public WebViewClient { };

I'm not 100% clear why we need this class.  Can't we just use WebViewClient directly?

> Source/WebKit/chromium/ChangeLog:7
> +        [chromium]The focus of an input field inside an Iframe doesn't get cleared even though clearFocusedNode is called.
> +        https://bugs.webkit.org/show_bug.cgi?id=68162
> +
> +        Reviewed by NOBODY (OOPS!).
> +

In the future, it would be better to add more explanation to the ChangeLog entry about why you're making this change.  It's not just a huge deal for this patch, but it's a good habit.

[chandra shekar vallala]

Created attachment 107700 [details]
updated patch

[Adam Barth]

Comment on attachment 107700 [details]
updated patch

Great.  Thanks for iterating on this.

[WebKit Review Bot]

Comment on attachment 107700 [details]
updated patch

Rejecting attachment 107700 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
der.o
make: *** [out/Debug/obj.target/webkit_unit_tests/Source/WebKit/chromium/tests/WebFrameTest.o] Error 1
make: *** Waiting for unfinished jobs....

Failed to run "['Tools/Scripts/build-webkit', '--debug', '--chromium', '--update-chromium']" exit_code: 2
.o
  CXX(target) out/Debug/obj.target/DumpRenderTree/Tools/DumpRenderTree/chromium/EventSender.o
make: *** [out/Debug/obj.target/webkit_unit_tests/Source/WebKit/chromium/tests/WebFrameTest.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/9729306

[WebKit Review Bot]

Comment on attachment 107700 [details]
updated patch

Attachment 107700 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9736005

[chandra shekar vallala]

Created attachment 107767 [details]
resolved-compilation-error.patch

resolved compilation error.

[WebKit Review Bot]

Comment on attachment 107767 [details]
resolved-compilation-error.patch

Clearing flags on attachment: 107767

Committed r95380: <http://trac.webkit.org/changeset/95380>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.