Don't show tabbed options page in OTR browser window.

Don't show tabbed options page or bookmarks manager in OTR browser window.

Instead, always show it in a normal window (creating one if necessary).

The change is made in BrowserNavigator because that is the control flow bottleneck.

BUG=61813
TEST=manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66189

[Peter Kasting, 2010-11-09 20:33:31]

This seems like the brutally hard way.

Why don't we just make the actual API functions the options content calls always
get the "original profile" (i.e. the non-incognito one) to serve its responses? 
Then you don't need any navigation changes, there's no opening new windows,
everything "just works" whether the tab appears to be in an incognito window or
not.

[Evan Stade, 2010-11-09 20:35:20]

ask Glen

[Peter Kasting, 2010-11-09 20:37:08]

On 2010/11/09 20:35:20, Evan Stade wrote:
> ask Glen

I think Glen is just trying to avoid bustage the easiest way he knows how.  This
way seems easier to me.  I'll chat with him when he's around.  It would be nice
to know if there's a technical reason why my suggestion wouldn't work, and work
much more simply, though.

[Evan Stade, 2010-11-09 20:42:30]

On Tue, Nov 9, 2010 at 12:37 PM, <pkasting@chromium.org> wrote:

> On 2010/11/09 20:35:20, Evan Stade wrote:
>
>> ask Glen
>>
>
> I think Glen is just trying to avoid bustage the easiest way he knows how.
>  This
> way seems easier to me.  I'll chat with him when he's around.  It would be
> nice
> to know if there's a technical reason why my suggestion wouldn't work, and
> work
> much more simply, though.


no, that solution would be very easy

-- Evan Stade


>
>
> http://codereview.chromium.org/4655002/show
>

[Glen Murphy, 2010-11-10 03:31:35]

Let's talk about this tomorrow - I feel that doing this in an
incognito tab breaks some expectations about incognito, so think that
opening a new window is the lesser nasty, but I'm willing to be talked
down from that ledge.


On Tue, Nov 9, 2010 at 12:37 PM,  <pkasting@chromium.org> wrote:
> On 2010/11/09 20:35:20, Evan Stade wrote:
>>
>> ask Glen
>
> I think Glen is just trying to avoid bustage the easiest way he knows how.
>  This
> way seems easier to me.  I'll chat with him when he's around.  It would be
> nice
> to know if there's a technical reason why my suggestion wouldn't work, and
> work
> much more simply, though.
>
> http://codereview.chromium.org/4655002/show
>

[Peter Kasting, 2010-11-10 20:58:08]

We talked and no one was happy with any solution.

Let's proceed with the "punt users to a regular window" direction.  I feel like
we should do it for the bookmarks manager while we're at it but if that's really
hard I don't care too much.

[Evan Stade, 2010-11-11 00:13:51]

updated to handle the bookmark manager as well. There are still some rare cases
where you can get chrome://bookmarks in an incognito window (see [5] in the
description), but that shouldn't break anything.

[Evan Stade, 2010-11-12 19:40:30]

ping

[Peter Kasting, 2010-11-12 19:49:01]

This looks fine to me but I don't understand navigation very well.  You may want
ben or sadrul (who is working on a big BrowserNavigator-related patch) to look
at this.

http://codereview.chromium.org/4655002/diff/17001/chrome/browser/gtk/tabs/tab...
File chrome/browser/gtk/tabs/tab_strip_gtk.cc (right):

http://codereview.chromium.org/4655002/diff/17001/chrome/browser/gtk/tabs/tab...
chrome/browser/gtk/tabs/tab_strip_gtk.cc:1979: if (!browser) {
Nit: Just DCHECK(browser) and keep going.  ("Don't handle DCHECK failures")

http://codereview.chromium.org/4655002/diff/17001/chrome/browser/gtk/tabs/tab...
chrome/browser/gtk/tabs/tab_strip_gtk.cc:1984:
browser->AddSelectedTabWithURL(url, PageTransition::TYPED);
This should actually be LINK rather than TYPED (the old code was wrong).

http://codereview.chromium.org/4655002/diff/17001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/4655002/diff/17001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:108: if (!params->target_contents &&
(isSettingsPage || isBookmarksManager)) {
Nit: It seems like it'd be simpler (and less redundant) to just do:

  if (!params->target_contents &&
      (params->url.scheme() == chrome::kChromeUIScheme) &&
      ((params->url.host() == chrome::kChromeUISettingsHost) ||
       (params->url.host() == chrome::kChromeUIBookmarksHost))) {

That is just as readable to me and three lines shorter.

[Evan Stade, 2010-11-12 20:47:11]

+sadrul, ben

http://codereview.chromium.org/4655002/diff/17001/chrome/browser/gtk/tabs/tab...
File chrome/browser/gtk/tabs/tab_strip_gtk.cc (right):

http://codereview.chromium.org/4655002/diff/17001/chrome/browser/gtk/tabs/tab...
chrome/browser/gtk/tabs/tab_strip_gtk.cc:1979: if (!browser) {
On 2010/11/12 19:49:01, Peter Kasting wrote:
> Nit: Just DCHECK(browser) and keep going.  ("Don't handle DCHECK failures")

Done.

http://codereview.chromium.org/4655002/diff/17001/chrome/browser/gtk/tabs/tab...
chrome/browser/gtk/tabs/tab_strip_gtk.cc:1984:
browser->AddSelectedTabWithURL(url, PageTransition::TYPED);
On 2010/11/12 19:49:01, Peter Kasting wrote:
> This should actually be LINK rather than TYPED (the old code was wrong).

Done.

http://codereview.chromium.org/4655002/diff/17001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/4655002/diff/17001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:108: if (!params->target_contents &&
(isSettingsPage || isBookmarksManager)) {
ok

[brettw, 2010-11-12 21:26:43]

I would really prefer a function "ShowOptionsOrSomething" that handles all this
window creation stuff and adding a new tab navigated to the right place. The
complexity should be added to there and callers should use it rather than
forcing Browser to know about some magic URLs and fix them up.

I bet there is functionality in the call sites to set up the navigation which
would be nicely factored into this new function, so it will end up making all
the callsites cleaner as well.

[sadrul, 2010-11-12 21:46:53]

I am only a little familiar with Navigate, so I just there.

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:113: params->disposition =
NEW_FOREGROUND_TAB;
I think it should be SINGLETON_TAB here? In addition, setting params->profile,
and unsetting params->browser may be enough?

[Evan Stade, 2010-11-12 23:53:41]

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:113: params->disposition =
NEW_FOREGROUND_TAB;
On 2010/11/12 21:46:54, sadrul wrote:
> I think it should be SINGLETON_TAB here?

oh yea, I think you're right.

> In addition, setting params->profile,
> and unsetting params->browser may be enough?

all the lines here are necessary. It might work to replace profile or browser
with NULL, but that doesn't shorten anything or make it clearer.

[Evan Stade, 2010-11-12 23:55:28]

On 2010/11/12 21:26:43, brettw wrote:
> I would really prefer a function "ShowOptionsOrSomething" that handles all
this
> window creation stuff and adding a new tab navigated to the right place. The
> complexity should be added to there and callers should use it rather than
> forcing Browser to know about some magic URLs and fix them up.
> 
> I bet there is functionality in the call sites to set up the navigation which
> would be nicely factored into this new function, so it will end up making all
> the callsites cleaner as well.

there is such a function. It's used when the wrench menu item is clicked. It is
not used when chrome://settings is typed directly into the omnibox, or some
other cases (see CL description).

[brettw, 2010-11-14 23:57:20]

I guess I don't have any high-level complaints. Will be AFK so don't wait for
another pass from me.

[Ben Goodger (Google), 2010-11-15 18:21:04]

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/options_ui.cc (right):

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/options_ui.cc:258: return;
Why are you also handling this here when you did so in Navigate?

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:111: Browser* browser =
Browser::GetOrCreateTabbedBrowser(profile);
nix this line

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:115: params->browser = browser;
params->browser = Browser::GetOrCreateTabbedBrowser(profile);

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:116: params->tabstrip_add_types =
TabStripModel::ADD_SELECTED;
This should be inferred from NEW_FOREGROUND_TAB

[sadrul, 2010-11-15 18:40:59]

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:116: params->tabstrip_add_types =
TabStripModel::ADD_SELECTED;
On 2010/11/15 18:21:04, Ben Goodger wrote:
> This should be inferred from NEW_FOREGROUND_TAB

I believe it should be SINGLETON_TAB (as noted earlier), and
http://codereview.chromium.org/4647007/ would add ADD_SELECTED for that.

[Evan Stade, 2010-11-15 18:51:46]

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/options_ui.cc (right):

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/options_ui.cc:258: return;
On 2010/11/15 18:21:04, Ben Goodger wrote:
> Why are you also handling this here when you did so in Navigate?

see CL description

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:115: params->browser = browser;
On 2010/11/15 18:21:04, Ben Goodger wrote:
> params->browser = Browser::GetOrCreateTabbedBrowser(profile);

Done.

[Ben Goodger (Google), 2010-11-15 18:57:19]

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/options_ui.cc (right):

http://codereview.chromium.org/4655002/diff/23001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/options_ui.cc:258: return;
Can you make the drag and drop handling code use browser_navigator too then
instead of doing this?

[Evan Stade, 2010-11-15 21:51:10]

On 2010/11/15 18:57:19, Ben Goodger wrote:
>
http://codereview.chromium.org/4655002/diff/23001/chrome/browser/dom_ui/optio...
> File chrome/browser/dom_ui/options/options_ui.cc (right):
> 
>
http://codereview.chromium.org/4655002/diff/23001/chrome/browser/dom_ui/optio...
> chrome/browser/dom_ui/options/options_ui.cc:258: return;
> Can you make the drag and drop handling code use browser_navigator too then
> instead of doing this?

ok, I've fixed the drag case on Linux. But my worry is that there are other ways
to trick the incognito window into loading the settings page. It's hard to audit
all uses of LoadURL because there are many of them, but not all could be used to
load the settings page. So instead of having a subtly broken settings tab in
some cases, this seems better.

[Ben Goodger (Google), 2010-11-15 21:54:56]

On 2010/11/15 21:51:10, Evan Stade wrote:
> load the settings page. So instead of having a subtly broken settings tab in
> some cases, this seems better.

My rebuttal is that those other places should be using browser_navigator too.
Perhaps so we can identify them quickly you could replace the code here with a
NOTREACHED or something?

[Evan Stade, 2010-11-15 21:57:51]

done

[Ben Goodger (Google), 2010-11-15 22:10:44]

Thanks! LGTM.