|
|
Created:
3 years, 11 months ago by Patrick Monette Modified:
3 years, 11 months ago Reviewers:
Peter Kasting CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet kHasSeenWin10PromoPage in local_state instead of in profile prefs.
The pref is registered to the local state so the old code was incorrect.
BUG=685799
Review-Url: https://codereview.chromium.org/2659643003
Cr-Commit-Position: refs/heads/master@{#446497}
Committed: https://chromium.googlesource.com/chromium/src/+/7c6a615b2b23452b33af668caab3945fe44b52a4
Patch Set 1 #
Total comments: 2
Patch Set 2 : Inline Profile::FromWebUI() #Patch Set 3 : Merge #Messages
Total messages: 20 (10 generated)
Patchset #1 (id:1) has been deleted
pmonette@chromium.org changed reviewers: + pkasting@chromium.org
PTAL. Small bug fix.
LGTM on making code consistent, but I'm wondering if this should actually be in the profile prefs instead of the local state. If we already showed a user the promo on one of their machines, isn't showing it again on another machine annoying? https://codereview.chromium.org/2659643003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_ui.cc (right): https://codereview.chromium.org/2659643003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:125: Profile* profile = Profile::FromWebUI(web_ui); Nit: Or just inline this
On 2017/01/26 at 21:14:32, pkasting wrote: > LGTM on making code consistent, but I'm wondering if this should actually be in the profile prefs instead of the local state. If we already showed a user the promo on one of their machines, isn't showing it again on another machine annoying? Drive-by comments: I actually checked with PM & UX on this point during implementation. Because this promo relates to per-machine state (default browser and whether Chrome is pinned to taskbar), they said they preferred to show once-per-machine, rather than once-per-profile.
On 2017/01/26 21:32:52, tmartino wrote: > On 2017/01/26 at 21:14:32, pkasting wrote: > > LGTM on making code consistent, but I'm wondering if this should actually be > in the profile prefs instead of the local state. If we already showed a user > the promo on one of their machines, isn't showing it again on another machine > annoying? > > Drive-by comments: I actually checked with PM & UX on this point during > implementation. Because this promo relates to per-machine state (default browser > and whether Chrome is pinned to taskbar), they said they preferred to show > once-per-machine, rather than once-per-profile. Fair enough. I assume we avoid suggesting to users that they pin/set as default in cases where that's already true. If not, that should be fixed separately :)
On 2017/01/26 21:41:03, Peter Kasting wrote: > On 2017/01/26 21:32:52, tmartino wrote: > > On 2017/01/26 at 21:14:32, pkasting wrote: > > > LGTM on making code consistent, but I'm wondering if this should actually be > > in the profile prefs instead of the local state. If we already showed a user > > the promo on one of their machines, isn't showing it again on another machine > > annoying? > > > > Drive-by comments: I actually checked with PM & UX on this point during > > implementation. Because this promo relates to per-machine state (default > browser > > and whether Chrome is pinned to taskbar), they said they preferred to show > > once-per-machine, rather than once-per-profile. > > Fair enough. I assume we avoid suggesting to users that they pin/set as default > in cases where that's already true. If not, that should be fixed separately :) Yeah we always check for default state before deciding to show.
Thanks https://codereview.chromium.org/2659643003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_win10_ui.cc (right): https://codereview.chromium.org/2659643003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_win10_ui.cc:125: Profile* profile = Profile::FromWebUI(web_ui); On 2017/01/26 21:14:32, Peter Kasting wrote: > Nit: Or just inline this Done.
Description was changed from ========== Set kHasSeenWin10PromoPage in local_state instead of in profile prefs. The pref is registered to the local state so the old code was incorrect. BUG=648686 ========== to ========== Set kHasSeenWin10PromoPage in local_state instead of in profile prefs. The pref is registered to the local state so the old code was incorrect. BUG=685799 ==========
The CQ bit was checked by pmonette@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/2659643003/#ps40001 (title: "Inline Profile::FromWebUI()")
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
Failed to apply patch for chrome/browser/ui/webui/welcome_win10_ui.cc: While running git apply --index -p1; error: patch failed: chrome/browser/ui/webui/welcome_win10_ui.cc:92 error: chrome/browser/ui/webui/welcome_win10_ui.cc: patch does not apply Patch: chrome/browser/ui/webui/welcome_win10_ui.cc Index: chrome/browser/ui/webui/welcome_win10_ui.cc diff --git a/chrome/browser/ui/webui/welcome_win10_ui.cc b/chrome/browser/ui/webui/welcome_win10_ui.cc index cf416e30e2e22d7db852d3c33e6e5510d548bf74..1d5922df126e88e24897a06fb2f1eebfff95588f 100644 --- a/chrome/browser/ui/webui/welcome_win10_ui.cc +++ b/chrome/browser/ui/webui/welcome_win10_ui.cc @@ -8,6 +8,7 @@ #include "base/feature_list.h" #include "base/memory/ptr_util.h" +#include "chrome/browser/browser_process.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/startup/startup_features.h" #include "chrome/browser/ui/webui/welcome_win10_handler.h" @@ -92,10 +93,9 @@ WelcomeWin10UI::WelcomeWin10UI(content::WebUI* web_ui, const GURL& url) static const char kCssFilePath[] = "welcome.css"; static const char kJsFilePath[] = "welcome.js"; - Profile* profile = Profile::FromWebUI(web_ui); - - // Store that this profile has been shown the Win10 promo page. - profile->GetPrefs()->SetBoolean(prefs::kHasSeenWin10PromoPage, true); + // Remember that the Win10 promo page has been shown. + g_browser_process->local_state()->SetBoolean(prefs::kHasSeenWin10PromoPage, + true); // Determine which variation to show. bool is_first_run = !UrlContainsKeyValueInQuery(url, "text", "faster"); @@ -122,7 +122,7 @@ WelcomeWin10UI::WelcomeWin10UI(content::WebUI* web_ui, const GURL& url) html_source->AddResourcePath("logo-small.png", IDR_PRODUCT_LOGO_64); html_source->AddResourcePath("logo-large.png", IDR_PRODUCT_LOGO_128); - content::WebUIDataSource::Add(profile, html_source); + content::WebUIDataSource::Add(Profile::FromWebUI(web_ui), html_source); } WelcomeWin10UI::~WelcomeWin10UI() = default;
The CQ bit was checked by pmonette@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/2659643003/#ps60001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485472563913560, "parent_rev": "1edbeeb9ee14e4bdbf673410967f3d0e9a03f8ce", "commit_rev": "7c6a615b2b23452b33af668caab3945fe44b52a4"}
Message was sent while issue was closed.
Description was changed from ========== Set kHasSeenWin10PromoPage in local_state instead of in profile prefs. The pref is registered to the local state so the old code was incorrect. BUG=685799 ========== to ========== Set kHasSeenWin10PromoPage in local_state instead of in profile prefs. The pref is registered to the local state so the old code was incorrect. BUG=685799 Review-Url: https://codereview.chromium.org/2659643003 Cr-Commit-Position: refs/heads/master@{#446497} Committed: https://chromium.googlesource.com/chromium/src/+/7c6a615b2b23452b33af668caab3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7c6a615b2b23452b33af668caab3... |