Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
162393002:
Enable surprise me wallpaper for new profiles. (Closed)
|
Created:
6 years, 10 months ago by Harry McCleave Modified:
6 years, 8 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, Albert Bodenhamer Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable surprise me wallpaper for new profiles.
BUG=305855, 322603
TBR=awatson@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263741
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : api functions #Patch Set 4 : Enabled on new profile creation #Patch Set 5 : cleanup #
Total comments: 2
Patch Set 6 : use event to trigger update #Patch Set 7 : . #Patch Set 8 : . #Patch Set 9 : . #
Total comments: 5
Patch Set 10 : + multi user check #
Total comments: 4
Patch Set 11 : more spaces and timer #Patch Set 12 : short timer (5 min) #Patch Set 13 : rebase #
Messages
Total messages: 30 (0 generated)
Nikita ptal if this will be a solution for enabling surprise me by default on profile creation.
+Biao as reviewer. https://codereview.chromium.org/162393002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/162393002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:858: chromeos::LoginDisplayHostImpl::default_host()) I assumes this call may happen inside session (when you sign in as a new user) and LoginDisplayHost instance may have been already deleted. https://codereview.chromium.org/162393002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/create_profile_handler.cc (right): https://codereview.chromium.org/162393002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/create_profile_handler.cc:176: chromeos::WallpaperManager::Get()->EnableSurpriseMe(); CreateProfileHandler is created only when chrome://settings page is opened. I assume you want this for all new users that never had this option set? How do you distinguish between users that have already signed in on Chrome OS and new users?
+Biao as reviewer. https://codereview.chromium.org/162393002/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wallpaper_manager.cc (right): https://codereview.chromium.org/162393002/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/login/wallpaper_manager.cc:858: chromeos::LoginDisplayHostImpl::default_host()) I assumes this call may happen inside session (when you sign in as a new user) and LoginDisplayHost instance may have been already deleted. https://codereview.chromium.org/162393002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/create_profile_handler.cc (right): https://codereview.chromium.org/162393002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/create_profile_handler.cc:176: chromeos::WallpaperManager::Get()->EnableSurpriseMe(); CreateProfileHandler is created only when chrome://settings page is opened. I assume you want this for all new users that never had this option set? How do you distinguish between users that have already signed in on Chrome OS and new users?
On 2014/02/28 10:44:13, Nikita Kostylev wrote: > +Biao as reviewer. > > https://codereview.chromium.org/162393002/diff/200001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/wallpaper_manager.cc (right): > > https://codereview.chromium.org/162393002/diff/200001/chrome/browser/chromeos... > chrome/browser/chromeos/login/wallpaper_manager.cc:858: > chromeos::LoginDisplayHostImpl::default_host()) > I assumes this call may happen inside session (when you sign in as a new user) > and LoginDisplayHost instance may have been already deleted. > > https://codereview.chromium.org/162393002/diff/200001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/options/create_profile_handler.cc (right): > > https://codereview.chromium.org/162393002/diff/200001/chrome/browser/ui/webui... > chrome/browser/ui/webui/options/create_profile_handler.cc:176: > chromeos::WallpaperManager::Get()->EnableSurpriseMe(); > CreateProfileHandler is created only when chrome://settings page is opened. > > I assume you want this for all new users that never had this option set? > > How do you distinguish between users that have already signed in on Chrome OS > and new users? Sorry for long delay I got sidetracked on other things as will happen however RKC pointed out the correct way for sending messages to api's so would you mind taking another look?
https://codereview.chromium.org/162393002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/login/login_utils.cc (right): https://codereview.chromium.org/162393002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/login/login_utils.cc:353: UserManager::Get()->IsLoggedInAsRegularUser()) { nit: Can you please add a check that UserManager::GetLoggedInUsers().size() == 1 IsCurrentUserNew() may not be correctly working in multi-profiles, need to do a cleanup. https://codereview.chromium.org/162393002/diff/280001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/162393002/diff/280001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:284: if (!items.hasOwnProperty(Constants.AccessSurpriseMeEnabledKey)) { I wonder if that's the right time to check for this property since it may not have been synced at this point.
https://codereview.chromium.org/162393002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/login/login_utils.cc (right): https://codereview.chromium.org/162393002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/login/login_utils.cc:353: UserManager::Get()->IsLoggedInAsRegularUser()) { On 2014/04/08 16:58:27, Nikita Kostylev wrote: > nit: Can you please add a check that > UserManager::GetLoggedInUsers().size() == 1 > > IsCurrentUserNew() may not be correctly working in multi-profiles, need to do a > cleanup. Done. https://codereview.chromium.org/162393002/diff/280001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/162393002/diff/280001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:284: if (!items.hasOwnProperty(Constants.AccessSurpriseMeEnabledKey)) { On 2014/04/08 16:58:27, Nikita Kostylev wrote: > I wonder if that's the right time to check for this property since it may not > have been synced at this point. My impression was that if this sync fails for whatever reason, a local copy of the preferences will split off and when sync succeeds conflicts will be resolved in the favor of the server values which would be the intended behaviour?
On 2014/04/09 02:41:33, Harry McCleave wrote: > https://codereview.chromium.org/162393002/diff/280001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/login_utils.cc (right): > > https://codereview.chromium.org/162393002/diff/280001/chrome/browser/chromeos... > chrome/browser/chromeos/login/login_utils.cc:353: > UserManager::Get()->IsLoggedInAsRegularUser()) { > On 2014/04/08 16:58:27, Nikita Kostylev wrote: > > nit: Can you please add a check that > > UserManager::GetLoggedInUsers().size() == 1 > > > > IsCurrentUserNew() may not be correctly working in multi-profiles, need to do > a > > cleanup. > > Done. > > https://codereview.chromium.org/162393002/diff/280001/chrome/browser/resource... > File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js > (right): > > https://codereview.chromium.org/162393002/diff/280001/chrome/browser/resource... > chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:284: if > (!items.hasOwnProperty(Constants.AccessSurpriseMeEnabledKey)) { > On 2014/04/08 16:58:27, Nikita Kostylev wrote: > > I wonder if that's the right time to check for this property since it may not > > have been synced at this point. > > My impression was that if this sync fails for whatever reason, a local copy of > the preferences will split off and when sync succeeds conflicts will be resolved > in the favor of the server values which would be the intended behaviour? I don't know exactly how this should behave. Biao?
https://codereview.chromium.org/162393002/diff/280001/chrome/browser/resource... File chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js (right): https://codereview.chromium.org/162393002/diff/280001/chrome/browser/resource... chrome/browser/resources/chromeos/wallpaper_manager/js/event_page.js:284: if (!items.hasOwnProperty(Constants.AccessSurpriseMeEnabledKey)) { Is this going to be called when an existing user logged in on a new device? If so, it might be safer to delay this operation for something like 1 hour or so (or after sync is done if we can detect such event). Otherwise, we may change the wallpaper of existing user first and then revert to the correct one. It may confuse user. On 2014/04/09 02:41:33, Harry McCleave wrote: > On 2014/04/08 16:58:27, Nikita Kostylev wrote: > > I wonder if that's the right time to check for this property since it may not > > have been synced at this point. > > My impression was that if this sync fails for whatever reason, a local copy of > the preferences will split off and when sync succeeds conflicts will be resolved > in the favor of the server values which would be the intended behaviour? https://codereview.chromium.org/162393002/diff/300001/chrome/browser/chromeos... File chrome/browser/chromeos/login/login_utils.cc (right): https://codereview.chromium.org/162393002/diff/300001/chrome/browser/chromeos... chrome/browser/chromeos/login/login_utils.cc:353: UserManager::Get()->IsLoggedInAsRegularUser() && Do you mind to add a short comment about why GetLoggedInUsers().size() == 1 is necessary? https://codereview.chromium.org/162393002/diff/300001/chrome/browser/chromeos... chrome/browser/chromeos/login/login_utils.cc:354: UserManager::Get()->GetLoggedInUsers().size() == 1) { nit: indent is off
Added a timer, this seems to me may also have some odd user behavior (if they check but don't change wallpaper settings during that hour and then later after, surprise me may have toggled for them from unset to true), but this is a case of six of one half dozen of the other as without it you as you pointed out there is also potential oddness. https://codereview.chromium.org/162393002/diff/300001/chrome/browser/chromeos... File chrome/browser/chromeos/login/login_utils.cc (right): https://codereview.chromium.org/162393002/diff/300001/chrome/browser/chromeos... chrome/browser/chromeos/login/login_utils.cc:353: UserManager::Get()->IsLoggedInAsRegularUser() && On 2014/04/09 14:32:15, bshe wrote: > Do you mind to add a short comment about why GetLoggedInUsers().size() == 1 is > necessary? Done. https://codereview.chromium.org/162393002/diff/300001/chrome/browser/chromeos... chrome/browser/chromeos/login/login_utils.cc:354: UserManager::Get()->GetLoggedInUsers().size() == 1) { On 2014/04/09 14:32:15, bshe wrote: > nit: indent is off Done.
On 2014/04/10 02:42:28, Harry McCleave wrote: > Added a timer, this seems to me may also have some odd user behavior (if they > check but don't change wallpaper settings during that hour and then later after, > surprise me may have toggled for them from unset to true), but this is a case of > six of one half dozen of the other as without it you as you pointed out there is > also potential oddness. > > https://codereview.chromium.org/162393002/diff/300001/chrome/browser/chromeos... > File chrome/browser/chromeos/login/login_utils.cc (right): > > https://codereview.chromium.org/162393002/diff/300001/chrome/browser/chromeos... > chrome/browser/chromeos/login/login_utils.cc:353: > UserManager::Get()->IsLoggedInAsRegularUser() && > On 2014/04/09 14:32:15, bshe wrote: > > Do you mind to add a short comment about why GetLoggedInUsers().size() == 1 is > > necessary? > > Done. > > https://codereview.chromium.org/162393002/diff/300001/chrome/browser/chromeos... > chrome/browser/chromeos/login/login_utils.cc:354: > UserManager::Get()->GetLoggedInUsers().size() == 1) { > On 2014/04/09 14:32:15, bshe wrote: > > nit: indent is off > > Done. Ideally we should call that function when sync is finished. But I dont know if there is a notification to notify wallpaper picker that sync has finished. What you said makes perfect sense. It makes me wounder if 1 hour is too aggressive? Considering the new user can login, the internet connection shouldn't be bad. So it wont take much time for sync to finish. Not sure if we have UMA stats to track how long a sync takes on Chromebook. But 10 mins is probably safe enough? Nikita, do you have any thought on this? lgtm
I worry that timer approach may not ever work. Suppose user who uses Chrome OS for short sessions (< 1h). I think you should explore who to eliminate timer and set this option in a way that it does apply only if sync property is not set. This means that sync value always has priority when it is set.
I'm not really a fan of the timer solution either. I wonder if we can tell the difference between a setting that's unset an one that just isn't synced yet. On Thu, Apr 10, 2014 at 8:30 AM, <nkostylev@chromium.org> wrote: > I worry that timer approach may not ever work. > Suppose user who uses Chrome OS for short sessions (< 1h). > > I think you should explore who to eliminate timer and set this option in a > way > that it does apply only if sync property is not set. This means that sync > value > always has priority when it is set. > > https://codereview.chromium.org/162393002/ > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
As far as I know, the timer should persist after logout. So if user has a short session, the timer will invoke next time user login. I agree timer is not a good solution. I dont know sync storage enough to be certain if there is a way to tell the difference of an unset value and one that is not synced yet. +miket who might know.
On 2014/04/10 16:30:58, Albert Bodenhamer wrote: > I'm not really a fan of the timer solution either. > > I wonder if we can tell the difference between a setting that's unset an > one that just isn't synced yet. Chrome prefs have IsDefaultValue() but this is extension.
On 2014/04/10 16:47:24, Nikita Kostylev wrote: > On 2014/04/10 16:30:58, Albert Bodenhamer wrote: > > I'm not really a fan of the timer solution either. > > > > I wonder if we can tell the difference between a setting that's unset an > > one that just isn't synced yet. > > Chrome prefs have IsDefaultValue() but this is extension. I am unaware of a extension equivalent of IsDefaultValue sadly thus using the check for undefined (hasOwnProperty) to determine the effective DefaultValue, I suspect though since this is on profile creation of non-guest users that the number of cases where the sync operation would fail / be deferred is sufficiently small that we could consider the initial (no timer submission)?
On 2014/04/10 19:19:20, Harry McCleave wrote: > On 2014/04/10 16:47:24, Nikita Kostylev wrote: > > On 2014/04/10 16:30:58, Albert Bodenhamer wrote: > > > I'm not really a fan of the timer solution either. > > > > > > I wonder if we can tell the difference between a setting that's unset an > > > one that just isn't synced yet. > > > > Chrome prefs have IsDefaultValue() but this is extension. > > I am unaware of a extension equivalent of IsDefaultValue sadly thus using the > check for undefined (hasOwnProperty) to determine the effective DefaultValue, I > suspect though since this is on profile creation of non-guest users that the > number of cases where the sync operation would fail / be deferred is > sufficiently small that we could consider the initial (no timer submission)? Do you mean sync is mostly liked finished when "surprise me" event is dispatched? I though we create profile first and then start sync. That's the main reason I suggested a timer. It could significantly reduce false positives. But maybe I am wrong.
On Thu, Apr 10, 2014 at 12:19 PM, <harrym@chromium.org> wrote: > On 2014/04/10 16:47:24, Nikita Kostylev wrote: > >> On 2014/04/10 16:30:58, Albert Bodenhamer wrote: >> > I'm not really a fan of the timer solution either. >> > >> > I wonder if we can tell the difference between a setting that's unset an >> > one that just isn't synced yet. >> > > Chrome prefs have IsDefaultValue() but this is extension. >> > > I am unaware of a extension equivalent of IsDefaultValue sadly thus using > the > check for undefined (hasOwnProperty) to determine the effective > DefaultValue, I > suspect though since this is on profile creation of non-guest users that > the > number of cases where the sync operation would fail / be deferred is > sufficiently small that we could consider the initial (no timer > submission)? > +1 I don't think we're going to get this perfect, but the edge cases are pretty small. Perhaps compromise on a short timer? If we're in a new user case and the value hasn't been pulled down in 5 minutes I'd guess the risk of a collision is vanishingly small. > > https://codereview.chromium.org/162393002/ > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/10 19:47:29, Albert Bodenhamer wrote: > On Thu, Apr 10, 2014 at 12:19 PM, <mailto:harrym@chromium.org> wrote: > > > On 2014/04/10 16:47:24, Nikita Kostylev wrote: > > > >> On 2014/04/10 16:30:58, Albert Bodenhamer wrote: > >> > I'm not really a fan of the timer solution either. > >> > > >> > I wonder if we can tell the difference between a setting that's unset an > >> > one that just isn't synced yet. > >> > > > > Chrome prefs have IsDefaultValue() but this is extension. > >> > > > > I am unaware of a extension equivalent of IsDefaultValue sadly thus using > > the > > check for undefined (hasOwnProperty) to determine the effective > > DefaultValue, I > > suspect though since this is on profile creation of non-guest users that > > the > > number of cases where the sync operation would fail / be deferred is > > sufficiently small that we could consider the initial (no timer > > submission)? > > > > +1 I don't think we're going to get this perfect, but the edge cases are > pretty small. > > Perhaps compromise on a short timer? If we're in a new user case and the > value hasn't been pulled down in 5 minutes I'd guess the risk of a > collision is vanishingly small. > > > > > > https://codereview.chromium.org/162393002/ > > > > > > -- > Albert Bodenhamer | Software Engineer | mailto:abodenha@chromium.<abodenha@google.com> > org > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I'm fine with adding a short timer, the RequestSur... event is dispatched after the "syncing prefs" splash screen though that may not include all extension prefs.
On 2014/04/10 19:55:47, Harry McCleave wrote: > On 2014/04/10 19:47:29, Albert Bodenhamer wrote: > > On Thu, Apr 10, 2014 at 12:19 PM, <mailto:harrym@chromium.org> wrote: > > > > > On 2014/04/10 16:47:24, Nikita Kostylev wrote: > > > > > >> On 2014/04/10 16:30:58, Albert Bodenhamer wrote: > > >> > I'm not really a fan of the timer solution either. > > >> > > > >> > I wonder if we can tell the difference between a setting that's unset an > > >> > one that just isn't synced yet. > > >> > > > > > > Chrome prefs have IsDefaultValue() but this is extension. > > >> > > > > > > I am unaware of a extension equivalent of IsDefaultValue sadly thus using > > > the > > > check for undefined (hasOwnProperty) to determine the effective > > > DefaultValue, I > > > suspect though since this is on profile creation of non-guest users that > > > the > > > number of cases where the sync operation would fail / be deferred is > > > sufficiently small that we could consider the initial (no timer > > > submission)? > > > > > > > +1 I don't think we're going to get this perfect, but the edge cases are > > pretty small. > > > > Perhaps compromise on a short timer? If we're in a new user case and the > > value hasn't been pulled down in 5 minutes I'd guess the risk of a > > collision is vanishingly small. > > > > > > > > > > https://codereview.chromium.org/162393002/ > > > > > > > > > > > -- > > Albert Bodenhamer | Software Engineer | > mailto:abodenha@chromium.<abodenha@google.com> > > org > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > I'm fine with adding a short timer, the RequestSur... event is dispatched after > the "syncing prefs" splash screen though that may not include all extension > prefs. I see. Thanks for explanation. A short timer sounds good to me.
Patch changed to 5 minute timer.
On 2014/04/10 20:11:31, Harry McCleave wrote: > Patch changed to 5 minute timer. lgtm
lgtm
The CQ bit was checked by harrym@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/harrym@chromium.org/162393002/330001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/chromeos/login/wallpaper_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/chromeos/login/wallpaper_manager.cc
Hunk #1 succeeded at 29 (offset 2 lines).
Hunk #2 FAILED at 43.
Hunk #3 succeeded at 979 (offset 65 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/login/wallpaper_manager.cc.rej
Patch: chrome/browser/chromeos/login/wallpaper_manager.cc
Index: chrome/browser/chromeos/login/wallpaper_manager.cc
diff --git a/chrome/browser/chromeos/login/wallpaper_manager.cc
b/chrome/browser/chromeos/login/wallpaper_manager.cc
index
a036ca9064fdbf43a1a63d07eba4e94046c9fc0f..6928e4a37bc98df1ee25afab76256171def4513d
100644
--- a/chrome/browser/chromeos/login/wallpaper_manager.cc
+++ b/chrome/browser/chromeos/login/wallpaper_manager.cc
@@ -27,11 +27,15 @@
#include "base/values.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/chromeos/extensions/wallpaper_manager_util.h"
+#include "chrome/browser/chromeos/extensions/wallpaper_private_api.h"
+#include "chrome/browser/chromeos/login/login_display_host_impl.h"
#include "chrome/browser/chromeos/login/startup_utils.h"
#include "chrome/browser/chromeos/login/user.h"
#include "chrome/browser/chromeos/login/user_manager.h"
#include "chrome/browser/chromeos/login/wizard_controller.h"
#include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/extensions/extension_service.h"
#include "chrome/common/chrome_paths.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/common/pref_names.h"
@@ -39,6 +43,8 @@
#include "chromeos/dbus/dbus_thread_manager.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/notification_service.h"
+#include "content/public/browser/web_ui.h"
+#include "extensions/browser/event_router.h"
#include "ui/base/resource/resource_bundle.h"
#include "ui/gfx/codec/jpeg_codec.h"
#include "ui/gfx/image/image_skia_operations.h"
@@ -910,6 +916,20 @@ void
WallpaperManager::RemoveObserver(WallpaperManager::Observer* observer) {
observers_.RemoveObserver(observer);
}
+void WallpaperManager::EnableSurpriseMe() {
+ Profile* profile = ProfileManager::GetActiveUserProfile();
+ DCHECK(profile);
+ DCHECK(extensions::ExtensionSystem::Get(profile)->event_router());
+ scoped_ptr<extensions::Event> event(
+ new extensions::Event(
+ extensions::api::wallpaper_private::OnRequestEnableSurpriseMe::kEventName,
+ extensions::api::wallpaper_private::OnRequestEnableSurpriseMe::Create()));
+
+ extensions::ExtensionSystem::Get(profile)->event_router()
+ ->DispatchEventToExtension(extension_misc::kWallpaperManagerId,
+ event.Pass());
+}
+
void WallpaperManager::NotifyAnimationFinished() {
FOR_EACH_OBSERVER(
Observer, observers_, OnWallpaperAnimationFinished(last_selected_user_));
lgtm
The CQ bit was checked by harrym@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/harrym@chromium.org/162393002/350001
Message was sent while issue was closed.
Change committed as 263741 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
