Font size in DomDistiller prefs syncs with local scaling

Font size in DomDistiller prefs syncs with local scaling

The font size in DomDistiller preference was linked with accessibility
scaling. Now it links with local scaling, which can also be adjusted by
pinch-to-zoom gesture. The local scaling syncs across tabs.

This local scaling doesn't sync across devices by design.

BUG=445632, 464822
TEST=browser_tests --gtest_filter="DomDistiller*"

Committed: https://crrev.com/30b0ae4316cd7a5eeabc9201342c7b9903978baf
Cr-Commit-Position: refs/heads/master@{#350976}

[wychen, 2015-07-09 01:41:26]

wychen@chromium.org changed reviewers:
+ cjhopman@chromium.org, nyquist@chromium.org

[wychen, 2015-07-09 01:41:27]

PTAL

[cjhopman, 2015-07-17 20:28:04]

https://codereview.chromium.org/1225183002/diff/20001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/1225183002/diff/20001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:234: ss <<
path.substr(strlen(kViewerSaveFontScalingPath));
This should probably use base::StringToDouble.

https://codereview.chromium.org/1225183002/diff/20001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/1225183002/diff/20001/components/dom_distille...
components/dom_distiller/core/viewer.cc:308: std::stringstream ss;
maybe:

return "useFontScaling(" + base::DoubleToString(scaling) + ");";

[wychen, 2015-07-31 21:03:37]

https://codereview.chromium.org/1225183002/diff/20001/components/dom_distille...
File components/dom_distiller/content/dom_distiller_viewer_source.cc (right):

https://codereview.chromium.org/1225183002/diff/20001/components/dom_distille...
components/dom_distiller/content/dom_distiller_viewer_source.cc:234: ss <<
path.substr(strlen(kViewerSaveFontScalingPath));
On 2015/07/17 20:28:04, cjhopman wrote:
> This should probably use base::StringToDouble.

Done.

https://codereview.chromium.org/1225183002/diff/20001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/1225183002/diff/20001/components/dom_distille...
components/dom_distiller/core/viewer.cc:308: std::stringstream ss;
On 2015/07/17 20:28:04, cjhopman wrote:
> maybe:
> 
> return "useFontScaling(" + base::DoubleToString(scaling) + ");";
> 

Done.

[nyquist, 2015-08-11 06:27:57]

https://codereview.chromium.org/1225183002/diff/80001/components/dom_distille...
File components/dom_distiller/content/browser/dom_distiller_viewer_source.cc
(right):

https://codereview.chromium.org/1225183002/diff/80001/components/dom_distille...
components/dom_distiller/content/browser/dom_distiller_viewer_source.cc:234:
base::StringToDouble(
Should we do something if base::StringToDouble returns false?

https://codereview.chromium.org/1225183002/diff/80001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_request_view_base.cc (right):

https://codereview.chromium.org/1225183002/diff/80001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.cc:105: float
scaling) {
Nit: move one line up

https://codereview.chromium.org/1225183002/diff/80001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/1225183002/diff/80001/components/dom_distille...
components/dom_distiller/core/viewer.cc:303: float scaling) {
Nit: move to previous line?

https://codereview.chromium.org/1225183002/diff/80001/components/test/data/do...
File components/test/data/dom_distiller/pinch_tester.js (right):

https://codereview.chromium.org/1225183002/diff/80001/components/test/data/do...
components/test/data/dom_distiller/pinch_tester.js:379: var oldState =
pincher.status();
Would it be possible to add a one line comment above each of these sections?

Something like:
- Start touch
- Pinch-in
- Verified scale is less

- clear state, and re-initialize

- Start touch
- Pinch-out
- Verified scale is more

But probably  with some better text, but something along those lines?

It would make it both simple to understand this test overall, and also to verify
each section.

[wychen, 2015-08-31 21:47:39]

https://codereview.chromium.org/1225183002/diff/80001/components/dom_distille...
File components/dom_distiller/content/browser/dom_distiller_viewer_source.cc
(right):

https://codereview.chromium.org/1225183002/diff/80001/components/dom_distille...
components/dom_distiller/content/browser/dom_distiller_viewer_source.cc:234:
base::StringToDouble(
On 2015/08/11 06:27:56, nyquist wrote:
> Should we do something if base::StringToDouble returns false?

Done.

https://codereview.chromium.org/1225183002/diff/80001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_request_view_base.cc (right):

https://codereview.chromium.org/1225183002/diff/80001/components/dom_distille...
components/dom_distiller/core/dom_distiller_request_view_base.cc:105: float
scaling) {
On 2015/08/11 06:27:57, nyquist wrote:
> Nit: move one line up

Done.

https://codereview.chromium.org/1225183002/diff/80001/components/dom_distille...
File components/dom_distiller/core/viewer.cc (right):

https://codereview.chromium.org/1225183002/diff/80001/components/dom_distille...
components/dom_distiller/core/viewer.cc:303: float scaling) {
On 2015/08/11 06:27:57, nyquist wrote:
> Nit: move to previous line?

Done.

https://codereview.chromium.org/1225183002/diff/80001/components/test/data/do...
File components/test/data/dom_distiller/pinch_tester.js (right):

https://codereview.chromium.org/1225183002/diff/80001/components/test/data/do...
components/test/data/dom_distiller/pinch_tester.js:379: var oldState =
pincher.status();
On 2015/08/11 06:27:57, nyquist wrote:
> Would it be possible to add a one line comment above each of these sections?
> 
> Something like:
> - Start touch
> - Pinch-in
> - Verified scale is less
> 
> - clear state, and re-initialize
> 
> - Start touch
> - Pinch-out
> - Verified scale is more
> 
> But probably  with some better text, but something along those lines?
> 
> It would make it both simple to understand this test overall, and also to
verify
> each section.

Done.

[nyquist, 2015-08-31 21:48:32]

lgtm

[wychen, 2015-08-31 21:58:57]

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

[commit-bot: I haz the power, 2015-08-31 21:59:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225183002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1225183002/140001

[commit-bot: I haz the power, 2015-08-31 22:10:35]

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

[commit-bot: I haz the power, 2015-08-31 22:10:35]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[wychen, 2015-08-31 22:41:11]

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

[commit-bot: I haz the power, 2015-08-31 22:41:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225183002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1225183002/160001

[commit-bot: I haz the power, 2015-08-31 22:57:55]

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

[commit-bot: I haz the power, 2015-08-31 22:57:56]

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

[wychen, 2015-09-01 00:14:03]

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

[commit-bot: I haz the power, 2015-09-01 00:14:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225183002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1225183002/180001

[commit-bot: I haz the power, 2015-09-01 00:44:44]

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

[commit-bot: I haz the power, 2015-09-01 00:44:45]

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

[wychen, 2015-09-01 00:58:30]

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

[commit-bot: I haz the power, 2015-09-01 00:58:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225183002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1225183002/200001

[commit-bot: I haz the power, 2015-09-01 04:52:39]

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

[commit-bot: I haz the power, 2015-09-01 04:52:40]

Dry run: This issue passed the CQ dry run.

[wychen, 2015-09-23 23:56:04]

The CQ bit was checked by wychen@chromium.org

[wychen, 2015-09-23 23:56:04]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1225183002/#ps200001
(title: "fix test init")

[commit-bot: I haz the power, 2015-09-23 23:58:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225183002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1225183002/200001

[commit-bot: I haz the power, 2015-09-24 00:03:28]

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

[commit-bot: I haz the power, 2015-09-24 00:03:28]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wychen, 2015-09-24 10:47:35]

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

[commit-bot: I haz the power, 2015-09-24 10:47:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225183002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1225183002/240001

[commit-bot: I haz the power, 2015-09-24 11:42:03]

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

[commit-bot: I haz the power, 2015-09-24 11:42:04]

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

[wychen, 2015-09-24 23:07:51]

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

[commit-bot: I haz the power, 2015-09-24 23:08:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225183002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1225183002/260001

[commit-bot: I haz the power, 2015-09-24 23:12:18]

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

[commit-bot: I haz the power, 2015-09-24 23:12:19]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wychen, 2015-09-24 23:39:27]

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

[commit-bot: I haz the power, 2015-09-24 23:40:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225183002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1225183002/280001

[wychen, 2015-09-25 00:14:08]

wychen@chromium.org changed reviewers:
+ mdjones@chromium.org
- cjhopman@chromium.org

[wychen, 2015-09-25 00:14:09]

+mdjones

Tommy, can you have another look? The last patch set you reviewed might be #9.
Thanks!

[commit-bot: I haz the power, 2015-09-25 01:22:59]

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

[commit-bot: I haz the power, 2015-09-25 01:23:00]

Dry run: This issue passed the CQ dry run.

[mdjones, 2015-09-25 22:26:28]

Looks good! Only one issue I think needs addressing.

https://codereview.chromium.org/1225183002/diff/280001/components/dom_distill...
File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right):

https://codereview.chromium.org/1225183002/diff/280001/components/dom_distill...
components/dom_distiller/core/javascript/dom_distiller_viewer.js:250:
clampedScale = Math.max(0.4, Math.min(2.5, fontSizeAnchor * slowedScale));
These numbers do not match the min/max of the android slider (i.e. is is
possible to zoom by pinch to min 40% but only possible to use the slider to make
it go to 50%). I would either change these or the android numbers to make them
consistent.

[wychen, 2015-09-25 23:31:43]

https://codereview.chromium.org/1225183002/diff/280001/components/dom_distill...
File components/dom_distiller/core/javascript/dom_distiller_viewer.js (right):

https://codereview.chromium.org/1225183002/diff/280001/components/dom_distill...
components/dom_distiller/core/javascript/dom_distiller_viewer.js:250:
clampedScale = Math.max(0.4, Math.min(2.5, fontSizeAnchor * slowedScale));
On 2015/09/25 22:26:28, mdjones wrote:
> These numbers do not match the min/max of the android slider (i.e. is is
> possible to zoom by pinch to min 40% but only possible to use the slider to
make
> it go to 50%). I would either change these or the android numbers to make them
> consistent.

Done.

[mdjones, 2015-09-25 23:49:29]

lgtm

[wychen, 2015-09-26 01:00:40]

The CQ bit was checked by wychen@chromium.org

[wychen, 2015-09-26 01:00:41]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1225183002/#ps300001
(title: "match android slider min/max")

[commit-bot: I haz the power, 2015-09-26 01:01:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225183002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1225183002/300001

[commit-bot: I haz the power, 2015-09-26 01:02:32]

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

[commit-bot: I haz the power, 2015-09-26 01:02:35]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wychen, 2015-09-26 01:11:16]

The CQ bit was checked by wychen@chromium.org

[wychen, 2015-09-26 01:11:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1225183002/#ps320001
(title: "merge master again")

[commit-bot: I haz the power, 2015-09-26 01:11:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1225183002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1225183002/320001

[commit-bot: I haz the power, 2015-09-26 02:54:02]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-09-26 02:54:54]

Patchset 17 (id:??) landed as
https://crrev.com/30b0ae4316cd7a5eeabc9201342c7b9903978baf
Cr-Commit-Position: refs/heads/master@{#350976}