628799 - Kill browser.scale and make fuzzy zoom API

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Benjamin Stover (:stechz)]

We only need one, and since it's related to the view and not the browser, let's put it in the view. We will, however, need a way to tell the displayport when to change resolution.

Perhaps a setter/getter for scale and an updateResolution call?

Comment 1

[Benjamin Stover (:stechz)]

One argument against scale in view: it's not really appropriate to scale things that aren't the root view. At least, we certainly don't support it. :)

Comment 2

[Benjamin Stover (:stechz)]

Another problem we have is that sometimes we need to scroll and scale without doing anything in the content process (fuzzy zoom).

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

Here's an approach for the "fuzzy zoom" problem.  See bug 629794 for some context - I'm trying to hide the code from that bug inside the browser binding.

This could also use a "finishFuzzyZoom" method to commit the new zoom level and update the displayport.  (In this patch, this happens in AnimatedZoom.updateTo when aFinish is true.)

It might be better to put the fuzzyZoom API on the browser, instead of the view.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: WIP 2 (rebased)

Comment 5

[Benjamin Stover (:stechz)]

Comment on attachment 508008 [details] [diff] [review]
WIP 2 (rebased)

>+    if (aFinish) {
>+      // If the scale level doesn't change ensure the view is well refreshed
>+      // otherwise setting the scale level of the browser will do it
>+      if (scale == browser.scale)
>+        browser.getRootView()._updateCacheViewport();
>+      else
>+        browser.scale = scale;
>+    }

Yuck. I'd like to see aFinish abolished (just put it in finish?).

The major yuck is we still need the if/else here. We could add a finishFuzzyZoom() function, that basically gets the displayport in sync with the current visible rect?

>diff --git a/chrome/content/bindings/browser.xml b/chrome/content/bindings/browser.xml
>--- a/chrome/content/bindings/browser.xml
>+++ b/chrome/content/bindings/browser.xml
>@@ -456,8 +456,8 @@
>             return true;
>           },
> 
>-          setScale: function(aXScale, aYScale) {
>-          },
>+          setScale: function(scale) {},
>+          fuzzyZoom: function(scale, x, y) {},
> 

Could we get rid of setScale altogether on the views, and leave browser.scale alone? Or, assuming we need to keep the current scale cached, make it _setScale so we know not to call it outside of browser binding. If we are leaving scale on browser (and it can't be used on iframes etc), then fuzzyZoom should go to the browser element as well.

Looks awesome otherwise.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This kills the browser._scale cached value, and uses view._scale as the source of truth everywhere.

The fuzzyZoom and finishFuzzyZoom methods are now on the browser object, and the contentView scale properties are no longer used outside browser.xml.

This also sets the stage for some cleanup of the pinch zoom code, in the next patch.

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: pinch cleanup

A bunch of pinch zoom values cached in GestureModule are also cached in AnimatedZoom or in browser.xml.  This gets rid of the duplicate properties.

These two patches shorten our code by a total of 32 lines.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: followup

This change accidentally caused the ZoomChanged event to no longer fire at the end of animated zooms.  This event is used only for the FennecMark zoom test (Talos "Tzoom").  This patch causes the event to fire after both fuzzy and non-fuzzy zoom changes.

Comment 9

[Benjamin Stover (:stechz)]

Comment on attachment 509272 [details] [diff] [review]
followup

We should be careful that finishFuzzyZoom is only called for zoom changes. Wes's reize patch walks the line, for instance.

Comment 10

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mobile-browser/rev/134e1e507ca3
http://hg.mozilla.org/mobile-browser/rev/744bdd9543c9
http://hg.mozilla.org/mobile-browser/rev/1165fcc93242