Bug 102424 - [chromium] Add unit tests for ChromeClientImpl::getNavigationPolicy
Summary: [chromium] Add unit tests for ChromeClientImpl::getNavigationPolicy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric U.
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-15 13:40 PST by Eric U.
Modified: 2012-11-28 11:31 PST (History)
2 users (show)

See Also:


Attachments
Patch (10.78 KB, patch)
2012-11-15 14:16 PST, Eric U.
no flags Details | Formatted Diff | Diff
Patch (10.79 KB, patch)
2012-11-15 14:23 PST, Eric U.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric U. 2012-11-15 13:40:37 PST
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.
Comment 1 Eric U. 2012-11-15 14:16:03 PST
Created attachment 174514 [details]
Patch
Comment 2 Adam Barth 2012-11-15 14:20:07 PST
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
Comment 3 WebKit Review Bot 2012-11-15 14:20:20 PST
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.
Comment 4 Eric U. 2012-11-15 14:23:07 PST
Created attachment 174516 [details]
Patch
Comment 5 Eric U. 2012-11-15 14:23:51 PST
Added the explicit.
I'll log a bug against check-webkit-style for the style nit.
Comment 6 Adam Barth 2012-11-15 14:24:36 PST
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.
Comment 7 WebKit Review Bot 2012-11-15 14:27:10 PST
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.
Comment 8 Eric U. 2012-11-15 14:29:53 PST
(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.
Comment 9 Adam Barth 2012-11-15 14:34:59 PST
> 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 10 WebKit Review Bot 2012-11-28 11:31:43 PST
Comment on attachment 174516 [details]
Patch

Clearing flags on attachment: 174516

Committed r136032: <http://trac.webkit.org/changeset/136032>
Comment 11 WebKit Review Bot 2012-11-28 11:31:46 PST
All reviewed patches have been landed.  Closing bug.