MD Settings: Stop calling chrome.settingsPrivate.setDefaultZoomLevel on startup.

MD Settings: Stop calling chrome.settingsPrivate.setDefaultZoomLevel on startup.

BUG=652483, 654588
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/7577ff2509a7a5d91ec319f12a42dd1acdb1a41f
Cr-Commit-Position: refs/heads/master@{#425519}

[dpapad, 2016-10-11 00:12:32]

Description was changed from

==========
MD Settings: Stop calling chrome.settingsPrivate.setDefaultZoomLevel on startup.

BUG=652483,654588
==========

to

==========
MD Settings: Stop calling chrome.settingsPrivate.setDefaultZoomLevel on startup.

BUG=652483,654588
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-10-11 00:14:09]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 00:14:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2016-10-11 00:42:44]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org, michaelpg@chromium.org

[dpapad, 2016-10-11 00:42:45]

Admittedly this solution is a bit hacky, but I could not find a better way to
not invoke the defaultZoomLevel_.value observer when first setting
defaultZoomLevel_.value. Suggestions welcome.

[chromium-reviews, 2016-10-11 00:53:56]

yay. We do the same thing in prefs.js, probably not helpful since this is a
fake "pref"

On Mon, Oct 10, 2016, 8:42 PM dpapad@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Reviewers: Dan Beam, michaelpg (OOO M-W)
> CL: https://codereview.chromium.org/2403353002/
>
> Message:
> Admittedly this solution is a bit hacky, but I could not find a better way
> to
> not invoke the defaultZoomLevel_.value observer when first setting
> defaultZoomLevel_.value. Suggestions welcome.
>
> Description:
> MD Settings: Stop calling chrome.settingsPrivate.setDefaultZoomLevel on
> startup.
>
> BUG=652483,654588
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
>
> Affected files (+11, -0 lines):
> M chrome/browser/resources/settings/appearance_page/appearance_page.js
>
>
> Index: chrome/browser/resources/settings/appearance_page/appearance_page.js
> diff --git
> a/chrome/browser/resources/settings/appearance_page/appearance_page.js
> b/chrome/browser/resources/settings/appearance_page/appearance_page.js
> index
>
5b7462bd9315bb65beb987e947d9e0919873dba2..5789801ebc1581c7caf8b11b8f39d0558ae662c0
> 100644
> --- a/chrome/browser/resources/settings/appearance_page/appearance_page.js
> +++ b/chrome/browser/resources/settings/appearance_page/appearance_page.js
> @@ -49,6 +49,9 @@ Polymer({
> value: function() {
> return {
> type: chrome.settingsPrivate.PrefType.NUMBER,
> + /* Needed such that the pref can be initialized without unnecessarily
> + * invoking chrome.settingsPrivate.setDefaultZoomPercent(). */
> + initialized: false,
> };
> },
> },
> @@ -213,6 +216,14 @@ Polymer({
> // The |percent| may be undefined on startup.
> if (percent === undefined)
> return;
> +
> + if (!this.defaultZoomLevel_.initialized) {
> + // Simply mark this pref as initialized, no actual change was requested
> by
> + // the user.
> + this.defaultZoomLevel_.initialized = true;
> + return;
> + }
> +
> chrome.settingsPrivate.setDefaultZoomPercent(percent);
> },
>
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dpapad, 2016-10-11 00:58:55]

On 2016/10/11 at 00:53:56, chromium-reviews wrote:
> yay. We do the same thing in prefs.js, probably not helpful since this is a
> fake "pref"
> 

Perhaps the code would be simpler if we were using a normal <select> instead of
<settings-dropdown-menu>, which would eliminate the need to mask the "zoom
level" as a fake pref?

[commit-bot: I haz the power, 2016-10-11 01:28:54]

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

[commit-bot: I haz the power, 2016-10-11 01:28:55]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-10-11 03:47:33]

On 2016/10/11 00:58:55, dpapad wrote:
> On 2016/10/11 at 00:53:56, chromium-reviews wrote:
> > yay. We do the same thing in prefs.js, probably not helpful since this is a
> > fake "pref"
> > 
> 
> Perhaps the code would be simpler if we were using a normal <select> instead
of
> <settings-dropdown-menu>, which would eliminate the need to mask the "zoom
> level" as a fake pref?

maybe? (The "yay" wasn't sarcastic btw.) settings-dropdown-menu does provide the
helpful behavior of failing gracefully for a "custom" value like 101%.

https://codereview.chromium.org/2403353002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:54:
initialized: false,
We probably shouldn't "overload" this value more than we already do -- it'd be a
partial PrefObject with an extra |initialized| member. How about a separate
defaultZoomLevelInitialized_ property?

https://codereview.chromium.org/2403353002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:220: if
(!this.defaultZoomLevel_.initialized) {
Since we only use this here, I wouldn't be averse to just defining it here (and
nowhere else) rather than making it part of a "type".

[dpapad, 2016-10-11 17:30:17]

>settings-dropdown-menu does provide the
helpful behavior of failing gracefully for a "custom" value like 101%.

I am not sure if this behavior is actually helpful. Is there a way to set a
custom zoom value that is not one of the displayed values? I don't see it in the
old UI either, and in this case the error that is happening with the rounding
error manifests as a "Custom" value.

https://codereview.chromium.org/2403353002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:54:
initialized: false,
On 2016/10/11 at 03:47:33, michaelpg (OOO M-W) wrote:
> We probably shouldn't "overload" this value more than we already do -- it'd be
a partial PrefObject with an extra |initialized| member. How about a separate
defaultZoomLevelInitialized_ property?

Done.

https://codereview.chromium.org/2403353002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:220: if
(!this.defaultZoomLevel_.initialized) {
On 2016/10/11 at 03:47:33, michaelpg (OOO M-W) wrote:
> Since we only use this here, I wouldn't be averse to just defining it here
(and nowhere else) rather than making it part of a "type".

What do you mean defining it only here? Here in this scope? Needs to be
persisted across different calls of  zoomLevelChanged.

[Dan Beam, 2016-10-11 19:03:23]

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:45: value:
{type: chrome.settingsPrivate.PrefType.NUMBER},
can we just omit the default value and use:

defaultZoomLevel_: {
  ...
  observer: 'zoomLevelChanged_',
},

and

zoomLevelChanged_: function(newZoomLevel, oldZoomLevel) {
  if (!oldZoomLevel)  { // or === undefined or something
    // Ignore the first time this is called.
    return;
  }
}

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:54:
defaultZoomLevelInitialized_: {
fyi, some polymer peeps asked me to use vanilla prototype properties instead of
properties: {} recently, so I suppose we should start doing that for things we
don't observe

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:128: //
default zoom percent.
i suppose this is more work than we want to do right now?  what happens if 2
settings pages are open?

[dpapad, 2016-10-11 19:29:53]

Patchset #4 (id:60001) has been deleted

[dpapad, 2016-10-11 19:32:06]

Patchset #4 (id:80001) has been deleted

[dpapad, 2016-10-11 19:33:06]

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:45: value:
{type: chrome.settingsPrivate.PrefType.NUMBER},
On 2016/10/11 at 19:03:23, Dan Beam wrote:
> can we just omit the default value and use:
> 
> defaultZoomLevel_: {
>   ...
>   observer: 'zoomLevelChanged_',
> },
> 
> and
> 
> zoomLevelChanged_: function(newZoomLevel, oldZoomLevel) {
>   if (!oldZoomLevel)  { // or === undefined or something
>     // Ignore the first time this is called.
>     return;
>   }
> }

Unfortunately that only works when you are setting the entire object, but not
when setting defaultZoomLevel.value directly (which is what the
settings-dropdown-menu does).

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:54:
defaultZoomLevelInitialized_: {
On 2016/10/11 at 19:03:23, Dan Beam wrote:
> fyi, some polymer peeps asked me to use vanilla prototype properties instead
of properties: {} recently, so I suppose we should start doing that for things
we don't observe

Done. I was thinking about this myself.

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:128: //
default zoom percent.
On 2016/10/11 at 19:03:23, Dan Beam wrote:
> i suppose this is more work than we want to do right now? 
Definitely not in this CL, but I think we should address.

> what happens if 2 settings pages are open?
Only the tab that initiates the change is updated, the other one displays an
obsolete value until refreshed.

Note that Options somehow manages to updated the zoom value on both tabs, so MD
Settings is currently not on par with it. I think this deserves a dedicated
crbug.

[Dan Beam, 2016-10-11 19:34:48]

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:45: value:
{type: chrome.settingsPrivate.PrefType.NUMBER},
On 2016/10/11 19:33:06, dpapad wrote:
> On 2016/10/11 at 19:03:23, Dan Beam wrote:
> > can we just omit the default value and use:
> > 
> > defaultZoomLevel_: {
> >   ...
> >   observer: 'zoomLevelChanged_',
> > },
> > 
> > and
> > 
> > zoomLevelChanged_: function(newZoomLevel, oldZoomLevel) {
> >   if (!oldZoomLevel)  { // or === undefined or something
> >     // Ignore the first time this is called.
> >     return;
> >   }
> > }
> 
> Unfortunately that only works when you are setting the entire object, but not
> when setting defaultZoomLevel.value directly (which is what the
> settings-dropdown-menu does).

and the problem with that is what?

[Dan Beam, 2016-10-11 19:35:51]

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:45: value:
{type: chrome.settingsPrivate.PrefType.NUMBER},
On 2016/10/11 19:34:48, Dan Beam wrote:
> On 2016/10/11 19:33:06, dpapad wrote:
> > On 2016/10/11 at 19:03:23, Dan Beam wrote:
> > > can we just omit the default value and use:
> > > 
> > > defaultZoomLevel_: {
> > >   ...
> > >   observer: 'zoomLevelChanged_',
> > > },
> > > 
> > > and
> > > 
> > > zoomLevelChanged_: function(newZoomLevel, oldZoomLevel) {
> > >   if (!oldZoomLevel)  { // or === undefined or something
> > >     // Ignore the first time this is called.
> > >     return;
> > >   }
> > > }
> > 
> > Unfortunately that only works when you are setting the entire object, but
not
> > when setting defaultZoomLevel.value directly (which is what the
> > settings-dropdown-menu does).
> 
> and the problem with that is what?

i.e. why not just change this code:

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:208:
this.set('defaultZoomLevel_.value', percent);
this.defaultZoomLevel_ = {
  type: chrome.settingsPrivate.PrefType.NUMBER,
  value: percent
};

[dpapad, 2016-10-11 19:43:25]

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:45: value:
{type: chrome.settingsPrivate.PrefType.NUMBER},
On 2016/10/11 at 19:35:50, Dan Beam wrote:
> On 2016/10/11 19:34:48, Dan Beam wrote:
> > On 2016/10/11 19:33:06, dpapad wrote:
> > > On 2016/10/11 at 19:03:23, Dan Beam wrote:
> > > > can we just omit the default value and use:
> > > > 
> > > > defaultZoomLevel_: {
> > > >   ...
> > > >   observer: 'zoomLevelChanged_',
> > > > },
> > > > 
> > > > and
> > > > 
> > > > zoomLevelChanged_: function(newZoomLevel, oldZoomLevel) {
> > > >   if (!oldZoomLevel)  { // or === undefined or something
> > > >     // Ignore the first time this is called.
> > > >     return;
> > > >   }
> > > > }
> > > 
> > > Unfortunately that only works when you are setting the entire object, but
not
> > > when setting defaultZoomLevel.value directly (which is what the
> > > settings-dropdown-menu does).
> > 
> > and the problem with that is what?
> 
> i.e. why not just change this code:

The problem is that settings-dropdown-menu does not do that, so when you change
the dropdown option with the mouse, nothing happens (no observer fires), see 

https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro....

Perhaps I am misunderstanding the suggestion? FWIW, I tried it before
responding. Yes, it fixed the issue of not calling chrome.settingsPrivate on
startup, at the expense of never calling it (even when user changes the value).

[michaelpg, 2016-10-11 22:01:10]

https://codereview.chromium.org/2403353002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:220: if
(!this.defaultZoomLevel_.initialized) {
On 2016/10/11 17:30:17, dpapad wrote:
> On 2016/10/11 at 03:47:33, michaelpg (OOO M-W) wrote:
> > Since we only use this here, I wouldn't be averse to just defining it here
> (and nowhere else) rather than making it part of a "type".
> 
> What do you mean defining it only here? Here in this scope? Needs to be
> persisted across different calls of  zoomLevelChanged.

Oh, I was just saying, this is JS, we don't have to declare all our properties.
So using this.defaultZLI_ *only* in this function is OK:

if (!this.defaultZoomLevelInitialized_) {
  // Simply mark this pref as initialized, etc....
  /** @private */this.defaultZoomLevelInitialized_ = true;
  return;
}

as a plus, you don't need the 6-line comment in the latest patch.

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:208:
this.set('defaultZoomLevel_.value', percent);
On 2016/10/11 19:35:50, Dan Beam wrote:
> this.defaultZoomLevel_ = {
>   type: chrome.settingsPrivate.PrefType.NUMBER,
>   value: percent
> };

but then you're relying on a mildly obscure fact about Polymer observers, and
someone could easily give defaultZoomLevel_ a default value without testing for
what this CL is fixing

[michaelpg, 2016-10-11 22:02:31]

but also lgtm as is

[dpapad, 2016-10-11 22:05:54]

https://codereview.chromium.org/2403353002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:220: if
(!this.defaultZoomLevel_.initialized) {
On 2016/10/11 at 22:01:10, michaelpg (OOO M-W) wrote:
> On 2016/10/11 17:30:17, dpapad wrote:
> > On 2016/10/11 at 03:47:33, michaelpg (OOO M-W) wrote:
> > > Since we only use this here, I wouldn't be averse to just defining it here
> > (and nowhere else) rather than making it part of a "type".
> > 
> > What do you mean defining it only here? Here in this scope? Needs to be
> > persisted across different calls of  zoomLevelChanged.
> 
> Oh, I was just saying, this is JS, we don't have to declare all our
properties. So using this.defaultZLI_ *only* in this function is OK:
> 
> if (!this.defaultZoomLevelInitialized_) {
>   // Simply mark this pref as initialized, etc....
>   /** @private */this.defaultZoomLevelInitialized_ = true;
>   return;
> }
> 
> as a plus, you don't need the 6-line comment in the latest patch.

Ah, got it now. Personally I don't prefer augmenting properties to an object
outside the constructor. JS Compiler enforces this if @struct is used (which I
know we are not using), but nonetheless just feels wrong. I am fine with
declaring it with @private {boolean} before using it here.

[dpapad, 2016-10-12 19:27:38]

On 2016/10/11 at 19:43:25, dpapad wrote:
>
https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):
> 
>
https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/appearance_page/appearance_page.js:45:
value: {type: chrome.settingsPrivate.PrefType.NUMBER},
> On 2016/10/11 at 19:35:50, Dan Beam wrote:
> > On 2016/10/11 19:34:48, Dan Beam wrote:
> > > On 2016/10/11 19:33:06, dpapad wrote:
> > > > On 2016/10/11 at 19:03:23, Dan Beam wrote:
> > > > > can we just omit the default value and use:
> > > > > 
> > > > > defaultZoomLevel_: {
> > > > >   ...
> > > > >   observer: 'zoomLevelChanged_',
> > > > > },
> > > > > 
> > > > > and
> > > > > 
> > > > > zoomLevelChanged_: function(newZoomLevel, oldZoomLevel) {
> > > > >   if (!oldZoomLevel)  { // or === undefined or something
> > > > >     // Ignore the first time this is called.
> > > > >     return;
> > > > >   }
> > > > > }
> > > > 
> > > > Unfortunately that only works when you are setting the entire object,
but not
> > > > when setting defaultZoomLevel.value directly (which is what the
> > > > settings-dropdown-menu does).
> > > 
> > > and the problem with that is what?
> > 
> > i.e. why not just change this code:
> 
> The problem is that settings-dropdown-menu does not do that, so when you
change the dropdown option with the mouse, nothing happens (no observer fires),
see 
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro....
> 
> Perhaps I am misunderstanding the suggestion? FWIW, I tried it before
responding. Yes, it fixed the issue of not calling chrome.settingsPrivate on
startup, at the expense of never calling it (even when user changes the value).

^Friendly ping.

[Dan Beam, 2016-10-12 23:17:43]

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:208:
this.set('defaultZoomLevel_.value', percent);
On 2016/10/11 22:01:10, michaelpg (OOO M-W) wrote:
> On 2016/10/11 19:35:50, Dan Beam wrote:
> > this.defaultZoomLevel_ = {
> >   type: chrome.settingsPrivate.PrefType.NUMBER,
> >   value: percent
> > };
> 
> but then you're relying on a mildly obscure fact about Polymer observers,

I think this is defined and intentional, not mildly obscure.  it's very easy to
add a comment to this effect if you're worried people don't know about it.

> and
> someone could easily give defaultZoomLevel_ a default value without testing
for
> what this CL is fixing

i don't care about people writing code without testing it.  they're screwed no
matter what.

[dpapad, 2016-10-12 23:31:48]

On 2016/10/12 at 23:17:43, dbeam wrote:
>
https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):
> 
>
https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/appearance_page/appearance_page.js:208:
this.set('defaultZoomLevel_.value', percent);
> On 2016/10/11 22:01:10, michaelpg (OOO M-W) wrote:
> > On 2016/10/11 19:35:50, Dan Beam wrote:
> > > this.defaultZoomLevel_ = {
> > >   type: chrome.settingsPrivate.PrefType.NUMBER,
> > >   value: percent
> > > };
> > 
> > but then you're relying on a mildly obscure fact about Polymer observers,
> 
> I think this is defined and intentional, not mildly obscure.  it's very easy
to add a comment to this effect if you're worried people don't know about it.
> 
> > and
> > someone could easily give defaultZoomLevel_ a default value without testing
for
> > what this CL is fixing
> 
> i don't care about people writing code without testing it.  they're screwed no
matter what.

@dbeam: You responded to Michael's comment but not mine. I tried your
suggestion, it did not work, so not sure how to proceed (see comment #18 for
details).

[Dan Beam, 2016-10-12 23:56:20]

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:45: value:
{type: chrome.settingsPrivate.PrefType.NUMBER},
On 2016/10/11 19:43:24, dpapad wrote:
> On 2016/10/11 at 19:35:50, Dan Beam wrote:
> > On 2016/10/11 19:34:48, Dan Beam wrote:
> > > On 2016/10/11 19:33:06, dpapad wrote:
> > > > On 2016/10/11 at 19:03:23, Dan Beam wrote:
> > > > > can we just omit the default value and use:
> > > > > 
> > > > > defaultZoomLevel_: {
> > > > >   ...
> > > > >   observer: 'zoomLevelChanged_',
> > > > > },
> > > > > 
> > > > > and
> > > > > 
> > > > > zoomLevelChanged_: function(newZoomLevel, oldZoomLevel) {
> > > > >   if (!oldZoomLevel)  { // or === undefined or something
> > > > >     // Ignore the first time this is called.
> > > > >     return;
> > > > >   }
> > > > > }
> > > > 
> > > > Unfortunately that only works when you are setting the entire object,
but
> not
> > > > when setting defaultZoomLevel.value directly (which is what the
> > > > settings-dropdown-menu does).
> > > 
> > > and the problem with that is what?
> > 
> > i.e. why not just change this code:
> 
> The problem is that settings-dropdown-menu does not do that, so when you
change
> the dropdown option with the mouse, nothing happens (no observer fires), see 
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/contro....
> 
> Perhaps I am misunderstanding the suggestion? FWIW, I tried it before
> responding. Yes, it fixed the issue of not calling chrome.settingsPrivate on
> startup, at the expense of never calling it (even when user changes the
value).

does this work?

this.defaultZoomLevel_ = {...};
this.notifyPath('defaultZoomLevel_.value');

[dpapad, 2016-10-13 00:42:55]

> does this work?
> 
> this.defaultZoomLevel_ = {...};
> this.notifyPath('defaultZoomLevel_.value');

No does not work. I think there is a disconnect here. No matter what we do in
appearance_page.js, your proposed observer 'zoomLevelChanged_' will never fire
when 'pref.value' changes, which is what settings_dropdown_menu.js does. We
would have to change settings_dropdown_menu.js to always replace the entire pref
object, instead of simply setting the 'value' path.

[Dan Beam, 2016-10-13 05:10:20]

On 2016/10/13 00:42:55, dpapad wrote:
> > does this work?
> > 
> > this.defaultZoomLevel_ = {...};
> > this.notifyPath('defaultZoomLevel_.value');
> 
> No does not work. I think there is a disconnect here. No matter what we do in
> appearance_page.js, your proposed observer 'zoomLevelChanged_' will never fire
> when 'pref.value' changes, which is what settings_dropdown_menu.js does. We
> would have to change settings_dropdown_menu.js to always replace the entire
pref
> object, instead of simply setting the 'value' path.

i think there's quite a few ways changing the outer scope could be observed from
the inner:

1) two-way binding from outer scope to inner
2) changing the observer syntax in settings_dropdown_menu.js to check for pref
or pref.* potentially

i'm fine to let you land this if you want, I just think it's continuing to allow
this weird code to get ... weirder

[dpapad, 2016-10-13 17:02:38]

On 2016/10/13 at 05:10:20, dbeam wrote:
> On 2016/10/13 00:42:55, dpapad wrote:
> > > does this work?
> > > 
> > > this.defaultZoomLevel_ = {...};
> > > this.notifyPath('defaultZoomLevel_.value');
> > 
> > No does not work. I think there is a disconnect here. No matter what we do
in
> > appearance_page.js, your proposed observer 'zoomLevelChanged_' will never
fire
> > when 'pref.value' changes, which is what settings_dropdown_menu.js does. We
> > would have to change settings_dropdown_menu.js to always replace the entire
pref
> > object, instead of simply setting the 'value' path.
> 
> i think there's quite a few ways changing the outer scope could be observed
from the inner:
> 
> 1) two-way binding from outer scope to inner
Isn't two way binding already used? See
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appear...

> 2) changing the observer syntax in settings_dropdown_menu.js to check for pref
or pref.* potentially
I'd rather not affect settings_dropdown_menu.js to accommodate the only "fake"
pref in our code.

> 
> i'm fine to let you land this if you want, I just think it's continuing to
allow this weird code to get ... weirder
My preference is to change this to a normal <select> not a
<settings-dropdown-menu> since it is not a real pref to begin with (proposed
this earlier in the CL). It would work identically
with the search engine selection in search_page.html and it would not look weird
at all.

[Dan Beam, 2016-10-13 17:43:27]

On 2016/10/13 17:02:38, dpapad wrote:
> On 2016/10/13 at 05:10:20, dbeam wrote:
> > On 2016/10/13 00:42:55, dpapad wrote:
> > > > does this work?
> > > > 
> > > > this.defaultZoomLevel_ = {...};
> > > > this.notifyPath('defaultZoomLevel_.value');
> > > 
> > > No does not work. I think there is a disconnect here. No matter what we do
> in
> > > appearance_page.js, your proposed observer 'zoomLevelChanged_' will never
> fire
> > > when 'pref.value' changes, which is what settings_dropdown_menu.js does.
We
> > > would have to change settings_dropdown_menu.js to always replace the
entire
> pref
> > > object, instead of simply setting the 'value' path.
> > 
> > i think there's quite a few ways changing the outer scope could be observed
> from the inner:
> > 
> > 1) two-way binding from outer scope to inner
> Isn't two way binding already used? See
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appear...
> 
> > 2) changing the observer syntax in settings_dropdown_menu.js to check for
pref
> or pref.* potentially
> I'd rather not affect settings_dropdown_menu.js to accommodate the only "fake"
> pref in our code.
> 
> > 
> > i'm fine to let you land this if you want, I just think it's continuing to
> allow this weird code to get ... weirder
> My preference is to change this to a normal <select> not a
> <settings-dropdown-menu> since it is not a real pref to begin with (proposed
> this earlier in the CL). It would work identically
> with the search engine selection in search_page.html and it would not look
weird
> at all.

sgtm

[dpapad, 2016-10-13 17:53:46]

> > > i'm fine to let you land this if you want, I just think it's continuing to
> > allow this weird code to get ... weirder
> > My preference is to change this to a normal <select> not a
> > <settings-dropdown-menu> since it is not a real pref to begin with (proposed
> > this earlier in the CL). It would work identically
> > with the search engine selection in search_page.html and it would not look
weird
> > at all.
> 
> sgtm

Michael had pointed out that <settings-dropdown-menu> gives us the behavior of
falling back
to a "custom" displayed value if the selected value does not match any of the
given <option>. As
said I don't think this is useful for zoom, but don't want to start implementing
this if Michael
still has objections about it.

@michaelpg: WDYT^

[dpapad, 2016-10-14 21:51:17]

On 2016/10/13 at 17:53:46, dpapad wrote:
> > > > i'm fine to let you land this if you want, I just think it's continuing
to
> > > allow this weird code to get ... weirder
> > > My preference is to change this to a normal <select> not a
> > > <settings-dropdown-menu> since it is not a real pref to begin with
(proposed
> > > this earlier in the CL). It would work identically
> > > with the search engine selection in search_page.html and it would not look
weird
> > > at all.
> > 
> > sgtm
> 
> Michael had pointed out that <settings-dropdown-menu> gives us the behavior of
falling back
> to a "custom" displayed value if the selected value does not match any of the
given <option>. As
> said I don't think this is useful for zoom, but don't want to start
implementing this if Michael
> still has objections about it.
> 
> @michaelpg: WDYT^

PTAL, I stopped using a fake pref, and use a normal select instead of a
settings-dropdown-menu (which is designed to work with prefs). I think it looks
better.

[dpapad, 2016-10-14 21:52:25]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 21:52:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 22:05:53]

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

[commit-bot: I haz the power, 2016-10-14 22:05:54]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Dan Beam, 2016-10-14 22:58:27]

lgtm

https://codereview.chromium.org/2403353002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.js:114:
this.$.zoomLevel.value = value;
this is all bonkers af

[dpapad, 2016-10-14 23:08:48]

https://codereview.chromium.org/2403353002/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/2403353002/diff/160001/chrome/browser/resourc...
chrome/browser/resources/settings/appearance_page/appearance_page.js:114:
this.$.zoomLevel.value = value;
On 2016/10/14 at 22:58:27, Dan Beam wrote:
> this is all bonkers af

Added a note with link to related bug.

[dpapad, 2016-10-14 23:14:05]

Landing this CL since it fixes one of the zoom bugs (runtime error). Further
zoom bugs will be addressed as part of crbug.com/655742.

@michaelpg: The zoom value returned from the browser should always be one of the
enumarated values (will be after above bug is fixed), there should never be a
case where "Custom" should be displayed, and therefore no good reason to keep
using <settings-dropdown-menu> for this.

[dpapad, 2016-10-14 23:14:08]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-10-14 23:14:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2403353002/#ps180001
(title: "More")

[commit-bot: I haz the power, 2016-10-14 23:14:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-15 00:42:25]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-10-15 00:46:18]

Description was changed from

==========
MD Settings: Stop calling chrome.settingsPrivate.setDefaultZoomLevel on startup.

BUG=652483,654588
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Stop calling chrome.settingsPrivate.setDefaultZoomLevel on startup.

BUG=652483,654588
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/7577ff2509a7a5d91ec319f12a42dd1acdb1a41f
Cr-Commit-Position: refs/heads/master@{#425519}
==========

[commit-bot: I haz the power, 2016-10-15 00:46:19]

Patchset 8 (id:??) landed as
https://crrev.com/7577ff2509a7a5d91ec319f12a42dd1acdb1a41f
Cr-Commit-Position: refs/heads/master@{#425519}