Move ZoomController out of ChromeWebViewGuestDelegate.

Move ZoomController out of ChromeWebViewGuestDelegate.

This CL moves the ZoomController code for WebView out of
ChromeWebViewGuestDelegate and into GuestViewBase and WebViewGuest. This
will allow non-chrome builds to access the WebView zoom api.

BUG=none

Committed: https://crrev.com/ec6bd52a1f567613e1fb60d2dbdb9a8d950e3c85
Cr-Commit-Position: refs/heads/master@{#308094}

[wjmaclean, 2014-12-09 19:47:42]

wjmaclean@chromium.org changed reviewers:
+ fsamuel@chromium.org, jamescook@chromium.org

[wjmaclean, 2014-12-09 19:50:10]

This is a (relatively) simple refactor to move the ZoomController for
GuestView/WebView into extensions, PTAL.

With this refactor, one can zoom a WebView in app_shell.

E.g. run

out/Debug/envoy_shell --load-apps=<path to
chrome-app-samples>/samples/webview-samples/browser/

and zoom the browser contents (in a WebView) using the provided UI.

[Fady Samuel, 2014-12-09 20:09:47]

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:214: // GetZoomLevel() and and
SetZoomLevel() in WebViewGuest.
This comment is no longer correct as this code lives in GuestViewBase. I'd
simply remove it and add a brief comment about what functionality ZoomController
provides.

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:216: // pass here.
This looks right to me.

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:516:
ui_zoom::ZoomController::FromWebContents(web_contents())
It would be nice to add a comment here that updating the embedder's zoom level
updates the guest's zoom level.

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.h:99: virtual void
EmbedderWillBeDestroyed();
This method is meant to be overriden by derived classes which means you'll lose
the code in GuestViewBase::EmbedderWillBeDestroyed.

I would not implement this here. Instead, I'd create a new private method:
perhaps OwnerWebContentsWillBeDestroyed(), do your cleanup work there and then
call EmbedderWillBeDestroyed() at the end.

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest.cc:616: double
WebViewGuest::GetZoom() {
Now that this method is trivial I'd make this a simple inline accessor in the
header file.

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest.cc:707:
current_zoom_factor_(1.0) {
weak_ptr_factory_ must be the last member variable. In construction, it's the
last object to construct, and destruction is the reverse order, so it will be
the first object to be destructed. That means that weak pointers get invalidated
early on to avoid re-entrant code.

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest.h:377: double
current_zoom_factor_;
Above weak_ptr_factory_

[wjmaclean, 2014-12-09 20:50:04]

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:214: // GetZoomLevel() and and
SetZoomLevel() in WebViewGuest.
On 2014/12/09 20:09:46, Fady Samuel wrote:
> This comment is no longer correct as this code lives in GuestViewBase. I'd
> simply remove it and add a brief comment about what functionality
ZoomController
> provides.

Done.

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:216: // pass here.
On 2014/12/09 20:09:46, Fady Samuel wrote:
> This looks right to me.

Acknowledged.

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.cc:516:
ui_zoom::ZoomController::FromWebContents(web_contents())
On 2014/12/09 20:09:46, Fady Samuel wrote:
> It would be nice to add a comment here that updating the embedder's zoom level
> updates the guest's zoom level.

Done.

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/guest_view_base.h:99: virtual void
EmbedderWillBeDestroyed();
On 2014/12/09 20:09:46, Fady Samuel wrote:
> This method is meant to be overriden by derived classes which means you'll
lose
> the code in GuestViewBase::EmbedderWillBeDestroyed.
> 
> I would not implement this here. Instead, I'd create a new private method:
> perhaps OwnerWebContentsWillBeDestroyed(), do your cleanup work there and then
> call EmbedderWillBeDestroyed() at the end.

Done.

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest.cc:616: double
WebViewGuest::GetZoom() {
On 2014/12/09 20:09:47, Fady Samuel wrote:
> Now that this method is trivial I'd make this a simple inline accessor in the
> header file.

Done.

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest.cc:707:
current_zoom_factor_(1.0) {
On 2014/12/09 20:09:46, Fady Samuel wrote:
> weak_ptr_factory_ must be the last member variable. In construction, it's the
> last object to construct, and destruction is the reverse order, so it will be
> the first object to be destructed. That means that weak pointers get
invalidated
> early on to avoid re-entrant code.

Done.

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/787813004/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest.h:377: double
current_zoom_factor_;
On 2014/12/09 20:09:47, Fady Samuel wrote:
> Above weak_ptr_factory_

Thanks, missed that!

[wjmaclean, 2014-12-09 21:38:12]

wjmaclean@chromium.org changed reviewers:
+ rockot@chromium.org
- jamescook@chromium.org

[wjmaclean, 2014-12-09 21:39:01]

Swapping rockot@ for jamescook@ in reviewer list as file lineup has changed
slightly ...

rockot@ PTAL?

[Fady Samuel, 2014-12-09 21:41:43]

lgtm + nits

https://codereview.chromium.org/787813004/diff/40001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/787813004/diff/40001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest.cc:370:
GuestViewBase::EmbedderWillBeDestroyed();
This should no longer be necessary, I think??

https://codereview.chromium.org/787813004/diff/40001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/787813004/diff/40001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest.h:178: double GetZoom() {
return current_zoom_factor_; }
Call zoom() const

[wjmaclean, 2014-12-09 22:12:37]

Addressed suggestions (will be included as part of next patch upload).

https://codereview.chromium.org/787813004/diff/40001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/787813004/diff/40001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest.cc:370:
GuestViewBase::EmbedderWillBeDestroyed();
On 2014/12/09 21:41:42, Fady Samuel wrote:
> This should no longer be necessary, I think??

Done.

https://codereview.chromium.org/787813004/diff/40001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/787813004/diff/40001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest.h:178: double GetZoom() {
return current_zoom_factor_; }
On 2014/12/09 21:41:42, Fady Samuel wrote:
> Call zoom() const

Done.

[Ken Rockot(use gerrit already), 2014-12-09 22:33:15]

lgtm

[wjmaclean, 2014-12-10 15:33:14]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-12-10 15:34:32]

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

[commit-bot: I haz the power, 2014-12-10 15:54:35]

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

[commit-bot: I haz the power, 2014-12-10 15:54:35]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2014-12-10 16:20:33]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-12-10 16:21:55]

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

[wjmaclean, 2014-12-10 16:46:30]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-12-10 16:47:19]

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

[commit-bot: I haz the power, 2014-12-10 18:23:30]

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

[commit-bot: I haz the power, 2014-12-10 18:23:31]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[wjmaclean, 2014-12-10 18:38:17]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-12-10 18:39:26]

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

[commit-bot: I haz the power, 2014-12-10 20:25:59]

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

[commit-bot: I haz the power, 2014-12-10 20:26:00]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Fady Samuel, 2014-12-11 23:18:12]

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:106:
guest_->StopObservingOwnersZoomControllerIfNecessary();
Move this to GuestViewBase::Destroy then you don't need to call it from multiple
call sites.

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:152:
observing_owners_zoom_controller_(false),
Is this necessary?

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:496:
StopObservingOwnersZoomControllerIfNecessary();
This should be in Destroy.

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:560: if (zoom_controller) {
Early exit preferred.

[wjmaclean, 2014-12-11 23:29:42]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2014-12-11 23:30:00]

The CQ bit was unchecked by wjmaclean@chromium.org

[wjmaclean, 2014-12-11 23:45:24]

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:106:
guest_->StopObservingOwnersZoomControllerIfNecessary();
On 2014/12/11 23:18:11, Fady Samuel wrote:
> Move this to GuestViewBase::Destroy then you don't need to call it from
multiple
> call sites.

Ok, but then we have to preserver owner_weeb_contents_ until after the call to
Destroy().

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:152:
observing_owners_zoom_controller_(false),
On 2014/12/11 23:18:12, Fady Samuel wrote:
> Is this necessary?

Yes ... I think we need to make sure we can detect if a GVB is ever destructed
with its owners_web_contents_ cleared without removing the observer on the
ZoomController.

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:496:
StopObservingOwnersZoomControllerIfNecessary();
On 2014/12/11 23:18:12, Fady Samuel wrote:
> This should be in Destroy.

OK. Then we are *guaranteed* never to call the destructor without call Destroy()
first?

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:560: if (zoom_controller) {
On 2014/12/11 23:18:11, Fady Samuel wrote:
> Early exit preferred.

Done.

[wjmaclean, 2014-12-11 23:45:30]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-12-11 23:47:22]

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

[Fady Samuel, 2014-12-12 00:03:46]

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:106:
guest_->StopObservingOwnersZoomControllerIfNecessary();
On 2014/12/11 23:45:24, wjmaclean wrote:
> On 2014/12/11 23:18:11, Fady Samuel wrote:
> > Move this to GuestViewBase::Destroy then you don't need to call it from
> multiple
> > call sites.
> 
> Ok, but then we have to preserver owner_weeb_contents_ until after the call to
> Destroy().

OK, that seems fine. I can't think of reason for that to be a problem.

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:152:
observing_owners_zoom_controller_(false),
On 2014/12/11 23:45:24, wjmaclean wrote:
> On 2014/12/11 23:18:12, Fady Samuel wrote:
> > Is this necessary?
> 
> Yes ... I think we need to make sure we can detect if a GVB is ever destructed
> with its owners_web_contents_ cleared without removing the observer on the
> ZoomController.

GuestViewBase should always have an owner_web_contents_ at this point. After
your move in OwnerLifetimeObserver::Destroy, this should always be the case.

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:496:
StopObservingOwnersZoomControllerIfNecessary();
On 2014/12/11 23:45:24, wjmaclean wrote:
> On 2014/12/11 23:18:12, Fady Samuel wrote:
> > This should be in Destroy.
> 
> OK. Then we are *guaranteed* never to call the destructor without call
Destroy()
> first?

Yes, ~GuestViewBase is protected, and in derived types it's protected. The only
way to delete a GuestViewBase is to call Destroy()

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:562:
zoom_controller->AddObserver(this);
A GuestViewBase's owner/embedder can change during its lifetime. You probably
want to remove the observer here. In fact, now that I think about it, you
probably want to call this in GuestViewBase::WillAttach instead of DidAttach.
That way you can see what the old owner was and remove the observer from the old
owner.

[Fady Samuel, 2014-12-12 00:07:44]

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/787813004/diff/180001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_base.cc:496:
StopObservingOwnersZoomControllerIfNecessary();
On 2014/12/12 00:03:46, Fady Samuel wrote:
> On 2014/12/11 23:45:24, wjmaclean wrote:
> > On 2014/12/11 23:18:12, Fady Samuel wrote:
> > > This should be in Destroy.
> > 
> > OK. Then we are *guaranteed* never to call the destructor without call
> Destroy()
> > first?
> 
> Yes, ~GuestViewBase is protected, and in derived types it's protected. The
only
> way to delete a GuestViewBase is to call Destroy()

Actually the derived types' destructors are private!

[commit-bot: I haz the power, 2014-12-12 01:45:02]

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

[commit-bot: I haz the power, 2014-12-12 01:45:03]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2014-12-12 14:44:39]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-12-12 14:45:30]

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

[commit-bot: I haz the power, 2014-12-12 16:18:03]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2014-12-12 16:19:03]

Patchset 13 (id:??) landed as
https://crrev.com/ec6bd52a1f567613e1fb60d2dbdb9a8d950e3c85
Cr-Commit-Position: refs/heads/master@{#308094}