Kill browser.scale and make fuzzy zoom API

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: stechz, Assigned: mbrubeck)

Tracking

Dependency tree / graph

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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?
(Reporter)

Comment 1

8 years ago
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. :)
(Reporter)

Updated

8 years ago
Depends on: 624451
(Reporter)

Updated

8 years ago
Summary: Kill browser.scale → Kill browser.scale and make fuzzy zoom API
(Reporter)

Comment 2

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

Updated

8 years ago
Blocks: 629794
(Assignee)

Comment 3

8 years ago
Created attachment 508004 [details] [diff] [review]
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.
Attachment #508004 - Flags: feedback?(ben)
(Assignee)

Comment 4

8 years ago
Created attachment 508008 [details] [diff] [review]
WIP 2 (rebased)
Attachment #508004 - Attachment is obsolete: true
Attachment #508008 - Flags: feedback?(ben)
Attachment #508004 - Flags: feedback?(ben)
(Reporter)

Comment 5

8 years ago
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+
(Assignee)

Comment 6

8 years ago
Created attachment 508819 [details] [diff] [review]
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.
Assignee: nobody → mbrubeck
Attachment #508008 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #508819 - Flags: review?(ben)
(Assignee)

Comment 7

8 years ago
Created attachment 508822 [details] [diff] [review]
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.
Attachment #508822 - Flags: review?(ben)
(Reporter)

Updated

8 years ago
Attachment #508819 - Flags: review?(ben) → review+
(Reporter)

Updated

8 years ago
Attachment #508822 - Flags: review?(ben) → review+
(Assignee)

Comment 8

8 years ago
Created attachment 509272 [details] [diff] [review]
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.
Attachment #509272 - Flags: review?(ben)
(Assignee)

Updated

8 years ago
Attachment #509272 - Attachment description: patch → followup
(Reporter)

Comment 9

8 years ago
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+
(Assignee)

Updated

8 years ago
Depends on: 633258
You need to log in before you can comment on or make changes to this bug.