Zoom Extension API

Implementation of a Zoom Extension API as an addition to the chrome.tabs API, as outlined in https://docs.google.com/a/chromium.org/document/d/1sCZjx1J3_M2a02T8NXd-ufGKZDoBHI5Ixis1DaGLQCA/edit?usp=sharing.

BUG=30583

[paulmeyer, 2014-04-07 19:43:23]

+fsamuel@

[Fady Samuel, 2014-04-07 20:01:14]

First set of comments.

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

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:1631: return false;
Spacing looks wrong here.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:1989: } else if
(web_contents->GetURL().SchemeIs("chrome")) {
Are we sure this API is also isolated from <webview>?

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:2020: DCHECK(type ==
chrome::NOTIFICATION_TAB_ZOOM_CHANGE_COMPLETE);
If we switch to a callback approach, would this notification still be necessary?

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:2061: } else if
(web_contents->GetURL().SchemeIs("chrome")) {
What about <webview>?

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:2067: if
(params->zoom_settings.scope ==
This is very confusing. I think it would be nice to better explain what these
conditions are. Perhaps store this in temporary variables and add comments.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:2123:
SetResult(zoom_settings->DeepCopy());
You're leaking memory here. Just SetResult(zoom_settings);

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:489: if (tab_id >= 0) {
Early exist preferred.

if (tab_id < 0)
  return;

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function_test_utils.cc:276: return
RunFunction(function, args, browser, NONE);
What is the purpose of this change?

https://codereview.chromium.org/225093019/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/225093019/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:267:
SetLayoutManager(grid_layout);
Please comment on what this code is doing. Presumably laying out the bubble such
that an extension icon is shown.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/ui/zoom/zoom_...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/225093019/diff/1/chrome/browser/ui/zoom/zoom_...
chrome/browser/ui/zoom/zoom_controller.cc:73: int ZoomController::last_zoom_id()
const {
I'm really not a fan of exposing ZoomIds.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/ui/zoom/zoom_...
chrome/browser/ui/zoom/zoom_controller.cc:141: web_contents()->GetZoomMode() !=
content::kZoomModeDefault ||
This needs some explanation.

[Fady Samuel, 2014-04-07 20:04:53]

An initial set of comments.

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

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:2011: registrar_.Add(this,
chrome::NOTIFICATION_TAB_ZOOM_CHANGE_COMPLETE,
So you don't need this notification if you have a callback.

[paulmeyer, 2014-04-07 20:56:10]

ptal

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

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:1631: return false;
On 2014/04/07 20:01:14, Fady Samuel wrote:
> Spacing looks wrong here.

Done.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:1989: } else if
(web_contents->GetURL().SchemeIs("chrome")) {
On 2014/04/07 20:01:14, Fady Samuel wrote:
> Are we sure this API is also isolated from <webview>?

Perhaps not for now due to the existing <webview> zooming per-origin bug, but
this can be fixed in a separate patch.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:2011: registrar_.Add(this,
chrome::NOTIFICATION_TAB_ZOOM_CHANGE_COMPLETE,
On 2014/04/07 20:04:53, Fady Samuel wrote:
> So you don't need this notification if you have a callback.

I believe that is true.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:2020: DCHECK(type ==
chrome::NOTIFICATION_TAB_ZOOM_CHANGE_COMPLETE);
On 2014/04/07 20:01:14, Fady Samuel wrote:
> If we switch to a callback approach, would this notification still be
necessary?

I believe it can be taken out if callbacks are used.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:2061: } else if
(web_contents->GetURL().SchemeIs("chrome")) {
On 2014/04/07 20:01:14, Fady Samuel wrote:
> What about <webview>?

Can be fixed in a separate patch.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:2067: if
(params->zoom_settings.scope ==
On 2014/04/07 20:01:14, Fady Samuel wrote:
> This is very confusing. I think it would be nice to better explain what these
> conditions are. Perhaps store this in temporary variables and add comments.

I made this clearer by setting the default mode and scope before this block, and
adding comments.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_api.cc:2123:
SetResult(zoom_settings->DeepCopy());
On 2014/04/07 20:01:14, Fady Samuel wrote:
> You're leaking memory here. Just SetResult(zoom_settings);

Done.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:489: if (tab_id >= 0) {
On 2014/04/07 20:01:14, Fady Samuel wrote:
> Early exist preferred.
> 
> if (tab_id < 0)
>   return;

Done.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

https://codereview.chromium.org/225093019/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_function_test_utils.cc:276: return
RunFunction(function, args, browser, NONE);
On 2014/04/07 20:01:14, Fady Samuel wrote:
> What is the purpose of this change?

Every other "RunFunction" in this file has an overload with the |flags|
parameter set to NONE. I needed to use this RunFunction with flags = NONE, so
for consistency, I added this overload.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/225093019/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:267:
SetLayoutManager(grid_layout);
On 2014/04/07 20:01:14, Fady Samuel wrote:
> Please comment on what this code is doing. Presumably laying out the bubble
such
> that an extension icon is shown.

Done.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/ui/zoom/zoom_...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/225093019/diff/1/chrome/browser/ui/zoom/zoom_...
chrome/browser/ui/zoom/zoom_controller.cc:73: int ZoomController::last_zoom_id()
const {
On 2014/04/07 20:01:14, Fady Samuel wrote:
> I'm really not a fan of exposing ZoomIds.

Okay, will work on getting rid of all zoom ID stuff once other changes are out
of the way and Ben can start reviewing this patch.

https://codereview.chromium.org/225093019/diff/1/chrome/browser/ui/zoom/zoom_...
chrome/browser/ui/zoom/zoom_controller.cc:141: web_contents()->GetZoomMode() !=
content::kZoomModeDefault ||
On 2014/04/07 20:01:14, Fady Samuel wrote:
> This needs some explanation. 

Done.

[paulmeyer, 2014-04-07 21:38:10]

+kalman@

[not at google - send to devlin, 2014-04-09 03:52:11]

2 more important things:
- split the UI changes out into a separate CL
- restrict this to dev channel until the UI stuff is committed by adding entries
into _api_features.json restricting these functions to dev channel.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:223: std::string
zoom_settings_scope;
you could use const char* for both of these and avoid copying strings. also
given the context is clearly zoom related you could just call these "mode" and
"scope".

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:242: NOTREACHED();
prefer not setting a default case so that you can get compile errors when cases
are left out.

you'd need NULL-checks for those pointers.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1953: content::WebContents**
web_contents) {
surely this functionality is already implemented for the other API functions.
basically everything else has the behaviour to operate on the current tab if no
tab is specified.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1988: } else if
(web_contents->GetURL().SchemeIs("chrome")) {
use kChromeUIScheme not "chrome".

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1999:
scoped_refptr<TabsSetZoomFunction> this_ref = this;
I don't think this makes any difference. just pass |this| into the base::Bind
call.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2003:
base::Bind(&TabsSetZoomFunction::Callback, this_ref))) {
you can also bind to SendReseponse directly:

base::Bind(&TabsSetZoomFunction::SendResponse, this, true)

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2030: SetResult(new
base::FundamentalValue(zoom_factor));
prefer

result_ = tabs::getZoom::Result::Create(...)

I can't remember the exact args to pass there, look in out/Debug/gen/...

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2076: default:
don't specify a default case, then you will get compile errors when you leave
the out

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2086: default:
ditto

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2109:
ZoomModeToZoomSettings(zoom_mode, zoom_settings);
my comments at the top aside - if this function is just used to set the result
of GetZoomSetings then you can use the schema generated model, which would be
better. see comment above about where to look for the actual model, but this
would reference tabs::GetZoomSettings::Result.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.h (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.h:47: base::DictionaryValue*
zoom_settings);
doesn't seem like this needs to be in the header file, could be in an anon
namespace in the .cc file.

however, you probably don't need it at all.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.h:269: bool GetWebContents(int*
tab_id, content::WebContents** web_contents);
hopefully this functionality is already provided, so you won't need this
ZoomAPIFunction interface.

if it *isn't* already provided - if every tab function implements it itself -
that sounds like a nice cleanup.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_constants.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_constants.cc:84: const char
kNoCurrentTabError[] = "No current tab.";
like I said in the .cc file there must already be code which does this, so you
shouldn't need this new error message.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_constants.h (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_constants.h:56: extern const char
kZoomSettingsScope[];
hopefully using the schema-generated model will mean you don't need these
changes

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:153:
registrar_.Add(this, content::NOTIFICATION_WEB_CONTENTS_ZOOM_CHANGE,
it's a shame this uses notifications rather than an observer interface.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:165:
content::Source<WebContents>(contents));
this code should be using registar_.RemoveAll()

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:495:
args->Append(zoom_settings);
a model will be getting generated for OnZoomChange as well, you shouldn't need
to interact with the Value API.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:515: if (type ==
content::NOTIFICATION_NAV_ENTRY_COMMITTED) {
bleh this should be a switch. whatevs.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:527:
content::Source<WebContents>(contents));
you presumably need to unregister for tab zoom notifications here too, but this
should just call UnregisterForTabNotifications.

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

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:619: browser());
it seems clearer to me to actually run the get/set calls after initialising each
function, rather than after all are initialized.

same elsewhere.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:656: new TabsSetZoomFunction());
perhaps you should have a helper function which gets/sets the zoom[settings] via
these extension function calls. then you don't need so much boilerplate in these
higher-level functionality tests.

would make these tests much easier to read.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:769: }
also test the chrome:// URL stuff.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:770: 
otherwise: great tests!

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_function_test_utils.cc:276: return
RunFunction(function, args, browser, NONE);
listing NONE in your tests doesn't seem like a big deal particularly if you pull
out the actual function calls into separate functions.

(i.e. not a strong case for making changes to this file)

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:125: } extension_info_;
why not just hold onto a scoped_refptr<const Extension> directly?

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:135: // Image button in
the zoom bubble that will show the |extension_icon_| image
so I got about this far in the views review before realising: you should split
this part of the review out, too. you don't want to be blocked on UI concerns
from checking code in.

https://codereview.chromium.org/225093019/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/tabs.json:84: "description": "Zoom changes only
take effect in this tab, and zoom changes in other tabs will not affect the
zooming of this tab. Also, <code>per-tab</code> zoom changes are reset on
navigation; navigating a tab will always load pages with their
<code>per-origin</code> zoom factors."
great documentation

https://codereview.chromium.org/225093019/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/tabs.json:1069: {"type": "number", "name":
"newZoomFactor"},
use an object here, not parameters, like

"parameters": [{
  "type": "object",
  "properties": [
    { "type": "integer", "name": "tabId", "mimimum": 0 },
    { "type": "number", "name" ..... }
    {}
  ]
}]

https://codereview.chromium.org/225093019/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/tabs/basics/events.js (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/tabs/basics/events.js:170:
assertEq(zoomSettings.scope, "per-origin");
reverse argument order of the 2 checks above.

[paulmeyer, 2014-04-11 03:01:01]

Addressed comments, moved UI changes out to
https://codereview.chromium.org/234553003/, and restricted to dev channel.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:223: std::string
zoom_settings_scope;
On 2014/04/09 03:52:11, kalman wrote:
> you could use const char* for both of these and avoid copying strings. also
> given the context is clearly zoom related you could just call these "mode" and
> "scope".

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:242: NOTREACHED();
On 2014/04/09 03:52:11, kalman wrote:
> prefer not setting a default case so that you can get compile errors when
cases
> are left out.
> 
> you'd need NULL-checks for those pointers.

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1953: content::WebContents**
web_contents) {
Unfortunately it is implemented but not in a share-able way at the moment, and
also every function implements this functionality slightly differently.

On 2014/04/09 03:52:11, kalman wrote:
> surely this functionality is already implemented for the other API functions.
> basically everything else has the behaviour to operate on the current tab if
no
> tab is specified.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1988: } else if
(web_contents->GetURL().SchemeIs("chrome")) {
On 2014/04/09 03:52:11, kalman wrote:
> use kChromeUIScheme not "chrome".

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1999:
scoped_refptr<TabsSetZoomFunction> this_ref = this;
I believe that the scoped_ptr is necessary to preserve the lifetime of
TabsSetZoomFunction. This was the recommended way to set up the callback in the
documentation in callback.h.

On 2014/04/09 03:52:11, kalman wrote:
> I don't think this makes any difference. just pass |this| into the base::Bind
> call.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2003:
base::Bind(&TabsSetZoomFunction::Callback, this_ref))) {
On 2014/04/09 03:52:11, kalman wrote:
> you can also bind to SendReseponse directly:
> 
> base::Bind(&TabsSetZoomFunction::SendResponse, this, true)

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2030: SetResult(new
base::FundamentalValue(zoom_factor));
On 2014/04/09 03:52:11, kalman wrote:
> prefer
> 
> result_ = tabs::getZoom::Result::Create(...)
> 
> I can't remember the exact args to pass there, look in out/Debug/gen/...

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2076: default:
On 2014/04/09 03:52:11, kalman wrote:
> don't specify a default case, then you will get compile errors when you leave
> the out

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2086: default:
On 2014/04/09 03:52:11, kalman wrote:
> ditto

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2109:
ZoomModeToZoomSettings(zoom_mode, zoom_settings);
It is also used in tabs_event_router.cc, but I improved the function already
using schema-generated stuff.

On 2014/04/09 03:52:11, kalman wrote:
> my comments at the top aside - if this function is just used to set the result
> of GetZoomSetings then you can use the schema generated model, which would be
> better. see comment above about where to look for the actual model, but this
> would reference tabs::GetZoomSettings::Result.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.h (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.h:47: base::DictionaryValue*
zoom_settings);
Talked about this over IM. I fixed up the function to use the schema-generated
data structure.

On 2014/04/09 03:52:11, kalman wrote:
> doesn't seem like this needs to be in the header file, could be in an anon
> namespace in the .cc file.
> 
> however, you probably don't need it at all.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.h:269: bool GetWebContents(int*
tab_id, content::WebContents** web_contents);
This functionality unfortunately isn't provided already. This might be a good
candidate for cleanup, though perhaps in a different patch.

On 2014/04/09 03:52:11, kalman wrote:
> hopefully this functionality is already provided, so you won't need this
> ZoomAPIFunction interface.
> 
> if it *isn't* already provided - if every tab function implements it itself -
> that sounds like a nice cleanup.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_constants.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_constants.cc:84: const char
kNoCurrentTabError[] = "No current tab.";
See other comments about GetWebContents(). That being said, it seems that some
of the other code uses kNoSelectedTabError to mean the same thing as there being
no active, so perhaps I could still get rid of this new error and use that
instead. Is "selected" tab really the right term to use though? Is there any
difference between a "selected" or "active" tab?

On 2014/04/09 03:52:11, kalman wrote:
> like I said in the .cc file there must already be code which does this, so you
> shouldn't need this new error message.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_constants.h (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_constants.h:56: extern const char
kZoomSettingsScope[];
See other comment about ZoomModeToZoomSettings().

On 2014/04/09 03:52:11, kalman wrote:
> hopefully using the schema-generated model will mean you don't need these
> changes

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:153:
registrar_.Add(this, content::NOTIFICATION_WEB_CONTENTS_ZOOM_CHANGE,
Jam just expressed something similar about the notifcations on the content side.
I have replaced them all now so that I am not using ANY notifications. I did use
an observer interface to replace this one. Done.

On 2014/04/09 03:52:11, kalman wrote:
> it's a shame this uses notifications rather than an observer interface.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:165:
content::Source<WebContents>(contents));
I agree, although I'm not using this notification anymore so I don't have any
code here. Is it worth cleaning up the existing "Remove"s here?

On 2014/04/09 03:52:11, kalman wrote:
> this code should be using registar_.RemoveAll()

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:495:
args->Append(zoom_settings);
On 2014/04/09 03:52:11, kalman wrote:
> a model will be getting generated for OnZoomChange as well, you shouldn't need
> to interact with the Value API.

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:515: if (type ==
content::NOTIFICATION_NAV_ENTRY_COMMITTED) {
My code is gone from here now too. Is it worth changing the existing code to use
a switch?

On 2014/04/09 03:52:11, kalman wrote:
> bleh this should be a switch. whatevs.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:527:
content::Source<WebContents>(contents));
Not using notifications anymore.

On 2014/04/09 03:52:11, kalman wrote:
> you presumably need to unregister for tab zoom notifications here too, but
this
> should just call UnregisterForTabNotifications.

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

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:619: browser());
On 2014/04/09 03:52:11, kalman wrote:
> it seems clearer to me to actually run the get/set calls after initialising
each
> function, rather than after all are initialized.
> 
> same elsewhere.

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:656: new TabsSetZoomFunction());
Done. Wow, you were right, that does look much better!

On 2014/04/09 03:52:11, kalman wrote:
> perhaps you should have a helper function which gets/sets the zoom[settings]
via
> these extension function calls. then you don't need so much boilerplate in
these
> higher-level functionality tests.
> 
> would make these tests much easier to read.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:769: }
On 2014/04/09 03:52:11, kalman wrote:
> also test the chrome:// URL stuff.

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:770: 
Thanks!

On 2014/04/09 03:52:11, kalman wrote:
> otherwise: great tests!

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_function_test_utils.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_function_test_utils.cc:276: return
RunFunction(function, args, browser, NONE);
On 2014/04/09 03:52:11, kalman wrote:
> listing NONE in your tests doesn't seem like a big deal particularly if you
pull
> out the actual function calls into separate functions.
> 
> (i.e. not a strong case for making changes to this file)

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:125: } extension_info_;
Because:

- These are the only parts of the extension the zoom bubble needs.
- The IconImage needs to be stored on its own anyways (since it is not just
trivially obtained from the Extension).
- This makes it more clear exactly what the zoom_bubble_view is using from the
extension, and why.

On 2014/04/09 03:52:11, kalman wrote:
> why not just hold onto a scoped_refptr<const Extension> directly?

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:135: // Image button in
the zoom bubble that will show the |extension_icon_| image
Okay, will move out to separate patch.

On 2014/04/09 03:52:11, kalman wrote:
> so I got about this far in the views review before realising: you should split
> this part of the review out, too. you don't want to be blocked on UI concerns
> from checking code in.

https://codereview.chromium.org/225093019/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/tabs.json:84: "description": "Zoom changes only
take effect in this tab, and zoom changes in other tabs will not affect the
zooming of this tab. Also, <code>per-tab</code> zoom changes are reset on
navigation; navigating a tab will always load pages with their
<code>per-origin</code> zoom factors."
Thanks!

On 2014/04/09 03:52:11, kalman wrote:
> great documentation

https://codereview.chromium.org/225093019/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/tabs.json:1069: {"type": "number", "name":
"newZoomFactor"},
On 2014/04/09 03:52:11, kalman wrote:
> use an object here, not parameters, like
> 
> "parameters": [{
>   "type": "object",
>   "properties": [
>     { "type": "integer", "name": "tabId", "mimimum": 0 },
>     { "type": "number", "name" ..... }
>     {}
>   ]
> }]
>     

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/tabs/basics/events.js (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/tabs/basics/events.js:170:
assertEq(zoomSettings.scope, "per-origin");
On 2014/04/09 03:52:11, kalman wrote:
> reverse argument order of the 2 checks above.

Done.

[not at google - send to devlin, 2014-04-11 15:04:24]

long tail

you'll need an owner for the chrome/browser/ui changes so I didn't look at those
too closely. some of them seem to relate to the actual UI changes you were
making?

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1999:
scoped_refptr<TabsSetZoomFunction> this_ref = this;
On 2014/04/11 03:01:02, Paul Meyer wrote:
> I believe that the scoped_ptr is necessary to preserve the lifetime of
> TabsSetZoomFunction. This was the recommended way to set up the callback in
the
> documentation in callback.h.
> 
> On 2014/04/09 03:52:11, kalman wrote:
> > I don't think this makes any difference. just pass |this| into the
base::Bind
> > call.
> 

I looked at the documentation in callback.h. It references patterns like

scoped_refptr<> foo = new Foo();

which isn't the same as what you have here; |this| is not being new'd, it'll
already have a refcount:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

so you don't need to increment it here as well. The base::Bind(...this) will
hold onto a reference as well so |this| will stay alive until that callback is
destroyed.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_constants.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_constants.cc:84: const char
kNoCurrentTabError[] = "No current tab.";
On 2014/04/11 03:01:02, Paul Meyer wrote:
> See other comments about GetWebContents(). That being said, it seems that some
> of the other code uses kNoSelectedTabError to mean the same thing as there
being
> no active, so perhaps I could still get rid of this new error and use that
> instead. Is "selected" tab really the right term to use though? Is there any
> difference between a "selected" or "active" tab?
> 
> On 2014/04/09 03:52:11, kalman wrote:
> > like I said in the .cc file there must already be code which does this, so
you
> > shouldn't need this new error message.
> 

We seem to use those terms interchangeably, though "active" is more accurate;
"selected" is per-window I believe, whereas "active" is per-browser.

I reckon just re-use the extensions error message though, and we can clean up
these error messages along with the error conditions later.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:165:
content::Source<WebContents>(contents));
On 2014/04/11 03:01:02, Paul Meyer wrote:
> I agree, although I'm not using this notification anymore so I don't have any
> code here. Is it worth cleaning up the existing "Remove"s here?
> 
> On 2014/04/09 03:52:11, kalman wrote:
> > this code should be using registar_.RemoveAll()
> 

Probably not. Somebody can clean those up separately.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:515: if (type ==
content::NOTIFICATION_NAV_ENTRY_COMMITTED) {
On 2014/04/11 03:01:02, Paul Meyer wrote:
> My code is gone from here now too. Is it worth changing the existing code to
use
> a switch?
> 
> On 2014/04/09 03:52:11, kalman wrote:
> > bleh this should be a switch. whatevs.
> 

nah

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:64: #include
"content/public/browser/notification_service.h"
no longer needed

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:227: break;
let's just return an api::tabs::ZoomSettings (or a scoped_ptr<> if it's
disallow-copy; but it's such a tiny struct that copying it is not a big deal)
rather than having it as an out-param.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1966: error_ =
keys::kNoCurrentTabError;
as discussed, you could re-use the "no selected tab" error.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1981: } else if
(web_contents->GetURL().SchemeIs(content::kChromeUIScheme)) {
no else after return

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1982: error_ =
keys::kCannotZoomChromePagesError;
you should be able to reuse some sort of generic "not allowed' message here.
ideally not give away information like this being a chrome:// url.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1990: // Attempt to set the zoom
level with a callback.
comment doesn't add much

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2029: } else if
(web_contents->GetURL().SchemeIs(content::kChromeUIScheme)) {
no else after return

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2030: error_ =
keys::kCannotChangeChromePageZoomSettingsError;
same comment re chrome:// url

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2036:
api::tabs::ZoomSettings::SCOPE_PER_ORIGIN &&
hm is this the indentation that "git cl format" gives? dang.

ok here's an idea: can you do like "using api::tabs::ZoomSettings" or something
and cut out a lot of this "api::tabs::" stuff? then this can go on 1 line and
it'll look better. also much less visual noise elsewhere.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2084: api::tabs::ZoomSettings
zoom_settings;
ZoomSettings zoom_settings = ZoomModeToZoomSettings(zoom_mode);

you could also basically inline all of this stuff (into the results_ assignment)
too if you wanted.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.h (right):

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.h:270: // TODO(...) many other tabs
API functions use similar behavior. There should
alright you can put TOOD(kalman)

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:595: class
ZoomExtensionAPITestHelper {
a more convenient/common pattern is to have

class ExtensionTabsZoomTest : public ExtensionTabsTest {
  ...
}

then have tests be

TEST_F(ExtensionTabsZoomTest, MyTest) {
  SetTabZoom(...);
}

rather than 

TEST_F(ExtensionTabsTest, MyTest) {
  helper = ...;
  helper.SetTabZoom(...);
}

very simple transformation.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:624: 
ASSERT_TRUE(get_zoom_result.get());

though an even better pattern might be to have these functions return
testing::AssertionErrors and have

if (!get_zoom_result.get())
  return testing::AssertionFailure() << "no result";
if (!get_zoom_result->GetAsDouble())
  return testing::AssertionFailure() << "result was not a double";

then callers do

ASSERT_TRUE(RunGetZoom(..));

lifting error handling up to the callers is usually a good thing.

even better would be to use RunFunctionAndGetError and return a
testing::AssertionFailure() if there was an error, too.

much the same comments apply elsewhere in this file.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:636: if (scope) {
instead, can you do like

std::string args;
if (scope) {
  args = base::StringPrintf(...);
} else {
  args = base::StringPrintf(...);
}
utils::RunFunction(function.get(), args, browser_,..);

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:683: void
RunSetZoomSettingsExpectError(int tab_id,
ideally you'd just have a single general function RunGetZoomSettings(..) which
accepted an error. the test caller decides whether to ignore the error or assert
something about it.

it might not work out with the structure of these test utilities though, not
sure.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:706: static const char
kNewTestTabArgs[] = "about:blank";
there isn't any advantage having this as "static". the "about:blank" will be
stored as data so won't be copied.

likewise the other places you have "static".

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller.cc:60: const extensions::Extension*
ZoomController::last_extension() const {
I can't find where this function, or anything referencing "last_extension", is
used?

I was looking because it seems like an odd pattern to be holding onto something
like this.

presumably it has something to do with the UI changes you were making? in which
case you should at least pull that into that separate CL; but I would hope that
you can factor this such that you don't need last_extension (e.g. the UI code
holds onto it itself if it needs).

[paulmeyer, 2014-04-11 19:09:48]

I have addressed your last set of comments, though I am not sure if it is worth
continuing the review at this point before the major changes have been done as
per moving most of the functionality out of content/.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1999:
scoped_refptr<TabsSetZoomFunction> this_ref = this;
On 2014/04/11 15:04:25, kalman wrote:
> On 2014/04/11 03:01:02, Paul Meyer wrote:
> > I believe that the scoped_ptr is necessary to preserve the lifetime of
> > TabsSetZoomFunction. This was the recommended way to set up the callback in
> the
> > documentation in callback.h.
> > 
> > On 2014/04/09 03:52:11, kalman wrote:
> > > I don't think this makes any difference. just pass |this| into the
> base::Bind
> > > call.
> > 
> 
> I looked at the documentation in callback.h. It references patterns like
> 
> scoped_refptr<> foo = new Foo();
> 
> which isn't the same as what you have here; |this| is not being new'd, it'll
> already have a refcount:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> so you don't need to increment it here as well. The base::Bind(...this) will
> hold onto a reference as well so |this| will stay alive until that callback is
> destroyed.

Done.

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_constants.cc (right):

https://codereview.chromium.org/225093019/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_constants.cc:84: const char
kNoCurrentTabError[] = "No current tab.";
On 2014/04/11 15:04:25, kalman wrote:
> On 2014/04/11 03:01:02, Paul Meyer wrote:
> > See other comments about GetWebContents(). That being said, it seems that
some
> > of the other code uses kNoSelectedTabError to mean the same thing as there
> being
> > no active, so perhaps I could still get rid of this new error and use that
> > instead. Is "selected" tab really the right term to use though? Is there any
> > difference between a "selected" or "active" tab?
> > 
> > On 2014/04/09 03:52:11, kalman wrote:
> > > like I said in the .cc file there must already be code which does this, so
> you
> > > shouldn't need this new error message.
> > 
> 
> We seem to use those terms interchangeably, though "active" is more accurate;
> "selected" is per-window I believe, whereas "active" is per-browser.
> 
> I reckon just re-use the extensions error message though, and we can clean up
> these error messages along with the error conditions later.

Done.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:64: #include
"content/public/browser/notification_service.h"
On 2014/04/11 15:04:25, kalman wrote:
> no longer needed

Done.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:227: break;
Yeah, it is disallow-copy, and being scoped_ptr isn't enough to fix that because
it still needs to be copied in tabs_event_router before being passed into
Create, so I think it's still better to leave it as an out-param.

On 2014/04/11 15:04:25, kalman wrote:
> let's just return an api::tabs::ZoomSettings (or a scoped_ptr<> if it's
> disallow-copy; but it's such a tiny struct that copying it is not a big deal)
> rather than having it as an out-param.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1966: error_ =
keys::kNoCurrentTabError;
On 2014/04/11 15:04:25, kalman wrote:
> as discussed, you could re-use the "no selected tab" error.

Done.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1981: } else if
(web_contents->GetURL().SchemeIs(content::kChromeUIScheme)) {
On 2014/04/11 15:04:25, kalman wrote:
> no else after return

Done.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1982: error_ =
keys::kCannotZoomChromePagesError;
There aren't any existing errors that general, though if you'd like I could
simply remove the mention of chrome:// from mine.

On 2014/04/11 15:04:25, kalman wrote:
> you should be able to reuse some sort of generic "not allowed' message here.
> ideally not give away information like this being a chrome:// url.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1990: // Attempt to set the zoom
level with a callback.
On 2014/04/11 15:04:25, kalman wrote:
> comment doesn't add much

Done.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2029: } else if
(web_contents->GetURL().SchemeIs(content::kChromeUIScheme)) {
On 2014/04/11 15:04:25, kalman wrote:
> no else after return

Done.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2030: error_ =
keys::kCannotChangeChromePageZoomSettingsError;
See above.

On 2014/04/11 15:04:25, kalman wrote:
> same comment re chrome:// url

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2036:
api::tabs::ZoomSettings::SCOPE_PER_ORIGIN &&
On 2014/04/11 15:04:25, kalman wrote:
> hm is this the indentation that "git cl format" gives? dang.
> 
> ok here's an idea: can you do like "using api::tabs::ZoomSettings" or
something
> and cut out a lot of this "api::tabs::" stuff? then this can go on 1 line and
> it'll look better. also much less visual noise elsewhere.

Done. This does clean it up a lot.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:2084: api::tabs::ZoomSettings
zoom_settings;
On 2014/04/11 15:04:25, kalman wrote:
> ZoomSettings zoom_settings = ZoomModeToZoomSettings(zoom_mode);
> 
> you could also basically inline all of this stuff (into the results_
assignment)
> too if you wanted.

Done. I personally like separating lines when possible, especially when
combining would put them over the 80-char limit.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_test.cc (right):

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:595: class
ZoomExtensionAPITestHelper {
On 2014/04/11 15:04:25, kalman wrote:
> a more convenient/common pattern is to have
> 
> class ExtensionTabsZoomTest : public ExtensionTabsTest {
>   ...
> }
> 
> then have tests be
> 
> TEST_F(ExtensionTabsZoomTest, MyTest) {
>   SetTabZoom(...);
> }
> 
> rather than 
> 
> TEST_F(ExtensionTabsTest, MyTest) {
>   helper = ...;
>   helper.SetTabZoom(...);
> }
> 
> very simple transformation.

Done.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:624: 
On 2014/04/11 15:04:25, kalman wrote:
> ASSERT_TRUE(get_zoom_result.get());
> 
> though an even better pattern might be to have these functions return
> testing::AssertionErrors and have
> 
> if (!get_zoom_result.get())
>   return testing::AssertionFailure() << "no result";
> if (!get_zoom_result->GetAsDouble())
>   return testing::AssertionFailure() << "result was not a double";
> 
> then callers do
> 
> ASSERT_TRUE(RunGetZoom(..));
> 
> lifting error handling up to the callers is usually a good thing.
> 
> even better would be to use RunFunctionAndGetError and return a
> testing::AssertionFailure() if there was an error, too.
> 
> much the same comments apply elsewhere in this file.

Done.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:636: if (scope) {
On 2014/04/11 15:04:25, kalman wrote:
> instead, can you do like
> 
> std::string args;
> if (scope) {
>   args = base::StringPrintf(...);
> } else {
>   args = base::StringPrintf(...);
> }
> utils::RunFunction(function.get(), args, browser_,..);

Done.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:683: void
RunSetZoomSettingsExpectError(int tab_id,
Yes, the structure doesn't allow for that.

On 2014/04/11 15:04:25, kalman wrote:
> ideally you'd just have a single general function RunGetZoomSettings(..) which
> accepted an error. the test caller decides whether to ignore the error or
assert
> something about it.
> 
> it might not work out with the structure of these test utilities though, not
> sure.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_test.cc:706: static const char
kNewTestTabArgs[] = "about:blank";
On 2014/04/11 15:04:25, kalman wrote:
> there isn't any advantage having this as "static". the "about:blank" will be
> stored as data so won't be copied.
> 
> likewise the other places you have "static".

Done.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller.cc:60: const extensions::Extension*
ZoomController::last_extension() const {
This is something that is going to change because of the content changes
anyways, so I won't worry about it now.

On 2014/04/11 15:04:25, kalman wrote:
> I can't find where this function, or anything referencing "last_extension", is
> used?
> 
> I was looking because it seems like an odd pattern to be holding onto
something
> like this.
> 
> presumably it has something to do with the UI changes you were making? in
which
> case you should at least pull that into that separate CL; but I would hope
that
> you can factor this such that you don't need last_extension (e.g. the UI code
> holds onto it itself if it needs).

[not at google - send to devlin, 2014-04-11 20:37:01]

lgtm for what it's worth.

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/225093019/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1982: error_ =
keys::kCannotZoomChromePagesError;
On 2014/04/11 19:09:49, Paul Meyer wrote:
> There aren't any existing errors that general, though if you'd like I could
> simply remove the mention of chrome:// from mine.
> 
> On 2014/04/11 15:04:25, kalman wrote:
> > you should be able to reuse some sort of generic "not allowed' message here.
> > ideally not give away information like this being a chrome:// url.
> 

Ok, looks like we do have a precedent here in manifest_constants.h; let's just
use that:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

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

https://codereview.chromium.org/225093019/diff/110001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:597: void Init() {
no need for a separate Init() method. Just override SetUp(), though don't forget
to call ExtensionTabsTest::SetUp() after you override it.

[raymes, 2014-04-24 06:50:08]

Hey Fady,
Do you think this will land sometime soon?
Thanks!

[wjmaclean, 2014-04-29 20:13:58]

*** Closing this issue, moving review to
https://codereview.chromium.org/232773011/