Add an automatic settings reset banner.

Add an automatic settings reset banner.

BUG=340803
TEST=NONE

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250144

[robertshield, 2014-02-04 22:26:54]

Gab, Erik, please take an initial look at the prefs code. If that seems sane,
I'll head for prefs and webui owners.

[gab, 2014-02-05 15:23:52]

Awesome :)!

Comments below.

Cheers!
Gab

https://codereview.chromium.org/151003004/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:10436: +          Learn more
Isn't there a generic "Learn more" string already?

i.e. IDS_LEARN_MORE?

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.cc:168: void
PrefHashStoreImpl::RecordResetEvent() {
I think this method belongs on TrackedPreferenceHelper like the UMA reporting
methods.

The PrefHashStore only knows about hashes, not resets.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/automatic_settings_reset_banner.html
(right):

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/automatic_settings_reset_banner.html:8: </span>
Should that span just use the

<span i18n-values="..." />

syntax? Or does our style prefer always having an explicit open/close scope for
HTML tags?

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/automatic_settings_reset_banner.html:11:
i18n-content="learnMore" target="_blank"></a>
Same question about this <a> tag.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc
(right):

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:26: void
AutomaticSettingsResetHandler::InitializePage() {
Log UMA stat about impression of this UI?

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:27:
PrefService* local_state = g_browser_process->local_state();
const (or simply inline the call since you only need local_state once)

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:31:
base::Time now = base::Time::Now();
const

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:32:
base::Time then = base::Time::FromInternalValue(reset_time);
const

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:35: }
remove {}

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:43: static
OptionsStringResource resources[] = {
static const

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:45:
IDS_AUTOMATIC_SETTINGS_RESET_BANNER_TEXT },
Align flush with string above (same below); e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:59:
OnDismissedAutomaticSettingsResetBanner,
This is a continuation of the above method name; either indent 4 spaces from
here; or preferably, indent all the arguments 4 spaces on the next line after
"base::Bind(" if this makes this one fit on a single line

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:60:
base::Unretained(this)));
This depends on the lifetime of this handler (which I think is bound to the
lifetime of the OptionsUI -- this could cause a crash if the settings page is
closed before this callback is called I think. 

Instead make OnDismissedAutomaticSettingsResetBanner() an anonymous method (it
doesn't need any class data anyways); the callback will then be forever valid
(as long as g_browser_process != NULL but I feel g_browser_process must be reset
way after the browser is done pumping tasks).

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:64: const
base::ListValue* value) {
Log UMA stat here?

How would we log UMA for UI only events (i.e. clicking Learn More)?

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.h (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.h:17: public
base::SupportsWeakPtr<AutomaticSettingsResetHandler> {
Doesn't seem you use weak ptrs; remove this?

https://codereview.chromium.org/151003004/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:2635: const char kProfilePreferenceResetTime[] =
"profile.preference_reset_time";
This the the local state section (profile prefs are between line 12 and 1318 --
obvious I know ;-)!).

Profile prefs do not need a "profile." prefix in their name either; consider
something like:

settings.last_reset_time

https://codereview.chromium.org/151003004/diff/20001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.h:931: extern const char kProfilePreferenceResetTime[];
Move to match new suggested position in the .cc file.

[erikwright (departed), 2014-02-05 15:50:03]

I don't like the mechanism that triggers the UI via a preference value:

(1) It seems inappropriate to be persisting this preference value across reboots
when it is only really used for intra-process state.
(2) It makes an assumption about the order of things (preference validation
happening prior to the UI being initialized) that is not self-evident.
(3) It's spooky action at a distance - it's hard to follow the thread to see the
cause & effect.

I propose the following:

a) Introduce an on_reset_handler Closure parameter to the PrefHashFilter
constructor.
b) In chrome_pref_service_factory, implement this closure to trigger the UI.
c) Update TrackedPreference::EnforceAndReport to return true if reset and false
otherwise.
d) Update PrefHashFilter::FilterOnLoad to call on_reset_handler_ if any of the
TrackedPreferences returns true.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.h (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.h:13: //
Handler for both the banner that displays a settings reset event at the
"both"?

[gab, 2014-02-05 15:58:11]

On 2014/02/05 15:50:03, erikwright wrote:
> I don't like the mechanism that triggers the UI via a preference value:
> 

I disagree:

> (1) It seems inappropriate to be persisting this preference value across
reboots
> when it is only really used for intra-process state.

This is the desired UX (otherwise the user may close Chrome thinking a restart
will help fix his state and be left with nothing on restart).

> (2) It makes an assumption about the order of things (preference validation
> happening prior to the UI being initialized) that is not self-evident.

That is self-evident IMO; we put our system before we even hand preferences to
Chrome; it doesn't make sense for the settings page to come up before Chrome is
aware of its preferences.

> (3) It's spooky action at a distance - it's hard to follow the thread to see
the
> cause & effect.

Isn't that what UI is all about?! :)

> 
> I propose the following:
> 
> a) Introduce an on_reset_handler Closure parameter to the PrefHashFilter
> constructor.
> b) In chrome_pref_service_factory, implement this closure to trigger the UI.
> c) Update TrackedPreference::EnforceAndReport to return true if reset and
false
> otherwise.
> d) Update PrefHashFilter::FilterOnLoad to call on_reset_handler_ if any of the
> TrackedPreferences returns true.
> 
>
https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/options/automatic_settings_reset_handler.h
(right):
> 
>
https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/options/automatic_settings_reset_handler.h:13: //
> Handler for both the banner that displays a settings reset event at the
> "both"?

[erikwright (departed), 2014-02-05 16:22:38]

On Wed, Feb 5, 2014 at 10:58 AM, <gab@chromium.org> wrote:

> On 2014/02/05 15:50:03, erikwright wrote:
>
>> I don't like the mechanism that triggers the UI via a preference value:
>>
>
>
> I disagree:
>
>
>  (1) It seems inappropriate to be persisting this preference value across
>>
> reboots
>
>> when it is only really used for intra-process state.
>>
>
> This is the desired UX (otherwise the user may close Chrome thinking a
> restart
> will help fix his state and be left with nothing on restart).


Is it correct that the dialog is displayed on every launch for X days after
a reset?

It might be appropriate to maintain some data between launches but I would
still prefer to use the callback mechanism to set the state rather than
burying it in PrefHashStoreImpl.

 (2) It makes an assumption about the order of things (preference validation
>> happening prior to the UI being initialized) that is not self-evident.
>>
>
> That is self-evident IMO; we put our system before we even hand
> preferences to
> Chrome; it doesn't make sense for the settings page to come up before
> Chrome is
> aware of its preferences.


It's not "coming up" - it's just initializing a handler for an Options
page. It seems inappropriate for me for an initialization of an options
page to potentially trigger UI actions.

I would define a PreferenceResetState object. It takes a pointer to the
PersistentPrefStore where it stores its state, and it exposes a method that
can be called (via a Closure) when a reset event takes place. It exposes
some other method that is called at the appropriate time to potentially
display UI, and it uses its state (internal or in preferences) to decide
whether to do that.

I would construct and wire up this object in chrome_pref_service_factory.cc


>
>
>  (3) It's spooky action at a distance - it's hard to follow the thread to
>> see
>>
> the
>
>> cause & effect.
>>
>
> Isn't that what UI is all about?! :)


Now you're just looking for a fight.. Why, if I were in Montreal, I'd...!

>
>
>
>  I propose the following:
>>
>
>  a) Introduce an on_reset_handler Closure parameter to the PrefHashFilter
>> constructor.
>> b) In chrome_pref_service_factory, implement this closure to trigger the
>> UI.
>> c) Update TrackedPreference::EnforceAndReport to return true if reset and
>>
> false
>
>> otherwise.
>> d) Update PrefHashFilter::FilterOnLoad to call on_reset_handler_ if any
>> of the
>> TrackedPreferences returns true.
>>
>
>
> https://codereview.chromium.org/151003004/diff/20001/
> chrome/browser/ui/webui/options/automatic_settings_reset_handler.h
>
>> File chrome/browser/ui/webui/options/automatic_settings_reset_handler.h
>>
> (right):
>
>
> https://codereview.chromium.org/151003004/diff/20001/
> chrome/browser/ui/webui/options/automatic_settings_
> reset_handler.h#newcode13
>
>> chrome/browser/ui/webui/options/automatic_settings_reset_handler.h:13: //
>> Handler for both the banner that displays a settings reset event at the
>> "both"?
>>
>
>
>
> https://codereview.chromium.org/151003004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[robertshield, 2014-02-05 18:35:00]

Hey, no fighting on my CL reviews!

Spoke with Erik, explained what the UI looks like and why the current approach
makes sense, will move the writing of the reset timestamp into PrefHashFilter
and out of PrefHashStore.

Will address other comments shortly, thanks again!

On 2014/02/05 16:22:38, erikwright wrote:
> On Wed, Feb 5, 2014 at 10:58 AM, <mailto:gab@chromium.org> wrote:
> 
> > On 2014/02/05 15:50:03, erikwright wrote:
> >
> >> I don't like the mechanism that triggers the UI via a preference value:
> >>
> >
> >
> > I disagree:
> >
> >
> >  (1) It seems inappropriate to be persisting this preference value across
> >>
> > reboots
> >
> >> when it is only really used for intra-process state.
> >>
> >
> > This is the desired UX (otherwise the user may close Chrome thinking a
> > restart
> > will help fix his state and be left with nothing on restart).
> 
> 
> Is it correct that the dialog is displayed on every launch for X days after
> a reset?
> 
> It might be appropriate to maintain some data between launches but I would
> still prefer to use the callback mechanism to set the state rather than
> burying it in PrefHashStoreImpl.
> 
>  (2) It makes an assumption about the order of things (preference validation
> >> happening prior to the UI being initialized) that is not self-evident.
> >>
> >
> > That is self-evident IMO; we put our system before we even hand
> > preferences to
> > Chrome; it doesn't make sense for the settings page to come up before
> > Chrome is
> > aware of its preferences.
> 
> 
> It's not "coming up" - it's just initializing a handler for an Options
> page. It seems inappropriate for me for an initialization of an options
> page to potentially trigger UI actions.
> 
> I would define a PreferenceResetState object. It takes a pointer to the
> PersistentPrefStore where it stores its state, and it exposes a method that
> can be called (via a Closure) when a reset event takes place. It exposes
> some other method that is called at the appropriate time to potentially
> display UI, and it uses its state (internal or in preferences) to decide
> whether to do that.
> 
> I would construct and wire up this object in chrome_pref_service_factory.cc
> 
> 
> >
> >
> >  (3) It's spooky action at a distance - it's hard to follow the thread to
> >> see
> >>
> > the
> >
> >> cause & effect.
> >>
> >
> > Isn't that what UI is all about?! :)
> 
> 
> Now you're just looking for a fight.. Why, if I were in Montreal, I'd...!
> 
> >
> >
> >
> >  I propose the following:
> >>
> >
> >  a) Introduce an on_reset_handler Closure parameter to the PrefHashFilter
> >> constructor.
> >> b) In chrome_pref_service_factory, implement this closure to trigger the
> >> UI.
> >> c) Update TrackedPreference::EnforceAndReport to return true if reset and
> >>
> > false
> >
> >> otherwise.
> >> d) Update PrefHashFilter::FilterOnLoad to call on_reset_handler_ if any
> >> of the
> >> TrackedPreferences returns true.
> >>
> >
> >
> > https://codereview.chromium.org/151003004/diff/20001/
> > chrome/browser/ui/webui/options/automatic_settings_reset_handler.h
> >
> >> File chrome/browser/ui/webui/options/automatic_settings_reset_handler.h
> >>
> > (right):
> >
> >
> > https://codereview.chromium.org/151003004/diff/20001/
> > chrome/browser/ui/webui/options/automatic_settings_
> > reset_handler.h#newcode13
> >
> >> chrome/browser/ui/webui/options/automatic_settings_reset_handler.h:13: //
> >> Handler for both the banner that displays a settings reset event at the
> >> "both"?
> >>
> >
> >
> >
> > https://codereview.chromium.org/151003004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[robertshield, 2014-02-06 18:56:50]

Thanks for your comments!

Moved the preference writing to PrefHashFilter, still writing to LocalState as
the user PrefService isn't up yet at time of filtering.

PTAL!

https://codereview.chromium.org/151003004/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:10436: +          Learn more
On 2014/02/05 15:23:52, gab wrote:
> Isn't there a generic "Learn more" string already?
> 
> i.e. IDS_LEARN_MORE?

There is! Removed this one.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_store_impl.cc (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_store_impl.cc:168: void
PrefHashStoreImpl::RecordResetEvent() {
On 2014/02/05 15:23:52, gab wrote:
> I think this method belongs on TrackedPreferenceHelper like the UMA reporting
> methods.
> 
> The PrefHashStore only knows about hashes, not resets.

Erik convinced me it should go on PrefHashFilter. Moved there.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/automatic_settings_reset_banner.html
(right):

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/automatic_settings_reset_banner.html:8: </span>
On 2014/02/05 15:23:52, gab wrote:
> Should that span just use the
> 
> <span i18n-values="..." />
> 
> syntax? Or does our style prefer always having an explicit open/close scope
for
> HTML tags?

Quick search suggests the explicit scope is more common.
http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml doesn't
explicitly cover this case. I'll leave it as is.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/automatic_settings_reset_banner.html:11:
i18n-content="learnMore" target="_blank"></a>
On 2014/02/05 15:23:52, gab wrote:
> Same question about this <a> tag.

Same answer.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc
(right):

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:26: void
AutomaticSettingsResetHandler::InitializePage() {
On 2014/02/05 15:23:52, gab wrote:
> Log UMA stat about impression of this UI?

Already done. See JS file.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:27:
PrefService* local_state = g_browser_process->local_state();
On 2014/02/05 15:23:52, gab wrote:
> const (or simply inline the call since you only need local_state once)

Done.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:31:
base::Time now = base::Time::Now();
On 2014/02/05 15:23:52, gab wrote:
> const

Done.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:32:
base::Time then = base::Time::FromInternalValue(reset_time);
On 2014/02/05 15:23:52, gab wrote:
> const

Done.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:35: }
On 2014/02/05 15:23:52, gab wrote:
> remove {}

Done.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:43: static
OptionsStringResource resources[] = {
On 2014/02/05 15:23:52, gab wrote:
> static const

Done.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:45:
IDS_AUTOMATIC_SETTINGS_RESET_BANNER_TEXT },
On 2014/02/05 15:23:52, gab wrote:
> Align flush with string above (same below); e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Done.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:59:
OnDismissedAutomaticSettingsResetBanner,
On 2014/02/05 15:23:52, gab wrote:
> This is a continuation of the above method name; either indent 4 spaces from
> here; or preferably, indent all the arguments 4 spaces on the next line after
> "base::Bind(" if this makes this one fit on a single line

Done.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:60:
base::Unretained(this)));
On 2014/02/05 15:23:52, gab wrote:
> This depends on the lifetime of this handler (which I think is bound to the
> lifetime of the OptionsUI -- this could cause a crash if the settings page is
> closed before this callback is called I think. 
> 
> Instead make OnDismissedAutomaticSettingsResetBanner() an anonymous method (it
> doesn't need any class data anyways); the callback will then be forever valid
> (as long as g_browser_process != NULL but I feel g_browser_process must be
reset
> way after the browser is done pumping tasks).

A nicer way is to use a weak ptr, which this was meant to do but I typo'd.
Sorry.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:64: const
base::ListValue* value) {
On 2014/02/05 15:23:52, gab wrote:
> Log UMA stat here?
> 
> How would we log UMA for UI only events (i.e. clicking Learn More)?

This is already logged via user action counts. See JS file for implementation.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.h (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.h:13: //
Handler for both the banner that displays a settings reset event at the
On 2014/02/05 15:50:03, erikwright wrote:
> "both"?

yes, both the banner and the banner. Also, the banner. Fixed.

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.h:17: public
base::SupportsWeakPtr<AutomaticSettingsResetHandler> {
On 2014/02/05 15:23:52, gab wrote:
> Doesn't seem you use weak ptrs; remove this?

Using them now. See .cc file.

https://codereview.chromium.org/151003004/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:2635: const char kProfilePreferenceResetTime[] =
"profile.preference_reset_time";
On 2014/02/05 15:23:52, gab wrote:
> This the the local state section (profile prefs are between line 12 and 1318
--
> obvious I know ;-)!).
> 
> Profile prefs do not need a "profile." prefix in their name either; consider
> something like:
> 
> settings.last_reset_time

This is a local state pref as discussed. Will keep it there for now, and leave a
todo for Erik to store it in the same place as tracked preferences.

https://codereview.chromium.org/151003004/diff/20001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.h:931: extern const char kProfilePreferenceResetTime[];
On 2014/02/05 15:23:52, gab wrote:
> Move to match new suggested position in the .cc file.

As discussed, this is the right place for it for now.

[erikwright (departed), 2014-02-06 20:04:03]

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:242:
PrefHashFilter::RegisterPrefs(registry);
I'd consider having a single call to chrome_prefs:: and having it delegate to
these two

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:95: virtual void WriteResetEvent();
Use a callback instead. Isolates the global state access to
chrome_pref_service_factory

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_atomic_preference.cc (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_atomic_preference.cc:52: return was_reset;
why not just return action == DO_RESET

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_split_preference.cc (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_split_preference.cc:78: return was_reset;
ditto

[gab, 2014-02-06 20:40:11]

Quick second pass; will wait for next cycle, I think you can also start looping
in webui owners.

Cheers,
Gab

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc
(right):

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:60:
base::Unretained(this)));
On 2014/02/06 18:56:50, robertshield wrote:
> On 2014/02/05 15:23:52, gab wrote:
> > This depends on the lifetime of this handler (which I think is bound to the
> > lifetime of the OptionsUI -- this could cause a crash if the settings page
is
> > closed before this callback is called I think. 
> > 
> > Instead make OnDismissedAutomaticSettingsResetBanner() an anonymous method
(it
> > doesn't need any class data anyways); the callback will then be forever
valid
> > (as long as g_browser_process != NULL but I feel g_browser_process must be
> reset
> > way after the browser is done pumping tasks).
> 
> A nicer way is to use a weak ptr, which this was meant to do but I typo'd.
> Sorry. 

The weak ptr is not nicer than an anonymous method when possible IMO; the
WeakPtr makes it such that if this is deleted, your callback won't be called.

In this specific case it means that if the OptionsBlah object owning this goes
away (chrome://settings is closed?) before you callback, you never get it.

Ideally I think if the user X's out the banner and quickly close the settings
tab. Even if your callback is called later it should still be able to set the
pref (no reason it should require this UI handler to still be up).

https://codereview.chromium.org/151003004/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:2635: const char kProfilePreferenceResetTime[] =
"profile.preference_reset_time";
On 2014/02/06 18:56:50, robertshield wrote:
> On 2014/02/05 15:23:52, gab wrote:
> > This the the local state section (profile prefs are between line 12 and 1318
> --
> > obvious I know ;-)!).
> > 
> > Profile prefs do not need a "profile." prefix in their name either; consider
> > something like:
> > 
> > settings.last_reset_time
> 
> This is a local state pref as discussed. Will keep it there for now, and leave
a
> todo for Erik to store it in the same place as tracked preferences.

You didn't actually leave it here :).

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:98: did_reset =
it->second->EnforceAndReport(pref_store_contents) || did_reset;
I prefer

if (it->second->EnforceAndReport(pref_store_contents))
  did_reset = true;

The current form otherwise relies on the call being made by the order of the
boolean expression (i.e., if you swap the expressions on either side of the ||
this no longer works) which is harder to read IMO.

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:212: class FakePrefHashFilter
: public PrefHashFilter {
If you use the callback approach, you don't need this class.

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_atomic_preference.cc (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_atomic_preference.cc:52: return was_reset;
On 2014/02/06 20:04:03, erikwright wrote:
> why not just return action == DO_RESET

I'd argue that it's nicer to have |was_reset| because it keeps the fact that
reset means "action == DO_RESET" in a single place (otherwise; should this ever
change and the return value not also be fixed, we have a bug).

[erikwright (departed), 2014-02-06 20:51:56]

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_atomic_preference.cc (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_atomic_preference.cc:52: return was_reset;
On 2014/02/06 20:40:11, gab wrote:
> On 2014/02/06 20:04:03, erikwright wrote:
> > why not just return action == DO_RESET
> 
> I'd argue that it's nicer to have |was_reset| because it keeps the fact that
> reset means "action == DO_RESET" in a single place (otherwise; should this
ever
> change and the return value not also be fixed, we have a bug).

I don't mind either way.

[robertshield, 2014-02-07 04:36:18]

Thanks Gab and Erik, PTAL.

Adding OWNERs now.

bauerb@: please could you look at the prefs, and webui code?
asvitkine@: please could you look at the tools/metrics changes?

Thank you!

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc
(right):

https://codereview.chromium.org/151003004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:60:
base::Unretained(this)));
On 2014/02/06 20:40:11, gab wrote:
> On 2014/02/06 18:56:50, robertshield wrote:
> > On 2014/02/05 15:23:52, gab wrote:
> > > This depends on the lifetime of this handler (which I think is bound to
the
> > > lifetime of the OptionsUI -- this could cause a crash if the settings page
> is
> > > closed before this callback is called I think. 
> > > 
> > > Instead make OnDismissedAutomaticSettingsResetBanner() an anonymous method
> (it
> > > doesn't need any class data anyways); the callback will then be forever
> valid
> > > (as long as g_browser_process != NULL but I feel g_browser_process must be
> > reset
> > > way after the browser is done pumping tasks).
> > 
> > A nicer way is to use a weak ptr, which this was meant to do but I typo'd.
> > Sorry. 
> 
> The weak ptr is not nicer than an anonymous method when possible IMO; the
> WeakPtr makes it such that if this is deleted, your callback won't be called.
> 
> In this specific case it means that if the OptionsBlah object owning this goes
> away (chrome://settings is closed?) before you callback, you never get it.
> 
> Ideally I think if the user X's out the banner and quickly close the settings
> tab. Even if your callback is called later it should still be able to set the
> pref (no reason it should require this UI handler to still be up).

Done.

https://codereview.chromium.org/151003004/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/151003004/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:2635: const char kProfilePreferenceResetTime[] =
"profile.preference_reset_time";
On 2014/02/06 20:40:11, gab wrote:
> On 2014/02/06 18:56:50, robertshield wrote:
> > On 2014/02/05 15:23:52, gab wrote:
> > > This the the local state section (profile prefs are between line 12 and
1318
> > --
> > > obvious I know ;-)!).
> > > 
> > > Profile prefs do not need a "profile." prefix in their name either;
consider
> > > something like:
> > > 
> > > settings.last_reset_time
> > 
> > This is a local state pref as discussed. Will keep it there for now, and
leave
> a
> > todo for Erik to store it in the same place as tracked preferences.
> 
> You didn't actually leave it here :).

Done.

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:242:
PrefHashFilter::RegisterPrefs(registry);
On 2014/02/06 20:04:03, erikwright wrote:
> I'd consider having a single call to chrome_prefs:: and having it delegate to
> these two

Done.

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:98: did_reset =
it->second->EnforceAndReport(pref_store_contents) || did_reset;
On 2014/02/06 20:40:11, gab wrote:
> I prefer
> 
> if (it->second->EnforceAndReport(pref_store_contents))
>   did_reset = true;
> 
> The current form otherwise relies on the call being made by the order of the
> boolean expression (i.e., if you swap the expressions on either side of the ||
> this no longer works) which is harder to read IMO.

Done.

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:95: virtual void WriteResetEvent();
On 2014/02/06 20:04:03, erikwright wrote:
> Use a callback instead. Isolates the global state access to
> chrome_pref_service_factory

Ok. Was torn between that and preserving all pref hash logic here. Switched to
callback.

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:212: class FakePrefHashFilter
: public PrefHashFilter {
On 2014/02/06 20:40:11, gab wrote:
> If you use the callback approach, you don't need this class.

Indeed.

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_atomic_preference.cc (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_atomic_preference.cc:52: return was_reset;
On 2014/02/06 20:51:57, erikwright wrote:
> On 2014/02/06 20:40:11, gab wrote:
> > On 2014/02/06 20:04:03, erikwright wrote:
> > > why not just return action == DO_RESET
> > 
> > I'd argue that it's nicer to have |was_reset| because it keeps the fact that
> > reset means "action == DO_RESET" in a single place (otherwise; should this
> ever
> > change and the return value not also be fixed, we have a bug).
> 
> I don't mind either way.

Mildest of preferences for as is for the reason Gab stated.

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_atomic_preference.cc:52: return was_reset;
On 2014/02/06 20:40:11, gab wrote:
> On 2014/02/06 20:04:03, erikwright wrote:
> > why not just return action == DO_RESET
> 
> I'd argue that it's nicer to have |was_reset| because it keeps the fact that
> reset means "action == DO_RESET" in a single place (otherwise; should this
ever
> change and the return value not also be fixed, we have a bug).

Done.

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_atomic_preference.cc:52: return was_reset;
On 2014/02/06 20:04:03, erikwright wrote:
> why not just return action == DO_RESET

Done.

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
File chrome/browser/prefs/tracked/tracked_split_preference.cc (right):

https://codereview.chromium.org/151003004/diff/310001/chrome/browser/prefs/tr...
chrome/browser/prefs/tracked/tracked_split_preference.cc:78: return was_reset;
On 2014/02/06 20:04:03, erikwright wrote:
> ditto

ditto

[Bernhard Bauer, 2014-02-07 08:14:52]

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:147: 
What does this do?

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:35: typedef base::Callback<void(void)>
PrefResetCallback;
This is typedef'd as a base::Closure already.

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/resource...
File chrome/browser/resources/options/automatic_settings_reset_banner.css
(right):

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/resource...
chrome/browser/resources/options/automatic_settings_reset_banner.css:5:
#automatic-settings-reset-banner {
Any chance you can share styles with the reset profile settings banner?

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/resource...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/resource...
chrome/browser/resources/options/automatic_settings_reset_banner.js:15: function
AutomaticSettingsResetBanner() {}
Same here. Maybe a shared base class?

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.h (right):

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.h:30: static
void OnDismissedAutomaticSettingsResetBanner(
If it's static and private, can you move it to an anonymous namespace?

[robertshield, 2014-02-07 19:08:28]

Thanks Barnhard, PTAL!

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:147: 
On 2014/02/07 08:14:53, Bernhard Bauer wrote:
> What does this do? 

Needlessly occupies valuable screen real estate. Sorry, cruft from an earlier
iteration. Removed.

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.h:35: typedef base::Callback<void(void)>
PrefResetCallback;
On 2014/02/07 08:14:53, Bernhard Bauer wrote:
> This is typedef'd as a base::Closure already.

Done.

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/resource...
File chrome/browser/resources/options/automatic_settings_reset_banner.css
(right):

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/resource...
chrome/browser/resources/options/automatic_settings_reset_banner.css:5:
#automatic-settings-reset-banner {
On 2014/02/07 08:14:53, Bernhard Bauer wrote:
> Any chance you can share styles with the reset profile settings banner?

And thus it was that I learned (some) CSS selector syntax. Done.

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/resource...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/resource...
chrome/browser/resources/options/automatic_settings_reset_banner.js:15: function
AutomaticSettingsResetBanner() {}
On 2014/02/07 08:14:53, Bernhard Bauer wrote:
> Same here. Maybe a shared base class?

Extracted most things to a base class. intialize() was different enough that I
left it here, and I couldn't figure out if it was possible to extract the "//
Forward public APIs to private implementations." block.

That block seems to set properties on the singleton instance of the derived
function which I couldn't see how to do from a super class.

I write non-trivial JS only slightly more often than I arm-wrestle unicorns so
apologies in advance if it is glaringly wrong.

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.h (right):

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.h:30: static
void OnDismissedAutomaticSettingsResetBanner(
On 2014/02/07 08:14:53, Bernhard Bauer wrote:
> If it's static and private, can you move it to an anonymous namespace?

Done.

[gab, 2014-02-07 19:38:30]

Final comments from me (I didn't look further at the UI as I know nothing about
how this works!).

Cheers,
Gab

https://codereview.chromium.org/151003004/diff/460003/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/151003004/diff/460003/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:247: reset_count_++;
nit: prefer pre-increment to post-increment.

https://codereview.chromium.org/151003004/diff/460003/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:254: int reset_count_;
"reset count" is kind of wrong here; this not the "number of resets", but rather
the number of calls which results in resets.

how about "reset_event_count" or something? w/ a comment

https://codereview.chromium.org/151003004/diff/460003/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc
(right):

https://codereview.chromium.org/151003004/diff/460003/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:61: //
static
Still prefer anonymous method (i.e. unnamed namespace in .cc file) to static
method, from style guide:

"If you must define a nonmember function and it is only needed in its .cc file,
use an unnamed namespace or static linkage... " -- but here by "static" they
mean static in .cc file which means it's local to the .cc file; not static as in
static class method.

https://codereview.chromium.org/151003004/diff/780001/chrome/browser/prefs/ch...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/151003004/diff/780001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:425:
PrefHashFilter::RegisterPrefs(registry);
PrefHashFilter no longer sets the pref so it should also no longer be the one
registering it (i.e. registration for kProfilePreferenceResetTime should be
here).

https://codereview.chromium.org/151003004/diff/780001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/151003004/diff/780001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:213: 
rm empty line

[robertshield, 2014-02-07 21:21:34]

Thanks Gab!

https://codereview.chromium.org/151003004/diff/460003/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/151003004/diff/460003/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:247: reset_count_++;
On 2014/02/07 19:38:31, gab wrote:
> nit: prefer pre-increment to post-increment.

Done.

https://codereview.chromium.org/151003004/diff/460003/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:254: int reset_count_;
On 2014/02/07 19:38:31, gab wrote:
> "reset count" is kind of wrong here; this not the "number of resets", but
rather
> the number of calls which results in resets.
> 
> how about "reset_event_count" or something? w/ a comment

Done.

https://codereview.chromium.org/151003004/diff/460003/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc
(right):

https://codereview.chromium.org/151003004/diff/460003/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:61: //
static
On 2014/02/07 19:38:31, gab wrote:
> Still prefer anonymous method (i.e. unnamed namespace in .cc file) to static
> method, from style guide:
> 
> "If you must define a nonmember function and it is only needed in its .cc
file,
> use an unnamed namespace or static linkage... " -- but here by "static" they
> mean static in .cc file which means it's local to the .cc file; not static as
in
> static class method.

Already done in current patchset. Apologies for confusion.

https://codereview.chromium.org/151003004/diff/780001/chrome/browser/prefs/ch...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/151003004/diff/780001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:425:
PrefHashFilter::RegisterPrefs(registry);
On 2014/02/07 19:38:31, gab wrote:
> PrefHashFilter no longer sets the pref so it should also no longer be the one
> registering it (i.e. registration for kProfilePreferenceResetTime should be
> here).

Done.

https://codereview.chromium.org/151003004/diff/780001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/151003004/diff/780001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter_unittest.cc:213: 
On 2014/02/07 19:38:31, gab wrote:
> rm empty line

Done.

[gab, 2014-02-07 22:00:18]

lgtm w/ nits.

Cheers!
Gab

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/c...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/c...
chrome/browser/prefs/chrome_pref_service_factory.cc:426: // reset event
occurred. Used to decide whether to show UI.
I don't think a comment about what the pref does is necessary; all details
should be on the pref declaration's comment IMO.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.cc:18: #include
"chrome/common/pref_names.h"
Remove all of the no longer necessary includes above.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:13: #include "base/bind.h"
Remove this include.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:14: #include "base/callback.h"
Include "base/callback_forward.h" in the header and "base/callback.h" only in
the .cc file.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:22: class PrefRegistrySimple;
Remove this.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter_unittest.cc:219: const base::Closure&
reset_callback) {
#include "base/callback_forward.h" for this.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc
(right):

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:17:
namespace {
Empty line at beginning/end of namespace.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:21:
namespace {
You have two anonymous namespaces :) -- merge this with the one above.

[Bernhard Bauer, 2014-02-08 17:38:45]

LGTM!

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/resource...
File chrome/browser/resources/options/automatic_settings_reset_banner.js
(right):

https://codereview.chromium.org/151003004/diff/620001/chrome/browser/resource...
chrome/browser/resources/options/automatic_settings_reset_banner.js:15: function
AutomaticSettingsResetBanner() {}
On 2014/02/07 19:08:28, robertshield wrote:
> On 2014/02/07 08:14:53, Bernhard Bauer wrote:
> > Same here. Maybe a shared base class?
> 
> Extracted most things to a base class. intialize() was different enough that I
> left it here, and I couldn't figure out if it was possible to extract the "//
> Forward public APIs to private implementations." block.
> 
> That block seems to set properties on the singleton instance of the derived
> function which I couldn't see how to do from a super class.
> 
> I write non-trivial JS only slightly more often than I arm-wrestle unicorns so
> apologies in advance if it is glaringly wrong.

Yes, it's fine this way. Thanks!

https://codereview.chromium.org/151003004/diff/760002/chrome/browser/resource...
File chrome/browser/resources/options/settings_banner.js (right):

https://codereview.chromium.org/151003004/diff/760002/chrome/browser/resource...
chrome/browser/resources/options/settings_banner.js:77:
$(this.setVisibilibyDomElementSelector_).hidden = !show;
I think I'd slightly prefer passing in the actual DOM element instead of the
selector; it seems more dependency-injection-y :)

[robertshield, 2014-02-10 02:30:52]

Thanks Gab and Bernhard!

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/c...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/c...
chrome/browser/prefs/chrome_pref_service_factory.cc:426: // reset event
occurred. Used to decide whether to show UI.
On 2014/02/07 22:00:19, gab wrote:
> I don't think a comment about what the pref does is necessary; all details
> should be on the pref declaration's comment IMO.

Done.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.cc:18: #include
"chrome/common/pref_names.h"
On 2014/02/07 22:00:19, gab wrote:
> Remove all of the no longer necessary includes above.

Done.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:13: #include "base/bind.h"
On 2014/02/07 22:00:19, gab wrote:
> Remove this include.

Done.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:14: #include "base/callback.h"
On 2014/02/07 22:00:19, gab wrote:
> Include "base/callback_forward.h" in the header and "base/callback.h" only in
> the .cc file.

No can do, there is a callback instance declared in the class (not a pointer or
a ref), so the full header is needed (unless there is another trick I am unaware
of).

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:22: class PrefRegistrySimple;
On 2014/02/07 22:00:19, gab wrote:
> Remove this.

Done.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter_unittest.cc (right):

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter_unittest.cc:219: const base::Closure&
reset_callback) {
On 2014/02/07 22:00:19, gab wrote:
> #include "base/callback_forward.h" for this.

Done.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc
(right):

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:17:
namespace {
On 2014/02/07 22:00:19, gab wrote:
> Empty line at beginning/end of namespace.

Done.

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/automatic_settings_reset_handler.cc:21:
namespace {
On 2014/02/07 22:00:19, gab wrote:
> You have two anonymous namespaces :) -- merge this with the one above.

ugh, tired. thanks, done :)

https://codereview.chromium.org/151003004/diff/760002/chrome/browser/resource...
File chrome/browser/resources/options/settings_banner.js (right):

https://codereview.chromium.org/151003004/diff/760002/chrome/browser/resource...
chrome/browser/resources/options/settings_banner.js:77:
$(this.setVisibilibyDomElementSelector_).hidden = !show;
On 2014/02/08 17:38:46, Bernhard Bauer wrote:
> I think I'd slightly prefer passing in the actual DOM element instead of the
> selector; it seems more dependency-injection-y :)

Done.

[gab, 2014-02-10 04:02:20]

last nit; lgtm++

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/151003004/diff/1150001/chrome/browser/prefs/p...
chrome/browser/prefs/pref_hash_filter.h:14: #include "base/callback.h"
On 2014/02/10 02:30:53, robertshield wrote:
> On 2014/02/07 22:00:19, gab wrote:
> > Include "base/callback_forward.h" in the header and "base/callback.h" only
in
> > the .cc file.
> 
> No can do, there is a callback instance declared in the class (not a pointer
or
> a ref), so the full header is needed (unless there is another trick I am
unaware
> of).

Oh of course my bad.

https://codereview.chromium.org/151003004/diff/760002/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/151003004/diff/760002/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:7: #include "base/callback.h"
Don't include this; already included in header.

[robertshield, 2014-02-10 13:23:46]

The CQ bit was checked by robertshield@chromium.org

[commit-bot: I haz the power, 2014-02-10 13:24:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/151003004/15...

[commit-bot: I haz the power, 2014-02-10 13:51:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-10 13:51:58]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[robertshield, 2014-02-10 13:56:14]

@Alexei: ping :)

https://codereview.chromium.org/151003004/diff/760002/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_hash_filter.cc (right):

https://codereview.chromium.org/151003004/diff/760002/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_hash_filter.cc:7: #include "base/callback.h"
On 2014/02/10 04:02:21, gab wrote:
> Don't include this; already included in header.

Done.

[Alexei Svitkine (slow), 2014-02-10 14:41:38]

Actions lgtm, but it would good to make the script be able to scan the
Javascript code for user actions, like it does for C++, rather than having to
list them manually.

[robertshield, 2014-02-10 15:26:44]

The CQ bit was checked by robertshield@chromium.org

[commit-bot: I haz the power, 2014-02-10 15:26:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/151003004/15...

[commit-bot: I haz the power, 2014-02-10 17:55:33]

Change committed as 250144