Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11640032:
Delay forcing natural scroll setting until after initial sync. (Closed)
|
Created:
8 years ago by achuithb Modified:
8 years ago CC:
chromium-reviews, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionDelay forcing natural scroll setting until after initial sync.
BUG=166760
TEST=manual.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174278
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 3
Messages
Total messages: 15 (0 generated)
Please take a look, Scott and Xiyuan.
https://codereview.chromium.org/11640032/diff/1/chrome/browser/chromeos/prefe... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/11640032/diff/1/chrome/browser/chromeos/prefe... chrome/browser/chromeos/preferences.cc:392: prefs->AddObserver(this); Where do we RemoveObserver? https://codereview.chromium.org/11640032/diff/1/chrome/browser/chromeos/prefe... chrome/browser/chromeos/preferences.cc:692: nit: nuke this emptly line https://codereview.chromium.org/11640032/diff/1/chrome/browser/chromeos/prefe... File chrome/browser/chromeos/preferences.h (right): https://codereview.chromium.org/11640032/diff/1/chrome/browser/chromeos/prefe... chrome/browser/chromeos/preferences.h:95: virtual void OnIsSyncingChanged() OVERRIDE; nit: keep virtual methods after non-virtual ones.
PTAL. I was worried about dtor ordering, but it's ok for RemoveObserver in ~Preferences. https://codereview.chromium.org/11640032/diff/1/chrome/browser/chromeos/prefe... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/11640032/diff/1/chrome/browser/chromeos/prefe... chrome/browser/chromeos/preferences.cc:392: prefs->AddObserver(this); On 2012/12/20 01:44:28, xiyuan wrote: > Where do we RemoveObserver? Done. https://codereview.chromium.org/11640032/diff/1/chrome/browser/chromeos/prefe... chrome/browser/chromeos/preferences.cc:692: On 2012/12/20 01:44:28, xiyuan wrote: > nit: nuke this emptly line Done. https://codereview.chromium.org/11640032/diff/1/chrome/browser/chromeos/prefe... File chrome/browser/chromeos/preferences.h (right): https://codereview.chromium.org/11640032/diff/1/chrome/browser/chromeos/prefe... chrome/browser/chromeos/preferences.h:95: virtual void OnIsSyncingChanged() OVERRIDE; On 2012/12/20 01:44:28, xiyuan wrote: > nit: keep virtual methods after non-virtual ones. Done.
LGTM How about explicitly releasing |chromeos_preferences_| in ProfileImpl to ensure the destruct order?
On 2012/12/20 02:35:13, xiyuan wrote: > LGTM > > How about explicitly releasing |chromeos_preferences_| in ProfileImpl to ensure > the destruct order? That's a good point. Where do you think we should do this in the dtor? http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profiles/pro... Right at the beginning of the dtor is probably the right place, I think?
On 2012/12/20 06:12:57, achuith.bhandarkar wrote: > That's a good point. Where do you think we should do this in the dtor? > http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profiles/pro... > > Right at the beginning of the dtor is probably the right place, I think? I was more inclined to add it close to the end in the dtor, just before SetExitType call. And don't forget to add a comment of why we need to to it.
On 2012/12/20 06:53:04, xiyuan wrote: > On 2012/12/20 06:12:57, achuith.bhandarkar wrote: > > That's a good point. Where do you think we should do this in the dtor? > > > http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profiles/pro... > > > > Right at the beginning of the dtor is probably the right place, I think? > > I was more inclined to add it close to the end in the dtor, just before > SetExitType call. > > And don't forget to add a comment of why we need to to it. I need to merge this change into M23. I'm getting a bit nervous about this change. Do you think I should make this ~ProfileImpl change in another CL (and not merge it)? If we get the ordering wrong and access deleted objects in destructors, it would compromise stability and security.
On 2012/12/20 11:23:18, achuith.bhandarkar wrote: > I need to merge this change into M23. I'm getting a bit nervous about this > change. Do you think I should make this ~ProfileImpl change in another CL (and > not merge it)? If we get the ordering wrong and access deleted objects in > destructors, it would compromise stability and security. I am okay with patch #2. We could do ProfileImpl dtor change when needed (i.e. RemoveObserver crashes because destruct order is wrong).
https://codereview.chromium.org/11640032/diff/6001/chrome/browser/chromeos/pr... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/11640032/diff/6001/chrome/browser/chromeos/pr... chrome/browser/chromeos/preferences.cc:392: // This causes OnIsSyncingChanged to be called when the value of OnIsSyncingChanged() https://codereview.chromium.org/11640032/diff/6001/chrome/browser/chromeos/pr... chrome/browser/chromeos/preferences.cc:461: ForceNaturalScrollDefault(); If you invoke ForceNaturalScrollDefault() here and IsSyncing is false won't you ignore updating natural_scroll_ even though the user just changed the pref?
https://codereview.chromium.org/11640032/diff/6001/chrome/browser/chromeos/pr... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/11640032/diff/6001/chrome/browser/chromeos/pr... chrome/browser/chromeos/preferences.cc:461: ForceNaturalScrollDefault(); On 2012/12/20 16:29:17, sky wrote: > If you invoke ForceNaturalScrollDefault() here and IsSyncing is false won't you > ignore updating natural_scroll_ even though the user just changed the pref? ForceNaturalScrollDefault just attempts to provide a default value based on command line switch. The pref value handling is @463-469 below.
Ah, ok. LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/11640032/6001
On 2012/12/20 16:04:57, xiyuan wrote: > On 2012/12/20 11:23:18, achuith.bhandarkar wrote: > > I need to merge this change into M23. I'm getting a bit nervous about this > > change. Do you think I should make this ~ProfileImpl change in another CL (and > > not merge it)? If we get the ordering wrong and access deleted objects in > > destructors, it would compromise stability and security. > > I am okay with patch #2. We could do ProfileImpl dtor change when needed (i.e. > RemoveObserver crashes because destruct order is wrong). sg. I'll leave it as be for right now. I've verified that the dtor ordering is correct (chromeos_preferences deleted before pref_service, etc).
Thanks for the reviews, Scott and Xiyuan!
Message was sent while issue was closed.
Change committed as 174278 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
