Enable new session crashed bubble UI by Finch experiment.

Enable new session crashed bubble UI by Finch experiment.

BUG=377813
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273032

[Alexei Svitkine (slow), 2014-05-15 19:02:34]

https://codereview.chromium.org/284113006/diff/40001/chrome/browser/ui/startu...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/284113006/diff/40001/chrome/browser/ui/startu...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:858:
ShowSessionCrashedBubble(browser)))
Hmm, I think it would be better to move this logic to
ShowSessionCrashedBubble(), given that we have a single implementation of it.

That way, we won't put users into the active Finch group if they're running a
version of Chrome that doesn't support it.

I think you should also add a --disable flag, so that when the behaviour is
enabled via Finch, one can both turn it on or off explicitly.

(Move both the flag handling and the Finch check into the function.)

[yao, 2014-05-15 21:19:38]

https://codereview.chromium.org/284113006/diff/40001/chrome/browser/ui/startu...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/284113006/diff/40001/chrome/browser/ui/startu...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:858:
ShowSessionCrashedBubble(browser)))
On 2014/05/15 19:02:35, Alexei Svitkine wrote:
> Hmm, I think it would be better to move this logic to
> ShowSessionCrashedBubble(), given that we have a single implementation of it.
> 
> That way, we won't put users into the active Finch group if they're running a
> version of Chrome that doesn't support it.
> 
> I think you should also add a --disable flag, so that when the behaviour is
> enabled via Finch, one can both turn it on or off explicitly.
> 
> (Move both the flag handling and the Finch check into the function.)

Done.

[Alexei Svitkine (slow), 2014-05-15 22:52:26]

https://codereview.chromium.org/284113006/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/284113006/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:355: const
base::CommandLine& command_line) {
I don't think you need this param. You can just do
CommandLine::ForCurrentProcess().

[yao, 2014-05-16 00:48:25]

https://codereview.chromium.org/284113006/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/284113006/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:355: const
base::CommandLine& command_line) {
On 2014/05/15 22:52:27, Alexei Svitkine wrote:
> I don't think you need this param. You can just do
> CommandLine::ForCurrentProcess().

I can't until the other CL (with posttask scheduled on thread FILE) is landed.
Otherwise, it will crash by calling
GoogleUpdateSettings::GetCollectStatsConsent(). 

(Currently there's no crash because incorrect command_line is used and infobar
will be shown instead when user switches to a new profile).

[Alexei Svitkine (slow), 2014-05-16 19:01:39]

https://codereview.chromium.org/284113006/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/284113006/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:355: const
base::CommandLine& command_line) {
On 2014/05/16 00:48:25, yao wrote:
> On 2014/05/15 22:52:27, Alexei Svitkine wrote:
> > I don't think you need this param. You can just do
> > CommandLine::ForCurrentProcess().
> 
> I can't until the other CL (with posttask scheduled on thread FILE) is landed.
> Otherwise, it will crash by calling
> GoogleUpdateSettings::GetCollectStatsConsent(). 
> 
> (Currently there's no crash because incorrect command_line is used and infobar
> will be shown instead when user switches to a new profile). 

I think you can just wait for the other CL to land first before landing this one
- so you can change this CL to use the current process command line.

https://codereview.chromium.org/284113006/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:359: return false;
No need for else or {}'s if you're doing a return in the if.

https://codereview.chromium.org/284113006/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:360: } else if
(base::FieldTrialList::FindFullName(kEnableBubbleUIFinchName) ==
The best practice for this is to get the group first (so the field trial gets
activated) then check the flags, then check the group.

[yao, 2014-05-26 21:36:50]

https://codereview.chromium.org/284113006/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/284113006/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:355: const
base::CommandLine& command_line) {
On 2014/05/16 19:01:40, Alexei Svitkine wrote:
> On 2014/05/16 00:48:25, yao wrote:
> > On 2014/05/15 22:52:27, Alexei Svitkine wrote:
> > > I don't think you need this param. You can just do
> > > CommandLine::ForCurrentProcess().
> > 
> > I can't until the other CL (with posttask scheduled on thread FILE) is
landed.
> > Otherwise, it will crash by calling
> > GoogleUpdateSettings::GetCollectStatsConsent(). 
> > 
> > (Currently there's no crash because incorrect command_line is used and
infobar
> > will be shown instead when user switches to a new profile). 
> 
> I think you can just wait for the other CL to land first before landing this
one
> - so you can change this CL to use the current process command line.

Done.

https://codereview.chromium.org/284113006/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:359: return false;
On 2014/05/16 19:01:40, Alexei Svitkine wrote:
> No need for else or {}'s if you're doing a return in the if.

Done.

https://codereview.chromium.org/284113006/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:360: } else if
(base::FieldTrialList::FindFullName(kEnableBubbleUIFinchName) ==
On 2014/05/16 19:01:40, Alexei Svitkine wrote:
> The best practice for this is to get the group first (so the field trial gets
> activated) then check the flags, then check the group.

Done.

[Alexei Svitkine (slow), 2014-05-27 15:42:46]

https://codereview.chromium.org/284113006/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/284113006/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:440: return false;
Please follow the pattern suggested on the "Best Practice" section on
go/finch-and-flags. Thanks!

[yao, 2014-05-27 16:42:01]

https://codereview.chromium.org/284113006/diff/160001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/284113006/diff/160001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:440: return false;
On 2014/05/27 15:42:46, Alexei Svitkine wrote:
> Please follow the pattern suggested on the "Best Practice" section on
> go/finch-and-flags. Thanks!

Done.

[Alexei Svitkine (slow), 2014-05-27 16:53:10]

LGTM % nits

https://codereview.chromium.org/284113006/diff/180001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/284113006/diff/180001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:842:
!ShowSessionCrashedBubble(browser))
Nit: Add {}'s

https://codereview.chromium.org/284113006/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/284113006/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:88: if
(CommandLine::ForCurrentProcess()->HasSwitch(
Nit: extract to a local var so you don't need to wrap the ifs:

  const base::CommandLine& command_line = *CommandLine::ForCurrentProcess();
  if (command_line->HasSwitch(switches::kDisableSessionCrashedBubble))
    return false;
  ...

[yao, 2014-05-27 17:22:48]

+sky for owner's review.

https://codereview.chromium.org/284113006/diff/180001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/284113006/diff/180001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:842:
!ShowSessionCrashedBubble(browser))
On 2014/05/27 16:53:10, Alexei Svitkine wrote:
> Nit: Add {}'s

Done.

https://codereview.chromium.org/284113006/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/session_crashed_bubble_view.cc (right):

https://codereview.chromium.org/284113006/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/session_crashed_bubble_view.cc:88: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2014/05/27 16:53:10, Alexei Svitkine wrote:
> Nit: extract to a local var so you don't need to wrap the ifs:
> 
>   const base::CommandLine& command_line = *CommandLine::ForCurrentProcess();
>   if (command_line->HasSwitch(switches::kDisableSessionCrashedBubble))
>     return false;
>   ...

Done.

[sky, 2014-05-27 17:35:16]

LGTM

[yao, 2014-05-27 18:38:26]

The CQ bit was checked by yiyaoliu@chromium.org

[commit-bot: I haz the power, 2014-05-27 18:39:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/284113006/200001

[commit-bot: I haz the power, 2014-05-27 20:10:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-05-27 21:16:20]

Change committed as 273032