MD Settings: chrome.settingsPrivate, fix rounding error.

MD Settings: chrome.settingsPrivate, fix rounding error.

BUG=652483
Committed: https://crrev.com/6a3ab25a3152c4ab095c78347c70fe082f6a1743
Cr-Commit-Position: refs/heads/master@{#425552}

[dpapad, 2016-10-10 22:07:44]

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

[commit-bot: I haz the power, 2016-10-10 22:08:09]

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-10 22:47:54]

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

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

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-10-10 23:11:52]

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

[michaelpg, 2016-10-10 23:59:59]

dang

lgtm

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

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-10-11 00:05:49]

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

[commit-bot: I haz the power, 2016-10-11 00:05:50]

This CL has an open dependency (Issue 2409813002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[dpapad, 2016-10-11 01:02:49]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-10-11 01:02:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2409753003/#ps30002
(title: "No diff")

[commit-bot: I haz the power, 2016-10-11 01:03:18]

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

[Dan Beam, 2016-10-11 01:38:52]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-10-11 01:38:54]

https://codereview.chromium.org/2409753003/diff/30002/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_private/settings_private_delegate.cc
(right):

https://codereview.chromium.org/2409753003/diff/30002/chrome/browser/extensio...
chrome/browser/extensions/api/settings_private/settings_private_delegate.cc:65:
new base::FundamentalValue(std::round(zoom)));
does this work well with 33.33%?

[dpapad, 2016-10-11 01:40:13]

The CQ bit was unchecked by dpapad@chromium.org

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

On 2016/10/11 at 01:38:54, dbeam wrote:
>
https://codereview.chromium.org/2409753003/diff/30002/chrome/browser/extensio...
> File
chrome/browser/extensions/api/settings_private/settings_private_delegate.cc
(right):
> 
>
https://codereview.chromium.org/2409753003/diff/30002/chrome/browser/extensio...
>
chrome/browser/extensions/api/settings_private/settings_private_delegate.cc:65:
new base::FundamentalValue(std::round(zoom)));
> does this work well with 33.33%?

Not really, it is subtle. Here is what is happening:

In the old Options, when the user sets the value of 33% in the dropdown, we were
actually setting the zoom to 33.333333. This is not the case in the new settings
(see comparison at [1] and [2]), where we only set the zoom percentage to
integer values, and have also designed the
chrome.setttingPrivate.setDefaultZoomPercent to accept
integers only, see [3].

This CL preserves the existing behavior of MD Settings, which was already
deviated by the old Options behavior.

[1] http://imgur.com/a/0Vxjj
[2]
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appear...
[3]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p...

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

On 2016/10/11 at 17:48:10, dpapad wrote:
> On 2016/10/11 at 01:38:54, dbeam wrote:
> >
https://codereview.chromium.org/2409753003/diff/30002/chrome/browser/extensio...
> > File
chrome/browser/extensions/api/settings_private/settings_private_delegate.cc
(right):
> > 
> >
https://codereview.chromium.org/2409753003/diff/30002/chrome/browser/extensio...
> >
chrome/browser/extensions/api/settings_private/settings_private_delegate.cc:65:
new base::FundamentalValue(std::round(zoom)));
> > does this work well with 33.33%?
> 
> Not really, it is subtle. Here is what is happening:
> 
> In the old Options, when the user sets the value of 33% in the dropdown, we
were actually setting the zoom to 33.333333. This is not the case in the new
settings
> (see comparison at [1] and [2]), where we only set the zoom percentage to
integer values, and have also designed the
chrome.setttingPrivate.setDefaultZoomPercent to accept
> integers only, see [3].
> 
> This CL preserves the existing behavior of MD Settings, which was already
deviated by the old Options behavior.
> 
> [1] http://imgur.com/a/0Vxjj
> [2]
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appear...
> [3]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p...

^Friendly ping.

[Dan Beam, 2016-10-12 23:15:53]

On 2016/10/11 17:48:10, dpapad wrote:
> On 2016/10/11 at 01:38:54, dbeam wrote:
> >
>
https://codereview.chromium.org/2409753003/diff/30002/chrome/browser/extensio...
> > File
> chrome/browser/extensions/api/settings_private/settings_private_delegate.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2409753003/diff/30002/chrome/browser/extensio...
> >
>
chrome/browser/extensions/api/settings_private/settings_private_delegate.cc:65:
> new base::FundamentalValue(std::round(zoom)));
> > does this work well with 33.33%?
> 
> Not really, it is subtle. Here is what is happening:
> 
> In the old Options, when the user sets the value of 33% in the dropdown, we
were
> actually setting the zoom to 33.333333. This is not the case in the new
settings
> (see comparison at [1] and [2]), where we only set the zoom percentage to
> integer values, and have also designed the
> chrome.setttingPrivate.setDefaultZoomPercent to accept
> integers only, see [3].
> 
> This CL preserves the existing behavior of MD Settings, which was already
> deviated by the old Options behavior.

Don't do that ^. Preserve old Options where possible.

> 
> [1] http://imgur.com/a/0Vxjj
> [2]
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appear...
> [3]
>
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p...

[dpapad, 2016-10-12 23:28:41]

On 2016/10/12 at 23:15:53, dbeam wrote:
> On 2016/10/11 17:48:10, dpapad wrote:
> > On 2016/10/11 at 01:38:54, dbeam wrote:
> > >
> >
https://codereview.chromium.org/2409753003/diff/30002/chrome/browser/extensio...
> > > File
> > chrome/browser/extensions/api/settings_private/settings_private_delegate.cc
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/2409753003/diff/30002/chrome/browser/extensio...
> > >
> >
chrome/browser/extensions/api/settings_private/settings_private_delegate.cc:65:
> > new base::FundamentalValue(std::round(zoom)));
> > > does this work well with 33.33%?
> > 
> > Not really, it is subtle. Here is what is happening:
> > 
> > In the old Options, when the user sets the value of 33% in the dropdown, we
were
> > actually setting the zoom to 33.333333. This is not the case in the new
settings
> > (see comparison at [1] and [2]), where we only set the zoom percentage to
> > integer values, and have also designed the
> > chrome.setttingPrivate.setDefaultZoomPercent to accept
> > integers only, see [3].
> > 
> > This CL preserves the existing behavior of MD Settings, which was already
> > deviated by the old Options behavior.
> 
> Don't do that ^. Preserve old Options where possible.
OK. I am going to file a separate bug then, because this is a much larger task.

Preserving old Options requires changes in
chrome.settingsPrivate.setDefaultZoompPercent API itself
(needs to allow setting a non-integer value) as well as adding a way to
dynamically populate the zoom dropdown values
instead of hardcoding them, similar to
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/browser_....


> 
> > 
> > [1] http://imgur.com/a/0Vxjj
> > [2]
> >
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appear...
> > [3]
> >
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p...

[dpapad, 2016-10-13 18:06:23]

On 2016/10/12 at 23:28:41, dpapad wrote:
> On 2016/10/12 at 23:15:53, dbeam wrote:
> > On 2016/10/11 17:48:10, dpapad wrote:
> > > On 2016/10/11 at 01:38:54, dbeam wrote:
> > > >
> > >
https://codereview.chromium.org/2409753003/diff/30002/chrome/browser/extensio...
> > > > File
> > >
chrome/browser/extensions/api/settings_private/settings_private_delegate.cc
> > > (right):
> > > > 
> > > >
> > >
https://codereview.chromium.org/2409753003/diff/30002/chrome/browser/extensio...
> > > >
> > >
chrome/browser/extensions/api/settings_private/settings_private_delegate.cc:65:
> > > new base::FundamentalValue(std::round(zoom)));
> > > > does this work well with 33.33%?
> > > 
> > > Not really, it is subtle. Here is what is happening:
> > > 
> > > In the old Options, when the user sets the value of 33% in the dropdown,
we were
> > > actually setting the zoom to 33.333333. This is not the case in the new
settings
> > > (see comparison at [1] and [2]), where we only set the zoom percentage to
> > > integer values, and have also designed the
> > > chrome.setttingPrivate.setDefaultZoomPercent to accept
> > > integers only, see [3].
> > > 
> > > This CL preserves the existing behavior of MD Settings, which was already
> > > deviated by the old Options behavior.
> > 
> > Don't do that ^. Preserve old Options where possible.
> OK. I am going to file a separate bug then, because this is a much larger
task.
> 
> Preserving old Options requires changes in
chrome.settingsPrivate.setDefaultZoompPercent API itself
> (needs to allow setting a non-integer value) as well as adding a way to
dynamically populate the zoom dropdown values
> instead of hardcoding them, similar to
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/browser_....
> 
> 
> > 
> > > 
> > > [1] http://imgur.com/a/0Vxjj
> > > [2]
> > >
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/appear...
> > > [3]
> > >
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p...

FYI, filed crbug.com/655742 about this.

[Dan Beam, 2016-10-14 23:31:17]

lgtm do whatever you want

[Dan Beam, 2016-10-15 06:07:55]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2016-10-15 06:08:10]

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 06:48:47]

Committed patchset #3 (id:30002)

[commit-bot: I haz the power, 2016-10-15 06:50:32]

Description was changed from

==========
MD Settings: chrome.settingsPrivate, fix rounding error.


BUG=652483
==========

to

==========
MD Settings: chrome.settingsPrivate, fix rounding error.

BUG=652483

Committed: https://crrev.com/6a3ab25a3152c4ab095c78347c70fe082f6a1743
Cr-Commit-Position: refs/heads/master@{#425552}
==========

[commit-bot: I haz the power, 2016-10-15 06:50:32]

Patchset 3 (id:??) landed as
https://crrev.com/6a3ab25a3152c4ab095c78347c70fe082f6a1743
Cr-Commit-Position: refs/heads/master@{#425552}