Zoom Extension API (chrome)

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.

Based on code from https://codereview.chromium.org/232773011/, with various
fixes and rebased against changes in https://codereview.chromium.org/287093002/

Depends on changes in https://codereview.chromium.org/302603012/

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

[wjmaclean, 2014-06-05 17:55:34]

This work is getting fairly mature, so I've removed the [WIP] from the
description, and would like to get some review feedback. Thanks in advance!

[raymes, 2014-06-10 04:17:25]

On 2014/06/05 17:55:34, wjmaclean wrote:
> This work is getting fairly mature, so I've removed the [WIP] from the
> description, and would like to get some review feedback. Thanks in advance!

Any update on this review? :)

[not at google - send to devlin, 2014-06-10 19:21:24]

I remember that the original zoom patch got tied up on the content side. has
this all been resolved now?

[wjmaclean, 2014-06-10 20:02:15]

On 2014/06/10 19:21:24, kalman wrote:
> I remember that the original zoom patch got tied up on the content side. has
> this all been resolved now?

Still working on the content/ review, but we have two CLs that end-to-end work
now, which we didn't originally have, so things are moving along ...

[wjmaclean, 2014-06-12 17:54:10]

This version (Patch Set 5) moves the book keeping for manual mode into
ZoomController. This creates one issue not covered in this CL, namely that UI
elements like the wrench menu register callbacks directly with HostZoomMap, and
so do not see the changes to manual mode levels.

I would like to propose the following solution (which I will start working on
immediately, so if there's opposition to the idea, please let me know ASAP).

I think we should create a wrapper singleton (possibly associated with
ZoomController) that receives all callbacks from HostZoomMap and relays them, as
well as adding in manual mode callbacks initiated by the various
ZoomControllers.

This solves the problem of having to parse messages from potentially many
ZoomControllers (consumers like the wrench menu shouldn't incur the overhead of
subscribing to every new ZoomController that comes along).

Consumers of ZoomLevelChangedCallback messages in chrome/ will subscribe to the
wrapper, and it in turn subscribes to the HostZoomMap callbacks. This way the
manual mode message can be injected, and the consumers needn't change. (We could
consider this as a sort of decorator for HostZoomMap, though we could simplify
by limiting it to the callback structure.)

Let me know if this seems reasonable.

[Fady Samuel, 2014-06-12 19:38:58]

A quick initial look. I'm probably not the best reviewer for this part of the
code anyway. Does navigation behavior match the API doc's spec?

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:1332: //  TODO(wjmaclean)
Should we talk to HostZoomMap directly instead?
I don't think so. WebViews should always be in isolated mode btw. It should not
be the case that zooming a tab is independent of zooming a webview and vice
versa.

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

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller.cc:36: const
std::string&)>(&ZoomController::UpdateState),
Funky cast. Is this necessary?

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller.cc:112: std::string host =
Move this to the else clause?

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller.cc:144: std::string host =
net::GetHostOrSpecFromURL(url);
Move this and its dependencies to the case kZoomModeDefault?

[wjmaclean, 2014-06-12 19:42:22]

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:1332: //  TODO(wjmaclean)
Should we talk to HostZoomMap directly instead?
On 2014/06/12 19:38:58, Fady Samuel wrote:
> I don't think so. WebViews should always be in isolated mode btw. It should
not
> be the case that zooming a tab is independent of zooming a webview and vice
> versa.

Sorry about this ... this comment died (in my mind ;-) ) a long time ago, and
should have been removed. Agreed, should not be talking directly to HostZoomMap
in this case.

[wjmaclean, 2014-06-12 19:45:25]

Sorry for spamming with multiple comments here, but I realized they could be
addressed soon as opposed to later.

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

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller.cc:36: const
std::string&)>(&ZoomController::UpdateState),
On 2014/06/12 19:38:58, Fady Samuel wrote:
> Funky cast. Is this necessary?

Not sure ... I inherited this chuck of code and haven't changed it. I'll
simplify if possible.

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller.cc:112: std::string host =
On 2014/06/12 19:38:58, Fady Samuel wrote:
> Move this to the else clause?

Done.

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller.cc:144: std::string host =
net::GetHostOrSpecFromURL(url);
On 2014/06/12 19:38:58, Fady Samuel wrote:
> Move this and its dependencies to the case kZoomModeDefault?

Done.

[Fady Samuel, 2014-06-12 19:46:57]

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/chrome_pa...
File chrome/browser/chrome_page_zoom.cc (right):

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/chrome_pa...
chrome/browser/chrome_page_zoom.cc:79: // TODO(wjmaclean) Should we be talking
to HostZoomMap directly instead?
Remove comment?

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/chrome_pa...
chrome/browser/chrome_page_zoom.cc:98: // TODO(wjmaclean) Should we be talking
to HostZoomMap directly instead?
Remove comment.

https://codereview.chromium.org/301733006/diff/80001/chrome/browser/chrome_pa...
chrome/browser/chrome_page_zoom.cc:114: // TODO(wjmaclean) Should we be talking
to HostZoomMap directly instead?
Remove comment.

[not at google - send to devlin, 2014-06-13 15:47:41]

I'm not going to have time to get to this expediently, so passing the baton to
Devlin.

[wjmaclean, 2014-06-13 15:48:56]

On 2014/06/13 15:47:41, kalman wrote:
> I'm not going to have time to get to this expediently, so passing the baton to
> Devlin.

Ok ... thanks for the heads up. I expect to have a new CL posted shortly.

[Devlin, 2014-06-13 16:26:41]

On 2014/06/13 15:48:56, wjmaclean wrote:
> On 2014/06/13 15:47:41, kalman wrote:
> > I'm not going to have time to get to this expediently, so passing the baton
to
> > Devlin.
> 
> Ok ... thanks for the heads up. I expect to have a new CL posted shortly.

By a new CL, do you mean another, separate patch, or a new version of this one? 
(Wondering if I should review now or wait for an update addressing Fady's
comments.)

[wjmaclean, 2014-06-13 17:19:11]

On 2014/06/13 16:26:41, Devlin wrote:
> On 2014/06/13 15:48:56, wjmaclean wrote:
> > On 2014/06/13 15:47:41, kalman wrote:
> > > I'm not going to have time to get to this expediently, so passing the
baton
> to
> > > Devlin.
> > 
> > Ok ... thanks for the heads up. I expect to have a new CL posted shortly.
> 
> By a new CL, do you mean another, separate patch, or a new version of this
one? 
> (Wondering if I should review now or wait for an update addressing Fady's
> comments.)

I mean an update to this one. I'm trying to sort out a technical difficulty
incurred by moving the book-keeping for manual mode zoom levels out of host zoom
map and into zoom controller, while still making sure that observers of zoom
change levels (like wrench menu) are notified of manual changes ...

[wjmaclean, 2014-06-13 20:28:47]

PTAL.

Revised to account for moving manual zoom levels out of HostZoomMap and into
ZoomController.

[wjmaclean, 2014-06-16 13:32:55]

Now that https://codereview.chromium.org/302603012/ has landed, this is the last
step (hopefully) in getting the tabs API up and running. Please take a look.

[Devlin, 2014-06-16 19:04:50]

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1777: bool
ZoomAPIFunction::GetWebContents(int* tab_id,
Why not content::WebContents* GetWebContents(int* tab_id), which returns NULL if
it's not found?

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1793: error_ =
keys::kNoCurrentWindowError;
It's odd that a function called GetWebContents also sets the error.  That should
be mentioned somewhere.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1812: if
(web_contents->GetURL().SchemeIs(content::kChromeUIScheme)) {
WebContents::GetURL() is deprecated.  See web_contents.h for more details.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1813: error_ =
keys::kCannotZoomChromePagesError;
This looks like it should probably be using PermissionsData::CanAccessPage(). 
Is there a reason to have the more lax behavior here?

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1838: return false;
nit: no braces around single-line conditionals.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1859: if
(web_contents->GetURL().SchemeIs(content::kChromeUIScheme)) {
Same comments as above.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1863: ZoomController*
zoom_controller =
nit: Move this down where it's used.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1887: break;
nit: Remove trailing break.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1895: break;
nit: Remove trailing break.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_constants.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_constants.cc:87: "'per-origin' in
'automatic' mode.";
nit: Please be consistent in use of single or double quotes.  It looks like most
places here, we use double (including in the errors you add above), so let's
stick with that.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:577:
DispatchEvent(profile, tabs::OnZoomChange::kEventName,
nit: One argument per line (unless all fit on one line).

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

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:596: // Provides helper
functions for testing the zoom extension API.
nit: Please change this to something like "Tester class for the tabs.zoom* api
functions."

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:597: class ExtensionTabsZoomTest
: public ExtensionTabsTest {
It seems odd that we subclass an empty subclass.  Maybe just inherit
InProcessBrowserTest.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:599: void Init() {
Why not do this in Test::SetUp()?

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:604: void RunSetZoom(int tab_id,
double zoom_factor) {
This is personal opinion, but for large tester classes like this, I prefer to
use full chrome style, e.g. no inlining methods.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:612: browser(),
extension_function_test_utils::NONE);
nit: One argument per line.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:613:
content::RunAllPendingInMessageLoop();
RunFuction() should be synchronous.  Is there another reason we need
RunAllPendingInMessageLoop?

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:617: bool RunGetZoom(int tab_id,
double* zoom_factor) {
Please make this return a testing::AssertionResult (otherwise you lose the
benefit of returning assertion failures on 629 and 631).  Then you'll need to
return a testing::AssertionSuccess() on line 633.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:652:
utils::RunFunction(set_zoom_settings_function.get(), args,
nit: One per line.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:658: bool RunGetZoomSettings(int
tab_id, std::string* mode, std::string* scope) {
Return testing::AssertionResult.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:658: bool RunGetZoomSettings(int
tab_id, std::string* mode, std::string* scope) {
Please DCHECK string and scope.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:681: double zoom_factor,
Both of the ExpectError functions are only called once; could we just inline
them in the test?

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:719: scoped_refptr<Extension>
extension_;
Thanks for making this private :) - lots of tests just protected all their data,
and it can lead to odd failures.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:731: content::OpenURLParams
params(GURL(kNewTestTabArgs), content::Referrer(),
one per line.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:738: RunSetZoom(tab_id, 0.8);
Please const this, i.e.
const double kZoomLevel = 0.8;

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:741: // Test
chrome.tabs.getZoom().
Maybe also test this before it's set, just for completeness.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:777: EXPECT_FLOAT_EQ(1.f,
maybe ASSERT_FLOAT_EQ this before any set zooms are called.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:32: #include
"content/public/browser/host_zoom_map.h"
Do we still need this include?

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1858: // TODO(wjmaclean) Is there any way we could
hook the TabsEventRouter up
Yes, this is bad.  There might be a good way to attach the router directly, and,
failing that, this type of forwarding should go on a class like TabHelper.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:683: 
nit: no newline between overridden methods from the same class.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:1810: namespace {
nit: The function is long enough that there should be a newline between the
namespace and the function definition.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:1818: }
bracket indentation is off.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:86: if (extension) {
nit: No brackets.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:189: int default_size =
default_extension_icon_image.width();
Is this really the only way to get the default size?

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:203: default_size =
action_icon.map().begin()->first;
doesn't this mean this is no longer the default_size?  (Just thinking it should
be renamed).

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

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:16: #include
"ui/views/controls/button/image_button.h"
Why do you need the full include?

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:110: struct
ExtensionInfo {
Appropriate name, but please don't add another ExtensionInfo - we already have
too many :)

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:125: } extension_info_;
We don't usually do struct declarations like this.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:36:
base::Bind(static_cast<void(ZoomController::*)(
Is this all just because you added the other method with the shadowing bool? 
Why not just make that one have a different name, rather than put this in?

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:61: return zoom >
default_zoom_level_.GetValue() ? IDR_ZOOM_PLUS : IDR_ZOOM_MINUS;
nit: inline double zoom's value.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:64: const extensions::Extension*
ZoomController::last_extension() const {
why not defined in the header?

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:111: // change completes..
Either 1 '.' or 3.  Not two. :)

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:128: // notify the observer below.
Re-write this before submitting.
Address this.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:165: // TODO(wjmaclean) How to convert
this call?
Address this.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:229: if (!extensions_.empty()) {
This seems odd for a few reasons.

1.  If we are certain that the last extension to make a request is the last to
update the state, why not set last_extension() when the call was made?
2.  extensions_ can be full of NULLs (above).
3.  UpdateState() will be called every time we navigate a page - but should we
really be setting last_extension_ in that case?  It seems more like this should
only happen in OnZoomLevelChanged...

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:259: void
ZoomController::ZoomCallback(
When is this used?

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:16: #include
"extensions/common/extension.h"
Why full include?

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:39: kZoomModeDefault,
Typically, we nest these enums in a class (unless they have their own file).

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:67: // Returns a boolean flag
indicating success (true) or failure (false).
Shorter: "Returns true on success."

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:102: const base::Callback<void(void)>&
callback);
base::Callback<void(void)> = base::Closure

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
no (c) in 2014.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.h (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:24: void OnZoomLevelChanged(const
content::HostZoomMap::ZoomLevelChange& change);
comment.

https://codereview.chromium.org/301733006/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/common/extension...
chrome/common/extensions/api/tabs.json:70: "description": "Disables all zooming
in the tab. The tab will revert to default (100%) zoom, and all attempted zoom
changes will be ignored."
Is this right?  What if the user has the settings to auto-zoom to 110%?

https://codereview.chromium.org/301733006/diff/100001/extensions/browser/exte...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/301733006/diff/100001/extensions/browser/exte...
extensions/browser/extension_function_histogram_value.h:852: TABS_SETZOOM,
These should have entries in histograms.xml.

[Devlin, 2014-06-17 19:10:20]

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1813: error_ =
keys::kCannotZoomChromePagesError;
On 2014/06/16 19:04:47, Devlin wrote:
> This looks like it should probably be using PermissionsData::CanAccessPage(). 
> Is there a reason to have the more lax behavior here?

Going back to this - I thought developers would need explicit access to the page
for the zoom API.  Talking to Ben, it seems this isn't the case.  If we want to
keep in any restriction, we should keep the chrome check and add in
ExtensionsClient::IsScriptableUrl().  If we don't require any checks (Ben thinks
this is what was decided, but isn't certain - and it's not documented in the
linked design doc), then we can take out the checks entirely.

[wjmaclean, 2014-06-18 19:03:56]

Here's a revised version, I think I've managed to address all your comments -
PTAL

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1777: bool
ZoomAPIFunction::GetWebContents(int* tab_id,
On 2014/06/16 19:04:47, Devlin wrote:
> Why not content::WebContents* GetWebContents(int* tab_id), which returns NULL
if
> it's not found?

I inherited this code from Paul Meyer, and I suspect he was trying mimic the
style of similar utility functions in the class. I've converted it to return a
pointer as requested.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1793: error_ =
keys::kNoCurrentWindowError;
On 2014/06/16 19:04:47, Devlin wrote:
> It's odd that a function called GetWebContents also sets the error.  That
should
> be mentioned somewhere.

I've mentioned in the .h file that this function may set the value of error_.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1812: if
(web_contents->GetURL().SchemeIs(content::kChromeUIScheme)) {
On 2014/06/16 19:04:47, Devlin wrote:
> WebContents::GetURL() is deprecated.  See web_contents.h for more details.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1813: error_ =
keys::kCannotZoomChromePagesError;
On 2014/06/16 19:04:47, Devlin wrote:
> This looks like it should probably be using PermissionsData::CanAccessPage(). 
> Is there a reason to have the more lax behavior here?

Possibly. It seems to mean that any extension wanting to be able to zoom a page
must either explicitly request permission for the domain(s) (which seems
restrictive since we just want to zoom) or specify <all_urls>, which may have
other baggage.

I've included CanAccessPage() in this version of the CL (please let me know if
I've done it correctly, as I wasn't sure how to handle the top_frame_url part).

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1838: return false;
On 2014/06/16 19:04:47, Devlin wrote:
> nit: no braces around single-line conditionals.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1859: if
(web_contents->GetURL().SchemeIs(content::kChromeUIScheme)) {
On 2014/06/16 19:04:47, Devlin wrote:
> Same comments as above.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1863: ZoomController*
zoom_controller =
On 2014/06/16 19:04:47, Devlin wrote:
> nit: Move this down where it's used.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1887: break;
On 2014/06/16 19:04:47, Devlin wrote:
> nit: Remove trailing break.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1895: break;
On 2014/06/16 19:04:47, Devlin wrote:
> nit: Remove trailing break.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_constants.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_constants.cc:87: "'per-origin' in
'automatic' mode.";
On 2014/06/16 19:04:47, Devlin wrote:
> nit: Please be consistent in use of single or double quotes.  It looks like
most
> places here, we use double (including in the errors you add above), so let's
> stick with that.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_event_router.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_event_router.cc:577:
DispatchEvent(profile, tabs::OnZoomChange::kEventName,
On 2014/06/16 19:04:47, Devlin wrote:
> nit: One argument per line (unless all fit on one line).

Done.

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

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:596: // Provides helper
functions for testing the zoom extension API.
On 2014/06/16 19:04:48, Devlin wrote:
> nit: Please change this to something like "Tester class for the tabs.zoom* api
> functions."

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:597: class ExtensionTabsZoomTest
: public ExtensionTabsTest {
On 2014/06/16 19:04:49, Devlin wrote:
> It seems odd that we subclass an empty subclass.  Maybe just inherit
> InProcessBrowserTest.

Agreed. However, I assume other tests in this same file that also do it do so
because someday it may not be empty. There's no harm to it I can see so I will
leave it as is.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:599: void Init() {
On 2014/06/16 19:04:48, Devlin wrote:
> Why not do this in Test::SetUp()?

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:604: void RunSetZoom(int tab_id,
double zoom_factor) {
On 2014/06/16 19:04:48, Devlin wrote:
> This is personal opinion, but for large tester classes like this, I prefer to
> use full chrome style, e.g. no inlining methods.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:612: browser(),
extension_function_test_utils::NONE);
On 2014/06/16 19:04:48, Devlin wrote:
> nit: One argument per line.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:613:
content::RunAllPendingInMessageLoop();
On 2014/06/16 19:04:48, Devlin wrote:
> RunFuction() should be synchronous.  Is there another reason we need
> RunAllPendingInMessageLoop?

Removed. This was added during a debugging phase, and never removed, so I'm glad
you spotted it.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:617: bool RunGetZoom(int tab_id,
double* zoom_factor) {
On 2014/06/16 19:04:48, Devlin wrote:
> Please make this return a testing::AssertionResult (otherwise you lose the
> benefit of returning assertion failures on 629 and 631).  Then you'll need to
> return a testing::AssertionSuccess() on line 633.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:652:
utils::RunFunction(set_zoom_settings_function.get(), args,
On 2014/06/16 19:04:48, Devlin wrote:
> nit: One per line.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:658: bool RunGetZoomSettings(int
tab_id, std::string* mode, std::string* scope) {
On 2014/06/16 19:04:48, Devlin wrote:
> Return testing::AssertionResult.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:658: bool RunGetZoomSettings(int
tab_id, std::string* mode, std::string* scope) {
On 2014/06/16 19:04:48, Devlin wrote:
> Return testing::AssertionResult.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:681: double zoom_factor,
On 2014/06/16 19:04:48, Devlin wrote:
> Both of the ExpectError functions are only called once; could we just inline
> them in the test?

Seemed too good to just use once ;-) Have added more test runs ...

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:719: scoped_refptr<Extension>
extension_;
On 2014/06/16 19:04:48, Devlin wrote:
> Thanks for making this private :) - lots of tests just protected all their
data,
> and it can lead to odd failures.

Paul did this, so I won't claim to take credit for it, but good nonetheless.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:731: content::OpenURLParams
params(GURL(kNewTestTabArgs), content::Referrer(),
On 2014/06/16 19:04:48, Devlin wrote:
> one per line.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:738: RunSetZoom(tab_id, 0.8);
On 2014/06/16 19:04:48, Devlin wrote:
> Please const this, i.e.
> const double kZoomLevel = 0.8;

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:741: // Test
chrome.tabs.getZoom().
On 2014/06/16 19:04:48, Devlin wrote:
> Maybe also test this before it's set, just for completeness.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:777: EXPECT_FLOAT_EQ(1.f,
On 2014/06/16 19:04:48, Devlin wrote:
> maybe ASSERT_FLOAT_EQ this before any set zooms are called.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/web_view_guest.cc:32: #include
"content/public/browser/host_zoom_map.h"
On 2014/06/16 19:04:49, Devlin wrote:
> Do we still need this include?

No, removed.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1858: // TODO(wjmaclean) Is there any way we could
hook the TabsEventRouter up
On 2014/06/16 19:04:49, Devlin wrote:
> Yes, this is bad.  There might be a good way to attach the router directly,
and,
> failing that, this type of forwarding should go on a class like TabHelper.

Done.

This is a bit messy, since Browser is handling the OnZoomChange event, but we
want the OnZoomChangeInitiated event to go to TabsEventRouter. ZoomObserver
provides default (empty) implementations, so presumably it's ok for a derived
class to choose which events it wants to handle, but then ZoomController only
has support for a single observer.

I think it's reasonable to modify ZoomController to allow it to service multiple
observers, and then the only slightly-ugly piece is hooking up the
TabsEventRouter, which is still managed by Browser.

Let me know if this looks ok.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:683: 
On 2014/06/16 19:04:49, Devlin wrote:
> nit: no newline between overridden methods from the same class.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:1810: namespace {
On 2014/06/16 19:04:49, Devlin wrote:
> nit: The function is long enough that there should be a newline between the
> namespace and the function definition.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:1818: }
On 2014/06/16 19:04:49, Devlin wrote:
> bracket indentation is off.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:86: if (extension) {
On 2014/06/16 19:04:49, Devlin wrote:
> nit: No brackets.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:189: int default_size =
default_extension_icon_image.width();
On 2014/06/16 19:04:49, Devlin wrote:
> Is this really the only way to get the default size?

Done.

It took some hunting to find gfx::kFaviconSize !

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:203: default_size =
action_icon.map().begin()->first;
On 2014/06/16 19:04:49, Devlin wrote:
> doesn't this mean this is no longer the default_size?  (Just thinking it
should
> be renamed).

Done.

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

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:16: #include
"ui/views/controls/button/image_button.h"
On 2014/06/16 19:04:49, Devlin wrote:
> Why do you need the full include?

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:110: struct
ExtensionInfo {
On 2014/06/16 19:04:49, Devlin wrote:
> Appropriate name, but please don't add another ExtensionInfo - we already have
> too many :)

Done.

Let me know if the new name, "ZoomBubbleExtensionInfo" is ok.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:125: } extension_info_;
On 2014/06/16 19:04:49, Devlin wrote:
> We don't usually do struct declarations like this.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:36:
base::Bind(static_cast<void(ZoomController::*)(
On 2014/06/16 19:04:49, Devlin wrote:
> Is this all just because you added the other method with the shadowing bool? 
> Why not just make that one have a different name, rather than put this in?

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:61: return zoom >
default_zoom_level_.GetValue() ? IDR_ZOOM_PLUS : IDR_ZOOM_MINUS;
On 2014/06/16 19:04:50, Devlin wrote:
> nit: inline double zoom's value.

Done.

Let me know if this is what you were looking for here.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:64: const extensions::Extension*
ZoomController::last_extension() const {
On 2014/06/16 19:04:49, Devlin wrote:
> why not defined in the header?

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:111: // change completes..
On 2014/06/16 19:04:50, Devlin wrote:
> Either 1 '.' or 3.  Not two. :)

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:128: // notify the observer below.
Re-write this before submitting.
On 2014/06/16 19:04:49, Devlin wrote:
> Address this.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:165: // TODO(wjmaclean) How to convert
this call?
On 2014/06/16 19:04:49, Devlin wrote:
> Address this.

Done.

I'm not sure what I had in mind when I wrote this TODO, but the code as it
stands seems ok. I did clean up the comment a bit.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:229: if (!extensions_.empty()) {
On 2014/06/16 19:04:50, Devlin wrote:
> This seems odd for a few reasons.
> 
> 1.  If we are certain that the last extension to make a request is the last to
> update the state, why not set last_extension() when the call was made?
> 2.  extensions_ can be full of NULLs (above).
> 3.  UpdateState() will be called every time we navigate a page - but should we
> really be setting last_extension_ in that case?  It seems more like this
should
> only happen in OnZoomLevelChanged...

Agreed, the code as it stands in this CL is convoluted and confusing. I have
re-thought it and simplified it considerably.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:259: void
ZoomController::ZoomCallback(
On 2014/06/16 19:04:50, Devlin wrote:
> When is this used?

Removed.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:16: #include
"extensions/common/extension.h"
On 2014/06/16 19:04:50, Devlin wrote:
> Why full include?

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:39: kZoomModeDefault,
On 2014/06/16 19:04:50, Devlin wrote:
> Typically, we nest these enums in a class (unless they have their own file).

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:67: // Returns a boolean flag
indicating success (true) or failure (false).
On 2014/06/16 19:04:50, Devlin wrote:
> Shorter: "Returns true on success."

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:102: const base::Callback<void(void)>&
callback);
On 2014/06/16 19:04:50, Devlin wrote:
> base::Callback<void(void)> = base::Closure

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.cc (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/06/16 19:04:50, Devlin wrote:
> no (c) in 2014.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.h (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
Removed (c) here as well.

https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:24: void OnZoomLevelChanged(const
content::HostZoomMap::ZoomLevelChange& change);
On 2014/06/16 19:04:50, Devlin wrote:
> comment.

Done.

https://codereview.chromium.org/301733006/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/tabs.json (right):

https://codereview.chromium.org/301733006/diff/100001/chrome/common/extension...
chrome/common/extensions/api/tabs.json:70: "description": "Disables all zooming
in the tab. The tab will revert to default (100%) zoom, and all attempted zoom
changes will be ignored."
On 2014/06/16 19:04:50, Devlin wrote:
> Is this right?  What if the user has the settings to auto-zoom to 110%?

Then in the current design the auto-zoom settings are overridden.

My understanding of this is that the caller does not want this tab being zoom,
and so it is displayed at 100% and only 100%.

https://codereview.chromium.org/301733006/diff/100001/extensions/browser/exte...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/301733006/diff/100001/extensions/browser/exte...
extensions/browser/extension_function_histogram_value.h:852: TABS_SETZOOM,
On 2014/06/16 19:04:50, Devlin wrote:
> These should have entries in histograms.xml.

Done.

[Devlin, 2014-06-19 21:15:30]

Getting closer!  Mostly nits this time around.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/chrome_p...
File chrome/browser/chrome_page_zoom.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/chrome_p...
chrome/browser/chrome_page_zoom.cc:79: // TODO(wjmaclean) Should we be talking
to HostZoomMap directly instead?
Remove comment.  I think it's fine either way, but we should be consistent.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1777: content::WebContents*
ZoomAPIFunction::GetWebContents(int* tab_id) {
nit: more common is to pass in -1 for "no tab id", so please do that for
consistency.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1784: NULL,
Please document anonymous nulls.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1808: GURL
url(web_contents->GetURL());
GetURL is still deprecated. :)

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1810:
!ExtensionsClient::Get()->IsScriptableURL(url, NULL)) {
Please document anonymous NULL.
...ExtensionsClient::Get()->IsScriptableURL(url, NULL /* ignore error */)) {

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_event_router.h (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_event_router.h:76: //
chrome::ZoomObserver.
nit: just ZoomObserver, since it's not in the chrome:: namespace.

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

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:619: // Runs
chrome.tabs.setZoomSettings(), expecting an error.
nit: newline between functions

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:631: void
ExtensionTabsZoomTest::SetUpOnMainThread() {
Call the super::SetUpOnMainThread() first.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:635: void
ExtensionTabsZoomTest::RunSetZoom(int tab_id, double zoom_factor) {
This should probably return the bool result from utils::RunFunction(), which
should then be checked at the call site.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:669: void
ExtensionTabsZoomTest::RunSetZoomSettings(int tab_id,
again, probably return a bool here.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:676: std:: string args;
no space after std::.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:688:
content::RunAllPendingInMessageLoop();
missed this one.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:717: void
ExtensionTabsZoomTest::RunSetZoomExpectError(int tab_id,
May as well have this return |error| directly.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:731: void
ExtensionTabsZoomTest::RunSetZoomSettingsExpectError(int tab_id,
ditto.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:796: content::OpenURLParams
params_A(GURL(kNewTestTabArgsA), content::Referrer(),
nit: one arg per line.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:861: content::OpenURLParams
params(GURL(kNewTestTabArgs), content::Referrer(),
nit: one arg per line

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:892: content::OpenURLParams
params(GURL(kNewTestTabArgs), content::Referrer(),
nit: one arg per line.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:893: NEW_FOREGROUND_TAB,
If these params are always the same in all these tests (it looks like they are?)
it might help readability to just have a function

content::OpenURLParams GetOpenParams() {
  return content::OpenURLParams(...);
}

Even though it doesn't really do much, it shortens the line count, and, more
importantly, hides the pesky implementation details when looking at the test.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:899: int bogus_id = tab_id + 1;
nit: Just in case, maybe tab_id + 100.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:908:
IN_PROC_BROWSER_TEST_F(ExtensionTabsZoomTest, CannotZoomChromeURL) {
Can we combine these last two tests into one CannotZoomInvalidTab?

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:2202:
extensions::TabsWindowsAPI::Get(web_contents->GetBrowserContext())
This still seems wrong to have here.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:73:
zoom_bubble_->Refresh();
Can we make this more readable and just put an early return here?

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:269: std::string
url("chrome://extensions?id=");
nit: stringprintf will be slightly faster than this.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:273:
content::PAGE_TRANSITION_FROM_API);
I don't know if "FROM_API" is strictly correct here.  I'll let a views person
who knows more determine it, though.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:798: if (selected_tab) {
nit: no brackets around 1-line if statements

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:64: if (zoom_mode_ == kZoomModeManual)
nit: Ternary if, please.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:78: return
SetZoomLevelByExtension(zoom_level, null_extension_pointer);
nit: You can just do
return SetZoomLevelByExtension(zoom_level, scoped_refptr<const
extensions::Extension>()), right?  I think that'll make it just as clear that
it's a null ptr. :)

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:88: if (zoom_mode_ == kZoomModeManual)
{
Do we not care about the extension in manual mode?

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:99: for (size_t i = 0; i <
observers_.size(); ++i)
From observer_list:
FOR_EACH_OBSERVER(Observer,
                  observers_,
                  OnZoomChangeInitiated(web_contents(), old_zoom_level,
zoom_level, zoom_mode_));

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:131:
observers_[i]->OnZoomChangeInitiated(
Same FOR_EACH_OBSERVER

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:133: last_extension_ = NULL;
To me, it seems that if we only have last_extension() valid for the observer
notification, we should just send it out as part of the observe function.  That
way, no one thinks that last_extension() will be valid at any other time.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:245: for (size_t i = 0; i <
observers_.size(); ++i)
FOR_EACH_OBSERVER

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:8: #include <queue>
Do we still need this?

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:66: void AddObserver(ZoomObserver*
observer) { observers_.push_back(observer); }
A "remove observer" would also be good :)

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:102: void
UpdateStateIncludingTemporary(const std::string& host,
Document the difference between this and UpdateState() (i.e., say what a
"temporary zoom" is in this context).

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:119: std::vector<ZoomObserver*>
observers_;
Take a look at ObserverList (base/observer_list.h)

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.h (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:16: class ZoomEventManager : public
base::SupportsUserData::Data {
This whole class seems a little odd to me.  Is this just to marshal all the
OnZoomLevelChanged() into a per-browser-context interface?  Is there any (good)
way of just having the subscribers to this class observe ZoomController
directly?

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:35: };
nit: newline here

[wjmaclean, 2014-06-20 22:01:34]

Here's another revision, I think I've addressed all the new comments.

PTAL

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/chrome_p...
File chrome/browser/chrome_page_zoom.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/chrome_p...
chrome/browser/chrome_page_zoom.cc:79: // TODO(wjmaclean) Should we be talking
to HostZoomMap directly instead?
On 2014/06/19 21:15:28, Devlin wrote:
> Remove comment.  I think it's fine either way, but we should be consistent.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1777: content::WebContents*
ZoomAPIFunction::GetWebContents(int* tab_id) {
On 2014/06/19 21:15:28, Devlin wrote:
> nit: more common is to pass in -1 for "no tab id", so please do that for
> consistency.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1784: NULL,
On 2014/06/19 21:15:28, Devlin wrote:
> Please document anonymous nulls.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1808: GURL
url(web_contents->GetURL());
On 2014/06/19 21:15:28, Devlin wrote:
> GetURL is still deprecated. :)

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1810:
!ExtensionsClient::Get()->IsScriptableURL(url, NULL)) {
On 2014/06/19 21:15:28, Devlin wrote:
> Please document anonymous NULL.
> ...ExtensionsClient::Get()->IsScriptableURL(url, NULL /* ignore error */)) {

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_event_router.h (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_event_router.h:76: //
chrome::ZoomObserver.
On 2014/06/19 21:15:28, Devlin wrote:
> nit: just ZoomObserver, since it's not in the chrome:: namespace.

Done.

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

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:619: // Runs
chrome.tabs.setZoomSettings(), expecting an error.
On 2014/06/19 21:15:28, Devlin wrote:
> nit: newline between functions

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:631: void
ExtensionTabsZoomTest::SetUpOnMainThread() {
On 2014/06/19 21:15:29, Devlin wrote:
> Call the super::SetUpOnMainThread() first.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:635: void
ExtensionTabsZoomTest::RunSetZoom(int tab_id, double zoom_factor) {
On 2014/06/19 21:15:28, Devlin wrote:
> This should probably return the bool result from utils::RunFunction(), which
> should then be checked at the call site.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:669: void
ExtensionTabsZoomTest::RunSetZoomSettings(int tab_id,
On 2014/06/19 21:15:28, Devlin wrote:
> again, probably return a bool here.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:676: std:: string args;
On 2014/06/19 21:15:28, Devlin wrote:
> no space after std::.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:688:
content::RunAllPendingInMessageLoop();
On 2014/06/19 21:15:28, Devlin wrote:
> missed this one.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:717: void
ExtensionTabsZoomTest::RunSetZoomExpectError(int tab_id,
On 2014/06/19 21:15:28, Devlin wrote:
> May as well have this return |error| directly.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:731: void
ExtensionTabsZoomTest::RunSetZoomSettingsExpectError(int tab_id,
On 2014/06/19 21:15:29, Devlin wrote:
> ditto.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:796: content::OpenURLParams
params_A(GURL(kNewTestTabArgsA), content::Referrer(),
On 2014/06/19 21:15:29, Devlin wrote:
> nit: one arg per line.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:861: content::OpenURLParams
params(GURL(kNewTestTabArgs), content::Referrer(),
On 2014/06/19 21:15:29, Devlin wrote:
> nit: one arg per line

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:892: content::OpenURLParams
params(GURL(kNewTestTabArgs), content::Referrer(),
On 2014/06/19 21:15:29, Devlin wrote:
> nit: one arg per line.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:893: NEW_FOREGROUND_TAB,
On 2014/06/19 21:15:29, Devlin wrote:
> If these params are always the same in all these tests (it looks like they
are?)
> it might help readability to just have a function
> 
> content::OpenURLParams GetOpenParams() {
>   return content::OpenURLParams(...);
> }
> 
> Even though it doesn't really do much, it shortens the line count, and, more
> importantly, hides the pesky implementation details when looking at the test.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:899: int bogus_id = tab_id + 1;
On 2014/06/19 21:15:28, Devlin wrote:
> nit: Just in case, maybe tab_id + 100.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:908:
IN_PROC_BROWSER_TEST_F(ExtensionTabsZoomTest, CannotZoomChromeURL) {
On 2014/06/19 21:15:29, Devlin wrote:
> Can we combine these last two tests into one CannotZoomInvalidTab?

Ok, although I'm not sure I understand the benefit.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:2202:
extensions::TabsWindowsAPI::Get(web_contents->GetBrowserContext())
On 2014/06/19 21:15:29, Devlin wrote:
> This still seems wrong to have here.

It does turn out to be simpler to do this in the ZoomController constructor,
though then it begs the question of whether the old way (line 2195) shouldn't
move there too.

Let me know.

It also turns out that this function is sometimes called with delegate = NULL in
order to clear the delegates, but that doesn't work with continually calling
AddObserver. I've split this out into two separate functions that use a clearer
Add/Remove model.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:73:
zoom_bubble_->Refresh();
On 2014/06/19 21:15:29, Devlin wrote:
> Can we make this more readable and just put an early return here?

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:269: std::string
url("chrome://extensions?id=");
On 2014/06/19 21:15:29, Devlin wrote:
> nit: stringprintf will be slightly faster than this.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:273:
content::PAGE_TRANSITION_FROM_API);
On 2014/06/19 21:15:29, Devlin wrote:
> I don't know if "FROM_API" is strictly correct here.  I'll let a views person
> who knows more determine it, though.

OK, can you suggest someone, and/or add them to the reviewer list?

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:798: if (selected_tab) {
On 2014/06/19 21:15:29, Devlin wrote:
> nit: no brackets around 1-line if statements

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:64: if (zoom_mode_ == kZoomModeManual)
On 2014/06/19 21:15:30, Devlin wrote:
> nit: Ternary if, please.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:78: return
SetZoomLevelByExtension(zoom_level, null_extension_pointer);
On 2014/06/19 21:15:29, Devlin wrote:
> nit: You can just do
> return SetZoomLevelByExtension(zoom_level, scoped_refptr<const
> extensions::Extension>()), right?  I think that'll make it just as clear that
> it's a null ptr. :)

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:88: if (zoom_mode_ == kZoomModeManual)
{
On 2014/06/19 21:15:30, Devlin wrote:
> Do we not care about the extension in manual mode?

It's probably true that consumers of OnZoomChanged events should be notified
here, and they may in turn query the zoom controller with regards to the
extension information. I've modified the code to reflect this.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:99: for (size_t i = 0; i <
observers_.size(); ++i)
On 2014/06/19 21:15:30, Devlin wrote:
> From observer_list:
> FOR_EACH_OBSERVER(Observer,
>                   observers_,
>                   OnZoomChangeInitiated(web_contents(), old_zoom_level,
> zoom_level, zoom_mode_));

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:131:
observers_[i]->OnZoomChangeInitiated(
On 2014/06/19 21:15:29, Devlin wrote:
> Same FOR_EACH_OBSERVER

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:133: last_extension_ = NULL;
On 2014/06/19 21:15:29, Devlin wrote:
> To me, it seems that if we only have last_extension() valid for the observer
> notification, we should just send it out as part of the observe function. 
That
> way, no one thinks that last_extension() will be valid at any other time.

The only consumer of this information at present is the zoom_bubble_view, and it
gets notified from UpdateState, so we still need to temporarily store the
last_extension information via SetZoomLevelByExtension().

Given that there may be other consumers of the observe messages that have no
interest in the extension, and perhaps shouldn't even know about it, I'm not
sure if we should send it or not.

When last_extension() is not valid, it returns NULL, so I'm not sure if this is
really a potential source of confusion.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:245: for (size_t i = 0; i <
observers_.size(); ++i)
On 2014/06/19 21:15:29, Devlin wrote:
> FOR_EACH_OBSERVER

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:8: #include <queue>
On 2014/06/19 21:15:30, Devlin wrote:
> Do we still need this?

Good catch, no. Removed.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:66: void AddObserver(ZoomObserver*
observer) { observers_.push_back(observer); }
On 2014/06/19 21:15:30, Devlin wrote:
> A "remove observer" would also be good :)

OK, I don't mind adding that. The reason I haven't added it before now is that
there are no callers for it at present, and I've generally been told not to add
unused interface in the past.

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:102: void
UpdateStateIncludingTemporary(const std::string& host,
On 2014/06/19 21:15:30, Devlin wrote:
> Document the difference between this and UpdateState() (i.e., say what a
> "temporary zoom" is in this context).

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:119: std::vector<ZoomObserver*>
observers_;
On 2014/06/19 21:15:30, Devlin wrote:
> Take a look at ObserverList (base/observer_list.h)

Done.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.h (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:16: class ZoomEventManager : public
base::SupportsUserData::Data {
On 2014/06/19 21:15:30, Devlin wrote:
> This whole class seems a little odd to me.  Is this just to marshal all the
> OnZoomLevelChanged() into a per-browser-context interface?  Is there any
(good)
> way of just having the subscribers to this class observe ZoomController
> directly?

The observers for this are per-browser-context as you note, and so is this.
ZoomControllers come and go with each WebContents. The observers would need to
be notified every time a new ZoomController was created, so I think this makes
more sense.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:35: };
On 2014/06/19 21:15:30, Devlin wrote:
> nit: newline here

Done.

[wjmaclean, 2014-06-23 18:40:48]

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:854: 
Oops, I'll remove this line.

[Devlin, 2014-06-23 22:56:08]

This version is mostly lg, so if you wanted to have some other reviewers start
looking more closely at c/b/ui and c/b/guest_view, I don't anticipate requesting
any more large changes.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:133: last_extension_ = NULL;
On 2014/06/20 22:01:33, wjmaclean wrote:
> On 2014/06/19 21:15:29, Devlin wrote:
> > To me, it seems that if we only have last_extension() valid for the observer
> > notification, we should just send it out as part of the observe function. 
> That
> > way, no one thinks that last_extension() will be valid at any other time.
> 
> The only consumer of this information at present is the zoom_bubble_view, and
it
> gets notified from UpdateState, so we still need to temporarily store the
> last_extension information via SetZoomLevelByExtension().
> 
> Given that there may be other consumers of the observe messages that have no
> interest in the extension, and perhaps shouldn't even know about it, I'm not
> sure if we should send it or not.
> 
> When last_extension() is not valid, it returns NULL, so I'm not sure if this
is
> really a potential source of confusion.

I'm confused.  It looks like last_extension_ is being set to NULL in this
function after the observers are notified.  If the only consumer is notified
from UpdateState(), how will last_extension() ever be non-null?  (I'm sure I
must be missing something, but I can't figure out what it is...)

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

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:759: content::OpenURLParams
ExtensionTabsZoomTest::GetOpenParams(
nit: this could actually go in the anonymous namespace

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:775: const char
kNewTestTabArgs[] = "about:blank";
nit: turns out we actually have a kAboutBlankURL in url/url_constants.h that
should be used.

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:777: content::WebContents*
web_contents = browser()->OpenURL(params);
Sorry for not bringing this up sooner, but it just occurred to me that this
should probably be using ui_test_utils::NavigateToURL(), which blocks for the
navigation (and is actually sometimes necessary for the urls to be valid).

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:901: }
nit: If there's no reason for the extra scoping, go ahead and remove it.

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:910: fprintf(stderr, "wjm:
error=[%s]\n", error.c_str());
Remove

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:105:
ZoomEventManager::GetForBrowserContext(web_contents()->GetBrowserContext())
nit: not your code, but since we have a private |browser_context_| cached, would
you mind updating these web_contents()->GetBrowserContext() to browser_context_?

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:66: observers_.AddObserver(observer);
nit: Even though these are pretty small, we usually only put near-constant time
functions as inlined in the .h.

[wjmaclean, 2014-06-24 15:38:16]

+ sky@ for chrome/browser/ui
fsamuel@ for chrome/browser/guest_view

I've addressed all the comments, please see my replies below.

I've made a slight change as to how icons are loaded in the zoom bubble code,
since it appears that (at least in tests) the browser action object may be
absent.

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/140001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:133: last_extension_ = NULL;
On 2014/06/23 22:56:07, Devlin wrote:
> On 2014/06/20 22:01:33, wjmaclean wrote:
> > On 2014/06/19 21:15:29, Devlin wrote:
> > > To me, it seems that if we only have last_extension() valid for the
observer
> > > notification, we should just send it out as part of the observe function. 
> > That
> > > way, no one thinks that last_extension() will be valid at any other time.
> > 
> > The only consumer of this information at present is the zoom_bubble_view,
and
> it
> > gets notified from UpdateState, so we still need to temporarily store the
> > last_extension information via SetZoomLevelByExtension().
> > 
> > Given that there may be other consumers of the observe messages that have no
> > interest in the extension, and perhaps shouldn't even know about it, I'm not
> > sure if we should send it or not.
> > 
> > When last_extension() is not valid, it returns NULL, so I'm not sure if this
> is
> > really a potential source of confusion.
> 
> I'm confused.  It looks like last_extension_ is being set to NULL in this
> function after the observers are notified.  If the only consumer is notified
> from UpdateState(), how will last_extension() ever be non-null?  (I'm sure I
> must be missing something, but I can't figure out what it is...)

Update state gets called from the observer notifications sent out by
HostZoomMap, so SetZoomLevelByExtension() is still on the stack, and
last_extension_ is still valid.

I've made the assumption here, based on observation and reading the code, that
these callbacks are all running synchronously.

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

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:759: content::OpenURLParams
ExtensionTabsZoomTest::GetOpenParams(
On 2014/06/23 22:56:08, Devlin wrote:
> nit: this could actually go in the anonymous namespace

Done.

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:775: const char
kNewTestTabArgs[] = "about:blank";
On 2014/06/23 22:56:08, Devlin wrote:
> nit: turns out we actually have a kAboutBlankURL in url/url_constants.h that
> should be used.

Done.

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:777: content::WebContents*
web_contents = browser()->OpenURL(params);
On 2014/06/23 22:56:08, Devlin wrote:
> Sorry for not bringing this up sooner, but it just occurred to me that this
> should probably be using ui_test_utils::NavigateToURL(), which blocks for the
> navigation (and is actually sometimes necessary for the urls to be valid).

Done. I assume it's OK if I use OpenUrlAndWaitForLoad()? This gives me back a
WebContents* directly and so is convenient.

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:901: }
On 2014/06/23 22:56:07, Devlin wrote:
> nit: If there's no reason for the extra scoping, go ahead and remove it.

Done.

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_test.cc:910: fprintf(stderr, "wjm:
error=[%s]\n", error.c_str());
On 2014/06/23 22:56:08, Devlin wrote:
> Remove

Done.

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:105:
ZoomEventManager::GetForBrowserContext(web_contents()->GetBrowserContext())
On 2014/06/23 22:56:08, Devlin wrote:
> nit: not your code, but since we have a private |browser_context_| cached,
would
> you mind updating these web_contents()->GetBrowserContext() to
browser_context_?

Done.

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/200001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:66: observers_.AddObserver(observer);
On 2014/06/23 22:56:08, Devlin wrote:
> nit: Even though these are pretty small, we usually only put near-constant
time
> functions as inlined in the .h.

Done.

[sky, 2014-06-24 16:23:42]

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:758: void SetAsObserver(content::WebContents*
web_contents);
The name and this function and description imply something very generic. Yet the
implementation is just about zoom. Since the implementation of these functions
is just a one liner I don't see any value in them.

Maybe I'm missing something, but AFAICT the only changes you should need in
browser.cc are to change 2195 (old) to something like:

if (delegate)
  ZoomController::FromWebContents(web_contents)->AddObserver(delegate);
else
  ZoomController::FromWebContents(web_contents)->RemoveObserver(this);

The latter is slightly unfortunate. It could be clarified by making the function
take a boolean instead of a delegate.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:295: base::Unretained(this)));
What is the difference between what this function does and that on 287?

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:74:
zoom_bubble_->Refresh();
nit: spacing.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:95: if
(!anchor_to_view) {
nit: no {}

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:249:
image_button_->SetImage(views::Button::STATE_NORMAL,
Because the image for this comes in async does the layout end up bouncing
around?

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:281: std::string url =
base::StringPrintf("chrome://extensions?id=");
Why the base::StringPrintf here?

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:32: zoom_mode_(kZoomModeDefault),
Can you initialize zoom_level_ too (I realize you do on 41 already, but better
to do explicitly here in case code following changes).

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:48: AddObserver(
Is this the common pattern for extension related stuff? I would expect the
TabsWindowAPI to manage who and when it wants to observer rather than other
places. In other words, I would have expected this call to exist in
TabsWindowsAPI and not here.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:69: observers_.AddObserver(observer);
nit: spacing (same on 73).

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:45: kZoomModeDefault,
ZOOM_MODE_DEFAULT is the right name and style for this.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.h (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:16: class ZoomEventManager : public
base::SupportsUserData::Data {
Add description.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:18: static ZoomEventManager*
GetForBrowserContext(
nit: constructor/destructor should be first.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:35: };
DISALLOW...

[wjmaclean, 2014-06-24 18:04:07]

I've done my best to address all the comments in this round, and have provided
some answers to questions asked.

PTAL

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:758: void SetAsObserver(content::WebContents*
web_contents);
On 2014/06/24 16:23:41, sky wrote:
> The name and this function and description imply something very generic. Yet
the
> implementation is just about zoom. Since the implementation of these functions
> is just a one liner I don't see any value in them.
> 
> Maybe I'm missing something, but AFAICT the only changes you should need in
> browser.cc are to change 2195 (old) to something like:
> 
> if (delegate)
>   ZoomController::FromWebContents(web_contents)->AddObserver(delegate);
> else
>   ZoomController::FromWebContents(web_contents)->RemoveObserver(this);
> 
> The latter is slightly unfortunate. It could be clarified by making the
function
> take a boolean instead of a delegate.

It was the latter case that caused me to create the separate functions, but if
it's ok to change to using a boolean then that makes it clear that the delegate
is only ever 'this' and we are setting/clearing.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:295: base::Unretained(this)));
On 2014/06/24 16:23:41, sky wrote:
> What is the difference between what this function does and that on 287?

Nothing, except the source of the events. I originally tried to create a model
where all events from HostZoomMap flowed through ZoomEventManager so that
observers only ever saw one source of events, but since there are no guarantees
on the order in which UserData objects are destroyed I ran into problems
guaranteeing the order of destruction, and this led to DCHECKs firing when
HostZoomMap was destroyed with subscriptions still active.

If there's a way to guarantee that ZoomEventManager is destroyed first (e.g. if
UserData is destroyed in the reverse order than it is created), then we could go
back to that model.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:74:
zoom_bubble_->Refresh();
On 2014/06/24 16:23:41, sky wrote:
> nit: spacing.

Done.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:95: if
(!anchor_to_view) {
On 2014/06/24 16:23:41, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:249:
image_button_->SetImage(views::Button::STATE_NORMAL,
On 2014/06/24 16:23:41, sky wrote:
> Because the image for this comes in async does the layout end up bouncing
> around?

I've checked in both debug and release builds on Linux, and it doesn't seem to
bounce around (though for an extension that has no default icon, causing us to
fall back to an existing browser  action icon, you do see the relayout). Is that
OK?

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:281: std::string url =
base::StringPrintf("chrome://extensions?id=");
On 2014/06/24 16:23:41, sky wrote:
> Why the base::StringPrintf here?

The call to base::StringPrintf() was a request in an earlier review round.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:32: zoom_mode_(kZoomModeDefault),
On 2014/06/24 16:23:42, sky wrote:
> Can you initialize zoom_level_ too (I realize you do on 41 already, but better
> to do explicitly here in case code following changes).

Sure!

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:48: AddObserver(
On 2014/06/24 16:23:42, sky wrote:
> Is this the common pattern for extension related stuff? I would expect the
> TabsWindowAPI to manage who and when it wants to observer rather than other
> places. In other words, I would have expected this call to exist in
> TabsWindowsAPI and not here.

I'm new to extension work, so I can't properly answer if this is common usage or
not. The reason I chose to put it here is that there are multiple creation
points for ZoomControllers, not all extension related, and this seemed like the
right place to ensure that the tabs event router was connected.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:69: observers_.AddObserver(observer);
On 2014/06/24 16:23:41, sky wrote:
> nit: spacing (same on 73).

Done.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:45: kZoomModeDefault,
On 2014/06/24 16:23:42, sky wrote:
> ZOOM_MODE_DEFAULT is the right name and style for this.

Done.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.h (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:16: class ZoomEventManager : public
base::SupportsUserData::Data {
On 2014/06/24 16:23:42, sky wrote:
> Add description.

Done.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:18: static ZoomEventManager*
GetForBrowserContext(
On 2014/06/24 16:23:42, sky wrote:
> nit: constructor/destructor should be first.

Done.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:35: };
On 2014/06/24 16:23:42, sky wrote:
> DISALLOW...

Done.

[Devlin, 2014-06-24 18:11:15]

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:281: std::string url =
base::StringPrintf("chrome://extensions?id=");
On 2014/06/24 18:04:07, wjmaclean wrote:
> On 2014/06/24 16:23:41, sky wrote:
> > Why the base::StringPrintf here?
> 
> The call to base::StringPrintf() was a request in an earlier review round.

Sorry, should have been more clear.

std::string url =
    base::StringPrintf("chrome://extensions?id=%s", extension_info_.id);
or even
GURL url = GURL(base::StringPrintf(...));
or even
chrome::AddSelectedTabWithURL(browser,
                              GURL(base::StringPrintf(...)),
                              content::PAGE_TRANSITION_FROM_API);

[wjmaclean, 2014-06-24 18:25:05]

On 2014/06/24 18:11:15, Devlin wrote:
>
https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
> File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):
> 
>
https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
> chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:281: std::string url
=
> base::StringPrintf("chrome://extensions?id=");
> On 2014/06/24 18:04:07, wjmaclean wrote:
> > On 2014/06/24 16:23:41, sky wrote:
> > > Why the base::StringPrintf here?
> > 
> > The call to base::StringPrintf() was a request in an earlier review round.
> 
> Sorry, should have been more clear.
> 
> std::string url =
>     base::StringPrintf("chrome://extensions?id=%s", extension_info_.id);
> or even
> GURL url = GURL(base::StringPrintf(...));
> or even
> chrome::AddSelectedTabWithURL(browser,
>                               GURL(base::StringPrintf(...)),
>                               content::PAGE_TRANSITION_FROM_API);

Ahh! Ok, sorry, I missed the boat there. I'll update accordingly on the next 
patch. :-)

[sky, 2014-06-25 16:02:40]

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:249:
image_button_->SetImage(views::Button::STATE_NORMAL,
On 2014/06/24 18:04:07, wjmaclean wrote:
> On 2014/06/24 16:23:41, sky wrote:
> > Because the image for this comes in async does the layout end up bouncing
> > around?
> 
> I've checked in both debug and release builds on Linux, and it doesn't seem to
> bounce around (though for an extension that has no default icon, causing us to
> fall back to an existing browser  action icon, you do see the relayout). Is
that
> OK?

I was thinking you would force the image_button_ to the size of the resulting
image. That way no bouncing around.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:48: AddObserver(
On 2014/06/24 18:04:07, wjmaclean wrote:
> On 2014/06/24 16:23:42, sky wrote:
> > Is this the common pattern for extension related stuff? I would expect the
> > TabsWindowAPI to manage who and when it wants to observer rather than other
> > places. In other words, I would have expected this call to exist in
> > TabsWindowsAPI and not here.
> 
> I'm new to extension work, so I can't properly answer if this is common usage
or
> not. The reason I chose to put it here is that there are multiple creation
> points for ZoomControllers, not all extension related, and this seemed like
the
> right place to ensure that the tabs event router was connected.

Please get a reviewer of the extension side of things to review this. I think
the registration belongs in the extension side. See TabsEventRouter how it adds
itself as an observer when appropriate.

https://codereview.chromium.org/301733006/diff/260001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/301733006/diff/260001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:758: void SetAsDelegate(content::WebContents*
web_contents, bool set_delegate);
Document what set_delegate means.

[wjmaclean, 2014-06-25 17:21:30]

+ kalman@ for extensions (TabsEventRouter)

I've moved the TabsEventRouter registration as a ZoomObserver for ZoomController
into TabsEventRouter.

PTAL

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:249:
image_button_->SetImage(views::Button::STATE_NORMAL,
On 2014/06/25 16:02:39, sky wrote:
> On 2014/06/24 18:04:07, wjmaclean wrote:
> > On 2014/06/24 16:23:41, sky wrote:
> > > Because the image for this comes in async does the layout end up bouncing
> > > around?
> > 
> > I've checked in both debug and release builds on Linux, and it doesn't seem
to
> > bounce around (though for an extension that has no default icon, causing us
to
> > fall back to an existing browser  action icon, you do see the relayout). Is
> that
> > OK?
> 
> I was thinking you would force the image_button_ to the size of the resulting
> image. That way no bouncing around.

Ahh, Ok ... I didn't understand where your comment was leading. Done.

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/220001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:48: AddObserver(
On 2014/06/25 16:02:39, sky wrote:
> On 2014/06/24 18:04:07, wjmaclean wrote:
> > On 2014/06/24 16:23:42, sky wrote:
> > > Is this the common pattern for extension related stuff? I would expect the
> > > TabsWindowAPI to manage who and when it wants to observer rather than
other
> > > places. In other words, I would have expected this call to exist in
> > > TabsWindowsAPI and not here.
> > 
> > I'm new to extension work, so I can't properly answer if this is common
usage
> or
> > not. The reason I chose to put it here is that there are multiple creation
> > points for ZoomControllers, not all extension related, and this seemed like
> the
> > right place to ensure that the tabs event router was connected.
> 
> Please get a reviewer of the extension side of things to review this. I think
> the registration belongs in the extension side. See TabsEventRouter how it
adds
> itself as an observer when appropriate.

Ok, moved observer registration to TabsEventRouter.

https://codereview.chromium.org/301733006/diff/260001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/301733006/diff/260001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:758: void SetAsDelegate(content::WebContents*
web_contents, bool set_delegate);
On 2014/06/25 16:02:40, sky wrote:
> Document what set_delegate means.

Done.

[Devlin, 2014-06-25 17:37:13]

On 2014/06/25 17:21:30, wjmaclean wrote:
> + kalman@ for extensions (TabsEventRouter)
> 
> I've moved the TabsEventRouter registration as a ZoomObserver for
ZoomController
> into TabsEventRouter.

FYI, I'm an owner of extensions. :)  Thought I mentioned the observer
registration back in patch set 6?

(https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...)

[wjmaclean, 2014-06-25 17:48:10]

On 2014/06/25 17:37:13, Devlin wrote:
> On 2014/06/25 17:21:30, wjmaclean wrote:
> > + kalman@ for extensions (TabsEventRouter)
> > 
> > I've moved the TabsEventRouter registration as a ZoomObserver for
> ZoomController
> > into TabsEventRouter.
> 
> FYI, I'm an owner of extensions. :)  Thought I mentioned the observer
> registration back in patch set 6?
> 
>
(https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...)

Sorry about that Devlin ... I checked your profile on teams and didn't see
"extensions". Yes, you did mention the registration issue earlier, and I
responded to that comment by moving the registration into the ZoomController
constructor, but at Sky's request I've moved it again, now it's in
TabsEventRouter.

[Devlin, 2014-06-25 17:52:35]

On 2014/06/25 17:48:10, wjmaclean wrote:
> On 2014/06/25 17:37:13, Devlin wrote:
> > On 2014/06/25 17:21:30, wjmaclean wrote:
> > > + kalman@ for extensions (TabsEventRouter)
> > > 
> > > I've moved the TabsEventRouter registration as a ZoomObserver for
> > ZoomController
> > > into TabsEventRouter.
> > 
> > FYI, I'm an owner of extensions. :)  Thought I mentioned the observer
> > registration back in patch set 6?
> > 
> >
>
(https://codereview.chromium.org/301733006/diff/100001/chrome/browser/ui/brows...)
> 
> Sorry about that Devlin ... I checked your profile on teams and didn't see
> "extensions". Yes, you did mention the registration issue earlier, and I
> responded to that comment by moving the registration into the ZoomController
> constructor, but at Sky's request I've moved it again, now it's in
> TabsEventRouter.

No worries :)  the 'OWNERS' files (e.g., chrome/browser/extensions/OWNERS) are
typically a better and easier source for reviewers.

[sky, 2014-06-25 18:53:53]

Can you get a local owner for the changes to to chrome/browser/ui/zoom? I'm
adding dbeam as he is listed as an owner.

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:248:
image_button_->SetPreferredSize(
You need to do this when adding, otherwise the layout may bounce around.

[wjmaclean, 2014-06-25 19:01:31]

Sorry, did you mean for me to find a local owner in addition to dbeam@ ?

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:248:
image_button_->SetPreferredSize(
On 2014/06/25 18:53:53, sky wrote:
> You need to do this when adding, otherwise the layout may bounce around.

Thanks for this, I didn't know :-)

[wjmaclean, 2014-06-25 19:07:05]

+ jam@ for
  web_contents.h

+ mpearson@ for 
  tools/metrics,  
  extensions/browser/extension_function_histogram_value.h

[Mark P, 2014-06-25 19:28:29]

lgtm for
extensions/browser/extension_function_histogram_value.h
tools/metrics/histograms/histograms.xml

--mark

[sky, 2014-06-25 19:31:42]

On Wed, Jun 25, 2014 at 12:01 PM, <wjmaclean@chromium.org> wrote:

> Sorry, did you mean for me to find a local owner in addition to dbeam@ ?


dbeam should be enough.


>
>
>
> https://codereview.chromium.org/301733006/diff/270001/
> chrome/browser/ui/views/location_bar/zoom_bubble_view.cc
> File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):
>
> https://codereview.chromium.org/301733006/diff/270001/
> chrome/browser/ui/views/location_bar/zoom_bubble_view.cc#newcode248
> chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:248:
> image_button_->SetPreferredSize(
> On 2014/06/25 18:53:53, sky wrote:
>
>> You need to do this when adding, otherwise the layout may bounce
>>
> around.
>
> Thanks for this, I didn't know :-)
>
> https://codereview.chromium.org/301733006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-06-25 19:32:30]

On Wed, Jun 25, 2014 at 12:01 PM, <wjmaclean@chromium.org> wrote:

> Sorry, did you mean for me to find a local owner in addition to dbeam@ ?
>
>
>
> https://codereview.chromium.org/301733006/diff/270001/
> chrome/browser/ui/views/location_bar/zoom_bubble_view.cc
> File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):
>
> https://codereview.chromium.org/301733006/diff/270001/
> chrome/browser/ui/views/location_bar/zoom_bubble_view.cc#newcode248
> chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:248:
> image_button_->SetPreferredSize(
> On 2014/06/25 18:53:53, sky wrote:
>
>> You need to do this when adding, otherwise the layout may bounce
>>
> around.
>
> Thanks for this, I didn't know :-)
>

Ping again when you update this. That's the only thing holding up my
approval.

  -Scott

>
> https://codereview.chromium.org/301733006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-06-25 21:02:42]

https://codereview.chromium.org/301733006/diff/290001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:248:
image_button_->SetPreferredSize(
Now I'm slightly confused. As long as the image here is the same size as that
from Init, this isn't needed nor is 314. You only need to set the preferred size
initially if you have no image and then later get the image. That doesn't seem
to be the case though. Sorry for the run around.

[wjmaclean, 2014-06-25 21:12:19]

https://codereview.chromium.org/301733006/diff/290001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/290001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:248:
image_button_->SetPreferredSize(
On 2014/06/25 21:02:42, sky wrote:
> Now I'm slightly confused. As long as the image here is the same size as that
> from Init, this isn't needed nor is 314. You only need to set the preferred
size
> initially if you have no image and then later get the image. That doesn't seem
> to be the case though. Sorry for the run around.

By the time SetImage() is called, we do have a definite icon image in mind;
specifically its size *is* know at this point.

I'll remove these.

[Fady Samuel, 2014-06-25 21:17:59]

lgtm

[sky, 2014-06-25 23:35:34]

LGTM

[Dan Beam, 2014-06-26 03:40:03]

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:77: :
content::HostZoomMap::GetZoomLevel(web_contents());
return zoom_mode_ == ZOOM_MODE_MANUAL ?
    zoom_level_ : content::HostZoomMap::GetZoomLevel(web_contents());

?/: (operators) at the end

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:82: return
static_cast<int>(zoom_factor * 100 + 0.5);
nit: mention in a comment that this is rounding

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:88: scoped_refptr<const
extensions::Extension>());
can you use NULL instead of scoped_refptr<const extensions::Extension>()?

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:93: scoped_refptr<const
extensions::Extension> extension) {
nit: const-ref if possible/cheaper

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:193:
zoom_map->ClearTemporaryZoomLevel(render_process_id, render_view_id);
i'm confused, why are you settings the temporary zoom then clearing it right
after?

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:199: // in the other mode.
can you give an example of when this ^ would happen?

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:210: // zoom as before.
i'm confused, is there any other way you can put this?

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:43: //     to default (100%) zoom, and
all attempted zoom changes will be ignored.
nit: move these comments to just about each enum entry, e.g.

  // Default zoom behavior, i.e. zoom changes are handled automatically...
  ZOOM_MODE_DEFAULT,
  // Results in zoom changes being handled automatically,...
  ZOOM_MODE_ISOLATED

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.h (right):

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:19: class ZoomEventManager : public
base::SupportsUserData::Data {
why not ZoomObserver?

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:23: 
nit: doc comment (maybe when this would be NULL, if ever...)

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_event_router.h (right):

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_event_router.h:81:
ZoomController::ZoomMode zoom_mode) OVERRIDE;
^ why do you need to pass all this info along?  can't you get everything from
|web_contents|?

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.cc (right):

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.cc:10: static const char*
kBrowserZoomEventManager = "browser_zoom_event_manager";
nit: namespace { ... }, also char* -> char[] for less static initializers

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.cc:16: if (!manager) {
nit:

if (!context->GetUserData(kBrowserZoomEventManager))
  context->SetUserData(kBrowserZoomEventManager, new ZoomEventManager);

return context->GetUserData(kBrowserZoomEventManager);

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.h (right):

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:19: class ZoomEventManager : public
base::SupportsUserData::Data {
why do both ZoomEventManager and ZoomObserver need to exist?

[wjmaclean, 2014-06-26 15:30:31]

I've addressed dbeam@'s comments, PTAL.

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:77: :
content::HostZoomMap::GetZoomLevel(web_contents());
On 2014/06/26 03:40:02, Dan Beam wrote:
> return zoom_mode_ == ZOOM_MODE_MANUAL ?
>     zoom_level_ : content::HostZoomMap::GetZoomLevel(web_contents());
> 
> ?/: (operators) at the end

Dang Clang formatter ... done.

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:82: return
static_cast<int>(zoom_factor * 100 + 0.5);
On 2014/06/26 03:40:02, Dan Beam wrote:
> nit: mention in a comment that this is rounding

Done.

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:88: scoped_refptr<const
extensions::Extension>());
On 2014/06/26 03:40:02, Dan Beam wrote:
> can you use NULL instead of scoped_refptr<const extensions::Extension>()?

Done. The current form was requested in a previous review round.

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:93: scoped_refptr<const
extensions::Extension> extension) {
On 2014/06/26 03:40:02, Dan Beam wrote:
> nit: const-ref if possible/cheaper

Done.

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:193:
zoom_map->ClearTemporaryZoomLevel(render_process_id, render_view_id);
On 2014/06/26 03:40:02, Dan Beam wrote:
> i'm confused, why are you settings the temporary zoom then clearing it right
> after?

The call to SetTemporaryZoomLevel is required to force the render view to
actually re-paint at the zoom level specified for the content's origin, prior to
clearing the temporary level.

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:199: // in the other mode.
On 2014/06/26 03:40:02, Dan Beam wrote:
> can you give an example of when this ^ would happen?

This will happen when going from default mode to isolated? In the transition
Disabled->Isolated, we know the page is already at the default zoom level.

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:210: // zoom as before.
On 2014/06/26 03:40:02, Dan Beam wrote:
> i'm confused, is there any other way you can put this?

Done.

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:43: //     to default (100%) zoom, and
all attempted zoom changes will be ignored.
On 2014/06/26 03:40:02, Dan Beam wrote:
> nit: move these comments to just about each enum entry, e.g.
> 
>   // Default zoom behavior, i.e. zoom changes are handled automatically...
>   ZOOM_MODE_DEFAULT,
>   // Results in zoom changes being handled automatically,...
>   ZOOM_MODE_ISOLATED

Done.

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.h (right):

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:19: class ZoomEventManager : public
base::SupportsUserData::Data {
On 2014/06/26 03:40:02, Dan Beam wrote:
> why not ZoomObserver?

This object never subscribes to ZoomObserver events, nor does it implement the
ZoomObserver interface. Perhaps I don't understand your question? (Are you
asking about the naming of the class?)

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:23: 
On 2014/06/26 03:40:02, Dan Beam wrote:
> nit: doc comment (maybe when this would be NULL, if ever...)

Done.

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_event_router.h (right):

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_event_router.h:81:
ZoomController::ZoomMode zoom_mode) OVERRIDE;
On 2014/06/26 03:40:02, Dan Beam wrote:
> ^ why do you need to pass all this info along?  can't you get everything from
> |web_contents|?

This information is destined for extensions (via the Tabs API) that won't have
access to WebContents.

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.cc (right):

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.cc:10: static const char*
kBrowserZoomEventManager = "browser_zoom_event_manager";
On 2014/06/26 03:40:03, Dan Beam wrote:
> nit: namespace { ... }, also char* -> char[] for less static initializers

Done.

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.cc:16: if (!manager) {
On 2014/06/26 03:40:03, Dan Beam wrote:
> nit:
> 
> if (!context->GetUserData(kBrowserZoomEventManager))
>   context->SetUserData(kBrowserZoomEventManager, new ZoomEventManager);
> 
> return context->GetUserData(kBrowserZoomEventManager);

Done.

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.h (right):

https://codereview.chromium.org/301733006/diff/310001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.h:19: class ZoomEventManager : public
base::SupportsUserData::Data {
On 2014/06/26 03:40:03, Dan Beam wrote:
> why do both ZoomEventManager and ZoomObserver need to exist?

Observers (such as the wrench menu) need a single point of registration for
event subscriptions - it would be unreasonable to expect them to register with
every new ZoomController, especially since they would have no way of knowing
when a new ZoomController is created.

[jam, 2014-06-26 16:26:10]

content lgtm

[Dan Beam, 2014-06-26 22:16:38]

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/270001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:199: // in the other mode.
On 2014/06/26 15:30:30, wjmaclean wrote:
> On 2014/06/26 03:40:02, Dan Beam wrote:
> > can you give an example of when this ^ would happen?
> 
> This will happen when going from default mode to isolated? In the transition
> Disabled->Isolated, we know the page is already at the default zoom level.

I mean, what can a user do to trigger this?  Put it in a comment.

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:188: ResourceBundle& rb
= ResourceBundle::GetSharedInstance();
nit: ui::ResourceBundle

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:246: DCHECK_EQ(image,
extension_info_.icon_image.get()) <<
if this is always equal, why not just use |extension_info_.icon_image.get()| and
comment out /*image*/

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:247: "Unknown ImageIcon
update.";
nit: IconImage, also don't think this message is all that useful

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:278:
DCHECK(extension_info_.icon_image.get()) << "Invalid button press.";
nit: s/.get()// when using a scoped_ptr in a boolean context

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:112: ->OnZoomLevelChanged(change);
nit: -> at end of L111

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:122: web_contents(), old_zoom_level,
zoom_level, zoom_mode_));
^ shouldn't this happen *before* OnZoomChanged()?

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:144: net::GetHostOrSpecFromURL(entry ?
entry->GetURL() : GURL::EmptyGURL());
if there's no entry, shouldn't we just ask the web_contents() it's visible URL
instead?

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:145:
zoom_map->SetZoomLevelForHost(host, zoom_level);
should we be calling SetZoomLevelForHost() with an empty URL?

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:174: url = entry->GetURL();
same re: empty GURL or !entry handling

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:68: int GetZoomPercent() const;
nit: doc comment?  usually if something is StudlyCaps() it's doing more than a
dumb_unix_hacker_accessor(), so I'd be interested to know why these are
StudlyCaps in this case when reading the header.

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.cc (right):

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.cc:7: #include "base/bind.h"
^ why do you need bind.h?

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.cc:23: }
nit: {}

[wjmaclean, 2014-07-02 22:24:59]

PTAL

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:188: ResourceBundle& rb
= ResourceBundle::GetSharedInstance();
On 2014/06/26 22:16:37, Dan Beam wrote:
> nit: ui::ResourceBundle

Done.

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:246: DCHECK_EQ(image,
extension_info_.icon_image.get()) <<
On 2014/06/26 22:16:38, Dan Beam wrote:
> if this is always equal, why not just use |extension_info_.icon_image.get()|
and
> comment out /*image*/

I think Paul Meyer (who originally wrote this function) was guarding against the
possibility that it could be invoked through some potential but unknown pathway,
resulting in a mismatch.

Perhaps it would be safer just to use the incoming image, on the assumption that
it will always be the most up to date one?

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:247: "Unknown ImageIcon
update.";
On 2014/06/26 22:16:37, Dan Beam wrote:
> nit: IconImage, also don't think this message is all that useful

Done.

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:278:
DCHECK(extension_info_.icon_image.get()) << "Invalid button press.";
On 2014/06/26 22:16:37, Dan Beam wrote:
> nit: s/.get()// when using a scoped_ptr in a boolean context

Done.

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:112: ->OnZoomLevelChanged(change);
On 2014/06/26 22:16:38, Dan Beam wrote:
> nit: -> at end of L111

Done.

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:122: web_contents(), old_zoom_level,
zoom_level, zoom_mode_));
On 2014/06/26 22:16:38, Dan Beam wrote:
> ^ shouldn't this happen *before* OnZoomChanged()?

Hmm, this might just be an unfortunate choice of naming. I think here we're just
delivering a different set of information to our observers. But since we're
delivering it to the same set of observers, it seems rather redundant (some
observers listen for one event, some for the other).

I've revised this so that we have just a single event handler, with all the data
included. The individual observers can decide which information is relevant to
them.

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:144: net::GetHostOrSpecFromURL(entry ?
entry->GetURL() : GURL::EmptyGURL());
On 2014/06/26 22:16:38, Dan Beam wrote:
> if there's no entry, shouldn't we just ask the web_contents() it's visible URL
> instead?

I don't think so ... zoom levels don't correspond to visible URLs in
HostZoomMap, rather they're based on the last committed entry value..

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:145:
zoom_map->SetZoomLevelForHost(host, zoom_level);
On 2014/06/26 22:16:38, Dan Beam wrote:
> should we be calling SetZoomLevelForHost() with an empty URL?

Hmmm, no, I don't think so (esp. to be consistent with how HostZoomMap is
handling empty entries). Fixed.

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.cc:174: url = entry->GetURL();
On 2014/06/26 22:16:38, Dan Beam wrote:
> same re: empty GURL or !entry handling

Done.

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:68: int GetZoomPercent() const;
On 2014/06/26 22:16:38, Dan Beam wrote:
> nit: doc comment?  usually if something is StudlyCaps() it's doing more than a
> dumb_unix_hacker_accessor(), so I'd be interested to know why these are
> StudlyCaps in this case when reading the header.

Done.

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_event_manager.cc (right):

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.cc:7: #include "base/bind.h"
On 2014/06/26 22:16:38, Dan Beam wrote:
> ^ why do you need bind.h?

We don't, this is a straggler from an earlier version ... removed.

https://codereview.chromium.org/301733006/diff/330001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_event_manager.cc:23: }
On 2014/06/26 22:16:38, Dan Beam wrote:
> nit: {}

Done.

[Dan Beam, 2014-07-03 01:28:53]

lgtm w/nits

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_observer.h (right):

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_observer.h:17: struct OnZoomChangedEventData {
nit: ZoomChangeData or ZoomChangeEventData

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_observer.h:32: bool can_show_bubble_;
_ implies these are private but default access in a struct is public (classes
are private by default).

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_observer.h:33: };
struct OnZoomChangedEventData {
  content::WebContents* web_contents;
  double old_zoom_level;
  double new_zoom_level;
  ZoomController::ZoomMode zoom_mode;
  bool can_show_bubble;
};

should just magically work...

[Devlin, 2014-07-03 14:55:37]

Also lgtm with nits.

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1807: if
(url.SchemeIs(content::kChromeUIScheme) ||
Recently we added PermissionsData::IsRestrictedUrl(), which I think will be a
perfect fit for this.  (In this case, you can pass in the same url for top and
document.)

if (PermissionsData::IsRestrictedUrl(url, url, GetExtension(), &error_))
  return false;

(This comment goes for the other places we check this, too.)

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_constants.h (right):

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_constants.h:78: extern const char
kCannotChangeChromePageZoomSettingsError[];
You can get rid of some of these errors once you use IsRestrictedUrl().

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_view.cc (right):

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_view.cc:36:
SetTooltipText(l10n_util::GetStringFUTF16Int(IDS_TOOLTIP_ZOOM,
nit: Not your code, but could you fix param wrapping?

SetTooltipText(l10n_util::GetStringFUTF16Int(
    IDS_TOOLTIP_ZOOOM, zoom_controller->GetZoomPercent()));

[wjmaclean, 2014-07-03 19:37:22]

I've addressed the last round of comments.

dbeam@ - I made some changes in ZoomController to (1) avoid duplicate events,
and (2) fill in some missing event information in SetMode(). I had to move some
things around a bit ... did you want to take another look?

devlin@ - I switched to IsRestrictedUrl() as you suggested, but also had to
update a test since it filters out data: urls.

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:1807: if
(url.SchemeIs(content::kChromeUIScheme) ||
On 2014/07/03 14:55:37, Devlin wrote:
> Recently we added PermissionsData::IsRestrictedUrl(), which I think will be a
> perfect fit for this.  (In this case, you can pass in the same url for top and
> document.)
> 
> if (PermissionsData::IsRestrictedUrl(url, url, GetExtension(), &error_))
>   return false;
> 
> (This comment goes for the other places we check this, too.)

Recent indeed! I had to rebase ... ;-)

Done.

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_constants.h (right):

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_constants.h:78: extern const char
kCannotChangeChromePageZoomSettingsError[];
On 2014/07/03 14:55:37, Devlin wrote:
> You can get rid of some of these errors once you use IsRestrictedUrl().

Done.

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_view.cc (right):

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_view.cc:36:
SetTooltipText(l10n_util::GetStringFUTF16Int(IDS_TOOLTIP_ZOOM,
On 2014/07/03 14:55:37, Devlin wrote:
> nit: Not your code, but could you fix param wrapping?
> 
> SetTooltipText(l10n_util::GetStringFUTF16Int(
>     IDS_TOOLTIP_ZOOOM, zoom_controller->GetZoomPercent()));

Done.

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_observer.h (right):

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_observer.h:17: struct OnZoomChangedEventData {
On 2014/07/03 01:28:53, Dan Beam wrote:
> nit: ZoomChangeData or ZoomChangeEventData

Done.

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_observer.h:32: bool can_show_bubble_;
On 2014/07/03 01:28:53, Dan Beam wrote:
> _ implies these are private but default access in a struct is public (classes
> are private by default).

Done.

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_observer.h:33: };
On 2014/07/03 01:28:53, Dan Beam wrote:
> struct OnZoomChangedEventData {
>   content::WebContents* web_contents;
>   double old_zoom_level;
>   double new_zoom_level;
>   ZoomController::ZoomMode zoom_mode;
>   bool can_show_bubble;
> };
> 
> should just magically work...

Not sure I follow ... work in what sense?

[wjmaclean, 2014-07-04 13:11:29]

I'm going to go ahead and try to land this while the tree is quiet. If there are
any additional items we can do them in a follow-on.

[wjmaclean, 2014-07-04 13:11:50]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-07-04 13:12:26]

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

[wjmaclean, 2014-07-04 13:27:31]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-07-04 13:27:48]

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

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

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/24925)

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

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

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

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[wjmaclean, 2014-07-04 17:30:07]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-07-04 17:30:33]

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

[wjmaclean, 2014-07-04 17:39:10]

The CQ bit was unchecked by wjmaclean@chromium.org

[wjmaclean, 2014-07-04 17:41:59]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-07-04 17:42:53]

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

[wjmaclean, 2014-07-04 18:40:14]

The CQ bit was unchecked by wjmaclean@chromium.org

[wjmaclean, 2014-07-04 18:42:06]

The CQ bit was checked by wjmaclean@chromium.org

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

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

[commit-bot: I haz the power, 2014-07-04 22:34:26]

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

[commit-bot: I haz the power, 2014-07-05 00:29:02]

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

[commit-bot: I haz the power, 2014-07-05 00:29:04]

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

[wjmaclean, 2014-07-05 01:35:07]

The CQ bit was checked by wjmaclean@chromium.org

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

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

[commit-bot: I haz the power, 2014-07-05 19:09:10]

Change committed as 281468

[Dan Beam, 2014-07-07 17:04:18]

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_observer.h (right):

https://codereview.chromium.org/301733006/diff/350001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_observer.h:33: };
On 2014/07/03 19:37:21, wjmaclean wrote:
> On 2014/07/03 01:28:53, Dan Beam wrote:
> > struct OnZoomChangedEventData {
> >   content::WebContents* web_contents;
> >   double old_zoom_level;
> >   double new_zoom_level;
> >   ZoomController::ZoomMode zoom_mode;
> >   bool can_show_bubble;
> > };
> > 
> > should just magically work...
> 
> Not sure I follow ... work in what sense?

no ctor or initializer list necessary

https://codereview.chromium.org/301733006/diff/470001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/470001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:62: can_show_bubble(can_show_bubble) {}
^ i don't think this initializer list is necessary

https://codereview.chromium.org/301733006/diff/470001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:139: std::vector<ZoomChangedEventData>
event_data_;
why is this a vector if you only ever use 1?

[wjmaclean, 2014-07-07 17:12:52]

https://codereview.chromium.org/301733006/diff/470001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/470001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:62: can_show_bubble(can_show_bubble) {}
On 2014/07/07 17:04:18, Dan Beam wrote:
> ^ i don't think this initializer list is necessary

Hmm, then I should initialize using

ZoomChangedEventData data = {val1, val2, ...}

or

ZoomChangedEventData data;
data.val1 = value1;
...

Is this what you had in mind? I had hoped to keep initialization simple by doing
it when the struct is declared.

https://codereview.chromium.org/301733006/diff/470001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:139: std::vector<ZoomChangedEventData>
event_data_;
On 2014/07/07 17:04:17, Dan Beam wrote:
> why is this a vector if you only ever use 1?

I wanted a clear way to test if there was an event object or not when
UpdateState is called ... perhaps the vector is overkill (could do it with an
member var and a bool)?

[Dan Beam, 2014-07-07 17:20:20]

https://codereview.chromium.org/301733006/diff/470001/chrome/browser/ui/zoom/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://codereview.chromium.org/301733006/diff/470001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:62: can_show_bubble(can_show_bubble) {}
On 2014/07/07 17:12:52, wjmaclean wrote:
> On 2014/07/07 17:04:18, Dan Beam wrote:
> > ^ i don't think this initializer list is necessary
> 
> Hmm, then I should initialize using
> 
> ZoomChangedEventData data = {val1, val2, ...}

yep, that'd work

> 
> or
> 
> ZoomChangedEventData data;
> data.val1 = value1;
> ...

or that

> 
> Is this what you had in mind?

yes

> I had hoped to keep initialization simple by doing
> it when the struct is declared.
> 

how does my suggestion prevent you from doing this?

https://codereview.chromium.org/301733006/diff/470001/chrome/browser/ui/zoom/...
chrome/browser/ui/zoom/zoom_controller.h:139: std::vector<ZoomChangedEventData>
event_data_;
On 2014/07/07 17:12:52, wjmaclean wrote:
> On 2014/07/07 17:04:17, Dan Beam wrote:
> > why is this a vector if you only ever use 1?
> 
> I wanted a clear way to test if there was an event object or not when
> UpdateState is called ... perhaps the vector is overkill (could do it with an
> member var and a bool)?

yes, or a scoped_ptr if you don't mind heap allocating

[wjmaclean, 2014-07-07 17:27:10]

On 2014/07/07 17:20:20, Dan Beam wrote:
>
https://codereview.chromium.org/301733006/diff/470001/chrome/browser/ui/zoom/...
> File chrome/browser/ui/zoom/zoom_controller.h (right):
> 
>
https://codereview.chromium.org/301733006/diff/470001/chrome/browser/ui/zoom/...
> chrome/browser/ui/zoom/zoom_controller.h:62: can_show_bubble(can_show_bubble)
{}
> On 2014/07/07 17:12:52, wjmaclean wrote:
> > On 2014/07/07 17:04:18, Dan Beam wrote:
> > > ^ i don't think this initializer list is necessary
> > 
> > Hmm, then I should initialize using
> > 
> > ZoomChangedEventData data = {val1, val2, ...}
> 
> yep, that'd work
> 
> > 
> > or
> > 
> > ZoomChangedEventData data;
> > data.val1 = value1;
> > ...
> 
> or that
> 
> > 
> > Is this what you had in mind?
> 
> yes
> 
> > I had hoped to keep initialization simple by doing
> > it when the struct is declared.
> > 
> 
> how does my suggestion prevent you from doing this?

I'd like to be able to do things like:

event_data_.push_back(ZoomChangedEventData(web_contents(),
                                           GetZoomLevel(),
                                           zoom_level,
                                           zoom_mode_,
                                           false /* can_show_bubble */));

which I don't think would work without the constructor. I just think this looks
cleaner. Is having the initializer extra overhead?

>
https://codereview.chromium.org/301733006/diff/470001/chrome/browser/ui/zoom/...
> chrome/browser/ui/zoom/zoom_controller.h:139:
std::vector<ZoomChangedEventData>
> event_data_;
> On 2014/07/07 17:12:52, wjmaclean wrote:
> > On 2014/07/07 17:04:17, Dan Beam wrote:
> > > why is this a vector if you only ever use 1?
> > 
> > I wanted a clear way to test if there was an event object or not when
> > UpdateState is called ... perhaps the vector is overkill (could do it with
an
> > member var and a bool)?
> 
> yes, or a scoped_ptr if you don't mind heap allocating

Heap allocation would be simpler I guess, though (as you note) a bit more
expensive. I'll think about it and come back with a CL to remove the vector.