Make sure WebContents passed to chrome_page_zoom::Zoom() have a ZoomController.

Make sure WebContents passed to chrome_page_zoom::Zoom() have a ZoomController.

Not all WebContents will be zoomed by chrome_page_zoom::Zoom(), but
those that are need to have a ZoomCOntroller. It is assumed that at
the point at which the WebContents is created it should be known if
it will be passed into chrome_page_zoom::Zoom(), so the ZoomController
is created at that point.

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

[wjmaclean, 2014-07-07 15:44:06]

Just a further comment: another option is to have chrome_page_zoom::Zoom() force
creation of the ZoomController if it's missing. I opted not to do that since I
felt this responsibility should rest with the WebContent's creator, but I am
willing to be convinced otherwise.

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

lgtm, though i'm not sure dev tools needs a ZoomController. the original reason
we wrote this class was to show zoom notifications/bubble in the omnibox, but
dev tools don't have this...

+pfeldman@ to answer this.

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

On 2014/07/07 17:09:04, Dan Beam wrote:
> lgtm, though i'm not sure dev tools needs a ZoomController. the original
reason
> we wrote this class was to show zoom notifications/bubble in the omnibox, but
> dev tools don't have this...
> 
> +pfeldman@ to answer this.

I guess with the extensions-tab zoom work building on ZoomController, it has
become sort of the "Chrome-side" manager for zoom, while HostZoomMap is the
"Content-side" manager. I don't know if it would ever make sense for an
extension to want to control zoom for dev tools, but I suppose it might.

I think it certainly makes it simpler if all the chrome_page_zoom access goes
through only one 'manager'.

[dgozman, 2014-07-08 09:16:43]

On 2014/07/07 17:09:04, Dan Beam wrote:
> lgtm, though i'm not sure dev tools needs a ZoomController. the original
reason
> we wrote this class was to show zoom notifications/bubble in the omnibox, but
> dev tools don't have this...

DevTools does not have bubble, there is a special check in browser.cc to bail
out of notifications. Maybe this check should be moved to ZoomController than?
We just need a simple way of "zoom all the pages at once" call. If there is a
better way to do this, please suggest.
If zoom is somehow resolved for the apps, we can reuse that too.

[wjmaclean, 2014-07-08 12:27:05]

On 2014/07/08 09:16:43, dgozman wrote:
> On 2014/07/07 17:09:04, Dan Beam wrote:
> > lgtm, though i'm not sure dev tools needs a ZoomController. the original
> reason
> > we wrote this class was to show zoom notifications/bubble in the omnibox,
but
> > dev tools don't have this...
> 
> DevTools does not have bubble, there is a special check in browser.cc to bail
> out of notifications. Maybe this check should be moved to ZoomController than?

ZoomController is no longer just about the zoom bubble ... it is now an
intermediary between the browser/apps and HostZoomMap. Anything that accesses
chrome_page_zoom::Zoom() should have one. It's a fairly light-weight object, so
it's not like there's a lot of overhead to add it.

Moving the check presumably only works if DevTools has a ZoomController?

> We just need a simple way of "zoom all the pages at once" call. If there is a
> better way to do this, please suggest.

What do you mean by "all the pages". Do you mean the page being inspected + the
inspector?

> If zoom is somehow resolved for the apps, we can reuse that too.

Yes, there is now the Chrome tabs zoom API that can be used to set the zoom of
(almost) any tab from an extension or app. The exceptions are those covered in
IsRestrictedURL().

[wjmaclean, 2014-07-08 12:48:35]

pkasting@chromium.org: Please review changes?

[dgozman, 2014-07-08 14:12:32]

> Moving the check presumably only works if DevTools has a ZoomController?
Sure. But you are creating one, so that's fine. I don't see anything bad in
creating ZoomController for DevTools.

> What do you mean by "all the pages". Do you mean the page being inspected +
the
> inspector?
I mean, all the pages for this domain (all chrome-devtools://devtools/...
pages).

> > If zoom is somehow resolved for the apps, we can reuse that too.
> 
> Yes, there is now the Chrome tabs zoom API that can be used to set the zoom of
> (almost) any tab from an extension or app. The exceptions are those covered in
> IsRestrictedURL().
Does this mean that apps are responsible for zoom shortcuts and UI themselves,
and should just call chrome.tabs.setZoom for their own tabs?
If so, DevTools zoom implementation is exactly the same.

[wjmaclean, 2014-07-08 14:49:05]

On 2014/07/08 14:12:32, dgozman wrote:
> > Moving the check presumably only works if DevTools has a ZoomController?
> Sure. But you are creating one, so that's fine. I don't see anything bad in
> creating ZoomController for DevTools.

Ahh, ok :-)

> > What do you mean by "all the pages". Do you mean the page being inspected +
> the
> > inspector?
> I mean, all the pages for this domain (all chrome-devtools://devtools/...
> pages).

Yes, the existing HostZoomMap will apply the zoom on a per-origin basis, and
ZoomController doesn't interfere with this.

> > > If zoom is somehow resolved for the apps, we can reuse that too.
> > 
> > Yes, there is now the Chrome tabs zoom API that can be used to set the zoom
of
> > (almost) any tab from an extension or app. The exceptions are those covered
in
> > IsRestrictedURL().
> Does this mean that apps are responsible for zoom shortcuts and UI themselves,
> and should just call chrome.tabs.setZoom for their own tabs?
> If so, DevTools zoom implementation is exactly the same.

The tabs zoom api doesn't change the behaviour of existing UI (like the wrench
menu or Ctrl+'+' or Ctrl-<mousewheel>). But it adds additional functionality:
the ability for an extension to modify the zoom of another tab (assuming the
other tab doesn't contain a restricted origin, which the chrome-devtools might
be), the ability to opt-out an individual tab from per-origin zoom levels, the
ability to disable zoom for a particular tab, or the ability to do 'manual' zoom
for a particular tab. In the last case, an app receives notification of zoom
changes (e.g. triggered via wrench menu) and is responsible for doing its own
re-layout/rendering to achieve zoom. For example, Google docs has a separate
zoom mechanism now on the toolbar that is currently independent of the wrench
menu zoom, but could be tied in using this feature. See
https://support.google.com/docs/answer/2492762?hl=en

[dgozman, 2014-07-08 15:07:52]

> The tabs zoom api doesn't change the behaviour of existing UI (like the wrench
> menu or Ctrl+'+' or Ctrl-<mousewheel>). But it adds additional functionality:
> the ability for an extension to modify the zoom of another tab (assuming the
> other tab doesn't contain a restricted origin, which the chrome-devtools might
> be), the ability to opt-out an individual tab from per-origin zoom levels, the
> ability to disable zoom for a particular tab, or the ability to do 'manual'
zoom
> for a particular tab. In the last case, an app receives notification of zoom
> changes (e.g. triggered via wrench menu) and is responsible for doing its own
> re-layout/rendering to achieve zoom. For example, Google docs has a separate
> zoom mechanism now on the toolbar that is currently independent of the wrench
> menu zoom, but could be tied in using this feature. See
> https://support.google.com/docs/answer/2492762?hl=en

Thanks for explanation. We can probably reuse this mechanism for DevTools, if we
will do some custom zooming.

[Peter Kasting, 2014-07-08 19:16:43]

LGTM

https://codereview.chromium.org/373733003/diff/1/chrome/browser/chrome_page_z...
File chrome/browser/chrome_page_zoom.cc (right):

https://codereview.chromium.org/373733003/diff/1/chrome/browser/chrome_page_z...
chrome/browser/chrome_page_zoom.cc:72: 
Nit: Extra blank line

https://codereview.chromium.org/373733003/diff/1/chrome/browser/chrome_page_z...
chrome/browser/chrome_page_zoom.cc:74: NOTREACHED();
This is banned by the style guide.  If |zoom_controller| can actually be NULL
here, remove the NOTREACHED().  If not, change to "DCHECK(zoom_controller);" and
eliminate the conditional return.  (I imagine you want the latter.)

[wjmaclean, 2014-07-08 20:51:21]

thakis@chromium.org: Please review changes in chrome_page_zoom.cc (pkasting@ has
already looked).

https://codereview.chromium.org/373733003/diff/1/chrome/browser/chrome_page_z...
File chrome/browser/chrome_page_zoom.cc (right):

https://codereview.chromium.org/373733003/diff/1/chrome/browser/chrome_page_z...
chrome/browser/chrome_page_zoom.cc:72: 
On 2014/07/08 19:16:42, Peter Kasting wrote:
> Nit: Extra blank line

Done.

https://codereview.chromium.org/373733003/diff/1/chrome/browser/chrome_page_z...
chrome/browser/chrome_page_zoom.cc:74: NOTREACHED();
On 2014/07/08 19:16:43, Peter Kasting wrote:
> This is banned by the style guide.  If |zoom_controller| can actually be NULL
> here, remove the NOTREACHED().  If not, change to "DCHECK(zoom_controller);"
and
> eliminate the conditional return.  (I imagine you want the latter.)

I was hoping to generate well-documented failures in debug builds while not
crashing release builds. Generally, zoom_controller should never be NULL, but
this is new behavior and we've already found at least one case we missed.

Converted to DCHECK.

[Peter Kasting, 2014-07-08 20:52:47]

https://codereview.chromium.org/373733003/diff/1/chrome/browser/chrome_page_z...
File chrome/browser/chrome_page_zoom.cc (right):

https://codereview.chromium.org/373733003/diff/1/chrome/browser/chrome_page_z...
chrome/browser/chrome_page_zoom.cc:74: NOTREACHED();
On 2014/07/08 20:51:21, wjmaclean wrote:
> On 2014/07/08 19:16:43, Peter Kasting wrote:
> > This is banned by the style guide.  If |zoom_controller| can actually be
NULL
> > here, remove the NOTREACHED().  If not, change to "DCHECK(zoom_controller);"
> and
> > eliminate the conditional return.  (I imagine you want the latter.)
> 
> I was hoping to generate well-documented failures in debug builds while not
> crashing release builds. Generally, zoom_controller should never be NULL, but
> this is new behavior and we've already found at least one case we missed.
> 
> Converted to DCHECK.

Yeah, DCHECK is correct for what you describe.  The other option is to
temporarily make this a CHECK, if you decide crashing release builds is OK.

[pfeldman, 2014-07-08 20:55:32]

devtools lgtm

[Nico, 2014-07-08 23:48:58]

chrome_page_zoom.cc lgtm

[wjmaclean, 2014-07-09 18:44:03]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-07-09 18:45:13]

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

[commit-bot: I haz the power, 2014-07-10 00:12:13]

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

[commit-bot: I haz the power, 2014-07-10 02:51:58]

Change committed as 282221