Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1308833004:
Show Goodies page to new Chromebook users (Closed)
|
Created:
5 years, 3 months ago by Greg Levin Modified:
5 years, 2 months ago Reviewers:
achuithb CC:
chromium-reviews, dzhioev+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow Goodies page to new Chromebook users
BUG=520119
TEST=Open new empty browser window for the first time on a new
Chromebook (non-enterprise, logged-in user). The Goodies page
should open in a second, inactive tab.
Patch Set 1 #
Total comments: 9
Patch Set 2 : Move code into goodies_displayer #
Total comments: 3
Patch Set 3 : Use LazyInstance<GoodiesDisplayer> #
Total comments: 28
Patch Set 4 : Address reviewer comments #
Total comments: 12
Patch Set 5 : Reviewer comments: Round 2 #Patch Set 6 : Reviewer comments: Round 3 #
Total comments: 8
Patch Set 7 : Reviewer comments: Round 4 #
Total comments: 2
Patch Set 8 : Add GoodiesDisplayer::Delegate for future testing #
Total comments: 23
Patch Set 9 : Change delegate from interface to implementation #
Total comments: 10
Messages
Total messages: 26 (5 generated)
glevin@chromium.org changed reviewers: + anthonyvd@chromium.org, dzhioev@chromium.org, pkasting@chromium.org
Please have a look! chrome/browser/chromeos/login/* -dzhioev@ chrome/browser/profiles/* -anthonyvd@ chrome/browser/ui/* chrome/common/* -pkasting@ See go/cros-goodies-prompt-dd for some explanation of what I'm doing and why, if you're interested. Also, note that the implementation differs a little from what's described in the issue. We've since decided to open the Goodies page in a second tab along side the displayed New Tab page, rather than in place of it (this change only affects browser_commands.cc). https://codereview.chromium.org/1308833004/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1308833004/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:1171: CheckCanShowGoodiesPref(); My choice to put this here was a little arbitrary. It just needs to go *somewhere* during session or browser startup. I chose here because (i) the browser thread has started by this point, (ii) there's other first-run-type URL/info stuff going on here, and (iii) this class already has a RegisterPrefs() function. However, I'd be open to suggestions for better places to put it. https://codereview.chromium.org/1308833004/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/1308833004/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_impl.cc:1226: GURL ProfileImpl::ShowingGoodiesPageInNewWindow() { This logic could probably go somewhere other than ProfileImpl. This just seemed like a convenient way of screening out undesirable profile types (i.e. incognito & guest) w/o explicit checks. https://codereview.chromium.org/1308833004/diff/1/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/1308833004/diff/1/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:360: AddTabAt(browser, goodies_url, 2, false); At the moment, this only shows Goodies when user opens a new, blank browser window. It would probably be better to add the second tab the first time they open *any* browser window. I could move this code into Navigate() in browser_navigator.cc, but that felt too... heavy-handed? Intrusive? I'd appreciate a suggestion for how open up a second tab the first time the browser is shown (subject to restrictions enumerated in ShowingGoodiesPageInNewWindow()).
https://codereview.chromium.org/1308833004/diff/1/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/1308833004/diff/1/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:360: AddTabAt(browser, goodies_url, 2, false); On 2015/09/09 23:20:33, Greg Levin wrote: > At the moment, this only shows Goodies when user opens a new, blank browser > window. It would probably be better to add the second tab the first time they > open *any* browser window. I could move this code into Navigate() in > browser_navigator.cc, but that felt too... heavy-handed? Intrusive? I'd > appreciate a suggestion for how open up a second tab the first time the browser > is shown (subject to restrictions enumerated in > ShowingGoodiesPageInNewWindow()). I don't know. Both these locations seem wrong. How does the code that shows tabs on firstrun work? Don't we want to do something similar?
https://codereview.chromium.org/1308833004/diff/1/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/1308833004/diff/1/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:360: AddTabAt(browser, goodies_url, 2, false); On 2015/09/10 00:10:05, Peter Kasting wrote: > On 2015/09/09 23:20:33, Greg Levin wrote: > > At the moment, this only shows Goodies when user opens a new, blank browser > > window. It would probably be better to add the second tab the first time they > > open *any* browser window. I could move this code into Navigate() in > > browser_navigator.cc, but that felt too... heavy-handed? Intrusive? I'd > > appreciate a suggestion for how open up a second tab the first time the > browser > > is shown (subject to restrictions enumerated in > > ShowingGoodiesPageInNewWindow()). > > I don't know. Both these locations seem wrong. How does the code that shows > tabs on firstrun work? Don't we want to do something similar? The first-run pages I know about are the ChromeVox manual (shown when CV is active during login) and the Help Center (which is opened instead of Tour app when not using OFFICIAL_BUILD). Both of these replace the Tour app, which isn't an option here; nor do we want to open both the Tour and the Goodies page simultaneously. The consensus was to wait until the user manually opened a browser. Is there some kind of Navigation observer class I could use?
https://codereview.chromium.org/1308833004/diff/1/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/1308833004/diff/1/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:360: AddTabAt(browser, goodies_url, 2, false); On 2015/09/10 13:20:21, Greg Levin wrote: > On 2015/09/10 00:10:05, Peter Kasting wrote: > > On 2015/09/09 23:20:33, Greg Levin wrote: > > > At the moment, this only shows Goodies when user opens a new, blank browser > > > window. It would probably be better to add the second tab the first time > they > > > open *any* browser window. I could move this code into Navigate() in > > > browser_navigator.cc, but that felt too... heavy-handed? Intrusive? I'd > > > appreciate a suggestion for how open up a second tab the first time the > > browser > > > is shown (subject to restrictions enumerated in > > > ShowingGoodiesPageInNewWindow()). > > > > I don't know. Both these locations seem wrong. How does the code that shows > > tabs on firstrun work? Don't we want to do something similar? > > The first-run pages I know about are the ChromeVox manual (shown when CV is > active during login) and the Help Center (which is opened instead of Tour app > when not using OFFICIAL_BUILD). Both of these replace the Tour app, which isn't > an option here; nor do we want to open both the Tour and the Goodies page > simultaneously. The consensus was to wait until the user manually opened a > browser. Is there some kind of Navigation observer class I could use? The bug suggests this should be in the foreground tab, not a background one. Implementing that might be easier as you could simply default the home page to this page and change the homepage once this successfully showed. Is there somewhere that explains why you're using an inactive background tab, which is unlikely to be noticed by most users? If you need a navigation observer you could ask creis, but I'd rather not hook every navigation. It seems like it might be better to instead modify the startup flow similar to the above, where out of the box the startup pages are set to "new tab, goodies page" and right after launch we reset the startup settings.
https://codereview.chromium.org/1308833004/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/1308833004/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_impl.cc:1226: GURL ProfileImpl::ShowingGoodiesPageInNewWindow() { On 2015/09/09 23:20:33, Greg Levin wrote: > This logic could probably go somewhere other than ProfileImpl. This just seemed > like a convenient way of screening out undesirable profile types (i.e. incognito > & guest) w/o explicit checks. Agreed. Since it's not a profile-specific pref I would be more inclined to move this to profile_window (where we tend to put similar code related to showing things to the user e.g. ShouldShow*Tutorial()) and explicitly check for Incognito and Guest by passing in a Profile*.
Please have another look! I've completely refactored this, and moved all code into a new file / class. It responds to BrowserListObserver::OnBrowserSetLastActive(), but only ever once, then removes itself from the observer list. I'm observing this as opposed to OnBrowserAdded(), because that would add the Goodies tab to the front of the tab strip. This way still puts it at the front of the tab strip when opening an app (e.g. gmail) as opposed to New Tab. Not sure why; doesn't seem too bad, but would like to fix. dzhioev@ - The code is now pretty much all in /chrome/browser/chromeos/first_run/, for which you're the sole owner. I notice you're gardening, though. Please let me know if you won't be able to get to this quickly, as we're trying to get it done by feature freeze. anthonyvd@ & pkasting@ - There's no code left in your owned directories, so you're welcome to remove yourselves as reviewers, or stick around if you'd like to continue helping out. Thanks for the good input, either way! https://codereview.chromium.org/1308833004/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/1308833004/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_impl.cc:1226: GURL ProfileImpl::ShowingGoodiesPageInNewWindow() { On 2015/09/14 13:59:35, anthonyvd wrote: > On 2015/09/09 23:20:33, Greg Levin wrote: > > This logic could probably go somewhere other than ProfileImpl. This just > seemed > > like a convenient way of screening out undesirable profile types (i.e. > incognito > > & guest) w/o explicit checks. > > Agreed. Since it's not a profile-specific pref I would be more inclined to move > this to profile_window (where we tend to put similar code related to showing > things to the user e.g. ShouldShow*Tutorial()) and explicitly check for > Incognito and Guest by passing in a Profile*. Done (Moved all code into new file / class). https://codereview.chromium.org/1308833004/diff/1/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/1308833004/diff/1/chrome/browser/ui/browser_c... chrome/browser/ui/browser_commands.cc:360: AddTabAt(browser, goodies_url, 2, false); On 2015/09/10 20:39:35, Peter Kasting wrote: > On 2015/09/10 13:20:21, Greg Levin wrote: > > On 2015/09/10 00:10:05, Peter Kasting wrote: > > > On 2015/09/09 23:20:33, Greg Levin wrote: > > > > At the moment, this only shows Goodies when user opens a new, blank > browser > > > > window. It would probably be better to add the second tab the first time > > they > > > > open *any* browser window. I could move this code into Navigate() in > > > > browser_navigator.cc, but that felt too... heavy-handed? Intrusive? I'd > > > > appreciate a suggestion for how open up a second tab the first time the > > > browser > > > > is shown (subject to restrictions enumerated in > > > > ShowingGoodiesPageInNewWindow()). > > > > > > I don't know. Both these locations seem wrong. How does the code that > shows > > > tabs on firstrun work? Don't we want to do something similar? > > > > The first-run pages I know about are the ChromeVox manual (shown when CV is > > active during login) and the Help Center (which is opened instead of Tour app > > when not using OFFICIAL_BUILD). Both of these replace the Tour app, which > isn't > > an option here; nor do we want to open both the Tour and the Goodies page > > simultaneously. The consensus was to wait until the user manually opened a > > browser. Is there some kind of Navigation observer class I could use? > > The bug suggests this should be in the foreground tab, not a background one. > Implementing that might be easier as you could simply default the home page to > this page and change the homepage once this successfully showed. Is there > somewhere that explains why you're using an inactive background tab, which is > unlikely to be noticed by most users? > > If you need a navigation observer you could ask creis, but I'd rather not hook > every navigation. It seems like it might be better to instead modify the > startup flow similar to the above, where out of the box the startup pages are > set to "new tab, goodies page" and right after launch we reset the startup > settings. Done (Moved all code into new file / class, which observes the BrowserList). https://codereview.chromium.org/1308833004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:53: new GoodiesDisplayer(); // TODO: Leaky? This probably isn't finished, but I wanted to get this big change uploaded for people to start looking at. This should be a relatively small change. This object deletes itself once it's been used, but if a user never opens a browser, it may not get cleaned up. I was thinking of adding a global var in this file to keep track of it, and a function to delete it that can be called in session shutdown. Any additional/alternate suggestions welcome. https://codereview.chromium.org/1308833004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1308833004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:354: registry->RegisterBooleanPref(prefs::kCanShowOobeGoodiesPage, true); I don't know the rules for where to register prefs. Is it okay to register this here, since it's "near" where it's used, or should I add a RegisterPrefs() function in goodies_displayer.cc?
Now using a LazyInstance of GoodiesDisplayer. I don't know if this is the *best* way to manage a single instance of this class, but it seems to work as needed. I'm no longer deleting the GoodiesDisplayer once it's used, but since it's a very small class and never gets created after a browser is first displayed, I don't think it matters. https://codereview.chromium.org/1308833004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:53: new GoodiesDisplayer(); // TODO: Leaky? On 2015/09/15 00:12:11, Greg Levin wrote: > This probably isn't finished, but I wanted to get this big change uploaded for > people to start looking at. This should be a relatively small change. > This object deletes itself once it's been used, but if a user never opens a > browser, it may not get cleaned up. I was thinking of adding a global var in > this file to keep track of it, and a function to delete it that can be called in > session shutdown. Any additional/alternate suggestions welcome. Done (fixed in Patch Set 3).
achuith@chromium.org changed reviewers: + achuith@chromium.org
https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:38: static bool CheckGoodiesPrefAgainstOobeTimestamp() { Don't need static. anon namespace functions do not have external linkage https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:39: DCHECK_CURRENTLY_ON(content::BrowserThread::FILE); BrowserThread::FILE is deprecated. You should use BlockingPool https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:44: base::TimeDelta time_since_oobe = const https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:55: static void UpdateGoodiesPrefCantShow(bool can_show_goodies) { No static https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:59: PrefService* prefs = g_browser_process->local_state(); No reason to have this temporary variable prefs https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:79: DCHECK(local_state); No point in having this DCHECK. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:103: content::BrowserThread::PostTaskAndReplyWithResult( Use BlockingPool https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.h (right): https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.h:14: #if defined(OS_CHROMEOS) Pretty sure you don't need this - gyp file should be taking care of this? https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.h:24: void OnBrowserSetLastActive(Browser* browser) override; Add comment like: // BrowserListObserver overrides. Also, make this private. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.h:31: void InitializeGoodiesDisplayer(); Make this a static member of GoodiesDisplayer, and call it Init https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/startup_utils.cc (right): https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/startup_utils.cc:113: base::FilePath oobe_complete_path = StartupUtils::GetOobeCompleteFlagPath(); const
https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:31: base::LazyInstance<GoodiesDisplayer> g_goodies_displayer = I don't see any reason why this should be a singleton. You can create this object in UpdateGoodiesPrefCantShow, and DeleteSoon it OnBrowserSetLastActive
https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:31: base::LazyInstance<GoodiesDisplayer> g_goodies_displayer = On 2015/09/15 18:24:32, achuithb wrote: > I don't see any reason why this should be a singleton. > > You can create this object in UpdateGoodiesPrefCantShow, and DeleteSoon it > OnBrowserSetLastActive My concern was the case when User A logs in and out w/o showing browser, then User B logs in. This would create a second instance, and unless I cleaned up the first one on logout (and user switch, if User A logs back in via Multiuser from B's session), they'd both be observing. It seemed easier to have a singleton, since it will use whichever profile is attached to the browser window. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:38: static bool CheckGoodiesPrefAgainstOobeTimestamp() { On 2015/09/15 18:20:54, achuithb wrote: > Don't need static. anon namespace functions do not have external linkage Done. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:39: DCHECK_CURRENTLY_ON(content::BrowserThread::FILE); On 2015/09/15 18:20:54, achuithb wrote: > BrowserThread::FILE is deprecated. You should use BlockingPool Done. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:44: base::TimeDelta time_since_oobe = On 2015/09/15 18:20:54, achuithb wrote: > const Done. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:55: static void UpdateGoodiesPrefCantShow(bool can_show_goodies) { On 2015/09/15 18:20:54, achuithb wrote: > No static Done. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:59: PrefService* prefs = g_browser_process->local_state(); On 2015/09/15 18:20:54, achuithb wrote: > No reason to have this temporary variable prefs Done. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:79: DCHECK(local_state); On 2015/09/15 18:20:53, achuithb wrote: > No point in having this DCHECK. Done. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:103: content::BrowserThread::PostTaskAndReplyWithResult( On 2015/09/15 18:20:53, achuithb wrote: > Use BlockingPool Done. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.h (right): https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.h:14: #if defined(OS_CHROMEOS) On 2015/09/15 18:20:54, achuithb wrote: > Pretty sure you don't need this - gyp file should be taking care of this? Yep, good call! https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.h:24: void OnBrowserSetLastActive(Browser* browser) override; On 2015/09/15 18:20:54, achuithb wrote: > Add comment like: > // BrowserListObserver overrides. > > Also, make this private. Done. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.h:31: void InitializeGoodiesDisplayer(); On 2015/09/15 18:20:54, achuithb wrote: > Make this a static member of GoodiesDisplayer, and call it Init Done. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/startup_utils.cc (right): https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/startup_utils.cc:113: base::FilePath oobe_complete_path = StartupUtils::GetOobeCompleteFlagPath(); On 2015/09/15 18:20:54, achuithb wrote: > const Done.
https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:31: base::LazyInstance<GoodiesDisplayer> g_goodies_displayer = On 2015/09/15 21:44:48, Greg Levin wrote: > On 2015/09/15 18:24:32, achuithb wrote: > > I don't see any reason why this should be a singleton. > > > > You can create this object in UpdateGoodiesPrefCantShow, and DeleteSoon it > > OnBrowserSetLastActive > > My concern was the case when User A logs in and out w/o showing browser, then > User B logs in. This would create a second instance, and unless I cleaned up > the first one on logout (and user switch, if User A logs back in via Multiuser > from B's session), they'd both be observing. It seemed easier to have a > singleton, since it will use whichever profile is attached to the browser > window. Hmm, are you sure that's possible? What about deleting OnBrowserRemoved? This isn't really a singleton, it just feels like we're trying to punt the lifetime management problem. https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:81: // removes itself from BrowserListObservers, and deletes itself. Where does it delete itself? Why not delete at the end of this? https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:93: ->IsEnterpriseManaged()) I believe you need {} when the if stmt is multi-line? Not sure https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/startup_utils.cc (right): https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/startup_utils.cc:98: base::FilePath StartupUtils::GetOobeCompleteFlagPath() { You need a comment like // static before this https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/startup_utils.h (right): https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/startup_utils.h:11: #include "base/files/file_util.h" file_path.h?
https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:31: base::LazyInstance<GoodiesDisplayer> g_goodies_displayer = |> Hmm, are you sure that's possible? Yes, I've tested it. Since I'm calling GoodiesDisplayer::Init() from UserSessionManager::FinalizePrepareProfile(), it does get called for each user login, and if a second user logs in before a browser has been opened, this will try to Init() a second GoodiesDisplayer while the first one is still running. Since we only need one, a singleton seemed appropriate. |> What about deleting OnBrowserRemoved? GoodiesDisplayer::OnBrowserRemoved() would never be called. It's called when a browser window is closed, but the observer would have been removed before that, when that browser window was opened. |> This isn't really a singleton, it just feels like we're trying to punt the |> lifetime management problem. I don't have much experience with singletons in our code, so I'm not sure why not. It doesn't really belong to any other class, and we don't need (and in fact, don't want) more than one. And we need to keep it around, possibly across multiple simultaneous sessions, until it gets used. https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:81: // removes itself from BrowserListObservers, and deletes itself. |> Where does it delete itself? Currently, it doesn't; it will be deleted on program exit. Certainly not ideal, but as it's a memberless class that only gets created once in a device's lifetime, not that harmful, either. |> Why not delete at the end of this? Wasn't sure how to delete a LazyInstance... g_goodies_displayer = LAZY_INSTANCE_INITIALIZER; doesn't work, and delete this; segfaults. As I said in previous comments, LazyInstance may not be the best choice here (still learning singletons in Chrome); it just seemed to be about what I needed. https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:93: ->IsEnterpriseManaged()) On 2015/09/16 03:40:29, achuithb wrote: > I believe you need {} when the if stmt is multi-line? Not sure All I found was this: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals "In general, curly braces are not required for single-line statements, but they are allowed if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces" To me, adding the braces made it look more cluttered as opposed to more readable. But there may be other style conventions that apply that I'm not aware of. (And I could just add them if that's your preference :-) https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/startup_utils.cc (right): https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/startup_utils.cc:98: base::FilePath StartupUtils::GetOobeCompleteFlagPath() { On 2015/09/16 03:40:29, achuithb wrote: > You need a comment like // static before this Done. https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/startup_utils.h (right): https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/startup_utils.h:11: #include "base/files/file_util.h" On 2015/09/16 03:40:29, achuithb wrote: > file_path.h? Done.
https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:31: base::LazyInstance<GoodiesDisplayer> g_goodies_displayer = On 2015/09/16 16:55:13, Greg Levin wrote: > |> Hmm, are you sure that's possible? > > Yes, I've tested it. Since I'm calling GoodiesDisplayer::Init() from > UserSessionManager::FinalizePrepareProfile(), it does get called for each user > login, and if a second user logs in before a browser has been opened, this will > try to Init() a second GoodiesDisplayer while the first one is still running. > Since we only need one, a singleton seemed appropriate. > > |> What about deleting OnBrowserRemoved? > > GoodiesDisplayer::OnBrowserRemoved() would never be called. It's called when a > browser window is closed, but the observer would have been removed before that, > when that browser window was opened. > > |> This isn't really a singleton, it just feels like we're trying to punt the > |> lifetime management problem. > > I don't have much experience with singletons in our code, so I'm not sure why > not. It doesn't really belong to any other class, and we don't need (and in > fact, don't want) more than one. And we need to keep it around, possibly across > multiple simultaneous sessions, until it gets used. I view singletons as being some kind of service that gets launched and is used from multiple places multiple times. Here you seem to have a one-time task that we haven't found a good destruction point for. Singletons are considered an anti-pattern and are supposed to be used sparingly. I'm sure we have signals for when the profile switches, which seems to be a good place to delete this object? If one of your other reviewers is fine with the use of a singleton here, I can lg this change, but personally I think we can do better. https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:81: // removes itself from BrowserListObservers, and deletes itself. On 2015/09/16 16:55:13, Greg Levin wrote: > |> Where does it delete itself? > > Currently, it doesn't; it will be deleted on program exit. Certainly not ideal, > but as it's a memberless class that only gets created once in a device's > lifetime, not that harmful, either. > delete on program exit is more commonly called a leak ;) > |> Why not delete at the end of this? > > Wasn't sure how to delete a LazyInstance... > g_goodies_displayer = LAZY_INSTANCE_INITIALIZER; > doesn't work, and > delete this; > segfaults. As I said in previous comments, LazyInstance may not be the best > choice here (still learning singletons in Chrome); it just seemed to be about > what I needed. Yeah, I don't have experience with LAZY_INSTANCE_INITIALIZER either. I was hoping you could simply delete it after it's single use. https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:93: ->IsEnterpriseManaged()) On 2015/09/16 16:55:13, Greg Levin wrote: > On 2015/09/16 03:40:29, achuithb wrote: > > I believe you need {} when the if stmt is multi-line? Not sure > > All I found was this: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals > "In general, curly braces are not required for single-line statements, but they > are allowed if you like them; conditional or loop statements with complex > conditions or statements may be more readable with curly braces" > > To me, adding the braces made it look more cluttered as opposed to more > readable. But there may be other style conventions that apply that I'm not > aware of. (And I could just add them if that's your preference :-) I believe there is no consensus on this, so let's leave it as is: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/jPXg9Bg_kOg/33...
Moved g_goodies_displayer to be UserSessionManager::goodies_displayer_. This now gets deleted either after its single use or when UserSessionManager is shutting down. https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:31: base::LazyInstance<GoodiesDisplayer> g_goodies_displayer = On 2015/09/16 17:17:30, achuithb wrote: > On 2015/09/16 16:55:13, Greg Levin wrote: > > |> Hmm, are you sure that's possible? > > > > Yes, I've tested it. Since I'm calling GoodiesDisplayer::Init() from > > UserSessionManager::FinalizePrepareProfile(), it does get called for each user > > login, and if a second user logs in before a browser has been opened, this > will > > try to Init() a second GoodiesDisplayer while the first one is still running. > > Since we only need one, a singleton seemed appropriate. > > > > |> What about deleting OnBrowserRemoved? > > > > GoodiesDisplayer::OnBrowserRemoved() would never be called. It's called when > a > > browser window is closed, but the observer would have been removed before > that, > > when that browser window was opened. > > > > |> This isn't really a singleton, it just feels like we're trying to punt the > > |> lifetime management problem. > > > > I don't have much experience with singletons in our code, so I'm not sure why > > not. It doesn't really belong to any other class, and we don't need (and in > > fact, don't want) more than one. And we need to keep it around, possibly > across > > multiple simultaneous sessions, until it gets used. > > I view singletons as being some kind of service that gets launched and is used > from multiple places multiple times. > > Here you seem to have a one-time task that we haven't found a good destruction > point for. > > Singletons are considered an anti-pattern and are supposed to be used sparingly. > > > I'm sure we have signals for when the profile switches, which seems to be a good > place to delete this object? > > If one of your other reviewers is fine with the use of a singleton here, I can > lg this change, but personally I think we can do better. Done- Moved instance to be member of UserSessionManager. Thanks for the insights on this! https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:81: // removes itself from BrowserListObservers, and deletes itself. On 2015/09/16 17:17:30, achuithb wrote: > On 2015/09/16 16:55:13, Greg Levin wrote: > > |> Where does it delete itself? > > > > Currently, it doesn't; it will be deleted on program exit. Certainly not > ideal, > > but as it's a memberless class that only gets created once in a device's > > lifetime, not that harmful, either. > > > > delete on program exit is more commonly called a leak ;) > > > |> Why not delete at the end of this? > > > > Wasn't sure how to delete a LazyInstance... > > g_goodies_displayer = LAZY_INSTANCE_INITIALIZER; > > doesn't work, and > > delete this; > > segfaults. As I said in previous comments, LazyInstance may not be the best > > choice here (still learning singletons in Chrome); it just seemed to be about > > what I needed. > > Yeah, I don't have experience with LAZY_INSTANCE_INITIALIZER either. I was > hoping you could simply delete it after it's single use. Done. https://codereview.chromium.org/1308833004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/first_run/goodies_displayer.cc:93: ->IsEnterpriseManaged()) On 2015/09/16 17:17:30, achuithb wrote: > On 2015/09/16 16:55:13, Greg Levin wrote: > > On 2015/09/16 03:40:29, achuithb wrote: > > > I believe you need {} when the if stmt is multi-line? Not sure > > > > All I found was this: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals > > "In general, curly braces are not required for single-line statements, but > they > > are allowed if you like them; conditional or loop statements with complex > > conditions or statements may be more readable with curly braces" > > > > To me, adding the braces made it look more cluttered as opposed to more > > readable. But there may be other style conventions that apply that I'm not > > aware of. (And I could just add them if that's your preference :-) > > I believe there is no consensus on this, so let's leave it as is: > https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/jPXg9Bg_kOg/33... Acknowledged.
glevin@chromium.org changed reviewers: - anthonyvd@chromium.org, dzhioev@chromium.org, pkasting@chromium.org
achuith@chromium.org changed reviewers: + anthonyvd@chromium.org, dzhioev@chromium.org, pkasting@chromium.org
Looking at UserSessionManager, it really doesn't look like the right place for this :( I tried to look for some other first run stuff, but nothing really made sense. Could you also please look? Also, this needs browser tests. https://codereview.chromium.org/1308833004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:36: base::File::Info fileInfo; Add a DCHECK for BlockingPool: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); https://codereview.chromium.org/1308833004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:50: if (can_show_goodies) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); https://codereview.chromium.org/1308833004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/1308833004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.h:45: } // namespace first_run Don't need comment https://codereview.chromium.org/1308833004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.h:243: void CreateGoodiesDisplayer(); Please add function comments
https://codereview.chromium.org/1308833004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:36: base::File::Info fileInfo; On 2015/09/16 21:51:36, achuithb wrote: > Add a DCHECK for BlockingPool: > DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); Done. https://codereview.chromium.org/1308833004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:50: if (can_show_goodies) { On 2015/09/16 21:51:36, achuithb wrote: > DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Done. https://codereview.chromium.org/1308833004/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/1308833004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.h:45: } // namespace first_run On 2015/09/16 21:51:36, achuithb wrote: > Don't need comment Was being stylistically consistent with the one-line namespaces above and below it. https://codereview.chromium.org/1308833004/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.h:243: void CreateGoodiesDisplayer(); On 2015/09/16 21:51:36, achuithb wrote: > Please add function comments Done.
https://codereview.chromium.org/1308833004/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:39: const base::TimeDelta time_since_oobe = Wonder if it makes sense to add a function to StartupUtils like GetTimeSinceOobeFileCreation instead
glevin@chromium.org changed reviewers: - anthonyvd@chromium.org, dzhioev@chromium.org, pkasting@chromium.org
Here's the code switching needed testing support into GoodiesDisplayer::Delegate. When this is ready, since it's looking less likely that I'll have a browser test by end of day tomorrow, could we go ahead and land this sooner, so I have a little leeway to deal with potential test failures or reverts? https://codereview.chromium.org/1308833004/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:39: const base::TimeDelta time_since_oobe = On 2015/09/17 17:04:41, achuithb wrote: > Wonder if it makes sense to add a function to StartupUtils like > GetTimeSinceOobeFileCreation instead Done. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/first_run/goodies_displayer.h (right): https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:22: virtual ~Delegate(); Is it possible to in-line these? When I tried I got error: [chromium-style] Complex con/destructor has an inlined body. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:31: virtual base::TimeDelta GetTimeSinceOobe(); It appears to be atypical to put implementation in the base delegate class, but that seems to make more sense here. The UserSessionManager and BrowserTest implementations should be identical for everything except GetTimeSinceOobe() (at least, based on my current thoughts). And the base implementation I have for this function is the one "correct" one, so making this function pure virtual and writing this implementation over in UserSessionManager didn't seem right either. I just need to override the "correct" version for testing. That's my intuition here, but my intuition on these things is often wrong :-P https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:47: Delegate* delegate_; Is there a smart pointer type I should use here instead? The delegate owns this object, and should destroy it before the delegate is itself destroyed, so this shouldn't be a problem...?
https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:36: return (delegate->GetTimeSinceOobe() <= Drop outer parens https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:59: if (goodies_displayer_ == nullptr) if (!goodies_displayer_) is more succinct https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/first_run/goodies_displayer.h (right): https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:18: // Primarily used to create, host, and destroy singleton GoodiesDisplayer. Drop Primarily, and singleton. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:22: virtual ~Delegate(); On 2015/09/17 23:21:40, Greg Levin wrote: > Is it possible to in-line these? When I tried I got > > error: [chromium-style] Complex con/destructor has an inlined body. No, you cannot/should not since this class owns a scoped_ptr. You should make the dtor protected. THe scoped_ptr should be owned by UserSessionManager. This class is supposed to just be an interface. A TestDelegate would implement this interface, right? https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:31: virtual base::TimeDelta GetTimeSinceOobe(); On 2015/09/17 23:21:40, Greg Levin wrote: > It appears to be atypical to put implementation in the base delegate class, but > that seems to make more sense here. The UserSessionManager and BrowserTest > implementations should be identical for everything except GetTimeSinceOobe() (at > least, based on my current thoughts). And the base implementation I have for > this function is the one "correct" one, so making this function pure virtual and > writing this implementation over in UserSessionManager didn't seem right either. > I just need to override the "correct" version for testing. > > That's my intuition here, but my intuition on these things is often wrong :-P No, you're not allowed to do this. You can only inherit from pure interface classes. You copy paste the common methods for the TestDelegate, and that's totally fine. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:36: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:47: Delegate* delegate_; On 2015/09/17 23:21:40, Greg Levin wrote: > Is there a smart pointer type I should use here instead? The delegate owns this > object, and should destroy it before the delegate is itself destroyed, so this > shouldn't be a problem...? It shouldn't be a problem, but you should add a comment like: // Pointer to owner. or // Not owned. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.h:87: public first_run::GoodiesDisplayer::Delegate, You cannot inherit from an implementation class. The implementation of Create/Destroy/GetTime have to be UserSessionManager. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/startup_utils.cc (right): https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/startup_utils.cc:116: StartupUtils::GetOobeCompleteFlagPath(); Don't think you need StartupUtils:: https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/startup_utils.h (right): https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/startup_utils.h:41: static base::FilePath GetOobeCompleteFlagPath(); Don't need this any more, right?
https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:36: return (delegate->GetTimeSinceOobe() <= On 2015/09/18 16:39:11, achuithb wrote: > Drop outer parens Done. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:59: if (goodies_displayer_ == nullptr) On 2015/09/18 16:39:11, achuithb wrote: > if (!goodies_displayer_) is more succinct Done. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/first_run/goodies_displayer.h (right): https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:18: // Primarily used to create, host, and destroy singleton GoodiesDisplayer. On 2015/09/18 16:39:11, achuithb wrote: > Drop Primarily, and singleton. Done. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:22: virtual ~Delegate(); On 2015/09/18 16:39:11, achuithb wrote: > On 2015/09/17 23:21:40, Greg Levin wrote: > > Is it possible to in-line these? When I tried I got > > > > error: [chromium-style] Complex con/destructor has an inlined body. > > No, you cannot/should not since this class owns a scoped_ptr. > > You should make the dtor protected. > > THe scoped_ptr should be owned by UserSessionManager. This class is supposed to > just be an interface. A TestDelegate would implement this interface, right? Acknowledged. Didn't make dtor protected since class contains implementation and is instantiated. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:31: virtual base::TimeDelta GetTimeSinceOobe(); On 2015/09/18 16:39:11, achuithb wrote: > On 2015/09/17 23:21:40, Greg Levin wrote: > > It appears to be atypical to put implementation in the base delegate class, > but > > that seems to make more sense here. The UserSessionManager and BrowserTest > > implementations should be identical for everything except GetTimeSinceOobe() > (at > > least, based on my current thoughts). And the base implementation I have for > > this function is the one "correct" one, so making this function pure virtual > and > > writing this implementation over in UserSessionManager didn't seem right > either. > > I just need to override the "correct" version for testing. > > > > That's my intuition here, but my intuition on these things is often wrong :-P > > No, you're not allowed to do this. You can only inherit from pure interface > classes. > > You copy paste the common methods for the TestDelegate, and that's totally fine. Acknowledged. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:36: }; On 2015/09/18 16:39:11, achuithb wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:47: Delegate* delegate_; On 2015/09/18 16:39:11, achuithb wrote: > On 2015/09/17 23:21:40, Greg Levin wrote: > > Is there a smart pointer type I should use here instead? The delegate owns > this > > object, and should destroy it before the delegate is itself destroyed, so this > > shouldn't be a problem...? > > It shouldn't be a problem, but you should add a comment like: > // Pointer to owner. > or > // Not owned. Done. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.h:87: public first_run::GoodiesDisplayer::Delegate, On 2015/09/18 16:39:11, achuithb wrote: > You cannot inherit from an implementation class. The implementation of > Create/Destroy/GetTime have to be UserSessionManager. Done... moved functionality into member scoped_ptr<first_run::GoodiesDisplayer::Delegate> goodies_displayer_delegate_; https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/startup_utils.cc (right): https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/startup_utils.cc:116: StartupUtils::GetOobeCompleteFlagPath(); On 2015/09/18 16:39:11, achuithb wrote: > Don't think you need StartupUtils:: Done. https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/startup_utils.h (right): https://codereview.chromium.org/1308833004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/startup_utils.h:41: static base::FilePath GetOobeCompleteFlagPath(); On 2015/09/18 16:39:11, achuithb wrote: > Don't need this any more, right? Done. https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/startup_utils.cc (right): https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/startup_utils.cc:151: bool file_exists = base::PathExists(oobe_complete_flag_path); Is it generally considered desirable or meddling to make little code changes like this to make things more uniform?
https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/first_run/goodies_displayer.cc (right): https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:33: bool CheckGoodiesPrefAgainstOobeTimestamp( Make this a private method of Delegate https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:42: void UpdateGoodiesPrefCantShow(GoodiesDisplayer::Delegate* delegate, Make this a private method of Delegate https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:45: delegate->CreateGoodiesDisplayer(); Inline the implementation of CreateGoodiesDisplayer here https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:80: void GoodiesDisplayer::Init(Delegate* delegate) { Make this a non-static method https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.cc:85: base::Bind(&CheckGoodiesPrefAgainstOobeTimestamp, delegate), use WeakFactory https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/first_run/goodies_displayer.h (right): https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:25: void CreateGoodiesDisplayer(); Don't think you need this method https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/first_run/goodies_displayer.h:28: void DestroyGoodiesDisplayer(); You don't need this method https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.cc:1762: goodies_displayer_delegate_->DestroyGoodiesDisplayer(); Call reset instead. https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/startup_utils.cc (right): https://codereview.chromium.org/1308833004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/startup_utils.cc:151: bool file_exists = base::PathExists(oobe_complete_flag_path); On 2015/09/18 18:49:28, Greg Levin wrote: > Is it generally considered desirable or meddling to make little code changes > like this to make things more uniform? It's fine to clean up as you go along. If you make a large number of mechanical changes, it's better to use a separate CL so it's not distracting during reviews |
