Suppressing First Run Bubble when chrome://welcome is displayed.

Suppressing First Run Bubble when chrome://welcome is displayed.

BUG=656178
Committed: https://crrev.com/14bebe60982a8b31a780712d523eaf4ca2103d7a
Cr-Commit-Position: refs/heads/master@{#426871}

[tmartino, 2016-10-20 21:09:39]

tmartino@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2016-10-21 15:40:22]

https://codereview.chromium.org/2441823002/diff/1/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2441823002/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:333: GURL
deprecated_welcome_page(l10n_util::GetStringUTF8(IDS_WELCOME_PAGE_URL));
Do we still show the deprecated one? Is this because new one is behind Finch or
something?

https://codereview.chromium.org/2441823002/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:338: return true;
nit: {} for multi-line conditional

https://codereview.chromium.org/2441823002/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:339: GURL
welcome_page(chrome::kChromeUIWelcomeURL);
const (or inline below? unless you think variable name helps readability?)

https://codereview.chromium.org/2441823002/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:340: return contents->GetURL() ==
welcome_page;
Why doesn't this need to check OriginalRequestURL like the other one?

[tmartino, 2016-10-21 17:41:40]

https://codereview.chromium.org/2441823002/diff/1/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2441823002/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:333: GURL
deprecated_welcome_page(l10n_util::GetStringUTF8(IDS_WELCOME_PAGE_URL));
On 2016/10/21 at 15:40:22, gab wrote:
> Do we still show the deprecated one? Is this because new one is behind Finch
or something?

That's correct. Added a TODO with the bug #.

https://codereview.chromium.org/2441823002/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:338: return true;
On 2016/10/21 at 15:40:22, gab wrote:
> nit: {} for multi-line conditional

Done

https://codereview.chromium.org/2441823002/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:339: GURL
welcome_page(chrome::kChromeUIWelcomeURL);
On 2016/10/21 at 15:40:22, gab wrote:
> const (or inline below? unless you think variable name helps readability?)

Yeah, I wanted to use the variable name for readability (esp. because it
contrasts against deprecated_welcome_page above).

https://codereview.chromium.org/2441823002/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:340: return contents->GetURL() ==
welcome_page;
On 2016/10/21 at 15:40:22, gab wrote:
> Why doesn't this need to check OriginalRequestURL like the other one?

The comment above indicates that the OriginalRequestURL is meant to handle an
expected redirect. We don't expect a redirect to happen for chrome://welcome.

Thinking about this, though, I did make one additional change here to strip the
path from the URL. We expect to use URL params in the future.

[gab, 2016-10-21 18:02:56]

lgtm

[tmartino, 2016-10-21 18:03:45]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 18:04:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 19:47:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 19:47:01]

Dry run: This issue passed the CQ dry run.

[tmartino, 2016-10-21 19:47:30]

The CQ bit was checked by tmartino@chromium.org

[commit-bot: I haz the power, 2016-10-21 19:48:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 20:07:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 20:09:26]

Description was changed from

==========
Suppressing First Run Bubble when chrome://welcome is displayed.

BUG=656178
==========

to

==========
Suppressing First Run Bubble when chrome://welcome is displayed.

BUG=656178

Committed: https://crrev.com/14bebe60982a8b31a780712d523eaf4ca2103d7a
Cr-Commit-Position: refs/heads/master@{#426871}
==========

[commit-bot: I haz the power, 2016-10-21 20:09:27]

Patchset 2 (id:??) landed as
https://crrev.com/14bebe60982a8b31a780712d523eaf4ca2103d7a
Cr-Commit-Position: refs/heads/master@{#426871}