Kill browser.scale and make fuzzy zoom API

RESOLVED FIXED

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: stechz, Assigned: mbrubeck)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

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
Posted 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)
Assignee

Comment 4

8 years ago
Posted patch WIP 2 (rebased) (obsolete) — Splinter Review
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
Posted 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)
Assignee

Comment 7

8 years ago
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
Posted 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)
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.