Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2955503002:
Make interstitial links open in a new tab (Closed)
DescriptionMake interstitial links open in a new tab
BUG=717616
https: //badssl.com and make sure that the various info links open in a
Review-Url: https://codereview.chromium.org/2955503002
Cr-Commit-Position: refs/heads/master@{#483231}
Committed: https://chromium.googlesource.com/chromium/src/+/a22cb4d1b1f6afc95aba0eb04246c13fe178697e
Patch Set 1 #Patch Set 2 : Fix build errors #Patch Set 3 : Add working test #Patch Set 4 : Rebase CL #Patch Set 5 : Rebase CL #
Total comments: 22
Patch Set 6 : iOS implementation of refactor #Patch Set 7 : Addressing CL comments and more refactoring #Patch Set 8 : Addressing CL comments #Patch Set 9 : Remove unnecessary comment #Patch Set 10 : Fix broken test #Patch Set 11 : Fix VisitWhitePaper test #Patch Set 12 : Mock out help center URL in SafeBrowsingBlockingPageTest #
Total comments: 28
Patch Set 13 : Address CL comments #
Total comments: 1
Patch Set 14 : Typo fix #Patch Set 15 : Remove all mentions of outdated OpenURLInCurrentTab function #Patch Set 16 : Rebase on master #Patch Set 17 : Fix typo #Patch Set 18 : Open links in a new tab only if should_open_links_in_new_tab flag is set #Patch Set 19 : Fix compilation error #Messages
Total messages: 95 (59 generated)
Description was changed from ========== Make interstitial links open in a new tab BUG=717616 TEST=Build and run Chromium. Visit the various interstitials on https://badssl.com and make sure that the various info links open in a new foreground tab. ========== to ========== Make interstitial links open in a new tab BUG=717616 TEST=Build and run Chromium. Visit the various interstitials on https://badssl.com and make sure that the various info links open in a new foreground tab. ==========
sperigo@chromium.org changed reviewers: + estark@chromium.org, meacer@chromium.org
Description was changed from ========== Make interstitial links open in a new tab BUG=717616 TEST=Build and run Chromium. Visit the various interstitials on https://badssl.com and make sure that the various info links open in a new foreground tab. ========== to ========== Make interstitial links open in a new tab BUG=717616 TEST=Build and run Chromium. Visit the various interstitials on https://badssl.com and make sure that the various info links open in a new foreground tab. There's also a browser test (TestInterstitialLinksOpenInNewTab) in SSLUITests. ==========
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This CL adds code to make the interstitial links ("Learn More", "Privacy
Policy", etc.) open in a new foreground tab. There is a browser test added in
ssl_browser_tests.cc.
Writing a test for this functionality involved quite a bit of refactoring.
Thanks to estark@ for helping me out with the refactor and testing!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sperigo@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Looks very good, thanks for writing the test as well. I only have minor comments. build/linux/bin/eu-strip shouldn't be needed. Also, SafeBrowsingBlockingPageBrowserTest.LearnMore is broken now because it expects the interstitial to navigate away. Instead, you can make it check that a new tab opens like you are doing here. https://codereview.chromium.org/2955503002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2955503002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:2988: CONTENT_SETTING_BLOCK); This will block javascript on all pages, shouldn't be necessary for this test case. https://codereview.chromium.org/2955503002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3023: GURL mock_help_center_url = https_server_.GetURL("/title1.html"); nit: You can make this const as it's a read only variable. Makes it slightly easier to reason about the code. (const GURL mock_help_center_url = ...) https://codereview.chromium.org/2955503002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3029: std::string javascript = nit: Also const https://codereview.chromium.org/2955503002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3038: } As an extra, you can also check the URL of the newly opened tab here. Since the new tab will be foreground, browser()->tab_strip_model()->GetActiveWebContents() should give you the new tab, and then you can check tab->GetURL() value on that new tab. https://codereview.chromium.org/2955503002/diff/80001/components/security_int... File components/security_interstitials/content/security_interstitial_controller_client.cc (right): https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/content/security_interstitial_controller_client.cc:20: // Help Center url nit: End comments with a period. https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/content/security_interstitial_controller_client.cc:35: help_center_url_ = GURL(kHelpCenterUrl); You can move this to the parameter list: ... default_safe_page_(default_safe_page), help_center_url_(GURL(kHelpCenterURL)) { // (or maybe even help_center_url_(kHelpCenterURL) } https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/content/security_interstitial_controller_client.cc:124: const GURL test_url) { const GURL& test_url https://codereview.chromium.org/2955503002/diff/80001/components/security_int... File components/security_interstitials/content/security_interstitial_controller_client.h (right): https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/content/security_interstitial_controller_client.h:54: void SetHelpCenterUrlForTesting(const GURL test_url) override; const GURL& test_url https://codereview.chromium.org/2955503002/diff/80001/components/security_int... File components/security_interstitials/core/controller_client.h (right): https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/core/controller_client.h:100: virtual void SetHelpCenterUrlForTesting(const GURL test_url) = 0; const GURL& https://codereview.chromium.org/2955503002/diff/80001/components/security_int... File components/security_interstitials/core/safe_browsing_loud_error_ui.h (right): https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/core/safe_browsing_loud_error_ui.h:51: GURL learn_more_test_url_; It seems like this variable isn't used (you have local learn_more_test_url variables in the .cc file)
The CQ bit was checked by sperigo@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
Addressed Mustafa's comments! https://codereview.chromium.org/2955503002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2955503002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:2988: CONTENT_SETTING_BLOCK); On 2017/06/23 20:28:32, meacer wrote: > This will block javascript on all pages, shouldn't be necessary for this test > case. Done. https://codereview.chromium.org/2955503002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3023: GURL mock_help_center_url = https_server_.GetURL("/title1.html"); On 2017/06/23 20:28:31, meacer wrote: > nit: You can make this const as it's a read only variable. Makes it slightly > easier to reason about the code. > > (const GURL mock_help_center_url = ...) Done. https://codereview.chromium.org/2955503002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3029: std::string javascript = On 2017/06/23 20:28:31, meacer wrote: > nit: Also const Done. https://codereview.chromium.org/2955503002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3038: } On 2017/06/23 20:28:32, meacer wrote: > As an extra, you can also check the URL of the newly opened tab here. > > Since the new tab will be foreground, > browser()->tab_strip_model()->GetActiveWebContents() should give you the new > tab, and then you can check tab->GetURL() value on that new tab. Good idea! https://codereview.chromium.org/2955503002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3038: } On 2017/06/23 20:28:32, meacer wrote: > As an extra, you can also check the URL of the newly opened tab here. > > Since the new tab will be foreground, > browser()->tab_strip_model()->GetActiveWebContents() should give you the new > tab, and then you can check tab->GetURL() value on that new tab. Done. https://codereview.chromium.org/2955503002/diff/80001/components/security_int... File components/security_interstitials/content/security_interstitial_controller_client.cc (right): https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/content/security_interstitial_controller_client.cc:20: // Help Center url On 2017/06/23 20:28:32, meacer wrote: > nit: End comments with a period. Done. https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/content/security_interstitial_controller_client.cc:35: help_center_url_ = GURL(kHelpCenterUrl); On 2017/06/23 20:28:32, meacer wrote: > You can move this to the parameter list: > > ... > default_safe_page_(default_safe_page), > help_center_url_(GURL(kHelpCenterURL)) { // (or maybe even > help_center_url_(kHelpCenterURL) > } Done. https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/content/security_interstitial_controller_client.cc:124: const GURL test_url) { On 2017/06/23 20:28:32, meacer wrote: > const GURL& test_url Done. https://codereview.chromium.org/2955503002/diff/80001/components/security_int... File components/security_interstitials/content/security_interstitial_controller_client.h (right): https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/content/security_interstitial_controller_client.h:54: void SetHelpCenterUrlForTesting(const GURL test_url) override; On 2017/06/23 20:28:32, meacer wrote: > const GURL& test_url Done. https://codereview.chromium.org/2955503002/diff/80001/components/security_int... File components/security_interstitials/core/controller_client.h (right): https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/core/controller_client.h:100: virtual void SetHelpCenterUrlForTesting(const GURL test_url) = 0; On 2017/06/23 20:28:32, meacer wrote: > const GURL& Done. https://codereview.chromium.org/2955503002/diff/80001/components/security_int... File components/security_interstitials/core/safe_browsing_loud_error_ui.h (right): https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/core/safe_browsing_loud_error_ui.h:51: GURL learn_more_test_url_; On 2017/06/23 20:28:32, meacer wrote: > It seems like this variable isn't used (you have local learn_more_test_url > variables in the .cc file) Oops! https://codereview.chromium.org/2955503002/diff/80001/components/security_int... components/security_interstitials/core/safe_browsing_loud_error_ui.h:51: GURL learn_more_test_url_; On 2017/06/23 20:28:32, meacer wrote: > It seems like this variable isn't used (you have local learn_more_test_url > variables in the .cc file) Done.
The CQ bit was checked by sperigo@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by sperigo@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
LGTM! You might want to want to wait for estark's lgtm as well.
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is looking great! Left some comments inline that are almost entirely style nits. https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:734: void MockHelpCenterUrl(InterstitialPage* interstitial_page) { nit: maybe add a brief comment above this method to explain why it exists (e.g.: "Modify the interstitial to use a test server URL for its help center link.") https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:749: const GURL mock_help_center_url = https_server_.GetURL("/title1.html"); nit: const GURL& or just inline it below: SetBaseHelpCenterUrlForTesting(https_server_.GetURL(...)) https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:820: WebContents* interstitial_tab = nit: ASSERT_TRUE(interstitial_tab) after this line to make the test fail nicely instead of crashing if for some reason the tab doesn't exist https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:833: AssertNoInterstitial(false); // Assert the interstitial is not present in nit: comment formatting is a little weird, you can probably just it on the line above https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:837: WebContents* new_tab = browser()->tab_strip_model()->GetActiveWebContents(); nit: ASSERT_TRUE(new_tab) after this line to make the test fail nicely instead of crashing if for some reason the tab doesn't exist https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1173: WebContents* new_tab = browser()->tab_strip_model()->GetWebContentsAt(1); same nit about ASSERT_TRUE(new_tab) https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:668: // Helper function for TestInterstitialLinksOpenInNewTab. nit: perhaps add a sentence to explain why this is a helper method instead of just inlined below, for example: "Implemented as a test fixture method because the whole test fixture class is friended by SSLBlockingPage." https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3012: // Mocking out the help center URL so that our test will hit the test server teeny-tiny nit: "Mocking" => "Mock" https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3019: const GURL mock_help_center_url = https_server_.GetURL("/title1.html"); nit: const GURL& (otherwise it'll make an unnecessary copy) https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3035: WebContents* new_tab = browser()->tab_strip_model()->GetActiveWebContents(); nit: ASSERT_TRUE(new_tab) after this line https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3037: EXPECT_EQ(mock_help_center_url.Resolve("answer/6098869"), new_tab->GetURL()); For a slightly less brittle test, you could do: EXPECT_EQ(mock_help_center_url.host(), new_tab->GetURL().host()); That'll check that the help center url was to the test server as expected, but someone can change the path without having to update the test. https://codereview.chromium.org/2955503002/diff/220001/components/security_in... File components/security_interstitials/core/controller_client.h (right): https://codereview.chromium.org/2955503002/diff/220001/components/security_in... components/security_interstitials/core/controller_client.h:92: virtual void OpenUrlInCurrentTab(const GURL& url) = 0; Is this still used anywhere? Can we get rid of it?
Oh, you'll also need a couple more OWNERS to review from chrome/browser/safe_browsing/OWNERS and ios/chrome/OWNERS
https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3019: const GURL mock_help_center_url = https_server_.GetURL("/title1.html"); On 2017/06/27 00:36:21, estark wrote: > nit: const GURL& > > (otherwise it'll make an unnecessary copy) I don't think you want a reference here, it'll be a ref to a temp variable so copying is correct. Consider this a "GURL mock_help_center_url" but with a const.
https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3019: const GURL mock_help_center_url = https_server_.GetURL("/title1.html"); On 2017/06/27 00:41:46, meacer wrote: > On 2017/06/27 00:36:21, estark wrote: > > nit: const GURL& > > > > (otherwise it'll make an unnecessary copy) > > I don't think you want a reference here, it'll be a ref to a temp variable so > copying is correct. Consider this a "GURL mock_help_center_url" but with a > const. Oops, that's right, my bad!
I addressed Emily's comments. A couple remaining questions: - Should I go ahead and remove the OpenLinkInCurrentTab method? I don't see it being used, but I left it in case there was a use case I was unfamiliar with. - Silly question, but how do I find OWNERS to add to my CL? Thanks! https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:734: void MockHelpCenterUrl(InterstitialPage* interstitial_page) { On 2017/06/27 00:36:21, estark wrote: > nit: maybe add a brief comment above this method to explain why it exists (e.g.: > "Modify the interstitial to use a test server URL for its help center link.") Done. https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:749: const GURL mock_help_center_url = https_server_.GetURL("/title1.html"); On 2017/06/27 00:36:21, estark wrote: > nit: const GURL& > or just inline it below: > SetBaseHelpCenterUrlForTesting(https_server_.GetURL(...)) Done. https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:820: WebContents* interstitial_tab = On 2017/06/27 00:36:21, estark wrote: > nit: ASSERT_TRUE(interstitial_tab) after this line to make the test fail nicely > instead of crashing if for some reason the tab doesn't exist Done. https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:833: AssertNoInterstitial(false); // Assert the interstitial is not present in On 2017/06/27 00:36:21, estark wrote: > nit: comment formatting is a little weird, you can probably just it on the line > above Done. https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:837: WebContents* new_tab = browser()->tab_strip_model()->GetActiveWebContents(); On 2017/06/27 00:36:21, estark wrote: > nit: ASSERT_TRUE(new_tab) after this line to make the test fail nicely instead > of crashing if for some reason the tab doesn't exist Done. https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1173: WebContents* new_tab = browser()->tab_strip_model()->GetWebContentsAt(1); On 2017/06/27 00:36:21, estark wrote: > same nit about ASSERT_TRUE(new_tab) Done. https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:668: // Helper function for TestInterstitialLinksOpenInNewTab. On 2017/06/27 00:36:21, estark wrote: > nit: perhaps add a sentence to explain why this is a helper method instead of > just inlined below, for example: "Implemented as a test fixture method because > the whole test fixture class is friended by SSLBlockingPage." Good idea! https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3012: // Mocking out the help center URL so that our test will hit the test server On 2017/06/27 00:36:21, estark wrote: > teeny-tiny nit: "Mocking" => "Mock" Done. https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3019: const GURL mock_help_center_url = https_server_.GetURL("/title1.html"); On 2017/06/27 00:36:21, estark wrote: > nit: const GURL& > > (otherwise it'll make an unnecessary copy) Acknowledged. https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3019: const GURL mock_help_center_url = https_server_.GetURL("/title1.html"); On 2017/06/27 00:41:46, meacer wrote: > On 2017/06/27 00:36:21, estark wrote: > > nit: const GURL& > > > > (otherwise it'll make an unnecessary copy) > > I don't think you want a reference here, it'll be a ref to a temp variable so > copying is correct. Consider this a "GURL mock_help_center_url" but with a > const. Acknowledged. https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3019: const GURL mock_help_center_url = https_server_.GetURL("/title1.html"); On 2017/06/27 00:45:46, estark wrote: > On 2017/06/27 00:41:46, meacer wrote: > > On 2017/06/27 00:36:21, estark wrote: > > > nit: const GURL& > > > > > > (otherwise it'll make an unnecessary copy) > > > > I don't think you want a reference here, it'll be a ref to a temp variable so > > copying is correct. Consider this a "GURL mock_help_center_url" but with a > > const. > > Oops, that's right, my bad! Acknowledged. https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3035: WebContents* new_tab = browser()->tab_strip_model()->GetActiveWebContents(); On 2017/06/27 00:36:21, estark wrote: > nit: ASSERT_TRUE(new_tab) after this line Done. https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3037: EXPECT_EQ(mock_help_center_url.Resolve("answer/6098869"), new_tab->GetURL()); On 2017/06/27 00:36:22, estark wrote: > For a slightly less brittle test, you could do: > > EXPECT_EQ(mock_help_center_url.host(), new_tab->GetURL().host()); > > That'll check that the help center url was to the test server as expected, but > someone can change the path without having to update the test. Good idea! Thanks. https://codereview.chromium.org/2955503002/diff/220001/components/security_in... File components/security_interstitials/core/controller_client.h (right): https://codereview.chromium.org/2955503002/diff/220001/components/security_in... components/security_interstitials/core/controller_client.h:92: virtual void OpenUrlInCurrentTab(const GURL& url) = 0; On 2017/06/27 00:36:22, estark wrote: > Is this still used anywhere? Can we get rid of it? I don't think so! I left it in in case there was a use case I was unfamiliar with. Should I remove it?
Still lgtm. Thanks for fixing all the tests! On 2017/06/27 17:31:59, sperigo wrote: > I addressed Emily's comments. A couple remaining questions: > > - Should I go ahead and remove the OpenLinkInCurrentTab method? I don't see it > being used, but I left it in case there was a use case I was unfamiliar with. Yes, as long as the trybots are happy you can remove it. > - Silly question, but how do I find OWNERS to add to my CL? Not silly at all, actually I find this the toughest part of landing CLs. I generally use the suggestions from git cl upload, but you can also use git cl owner after uploading your CL. There is also the Chromite Butler extension which modifies this UI (Rietveld) and suggests OWNERs (https://chrome.google.com/webstore/detail/chromite-butler/bhcnanendmgjjeghama...). Rietveld is being deprecated though, so I don't know whether the extension will work for a long time. > > Thanks! > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:734: void > MockHelpCenterUrl(InterstitialPage* interstitial_page) { > On 2017/06/27 00:36:21, estark wrote: > > nit: maybe add a brief comment above this method to explain why it exists > (e.g.: > > "Modify the interstitial to use a test server URL for its help center link.") > > Done. > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:749: const GURL > mock_help_center_url = https_server_.GetURL("/title1.html"); > On 2017/06/27 00:36:21, estark wrote: > > nit: const GURL& > > or just inline it below: > > SetBaseHelpCenterUrlForTesting(https_server_.GetURL(...)) > > Done. > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:820: > WebContents* interstitial_tab = > On 2017/06/27 00:36:21, estark wrote: > > nit: ASSERT_TRUE(interstitial_tab) after this line to make the test fail > nicely > > instead of crashing if for some reason the tab doesn't exist > > Done. > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:833: > AssertNoInterstitial(false); // Assert the interstitial is not present in > On 2017/06/27 00:36:21, estark wrote: > > nit: comment formatting is a little weird, you can probably just it on the > line > > above > > Done. > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:837: > WebContents* new_tab = browser()->tab_strip_model()->GetActiveWebContents(); > On 2017/06/27 00:36:21, estark wrote: > > nit: ASSERT_TRUE(new_tab) after this line to make the test fail nicely instead > > of crashing if for some reason the tab doesn't exist > > Done. > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1173: > WebContents* new_tab = browser()->tab_strip_model()->GetWebContentsAt(1); > On 2017/06/27 00:36:21, estark wrote: > > same nit about ASSERT_TRUE(new_tab) > > Done. > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:668: // Helper function for > TestInterstitialLinksOpenInNewTab. > On 2017/06/27 00:36:21, estark wrote: > > nit: perhaps add a sentence to explain why this is a helper method instead of > > just inlined below, for example: "Implemented as a test fixture method because > > the whole test fixture class is friended by SSLBlockingPage." > > Good idea! > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:3012: // Mocking out the help center URL > so that our test will hit the test server > On 2017/06/27 00:36:21, estark wrote: > > teeny-tiny nit: "Mocking" => "Mock" > > Done. > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:3019: const GURL mock_help_center_url = > https_server_.GetURL("/title1.html"); > On 2017/06/27 00:36:21, estark wrote: > > nit: const GURL& > > > > (otherwise it'll make an unnecessary copy) > > Acknowledged. > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:3019: const GURL mock_help_center_url = > https_server_.GetURL("/title1.html"); > On 2017/06/27 00:41:46, meacer wrote: > > On 2017/06/27 00:36:21, estark wrote: > > > nit: const GURL& > > > > > > (otherwise it'll make an unnecessary copy) > > > > I don't think you want a reference here, it'll be a ref to a temp variable so > > copying is correct. Consider this a "GURL mock_help_center_url" but with a > > const. > > Acknowledged. > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:3019: const GURL mock_help_center_url = > https_server_.GetURL("/title1.html"); > On 2017/06/27 00:45:46, estark wrote: > > On 2017/06/27 00:41:46, meacer wrote: > > > On 2017/06/27 00:36:21, estark wrote: > > > > nit: const GURL& > > > > > > > > (otherwise it'll make an unnecessary copy) > > > > > > I don't think you want a reference here, it'll be a ref to a temp variable > so > > > copying is correct. Consider this a "GURL mock_help_center_url" but with a > > > const. > > > > Oops, that's right, my bad! > > Acknowledged. > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:3035: WebContents* new_tab = > browser()->tab_strip_model()->GetActiveWebContents(); > On 2017/06/27 00:36:21, estark wrote: > > nit: ASSERT_TRUE(new_tab) after this line > > Done. > > https://codereview.chromium.org/2955503002/diff/220001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:3037: > EXPECT_EQ(mock_help_center_url.Resolve("answer/6098869"), new_tab->GetURL()); > On 2017/06/27 00:36:22, estark wrote: > > For a slightly less brittle test, you could do: > > > > EXPECT_EQ(mock_help_center_url.host(), new_tab->GetURL().host()); > > > > That'll check that the help center url was to the test server as expected, but > > someone can change the path without having to update the test. > > Good idea! Thanks. > > https://codereview.chromium.org/2955503002/diff/220001/components/security_in... > File components/security_interstitials/core/controller_client.h (right): > > https://codereview.chromium.org/2955503002/diff/220001/components/security_in... > components/security_interstitials/core/controller_client.h:92: virtual void > OpenUrlInCurrentTab(const GURL& url) = 0; > On 2017/06/27 00:36:22, estark wrote: > > Is this still used anywhere? Can we get rid of it? > > I don't think so! I left it in in case there was a use case I was unfamiliar > with. Should I remove it?
Thanks, Sasha! lgtm with one more tiny nit I <3 the Chromite butler extension for picking owners, I will be sad if it stops working. :( You can also just do it the old-fashioned way, i.e. look at the files you changed, randomly pick an OWNER from the nearest OWNERS file that you encounter as you walk up the directory tree, repeat until you've covered all the files you changed. https://codereview.chromium.org/2955503002/diff/240001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2955503002/diff/240001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:734: // Helper method for LearnMore test below. Implelemnted as a test fixture nit: typo, Implemented
sperigo@chromium.org changed reviewers: + felt@chromium.org, rohitrao@chromium.org
On 2017/06/27 17:48:33, estark wrote: > Thanks, Sasha! lgtm with one more tiny nit > > I <3 the Chromite butler extension for picking owners, I will be sad if it stops > working. :( You can also just do it the old-fashioned way, i.e. look at the > files you changed, randomly pick an OWNER from the nearest OWNERS file that you > encounter as you walk up the directory tree, repeat until you've covered all the > files you changed. > > https://codereview.chromium.org/2955503002/diff/240001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): > > https://codereview.chromium.org/2955503002/diff/240001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:734: // Helper > method for LearnMore test below. Implelemnted as a test fixture > nit: typo, Implemented Oh, and also, as Mustafa mentioned, please do try to remove the OpenUrlInCurrentTab method before you land; if anything breaks, the bots will let you know about it.
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
felt@chromium.org changed reviewers: + ntfschr@chromium.org
+nate, will this break the WebView interstitial links? We want those to open in the same WV not a new window
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ios/ lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/27 19:22:39, rohitrao (ping after 24h) wrote: > ios/ lgtm Yeah, this appears to break the links in WebView, since we don't really have tabs. Would it be reasonable to add a new field to SBErrorDisplayOptions to toggle this behavior (perhaps `bool open_links_in_new_tab`)?
On 2017/06/27 20:48:08, Nate Fischer wrote: > On 2017/06/27 19:22:39, rohitrao (ping after 24h) wrote: > > ios/ lgtm > > Yeah, this appears to break the links in WebView, since we don't really have > tabs. > > Would it be reasonable to add a new field to SBErrorDisplayOptions to toggle > this behavior (perhaps `bool open_links_in_new_tab`)? Doh. Nate, I don't suppose it would be possible to add a test that the links work properly on WebView interstitials? It's a bummer that the bots didn't catch this. :( Related question, have WebView interstitials shipped already? I wonder if we could land this as-is and then you or someone on your team could help adapt WV by toggling the link click behavior based on SBErrorDisplayOptions. This is a starter bug for Sasha (she's an intern) and I think learning how to build/test WebView might be a rather large tangent.
On 2017/06/27 21:17:35, estark wrote: > On 2017/06/27 20:48:08, Nate Fischer wrote: > > On 2017/06/27 19:22:39, rohitrao (ping after 24h) wrote: > > > ios/ lgtm > > > > Yeah, this appears to break the links in WebView, since we don't really have > > tabs. > > > > Would it be reasonable to add a new field to SBErrorDisplayOptions to toggle > > this behavior (perhaps `bool open_links_in_new_tab`)? > > Doh. Nate, I don't suppose it would be possible to add a test that the links > work properly on WebView interstitials? It's a bummer that the bots didn't catch > this. :( I completely agree. This involves adding some plumbing from Java to C++ to allow us to use JavaScript on interstitial pages. I've started work on this in crbug/733815. Once that's resolved, I'll make sure to add tests for the various in-page links. > Related question, have WebView interstitials shipped already? I wonder if we > could land this as-is and then you or someone on your team could help adapt WV > by toggling the link click behavior based on SBErrorDisplayOptions. This is a > starter bug for Sasha (she's an intern) and I think learning how to build/test > WebView might be a rather large tangent. We haven't officially shipped WebView Safe Browsing. Temporarily breaking things isn't ideal, but not catastrophic. I agree, teaching webview is out of scope. If you feel comfortable with her making the change here (it should just involve checking a boolean and calling OpenUrlInNewForegroundTab or OpenUrlInCurrentTab as appropriate), I can test out the patchset and provide guidance for the webview end. I also don't mind doing a follow-up CL after the fact.
On 2017/06/27 21:37:06, Nate Fischer wrote: > On 2017/06/27 21:17:35, estark wrote: > > On 2017/06/27 20:48:08, Nate Fischer wrote: > > > On 2017/06/27 19:22:39, rohitrao (ping after 24h) wrote: > > > > ios/ lgtm > > > > > > Yeah, this appears to break the links in WebView, since we don't really have > > > tabs. > > > > > > Would it be reasonable to add a new field to SBErrorDisplayOptions to toggle > > > this behavior (perhaps `bool open_links_in_new_tab`)? > > > > Doh. Nate, I don't suppose it would be possible to add a test that the links > > work properly on WebView interstitials? It's a bummer that the bots didn't > catch > > this. :( > > I completely agree. This involves adding some plumbing from Java to C++ to allow > us > to use JavaScript on interstitial pages. I've started work on this in > crbug/733815. > Once that's resolved, I'll make sure to add tests for the various in-page links. > > > Related question, have WebView interstitials shipped already? I wonder if we > > could land this as-is and then you or someone on your team could help adapt WV > > by toggling the link click behavior based on SBErrorDisplayOptions. This is a > > starter bug for Sasha (she's an intern) and I think learning how to build/test > > WebView might be a rather large tangent. > > We haven't officially shipped WebView Safe Browsing. Temporarily breaking things > isn't ideal, but not catastrophic. Wait, I thought it's shipping with 59? > > I agree, teaching webview is out of scope. If you feel comfortable with her > making the change here (it should just involve checking a boolean and calling > OpenUrlInNewForegroundTab or OpenUrlInCurrentTab as appropriate), I can test out > the patchset and provide guidance for the webview end. I also don't mind doing a > follow-up CL after the fact.
On 2017/06/27 21:46:07, felt wrote: > On 2017/06/27 21:37:06, Nate Fischer wrote: > > On 2017/06/27 21:17:35, estark wrote: > > > On 2017/06/27 20:48:08, Nate Fischer wrote: > > > > On 2017/06/27 19:22:39, rohitrao (ping after 24h) wrote: > > > > > ios/ lgtm > > > > > > > > Yeah, this appears to break the links in WebView, since we don't really > have > > > > tabs. > > > > > > > > Would it be reasonable to add a new field to SBErrorDisplayOptions to > toggle > > > > this behavior (perhaps `bool open_links_in_new_tab`)? > > > > > > Doh. Nate, I don't suppose it would be possible to add a test that the links > > > work properly on WebView interstitials? It's a bummer that the bots didn't > > catch > > > this. :( > > > > I completely agree. This involves adding some plumbing from Java to C++ to > allow > > us > > to use JavaScript on interstitial pages. I've started work on this in > > crbug/733815. > > Once that's resolved, I'll make sure to add tests for the various in-page > links. > > > > > Related question, have WebView interstitials shipped already? I wonder if we > > > could land this as-is and then you or someone on your team could help adapt > WV > > > by toggling the link click behavior based on SBErrorDisplayOptions. This is > a > > > starter bug for Sasha (she's an intern) and I think learning how to > build/test > > > WebView might be a rather large tangent. > > > > We haven't officially shipped WebView Safe Browsing. Temporarily breaking > things > > isn't ideal, but not catastrophic. > > Wait, I thought it's shipping with 59? I mean 60 > > > > > I agree, teaching webview is out of scope. If you feel comfortable with her > > making the change here (it should just involve checking a boolean and calling > > OpenUrlInNewForegroundTab or OpenUrlInCurrentTab as appropriate), I can test > out > > the patchset and provide guidance for the webview end. I also don't mind doing > a > > follow-up CL after the fact.
not lgtm Sorry to be the debbie downer, but I don't think it's OK to knowingly break functionality and plan to fix it later. However: an idea for a workaround. Perhaps Sasha can leave the existing OpenUrlInCurrentTab method alone. Then, as Nate suggests, we add an option that allows for choosing which one to use. Only have the browser code set the option. That will keep the WV behavior the same.
On 2017/06/27 21:49:09, felt wrote: > not lgtm > > Sorry to be the debbie downer, but I don't think it's OK to knowingly break > functionality and plan to fix it later. > > However: an idea for a workaround. Perhaps Sasha can leave the existing > OpenUrlInCurrentTab method alone. Then, as Nate suggests, we add an option that > allows for choosing which one to use. Only have the browser code set the option. > That will keep the WV behavior the same. (By which I mean: we have both the old method and the new method in parallel. The interstitial uses whichever one is appropriate per the option.)
On 2017/06/27 21:49:55, felt wrote: > On 2017/06/27 21:49:09, felt wrote: > > not lgtm > > > > Sorry to be the debbie downer, but I don't think it's OK to knowingly break > > functionality and plan to fix it later. > > > > However: an idea for a workaround. Perhaps Sasha can leave the existing > > OpenUrlInCurrentTab method alone. Then, as Nate suggests, we add an option > that > > allows for choosing which one to use. Only have the browser code set the > option. > > That will keep the WV behavior the same. > > (By which I mean: we have both the old method and the new method in parallel. > The interstitial uses whichever one is appropriate per the option.) That's fine with me. I'll have a CL up for review shortly.
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ok, CL has landed. Before calling OpenUrlInNewForegroundTab() you'll need to check should_open_links_in_new_tab().
Perfect! Thank you so much for your help with this. On 2017/06/28 at 03:11:15, ntfschr wrote: > Ok, CL has landed. Before calling OpenUrlInNewForegroundTab() you'll need to check should_open_links_in_new_tab().
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sperigo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/28 21:12:37, sperigo wrote: > Perfect! Thank you so much for your help with this. > > On 2017/06/28 at 03:11:15, ntfschr wrote: > > Ok, CL has landed. Before calling OpenUrlInNewForegroundTab() you'll need to > check should_open_links_in_new_tab(). WebView lgtm (verified locally). Thanks!
lgtm thx!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sperigo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org, meacer@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2955503002/#ps360001 (title: "Fix compilation error")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 360001, "attempt_start_ts": 1498697123727630,
"parent_rev": "0d07914a74851ab7f58a96cdc2ec2b563ffbcca8", "commit_rev":
"a22cb4d1b1f6afc95aba0eb04246c13fe178697e"}
Message was sent while issue was closed.
Description was changed from ========== Make interstitial links open in a new tab BUG=717616 TEST=Build and run Chromium. Visit the various interstitials on https://badssl.com and make sure that the various info links open in a new foreground tab. There's also a browser test (TestInterstitialLinksOpenInNewTab) in SSLUITests. ========== to ========== Make interstitial links open in a new tab BUG=717616 https: //badssl.com and make sure that the various info links open in a Review-Url: https://codereview.chromium.org/2955503002 Cr-Commit-Position: refs/heads/master@{#483231} Committed: https://chromium.googlesource.com/chromium/src/+/a22cb4d1b1f6afc95aba0eb04246... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001) as https://chromium.googlesource.com/chromium/src/+/a22cb4d1b1f6afc95aba0eb04246... |
