895905 - Contents needs to inform APZC about scroll events on B2G

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

As a result of the fix to bug 859929, APZC needs to be informed about scroll events. This was implemented for Fenec in bug 859929, but it needs to be implemented for B2G as well.

Comment 1

[Botond Ballo [:botond]]

Attachment: bug895905.patch

Comment 2

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

Comment on attachment 781782 [details] [diff] [review]
bug895905.patch

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

Looks good overall, just a few comments below I'd like to see addressed.

I assume you'll do the metro hookup as part of a follow-up or bump that to Brian? Either of those is fine as long as it's clear who will do that.

::: dom/ipc/TabChild.cpp
@@ +312,5 @@
> +    nsCOMPtr<nsIDOMWindowUtils> utils = GetDOMWindowUtils();
> +    utils->GetScrollXY(false, &scrollX, &scrollY);
> +    CSSIntPoint scrollOffset(scrollX, scrollY);
> +    CSSIntPoint lastScrollOffset = CSSIntPoint::FromAppUnitsRounded(
> +        CSSPoint::ToAppUnits(mLastMetrics.mScrollOffset));

You can use RoundedToInt(mLastMetrics.mScrollOffset) here. RoundedToInt is defined in gfx/2d/Point.h

@@ +315,5 @@
> +    CSSIntPoint lastScrollOffset = CSSIntPoint::FromAppUnitsRounded(
> +        CSSPoint::ToAppUnits(mLastMetrics.mScrollOffset));
> +    if (lastScrollOffset != scrollOffset) {
> +      // Update mLastMetrics.mScrollOffset now, otherwise RecvUpdateDimensions()
> +      // might trigger scroll to the olf offset before RecvUpdateFrame() gets a

s/olf/old/

@@ +317,5 @@
> +    if (lastScrollOffset != scrollOffset) {
> +      // Update mLastMetrics.mScrollOffset now, otherwise RecvUpdateDimensions()
> +      // might trigger scroll to the olf offset before RecvUpdateFrame() gets a
> +      // chance to update it.
> +      mLastMetrics.mScrollOffset = CSSPoint(scrollX, scrollY);

you should just be able put scrollOffset on the RHS of this assignment. There's an implicit constructor that will do the conversion.

@@ +1566,5 @@
> +    // in mLastMetrics is the same as the offset stored in the window,
> +    // re-query the latter.
> +    int32_t actualScrollX, actualScrollY;
> +    window->GetScrollX(&actualScrollX);
> +    window->GetScrollY(&actualScrollY);

I'd rather have this call util->GetScrollXY for consistency with the other bit of code above. That way if GetScrollX for whatever reason returns something different GetScrollXY we'll still be covered.

Comment 3

[Botond Ballo [:botond]]

Attachment: bug895905.patch

updated patch

Comment 4

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> I assume you'll do the metro hookup as part of a follow-up or bump that to
> Brian? Either of those is fine as long as it's clear who will do that.

I will file a new bug for that (since the Fennec and B2G hookups had different bugs, I think it's consistent that the Metro hookup gets one too), and take it once I get my Metro device. Of course, I'm cool with Brian taking it if he'd like.

The other comments are addressed by the updated patch.

Comment 5

[Botond Ballo [:botond]]

Attachment: small documentation improvement

This one-line patch adds a comment to nsPoint.h documenting that nsPoint and nsIntPoint store coordinates in app units.

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 781855 [details] [diff] [review]
small documentation improvement

nsPoint is app units, but nsIntPoint is (one of the types of) pixels.

Comment 7

[Botond Ballo [:botond]]

Attachment: Part 4 - Small documentation improvement

Corrected documentation patch.

Comment 8

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #6)
> Comment on attachment 781855 [details] [diff] [review]
> small documentation improvement
> 
> nsPoint is app units, but nsIntPoint is (one of the types of) pixels.

Thanks for pointing that out. I had incorrectly ascribed properties of the CSSPoint/CSSIntPoint etc. pairs to nsPoint/nsIntPoint in my mind.

I updated the documentation patch accordingly.

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 781873 [details] [diff] [review]
Part 4 - Small documentation improvement

Thanks!

Comment 10

[Botond Ballo [:botond]]

Note that this patch will need to be revised for multi-APZC. Here's an outline of how it can be done:
  - The UpdateScrollOffset message from TabChild to TabParent can be modified to 
    include the view ID corresponding to the scroll frame that was scrolled.
  - When sending the message, TabChild can get the correct view ID from the event
    target of the scroll event it received, and the correct scroll offset by
    finding the scroll frame corresponding to that view ID.
  - When receiving the message, TabParent can dispatch it to the correct APZC
    based on the view ID in the message.

Jeff also suggested moving the communication from TabChild->TabParent to CompositorChild->CompositorParent to reduce the amount of locking that is done in the parent process (which will become more significant when there will be a tree of APZCs rather than just one). I believe this is independent of the above.

I will wait for bug 866232 to land before making these revisions.

Comment 11

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

Comment on attachment 781846 [details] [diff] [review]
bug895905.patch

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

Looks good, thanks!

::: dom/ipc/TabParent.cpp
@@ +1488,5 @@
> +  }
> +  return true;
> +}
> +
> +

nit: One blank line here is enough

::: layout/ipc/RenderFrameParent.cpp
@@ +1000,5 @@
> +    mPanZoomController->UpdateScrollOffset(aScrollOffset);
> +  }
> +}
> +
> +

ditto

Comment 12

[Botond Ballo [:botond]]

Attachment: bug895905.patch

Comment 13

[Botond Ballo [:botond]]

Attachment: bug895905-multi.patch

Updated patch to handle multi-APZC.

Comment 14

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

Comment on attachment 785045 [details] [diff] [review]
bug895905-multi.patch

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

Some minor things noted below, but otherwise looks good. I'd like to see an updated patch with the comments addressed though, so minusing for now.

::: dom/ipc/PBrowser.ipdl
@@ +284,5 @@
>      UpdateZoomConstraints(bool aAllowZoom, float aMinZoom, float aMaxZoom);
>  
> +    /**
> +     * Notifies the parent about a scroll event. The pres shell ID and
> +     * view ID identify which scrollable (sub-)frame was scrolled, and 

nit: remove trailing whitespace

::: dom/ipc/TabChild.cpp
@@ +359,5 @@
> +    } else if ((content = do_QueryInterface(target))) {
> +      // Case 3: Content.
> +      nsCOMPtr<nsIDOMWindowUtils> utils = ::GetDOMWindowUtils(content);
> +      utils->GetPresShellId(&presShellId);
> +      NS_ENSURE_STATE(nsLayoutUtils::FindIDFor(content, &viewId));

I'm not sure NS_ENSURE_STATE is appropriate here. According to the docs in nsDebug.h, it's meant to be used "for checking state and arguments upon entering interface boundaries" which is not the case here.

Also while NS_ENSURE_STATE appears to be compiled in debug mode, often macros like this are #define'd to be no-ops in debug mode so it's good practice to call the function outside of the macro and then just check the return value in the macro, otherwise the entire function call disappears.

@@ +2248,5 @@
>      NS_ENSURE_TRUE(root, false);
>      root->SetParentTarget(scope);
>  
>      chromeHandler->AddEventListener(NS_LITERAL_STRING("DOMMetaAdded"), this, false);
> +    chromeHandler->AddEventListener(NS_LITERAL_STRING("scroll"), this, false);

I'm a little surprised that the listener (for DOMMetaAdded) isn't removed in RecvDestroy. I wonder if that's a bug.

::: dom/ipc/TabChild.h
@@ -426,5 @@
>      // Get the DOMWindowUtils for the top-level window in this tab.
>      already_AddRefed<nsIDOMWindowUtils> GetDOMWindowUtils();
> -    // Get the DOMWindowUtils for the window corresponding to the givent content
> -    // element. This might be an iframe inside the tab, for instance.
> -    already_AddRefed<nsIDOMWindowUtils> GetDOMWindowUtils(nsIContent* aContent);

Why remove this from TabChild?

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1475,5 @@
>  }
>  
>  bool AsyncPanZoomController::Matches(const ScrollableLayerGuid& aGuid)
>  {
> +  // TODO: also check the presShellId, once that is fully propagated 

nit: trailing whitespace

::: layout/base/nsLayoutUtils.h
@@ +65,5 @@
>  public:
>    typedef mozilla::layers::FrameMetrics::ViewID ViewID;
>  
>    /**
> +   * Finds previously assigned ViewID for the given content element.

Update comment to indicate what the return value means (it's pretty obvious, but still good practice).

Comment 15

[Botond Ballo [:botond]]

Attachment: Part 1 - Inform APZC about scroll events on B2G

Updated patch to address review comments.

Comment 16

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> Comment on attachment 785045 [details] [diff] [review]
> ::: dom/ipc/TabChild.h
> @@ -426,5 @@
> >      // Get the DOMWindowUtils for the top-level window in this tab.
> >      already_AddRefed<nsIDOMWindowUtils> GetDOMWindowUtils();
> > -    // Get the DOMWindowUtils for the window corresponding to the givent content
> > -    // element. This might be an iframe inside the tab, for instance.
> > -    already_AddRefed<nsIDOMWindowUtils> GetDOMWindowUtils(nsIContent* aContent);
> 
> Why remove this from TabChild?

Unlike the no-argument overload, this overload didn't use any state from TabChild, so it could be made a non-member and moved into the cpp file.

> 
> @@ +2248,5 @@
> >      NS_ENSURE_TRUE(root, false);
> >      root->SetParentTarget(scope);
> >  
> >      chromeHandler->AddEventListener(NS_LITERAL_STRING("DOMMetaAdded"), this, false);
> > +    chromeHandler->AddEventListener(NS_LITERAL_STRING("scroll"), this, false);
> 
> I'm a little surprised that the listener (for DOMMetaAdded) isn't removed in
> RecvDestroy. I wonder if that's a bug.

I wonder too. Let's ask smaug.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

It shouldn't be a problem, since the listener is added to windowroot, and TabChild itself shouldn't
keep windowroot alive. So there isn't even a cycle to collect, as far as I see.
In fact, we should see leaks in debug builds if there was a leak.

The event listener will be removed when cycle collector runs after tab close and notices that
WindowRoot can be collected.

(WindowRoot is the "chromeEventHandler" in this case)

Comment 18

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

Comment on attachment 786342 [details] [diff] [review]
Part 1 - Inform APZC about scroll events on B2G

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

Looks good then.

Comment 19

[Botond Ballo [:botond]]

Attachment: bug895905-multi-part2.patch

This patch simplifies some of the added code in TabChild by eliminating some of the special handling of ROOT_SCROLL_ID.

Comment 20

[Botond Ballo [:botond]]

Attachment: bug895905-multi-part2.patch

Removed some debug output that snuck into the last patch.

Comment 21

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

Comment on attachment 786515 [details] [diff] [review]
bug895905-multi-part2.patch

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

::: layout/base/nsDisplayList.cpp
@@ +1158,5 @@
> +
> +      // Record the mapping between the root scroll frame's content and
> +      // ROOT_SCROLL_ID so that users of nsLayoutUtils::FindIDFor() and
> +      // nsLayoutUtils::FindContentFor() don't have to special-case the root.
> +      nsLayoutUtils::FindOrCreateIDFor(content, true);

I feel like this should be in an "if (id == FrameMetrics::ROOT_SCROLL_ID)" (or equivalently, "if (presContext->IsRootContentDocument())" guard. I think this code path can get executed for cases where we are not at the root content document and in those cases you don't want to be passing true here. Does that make sense?

Comment 22

[Botond Ballo [:botond]]

Attachment: bug895905-multi-part2.patch

Comment 23

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> ::: layout/base/nsDisplayList.cpp
> @@ +1158,5 @@
> > +
> > +      // Record the mapping between the root scroll frame's content and
> > +      // ROOT_SCROLL_ID so that users of nsLayoutUtils::FindIDFor() and
> > +      // nsLayoutUtils::FindContentFor() don't have to special-case the root.
> > +      nsLayoutUtils::FindOrCreateIDFor(content, true);
> 
> I feel like this should be in an "if (id == FrameMetrics::ROOT_SCROLL_ID)"
> (or equivalently, "if (presContext->IsRootContentDocument())" guard. I think
> this code path can get executed for cases where we are not at the root
> content document and in those cases you don't want to be passing true here.
> Does that make sense?

Good catch, thanks! Fixed in updated patch.

Comment 24

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

Comment on attachment 787008 [details] [diff] [review]
bug895905-multi-part2.patch

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

Looks fine to me. Also tagging :tn for review in case there's some reason this can't be done that I'm not aware of.

Comment 25

[Timothy Nikkel (:tnikkel)]

Comment on attachment 787008 [details] [diff] [review]
bug895905-multi-part2.patch

Should be fine.

Comment 26

[Botond Ballo [:botond]]

Attachment: fix a compiler error on GCC 4.7

Fixed a compiler error related to name lookup on GCC 4.7.

Comment 27

[Botond Ballo [:botond]]

Attachment: Part 2 - Eliminate some of the special handling of ROOT_SCROLL_ID

Updated patch to fix a assertion failure.

Comment 28

[Botond Ballo [:botond]]

Try results: https://tbpl.mozilla.org/?tree=Try&rev=573f63e89480

Tested this on B2G after the multi-APZC regression fixes landed, looks good.

Comment 29

[Botond Ballo [:botond]]

Attachment: Part 3 - Fix a compiler error on GCC 4.7

Update patch to gfx/2d/Point.h: instead of #including nsMathUtils.h, don't use NS_lround() in the file.

Comment 30

[Botond Ballo [:botond]]

Attachment: Part 1 - Inform APZC about scroll events on B2G

Comment 31

[Botond Ballo [:botond]]

Attachment: Part 2 - Eliminate some of the special handling of ROOT_SCROLL_ID

Comment 32

[Botond Ballo [:botond]]

Attachment: Part 3 - Fix a compiler error on GCC 4.7

Comment 33

[Botond Ballo [:botond]]

Attachment: Part 4 - Small documentation improvement

Comment 34

[Botond Ballo [:botond]]

Rebased all patches to apply to latest central. Carrying r+ for them.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8be61e224a9c
https://hg.mozilla.org/integration/b2g-inbound/rev/86305651c6b8
https://hg.mozilla.org/integration/b2g-inbound/rev/4d31a403f50f
https://hg.mozilla.org/integration/b2g-inbound/rev/15405875bf62

Comment 36

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

https://hg.mozilla.org/mozilla-central/rev/8be61e224a9c
https://hg.mozilla.org/mozilla-central/rev/86305651c6b8
https://hg.mozilla.org/mozilla-central/rev/4d31a403f50f
https://hg.mozilla.org/mozilla-central/rev/15405875bf62

Comment 37

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 790913 [details] [diff] [review]
Part 4 - Small documentation improvement

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

> Document that ns[Int]Point stores coordinates in app units.

In the future, please be more careful with your commit messages. This is not true about nsIntPoint, and doesn't reflect the patch.

Comment 38

[Botond Ballo [:botond]]

(In reply to :Ms2ger from comment #37)
> Comment on attachment 790913 [details] [diff] [review]
> Part 4 - Small documentation improvement
> 
> Review of attachment 790913 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > Document that ns[Int]Point stores coordinates in app units.
> 
> In the future, please be more careful with your commit messages. This is not
> true about nsIntPoint, and doesn't reflect the patch.

Sorry about that. I initially made the mistake of thinking nsIntPoint was also in app units, then corrected the patch but forgot to also correct the commit messages. I will certainly be more careful next time.

Is there a way to correct the commit message? Some equivalent of "svn propedit --revprop svn.log"?