[Android] Move zoom controls management into WebView

[Android] Move zoom controls management into WebView

Chrome for Android doesn't use zoom controls, so it makes sense
to move all the related code into android_webview.

However, as visibility of zoom controls is controlled from the
internals of ContentView*, we are providing hooks through a new
delegate interface.

BUG=b/8296421
R=benm@chromium.org, joth@chromium.org

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

[mnaganov (inactive), 2013-05-02 12:26:54]

ContentSettings / AwSettings refactoring continued

[benm (inactive), 2013-05-02 13:08:44]

https://codereview.chromium.org/14840011/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ZoomManager.java
(right):

https://codereview.chromium.org/14840011/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ZoomManager.java:140:
public void invokeZoomPicker() {
Would it be nice I wonder if we had a ZoomControlsDelegate interface that could
be defined and invoked from here but only implemented and set by the
android_webview layer? i.e. AwZoomManger becomes AwZoomControlsImpl or something
like that. WDYT?

[mnaganov (inactive), 2013-05-03 09:00:17]

https://codereview.chromium.org/14840011/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/ZoomManager.java
(right):

https://codereview.chromium.org/14840011/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/ZoomManager.java:140:
public void invokeZoomPicker() {
On 2013/05/02 13:08:44, benm wrote:
> Would it be nice I wonder if we had a ZoomControlsDelegate interface that
could
> be defined and invoked from here but only implemented and set by the
> android_webview layer? i.e. AwZoomManger becomes AwZoomControlsImpl or
something
> like that. WDYT?

That's a brilliant idea, thanks! Done.

[benm (inactive), 2013-05-03 09:31:33]

Thanks Mikhail!

https://codereview.chromium.org/14840011/diff/6001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/14840011/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:57:
return false;
nit: maybe make this a ? : for brevity.

https://codereview.chromium.org/14840011/diff/6001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14840011/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:297:
private ZoomControlsDelegate mZoomControlsDelegate;
It seems OK that ContentViewCore controls the visibility of zoom controls, and
ZoomManager has responsibility of maintaining the current zoom level, but I've
got an itch that it would be nicer to move ZoomControls stuff into ZoomManager
too. That way we can (for example) move the logic around whether to show/hide
out of CVC and into ZoomManager. WDYT?

[mnaganov (inactive), 2013-05-03 10:30:57]

https://codereview.chromium.org/14840011/diff/6001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/14840011/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:57:
return false;
On 2013/05/03 09:31:34, benm wrote:
> nit: maybe make this a ? : for brevity.

Who cares about brevity when writing in Java? Joking :) Done.

https://codereview.chromium.org/14840011/diff/6001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14840011/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:297:
private ZoomControlsDelegate mZoomControlsDelegate;
On 2013/05/03 09:31:34, benm wrote:
> It seems OK that ContentViewCore controls the visibility of zoom controls, and
> ZoomManager has responsibility of maintaining the current zoom level, but I've
> got an itch that it would be nicer to move ZoomControls stuff into ZoomManager
> too. That way we can (for example) move the logic around whether to show/hide
> out of CVC and into ZoomManager. WDYT?

I think what actually worries you is abundance of "mZoomControlsDelegate !=
null" checks. I've got rid of them by providing a stub class that
ContentViewCore uses by default.

[benm (inactive), 2013-05-03 16:01:12]

lgtm

thanks mikhail!

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:678:
mZoomControlsDelegate = new ZoomControlsDelegate() {
nit: maybe just add a comment to say that by default ContentViewCore doesn't
support zoom controls.

[mnaganov (inactive), 2013-05-03 16:05:08]

Jonathan, PTAL! I need an owner's review for the changes in content.

[mnaganov (inactive), 2013-05-03 16:09:51]

Thanks, Ben!

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:678:
mZoomControlsDelegate = new ZoomControlsDelegate() {
On 2013/05/03 16:01:12, benm wrote:
> nit: maybe just add a comment to say that by default ContentViewCore doesn't
> support zoom controls.

I think, it is more correct say that Chrome for Android doesn't provide zoom
controls, but telling about Chrome in the content layer doesn't make much sense.
Also, the comment on the interface says "embedder-provided zoom controls", so
it's natural to assume that ContentViewCore doesn't provide its own.

So I suppose that the comment isn't really needed.

[joth, 2013-05-03 19:32:43]

nice cleanup, lgtm

some suggestions for going even further with this, but maybe as another step...

https://codereview.chromium.org/14840011/diff/6003/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwZoomControls.java
(right):

https://codereview.chromium.org/14840011/diff/6003/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwZoomControls.java:26: if
(zoomControls != null && !zoomControls.isVisible()) {
maybe we should call setVisible(true) even if they're already visible, as this
has side effect of refreshing the delayed hide timer inside
ZoomButtonsController.dismissControlsDelay ?

https://codereview.chromium.org/14840011/diff/6003/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwZoomControls.java:27:
zoomControls.setVisible(true);
do we need to update the individual button states too?  - updateZoomControls()

https://codereview.chromium.org/14840011/diff/6003/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwZoomControls.java:41: if
(mZoomButtonsController == null) return;
not obvious why you use mZoomButtonsController here but getZoomControls() above?
maybe comment one or other.

https://codereview.chromium.org/14840011/diff/6003/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwZoomControls.java:50:
mZoomButtonsController.setZoomOutEnabled(canZoomOut);
how do these settings nest? do we need to call 
mZoomButtonsController.getZoomControls().setVisibility(VISISBLE) here too, or
does enabling either button automatically cause the controls to be shown?

https://codereview.chromium.org/14840011/diff/6003/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwZoomControls.java:59:
private ZoomButtonsController getZoomControls() {
getZoomController() ? Maybe comment when to use this vs. mZoomButtonsController
directly. (Or make the fn name reflect the side effects it has)

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:13:
* from ContentView.getContentSettings().
ContentViewCore.get...
(I'd rather we point folks to that than the pointless ContentView wrapper
method)

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:187:
public static interface ZoomControlsDelegate {
nit: I think static is spurious here? (didn't know you could say this, in fact)

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2470:
mZoomControlsDelegate.invokeZoomPicker();
feels like pulling at a bit of string here, but..
this method is only called from onScrolled and by AwContents. AwContents already
knows when we scrolled, so could do invokeZoomPicker itself.
But then... what about the other ZoomControlsDelegate methods. window
attach/detatch and visibility change are all known in AwContents. so it just
leaves updateFrameInfo(). And you know what.. with the synchronous compositor
that's easier to route to aw/browser view renderer than it is to get it into
ContentViewCore. (We already receive a distilled version of this call in 
BrowserViewRendererImpl::OnFrameInfoUpdated() anyway.
(ContentViewClient.onPageScaleChanged might also be an appropriate hook for
this)

So in fact, it'd be an option to entirely remove this stuff from CVC if we
wanted to.

[mnaganov (inactive), 2013-05-07 09:55:44]

Thanks, Jonathan! Very insightful comments.

https://codereview.chromium.org/14840011/diff/6003/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwZoomControls.java
(right):

https://codereview.chromium.org/14840011/diff/6003/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwZoomControls.java:26: if
(zoomControls != null && !zoomControls.isVisible()) {
On 2013/05/03 19:32:43, joth wrote:
> maybe we should call setVisible(true) even if they're already visible, as this
> has side effect of refreshing the delayed hide timer inside
> ZoomButtonsController.dismissControlsDelay ?

Right, it seems better to omit this check. Also, the legacy ZoomManager doesn't
have one.

https://codereview.chromium.org/14840011/diff/6003/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwZoomControls.java:27:
zoomControls.setVisible(true);
On 2013/05/03 19:32:43, joth wrote:
> do we need to update the individual button states too?  - updateZoomControls()
> 

ZoomButtonsController calls ZoomListener's onVisibilityChanged, which calls
updateZoomControls.

https://codereview.chromium.org/14840011/diff/6003/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwZoomControls.java:41: if
(mZoomButtonsController == null) return;
On 2013/05/03 19:32:43, joth wrote:
> not obvious why you use mZoomButtonsController here but getZoomControls()
above?
> maybe comment one or other.
> 

Yes, that's a strange assymetry. Changed to getZoomControls() here.

https://codereview.chromium.org/14840011/diff/6003/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwZoomControls.java:50:
mZoomButtonsController.setZoomOutEnabled(canZoomOut);
On 2013/05/03 19:32:43, joth wrote:
> how do these settings nest? do we need to call 
> mZoomButtonsController.getZoomControls().setVisibility(VISISBLE) here too, or
> does enabling either button automatically cause the controls to be shown?

Visibility and button status are orthogonal. But we don't want to show controls
from here. Recall that the controls are not always shown. They appear for a
short interval after a scroll event. Thus, forcing them to show from here is
undesired.

https://codereview.chromium.org/14840011/diff/6003/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwZoomControls.java:59:
private ZoomButtonsController getZoomControls() {
On 2013/05/03 19:32:43, joth wrote:
> getZoomController() ? Maybe comment when to use this vs.
mZoomButtonsController
> directly. (Or make the fn name reflect the side effects it has)

Renamed. Added a comment on the field to advice to use getZoomController where
possible.

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
(right):

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentSettings.java:13:
* from ContentView.getContentSettings().
On 2013/05/03 19:32:43, joth wrote:
> ContentViewCore.get...
> (I'd rather we point folks to that than the pointless ContentView wrapper
> method)

Done.

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:187:
public static interface ZoomControlsDelegate {
On 2013/05/03 19:32:43, joth wrote:
> nit: I think static is spurious here? (didn't know you could say this, in
fact)

I've copied it from the two interfaces above. According to Stack Overflow, this
is indeed redundant, so removing from this interface and the two above.

https://codereview.chromium.org/14840011/diff/6003/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2470:
mZoomControlsDelegate.invokeZoomPicker();
On 2013/05/03 19:32:43, joth wrote:
> feels like pulling at a bit of string here, but..
> this method is only called from onScrolled and by AwContents. AwContents
already
> knows when we scrolled, so could do invokeZoomPicker itself.
> But then... what about the other ZoomControlsDelegate methods. window
> attach/detatch and visibility change are all known in AwContents. so it just
> leaves updateFrameInfo(). And you know what.. with the synchronous compositor
> that's easier to route to aw/browser view renderer than it is to get it into
> ContentViewCore. (We already receive a distilled version of this call in 
> BrowserViewRendererImpl::OnFrameInfoUpdated() anyway.
> (ContentViewClient.onPageScaleChanged might also be an appropriate hook for
> this)
> 
> So in fact, it'd be an option to entirely remove this stuff from CVC if we
> wanted to.

Yes, I was also considering that. Decided to keep as is for now, but will take a
look later.

[commit-bot: I haz the power, 2013-05-07 09:55:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/14840011/6003

[mnaganov (inactive), 2013-05-07 13:00:14]

This should land soonish

[mnaganov (inactive), 2013-05-08 07:56:24]

Committed patchset #4 manually as r198862 (presubmit successful).