Closed Bug 1201529 Opened 4 years ago Closed 4 years ago

When APZ is enabled, all browser.js codepaths that set a resolution or displayport should be disabled

Categories

(Firefox for Android :: Toolbar, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(3 files, 5 obsolete files)

STR to reproduce a specific problem:
1. Start a fennec instance built with --enable-android-apz
2. Open about:config
3. Put focus in the search field at the top

Actual:
Page gets zoomed out. You may need to un-focus and re-focus the search field to see this if it doesn't happen the first time.

Expected:
Page doesn't get zoomed out

I added some dump((new Error()).stack) calls in browser.js and got a bunch of different call stacks to code that is still calling setResolutionAndScaleTo. e.g.:

setResolution Tab.prototype.setDisplayPort@chrome://browser/content/browser.js:3835:38
Tab.prototype.sendViewportUpdate@chrome://browser/content/browser.js:4006:7
Tab.prototype.handleEvent@chrome://browser/content/browser.js:4369:9

That one is from the MozScrolledAreaChanged event listener.

setResolution Tab.prototype.setDisplayPort@chrome://browser/content/browser.js:3835:38
Tab.prototype.sendViewportUpdate@chrome://browser/content/browser.js:4006:7
viewportSizeUpdated@chrome://browser/content/browser.js:4742:5
ViewportHandler.observe@chrome://browser/content/browser.js:6281:11

That one is from the "Window:Resize" event handler.

Basically, once the C++ APZ is enabled, browser.js should not be making any calls whatsoever to setResolutionAndScaleTo, or to setDisplayPortMarginsForElement. If it does they will just interfere with what APZ is doing, and produce undefined (i.e. buggy) results.

I think the best path forward here is to disable all of these calls when APZ is enabled, and then look at them one by one to see if they result in broken features or behaviours. If so, we should implement that feature or behaviour in C++ code.

As an example, consider the code in the before-first-paint handler in browser.js. There is a bit of code there that adjusts the initial zoom in the case where we are loading an image or SVG document as the top-level document, and zooms it to fit the screen. After we disable the setResolution code in browser.js, this will probably stop working, and we will need to implement the desired behaviour in MobileViewportManager.cpp.
Attached patch WIP (obsolete) — Splinter Review
What I had in mind.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → bugmail.mozilla
Attachment #8656602 - Attachment is obsolete: true
Attachment #8657180 - Flags: review?(rbarker)
Randall pointed out that this causes my griddiv.html page to not render at all when it is loaded. I can reproduce this misbehaviour, and it happens because the RCD doesn't get scrollable metrics. This in turn is a "regression" (using the term loosely) from bug 1198839, which stopped us from setting a displayport on the RCD-RSF. On Fennec the metrics with the RCD flag is special because that's what we sync to Java-land, and not having such a layer will cause problems.

In retrospect though I think the RCD-RSF is special (and not just on Fennec) because it needs to have scrollable metrics in order to get an APZC, and it needs an APZC in order for zooming to work. In the B2G parent process we don't care about this because there is a meta-viewport tag that disallows zooming but I suspect that if somebody modified the meta-viewport tag to allow zooming, it would be broken now with bug 1198839. Botond, what do you think? Should we back out bug 1198839?

Another alternative is to #ifdef it for Fennec so that we do always set a displayport on the RCD-RSF but I feel like that's less correct.
Flags: needinfo?(botond)
Some discussion on IRC starting at http://logs.glob.uno/?c=mozilla%23apz&s=4%20Sep%202015&e=4%20Sep%202015#c14516

I'm going to try and find more use cases that will help inform the right decision here.
Flags: needinfo?(botond)
Comment on attachment 8657180 [details] [diff] [review]
Patch

Dropping review for now.
Attachment #8657180 - Flags: review?(rbarker)
I gave this some thought and agree with botond that in the long term the ideal state would be:
(1) the root document's root element always has a displayport (to "cover" the entire tree)
(2) the RCD's root element only gets a displayport/layerized if it is scrollable or zoomable

In addition, this means that:
(3) the AsyncCompositionManager code for fennec which calls setFirstPaintViewport with the RCD's metrics should just use the root scrollable metrics on the tree instead, because there may not be a RCD at all
(4) this therefore means that Java code should not rely on getting metrics corresponding to the RCD

(1) happens already. (2) half-happens (it happens if the layer is scrollable, but not if the layer is only zoomable). (3) needs to be done - my fix for bug 1201625 touched on this but is incomplete in that it is restricted to B2GDroid. For (4) I filed bug 1202020 as it is more involved.

Since tackling (4) will take some time, I think that in the short-term we need a temporary fix where we fix (2) properly and do a partial fix for (3) in that it uses the metrics on the RCD if it finds one, and if it doesn't it can fall back to the root metrics. That will allow this bug and bug 1201581 to land without having to fix (4), which I feel is somewhat important to unblock further work.

I'll give this some more thought over the weekend and post patches on Monday if I don't change my mind in the meantime.
This is the partial step (3) from comment 6. That is, if there is a RCD layer, use that for sending metrics to Java-land. If there is no RCD layer, use the metrics from the root layer instead.
Attachment #8658287 - Flags: review?(botond)
This implements step (2) from comment 6, which is my interpretation of what we discussed on Friday. That is, if the RCD-RSF is zoomable, then it should return true from WantAsyncScroll() so that it can get layerized. Let me know if I misunderstood something.
Attachment #8658288 - Flags: review?(botond)
Note that this applies on top of the patch from bug 1201416 which I landed on fx-team just now. This prevents browser.js from interfering with APZ properties while APZ is enabled.
Attachment #8657180 - Attachment is obsolete: true
Attachment #8658289 - Flags: review?(rbarker)
Attachment #8658289 - Flags: review?(rbarker) → review+
Comment on attachment 8658287 [details] [diff] [review]
Part 1 - Implement step 3 in comment 6

Review of attachment 8658287 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +1163,5 @@
>      // in Gecko and partially in Java.
>      wantNextFrame |= SampleAPZAnimations(LayerMetricsWrapper(root), aCurrentFrame);
> +    bool foundRoot = false;
> +    if (ApplyAsyncContentTransformToTree(root, foundRoot)) {
> +      MOZ_ASSERT(foundRoot);

This assertion is failing on B2G emulator tests; I'll have to figure out why...
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> > +      MOZ_ASSERT(foundRoot);
> 
> This assertion is failing on B2G emulator tests; I'll have to figure out
> why...

Doh. The flag is only set #ifdef MOZ_ANDROID_APZ, so the assertion should be also #ifdef'd. Updated try push with that corrected: https://treeherder.mozilla.org/#/jobs?repo=try&revision=985776e289af
Attachment #8658287 - Attachment is obsolete: true
Attachment #8658287 - Flags: review?(botond)
Attachment #8658651 - Flags: review?(botond)
There's still three failures on B2G emulator. I've fixed the mochitest locally and am looking into the two reftest failures.
The R1 failure seems to be a bug in MultiTiledContentClient's painting code, it's wrapping the painted content from end of the tile to the other end. When WantAsyncScroll() returns false we use a TiledContentClient instead of a MultiTiledContentClient which doesn't have this problem. I'll file a separate bug for that in a bit.
And the R11 failure also seems to be a problem with using a MultiTiledContentClient vs a SingleTiledContentClient; the display list and layer dumps are identical with and without the patch. Specifically the MultiTiledContentClient uses the tiled drawtarget which is causing problems; if I disable the tiled drawtarget code the tests pass.
Comment on attachment 8658651 [details] [diff] [review]
Part 1 - Implement step 3 in comment 6 (v2)

Review of attachment 8658651 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ -647,5 @@
>      // bug 1036967 removed the (dead) call.
>  
>  #if defined(MOZ_ANDROID_APZ)
> -    bool rootContentLayer = metrics.IsRootContent();
> -#ifdef MOZ_B2GDROID

I'm glad to see this #ifdef go away :)

::: gfx/layers/composite/AsyncCompositionManager.h
@@ +127,5 @@
>  
>  private:
>    void TransformScrollableLayer(Layer* aLayer);
>    // Return true if an AsyncPanZoomController content transform was
>    // applied for |aLayer|.

// |aOutFoundRoot| is set to true if ... Used on Android only.

@@ +128,5 @@
>  private:
>    void TransformScrollableLayer(Layer* aLayer);
>    // Return true if an AsyncPanZoomController content transform was
>    // applied for |aLayer|.
> +  bool ApplyAsyncContentTransformToTree(Layer* aLayer, bool& aFoundRoot);

|aOutFoundRoot| please (and if I recall our previous discussions on this subject correctly, |bool*| instead of |bool&|).
Attachment #8658651 - Flags: review?(botond) → review+
Comment on attachment 8658288 [details] [diff] [review]
Part 2 - Implement step 2 in comment 6

Review of attachment 8658288 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/ZoomConstraintsClient.cpp
@@ +218,5 @@
> +  // We only ever create a ZoomConstraintsClient for an RCD, so the RSF of
> +  // the presShell must be the RCD-RSF (if it exists).
> +  MOZ_ASSERT(mPresShell->GetPresContext()->IsRootContentDocument());
> +  if (nsIScrollableFrame* rcdrsf = mPresShell->GetRootScrollFrameAsScrollable()) {
> +    rcdrsf->SetZoomableByAPZ(zoomConstraints.mAllowZoom);

Do we need to trigger a layout? [1]

[1] http://logs.glob.uno/?c=mozilla%23apz&s=4%20Sep%202015&e=4%20Sep%202015#c14558
Attachment #8658288 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #17)
> >    // Return true if an AsyncPanZoomController content transform was
> >    // applied for |aLayer|.
> 
> // |aOutFoundRoot| is set to true if ... Used on Android only.

Will fix.

> @@ +128,5 @@
> >    // Return true if an AsyncPanZoomController content transform was
> >    // applied for |aLayer|.
> > +  bool ApplyAsyncContentTransformToTree(Layer* aLayer, bool& aFoundRoot);
> 
> |aOutFoundRoot| please (and if I recall our previous discussions on this
> subject correctly, |bool*| instead of |bool&|).

Heh. I did it as bool* first but I guess I misremembered our previous discussion. I was under the impression that if I did bool* I would have to null-check it in the body which I didn't want to do. I can change it back though.

(In reply to Botond Ballo [:botond] from comment #18)
> > +  MOZ_ASSERT(mPresShell->GetPresContext()->IsRootContentDocument());
> > +  if (nsIScrollableFrame* rcdrsf = mPresShell->GetRootScrollFrameAsScrollable()) {
> > +    rcdrsf->SetZoomableByAPZ(zoomConstraints.mAllowZoom);
> 
> Do we need to trigger a layout? [1]
> 
> [1]
> http://logs.glob.uno/
> ?c=mozilla%23apz&s=4%20Sep%202015&e=4%20Sep%202015#c14558

Hm I guess we should, yes. :) I'll update this patch with that change and the fixes for the tests once I have those sorted out.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19)
> > @@ +128,5 @@
> > >    // Return true if an AsyncPanZoomController content transform was
> > >    // applied for |aLayer|.
> > > +  bool ApplyAsyncContentTransformToTree(Layer* aLayer, bool& aFoundRoot);
> > 
> > |aOutFoundRoot| please (and if I recall our previous discussions on this
> > subject correctly, |bool*| instead of |bool&|).
> 
> Heh. I did it as bool* first but I guess I misremembered our previous
> discussion. I was under the impression that if I did bool* I would have to
> null-check it in the body which I didn't want to do. I can change it back
> though.

My recollection of the outcome of our discussion was this:

  - Always use pointers for out-parameters, so it's syntactically obvious
    at the call site that the caller will modify the value.

  - Document (in a comment) whether or not the out-parameter can be null.

An alternative was "use a reference when it can't be null", but we preferred the above for the syntactic-obviousness reason.
Part 1 updated to address review comments, carrying r+
Attachment #8658651 - Attachment is obsolete: true
Attachment #8660079 - Flags: review+
Update part 2 which includes the code to trigger a repaint as well the test workarounds; I filed bug 1204076 for the root cause there.

Try push in progress at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f4f3acb8cb3

I also need to do one more test locally to make sure the repaint code is working as expected; once that's done I'll reflag for review.
Attachment #8658288 - Attachment is obsolete: true
Comment on attachment 8660080 [details] [diff] [review]
Part 2 - Implement step 2 in comment 6 (v2)

The SchedulePaint call works as expected, re-requesting review on the changes.
Attachment #8660080 - Flags: review?(botond)
Attachment #8660080 - Flags: review?(botond) → review+
You need to log in before you can comment on or make changes to this bug.