Display port needs to be recalculated on page-size change

RESOLVED FIXED in Firefox 14

Status

()

defect
P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

Trunk
Firefox 14
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 affected, firefox14 fixed, blocking-fennec1.0 beta+)

Details

Attachments

(1 attachment, 5 obsolete attachments)

A bug I've noticed using fennec this weekend, is that the displayport is often too small after a page loads until you scroll a bit. This is almost certainly caused by it not being recalculated when the page-size changes (so it starts off the size of the viewport and doesn't expand as the real page size becomes known).

This manifests as very noticeable checkerboard on the first scroll action (it stays for longer than normal too, as resizing the displayport will cause the texture to have to be recreated and probably force an invalidation).

This is quite small, assigning to myself and I'll sort it out this weekend or on Monday.
Actually, having had a think about it, this isn't as simple as I thought. Really, the display-port needs to be set immediately when page-size/scroll position change - We have the latter, but we don't have the former, as page-size changes are notified via the compositor.

A simple work-around is to just add a display-port message that Java can send to only change the display-port. This will result in some redundant drawing when the display-port changes, but it shouldn't otherwise have any adverse effect.

A better solution would be to be able to monitor page-size changes in browser.js, before they're acted on (with respect to painting), and send a Viewport:CalculateDisplayPort message. I'll look to see if this is possible.
(In reply to Chris Lord [:cwiiis] from comment #1)
> A better solution would be to be able to monitor page-size changes in
> browser.js, before they're acted on (with respect to painting), and send a
> Viewport:CalculateDisplayPort message.

Agreed, but I didn't see any obvious way to do this. We should ask somebody who knows layout if there's some event that gets dispatched when this happens.
(In reply to Kartikaya Gupta (:kats) from comment #2)
> (In reply to Chris Lord [:cwiiis] from comment #1)
> > A better solution would be to be able to monitor page-size changes in
> > browser.js, before they're acted on (with respect to painting), and send a
> > Viewport:CalculateDisplayPort message.
> 
> Agreed, but I didn't see any obvious way to do this. We should ask somebody
> who knows layout if there's some event that gets dispatched when this
> happens.

bz confirmed that there's no way of doing this currently, but there should be bugs open for it... Searching.

We may want to include a quick, inefficient fix and file a bug for the proper fix, depending on the size-change notification bug.
Depends on: 737040
I couldn't find any bug filed for this issue, so I filed bug #737040.
blocking-fennec1.0: --- → ?
smaug points to MozScrolledAreaChanged, which seems to be exactly what we want... Seeing if it works adequately now.
No longer depends on: 737040
Attachment #607200 - Flags: review?(bugmail.mozilla)
kats noticed a silly mistake I'd made, re-uploading with this corrected and some extra comments.
Attachment #607200 - Attachment is obsolete: true
Attachment #607200 - Flags: review?(bugmail.mozilla)
Attachment #607210 - Flags: review?(bugmail.mozilla)
Comment on attachment 607210 [details] [diff] [review]
Update display-port when document size changes (corrected)

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

r=me with comment addressed.

::: mobile/android/chrome/content/browser.js
@@ +1491,5 @@
>      this.browser.addEventListener("DOMTitleChanged", this, true);
>      this.browser.addEventListener("DOMWindowClose", this, true);
>      this.browser.addEventListener("DOMWillOpenModalDialog", this, true);
>      this.browser.addEventListener("scroll", this, true);
> +    this.browser.addEventListener("MozScrolledAreaChanged", this, true);

Add a removeEventListener call as well in the destroy function. (I don't know if that even makes a difference, but all the other events do it...)
Attachment #607210 - Flags: review?(bugmail.mozilla) → review+
hmm, I just noticed that the patch fixes the issue on engadget.com, but doesn't on taskjs.org... I'm just looking to see if I can find out why...
blocking-fennec1.0: ? → beta+
Priority: -- → P2
Argh, so the problem of page-size not being updated in this patch is causing a bad display-port to be set after the correct one (I think). Looks like I can't let the page-size be incorrect for that short time.
Ok, the problem ended up not being that, though I've fixed that anyway - the problem was the call that resets the display-port on first-paint. I don't think this is necessary anymore, so I'll remove it.
kats pointed out on IRC that this was actually conflating two separate issues - display-port on page-size changes, and display-port before first-paint. My original patch fixed the former, but not the latter - after some discussion on IRC, here is a patch that fixes both situations.
Attachment #607210 - Attachment is obsolete: true
Attachment #607569 - Flags: review?(bugmail.mozilla)
m-c had changed significantly underneath me, so here's the rebased patch.

Note, I'm not sure if this builds yet (as it takes me quite a while to rebuild after reconfiguring), so there may be some typo-related issues here... Hoping that it's in a good enough state to review though.
Attachment #607569 - Attachment is obsolete: true
Attachment #607569 - Flags: review?(bugmail.mozilla)
Attachment #607578 - Flags: review?(bugmail.mozilla)
Comment on attachment 607578 [details] [diff] [review]
Fix displayport when page size changes and before a document is displayed (rebased)

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

r- for the incorrect display port calculation, but I'd be happy to be proven wrong on that.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +252,5 @@
> +            newMetrics.setPageSize(pageSize);
> +            if (update)
> +                mLayerController.setViewportMetrics(newMetrics);
> +        }
> +        mReturnDisplayPort = calculateDisplayPort(new ImmutableViewportMetrics(newMetrics));

At this point, if handlePageSizeMessage is called during page load before the first paint, newMetrics would have the scroll coordinates from the "old" page (because that's what mLayerController.getViewportMetrics()) has, combined with the page size for the new page. This means the display port calculated will be wrong.

I think a better solution is to just roll the page size messages into the viewport messages; they are almost identical except for two things. (1) In the page size version, we don't update the scroll position on mLayerController's viewport metrics and (2) in the page size version we don't call abortPanZoomAnimation. I think it would be cleaner to do this via an if-condition in the Viewport:Update handler.

::: mobile/android/chrome/content/browser.js
@@ +1620,5 @@
>      win.scrollTo(x, y);
>      this.userScrollPos.x = win.scrollX;
>      this.userScrollPos.y = win.scrollY;
>      this.setResolution(aViewport.zoom, false);
> +

nit: Remove extra line

@@ +1886,5 @@
> +        // Just make sure it's the event for the correct root scroll frame.
> +        if (aEvent.originalTarget != this.browser.contentDocument)
> +          return;
> +
> +        let doc = aEvent.originalTarget;

Don't need "doc" here.

@@ -2197,5 @@
> -          // The document element must have a display port on it whenever we are about to
> -          // paint. This is the point just before the first paint, so we set the display port
> -          // to a default value here. Once Java is aware of this document it will overwrite
> -          // it with a better-calculated display port.
> -          this.setDisplayPort(0, 0, {left: 0, top: 0, right: gScreenWidth, bottom: gScreenHeight });

Are we guaranteed to always get a MozScrollAreaChanged before this code runs? Because if we don't then we won't be setting a display port for the initial draw of this page, which is Very Bad (TM).
Attachment #607578 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (:kats) from comment #14)
> Comment on attachment 607578 [details] [diff] [review]
> Fix displayport when page size changes and before a document is displayed
> (rebased)
> 
> Review of attachment 607578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the incorrect display port calculation, but I'd be happy to be proven
> wrong on that.
> 
> ::: mobile/android/base/gfx/GeckoLayerClient.java
> @@ +252,5 @@
> > +            newMetrics.setPageSize(pageSize);
> > +            if (update)
> > +                mLayerController.setViewportMetrics(newMetrics);
> > +        }
> > +        mReturnDisplayPort = calculateDisplayPort(new ImmutableViewportMetrics(newMetrics));
> 
> At this point, if handlePageSizeMessage is called during page load before
> the first paint, newMetrics would have the scroll coordinates from the "old"
> page (because that's what mLayerController.getViewportMetrics()) has,
> combined with the page size for the new page. This means the display port
> calculated will be wrong.
> 
> I think a better solution is to just roll the page size messages into the
> viewport messages; they are almost identical except for two things. (1) In
> the page size version, we don't update the scroll position on
> mLayerController's viewport metrics and (2) in the page size version we
> don't call abortPanZoomAnimation. I think it would be cleaner to do this via
> an if-condition in the Viewport:Update handler.

I think you're right, will do.

> ::: mobile/android/chrome/content/browser.js
> @@ +1620,5 @@
> >      win.scrollTo(x, y);
> >      this.userScrollPos.x = win.scrollX;
> >      this.userScrollPos.y = win.scrollY;
> >      this.setResolution(aViewport.zoom, false);
> > +
> 
> nit: Remove extra line

Whoops, will remove.

> @@ +1886,5 @@
> > +        // Just make sure it's the event for the correct root scroll frame.
> > +        if (aEvent.originalTarget != this.browser.contentDocument)
> > +          return;
> > +
> > +        let doc = aEvent.originalTarget;
> 
> Don't need "doc" here.

ditto

> @@ -2197,5 @@
> > -          // The document element must have a display port on it whenever we are about to
> > -          // paint. This is the point just before the first paint, so we set the display port
> > -          // to a default value here. Once Java is aware of this document it will overwrite
> > -          // it with a better-calculated display port.
> > -          this.setDisplayPort(0, 0, {left: 0, top: 0, right: gScreenWidth, bottom: gScreenHeight });
> 
> Are we guaranteed to always get a MozScrollAreaChanged before this code
> runs? Because if we don't then we won't be setting a display port for the
> initial draw of this page, which is Very Bad (TM).

I don't know whether it's guaranteed, but it seems to be from my limited testing. What's wrong with not having a display-port set though? Setting a display-port that's the size of the window (we really ought to rename gScreenWidth/Height...) ought to provide the same behaviour as not having one set at all?
This addresses all review comments, hopefully.
Attachment #607578 - Attachment is obsolete: true
Attachment #607635 - Flags: review?(bugmail.mozilla)
Comment on attachment 607635 [details] [diff] [review]
Fix displayport when page size changes and before a document is displayed (rebased, revised)

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

r- for the mGeckoViewport assignment getting taken out (presumably rebase error). I'm sure you could fix it up fine but this stuff can be tricky in subtle ways so I'd prefer to see another patch before approving. (As it is, I had to stare at this for a half hour before deciding it is correct).

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +250,5 @@
> +     */
> +    private enum ViewportMessageType {
> +        UPDATE,       // The viewport has changed and should be entirely updated
> +        PAGE_SIZE     // The viewport's page-size has changed
> +    }

Not sure why you used an enum instead of a simple boolean, but I don't particularly care either way.

@@ +255,5 @@
> +
> +    /** Viewport message handler. */
> +    private void handleViewportMessage(JSONObject message, ViewportMessageType type) throws JSONException {
> +        ViewportMetrics newMetrics;
> +        FloatSize pageSize = new FloatSize(message);

"pageSize" is not used, take it out. (Also if you were using it - FloatSize(JSONObject) reads the "width" and "height" properties from the message, not the "pageWidth" and "pageHeight")

@@ +273,5 @@
> +                newMetrics.setPageSize(messageMetrics.getPageSize());
> +            }
> +            mLayerController.setViewportMetrics(newMetrics);
> +        }
> +        mDisplayPort = calculateDisplayPort(new ImmutableViewportMetrics(newMetrics));

Creating a new ImmutableViewportMetrics object here is kind of unnecessary; you could just use mLayerController.getViewportMetrics() which returns the immutable one created in the previous line. However if you do that make sure to move this line inside the synchronized block, because otherwise metrics could change in between. I'd actually prefer this line moved into the synchronized block regardless just for a little extra safety.

@@ -254,5 @@
> -                    mLayerController.post(new Runnable() {
> -                        public void run() {
> -                            mGeckoViewport = newMetrics;
> -                        }
> -                    });

This mGeckoViewport assignment is not present in the new code.

::: mobile/android/chrome/content/browser.js
@@ +2213,5 @@
> +          // almost certain a display-port has been set via the
> +          // MozScrolledAreaChanged event, make sure by sending a viewport
> +          // update here. As it's the first paint, this will end up being a
> +          // display-port request only.
> +          this.sendViewportUpdate();

Was there a reason you moved this to after the contentDocumentIsDisplayed assignment? I don't think it matters here but just wondering.
Attachment #607635 - Flags: review?(bugmail.mozilla) → review-
Sorry for wasting your time with the last one, that was way too rushed - here's one that actually does address all comments (I think).
Attachment #607635 - Attachment is obsolete: true
Attachment #607939 - Flags: review?(bugmail.mozilla)
Comment on attachment 607939 [details] [diff] [review]
Fix displayport when page size changes and before a document is displayed (rebased, revised)

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

r=me with nit fixed.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +270,5 @@
> +                break;
> +            case PAGE_SIZE:
> +                newMetrics = new ViewportMetrics(oldMetrics);
> +                newMetrics.setPageSize(messageMetrics.getPageSize());
> +            }

nit: Add a break statement after this case block.
Attachment #607939 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/d7aa02178f7f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
You need to log in before you can comment on or make changes to this bug.