Add default zoom functionality to chrome.tabs Zoom API.

Add default zoom functionality to chrome.tabs Zoom API.

This CL adds default zoom factor functionality to the tabs zoom api.

Tabs can now be set to the current default zoom via

setZoom(tabId, 0)

and the current default zoom level can be retrieved as a parameter
default_zoom_factor in the ZoomSettings structure returned by
getZoomSettings.

BUG=470550
Committed: https://crrev.com/5448610da81a6438bdf2f1b0a1827c0287073f6f
Cr-Commit-Position: refs/heads/master@{#323505}

[wjmaclean, 2015-04-01 18:49:29]

wjmaclean@chromium.org changed reviewers:
+ kalman@chromium.org

[wjmaclean, 2015-04-01 18:49:55]

This is ready for review; please take a look?

[wjmaclean, 2015-04-01 18:51:06]

Adding link to (re-)design doc: 

https://docs.google.com/a/chromium.org/document/d/15DWyVeK7DUCswPO5eBVTLltEON...

[not at google - send to devlin, 2015-04-01 20:34:07]

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_api.cc:1960:
content::ZoomLevelToZoomFactor(zoom_controller->GetDefaultZoomLevel());
Simpler to write this as just 1 statement:

zoom_settings.default_zoom_factor.reset(new double(
    content::ZoomLevelToZoomFactor(zoom_controller->GetDefaultZoomLevel()));

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_test.cc:889: auto zoom_controller =
ui_zoom::ZoomController::FromWebContents(web_contents);
Using auto here makes it harder to read IMO, so just spell out the type.

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_test.cc:892:
EXPECT_NEAR(zoom_controller->GetDefaultZoomLevel(),
I've been told that it's better to use EXPECT_DOUBLE_EQ rather than EXPECT_NEAR.
The former figures out the epsilon automatically.

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_test.cc:906: default_zoom_level += 0.5;
I'd rather not mutate this default_zoom_level variable, keep it as a constant,
and instead add/subtract 0.5 as needed.

https://codereview.chromium.org/1051013002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/1051013002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/tabs.json:75: "optional": true,
Why optional?

https://codereview.chromium.org/1051013002/diff/1/chrome/common/extensions/do...
File chrome/common/extensions/docs/examples/api/tabs/zoom/popup.js (right):

https://codereview.chromium.org/1051013002/diff/1/chrome/common/extensions/do...
chrome/common/extensions/docs/examples/api/tabs/zoom/popup.js:100:
chrome.tabs.setZoom(tabId, 'default', function() {
Need to update this.

[wjmaclean, 2015-04-01 21:23:24]

Comments addressed, ptal?

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_api.cc:1960:
content::ZoomLevelToZoomFactor(zoom_controller->GetDefaultZoomLevel());
On 2015/04/01 20:34:07, kalman wrote:
> Simpler to write this as just 1 statement:
> 
> zoom_settings.default_zoom_factor.reset(new double(
>     content::ZoomLevelToZoomFactor(zoom_controller->GetDefaultZoomLevel()));

Done.

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_test.cc:889: auto zoom_controller =
ui_zoom::ZoomController::FromWebContents(web_contents);
On 2015/04/01 20:34:07, kalman wrote:
> Using auto here makes it harder to read IMO, so just spell out the type.

I thought this would save typing ui_zoom::ZoomController twice on the same line,
but no worries!

Done.

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_test.cc:892:
EXPECT_NEAR(zoom_controller->GetDefaultZoomLevel(),
On 2015/04/01 20:34:07, kalman wrote:
> I've been told that it's better to use EXPECT_DOUBLE_EQ rather than
EXPECT_NEAR.
> The former figures out the epsilon automatically.

I tried using that initially, but it fails with output like:

../../chrome/browser/extensions/api/tabs/tabs_test.cc:911: Failure
Value of: content::ZoomFactorToZoomLevel(default_zoom_factor)
  Actual: 0.49999999999999972
Expected: default_zoom_level + 0.5
Which is: 0.5

Given that here they differ by ~0.000000000000001/.5 = 1 part in a hundred
trillion (if I'm counting zeroes correctly), I think the other way is safer. The
Zoom API forces conversions between zoom factors and levels, which involves
log(), so we expect some non-trivial epsilon.

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_test.cc:906: default_zoom_level += 0.5;
On 2015/04/01 20:34:07, kalman wrote:
> I'd rather not mutate this default_zoom_level variable, keep it as a constant,
> and instead add/subtract 0.5 as needed.

Done.

https://codereview.chromium.org/1051013002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/1051013002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/tabs.json:75: "optional": true,
On 2015/04/01 20:34:07, kalman wrote:
> Why optional?

Making it not-optional requires callers to setZoomSettings(), which uses this
same struct, to specify a value for this parameter (and the value is just
ignored). I think it's best not to require input of a dummy value.

https://codereview.chromium.org/1051013002/diff/1/chrome/common/extensions/do...
File chrome/common/extensions/docs/examples/api/tabs/zoom/popup.js (right):

https://codereview.chromium.org/1051013002/diff/1/chrome/common/extensions/do...
chrome/common/extensions/docs/examples/api/tabs/zoom/popup.js:100:
chrome.tabs.setZoom(tabId, 'default', function() {
On 2015/04/01 20:34:07, kalman wrote:
> Need to update this.

Ooops, missed that one, thanks!

Done.

[not at google - send to devlin, 2015-04-01 22:26:12]

lg with a last couple of test nits.

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/1051013002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/tabs/tabs_test.cc:892:
EXPECT_NEAR(zoom_controller->GetDefaultZoomLevel(),
On 2015/04/01 21:23:24, wjmaclean wrote:
> On 2015/04/01 20:34:07, kalman wrote:
> > I've been told that it's better to use EXPECT_DOUBLE_EQ rather than
> EXPECT_NEAR.
> > The former figures out the epsilon automatically.
> 
> I tried using that initially, but it fails with output like:
> 
> ../../chrome/browser/extensions/api/tabs/tabs_test.cc:911: Failure
> Value of: content::ZoomFactorToZoomLevel(default_zoom_factor)
>   Actual: 0.49999999999999972
> Expected: default_zoom_level + 0.5
> Which is: 0.5
> 
> Given that here they differ by ~0.000000000000001/.5 = 1 part in a hundred
> trillion (if I'm counting zeroes correctly), I think the other way is safer.
The
> Zoom API forces conversions between zoom factors and levels, which involves
> log(), so we expect some non-trivial epsilon.

Could you make a note of that; or perhaps better, document it via a method which
does this for you. I had in mind something like

// Explain why implementing own floating point checker.
testing::AssertionResult CheckDefaultZoomLevel(
    double expected, ZoomController controller) {
  double kEpsilon = ...;
  double actual = controller->GetDefaultZoomLevel();
  return abs(expected - actual) < kEpsilon ?
      testing::AssertionSuccess() :
      testing::AssertionFailure() << // etc;
}

https://codereview.chromium.org/1051013002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/1051013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:922: auto zoom_controller =
ui_zoom::ZoomController::FromWebContents(web_contents);
Same comments apply to this method as apply to the one above in previous
patchset.

[wjmaclean, 2015-04-02 12:03:14]

PTAL?

Your suggestion of a function to document the issue with comparing zoom levels
reminded we should be using content::ZoomValuesEqual(). I think this is in
keeping with your suggestion.

https://codereview.chromium.org/1051013002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/1051013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:922: auto zoom_controller =
ui_zoom::ZoomController::FromWebContents(web_contents);
On 2015/04/01 22:26:12, kalman wrote:
> Same comments apply to this method as apply to the one above in previous
> patchset.

Done.

[not at google - send to devlin, 2015-04-02 14:41:21]

lgtm

https://codereview.chromium.org/1051013002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/1051013002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:798: << "default zoom factor not
found in result";
The body of this should be in braces, both the condition and the body are >1
line each.

[wjmaclean, 2015-04-02 14:59:24]

Thanks!

https://codereview.chromium.org/1051013002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/1051013002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:798: << "default zoom factor not
found in result";
On 2015/04/02 14:41:21, kalman wrote:
> The body of this should be in braces, both the condition and the body are >1
> line each.

Done.

[wjmaclean, 2015-04-02 14:59:31]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2015-04-02 14:59:31]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1051013002/#ps60001
(title: "Add braces around if-clause statement.")

[commit-bot: I haz the power, 2015-04-02 15:01:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1051013002/60001

[commit-bot: I haz the power, 2015-04-02 16:21:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-03 20:26:18]

Patchset 4 (id:??) landed as
https://crrev.com/5448610da81a6438bdf2f1b0a1827c0287073f6f
Cr-Commit-Position: refs/heads/master@{#323505}