Sample Extension for Tabs Zoom API.

Sample Extension for Tabs Zoom API.

Add sample extension code for the new Tabs Zoom API.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284430

[wjmaclean, 2014-07-16 14:57:18]

This CL contains a simple demo extension to demonstrate the use of the new Tabs
Zoom API.

I'm new to JS/extensions, so apologies in advance for any newbie mistakes. Also,
I picked the popup html colours to "stand out", but I'm more than happy to
entertain colour suggestions that are more aesthetically pleasing.

[not at google - send to devlin, 2014-07-16 15:55:58]

Raymes, perhaps you would be the best person to look at this since you've been
using the API?

[raymes, 2014-07-17 00:39:10]

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/examples/api/tabs/background.js (right):

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/background.js:8: ", scope:" +
zoomChangeInfo.zoomSettings.scope;
nit: the indentation looks funny here. I think a 4 space indent is what you
want?

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/examples/api/tabs/manifest.json (right):

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/manifest.json:10: "tabs"
nit: 2 space indent
I'm not sure if the tabs permission actually needs to be included in the
manifest at all. You might consider removing it.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/manifest.json:24:
"default_icon": "zoom19.png",
nit: 2 space indent

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/examples/api/tabs/popup.html (right):

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.html:26: <button
type="button" id="displayButton">100%</button>
Since clicking this doesn't seem to do anything, maybe it's better to get rid of
it and just use a div here instead?

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.html:29: <div
id="zoomFactorDisplay"></div>
This div seems to be unused so I think it can be removed.

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

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:29: tabId= tabs[0].id;
nit: tabId =
This seems like it is just going to use an arbitrary tab (not necessarily the
current one, which would make behavior weird)?

Since you only ever choose 1 tab, why not just omit the tab ID altogether which
causes the zoom API to default to the current tab. Alternatively you could allow
the user to select the tab to zoom, but this would make the example
significantly more complicated.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:33: for (var i = 0; i <
modeRadios.length; i++)
need {} around this multi-line for loop (here and below)

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:64: currentZoomFactor =
zoomFactor;
I think you can get rid of the "currentZoomFactor" variable. It's only used on
the next line in place of zoomFactor.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:106: if ((modeVal ===
undefined) || (scopeVal === undefined)) {
if (!modeVal || !scopeVal)

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:108: self.close();
My guess is that this case should never really happen? It might be worth just
logging something to the console rather than the above 2 lines.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:114: if
(chrome.runtime.lastError)
nit: {} for multi-line if

[wjmaclean, 2014-07-17 18:46:27]

PTAL

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/examples/api/tabs/background.js (right):

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/background.js:8: ", scope:" +
zoomChangeInfo.zoomSettings.scope;
On 2014/07/17 00:39:10, raymes wrote:
> nit: the indentation looks funny here. I think a 4 space indent is what you
> want?

Done.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/examples/api/tabs/manifest.json (right):

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/manifest.json:10: "tabs"
On 2014/07/17 00:39:10, raymes wrote:
> nit: 2 space indent
> I'm not sure if the tabs permission actually needs to be included in the
> manifest at all. You might consider removing it.

Done.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/manifest.json:24:
"default_icon": "zoom19.png",
On 2014/07/17 00:39:10, raymes wrote:
> nit: 2 space indent

Done.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/examples/api/tabs/popup.html (right):

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.html:26: <button
type="button" id="displayButton">100%</button>
On 2014/07/17 00:39:10, raymes wrote:
> Since clicking this doesn't seem to do anything, maybe it's better to get rid
of
> it and just use a div here instead?

Done.

I had made it a button in order to get a consistent visual appearance, but a div
is file too.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.html:29: <div
id="zoomFactorDisplay"></div>
On 2014/07/17 00:39:10, raymes wrote:
> This div seems to be unused so I think it can be removed.

Done.

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

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:29: tabId= tabs[0].id;
On 2014/07/17 00:39:10, raymes wrote:
> nit: tabId =
> This seems like it is just going to use an arbitrary tab (not necessarily the
> current one, which would make behavior weird)?

I thought the query would only ever return the current tab, so the array would
only have one element. I wanted to showcase the zoom api calls with a tabId ...
is there a better way to get the tabId of the current tab?

> Since you only ever choose 1 tab, why not just omit the tab ID altogether
which
> causes the zoom API to default to the current tab. Alternatively you could
allow
> the user to select the tab to zoom, but this would make the example
> significantly more complicated.

Done.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:33: for (var i = 0; i <
modeRadios.length; i++)
On 2014/07/17 00:39:10, raymes wrote:
> need {} around this multi-line for loop (here and below)

Done.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:64: currentZoomFactor =
zoomFactor;
On 2014/07/17 00:39:10, raymes wrote:
> I think you can get rid of the "currentZoomFactor" variable. It's only used on
> the next line in place of zoomFactor.

Done.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:106: if ((modeVal ===
undefined) || (scopeVal === undefined)) {
On 2014/07/17 00:39:10, raymes wrote:
> if (!modeVal || !scopeVal)

Done.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:108: self.close();
On 2014/07/17 00:39:10, raymes wrote:
> My guess is that this case should never really happen? It might be worth just
> logging something to the console rather than the above 2 lines.

Done.

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:114: if
(chrome.runtime.lastError)
On 2014/07/17 00:39:10, raymes wrote:
> nit: {} for multi-line if

Done.

[raymes, 2014-07-18 00:10:06]

lgtm

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

https://codereview.chromium.org/398823002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/popup.js:29: tabId= tabs[0].id;
Ah, my bad it seems like your query would correctly only return the current tab.
If you want to keep it that way, it's fine by me. Sorry for the noise.

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

https://codereview.chromium.org/398823002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/tabs/zoom/popup.js:39:
chrome.tabs.getZoom(displayZoomLevel);
All the code in this function up to here seems indented one level too much.

[wjmaclean, 2014-07-18 18:12:49]

I've gone ahead and added the tabId back in, but added comments to indicate what
the query is doing and why. The comments include the fact that tabId, when
omitted, will act on the current tab by default.

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

https://codereview.chromium.org/398823002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/tabs/zoom/popup.js:39:
chrome.tabs.getZoom(displayZoomLevel);
On 2014/07/18 00:10:06, raymes wrote:
> All the code in this function up to here seems indented one level too much.

Done.

[wjmaclean, 2014-07-18 18:13:04]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-07-18 18:14:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/398823002/40001

[commit-bot: I haz the power, 2014-07-18 20:49:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-18 21:01:40]

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

[commit-bot: I haz the power, 2014-07-18 21:01:41]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[wjmaclean, 2014-07-18 21:03:49]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-07-18 21:05:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/398823002/40001

[not at google - send to devlin, 2014-07-18 21:07:36]

rslgtm

[commit-bot: I haz the power, 2014-07-18 21:48:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-18 22:01:10]

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

[commit-bot: I haz the power, 2014-07-18 22:01:11]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[wjmaclean, 2014-07-21 13:08:53]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-07-21 13:09:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/398823002/40001

[commit-bot: I haz the power, 2014-07-21 13:12:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-21 13:15:50]

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

[commit-bot: I haz the power, 2014-07-21 13:15:51]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[wjmaclean, 2014-07-21 13:32:36]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-07-21 13:33:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/398823002/60001

[commit-bot: I haz the power, 2014-07-21 15:16:51]

Change committed as 284430