Bug 102424 – [chromium] Add unit tests for ChromeClientImpl::getNavigationPolicy

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric U.]

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.

[Eric U.]

Created attachment 174514 [details]
Patch

[Adam Barth]

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

[WebKit Review Bot]

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.

[Eric U.]

Created attachment 174516 [details]
Patch

[Eric U.]

Added the explicit.
I'll log a bug against check-webkit-style for the style nit.

[Adam Barth]

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.

[WebKit Review Bot]

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.

[Eric U.]

(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.

[Adam Barth]

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

[WebKit Review Bot]

Comment on attachment 174516 [details]
Patch

Clearing flags on attachment: 174516

Committed r136032: <http://trac.webkit.org/changeset/136032>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.