930995 - Make setting the display port in front end script sync

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Pan and Zoom, defect)

Description

[Jim Mathies [:jimm]]

When the apz sends a content repaint request [1] (which generally is a sync call unless there are multiple paint requests queued up in the apz paint throttler) widget fires an observer which front end picks up. The observer call is pretty low in terms of overhead and is synchronous. Once we receive this in the front end, we fire off an async message manager call over to content script that sets the actual display port [3].

We can avoid bouncing this through the Windows event queue by moving the logic in browser bindings over to apzc.js. This breaks remote content compatibility but we really don't care about that since we'll switch to using TabParent/TabChild when we turn on remote content, which already has this functionality built in.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/APZController.cpp#57
[2] http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/apzc.js#88
[3] http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/browser.js#592

Comment 1

[Jim Mathies [:jimm]]

Attachment: front end wip

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 823506 [details] [diff] [review]
front end wip

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

::: browser/metro/base/content/apzc.js
@@ +106,5 @@
> +
> +      let content = Browser.selectedBrowser.contentWindow;
> +      let rootCwu = Browser.selectedBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +      let displayport = new Rect(displayPort.x + scrollTo.x, displayPort.y + scrollTo.y, displayPort.width, displayPort.height);
> +      if (displayport.isEmpty()) {

Do you know if this actually happens? If the displayport provided here is empty then it's an APZC bug and I would like to know.

@@ +122,5 @@
> +
> +      if (!element) {
> +        return;
> +      }
> +

Note that this patch touches the same code as my patch in bug 902505, so the second one to land will need rebasing.

Comment 3

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Comment on attachment 823506 [details] [diff] [review]
> front end wip
> 
> Review of attachment 823506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/content/apzc.js
> @@ +106,5 @@
> > +
> > +      let content = Browser.selectedBrowser.contentWindow;
> > +      let rootCwu = Browser.selectedBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> > +      let displayport = new Rect(displayPort.x + scrollTo.x, displayPort.y + scrollTo.y, displayPort.width, displayPort.height);
> > +      if (displayport.isEmpty()) {
> 
> Do you know if this actually happens? If the displayport provided here is
> empty then it's an APZC bug and I would like to know.

No I don't, it's left over from older work. I'll look at it though as I try to move the code to widget.

Comment 4

[Jim Mathies [:jimm]]

Hey kats, had a question - in TabChild code [1] we use mScrollId to differentiate between the root frame and sub frames, and based on that call two different helpers [2],[3] to update the display port.

In metro the root scroll id of 1 identifies the scrollable frame for the first tab with subsequent tabs getting increasing scroll ids values. If you close the main tab, FrameMetrics::ROOT_SCROLL_ID no longer exists.

So we have a lot of special casing for ROOT_SCROLL_ID [4] that won't work in metro on subsequent tabs. Is metro broken in this respect and should we file a bug on tracking down why we loose the root scroll id, or is this expected?

In working ont he widget patch here, I'm noticing different behavior between the first tab and subsequent tabs, which I'm guessing is due to this odd scroll id issue.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1507
[2] http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/APZCCallbackHelper.cpp#28
[3] http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/APZCCallbackHelper.cpp#81
[4] http://mxr.mozilla.org/mozilla-central/ident?i=ROOT_SCROLL_ID

Comment 5

[Jim Mathies [:jimm]]

Attachment: back end wip

This is acting really strange at this point, with odd layout issues and repainting problems.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jim Mathies [:jimm] from comment #4)
> Hey kats, had a question - in TabChild code [1] we use mScrollId to
> differentiate between the root frame and sub frames, and based on that call
> two different helpers [2],[3] to update the display port.

Correct.

> In metro the root scroll id of 1 identifies the scrollable frame for the
> first tab with subsequent tabs getting increasing scroll ids values. If you
> close the main tab, FrameMetrics::ROOT_SCROLL_ID no longer exists.

So I'm surprised that you're seeing a scroll id of 1 at all. Metro should behave the same as Fennec in this respect, because they are both running content in-process. In Fennec I never see anything with a scroll id of 1, it just starts at 2 and goes up from there. I see the same in Metro with my logging, but I'm attaching VS after Metro has started, so maybe there's something transient that happens before?

> So we have a lot of special casing for ROOT_SCROLL_ID [4] that won't work in
> metro on subsequent tabs. Is metro broken in this respect and should we file
> a bug on tracking down why we loose the root scroll id, or is this expected?

I think this is actually expected. We do have a bug on file (bug 900092) for getting rid of the ROOT_SCROLL_ID entirely and just using a regular scroll id value. We should also be able to get rid of all of the special casing behaviour.

> In working ont he widget patch here, I'm noticing different behavior between
> the first tab and subsequent tabs, which I'm guessing is due to this odd
> scroll id issue.

Do you have STR for when you see the scrollId being 1? I'm not sure that should ever happen, and if it does, it certainly shouldn't happen on just one tab and not the rest.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> (In reply to Jim Mathies [:jimm] from comment #4)
> > In working ont he widget patch here, I'm noticing different behavior between
> > the first tab and subsequent tabs, which I'm guessing is due to this odd
> > scroll id issue.
> 
> Do you have STR for when you see the scrollId being 1? I'm not sure that
> should ever happen, and if it does, it certainly shouldn't happen on just
> one tab and not the rest.

In a debug build I get a scrollid of 1 for the first tab pretty reliably. Switching to the c++ display port code seems to improve the chances I'll get it.

Comment 8

[Jim Mathies [:jimm]]

Attachment: back end wip

Comment 9

[Jim Mathies [:jimm]]

Attachment: cleaned up wip

I think this is pretty much ready to go. Just want to do some additional testing and go over the patch looking for bugs.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 824621 [details] [diff] [review]
cleaned up wip

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

::: widget/windows/winrt/APZController.cpp
@@ +114,4 @@
>  
> +    // If we're dealing with a sub frame, call UpdateSubFrame.
> +    if (!IsTab(subDocument)) {
> +      nsCOMPtr<nsIContent> content = do_QueryInterface(subDocument->GetRootElement());

Subframes can be divs with overflow:scroll set too. They're not necessarily subdocuments.

Comment 11

[Jim Mathies [:jimm]]

I can fix that I think, if I return the element found via FindElementWithViewId in GetTargetSubDocument, that should be the element in question. Then I can pass that into UpdateSubFrame. I'll still need the owner doc for the tab check.

Comment 12

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 13

[Jim Mathies [:jimm]]

Attachment: patch v.1

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 824713 [details] [diff] [review]
patch v.1

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

LGTM. Please ensure that scrolling the subframes at http://people.mozilla.org/~kgupta/tmp/div.html and http://people.mozilla.org/~kgupta/tmp/iframe.html still work.

::: widget/windows/winrt/APZController.cpp
@@ +85,5 @@
> +  if (aSubDocument->GetRootElement() == domElement && IsTab(aSubDocument)) {
> +    aTargetElement = nullptr;
> +  }
> +
> +  return !!aSubDocument;

Can aSubDocument ever be null here? If it is then the ->GetRootElement() call a few lines above will crash. If not then you can just return true.

Comment 15

[Jim Mathies [:jimm]]

> Can aSubDocument ever be null here? If it is then the ->GetRootElement()
> call a few lines above will crash. If not then you can just return true.

Yes it can, I missed adding a check on that when I reorganized it a patch or so back. Updated locally.

Comment 16

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/55ebd7f6916f

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55ebd7f6916f

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/55ebd7f6916f

Comment 19

[Kartikaya Gupta (email:[e-mail])]

So this patch ended up bitrotting one of my patches [1] for bug 902505, which shouldn't have been a problem in theory, since APZCCallbackHelper should be setting the resolution in the right place anyway. However in practice it looks like this doesn't happen, and in order to continue working on bug 902505 I had to locally back out this patch and re-apply [1]. Even hacking around bug 934233 locally (by hard-coding IsTab to return true) didn't fix the problem for me; it looks like the code in APZController.cpp is finding the wrong elements and/or window utils, so APZCCallbackHelper ends up operating on the wrong thing. I haven't looked into why this happens because I've been trying to figure out the other problems I've had for bug 902505 but I wanted to put a note here since this will have to be addressed at some point before bug 902505 can land.

[1] https://bug902505.bugzilla.mozilla.org/attachment.cgi?id=823499

Comment 20

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> So this patch ended up bitrotting one of my patches [1] for bug 902505,
> which shouldn't have been a problem in theory, since APZCCallbackHelper
> should be setting the resolution in the right place anyway. However in
> practice it looks like this doesn't happen, and in order to continue working
> on bug 902505 I had to locally back out this patch and re-apply [1]. Even
> hacking around bug 934233 locally (by hard-coding IsTab to return true)
> didn't fix the problem for me; it looks like the code in APZController.cpp
> is finding the wrong elements and/or window utils, so APZCCallbackHelper
> ends up operating on the wrong thing. I haven't looked into why this happens
> because I've been trying to figure out the other problems I've had for bug
> 902505 but I wanted to put a note here since this will have to be addressed
> at some point before bug 902505 can land.
> 
> [1] https://bug902505.bugzilla.mozilla.org/attachment.cgi?id=823499

I have bug 934233 filed on IsTab missing tabs with certain pages, which is probably what you're running into.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

As I mentioned in comment 19, I hacked around bug 934233 but it didn't quite solve the problem. I suspect both but 934233 and the issues I'm seeing are related to the thing you mentioned earlier where the first tab has a scrollId of 1 but then subsequent tabs do not.

The scrollId of 1 is special and if the first tab is getting that scrollId then that means it is being treated specially, probably in the front-end code. Do you know if that is the case? The sort of thing I would expect to cause this is if the first page is loaded right into the XUL window rather than into the deck or whatever the front end code is using to hold the browser tabs.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Actually I think I might know what's going on here. I will confirm and comment over on bug 934233.