WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
68162
[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
Summary
[chromium]The focus of an input field inside an Iframe doesn't get cleared ev...
chandra shekar vallala
Reported
2011-09-15 07:59:43 PDT
call of WebKit::WebViewImpl::clearFocusedNode is not clearing the focus of input node inside a iframe.
Attachments
fix +unit test patch
(4.09 KB, patch)
2011-09-15 08:10 PDT
,
chandra shekar vallala
abarth
: review-
Details
Formatted Diff
Diff
updated patch
(4.15 KB, patch)
2011-09-15 23:14 PDT
,
chandra shekar vallala
abarth
: review+
Details
Formatted Diff
Diff
updated patch
(4.17 KB, patch)
2011-09-16 12:21 PDT
,
chandra shekar vallala
abarth
: review+
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
resolved-compilation-error.patch
(4.35 KB, patch)
2011-09-17 01:38 PDT
,
chandra shekar vallala
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
chandra shekar vallala
Comment 1
2011-09-15 08:10:11 PDT
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
Comment 2
2011-09-15 08:13:18 PDT
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 3
2011-09-15 11:21:11 PDT
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
Comment 4
2011-09-15 23:14:23 PDT
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 5
2011-09-16 10:46:05 PDT
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
Comment 6
2011-09-16 12:21:34 PDT
Created
attachment 107700
[details]
updated patch
Adam Barth
Comment 7
2011-09-16 12:49:50 PDT
Comment on
attachment 107700
[details]
updated patch Great. Thanks for iterating on this.
WebKit Review Bot
Comment 8
2011-09-16 22:35:32 PDT
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 9
2011-09-16 22:45:34 PDT
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
Comment 10
2011-09-17 01:38:44 PDT
Created
attachment 107767
[details]
resolved-compilation-error.patch resolved compilation error.
WebKit Review Bot
Comment 11
2011-09-17 02:48:32 PDT
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
Comment 12
2011-09-17 02:48:37 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.
Top of Page
Format For Printing
XML
Clone This Bug