Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13468005:
Show only the settings pages for new managed users (Closed)
|
Created:
7 years, 8 months ago by Sergiu Modified:
7 years, 8 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionShow only the settings pages for new managed users
Currently when creating a new managed user we open other tabs in addition to
the managed users settings one, which we don't want to do.
This makes sure that only that tab gets added to the list of URLs to open on
start-up by early-returning from that function.
R=ben@chromium.org
BUG=171370
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192551
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Add a browser test #Patch Set 4 : Remove extra line #
Total comments: 6
Patch Set 5 : Fixes #
Total comments: 13
Patch Set 6 : Minor fixes #Patch Set 7 : Return the browser #Patch Set 8 : Rebase #
Messages
Total messages: 18 (0 generated)
Hey Ben, please take a look. Thanks.
Seems like you could write a test for this, e.g. in startup_browser_creator_browsertest.cc.
Added a browser test as well, hope that is ok.
https://codereview.chromium.org/13468005/diff/10004/chrome/browser/ui/startup... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/13468005/diff/10004/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:904: void FindOneOtherBrowser(Browser** out_other_browser) { This method is a copy of the one starting at line 88. Can you find a way to reuse that code? https://codereview.chromium.org/13468005/diff/10004/chrome/browser/ui/startup... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/13468005/diff/10004/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_impl.cc:764: SyncPromoUI::ShouldShowSyncPromoAtStartup(profile_, is_first_run_) && This method is supposed to tell a caller whether the sync promo should be shown. If we don't want to show the sync promo for a managed profile, why don't we move the check there? https://codereview.chromium.org/13468005/diff/10004/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_impl.cc:942: // If this is the first run then show only the managed user settings Wait, is this actually necessary? If we have set a startup URL, none of the branches below should execute (because they check for empty |startup_urls|). We also have a test now (thanks!) to prevent regressions if someone inadvertently modifies the code.
+dbeam for sync_promo_ui.cc change. Thanks https://codereview.chromium.org/13468005/diff/10004/chrome/browser/ui/startup... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/13468005/diff/10004/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:904: void FindOneOtherBrowser(Browser** out_other_browser) { On 2013/04/03 16:41:23, Bernhard Bauer wrote: > This method is a copy of the one starting at line 88. Can you find a way to > reuse that code? Moved it as a function in the anonymous namespace and changed the other calls. https://codereview.chromium.org/13468005/diff/10004/chrome/browser/ui/startup... File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right): https://codereview.chromium.org/13468005/diff/10004/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_impl.cc:764: SyncPromoUI::ShouldShowSyncPromoAtStartup(profile_, is_first_run_) && On 2013/04/03 16:41:23, Bernhard Bauer wrote: > This method is supposed to tell a caller whether the sync promo should be shown. > If we don't want to show the sync promo for a managed profile, why don't we move > the check there? Makes total sense, moved it to sync_promo_ui.cc. https://codereview.chromium.org/13468005/diff/10004/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_impl.cc:942: // If this is the first run then show only the managed user settings On 2013/04/03 16:41:23, Bernhard Bauer wrote: > Wait, is this actually necessary? If we have set a startup URL, none of the > branches below should execute (because they check for empty |startup_urls|). We > also have a test now (thanks!) to prevent regressions if someone inadvertently > modifies the code. That is true, we can do without the else branch. Removed.
LGTM with a nit and some cleanup that would be nice: https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/startup... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:43: void FindOneOtherBrowser(Browser* browser, Browser** out_other_browser) { Could you simply return the other browser here? https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:55: ASSERT_TRUE(other_browser != browser); This check isn't really necessary (that's the exact condition we test above). https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:933: } Nit: a newline after this line would be nice.
+sail@ as he knows this code and wants to be CC'd on sync promo CLs https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right): https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:136: // Don't show for managed users. nit: managed profiles https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:137: if (profile->GetPrefs()->GetBoolean(prefs::kProfileIsManaged)) do you want to disable the promo based on whether the *profile* is managed or whether *sync* is managed? if it's sync, you should probably use if (!profile->IsSyncAccessible()) instead
https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right): https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:137: if (profile->GetPrefs()->GetBoolean(prefs::kProfileIsManaged)) On 2013/04/03 23:39:20, Dan Beam wrote: > do you want to disable the promo based on whether the *profile* is managed or > whether *sync* is managed? if it's sync, you should probably use > > if (!profile->IsSyncAccessible()) > > instead er, if (profile->IsSyncAccessible()) I mean
https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right): https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:137: if (profile->GetPrefs()->GetBoolean(prefs::kProfileIsManaged)) On 2013/04/04 00:01:45, Dan Beam wrote: > On 2013/04/03 23:39:20, Dan Beam wrote: > > do you want to disable the promo based on whether the *profile* is managed or > > whether *sync* is managed? if it's sync, you should probably use > > > > if (!profile->IsSyncAccessible()) > > > > instead > > er, if (profile->IsSyncAccessible()) I mean ... and I'm going to go drink more coffee
https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/startup... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:43: void FindOneOtherBrowser(Browser* browser, Browser** out_other_browser) { On 2013/04/03 17:55:20, Bernhard Bauer wrote: > Could you simply return the other browser here? Well, there is a comment below that says: // Helper functions return void so that we can ASSERT*(). // Use ASSERT_NO_FATAL_FAILURE around calls to these functions to stop the // test if an assert fails. I guess it is related to the way assert stops execution that requires void functions. I also guess I could return NULL when something is wrong and assert that if you think that is better. https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:55: ASSERT_TRUE(other_browser != browser); On 2013/04/03 17:55:20, Bernhard Bauer wrote: > This check isn't really necessary (that's the exact condition we test above). Done. https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:933: } On 2013/04/03 17:55:20, Bernhard Bauer wrote: > Nit: a newline after this line would be nice. Done. https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/webui/s... File chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc (right): https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:136: // Don't show for managed users. On 2013/04/03 23:39:20, Dan Beam wrote: > nit: managed profiles Done. https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/webui/s... chrome/browser/ui/webui/sync_promo/sync_promo_ui.cc:137: if (profile->GetPrefs()->GetBoolean(prefs::kProfileIsManaged)) On 2013/04/03 23:39:20, Dan Beam wrote: > do you want to disable the promo based on whether the *profile* is managed or > whether *sync* is managed? if it's sync, you should probably use > > if (!profile->IsSyncAccessible()) > > instead It is for managed users (see the referenced bug for our definition of a managed user).
https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/startup... File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/startup... chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:43: void FindOneOtherBrowser(Browser* browser, Browser** out_other_browser) { On 2013/04/04 09:02:55, Sergiu wrote: > On 2013/04/03 17:55:20, Bernhard Bauer wrote: > > Could you simply return the other browser here? > > Well, there is a comment below that says: > // Helper functions return void so that we can ASSERT*(). > // Use ASSERT_NO_FATAL_FAILURE around calls to these functions to stop the > // test if an assert fails. > > I guess it is related to the way assert stops execution that requires void > functions. I also guess I could return NULL when something is wrong and assert > that if you think that is better. Well, yeah, an ASSERT expands to something that returns on failure, so you can only use it in a void method. OTOH, one of the ASSERTs isn't necessary, and the other one you can just as well do in the caller (instead of the ASSERT_NO_FATAL_FAILURE). The one in line 45 can be made an EXPECT (nothing breaks if you continue).
lgtm
On 2013/04/04 09:08:40, Bernhard Bauer wrote: > https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/startup... > File chrome/browser/ui/startup/startup_browser_creator_browsertest.cc (right): > > https://codereview.chromium.org/13468005/diff/21001/chrome/browser/ui/startup... > chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:43: void > FindOneOtherBrowser(Browser* browser, Browser** out_other_browser) { > On 2013/04/04 09:02:55, Sergiu wrote: > > On 2013/04/03 17:55:20, Bernhard Bauer wrote: > > > Could you simply return the other browser here? > > > > Well, there is a comment below that says: > > // Helper functions return void so that we can ASSERT*(). > > // Use ASSERT_NO_FATAL_FAILURE around calls to these functions to stop the > > // test if an assert fails. > > > > I guess it is related to the way assert stops execution that requires void > > functions. I also guess I could return NULL when something is wrong and assert > > that if you think that is better. > > Well, yeah, an ASSERT expands to something that returns on failure, so you can > only use it in a void method. OTOH, one of the ASSERTs isn't necessary, and the > other one you can just as well do in the caller (instead of the > ASSERT_NO_FATAL_FAILURE). The one in line 45 can be made an EXPECT (nothing > breaks if you continue). Done.
lgtm
Ben, can I get an OWNERS review? Thanks, Sergiu
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/13468005/44001
Message was sent while issue was closed.
Change committed as 192551 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
