Closed Bug 628799 Opened 13 years ago Closed 13 years ago

Kill browser.scale and make fuzzy zoom API

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stechz, Assigned: mbrubeck)

References

Details

Attachments

(3 files, 2 obsolete files)

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?
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. :)
Depends on: 624451
Summary: Kill browser.scale → Kill browser.scale and make fuzzy zoom API
Another problem we have is that sometimes we need to scroll and scale without doing anything in the content process (fuzzy zoom).
Blocks: 629794
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #508004 - Flags: feedback?(ben)
Attached patch WIP 2 (rebased) (obsolete) — Splinter Review
Attachment #508004 - Attachment is obsolete: true
Attachment #508008 - Flags: feedback?(ben)
Attachment #508004 - Flags: feedback?(ben)
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.
Attachment #508008 - Flags: feedback?(ben) → feedback+
Attached patch patchSplinter Review
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.
Assignee: nobody → mbrubeck
Attachment #508008 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #508819 - Flags: review?(ben)
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.
Attachment #508822 - Flags: review?(ben)
Attachment #508819 - Flags: review?(ben) → review+
Attachment #508822 - Flags: review?(ben) → review+
Attached patch followupSplinter Review
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.
Attachment #509272 - Flags: review?(ben)
Attachment #509272 - Attachment description: patch → followup
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.
Attachment #509272 - Flags: review?(ben) → review+
Depends on: 633258
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: