As mentioned in https://bugs.webkit.org/show_bug.cgi?id=102254, we have no tests for this function. It would be good to make sure that we know if its behavior changes.
Created attachment 174514 [details] Patch
Comment on attachment 174514 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174514&action=review > Source/WebKit/chromium/tests/ChromeClientImplTest.cpp:62 > + TestWebViewClient(WebNavigationPolicy* target) : m_target(target) { } explicit
Attachment 174514 [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/tests/ChromeClientImplTest.cpp:35: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 174516 [details] Patch
Added the explicit. I'll log a bug against check-webkit-style for the style nit.
Comment on attachment 174516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174516&action=review > Source/WebKit/chromium/tests/ChromeClientImplTest.cpp:49 > +void setCurrentInputEventForTest(const WebInputEvent* event) > +{ > + WebViewImpl::m_currentInputEvent = event; > +} Would you be willing to put this in WebViewImpl.cpp? The friend trick is nice, but having some distant file poke at private member variables seems worse that linking an extra one-line function into the production build.
Attachment 174516 [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/tests/ChromeClientImplTest.cpp:35: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #6) > (From update of attachment 174516 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174516&action=review > > > Source/WebKit/chromium/tests/ChromeClientImplTest.cpp:49 > > +void setCurrentInputEventForTest(const WebInputEvent* event) > > +{ > > + WebViewImpl::m_currentInputEvent = event; > > +} > > Would you be willing to put this in WebViewImpl.cpp? The friend trick is nice, but having some distant file poke at private member variables seems worse that linking an extra one-line function into the production build. Really? It's a function that nobody should call, ever, in a production binary, and it's going to take up just that extra little bit of space, so I figured that keeping it in test code only was cleaner. You're the reviewer, so I'll do it if you want, but it feels wrong to me.
> Really? It's a function that nobody should call, ever, in a production binary, and it's going to take up just that extra little bit of space, so I figured that keeping it in test code only was cleaner. Ok, let's try it the way you've got it in the patch.
Comment on attachment 174516 [details] Patch Clearing flags on attachment: 174516 Committed r136032: <http://trac.webkit.org/changeset/136032>
All reviewed patches have been landed. Closing bug.