Disable 'duplicate tab' functionality on interstitial pages

Disable 'duplicate tab' functionality on interstitial pages

Currently, duplicating a tab that is showing an interstitial page will
duplicate the last committed navigation entry, which will likely not be
what the user expects. This CL simply disables the Duplicate Tab
functionality on interstitial pages. This matches the behavior when
there is no previously committed navigation entry before an
interstitial. (For example, navigate to https://badssl.com,
right-click on "expired", and open link in new tab. Note that Duplicate
Tab is disabled.)

BUG=310812
TEST=Visit https://badssl.com. Click "expired" and observe an SSL
page. Right-click on the tab at the top and observe that "Duplicate Tab"
is disabled.

Committed: https://crrev.com/9ecf2edac44ee510b7ec81267e4b6610e7c0a812
Cr-Commit-Position: refs/heads/master@{#332614}

[estark, 2015-06-02 17:17:37]

[email protected] changed reviewers:
+ [email protected]

[estark, 2015-06-02 17:17:37]

msw, could you please take a look?

In this CL I'm following meacer's (cc'ed) idea on crbug.com/310812 of just
disabling Duplicate Tab on interstitial pages, since it doesn't do what the user
expects.

If we want to try to fix Duplicate Tab instead of disabling it, I can look into
that instead. I think the root cause is that cloning a NavigationEntry skips
over interstitial pages:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
But I'm not sure if changing that behavior is an option or what it would affect.
(I can ask creis if we think it's worth looking into.)

[meacer, 2015-06-02 17:48:46]

On 2015/06/02 17:17:37, estark wrote:
> msw, could you please take a look?
> 
> In this CL I'm following meacer's (cc'ed) idea on crbug.com/310812 of just
> disabling Duplicate Tab on interstitial pages, since it doesn't do what the
user
> expects.
> 
> If we want to try to fix Duplicate Tab instead of disabling it, I can look
into
> that instead. I think the root cause is that cloning a NavigationEntry skips
> over interstitial pages:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> But I'm not sure if changing that behavior is an option or what it would
affect.
> (I can ask creis if we think it's worth looking into.)

As a side note: duplicating an SSL interstitial doesn't work in the first place,
as the new tab will just hang. This is a very old bug: https://crbug.com/6697

[msw, 2015-06-02 23:59:34]

https://codereview.chromium.org/1152963007/diff/1/chrome/browser/ui/browser_b...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1152963007/diff/1/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_browsertest.cc:1897:
IN_PROC_BROWSER_TEST_F(BrowserTest, InterstitialCommandDisable) {
Can you add IDC_DUPLICATE_TAB to the set of commands tested here?

https://codereview.chromium.org/1152963007/diff/1/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_browsertest.cc:2745:
IN_PROC_BROWSER_TEST_F(BrowserTest, DuplicateTabDisabledOnInterstitials) {
Hmm, odd that there are no other duplicate tab tests. Can you make this a more
general tests "DuplicateTab" that also covers the interstitial case?

https://codereview.chromium.org/1152963007/diff/1/chrome/browser/ui/browser_c...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/1152963007/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_commands.cc:637: WebContents* contents =
browser->tab_strip_model()->GetActiveWebContents();
Just return CanDuplicateTabAt(browser,
browser->tab_strip_model()->active_index());

https://codereview.chromium.org/1152963007/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_commands.cc:700: content::NavigationController& nc =
Weird, why doesn't this function look more like CanDuplicateTab? Like:
  WebContents* contents = browser->tab_strip_model()->GetWebContentsAt(index);
  return contents && !contents->ShowingInterstitialPage() &&
         contents->GetController().GetLastCommittedEntry();

[estark, 2015-06-03 00:56:20]

Thanks msw.

https://codereview.chromium.org/1152963007/diff/1/chrome/browser/ui/browser_b...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1152963007/diff/1/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_browsertest.cc:1897:
IN_PROC_BROWSER_TEST_F(BrowserTest, InterstitialCommandDisable) {
On 2015/06/02 23:59:34, msw wrote:
> Can you add IDC_DUPLICATE_TAB to the set of commands tested here?

Done.

https://codereview.chromium.org/1152963007/diff/1/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_browsertest.cc:2745:
IN_PROC_BROWSER_TEST_F(BrowserTest, DuplicateTabDisabledOnInterstitials) {
On 2015/06/02 23:59:34, msw wrote:
> Hmm, odd that there are no other duplicate tab tests. Can you make this a more
> general tests "DuplicateTab" that also covers the interstitial case?

Done, I think. Let me know if what I've done matches what you had in mind. (I
just added a bit at the end that destroys the interstitial and then checks
CanDuplicateTab on the non-interstitial page.)

https://codereview.chromium.org/1152963007/diff/1/chrome/browser/ui/browser_c...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/1152963007/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_commands.cc:637: WebContents* contents =
browser->tab_strip_model()->GetActiveWebContents();
On 2015/06/02 23:59:34, msw wrote:
> Just return CanDuplicateTabAt(browser,
> browser->tab_strip_model()->active_index());

Done.

https://codereview.chromium.org/1152963007/diff/1/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_commands.cc:700: content::NavigationController& nc =
On 2015/06/02 23:59:34, msw wrote:
> Weird, why doesn't this function look more like CanDuplicateTab? Like:
>   WebContents* contents = browser->tab_strip_model()->GetWebContentsAt(index);
>   return contents && !contents->ShowingInterstitialPage() &&
>          contents->GetController().GetLastCommittedEntry();

Done. (Not sure why it was different in the first place.)

[msw, 2015-06-03 01:11:06]

lgtm with nits and an optional test expansion idea.

https://codereview.chromium.org/1152963007/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1152963007/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2746: // Verify that the "Duplicate
tab" command is disabled on interstitial
nit: optionally move this comment to ~line 2768.

https://codereview.chromium.org/1152963007/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2748:
IN_PROC_BROWSER_TEST_F(BrowserTest, CanDuplicateTab) {
Not bad, but I wonder if it should also test some other stuff (duplicating a
non-active tab), but that can be added to a separate test or left to do here
another time.

https://codereview.chromium.org/1152963007/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2752:
ui_test_utils::NavigateToURL(browser(), url);
nit: can you use about:blank or similar to avoid the test_server, etc. above?

[estark, 2015-06-03 14:26:44]

https://codereview.chromium.org/1152963007/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1152963007/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2746: // Verify that the "Duplicate
tab" command is disabled on interstitial
On 2015/06/03 01:11:06, msw wrote:
> nit: optionally move this comment to ~line 2768.

Done.

https://codereview.chromium.org/1152963007/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2748:
IN_PROC_BROWSER_TEST_F(BrowserTest, CanDuplicateTab) {
On 2015/06/03 01:11:06, msw wrote:
> Not bad, but I wonder if it should also test some other stuff (duplicating a
> non-active tab), but that can be added to a separate test or left to do here
> another time.

Added some basic testing for non-active tabs.

https://codereview.chromium.org/1152963007/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2752:
ui_test_utils::NavigateToURL(browser(), url);
On 2015/06/03 01:11:06, msw wrote:
> nit: can you use about:blank or similar to avoid the test_server, etc. above?

Switched to a test file:// URL instead, which is what some of the other tests in
this file do too.

[estark, 2015-06-03 14:27:31]

The CQ bit was checked by [email protected]

[estark, 2015-06-03 14:27:32]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected]
Link to the patchset: https://codereview.chromium.org/1152963007/#ps60001
(title: "style fix")

[commit-bot: I haz the power, 2015-06-03 14:28:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1152963007/60001

[commit-bot: I haz the power, 2015-06-03 15:25:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-03 15:26:59]

Patchset 4 (id:??) landed as
https://crrev.com/9ecf2edac44ee510b7ec81267e4b6610e7c0a812
Cr-Commit-Position: refs/heads/master@{#332614}