Fix crash in SyncSetupWizard::IsVisible()

Fix crash in SyncSetupWizard::IsVisible()

BUG=103029
TEST=Open an incognito window. Close all other windows. Navigate to chrome://syncpromo. Should not crash.


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

[binji, 2011-11-17 01:23:51]

sky: browser_navigator
sail: sync_promo

Navigating to chrome://syncpromo in an incognito window will now open in a
non-incognito window. I think that makes sense.

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:81: if (params->target_contents != NULL
||
I think it is clearer to early out here.

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:637: return !(url.scheme() ==
chrome::kChromeUIScheme &&
Reversed the logic here because the name was inconsistent with the logic.

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.h (right):

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.h:194: // Returns true if the url is allowed
to open in incognito window.
This comment was inconsistent with the function name.

Also, it seems like this exception list could easily become out of sync with the
code, so I removed it.

[sail, 2011-11-17 01:30:31]

Not sure about the sync setup code. Maybe atwilson or jhawkins might be better?

[binji, 2011-11-17 01:42:31]

On 2011/11/17 01:30:31, sail wrote:
> Not sure about the sync setup code. Maybe atwilson or jhawkins might be
better?

+jhawkins

sail, do you have an opinion on showing chrome://syncpromo in a non-incognito
window? The only alternative I can think of is to just not display anything if
the user navigates there.

[sail, 2011-11-17 01:43:42]

> sail, do you have an opinion on showing chrome://syncpromo in a non-incognito
> window? The only alternative I can think of is to just not display anything if
> the user navigates there.

Your approach seems really good. I think doing anything is fine since we don't
support manually navigating to that URL.

[James Hawkins, 2011-11-17 02:23:52]

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:75: // in a normal (not incognito)
window. Guest session is an exception.
s/normal (non incognito)/non-incognito/

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:75: // in a normal (not incognito)
window. Guest session is an exception.
While you're here, can you clarify the guest session comment?

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:81: if (params->target_contents != NULL
||
On 2011/11/17 01:23:51, binji wrote:
> I think it is clearer to early out here.

Always and forever.

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:637: return !(url.scheme() ==
chrome::kChromeUIScheme &&
On 2011/11/17 01:23:51, binji wrote:
> Reversed the logic here because the name was inconsistent with the logic.

Don't you need to reverse the call sites too?

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/webui/sync_se...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_handler.cc:706: if (!service)
When is |service| NULL?

[sky, 2011-11-17 04:56:17]

browser_navigator LGTM

[sky, 2011-11-17 04:56:35]

One additional comment. How about adding a test for coverage of some of this?

[binji, 2011-11-17 21:02:08]

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:75: // in a normal (not incognito)
window. Guest session is an exception.
On 2011/11/17 02:23:53, James Hawkins wrote:
> s/normal (non incognito)/non-incognito/

Done.

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:75: // in a normal (not incognito)
window. Guest session is an exception.
On 2011/11/17 02:23:53, James Hawkins wrote:
> While you're here, can you clarify the guest session comment?

Done.

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:637: return !(url.scheme() ==
chrome::kChromeUIScheme &&
On 2011/11/17 02:23:53, James Hawkins wrote:
> On 2011/11/17 01:23:51, binji wrote:
> > Reversed the logic here because the name was inconsistent with the logic.
> 
> Don't you need to reverse the call sites too?

Good catch.

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/webui/sync_se...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/8588013/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_handler.cc:706: if (!service)
On 2011/11/17 02:23:53, James Hawkins wrote:
> When is |service| NULL?

When using an incognito profile.

http://codereview.chromium.org/8588013/diff/5/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator_browsertest.cc (right):

http://codereview.chromium.org/8588013/diff/5/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator_browsertest.cc:946:
RunUseNonIncognitoWindowTest(GetSettingsURL());
Is there a better way to group all of these similar tests together? (is that
even desired?) If they are in one test I'd have to reset the browser after each
call to RunUse...Test().

[binji, 2011-11-22 00:54:49]

James, any further comments?

[James Hawkins, 2011-11-22 00:59:27]

LGTM with nit.

http://codereview.chromium.org/8588013/diff/5/chrome/browser/ui/browser_navig...
File chrome/browser/ui/browser_navigator.cc (right):

http://codereview.chromium.org/8588013/diff/5/chrome/browser/ui/browser_navig...
chrome/browser/ui/browser_navigator.cc:82: Profile::IsGuestSession())
Multi-line logic requires braces on the if block.

[commit-bot: I haz the power, 2011-11-28 21:41:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/binji@chromium.org/8588013/12001

[commit-bot: I haz the power, 2011-11-28 23:14:37]

Try job failure for 8588013-12001 (retry) on win_rel for step
"interactive_ui_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-29 21:37:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/binji@chromium.org/8588013/12001

[commit-bot: I haz the power, 2011-11-29 23:17:21]

Change committed as 112061