Fix prefs registration for BrowserInstantController.

Fix prefs registration for BrowserInstantController.

All preferences should be registered _before_ a PrefService is created, 
so reading other prefs during registration is a no-no.

BUG=155525

[Jói, 2013-02-22 12:13:04]

mnissler: Entire change.

robertshield: Looks like you are familiar with the registration code there.

Will TBR ben@ for OWNERS approval once reviews are done.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2013-02-22 13:06:16]

https://codereview.chromium.org/12328054/diff/3001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12328054/diff/3001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:55:
prefs->GetBoolean(prefs::kInstantEnabled));
Note this is a change in semantics, not sure whether that's a good idea. I'd
rather change the code consuming kInstantExtendedEnabled to check for
IsDefaultValue() and if so, use the value of kInstantEnabled.

[robertshield, 2013-02-22 13:36:55]

One request for additional comments, also please could you expand the CL
description a bit to explain what is being fixed and why by mentioning the prefs
refactoring described in the bug?

https://codereview.chromium.org/12328054/diff/3001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12328054/diff/3001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:99: case
search::INSTANT_USE_EXISTING:  // Initialized in constructor.
It took me a few minutes to convince myself that this would work since it wasn't
obvious to me that the default value passed to RegisterBooleanPref wouldn't be
used if prefs->SetBoolean was called in the ctor. 

Please expand the comment to say that "Initialized in constructor." means that
the default value passed to RegisterBooleanPref will be ignored.

[Jói, 2013-02-22 15:53:19]

I uploaded a new version to address Robert's comments, but after taking a closer
look I think the implementation was already broken.  When
chrome::search::IsInstantExtendedAPIEnabled returns true,
BrowserInstantController will subscribe to changes in kInstantExtendedEnabled,
which is never changed outside of that class.  This will cause
BrowserInstantController::ResetInstant to never get called in this case, even
when e.g. extensions change the setting via the preference API, or a policy
change is detected.

I think the right thing to do is to get rid of the kInstantExtendedEnabled pref,
document that users may set the kInstantEnabled pref but should only read it via
BrowserInstantController::IsInstantEnabled, and there have logic that returns
the right thing depending on the value of
chrome::search::IsInstantExtendedAPIEnabled and the value of
search::GetInstantExtendedDefaultSetting (the field trial).

We could also completely hide the pref name within the BrowserInstantController
implementation. Perhaps that is a bit safer.

I'm happy to take on this implementation if we agree it's the right thing.

Cheers,
Jói

[robertshield, 2013-02-22 16:35:51]

On 2013/02/22 15:53:19, Jói wrote:
> I uploaded a new version to address Robert's comments, but after taking a
closer
> look I think the implementation was already broken.  When
> chrome::search::IsInstantExtendedAPIEnabled returns true,
> BrowserInstantController will subscribe to changes in kInstantExtendedEnabled,
> which is never changed outside of that class. 

I'm still learning how this code works, but is it not changed via the settings
dialog due to the code in browser_options_handler.cc:409 which duplicates the
instant_extended pref name in a hard coded string? Debugging, this seems to be
the case, as with instant extended enabled, I do see the ResetInstant being
called.

 This will cause
> BrowserInstantController::ResetInstant to never get called in this case, even
> when e.g. extensions change the setting via the preference API, or a policy
> change is detected.
> 
> I think the right thing to do is to get rid of the kInstantExtendedEnabled
pref,
> document that users may set the kInstantEnabled pref but should only read it
via
> BrowserInstantController::IsInstantEnabled, and there have logic that returns
> the right thing depending on the value of
> chrome::search::IsInstantExtendedAPIEnabled and the value of
> search::GetInstantExtendedDefaultSetting (the field trial).
> 
> We could also completely hide the pref name within the
BrowserInstantController
> implementation. Perhaps that is a bit safer.
> 

FWIW, that sounds much saner than the current implementation, let me check with
the 1993 folk to see why that wasn't done.

> I'm happy to take on this implementation if we agree it's the right thing.
> 
> Cheers,
> Jói

[Jói, 2013-02-22 16:37:57]

I uploaded a new patch with the approach I described.  I'm pretty sure this way
things are correct.

I couldn't fully hide the kInstantEnabled pref name because it's used in both
the policy code and the extensions API code for the preferences API, so instead
I just added to its documentation to clarify that
BrowserInstantController::IsInstantEnabled should be considered the source of
truth.

Cheers,
Jói

[Jói, 2013-02-22 16:39:46]

Argh, you're kidding me, a hard-coded string? :(  Let me look again.

On Fri, Feb 22, 2013 at 4:37 PM,  <joi@chromium.org> wrote:
> I uploaded a new patch with the approach I described.  I'm pretty sure this
> way
> things are correct.
>
> I couldn't fully hide the kInstantEnabled pref name because it's used in
> both
> the policy code and the extensions API code for the preferences API, so
> instead
> I just added to its documentation to clarify that
> BrowserInstantController::IsInstantEnabled should be considered the source
> of
> truth.
>
> Cheers,
> Jói
>
>
> https://codereview.chromium.org/12328054/

[robertshield, 2013-02-22 16:43:31]

On Fri, Feb 22, 2013 at 11:39 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> Argh, you're kidding me, a hard-coded string? :(  Let me look again.
>

I never kid, and don't call me Argh.


>
> On Fri, Feb 22, 2013 at 4:37 PM,  <joi@chromium.org> wrote:
> > I uploaded a new patch with the approach I described.  I'm pretty sure
> this
> > way
> > things are correct.
> >
> > I couldn't fully hide the kInstantEnabled pref name because it's used in
> > both
> > the policy code and the extensions API code for the preferences API, so
> > instead
> > I just added to its documentation to clarify that
> > BrowserInstantController::IsInstantEnabled should be considered the
> source
> > of
> > truth.
> >
> > Cheers,
> > Jói
> >
> >
> > https://codereview.chromium.org/12328054/
>

[sreeram, 2013-02-22 16:46:15]

What we were trying to do with the instant_extended.enabed pref is this:

Previously, the chrome://settings checkbox reflected the value of the
instant.enabled pref. Users may have disabled it, for example. It also syncs
across devices.

For the Instant Extended experience dogfood, we wanted to start from a "clean
slate". So, we introduced a new pref (instant_extended.enabled). The
chrome://settings page still has only one checkbox. When Chrome is running in
Instant Extended mode (as returned by
chrome::search::IsInstantExtendedAPIEnabled()), we switch that checkbox to
reflect the value of the instant_extended.enabled pref, instead of
instant.enabled.

The code in chrome::search::GetInstantExtendedDefaultSetting() says what to
start out this new pref with. Based on field trial params, we wanted the ability
to start with true (enabling the new experience by default), false or even just
whatever the user previously had with their instant.enabled pref.

Perhaps the previous code didn't quite achieve that? It looks to me that this
CL, by getting rid of instant_extended.enabled, doesn't let us do that either.

[sreeram, 2013-02-22 16:48:25]

Also, forgot to say: For the time being, we want to keep both the prefs, and
keep them separate. So, for example, after trying out the Instant Extended
experience, you may choose to restart Chrome with it disabled. In that case, we
want to go back to using the instant.enabled pref and whatever setting you had
for it.

[Jói, 2013-02-22 16:53:53]

OK, thanks Sreeram for the background.  In this case I think I know
how to solve things.

I think there may still be a bug w.r.t. the instant preference being
set by extensions (via the preferences API) or by policy in the case
where instant extended is enabled. If extensions or policy change the
kInstantEnabled pref in that scenario, it will have no effect. Perhaps
that is intentional?

Cheers,
Jói


On Fri, Feb 22, 2013 at 4:48 PM,  <sreeram@chromium.org> wrote:
> Also, forgot to say: For the time being, we want to keep both the prefs, and
> keep them separate. So, for example, after trying out the Instant Extended
> experience, you may choose to restart Chrome with it disabled. In that case,
> we
> want to go back to using the instant.enabled pref and whatever setting you
> had
> for it.
>
> https://codereview.chromium.org/12328054/

[sreeram, 2013-02-22 16:57:23]

On 2013/02/22 16:53:53, Jói wrote:
> I think there may still be a bug w.r.t. the instant preference being
> set by extensions (via the preferences API) or by policy in the case
> where instant extended is enabled. If extensions or policy change the
> kInstantEnabled pref in that scenario, it will have no effect. Perhaps
> that is intentional?

Correct. For now, instant_extended.enabled isn't exposed via policy or to
extensions. We'll do so when we are past dogfood/experimentation stage. When we
eventually launch, we'll delete instant.enabled.

[Jói, 2013-02-22 16:59:56]

Great, thanks for all the background Sreeram.  Could you take a look
at the latest version of this change and let me know if it looks good?
 This version should preserve the semantics that you desire.

Cheers,
Jói


On Fri, Feb 22, 2013 at 4:57 PM,  <sreeram@chromium.org> wrote:
> On 2013/02/22 16:53:53, Jói wrote:
>>
>> I think there may still be a bug w.r.t. the instant preference being
>> set by extensions (via the preferences API) or by policy in the case
>> where instant extended is enabled. If extensions or policy change the
>> kInstantEnabled pref in that scenario, it will have no effect. Perhaps
>> that is intentional?
>
>
> Correct. For now, instant_extended.enabled isn't exposed via policy or to
> extensions. We'll do so when we are past dogfood/experimentation stage. When
> we
> eventually launch, we'll delete instant.enabled.
>
> https://codereview.chromium.org/12328054/

[sreeram, 2013-02-22 17:05:34]

lgtm

[Mattias Nissler (ping if slow), 2013-02-22 17:07:55]

https://codereview.chromium.org/12328054/diff/1006/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/12328054/diff/1006/chrome/browser/ui/browser_...
chrome/browser/ui/browser_instant_controller.cc:59:
prefs->GetBoolean(prefs::kInstantEnabled));
After all the discussion, I don't think copying the value is a good idea, given
that he existing value may come from policy or whatever and coping that doesn't
reflect what we want.

Can we:

(1) not copy pref values

(2) Change IsInstantEnabled below to look at both prefs and use the appropriate
one as determined by 

(search::GetInstantExtendedDefaultSetting() == search::INSTANT_USE_EXISTING &&
prefs->FindPref(prefs::kInstantExtendedEnabled)->IsDefaultValue())

(3) wire the pref registrar to both pref names so we're sure to pick up changes
no matter which pref changes?

[sreeram, 2013-02-22 17:09:52]

Oh wait. One question: I guess the prefs->SetBoolean() call in the
BrowserInstantController ctor will cause the pref to no longer be
IsDefaultValue()? If so, this slightly changes the old semantics:

Previously, if a user ran with extended mode, and say the field trial said
"default to false", the pref would be false. But later, after restarting Chrome,
if the field trial now said "default to true", the pref would be true.

Now, it looks like, after you run with extended mode for the first time, you'll
get whatever field trial was in effect at that time as the default. Even if you
don't touch the pref manually, future field trials wouldn't be able to toggle
the default.

[Mattias Nissler (ping if slow), 2013-02-22 17:11:03]

On Fri, Feb 22, 2013 at 6:09 PM, <sreeram@chromium.org> wrote:

> Oh wait. One question: I guess the prefs->SetBoolean() call in the
> BrowserInstantController ctor will cause the pref to no longer be
> IsDefaultValue()? If so, this slightly changes the old semantics:
>
> Previously, if a user ran with extended mode, and say the field trial said
> "default to false", the pref would be false. But later, after restarting
> Chrome,
> if the field trial now said "default to true", the pref would be true.
>
> Now, it looks like, after you run with extended mode for the first time,
> you'll
> get whatever field trial was in effect at that time as the default. Even
> if you
> don't touch the pref manually, future field trials wouldn't be able to
> toggle
> the default.
>

Correct, what I proposed should address that.


>
>
https://codereview.chromium.**org/12328054/<https://codereview.chromium.org/1...
>

[sreeram, 2013-02-22 17:13:43]

On 2013/02/22 17:11:03, Mattias Nissler wrote:
> Correct, what I proposed should address that.

Sounds good. We'll also need to change the webui code to then ask for
IsInstantEnabled() and use that as the setting for the checkbox, instead of
directly relying on the pref value.

[Jói, 2013-02-22 17:44:29]

> Sounds good. We'll also need to change the webui code to then ask for
> IsInstantEnabled() and use that as the setting for the checkbox, instead of
> directly relying on the pref value.

Thanks. Could you point me to the WebUI code that fetches the pref
value?  The only thing I can find is in browser_options_handler.cc
where it sets a key "instant_enabled" to different values depending on
chrome::search::IsInstantExtendedAPIEnabled but I don't see where it
actually tells the WebUI page whether or not instant is enabled.

Cheers,
Jói


On Fri, Feb 22, 2013 at 5:13 PM,  <sreeram@chromium.org> wrote:
> On 2013/02/22 17:11:03, Mattias Nissler wrote:
>>
>> Correct, what I proposed should address that.
>
>
> Sounds good. We'll also need to change the webui code to then ask for
> IsInstantEnabled() and use that as the setting for the checkbox, instead of
> directly relying on the pref value.
>
> https://codereview.chromium.org/12328054/

[sreeram, 2013-02-22 18:09:59]

On 2013/02/22 17:44:29, Jói wrote:
> > Sounds good. We'll also need to change the webui code to then ask for
> > IsInstantEnabled() and use that as the setting for the checkbox, instead of
> > directly relying on the pref value.
> 
> Thanks. Could you point me to the WebUI code that fetches the pref
> value?  The only thing I can find is in browser_options_handler.cc
> where it sets a key "instant_enabled" to different values depending on
> chrome::search::IsInstantExtendedAPIEnabled but I don't see where it
> actually tells the WebUI page whether or not instant is enabled.

It's done by webui automatically because of the "pref:instant_enabled" bit
that's set on the checkbox. See this part in
chrome/browser/resources/options/browser_options.html:
        <div class="checkbox" guest-visibility="disabled">
          <span class="controlled-setting-with-label">
            <input id="instant-enabled-control" type="checkbox"
                metric="Options_InstantEnabled"
                i18n-values="pref:instant_enabled" dialog-pref>

[Jói, 2013-02-22 21:58:54]

Thanks Sreeram.  I haven't used WebUI, so any hints on how to call
methods instead of this automatic prefs magic would be appreciated.
Either way I'll figure this out on Monday and send out an update.

Cheers,
Jói


On Fri, Feb 22, 2013 at 6:09 PM,  <sreeram@chromium.org> wrote:
> On 2013/02/22 17:44:29, Jói wrote:
>>
>> > Sounds good. We'll also need to change the webui code to then ask for
>> > IsInstantEnabled() and use that as the setting for the checkbox, instead
>> > of
>> > directly relying on the pref value.
>
>
>> Thanks. Could you point me to the WebUI code that fetches the pref
>> value?  The only thing I can find is in browser_options_handler.cc
>> where it sets a key "instant_enabled" to different values depending on
>> chrome::search::IsInstantExtendedAPIEnabled but I don't see where it
>> actually tells the WebUI page whether or not instant is enabled.
>
>
> It's done by webui automatically because of the "pref:instant_enabled" bit
> that's set on the checkbox. See this part in
> chrome/browser/resources/options/browser_options.html:
>         <div class="checkbox" guest-visibility="disabled">
>           <span class="controlled-setting-with-label">
>             <input id="instant-enabled-control" type="checkbox"
>                 metric="Options_InstantEnabled"
>                 i18n-values="pref:instant_enabled" dialog-pref>
>
>
> https://codereview.chromium.org/12328054/

[Jói, 2013-02-25 15:54:22]

I don't have a full solution yet, but I've uploaded a sketch of what it would
look like.

Mattias, I did as you suggested in browser_instant_controller.cc

That leaves fixing the WebUI behavior for the settings page. This is more
difficult than it seems, because using prefs as the underlying object for items
on the settings page is pretty ingrained in how it's coded.

The current approach (see browser_options_handler.cc) almost works; the problem
is, the starting value gets overwritten from prefs later on during
initialization.

If I switch to an approach where I set the value of the checkbox later in
initialization, this will trigger the notification that causes the new value to
be written to prefs.

I guess I could switch completely to initializing this checkbox in a custom way
and writing its value back to prefs in a custom way, instead of relying on the
pref= decorator magic currently being used, but that seems like a ton of work
just to avoid registering one pref's default based on the current value of
another pref.  Mattias, what do you think?  Is there another way we could
accomplish this?

Cheers,
Jói

[Mattias Nissler (ping if slow), 2013-02-25 18:53:41]

https://codereview.chromium.org/12328054/diff/11008/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/12328054/diff/11008/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.js:171: // it may be different
from the value of the preference.
If the instant_enabled is set to the right pref name, the UI should do the right
thing already if I'm not mistaken. See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

https://codereview.chromium.org/12328054/diff/11008/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/12328054/diff/11008/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/browser_options_handler.cc:430:
Profile::FromWebUI(web_ui())));
I think this is not needed, but you just have to fix the conditional in lines
413 to 414 to pass the name of the correct pref to use to the UI - the rest
works automagically if we're lucky.

[Jói, 2013-02-25 19:53:10]

It's kind of tricky.  When I originally re-read this review thread
this morning, and was thinking about Sreeram's comment that we'd need
a function to retrieve the value for the checkbox, I came to the same
conclusion as you did.

The case that misses is when
search::GetInstantExtendedDefaultSetting() ==
search::INSTANT_USE_EXISTING and the value of kInstantExtendedEnabled
hasn't been explicitly set by the user.  In that case, the desired
behavior is for the checkbox to show the value of kInstantEnabled,
rather than the default value of kInstantExtendedEnabled.

Before the current change, that was accomplished by registering
kInstantExtendedEnabled after PrefService creation, and using the
current value of kInstantEnabled as the default value for
kInstantExtendedEnabled.

Cheers,
Jói



On Mon, Feb 25, 2013 at 6:53 PM,  <mnissler@chromium.org> wrote:
>
>
https://codereview.chromium.org/12328054/diff/11008/chrome/browser/resources/...
> File chrome/browser/resources/options/browser_options.js (right):
>
>
https://codereview.chromium.org/12328054/diff/11008/chrome/browser/resources/...
> chrome/browser/resources/options/browser_options.js:171: // it may be
> different from the value of the preference.
> If the instant_enabled is set to the right pref name, the UI should do
> the right thing already if I'm not mistaken. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
>
>
https://codereview.chromium.org/12328054/diff/11008/chrome/browser/ui/webui/o...
> File chrome/browser/ui/webui/options/browser_options_handler.cc (right):
>
>
https://codereview.chromium.org/12328054/diff/11008/chrome/browser/ui/webui/o...
> chrome/browser/ui/webui/options/browser_options_handler.cc:430:
> Profile::FromWebUI(web_ui())));
> I think this is not needed, but you just have to fix the conditional in
> lines 413 to 414 to pass the name of the correct pref to use to the UI -
> the rest works automagically if we're lucky.
>
> https://codereview.chromium.org/12328054/

[sreeram, 2013-02-25 21:51:02]

On Mon, Feb 25, 2013 at 7:54 AM,  <joi@chromium.org> wrote:
> I guess I could switch completely to initializing this checkbox in a custom
> way
> and writing its value back to prefs in a custom way, instead of relying on
> the
> pref= decorator magic currently being used, but that seems like a ton of
> work
> just to avoid registering one pref's default based on the current value of
> another pref.  Mattias, what do you think?  Is there another way we could
> accomplish this?

I think we may have to do this. If you like, I can do it. I am already
in the midst of a refactor where I'm moving IsInstantEnabled() and
such from BrowserInstantController to c/b/ui/search/search.cc, and
have already factored in your change to not require a PrefService*
(and everything else discussed in this thread). I also have some
experience mucking with the checkbox without using the pref-decorator
magic (I had to do the same thing long ago when we were running
Instant experiments). It will probably be a few more days before I
upload my CL though, so if you need to get this done sooner, you'll
have to do it.

[Jói, 2013-02-26 10:56:37]

Hi Sreeram,

If you are able to take this over as part of your refactoring, and
given that you have more experience with twisting the WebUI code to do
what we want, I think that would be ideal.  There's no big rush to
land this change.

Could you CC me on your change once it's ready, so that I know when it
lands?  There are some further cleanup actions I can take once it does
(in code elsewhere, namely chrome/browser/prefs/browser_prefs.cc).

Cheers,
Jói



On Mon, Feb 25, 2013 at 9:50 PM, Sreeram Ramachandran
<sreeram@chromium.org> wrote:
> On Mon, Feb 25, 2013 at 7:54 AM,  <joi@chromium.org> wrote:
>> I guess I could switch completely to initializing this checkbox in a custom
>> way
>> and writing its value back to prefs in a custom way, instead of relying on
>> the
>> pref= decorator magic currently being used, but that seems like a ton of
>> work
>> just to avoid registering one pref's default based on the current value of
>> another pref.  Mattias, what do you think?  Is there another way we could
>> accomplish this?
>
> I think we may have to do this. If you like, I can do it. I am already
> in the midst of a refactor where I'm moving IsInstantEnabled() and
> such from BrowserInstantController to c/b/ui/search/search.cc, and
> have already factored in your change to not require a PrefService*
> (and everything else discussed in this thread). I also have some
> experience mucking with the checkbox without using the pref-decorator
> magic (I had to do the same thing long ago when we were running
> Instant experiments). It will probably be a few more days before I
> upload my CL though, so if you need to get this done sooner, you'll
> have to do it.

[sreeram, 2013-02-26 14:51:30]

On Tue, Feb 26, 2013 at 2:55 AM, Jói Sigurðsson <joi@chromium.org> wrote:
> If you are able to take this over as part of your refactoring, and
> given that you have more experience with twisting the WebUI code to do
> what we want, I think that would be ideal.  There's no big rush to
> land this change.
>
> Could you CC me on your change once it's ready, so that I know when it
> lands?  There are some further cleanup actions I can take once it does
> (in code elsewhere, namely chrome/browser/prefs/browser_prefs.cc).

Will do!

[Jói, 2013-02-26 17:03:22]

Hi Sreeram,

I discussed this with Mattias and we have an idea that might avoid the re-work
of the settings page (unless you are planning to do it for reasons other than
this patch of mine).

Basically, we would register the kInstantExtendedEnabled preference with some
constant default value in RegisterUserPreferences, but then in the
BrowserInstantController constructor, we would call a new method on PrefService,
SetDefaultValue, which sets the default value rather than setting the explicit
value.  This should give the exact same semantics as before.

I can take care of doing this, assuming it's a satisfactory solution.

Cheers,
Jói



On 2013/02/26 14:51:30, sreeram wrote:
> On Tue, Feb 26, 2013 at 2:55 AM, Jói Sigurðsson <mailto:joi@chromium.org>
wrote:
> > If you are able to take this over as part of your refactoring, and
> > given that you have more experience with twisting the WebUI code to do
> > what we want, I think that would be ideal.  There's no big rush to
> > land this change.
> >
> > Could you CC me on your change once it's ready, so that I know when it
> > lands?  There are some further cleanup actions I can take once it does
> > (in code elsewhere, namely chrome/browser/prefs/browser_prefs.cc).
> 
> Will do!

[sreeram, 2013-02-26 17:27:48]

On 2013/02/26 17:03:22, Jói wrote:
> Hi Sreeram,
> 
> I discussed this with Mattias and we have an idea that might avoid the re-work
> of the settings page (unless you are planning to do it for reasons other than
> this patch of mine).
> 
> Basically, we would register the kInstantExtendedEnabled preference with some
> constant default value in RegisterUserPreferences, but then in the
> BrowserInstantController constructor, we would call a new method on
PrefService,
> SetDefaultValue, which sets the default value rather than setting the explicit
> value.  This should give the exact same semantics as before.
> 
> I can take care of doing this, assuming it's a satisfactory solution.

I think that's great! No, I have no need to modify the webui/preferences code
otherwise, so such a method (SetDefaultValue()) would work nicely. I'll let you
figure out semantics (should a pref-changed notification fire if in fact,
setting the default value changes the value of the pref, etc).

So, yes, please proceed!

[Jói, 2013-02-26 17:35:11]

Great, will do Sreeram.

Cheers,
Jói


On Tue, Feb 26, 2013 at 5:27 PM,  <sreeram@chromium.org> wrote:
> On 2013/02/26 17:03:22, Jói wrote:
>>
>> Hi Sreeram,
>
>
>> I discussed this with Mattias and we have an idea that might avoid the
>> re-work
>> of the settings page (unless you are planning to do it for reasons other
>> than
>> this patch of mine).
>
>
>> Basically, we would register the kInstantExtendedEnabled preference with
>> some
>> constant default value in RegisterUserPreferences, but then in the
>> BrowserInstantController constructor, we would call a new method on
>
> PrefService,
>>
>> SetDefaultValue, which sets the default value rather than setting the
>> explicit
>> value.  This should give the exact same semantics as before.
>
>
>> I can take care of doing this, assuming it's a satisfactory solution.
>
>
> I think that's great! No, I have no need to modify the webui/preferences
> code
> otherwise, so such a method (SetDefaultValue()) would work nicely. I'll let
> you
> figure out semantics (should a pref-changed notification fire if in fact,
> setting the default value changes the value of the pref, etc).
>
> So, yes, please proceed!
>
> https://codereview.chromium.org/12328054/

[Mattias Nissler (ping if slow), 2013-02-26 17:38:59]

On Tue, Feb 26, 2013 at 6:34 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Great, will do Sreeram.
>
> Cheers,
> Jói
>
>
> On Tue, Feb 26, 2013 at 5:27 PM,  <sreeram@chromium.org> wrote:
> > On 2013/02/26 17:03:22, Jói wrote:
> >>
> >> Hi Sreeram,
> >
> >
> >> I discussed this with Mattias and we have an idea that might avoid the
> >> re-work
> >> of the settings page (unless you are planning to do it for reasons other
> >> than
> >> this patch of mine).
> >
> >
> >> Basically, we would register the kInstantExtendedEnabled preference with
> >> some
> >> constant default value in RegisterUserPreferences, but then in the
> >> BrowserInstantController constructor, we would call a new method on
> >
> > PrefService,
> >>
> >> SetDefaultValue, which sets the default value rather than setting the
> >> explicit
> >> value.  This should give the exact same semantics as before.
> >
> >
> >> I can take care of doing this, assuming it's a satisfactory solution.
> >
> >
> > I think that's great! No, I have no need to modify the webui/preferences
> > code
> > otherwise, so such a method (SetDefaultValue()) would work nicely. I'll
> let
> > you
> > figure out semantics (should a pref-changed notification fire if in fact,
> > setting the default value changes the value of the pref, etc).
>

Yes, we should fire pref notifications IMHO, same semantics as changes in
other pref stores.


> >
> > So, yes, please proceed!
> >
> > https://codereview.chromium.org/12328054/
>

[sreeram, 2013-02-27 03:17:32]

FYI: Joi is doing the SetDefaultValue stuff in
https://codereview.chromium.org/12315116/. Perhaps this CL can be considered
abandoned/closed?

[Jói, 2013-02-27 09:43:44]

Yes, closing.

On Wed, Feb 27, 2013 at 3:17 AM,  <sreeram@chromium.org> wrote:
> FYI: Joi is doing the SetDefaultValue stuff in
> https://codereview.chromium.org/12315116/. Perhaps this CL can be considered
> abandoned/closed?
>
> https://codereview.chromium.org/12328054/