Tech Debt Repayment for StartupBrowserCreatorImpl Refactor

Cleaning up a number of outstanding comments from refactor of StartupBrowserCreatorImpl. Specifically:

- Adds SessionRestore::Behavior typedef, replacing uint32_t as arg

- Renames |is_ephemeral_profile| to |is_incognito_or_guest|, as concerns were raised that “ephemeral profile” is a specific, loaded term.

- Changes DetermineBrowserOpenBehavior to use bitmask.

- Adds unit tests for DetermineBrowserOpenBehavior.

- Removes extraneous TODOs.

- Adds browser tests testing welcome pages.

BUG=688574
Review-Url: https://codereview.chromium.org/2469363002
Cr-Commit-Position: refs/heads/master@{#449405}
Committed: https://chromium.googlesource.com/chromium/src/+/e2288ebbd5ec887e98b1140b6b8c12d56514c9aa

[tmartino, 2016-11-02 17:36:22]

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

[Peter Kasting, 2016-11-02 17:54:51]

https://codereview.chromium.org/2469363002/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/2469363002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:757: // does not
normally restore, when a Chrome process is already running.
I'm concerned with "this shouldn't happen" (which to me reads like "DCHECK"/code
bug) in conjunction with "has been observed".

It seems like either the current behavior is a weird edge case but correct, in
which case we shouldn't say things like "fallback" and "shouldn't happen" and
should just handle this, or else this is a real bug (maybe in code elsewhere?),
in which case we should fix the bug so we don't get here with no tabs.

I would imagine that stepping through the callstack in the case you describe
might illuminate this.

[tmartino, 2016-11-02 18:04:40]

https://codereview.chromium.org/2469363002/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/2469363002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:757: // does not
normally restore, when a Chrome process is already running.
On 2016/11/02 at 17:54:51, Peter Kasting wrote:
> I'm concerned with "this shouldn't happen" (which to me reads like
"DCHECK"/code bug) in conjunction with "has been observed".
> 
> It seems like either the current behavior is a weird edge case but correct, in
which case we shouldn't say things like "fallback" and "shouldn't happen" and
should just handle this, or else this is a real bug (maybe in code elsewhere?),
in which case we should fix the bug so we don't get here with no tabs.
> 
> I would imagine that stepping through the callstack in the case you describe
might illuminate this.

Hmm, I can certainly take a look at stepping through the callstack in more
detail and report back.

What sticks out to me here is that the documentation for the switch in question
only specifies behavior "at startup":

// Indicates the last session should be restored on startup. This overrides the
// preferences value and is primarily intended for testing. The value of this
// switch is the number of tabs to wait until loaded before 'load completed' is
// sent to the ui_test.

So we're sort of venturing into undefined behavior territory here--with the
caveat that even if the behavior is only defined at startup, someone absolutely
can still pass the switch later.

[Peter Kasting, 2016-11-02 18:05:47]

https://codereview.chromium.org/2469363002/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/2469363002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:757: // does not
normally restore, when a Chrome process is already running.
On 2016/11/02 18:04:40, tmartino wrote:
> On 2016/11/02 at 17:54:51, Peter Kasting wrote:
> > I'm concerned with "this shouldn't happen" (which to me reads like
> "DCHECK"/code bug) in conjunction with "has been observed".
> > 
> > It seems like either the current behavior is a weird edge case but correct,
in
> which case we shouldn't say things like "fallback" and "shouldn't happen" and
> should just handle this, or else this is a real bug (maybe in code
elsewhere?),
> in which case we should fix the bug so we don't get here with no tabs.
> > 
> > I would imagine that stepping through the callstack in the case you describe
> might illuminate this.
> 
> Hmm, I can certainly take a look at stepping through the callstack in more
> detail and report back.
> 
> What sticks out to me here is that the documentation for the switch in
question
> only specifies behavior "at startup":
> 
> // Indicates the last session should be restored on startup. This overrides
the
> // preferences value and is primarily intended for testing. The value of this
> // switch is the number of tabs to wait until loaded before 'load completed'
is
> // sent to the ui_test.
> 
> So we're sort of venturing into undefined behavior territory here--with the
> caveat that even if the behavior is only defined at startup, someone
absolutely
> can still pass the switch later.

Right, I'm wondering if the fix is "detect that the switch is being passed at
not-startup and ignore it".  Dunno where that code would live.

[tmartino, 2016-11-08 23:11:31]

On 2016/11/02 at 18:05:47, pkasting wrote:
>
https://codereview.chromium.org/2469363002/diff/1/chrome/browser/ui/startup/s...
> File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):
> 
>
https://codereview.chromium.org/2469363002/diff/1/chrome/browser/ui/startup/s...
> chrome/browser/ui/startup/startup_browser_creator_impl.cc:757: // does not
normally restore, when a Chrome process is already running.
> On 2016/11/02 18:04:40, tmartino wrote:
> > On 2016/11/02 at 17:54:51, Peter Kasting wrote:
> > > I'm concerned with "this shouldn't happen" (which to me reads like
> > "DCHECK"/code bug) in conjunction with "has been observed".
> > > 
> > > It seems like either the current behavior is a weird edge case but
correct, in
> > which case we shouldn't say things like "fallback" and "shouldn't happen"
and
> > should just handle this, or else this is a real bug (maybe in code
elsewhere?),
> > in which case we should fix the bug so we don't get here with no tabs.
> > > 
> > > I would imagine that stepping through the callstack in the case you
describe
> > might illuminate this.
> > 
> > Hmm, I can certainly take a look at stepping through the callstack in more
> > detail and report back.
> > 
> > What sticks out to me here is that the documentation for the switch in
question
> > only specifies behavior "at startup":
> > 
> > // Indicates the last session should be restored on startup. This overrides
the
> > // preferences value and is primarily intended for testing. The value of
this
> > // switch is the number of tabs to wait until loaded before 'load completed'
is
> > // sent to the ui_test.
> > 
> > So we're sort of venturing into undefined behavior territory here--with the
> > caveat that even if the behavior is only defined at startup, someone
absolutely
> > can still pass the switch later.
> 
> Right, I'm wondering if the fix is "detect that the switch is being passed at
not-startup and ignore it".  Dunno where that code would live.

Updating thread to say that I haven't abandoned this; I just had to switch
tracks to a launch blocking bug. I intend to return to this in the next few days
and will bundle in some other cleanup tasks we had discussed:

- Run the failing session restore through a debugger and hopefully reconcile
this instead of just documenting it
- Renaming static functions in StartupTabProvider
- Improve test coverage for StartupBrowserCreatorImpl
- Add typedef to session restore options, replacing current uint32_t

[tmartino, 2017-02-03 23:12:23]

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

[commit-bot: I haz the power, 2017-02-03 23:12:50]

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

[tmartino, 2017-02-03 23:17:15]

Description was changed from

==========
Clarifying documentation of conditions when OpenTabsInBrowser may fail.

BUG=660210
==========

to

==========
Cleaning up a number of outstanding comments from refactor of
StartupBrowserCreatorImpl. Specifically:

- Adds SessionRestore::Behavior typedef, replacing uint32_t as arg

- Renames |is_ephemeral_profile| to |is_incognito_or_guest|, as concerns were
raised that “ephemeral profile” is a specific, loaded term.

- Changes DetermineBrowserOpenBehavior to use bitmask.

- Adds unit tests for DetermineBrowserOpenBehavior.

- Removes extraneous TODOs.

- Adds browser tests testing welcome pages.

BUG=688574
==========

[tmartino, 2017-02-03 23:30:34]

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

[commit-bot: I haz the power, 2017-02-03 23:31:00]

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

[tmartino, 2017-02-03 23:44:12]

Hey Peter,

I followed up on the problem I'd been looking at in this old CL, and took the
opportunity to clean up some of the outstanding comments from the refactoring
effort. I also added some missing unit and browser tests. PTAL!

https://codereview.chromium.org/2469363002/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/2469363002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:757: // does not
normally restore, when a Chrome process is already running.
On 2016/11/02 at 18:05:47, Peter Kasting wrote:
> On 2016/11/02 18:04:40, tmartino wrote:
> > On 2016/11/02 at 17:54:51, Peter Kasting wrote:
> > > I'm concerned with "this shouldn't happen" (which to me reads like
> > "DCHECK"/code bug) in conjunction with "has been observed".
> > > 
> > > It seems like either the current behavior is a weird edge case but
correct, in
> > which case we shouldn't say things like "fallback" and "shouldn't happen"
and
> > should just handle this, or else this is a real bug (maybe in code
elsewhere?),
> > in which case we should fix the bug so we don't get here with no tabs.
> > > 
> > > I would imagine that stepping through the callstack in the case you
describe
> > might illuminate this.
> > 
> > Hmm, I can certainly take a look at stepping through the callstack in more
> > detail and report back.
> > 
> > What sticks out to me here is that the documentation for the switch in
question
> > only specifies behavior "at startup":
> > 
> > // Indicates the last session should be restored on startup. This overrides
the
> > // preferences value and is primarily intended for testing. The value of
this
> > // switch is the number of tabs to wait until loaded before 'load completed'
is
> > // sent to the ui_test.
> > 
> > So we're sort of venturing into undefined behavior territory here--with the
> > caveat that even if the behavior is only defined at startup, someone
absolutely
> > can still pass the switch later.
> 
> Right, I'm wondering if the fix is "detect that the switch is being passed at
not-startup and ignore it".  Dunno where that code would live.

Spent about a day on this and wanted to close the loop. Here's what I verified:

- GetSessionStartupPref doesn't know whether or not this is process startup, so
it allows the switch to override when it should not. This seems like the
appropriate place to make such a change, but unfortunately it would be difficult
to add a |process_startup| bool--this is a static method, and there are a number
of callsites outside these files that wouldn't have visibility into whether this
is startup.

- Because of this, the New Tab Page is not added in GetNewTabPageTabs.

- We expect MaybeAsyncRestore to perform a restore and leave this codepath
entirely before the line in question, but none occurs.

For the time being, I don't want to make any changes. I tried several approaches
to resolve this particular inconsistency; the only place that could actually
have the needed visibility would be  GetNewTabPageTabs. I just don't think the
added complexity of special-casing that method is proportional to the problem it
would be solving. More generally, I think the possibility of inconsistency
between restore expectations and outcomes more broadly is one we should probably
treat as possible, and not particularly high-risk.

https://codereview.chromium.org/2469363002/diff/40001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (left):

https://codereview.chromium.org/2469363002/diff/40001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:867: //
GetSessionStartupPref.
Note: Removing this TODO because I've closed the bug after deciding that such an
organization doesn't actually make sense.

[commit-bot: I haz the power, 2017-02-04 00:15:25]

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

[commit-bot: I haz the power, 2017-02-04 00:15:26]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-02-06 23:26:40]

LGTM

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore.h (right):

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.h:31: // Bitmask representing behaviors
available when restoring session. Populate
Nit: session -> a session

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right):

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:125: {
Nit: Only add these if it's important that the objects inside the scope be torn
down before the function's return statement. (2 places)

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:126:
StartupBrowserCreatorImpl launch(base::FilePath(), dummy,
Nit: I'd name this |creator|

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1071:
TabStripModel* tab_strip;
Nit: Have this do:

  TabStripModel* tab_strip = browser->tab_strip_model();

Then the IsWindows10OrNewer() block just below can omit the first two
statements, and hoist the "tab_strip = browser->tab_strip_model();" up from just
below that conditional block.

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1080:
ASSERT_EQ("chrome://welcome-win10/",
Nit: Seems like this should be EXPECT (couple of places)

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.h (right):

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.h:168: enum BehaviorFlags
{
Nit: Technically typedefs and enums belong at the top of the section, per the
styleguide.

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.h:176: typedef uint32_t
BrowserOpenBehaviorOptions;
I'm of two minds on this set of flags.

On one hand, it's nice for the unittesting code to have named constants here. 
Makes it more readable.

OTOH, it makes the .cc file strictly uglier.  And it's inconsistent with a lot
of the other startup code, including everything in StartupTabProvider, which
goes the bag-of-bools route.

I'm not sure whether it's a win.  I might end up killing it if it were me.

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc (right):

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:197:
StartupBrowserCreatorImpl::BrowserOpenBehavior output =
Nit: This file is really verbose because of how long the name
StartupBrowserCreatorImpl is... I wonder if it makes sense to do something like
"using Creator = StartupBrowserCreatorImpl" atop the file.

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:200: 
Nit: Why a blank line between setting and checking |output|?  (several places)

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:201:
ASSERT_EQ(StartupBrowserCreatorImpl::BrowserOpenBehavior::NEW, output);
Nit: Should probably be EXPECT (several places)

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:260: //
Exception: this can be overridden by switch.
Nit: switch -> passing a switch (2 places)

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:280: 
Nit: No blank line

[tmartino, 2017-02-06 23:52:19]

Thanks for the feedback. I'm heading out for the night but wanted to ask for
some further advice on one point.

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.h (right):

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.h:176: typedef uint32_t
BrowserOpenBehaviorOptions;
On 2017/02/06 at 23:26:40, Peter Kasting wrote:
> I'm of two minds on this set of flags.
> 
> On one hand, it's nice for the unittesting code to have named constants here. 
Makes it more readable.
> 
> OTOH, it makes the .cc file strictly uglier.  And it's inconsistent with a lot
of the other startup code, including everything in StartupTabProvider, which
goes the bag-of-bools route.
> 
> I'm not sure whether it's a win.  I might end up killing it if it were me.

I originally wrote the tests using the bools approach, so having seen it both
ways, I think it's quite a big win. Frankly, I'm much more confident not just in
the readability of the tests this way, but the correctness of them as well, and
solid unit tests were a big goal in this refactor.

Re: .cc ugliness, WDYT of using kProcessStartup naming instead? We'd still add a
few extra lines compared to the inline bools, but it wouldn't take the
readability hit of having all these BIG_MACRO_STYLE_IDENTIFIERS in the middle of
the function. I'm pretty sure I've seen bitmask flags written both ways, and
because these "act like" a grouping of consts, rather than a list of mutually
exclusive choices, I think it's in the spirit of the style guide.

Re: consistency, I was already mulling over changing StartupTabProvider
functions with 3 or more bools and could add that on here if you think it would
be a win. WDYT?

[Peter Kasting, 2017-02-07 00:11:59]

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.h (right):

https://codereview.chromium.org/2469363002/diff/60001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.h:176: typedef uint32_t
BrowserOpenBehaviorOptions;
On 2017/02/06 23:52:19, tmartino wrote:
> On 2017/02/06 at 23:26:40, Peter Kasting wrote:
> > I'm of two minds on this set of flags.
> > 
> > On one hand, it's nice for the unittesting code to have named constants
here. 
> Makes it more readable.
> > 
> > OTOH, it makes the .cc file strictly uglier.  And it's inconsistent with a
lot
> of the other startup code, including everything in StartupTabProvider, which
> goes the bag-of-bools route.
> > 
> > I'm not sure whether it's a win.  I might end up killing it if it were me.
> 
> I originally wrote the tests using the bools approach, so having seen it both
> ways, I think it's quite a big win. Frankly, I'm much more confident not just
in
> the readability of the tests this way, but the correctness of them as well,
and
> solid unit tests were a big goal in this refactor.

OK.  I think if you're confident, your instinct should override mine.

> Re: .cc ugliness, WDYT of using kProcessStartup naming instead? We'd still add
a
> few extra lines compared to the inline bools, but it wouldn't take the
> readability hit of having all these BIG_MACRO_STYLE_IDENTIFIERS in the middle
of
> the function. I'm pretty sure I've seen bitmask flags written both ways, and
> because these "act like" a grouping of consts, rather than a list of mutually
> exclusive choices, I think it's in the spirit of the style guide.

For me, it would make no difference either way; it's code like this:

  if (foo_bar)
    options |= FOO_BAR;

...that reads as ugly, and it's no simpler with a different name.

> Re: consistency, I was already mulling over changing StartupTabProvider
> functions with 3 or more bools and could add that on here if you think it
would
> be a win. WDYT?

It sounds like my instinct and yours may not match, so... :)

I think the biggest thing that feels bad is when we have the same "concept"
passed to several functions but as a bool in some places and an enum in others. 
It would be nice to represent particular concepts the same way everywhere, but
that might require converting everything (few args or not) and having the "big
union of random concepts" enum, which has its own readability issues.

[tmartino, 2017-02-08 22:56:41]

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

[commit-bot: I haz the power, 2017-02-08 22:57:46]

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

[tmartino, 2017-02-08 23:05:10]

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

[tmartino, 2017-02-08 23:05:10]

OK! Done to all original comments, and I'll stop fiddling with the BehaviorFlags
until/unless inspiration hits me.

+sky for OWNERS on session_restore.*

[sky, 2017-02-08 23:56:21]

https://codereview.chromium.org/2469363002/diff/80001/chrome/browser/sessions...
File chrome/browser/sessions/session_restore.h (right):

https://codereview.chromium.org/2469363002/diff/80001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.h:33: typedef uint32_t Behavior;
typedef->usint
Also, to make this class name it BehaviorBitmask.

https://codereview.chromium.org/2469363002/diff/80001/chrome/browser/sessions...
chrome/browser/sessions/session_restore.h:35: enum {
enum class, which gives you the scoping should other enums be added to this
class.

https://codereview.chromium.org/2469363002/diff/80001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.h (right):

https://codereview.chromium.org/2469363002/diff/80001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.h:100: enum BehaviorFlags
{
enum class?

https://codereview.chromium.org/2469363002/diff/80001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.h:108: typedef uint32_t
BrowserOpenBehaviorOptions;
using.

[commit-bot: I haz the power, 2017-02-09 00:03:33]

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

[commit-bot: I haz the power, 2017-02-09 00:03:34]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-02-09 19:48:59]

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

[tmartino, 2017-02-09 19:49:47]

On 2017/02/08 at 23:56:21, sky wrote:
>
https://codereview.chromium.org/2469363002/diff/80001/chrome/browser/sessions...
> File chrome/browser/sessions/session_restore.h (right):
> 
>
https://codereview.chromium.org/2469363002/diff/80001/chrome/browser/sessions...
> chrome/browser/sessions/session_restore.h:33: typedef uint32_t Behavior;
> typedef->usint
> Also, to make this class name it BehaviorBitmask.
> 
>
https://codereview.chromium.org/2469363002/diff/80001/chrome/browser/sessions...
> chrome/browser/sessions/session_restore.h:35: enum {
> enum class, which gives you the scoping should other enums be added to this
class.
> 
>
https://codereview.chromium.org/2469363002/diff/80001/chrome/browser/ui/start...
> File chrome/browser/ui/startup/startup_browser_creator_impl.h (right):
> 
>
https://codereview.chromium.org/2469363002/diff/80001/chrome/browser/ui/start...
> chrome/browser/ui/startup/startup_browser_creator_impl.h:100: enum
BehaviorFlags {
> enum class?
> 
>
https://codereview.chromium.org/2469363002/diff/80001/chrome/browser/ui/start...
> chrome/browser/ui/startup/startup_browser_creator_impl.h:108: typedef uint32_t
BrowserOpenBehaviorOptions;
> using.

Changed both typedefs to using, and renamed SessionRestore::Behavior to
SessionRestore::BehaviorBitmask.

As for the enum class suggestions, unfortunately enum classes don't support the
operators necessary for bitmasking (|, &, etc.).

One alternative that I *think* should make the compiler happy would be to
replace the enum entirely with a namespace, e.g.,:

namespace Behavior {
  using Bitmask = uint32_t;
  constexpr Bitmask kClobberCurrentTab = 1 << 0;
  // others...
}

In context, this would look like:
Behavior::Bitmask = Behavior::kClobberCurrentTab;

[commit-bot: I haz the power, 2017-02-09 19:49:49]

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, 2017-02-09 20:36:01]

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

[commit-bot: I haz the power, 2017-02-09 20:36:02]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-09 21:00:45]

Ok, LGTM

[tmartino, 2017-02-09 21:01:43]

The CQ bit was checked by tmartino@chromium.org

[tmartino, 2017-02-09 21:01:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2469363002/#ps100001
(title: "sky comments")

[commit-bot: I haz the power, 2017-02-09 21:02:29]

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, 2017-02-09 21:07:42]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486674103604870,
"parent_rev": "c1dafff684999d4cc64e9666e14421b1ef56f542", "commit_rev":
"e2288ebbd5ec887e98b1140b6b8c12d56514c9aa"}

[commit-bot: I haz the power, 2017-02-09 21:08:18]

Description was changed from

==========
Cleaning up a number of outstanding comments from refactor of
StartupBrowserCreatorImpl. Specifically:

- Adds SessionRestore::Behavior typedef, replacing uint32_t as arg

- Renames |is_ephemeral_profile| to |is_incognito_or_guest|, as concerns were
raised that “ephemeral profile” is a specific, loaded term.

- Changes DetermineBrowserOpenBehavior to use bitmask.

- Adds unit tests for DetermineBrowserOpenBehavior.

- Removes extraneous TODOs.

- Adds browser tests testing welcome pages.

BUG=688574
==========

to

==========
Cleaning up a number of outstanding comments from refactor of
StartupBrowserCreatorImpl. Specifically:

- Adds SessionRestore::Behavior typedef, replacing uint32_t as arg

- Renames |is_ephemeral_profile| to |is_incognito_or_guest|, as concerns were
raised that “ephemeral profile” is a specific, loaded term.

- Changes DetermineBrowserOpenBehavior to use bitmask.

- Adds unit tests for DetermineBrowserOpenBehavior.

- Removes extraneous TODOs.

- Adds browser tests testing welcome pages.

BUG=688574

Review-Url: https://codereview.chromium.org/2469363002
Cr-Commit-Position: refs/heads/master@{#449405}
Committed:
https://chromium.googlesource.com/chromium/src/+/e2288ebbd5ec887e98b1140b6b8c...
==========

[commit-bot: I haz the power, 2017-02-09 21:08:19]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/e2288ebbd5ec887e98b1140b6b8c...