Refactor Instant web UI (chrome://settings page).

Refactor Instant web UI (chrome://settings page).

Currently, the web UI directly sets two Instant prefs (instant.enabled and
instant.confirm_dialog_shown), but doesn't call InstantController's Enable()
or Disable(). Those methods set some additional prefs, so this refactoring
calls those instead. This will also be helpful in a later CL where we enable
Instant by default in a field trial.

There are two behavioural changes:
+ The kInstantEnabledOnce pref is set, even for a disable (@sky approves).
+ The first time the user enables Instant, the old behaviour used to be that
  the checkbox would get cleared, then the confirm dialog would be shown. I
  found that this was slightly janky, so we now leave the checkbox enabled
  when showing the confirm dialog. After the user clicks OK or Cancel, the
  checkbox acquires the expected state (as before).

BUG=none
TEST=Enabled/disabled checkbox, and saw no regression in behaviour.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=94005

[sreeram, 2011-07-18 17:43:05]

Please review.

[sky, 2011-07-18 21:10:56]

You should get estade to review the webui changes.

The instant change LGTM

[sky, 2011-07-18 21:11:19]

This time with Evan as a reviewer.

  -Scott

[Evan Stade, 2011-07-19 00:15:32]

would it not work to just leave it as is, but override the click handling?

[sreeram, 2011-07-19 00:47:05]

On Mon, Jul 18, 2011 at 17:15,  <estade@chromium.org> wrote:
> would it not work to just leave it as is, but override the click handling?

There's at least one disadvantage to leaving things as they are: When
the user enables Instant for the first time, since the checkbox is
tied to the pref, the prefs system automatically sends an "instant
enabled" notification (received by browser.cc). But the user could
cancel the confirm dialog, returning to the original state (and
causing an "instant disabled" notification to be sent).

The reason why this matters is: For the upcoming Instant field trial,
we want to avoid triggering the field trial for users who have
explicitly enabled or disabled the preference. The above scenario
shouldn't count as having enabled/disabled the pref, but we have no
way to tell it apart from a regular enable/disable. This can be
avoided if we untie the pref from the checkbox and let
InstantController set the pref, as in the current CL.

[Evan Stade, 2011-07-19 00:59:48]

I think you could fix that by preventing propagation of the 'change' event if
the confirmation dialog hasn't been shown

[sreeram, 2011-07-19 03:59:37]

On Mon, Jul 18, 2011 at 17:59,  <estade@chromium.org> wrote:
> I think you could fix that by preventing propagation of the 'change' event
> if the confirmation dialog hasn't been shown

I am not sure how to do that. Could you explain? From what I
understand, if the checkbox is defined thusly:
    <input ... pref="instant.enabled">
the prefs machinery kicks in automatically to keep the checkbox state
in sync with the pref, including sending notifications when it
changes, etc.

It seems like it would be sorta hacky to prevent that from working as
intended (preventing change notifications, conditioned on another
pref). It also seems cleaner to make sure that the different code
paths eventually call InstantController::Enable/Disable. For example,
I don't know if the native dialogs are ever going to be used (or if
it's all deprecated, and only web UI is relevant); those native
dialogs call InstantController and don't manage prefs by themselves.

In future, if we add more logic to InstantController that can't be
achieved simply by setting a pref, we'll need to add these hooks
anyway.

[Evan Stade, 2011-07-19 23:42:08]

On 2011/07/19 03:59:37, sreeram wrote:
> On Mon, Jul 18, 2011 at 17:59,  <mailto:estade@chromium.org> wrote:
> > I think you could fix that by preventing propagation of the 'change' event
> > if the confirmation dialog hasn't been shown
> 
> I am not sure how to do that. Could you explain? From what I
> understand, if the checkbox is defined thusly:
>     <input ... pref="instant.enabled">
> the prefs machinery kicks in automatically to keep the checkbox state
> in sync with the pref, including sending notifications when it
> changes, etc.

yes, and you want that the checkbox state should keep in sync with the pref. The
only thing you don't want is for the checkbox to toggle when you activate it the
first time (instead show a confirmation dialog). So instead of allowing the
checkbox to toggle and then toggling it back, don't allow it to toggle in the
first place until the confirmation comes in.

> 
> It seems like it would be sorta hacky to prevent that from working as
> intended (preventing change notifications, conditioned on another
> pref).

you don't think the code duplication you have in onInstantEnabledChanged_ is
hacky and prone to breakage?

> It also seems cleaner to make sure that the different code
> paths eventually call InstantController::Enable/Disable. For example,
> I don't know if the native dialogs are ever going to be used (or if
> it's all deprecated, and only web UI is relevant); those native
> dialogs call InstantController and don't manage prefs by themselves.
> 
> In future, if we add more logic to InstantController that can't be
> achieved simply by setting a pref, we'll need to add these hooks
> anyway.

[sreeram, 2011-07-20 16:35:18]

On Tue, Jul 19, 2011 at 16:42,  <estade@chromium.org> wrote:
> So instead of allowing the
> checkbox to toggle and then toggling it back, don't allow it to toggle in
> the first place until the confirmation comes in.

That's what I'm suggesting might be kludgy. In the old code, even
though we have an onclick handler for the checkbox, by the time it's
called, the prefs system has already toggled the checkbox, set the
pref, and sent out the notification. Which is why the old code had to
do a "Preferences.setBooleanPref(..., false, ...)" to undo it before
popping up the confirm dialog.

> you don't think the code duplication you have in onInstantEnabledChanged_ is
> hacky and prone to breakage?

I'm not sure about hacky and prone to breakage, but there definitely
was some code there that wasn't needed. I've excised all of it in the
new patch. Please take a look.

To summarize my arguments (mentioned so far in the CL description and
this thread):

+ It's important that Enable/Disable be called. They do things that
can't be achieved by the webui merely setting the instant.enabled
pref. For example, they set instant.enabled_once, and record some UMA
histograms.

+ All other code paths (such as those using native dialogs) call
Enable/Disable. It seems strange to have a different path for webui
for the same functionality.

+ The current mechanism of undoing the pref change before popping up
the confirm dialog causes spurious notifications, which is a big
problem for me in the upcoming field trial. Trying to hack around it
seems less elegant than doing this CL.

[sreeram, 2011-07-21 16:35:29]

estade: ping?

I'd like to get this in soon, so that it makes the M14 cut. Additionally,
http://codereview.chromium.org/7337007/ depends on this, so please take a look
at that too.

Thanks!

[Evan Stade, 2011-07-21 22:38:57]

On 2011/07/20 16:35:18, sreeram wrote:
> On Tue, Jul 19, 2011 at 16:42,  <mailto:estade@chromium.org> wrote:
> > So instead of allowing the
> > checkbox to toggle and then toggling it back, don't allow it to toggle in
> > the first place until the confirmation comes in.
> 
> That's what I'm suggesting might be kludgy. In the old code, even
> though we have an onclick handler for the checkbox, by the time it's
> called, the prefs system has already toggled the checkbox, set the
> pref, and sent out the notification. Which is why the old code had to
> do a "Preferences.setBooleanPref(..., false, ...)" to undo it before
> popping up the confirm dialog.

yes, the old code is kludgy, but I'm suggesting you fix it if the kludge no
longer works.

> 
> > you don't think the code duplication you have in onInstantEnabledChanged_ is
> > hacky and prone to breakage?
> 
> I'm not sure about hacky and prone to breakage, but there definitely
> was some code there that wasn't needed. I've excised all of it in the
> new patch. Please take a look.

does this patch still show the managed pref banner if instant is managed?

> 
> To summarize my arguments (mentioned so far in the CL description and
> this thread):
> 
> + It's important that Enable/Disable be called. They do things that
> can't be achieved by the webui merely setting the instant.enabled
> pref. For example, they set instant.enabled_once, and record some UMA
> histograms.
> 
> + All other code paths (such as those using native dialogs) call
> Enable/Disable. It seems strange to have a different path for webui
> for the same functionality.
> 
> + The current mechanism of undoing the pref change before popping up
> the confirm dialog causes spurious notifications, which is a big
> problem for me in the upcoming field trial. Trying to hack around it
> seems less elegant than doing this CL.

[sky, 2011-07-21 22:49:48]

On Thu, Jul 21, 2011 at 9:35 AM,  <sreeram@chromium.org> wrote:
> estade: ping?
>
> I'd like to get this in soon, so that it makes the M14 cut.

It's my understanding this experiment is for the dev branch. As such,
I think you should hold off on landing until after the m14 branch.
Otherwise you'll need to merge something over to the m14 branch after
you're done with the experiment.

  -Scott

Additionally,
> http://codereview.chromium.org/7337007/ depends on this, so please take a
> look
> at that too.
>
> Thanks!
>
> http://codereview.chromium.org/7396025/
>

[sreeram, 2011-07-22 13:32:09]

On Thu, Jul 21, 2011 at 15:49, Scott Violet <sky@chromium.org> wrote:
> It's my understanding this experiment is for the dev branch. As such,
> I think you should hold off on landing until after the m14 branch.
> Otherwise you'll need to merge something over to the m14 branch after
> you're done with the experiment.

Arvind wants us to experiment on the beta/stable branches as well. So,
indeed we'd like to get in for M14. To be clear, even on the stable
channel, we are only talking about a small percentage experiment. Once
the branch has been cut, I'll reduce the percentages there, after
consulting with Brian and Jeff.

[sreeram, 2011-07-22 13:45:16]

On Thu, Jul 21, 2011 at 15:38,  <estade@chromium.org> wrote:
> On 2011/07/20 16:35:18, sreeram wrote:
>> On Tue, Jul 19, 2011 at 16:42,  <mailto:estade@chromium.org> wrote:
>> > So instead of allowing the
>> > checkbox to toggle and then toggling it back, don't allow it to toggle
>> > in
>> > the first place until the confirmation comes in.
>
>> That's what I'm suggesting might be kludgy. In the old code, even
>> though we have an onclick handler for the checkbox, by the time it's
>> called, the prefs system has already toggled the checkbox, set the
>> pref, and sent out the notification. Which is why the old code had to
>> do a "Preferences.setBooleanPref(..., false, ...)" to undo it before
>> popping up the confirm dialog.
>
> yes, the old code is kludgy, but I'm suggesting you fix it if the kludge no
> longer works.

I think we are just going around in circles. I don't see how to make
it less kludgy without inherently untying the pref from the checkbox.
If you have some example code that achieves this, please let me know.
Please also consider that it's not just a matter of fixing how the
pref is set (cf: the other reasons mentioned before).

> does this patch still show the managed pref banner if instant is managed?

Yes, due to the already existing call to
AddUserPref(prefs::kInstantEnabled) in managed_prefs_banner_base.cc.

[Evan Stade, 2011-07-22 19:28:30]

yes, it does seem we are having trouble communicating. Your patch does break
pref management. I've downloaded your patch and applied it. See:

http://www/~estade/no_crawl/not_broken.png
http://www/~estade/no_crawl/broken.png

managed_pref_banner_base.cc doesn't exist any more, so if you were relying on
that the patch may be bitrotten.

[sreeram, 2011-07-22 19:42:09]

On Fri, Jul 22, 2011 at 12:28,  <estade@chromium.org> wrote:
> yes, it does seem we are having trouble communicating. Your patch does break
> pref management. I've downloaded your patch and applied it. See:
>
> http://www/~estade/no_crawl/not_broken.png
> http://www/~estade/no_crawl/broken.png

Strange. So, even though the homepage pref is managed, there's no
banner. Are you saying my patch somehow affected the homepage pref
too? Could you share instructions on how I can fake the group policy
on Linux or MacOS (I don't have a Windows machine to test with).

> managed_pref_banner_base.cc doesn't exist any more, so if you were relying
> on that the patch may be bitrotten.

Thanks for the pointer. I'll look into it.

[Evan Stade, 2011-07-22 20:35:14]

On 2011/07/22 19:42:09, sreeram wrote:
> On Fri, Jul 22, 2011 at 12:28,  <mailto:estade@chromium.org> wrote:
> > yes, it does seem we are having trouble communicating. Your patch does break
> > pref management. I've downloaded your patch and applied it. See:
> >
> > http://www/%7Eestade/no_crawl/not_broken.png
> > http://www/%7Eestade/no_crawl/broken.png
> 
> Strange. So, even though the homepage pref is managed, there's no
> banner. Are you saying my patch somehow affected the homepage pref
> too?

yes, that part was unexpected to me from reading the code

> Could you share instructions on how I can fake the group policy
> on Linux or MacOS (I don't have a Windows machine to test with).

pref management instructions are here:
http://dev.chromium.org/administrators/linux-quick-start

basically, create /etc/chromium/policies/managed/test.json . The policy I used
was

{
  "HomepageIsNewTabPage": false,
  "InstantEnabled": true
}


> 
> > managed_pref_banner_base.cc doesn't exist any more, so if you were relying
> > on that the patch may be bitrotten.
> 
> Thanks for the pointer. I'll look into it.

[sreeram, 2011-07-25 12:16:50]

@estade: I've updated the patch to keep the pref tied to the checkbox, but to
call a custom change handler, as we discussed. Please take a look. Thanks!

[Evan Stade, 2011-07-25 23:01:33]

just nits

http://codereview.chromium.org/7396025/diff/19002/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/7396025/diff/19002/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:76: if
(self.instantConfirmDialogShown_) {
nit: no curlies

http://codereview.chromium.org/7396025/diff/19002/chrome/browser/resources/op...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/7396025/diff/19002/chrome/browser/resources/op...
chrome/browser/resources/options/pref_ui.js:101: if (self.customChangeHandler &&
self.customChangeHandler(e))
if you make this a function of PrefCheckbox that has a default stub
implementation that just returns false, then you won't have to check if it
exists and (more importantly) you will have a place to add documentation.

http://codereview.chromium.org/7396025/diff/19002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/7396025/diff/19002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:479: void
BrowserOptionsHandler::EnableInstant(const ListValue* args) {
I feel these two functions should be a single function that has a true/false
argument

[sreeram, 2011-07-25 23:56:42]

http://codereview.chromium.org/7396025/diff/19002/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/7396025/diff/19002/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:76: if
(self.instantConfirmDialogShown_) {
On 2011/07/25 23:01:34, Evan Stade wrote:
> nit: no curlies

Done.

http://codereview.chromium.org/7396025/diff/19002/chrome/browser/resources/op...
File chrome/browser/resources/options/pref_ui.js (right):

http://codereview.chromium.org/7396025/diff/19002/chrome/browser/resources/op...
chrome/browser/resources/options/pref_ui.js:101: if (self.customChangeHandler &&
self.customChangeHandler(e))
On 2011/07/25 23:01:34, Evan Stade wrote:
> if you make this a function of PrefCheckbox that has a default stub
> implementation that just returns false, then you won't have to check if it
> exists and (more importantly) you will have a place to add documentation.

Done.

http://codereview.chromium.org/7396025/diff/19002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/7396025/diff/19002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:479: void
BrowserOptionsHandler::EnableInstant(const ListValue* args) {
On 2011/07/25 23:01:34, Evan Stade wrote:
> I feel these two functions should be a single function that has a true/false
> argument

I considered that, but I think we should keep them separate. They nicely mirror
the Enable/Disable of the controller, are more natural to grep for and have
better names than anything I can come up with for a unified method (SetInstant?
SetInstantState? EnableDisableInstant?).

[Evan Stade, 2011-07-26 00:04:28]

SetInstantEnabled was the name I was thinking of. but anyway, LGTM

[commit-bot: I haz the power, 2011-07-26 01:07:02]

Change committed as 94005