Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2457653003:
Completing refactor of startup_browser_creator_impl (Closed)
|
Created:
4 years, 1 month ago by tmartino Modified:
4 years, 1 month ago Reviewers:
Peter Kasting CC:
chromium-reviews, Mathieu Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCompleting refactor of startup_browser_creator_impl. This includes:
- Reintroducing session restore logic.
- Preventing Win 10 from branching on this codepath, as Win10 has been descoped from this launch.
- Correcting several small but critical bugs surfaced by session restore path:
- Unexpected content in Guest Mode.
- NTP showing unexpectedly on restored sessions.
- Pinned tabs appearing duplicated on restored sessions.
BUG=660210
Committed: https://crrev.com/43f9c50bcb8b440c0f462b69f13b4bf9994e639b
Cr-Commit-Position: refs/heads/master@{#429303}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Addressing grt comments #
Total comments: 2
Patch Set 3 : TabsToUrls optimizations #
Total comments: 44
Patch Set 4 : Addressing pkasting comments #
Dependent Patchsets: Messages
Total messages: 31 (9 generated)
Description was changed from ========== Completing refactor of startup_browser_creator_impl BUG= ========== to ========== Completing refactor of startup_browser_creator_impl. This includes: - Reintroducing session restore logic. - Preventing Win 10 from branching on this codepath, as Win10 has been descoped from this launch. - Correcting several small but critical bugs surfaced by session restore path: - Unexpected content in Guest Mode. - NTP showing unexpectedly on restored sessions. - Pinned tabs appearing duplicated on restored sessions. BUG=660210 ==========
tmartino@chromium.org changed reviewers: + pkasting@chromium.org
Hi Peter,
This should be the grand finale of the refactor we've been working on. There are
some interesting division-of-logic questions here, some of which I've handled
rather arbitrarily, so I'm looking forward to your feedback.
I've tested this logic against the Cartesian product of:
{Profile has LAST (Session Restore) pref, Profile has URLS pref}
x
{URL passed, No URLs passed}
x
{Process startup, not process startup}
As well as:
- with the --restore-last-session flag (both at startup and after startup)
- with a window already open for the given profile (with both the URLs pref and
the LAST pref)
Let me know if there are any other test cases that seem noteworthy for me to run
through.
Thanks!
Tommy
https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):
https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:627:
BrowserOpenBehavior behavior = DetermineBrowserOpenBehavior(
One example of weird logic structure questions: do you think this would be
cleaner with an extra layer of lasagna code that grabs the bits of system state
and passes them on to the static function? Same goes for the
DetermineSynchronousRestoreOptions call below.
https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:751: browser =
OpenTabsInBrowser(
This seems gross and unnecessary; however, I was able to trigger this crash at
least once, so I think it's a worthwhile fallback.
The scenario where it crashed was obscure: using the --restore-last-session flag
on a profile without the restore pref set, while Chrome process was already
running. We view this as a restore desired (the pref comes back as Type::LAST)
but for some reason downstream, the restore doesn't happen.
Since that flag is only meant to work at startup per its documentation, I'm not
too worried that there's a substantial flaw in our logic here. But I do think
it's better to launch just the NTP than to crash in this situation (and any
other session restore mismatches that might pop up). That also preserves
existing behavior--I'm not sure what sequence of steps is invoked in the
existing logic, but the end result is that the NTP is displayed instead of a
session restore.
https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:770: // static
Will add unit tests for static code to this CL later in review process. Wanted
you to get a look at the structure before writing tests.
is it possible to create a unittest to cover the matrix of possibilities you covered manually? https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:624: if (MaybeAsyncRestore(tabs, process_startup, is_post_crash_launch)) nit: i've had other reviewers ask me not to use "Maybe" since it's a bit wishy-washy. wdyt of "BeginAsyncRestore" and document it as returning false if nothing was initiated? something like: // Begins an asynchronous session restore if blahdeblah. Returns true if the // operation was initiated, or false if a synchronous launch should continue. https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:722: std::vector<GURL> urls; i think this looks nicer as: return (service && service->RestoreIfNecessary(TabsToUrls(tabs))); below, too https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:751: browser = OpenTabsInBrowser( On 2016/10/27 23:00:43, tmartino wrote: > This seems gross and unnecessary; however, I was able to trigger this crash at > least once, so I think it's a worthwhile fallback. > > The scenario where it crashed was obscure: using the --restore-last-session flag > on a profile without the restore pref set, while Chrome process was already > running. We view this as a restore desired (the pref comes back as Type::LAST) > but for some reason downstream, the restore doesn't happen. > > Since that flag is only meant to work at startup per its documentation, The doc comment on kRestoreLastSession says it's "primarily used for testing", which doesn't seem to be the case. Since it's a fundamental part of the product (it's used for restarts driven by the OS on Windows and macOS), would you remove that part of the comment? Also, the part of the comment about the switch's value seems out of date. A simple codesearch doesn't reveal any code that sets or gets a value from it. > I'm not > too worried that there's a substantial flaw in our logic here. But I do think > it's better to launch just the NTP than to crash in this situation (and any > other session restore mismatches that might pop up). That also preserves > existing behavior--I'm not sure what sequence of steps is invoked in the > existing logic, but the end result is that the NTP is displayed instead of a > session restore. https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:779: if (!process_startup) { i forget: is |process_startup| true or false for the case where the browser process is already running in background mode? i think that case should be handled as if the process were being started for the first time. https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:789: if (pref.type == SessionStartupPref::Type::LAST) { nit: omit "::Type" since it's a regular enum rather than an enum class. https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:808: if (has_create_browser_default || has_create_browser_switch) nit: rather than setting this bit and then clearing it below for the mac case, how about "if (!was_mac_login_or_resume && (foo || bar))" here (with the mac-specific comment just above)? https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:813: // login item or Lion's resume feature. nit: while everyone new what "Lion" meant when it was the new hot and shiny, i think it would be better for this to say "...or the OS's resume feature introduced in OS X 10.7." https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.h:90: USE_EXISTING // Attempt to add to an existing tabbed browser. nit: add a trailing comma
On 2016/10/28 at 09:22:12, grt wrote: > is it possible to create a unittest to cover the matrix of possibilities you covered manually? Hmm, I don't think so. The matrix I covered manually was really end-to-end: not just making sure we follow the right restore (etc) behavior, but adding pinned tabs, prefs tabs, command line URLs, etc. and making sure that everything came out on the other end. (I was also testing the implicit assumptions I've made about input states.) That doesn't necessarily translate nicely into unit tests. Like I said above, I do intend to unit-test the meaningful combinations of state going into the static functions I've written (once we're a bit further into the review cycle), and I tried to structure them to expose the most possible decision-making to those tests. Do you see opportunities for further gains from that perspective?
https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:624: if (MaybeAsyncRestore(tabs, process_startup, is_post_crash_launch)) On 2016/10/28 at 09:22:12, grt (UTC plus 2) wrote: > nit: i've had other reviewers ask me not to use "Maybe" since it's a bit wishy-washy. wdyt of "BeginAsyncRestore" and document it as returning false if nothing was initiated? something like: > > // Begins an asynchronous session restore if blahdeblah. Returns true if the > // operation was initiated, or false if a synchronous launch should continue. Hmm, I've tried three different names (Maybe..., Begin..., and Attempt...) and read through the code again. The problem is that IMO the latter two imply that Async restore is the "normal" case and that it *should* usually happen. When I read if(Begin...) or if(Attempt...) it comes across as a success/failure check, and this is more of a "does this apply?" check. I did expand the comments per your suggestions, but feel free to push back on the name if you feel strongly. https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:722: std::vector<GURL> urls; On 2016/10/28 at 09:22:12, grt (UTC plus 2) wrote: > i think this looks nicer as: > return (service && service->RestoreIfNecessary(TabsToUrls(tabs))); > below, too Agreed, done. https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:751: browser = OpenTabsInBrowser( On 2016/10/28 at 09:22:12, grt (UTC plus 2) wrote: > On 2016/10/27 23:00:43, tmartino wrote: > > This seems gross and unnecessary; however, I was able to trigger this crash at > > least once, so I think it's a worthwhile fallback. > > > > The scenario where it crashed was obscure: using the --restore-last-session flag > > on a profile without the restore pref set, while Chrome process was already > > running. We view this as a restore desired (the pref comes back as Type::LAST) > > but for some reason downstream, the restore doesn't happen. > > > > Since that flag is only meant to work at startup per its documentation, > > The doc comment on kRestoreLastSession says it's "primarily used for testing", which doesn't seem to be the case. Since it's a fundamental part of the product (it's used for restarts driven by the OS on Windows and macOS), would you remove that part of the comment? Also, the part of the comment about the switch's value seems out of date. A simple codesearch doesn't reveal any code that sets or gets a value from it. > > > I'm not > > too worried that there's a substantial flaw in our logic here. But I do think > > it's better to launch just the NTP than to crash in this situation (and any > > other session restore mismatches that might pop up). That also preserves > > existing behavior--I'm not sure what sequence of steps is invoked in the > > existing logic, but the end result is that the NTP is displayed instead of a > > session restore. Done, and thank you for helping to keep docs up-to-date, even in places not directly touched by the current CL. This is not a common enough habit. https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:779: if (!process_startup) { On 2016/10/28 at 09:22:12, grt (UTC plus 2) wrote: > i forget: is |process_startup| true or false for the case where the browser process is already running in background mode? i think that case should be handled as if the process were being started for the first time. I haven't been able to completely trace that particular use case. This is passed all the way down from StartupBrowserCreator::LaunchBrowser, specifically the value of in_synchronous_profile_launch_, which in turn is determined by what value is passed as |process_startup| to ProcessCmdLineImpl; it diverges from there. The existing code doesn't do anything special to check for and handle the background case--I suspect the idea was to trust the caller to figure out whether this should "look like" startup or not. https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:789: if (pref.type == SessionStartupPref::Type::LAST) { On 2016/10/28 at 09:22:12, grt (UTC plus 2) wrote: > nit: omit "::Type" since it's a regular enum rather than an enum class. Done https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:808: if (has_create_browser_default || has_create_browser_switch) On 2016/10/28 at 09:22:12, grt (UTC plus 2) wrote: > nit: rather than setting this bit and then clearing it below for the mac case, how about "if (!was_mac_login_or_resume && (foo || bar))" here (with the mac-specific comment just above)? Done https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.cc:813: // login item or Lion's resume feature. On 2016/10/28 at 09:22:12, grt (UTC plus 2) wrote: > nit: while everyone new what "Lion" meant when it was the new hot and shiny, i think it would be better for this to say "...or the OS's resume feature introduced in OS X 10.7." Done https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2457653003/diff/1/chrome/browser/ui/startup/s... chrome/browser/ui/startup/startup_browser_creator_impl.h:90: USE_EXISTING // Attempt to add to an existing tabbed browser. On 2016/10/28 at 09:22:12, grt (UTC plus 2) wrote: > nit: add a trailing comma Done
On 2016/10/28 18:34:03, tmartino wrote: > On 2016/10/28 at 09:22:12, grt wrote: > > is it possible to create a unittest to cover the matrix of possibilities you > covered manually? > > Hmm, I don't think so. The matrix I covered manually was really end-to-end: not > just making sure we follow the right restore (etc) behavior, but adding pinned > tabs, prefs tabs, command line URLs, etc. and making sure that everything came > out on the other end. (I was also testing the implicit assumptions I've made > about input states.) That doesn't necessarily translate nicely into unit tests. Is it possible to do all that in browser tests? I think it's possible to set command line args, a pre-canned profile, etc, then verify that a browser was opened with the expected tabs. My concern is that this code is fairly complex, so future regressions are likely. Anything you can do to prevent these regressions will save you time, since you won't have to debug someone else's additions. :-)
looks good to me, though i'll defer to PK since i was OoO during the last CL and detached a bit from what's going on. i'd be happy to take another pass if you'd like, just let me know. cheers. https://codereview.chromium.org/2457653003/diff/20001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:194: std::vector<GURL> urls(tabs.size()); micro-optimization: you could default construct the vector, urls.reserve(tabs.size()) to get the memory allocated, then use std::back_inserter(urls) below. this avoids constructing a bunch of URLs on this line only to overwrite them during the transform. https://codereview.chromium.org/2457653003/diff/20001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:196: [](StartupTab tab) { return tab.url; }); const StartupTab& tab
Added optimizations. Thanks for all the energy you've put into this, Greg. Browser Tests sound like a good idea. I'm pretty unfamiliar with them, so I will have to put some research and thought into implementing them; will follow up on that point next week.
On 2016/10/28 19:43:20, tmartino wrote: > Added optimizations. Thanks for all the energy you've put into this, Greg. > > Browser Tests sound like a good idea. I'm pretty unfamiliar with them, so I will > have to put some research and thought into implementing them; will follow up on > that point next week. Have a look at https://chromium.googlesource.com/chromium/src/+/master/chrome/test/base/in_p.... There are many possibilities ranging from pre-canned prefs files dropped in SetUpUserDataDirectory to executing two or more runs in sequence via PRE_Foo and Foo. I'm not an expert in browser tests, though I hear caitkp is (be sure to smile if you tell her "Greg told me you were an expert in browser tests." :-) ).
Not going to get to this today, please harass me on IM if you haven't heard from me by Monday afternoonish.
LGTM https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:284: if (base::win::GetVersion() >= base::win::VERSION_WIN10) Nit: Should there be a comment here about why Win 10 does not use the consolidated flow? https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:618: bool is_incognito_or_guest = Nit: Do you think it would be any clearer to reverse the sense of this bool (|is_regular_profile|) and pass that around to various places instead? On one hand I think "is A or B" is inherently an awkward concept to reason about, on the other hand "regular profile" is kind of nebulous. Maybe there's a phrase which describes the common nature of incognito/guest profiles, e.g. "is_tempory_profile" or "is_ephemeral_profile" or something? https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:626: // remainder of the process is self-contained, so return. Nit: Passive voice; maybe "Return immediately if we start an async restore, since the remainder of that process is self-contained." https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:726: // Service will be null in incognito and guest mode. Nit: Service -> |service| Or: // Note: there's no session service in incognito or guest mode. auto* service = SessionServiceFactory::GetForProfileForSessionRestore(profile_); return ...; Or (more verbose): // There's no session service in incognito or guest mode. auto* service = SessionServiceFactory::GetForProfileForSessionRestore(profile_); if (!service) return false; return service->RestoreIfNecessary(TabsToUrls(tabs)); https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:727: return (service && service->RestoreIfNecessary(TabsToUrls(tabs))); Nit: No need for outer () https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:737: Nit: I'd remove this blank, since the subsequent block is still about setting |browser| correctly. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:747: StartupBrowserCreator::in_synchronous_profile_launch_ = true; Nit: I suggest using base::AutoReset to make clear this is a "set for limited scope". This also makes it harder to add an early-return later that jumps out before we reset this variable. { base::AutoReset<bool> synchronous_launch_resetter( &StartupBrowserCreator::in_synchronous_profile_launch_, true); browser = OpenTabsInBrowser(...); } (Also, is it just me, or is it really weird that a class called "StartupBrowserCreatorImpl" does not inherit from "StartupBrowserCreator", so we have to qualify the name of this member?) https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:750: // prevent a crash, use the NTP if |tabs| is empty. Nit: "As a fallback to prevent a crash" makes it sound like this is some sort of error case. Maybe instead of this, say when we'd expect this to be empty? https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:782: // be appended to an existing window if possible, unless overridden by Nit: by -> by a? https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:810: // login item or the resume feature in OS 10.7+. Nit: This sentence reads confusingly because the part about what to do comes in the middle of two different groups of qualifiers about when we do it. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:812: (has_create_browser_default || has_create_browser_switch)) { Nit: {} optional https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:145: uint32_t restore_options, Nit: Here and in DetermineSynchronousRestoreOptions() below we use a uint32_t to refer to some nebulous "restore options". It took me a while to figure out this is just SessionRestore::Behavior (but expressed as a uint32_t, I guess to emphasize it could be a bitmask of any of those values. It would be nice if there was a type alias for this type, defined in session_restore.h, so that functions could pass that around instead of uint32_t, and its meaning would be clear. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:150: static BrowserOpenBehavior DetermineBrowserOpenBehavior( Nit: I suggest placing static methods either above or below non-static methods, since they're really more like namespaced globals. (2 places) https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:158: // Populates and returns the relevant bitmask options which must be passed Nit: No need for "Populates and" https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:20: constexpr uint32_t kNewTabPageTabs = 0x20; Nit: Might be good to make it clearer these are successive bits via formatting: constexpr uint32_t kOnboardingTabs = 1 << 0; constexpr uint32_t kDistributionFirstRunTabs = 1 << 1; ... https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:103: // Provider is ignored entirely when short-circuited by incognito/crash. Nit: Perhaps this comment (reworded slightly as "Check for...") belongs e.g. just above line 116. It seems like an explanation of an implementation detail, rather than an explanation of the overall purpose of the test. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_tab_provider.h:47: // Possibly returns the New Tab Page, if the user's preferences indicate a Nit: No need for "Possibly"
The CQ bit was checked by tmartino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tmartino@chromium.org
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/2457653003/#ps60001 (title: "Addressing pkasting comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:284: if (base::win::GetVersion() >= base::win::VERSION_WIN10) On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: Should there be a comment here about why Win 10 does not use the consolidated flow? Added TODO. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:618: bool is_incognito_or_guest = On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: Do you think it would be any clearer to reverse the sense of this bool (|is_regular_profile|) and pass that around to various places instead? On one hand I think "is A or B" is inherently an awkward concept to reason about, on the other hand "regular profile" is kind of nebulous. > > Maybe there's a phrase which describes the common nature of incognito/guest profiles, e.g. "is_tempory_profile" or "is_ephemeral_profile" or something? Definitely don't like "regular", but I'm OK with "ephemeral profile". https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:626: // remainder of the process is self-contained, so return. On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: Passive voice; maybe "Return immediately if we start an async restore, since the remainder of that process is self-contained." Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:726: // Service will be null in incognito and guest mode. On 2016/10/31 at 19:24:05, Peter Kasting wrote: > Nit: Service -> |service| > > Or: > > // Note: there's no session service in incognito or guest mode. > auto* service = > SessionServiceFactory::GetForProfileForSessionRestore(profile_); > return ...; > > Or (more verbose): > > // There's no session service in incognito or guest mode. > auto* service = > SessionServiceFactory::GetForProfileForSessionRestore(profile_); > if (!service) > return false; > > return service->RestoreIfNecessary(TabsToUrls(tabs)); Done (went for the middle option) https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:727: return (service && service->RestoreIfNecessary(TabsToUrls(tabs))); On 2016/10/31 at 19:24:05, Peter Kasting wrote: > Nit: No need for outer () Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:737: On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: I'd remove this blank, since the subsequent block is still about setting |browser| correctly. Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:747: StartupBrowserCreator::in_synchronous_profile_launch_ = true; On 2016/10/31 at 19:24:05, Peter Kasting wrote: > Nit: I suggest using base::AutoReset to make clear this is a "set for limited scope". This also makes it harder to add an early-return later that jumps out before we reset this variable. > > { > base::AutoReset<bool> synchronous_launch_resetter( > &StartupBrowserCreator::in_synchronous_profile_launch_, true); > > browser = OpenTabsInBrowser(...); > } > > (Also, is it just me, or is it really weird that a class called "StartupBrowserCreatorImpl" does not inherit from "StartupBrowserCreator", so we have to qualify the name of this member?) Done. And, yeah, I don't care for the structure of these two classes. It feels like the Impl class isn't so much an implementation as "more functions we moved out of the class because it was getting unwieldy." https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:750: // prevent a crash, use the NTP if |tabs| is empty. On 2016/10/31 at 19:24:05, Peter Kasting wrote: > Nit: "As a fallback to prevent a crash" makes it sound like this is some sort of error case. Maybe instead of this, say when we'd expect this to be empty? It really *is* an error case. I had a comment earlier in the thread when grt was reviewing that explained this further: This seems gross and unnecessary; however, I was able to trigger this crash at least once, so I think it's a worthwhile fallback. The scenario where it crashed was obscure: using the --restore-last-session flag on a profile without the restore pref set, while Chrome process was already running. We view this as a restore desired (the pref comes back as Type::LAST) but for some reason downstream, the restore doesn't happen. Since that flag is only meant to work at startup per its documentation, I'm not too worried that there's a substantial flaw in our logic here. But I do think it's better to launch just the NTP than to crash in this situation (and any other session restore mismatches that might pop up). That also preserves existing behavior--I'm not sure what sequence of steps is invoked in the existing logic, but the end result is that the NTP is displayed instead of a session restore. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:782: // be appended to an existing window if possible, unless overridden by On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: by -> by a? Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:810: // login item or the resume feature in OS 10.7+. On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: This sentence reads confusingly because the part about what to do comes in the middle of two different groups of qualifiers about when we do it. There are a lot of conditions to fit in, but I moved the action ("suppress the creation...") to the beginning. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:812: (has_create_browser_default || has_create_browser_switch)) { On 2016/10/31 at 19:24:05, Peter Kasting wrote: > Nit: {} optional Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:145: uint32_t restore_options, On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: Here and in DetermineSynchronousRestoreOptions() below we use a uint32_t to refer to some nebulous "restore options". It took me a while to figure out this is just SessionRestore::Behavior (but expressed as a uint32_t, I guess to emphasize it could be a bitmask of any of those values. > > It would be nice if there was a type alias for this type, defined in session_restore.h, so that functions could pass that around instead of uint32_t, and its meaning would be clear. Ack. Made a note to self to look at this when I make a cleanup pass post-launch, but I'd rather not let /browser/sessions creep into scope right now. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:150: static BrowserOpenBehavior DetermineBrowserOpenBehavior( On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: I suggest placing static methods either above or below non-static methods, since they're really more like namespaced globals. (2 places) Agreed. Moved them to be the last non-deprecated functions. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:158: // Populates and returns the relevant bitmask options which must be passed On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: No need for "Populates and" Done. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:20: constexpr uint32_t kNewTabPageTabs = 0x20; On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: Might be good to make it clearer these are successive bits via formatting: > > constexpr uint32_t kOnboardingTabs = 1 << 0; > constexpr uint32_t kDistributionFirstRunTabs = 1 << 1; > ... Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:103: // Provider is ignored entirely when short-circuited by incognito/crash. On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: Perhaps this comment (reworded slightly as "Check for...") belongs e.g. just above line 116. It seems like an explanation of an implementation detail, rather than an explanation of the overall purpose of the test. Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_tab_provider.h:47: // Possibly returns the New Tab Page, if the user's preferences indicate a On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: No need for "Possibly" Done
https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:284: if (base::win::GetVersion() >= base::win::VERSION_WIN10) On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: Should there be a comment here about why Win 10 does not use the consolidated flow? Added TODO. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:618: bool is_incognito_or_guest = On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: Do you think it would be any clearer to reverse the sense of this bool (|is_regular_profile|) and pass that around to various places instead? On one hand I think "is A or B" is inherently an awkward concept to reason about, on the other hand "regular profile" is kind of nebulous. > > Maybe there's a phrase which describes the common nature of incognito/guest profiles, e.g. "is_tempory_profile" or "is_ephemeral_profile" or something? Definitely don't like "regular", but I'm OK with "ephemeral profile". https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:626: // remainder of the process is self-contained, so return. On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: Passive voice; maybe "Return immediately if we start an async restore, since the remainder of that process is self-contained." Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:726: // Service will be null in incognito and guest mode. On 2016/10/31 at 19:24:05, Peter Kasting wrote: > Nit: Service -> |service| > > Or: > > // Note: there's no session service in incognito or guest mode. > auto* service = > SessionServiceFactory::GetForProfileForSessionRestore(profile_); > return ...; > > Or (more verbose): > > // There's no session service in incognito or guest mode. > auto* service = > SessionServiceFactory::GetForProfileForSessionRestore(profile_); > if (!service) > return false; > > return service->RestoreIfNecessary(TabsToUrls(tabs)); Done (went for the middle option) https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:727: return (service && service->RestoreIfNecessary(TabsToUrls(tabs))); On 2016/10/31 at 19:24:05, Peter Kasting wrote: > Nit: No need for outer () Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:737: On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: I'd remove this blank, since the subsequent block is still about setting |browser| correctly. Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:747: StartupBrowserCreator::in_synchronous_profile_launch_ = true; On 2016/10/31 at 19:24:05, Peter Kasting wrote: > Nit: I suggest using base::AutoReset to make clear this is a "set for limited scope". This also makes it harder to add an early-return later that jumps out before we reset this variable. > > { > base::AutoReset<bool> synchronous_launch_resetter( > &StartupBrowserCreator::in_synchronous_profile_launch_, true); > > browser = OpenTabsInBrowser(...); > } > > (Also, is it just me, or is it really weird that a class called "StartupBrowserCreatorImpl" does not inherit from "StartupBrowserCreator", so we have to qualify the name of this member?) Done. And, yeah, I don't care for the structure of these two classes. It feels like the Impl class isn't so much an implementation as "more functions we moved out of the class because it was getting unwieldy." https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:750: // prevent a crash, use the NTP if |tabs| is empty. On 2016/10/31 at 19:24:05, Peter Kasting wrote: > Nit: "As a fallback to prevent a crash" makes it sound like this is some sort of error case. Maybe instead of this, say when we'd expect this to be empty? It really *is* an error case. I had a comment earlier in the thread when grt was reviewing that explained this further: This seems gross and unnecessary; however, I was able to trigger this crash at least once, so I think it's a worthwhile fallback. The scenario where it crashed was obscure: using the --restore-last-session flag on a profile without the restore pref set, while Chrome process was already running. We view this as a restore desired (the pref comes back as Type::LAST) but for some reason downstream, the restore doesn't happen. Since that flag is only meant to work at startup per its documentation, I'm not too worried that there's a substantial flaw in our logic here. But I do think it's better to launch just the NTP than to crash in this situation (and any other session restore mismatches that might pop up). That also preserves existing behavior--I'm not sure what sequence of steps is invoked in the existing logic, but the end result is that the NTP is displayed instead of a session restore. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:782: // be appended to an existing window if possible, unless overridden by On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: by -> by a? Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:810: // login item or the resume feature in OS 10.7+. On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: This sentence reads confusingly because the part about what to do comes in the middle of two different groups of qualifiers about when we do it. There are a lot of conditions to fit in, but I moved the action ("suppress the creation...") to the beginning. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:812: (has_create_browser_default || has_create_browser_switch)) { On 2016/10/31 at 19:24:05, Peter Kasting wrote: > Nit: {} optional Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:145: uint32_t restore_options, On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: Here and in DetermineSynchronousRestoreOptions() below we use a uint32_t to refer to some nebulous "restore options". It took me a while to figure out this is just SessionRestore::Behavior (but expressed as a uint32_t, I guess to emphasize it could be a bitmask of any of those values. > > It would be nice if there was a type alias for this type, defined in session_restore.h, so that functions could pass that around instead of uint32_t, and its meaning would be clear. Ack. Made a note to self to look at this when I make a cleanup pass post-launch, but I'd rather not let /browser/sessions creep into scope right now. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:150: static BrowserOpenBehavior DetermineBrowserOpenBehavior( On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: I suggest placing static methods either above or below non-static methods, since they're really more like namespaced globals. (2 places) Agreed. Moved them to be the last non-deprecated functions. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:158: // Populates and returns the relevant bitmask options which must be passed On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: No need for "Populates and" Done. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:20: constexpr uint32_t kNewTabPageTabs = 0x20; On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: Might be good to make it clearer these are successive bits via formatting: > > constexpr uint32_t kOnboardingTabs = 1 << 0; > constexpr uint32_t kDistributionFirstRunTabs = 1 << 1; > ... Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl_unittest.cc:103: // Provider is ignored entirely when short-circuited by incognito/crash. On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: Perhaps this comment (reworded slightly as "Check for...") belongs e.g. just above line 116. It seems like an explanation of an implementation detail, rather than an explanation of the overall purpose of the test. Done https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_tab_provider.h (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_tab_provider.h:47: // Possibly returns the New Tab Page, if the user's preferences indicate a On 2016/10/31 at 19:24:06, Peter Kasting wrote: > Nit: No need for "Possibly" Done
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Completing refactor of startup_browser_creator_impl. This includes: - Reintroducing session restore logic. - Preventing Win 10 from branching on this codepath, as Win10 has been descoped from this launch. - Correcting several small but critical bugs surfaced by session restore path: - Unexpected content in Guest Mode. - NTP showing unexpectedly on restored sessions. - Pinned tabs appearing duplicated on restored sessions. BUG=660210 ========== to ========== Completing refactor of startup_browser_creator_impl. This includes: - Reintroducing session restore logic. - Preventing Win 10 from branching on this codepath, as Win10 has been descoped from this launch. - Correcting several small but critical bugs surfaced by session restore path: - Unexpected content in Guest Mode. - NTP showing unexpectedly on restored sessions. - Pinned tabs appearing duplicated on restored sessions. BUG=660210 Committed: https://crrev.com/43f9c50bcb8b440c0f462b69f13b4bf9994e639b Cr-Commit-Position: refs/heads/master@{#429303} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/43f9c50bcb8b440c0f462b69f13b4bf9994e639b Cr-Commit-Position: refs/heads/master@{#429303}
Message was sent while issue was closed.
https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:750: // prevent a crash, use the NTP if |tabs| is empty. On 2016/11/02 16:33:59, tmartino wrote: > On 2016/10/31 at 19:24:05, Peter Kasting wrote: > > Nit: "As a fallback to prevent a crash" makes it sound like this is some sort > of error case. Maybe instead of this, say when we'd expect this to be empty? > > It really *is* an error case. I had a comment earlier in the thread when grt was > reviewing that explained this further: > > This seems gross and unnecessary; however, I was able to trigger this crash at > least once, so I think it's a worthwhile fallback. > > The scenario where it crashed was obscure: using the --restore-last-session flag > on a profile without the restore pref set, while Chrome process was already > running. We view this as a restore desired (the pref comes back as Type::LAST) > but for some reason downstream, the restore doesn't happen. > > Since that flag is only meant to work at startup per its documentation, I'm not > too worried that there's a substantial flaw in our logic here. But I do think > it's better to launch just the NTP than to crash in this situation (and any > other session restore mismatches that might pop up). That also preserves > existing behavior--I'm not sure what sequence of steps is invoked in the > existing logic, but the end result is that the NTP is displayed instead of a > session restore. Can we add this case to the comment here? For weird obscure code it's always good to have a description of the actual triggering flow, so ten years later someone can retest to see whether this is still needed. By "error case" I meant "it sounds like you're working around someone else's bug", but the description you give just sounds like this is an obscure/unexpected use. (But maybe you still are saying this is someone else's bug? Should we be detecting and handling this differently somewhere else?) https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:145: uint32_t restore_options, On 2016/11/02 16:34:00, tmartino wrote: > On 2016/10/31 at 19:24:06, Peter Kasting wrote: > > Nit: Here and in DetermineSynchronousRestoreOptions() below we use a uint32_t > to refer to some nebulous "restore options". It took me a while to figure out > this is just SessionRestore::Behavior (but expressed as a uint32_t, I guess to > emphasize it could be a bitmask of any of those values. > > > > It would be nice if there was a type alias for this type, defined in > session_restore.h, so that functions could pass that around instead of uint32_t, > and its meaning would be clear. > > Ack. Made a note to self to look at this when I make a cleanup pass post-launch, > but I'd rather not let /browser/sessions creep into scope right now. SGTM
Message was sent while issue was closed.
https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:750: // prevent a crash, use the NTP if |tabs| is empty. On 2016/11/02 at 17:00:46, Peter Kasting wrote: > On 2016/11/02 16:33:59, tmartino wrote: > > On 2016/10/31 at 19:24:05, Peter Kasting wrote: > > > Nit: "As a fallback to prevent a crash" makes it sound like this is some sort > > of error case. Maybe instead of this, say when we'd expect this to be empty? > > > > It really *is* an error case. I had a comment earlier in the thread when grt was > > reviewing that explained this further: > > > > This seems gross and unnecessary; however, I was able to trigger this crash at > > least once, so I think it's a worthwhile fallback. > > > > The scenario where it crashed was obscure: using the --restore-last-session flag > > on a profile without the restore pref set, while Chrome process was already > > running. We view this as a restore desired (the pref comes back as Type::LAST) > > but for some reason downstream, the restore doesn't happen. > > > > Since that flag is only meant to work at startup per its documentation, I'm not > > too worried that there's a substantial flaw in our logic here. But I do think > > it's better to launch just the NTP than to crash in this situation (and any > > other session restore mismatches that might pop up). That also preserves > > existing behavior--I'm not sure what sequence of steps is invoked in the > > existing logic, but the end result is that the NTP is displayed instead of a > > session restore. > > Can we add this case to the comment here? For weird obscure code it's always good to have a description of the actual triggering flow, so ten years later someone can retest to see whether this is still needed. > > By "error case" I meant "it sounds like you're working around someone else's bug", but the description you give just sounds like this is an obscure/unexpected use. (But maybe you still are saying this is someone else's bug? Should we be detecting and handling this differently somewhere else?) Sorry for CQing before responding--I thought I had sent these responses last night. https://codereview.chromium.org/2469363002
Message was sent while issue was closed.
https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:618: bool is_incognito_or_guest = On 2016/11/02 16:33:59, tmartino wrote: > On 2016/10/31 at 19:24:06, Peter Kasting wrote: > > Nit: Do you think it would be any clearer to reverse the sense of this bool > (|is_regular_profile|) and pass that around to various places instead? On one > hand I think "is A or B" is inherently an awkward concept to reason about, on > the other hand "regular profile" is kind of nebulous. > > > > Maybe there's a phrase which describes the common nature of incognito/guest > profiles, e.g. "is_tempory_profile" or "is_ephemeral_profile" or something? > > Definitely don't like "regular", but I'm OK with "ephemeral profile". Note that an "ephemeral profile" is a thing in and of itself (https://cs.chromium.org/chromium/src/chrome/browser/chromeos/profiles/profile...). Please only use that term if it truly applies here. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:747: StartupBrowserCreator::in_synchronous_profile_launch_ = true; On 2016/11/02 16:33:59, tmartino wrote: > On 2016/10/31 at 19:24:05, Peter Kasting wrote: > > Nit: I suggest using base::AutoReset to make clear this is a "set for limited > scope". This also makes it harder to add an early-return later that jumps out > before we reset this variable. > > > > { > > base::AutoReset<bool> synchronous_launch_resetter( > > &StartupBrowserCreator::in_synchronous_profile_launch_, true); > > > > browser = OpenTabsInBrowser(...); > > } > > > > (Also, is it just me, or is it really weird that a class called > "StartupBrowserCreatorImpl" does not inherit from "StartupBrowserCreator", so we > have to qualify the name of this member?) > > Done. > > And, yeah, I don't care for the structure of these two classes. It feels like > the Impl class isn't so much an implementation as "more functions we moved out > of the class because it was getting unwieldy." My favorite part of this class's "impl"-ness is the fact that it's used by code outside of ui/startup: https://cs.chromium.org/chromium/src/chrome/browser/app_controller_mac.mm?typ.... Barf. https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:150: static BrowserOpenBehavior DetermineBrowserOpenBehavior( On 2016/11/02 16:34:00, tmartino wrote: > On 2016/10/31 at 19:24:06, Peter Kasting wrote: > > Nit: I suggest placing static methods either above or below non-static > methods, since they're really more like namespaced globals. (2 places) > > Agreed. Moved them to be the last non-deprecated functions. Minor note: the style guide doesn't dictate where static methods should go in relation to non-static methods (https://google.github.io/styleguide/cppguide.html#Declaration_Order). I interpret this to mean "put them where it makes the most sense given the class as a whole."
Message was sent while issue was closed.
https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:618: bool is_incognito_or_guest = On 2016/11/03 at 09:43:33, grt (UTC plus 1) wrote: > On 2016/11/02 16:33:59, tmartino wrote: > > On 2016/10/31 at 19:24:06, Peter Kasting wrote: > > > Nit: Do you think it would be any clearer to reverse the sense of this bool > > (|is_regular_profile|) and pass that around to various places instead? On one > > hand I think "is A or B" is inherently an awkward concept to reason about, on > > the other hand "regular profile" is kind of nebulous. > > > > > > Maybe there's a phrase which describes the common nature of incognito/guest > > profiles, e.g. "is_tempory_profile" or "is_ephemeral_profile" or something? > > > > Definitely don't like "regular", but I'm OK with "ephemeral profile". > > Note that an "ephemeral profile" is a thing in and of itself (https://cs.chromium.org/chromium/src/chrome/browser/chromeos/profiles/profile...). Please only use that term if it truly applies here. Changing in forthcoming CL. I see the term "off the record" being thrown around in the code; would that be an appropriate term for "incognito/guest" or does that, too, mean something specific?
Message was sent while issue was closed.
https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:618: bool is_incognito_or_guest = On 2016/11/03 at 09:43:33, grt (UTC plus 1) wrote: > On 2016/11/02 16:33:59, tmartino wrote: > > On 2016/10/31 at 19:24:06, Peter Kasting wrote: > > > Nit: Do you think it would be any clearer to reverse the sense of this bool > > (|is_regular_profile|) and pass that around to various places instead? On one > > hand I think "is A or B" is inherently an awkward concept to reason about, on > > the other hand "regular profile" is kind of nebulous. > > > > > > Maybe there's a phrase which describes the common nature of incognito/guest > > profiles, e.g. "is_tempory_profile" or "is_ephemeral_profile" or something? > > > > Definitely don't like "regular", but I'm OK with "ephemeral profile". > > Note that an "ephemeral profile" is a thing in and of itself (https://cs.chromium.org/chromium/src/chrome/browser/chromeos/profiles/profile...). Please only use that term if it truly applies here. Changing in forthcoming CL. I see the term "off the record" being thrown around in the code; would that be an appropriate term for "incognito/guest" or does that, too, mean something specific?
Message was sent while issue was closed.
https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:618: bool is_incognito_or_guest = On 2016/11/03 18:39:24, tmartino wrote: > On 2016/11/03 at 09:43:33, grt (UTC plus 1) wrote: > > On 2016/11/02 16:33:59, tmartino wrote: > > > On 2016/10/31 at 19:24:06, Peter Kasting wrote: > > > > Nit: Do you think it would be any clearer to reverse the sense of this > bool > > > (|is_regular_profile|) and pass that around to various places instead? On > one > > > hand I think "is A or B" is inherently an awkward concept to reason about, > on > > > the other hand "regular profile" is kind of nebulous. > > > > > > > > Maybe there's a phrase which describes the common nature of > incognito/guest > > > profiles, e.g. "is_tempory_profile" or "is_ephemeral_profile" or something? > > > > > > Definitely don't like "regular", but I'm OK with "ephemeral profile". > > > > Note that an "ephemeral profile" is a thing in and of itself > (https://cs.chromium.org/chromium/src/chrome/browser/chromeos/profiles/profile...). > Please only use that term if it truly applies here. > > Changing in forthcoming CL. > > I see the term "off the record" being thrown around in the code; would that be > an appropriate term for "incognito/guest" or does that, too, mean something > specific? "off the record" is an archaic term for incognito. It is confusingly unclear whether it means guest or not; I would avoid it. (Is an incognito profile not actually an ephemeral profile, then? I had a hard time telling whether it would actually count as one in the linked function, since it wasn't checked directly.) https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.h (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.h:150: static BrowserOpenBehavior DetermineBrowserOpenBehavior( On 2016/11/03 09:43:34, grt (UTC plus 1) wrote: > On 2016/11/02 16:34:00, tmartino wrote: > > On 2016/10/31 at 19:24:06, Peter Kasting wrote: > > > Nit: I suggest placing static methods either above or below non-static > > methods, since they're really more like namespaced globals. (2 places) > > > > Agreed. Moved them to be the last non-deprecated functions. > > Minor note: the style guide doesn't dictate where static methods should go in > relation to non-static methods > (https://google.github.io/styleguide/cppguide.html#Declaration_Order). I > interpret this to mean "put them where it makes the most sense given the class > as a whole." Yes; there is no style rule on this. The majority of Chrome code I've seen seems to keep them together above or below non-statics, but it's not required.
Message was sent while issue was closed.
https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/2457653003/diff/40001/chrome/browser/ui/start... chrome/browser/ui/startup/startup_browser_creator_impl.cc:618: bool is_incognito_or_guest = On 2016/11/03 21:46:42, Peter Kasting wrote: > On 2016/11/03 18:39:24, tmartino wrote: > > On 2016/11/03 at 09:43:33, grt (UTC plus 1) wrote: > > > On 2016/11/02 16:33:59, tmartino wrote: > > > > On 2016/10/31 at 19:24:06, Peter Kasting wrote: > > > > > Nit: Do you think it would be any clearer to reverse the sense of this > > bool > > > > (|is_regular_profile|) and pass that around to various places instead? On > > one > > > > hand I think "is A or B" is inherently an awkward concept to reason about, > > on > > > > the other hand "regular profile" is kind of nebulous. > > > > > > > > > > Maybe there's a phrase which describes the common nature of > > incognito/guest > > > > profiles, e.g. "is_tempory_profile" or "is_ephemeral_profile" or > something? > > > > > > > > Definitely don't like "regular", but I'm OK with "ephemeral profile". > > > > > > Note that an "ephemeral profile" is a thing in and of itself > > > (https://cs.chromium.org/chromium/src/chrome/browser/chromeos/profiles/profile...). > > Please only use that term if it truly applies here. > > > > Changing in forthcoming CL. > > > > I see the term "off the record" being thrown around in the code; would that be > > an appropriate term for "incognito/guest" or does that, too, mean something > > specific? > > "off the record" is an archaic term for incognito. It is confusingly unclear > whether it means guest or not; I would avoid it. > > (Is an incognito profile not actually an ephemeral profile, then? I'm not sure. I had a memory of hearing about "ephemeral profiles" a year or more ago and did a little code search. > I had a hard > time telling whether it would actually count as one in the linked function, > since it wasn't checked directly.) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
