Closed Bug 895905 Opened 7 years ago Closed 6 years ago

Contents needs to inform APZC about scroll events on B2G

Categories

(Core :: Graphics: Layers, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: botond, Assigned: botond)

References

Details

(Whiteboard: [preview])

Attachments

(4 files, 13 obsolete files)

18.89 KB, patch
botond
: review+
Details | Diff | Splinter Review
6.38 KB, patch
botond
: review+
Details | Diff | Splinter Review
1.34 KB, patch
botond
: review+
Details | Diff | Splinter Review
1.71 KB, patch
botond
: review+
Details | Diff | Splinter Review
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.
Depends on: 859929
Blocks: 895358
Assignee: nobody → botond
Attached patch bug895905.patch (obsolete) — Splinter Review
Attachment #781782 - Flags: review?(bugmail.mozilla)
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.
Attachment #781782 - Flags: review?(bugmail.mozilla) → review-
Attached patch bug895905.patch (obsolete) — Splinter Review
updated patch
Attachment #781782 - Attachment is obsolete: true
Attachment #781846 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attached patch small documentation improvement (obsolete) — Splinter Review
This one-line patch adds a comment to nsPoint.h documenting that nsPoint and nsIntPoint store coordinates in app units.
Attachment #781855 - Flags: review?(tnikkel)
Attachment #781855 - Attachment description: bug895905a.patch → small documentation improvement
Comment on attachment 781855 [details] [diff] [review]
small documentation improvement

nsPoint is app units, but nsIntPoint is (one of the types of) pixels.
Corrected documentation patch.
Attachment #781855 - Attachment is obsolete: true
Attachment #781855 - Flags: review?(tnikkel)
Attachment #781873 - Flags: review?(tnikkel)
(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.
Blocks: 898580
Comment on attachment 781873 [details] [diff] [review]
Part 4 - Small documentation improvement

Thanks!
Attachment #781873 - Flags: review?(tnikkel) → review+
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.
Depends on: 866232
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
Attachment #781846 - Flags: review?(bugmail.mozilla) → review+
Attached patch bug895905.patch (obsolete) — Splinter Review
Attachment #781846 - Attachment is obsolete: true
Attachment #782576 - Flags: review+
Whiteboard: [preview]
Attached patch bug895905-multi.patch (obsolete) — Splinter Review
Updated patch to handle multi-APZC.
Attachment #782576 - Attachment is obsolete: true
Attachment #785045 - Flags: review?(bugmail.mozilla)
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).
Attachment #785045 - Flags: review?(bugmail.mozilla) → review-
Updated patch to address review comments.
Attachment #785045 - Attachment is obsolete: true
Attachment #786342 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(bugs)
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)
Flags: needinfo?(bugs)
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.
Attachment #786342 - Flags: review?(bugmail.mozilla) → review+
Attached patch bug895905-multi-part2.patch (obsolete) — Splinter Review
This patch simplifies some of the added code in TabChild by eliminating some of the special handling of ROOT_SCROLL_ID.
Attachment #786514 - Flags: review?(bugmail.mozilla)
Attached patch bug895905-multi-part2.patch (obsolete) — Splinter Review
Removed some debug output that snuck into the last patch.
Attachment #786514 - Attachment is obsolete: true
Attachment #786514 - Flags: review?(bugmail.mozilla)
Attachment #786515 - Flags: review?(bugmail.mozilla)
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?
Attached patch bug895905-multi-part2.patch (obsolete) — Splinter Review
Attachment #786515 - Attachment is obsolete: true
Attachment #786515 - Flags: review?(bugmail.mozilla)
Attachment #787008 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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.
Attachment #787008 - Flags: review?(tnikkel)
Attachment #787008 - Flags: review?(bugmail.mozilla)
Attachment #787008 - Flags: review+
Comment on attachment 787008 [details] [diff] [review]
bug895905-multi-part2.patch

Should be fine.
Attachment #787008 - Flags: review?(tnikkel) → review+
Attached patch fix a compiler error on GCC 4.7 (obsolete) — Splinter Review
Fixed a compiler error related to name lookup on GCC 4.7.
Attachment #787064 - Flags: review?(bugmail.mozilla)
Attachment #787064 - Flags: review?(bugmail.mozilla) → review?(bas)
Updated patch to fix a assertion failure.
Attachment #787008 - Attachment is obsolete: true
Attachment #787791 - Flags: review?(bugmail.mozilla)
Try results: https://tbpl.mozilla.org/?tree=Try&rev=573f63e89480

Tested this on B2G after the multi-APZC regression fixes landed, looks good.
Attachment #787791 - Flags: review?(bugmail.mozilla) → review+
Update patch to gfx/2d/Point.h: instead of #including nsMathUtils.h, don't use NS_lround() in the file.
Attachment #787064 - Attachment is obsolete: true
Attachment #787064 - Flags: review?(bas)
Attachment #790349 - Flags: review?(bas)
Attachment #786342 - Attachment description: bug895905-multi.patch → Part 1 - Inform APZC about scroll events on B2G
Attachment #787791 - Attachment description: bug895905-multi-part2.patch → Part 2 - Eliminate some of the special handling of ROOT_SCROLL_ID
Attachment #790349 - Attachment description: bug895905-multi-gcc47fix.patch → Part 3 - Fix a compiler error on GCC 4.7
Attachment #781873 - Attachment description: small documentation improvement → Part 4 - Small documentation improvement
Attachment #790349 - Flags: review?(bas) → review+
Attachment #786342 - Attachment is obsolete: true
Attachment #790902 - Flags: review+
Attachment #790349 - Attachment is obsolete: true
Attachment #790911 - Flags: review+
Attachment #781873 - Attachment is obsolete: true
Attachment #790913 - Flags: review+
Rebased all patches to apply to latest central. Carrying r+ for them.
Keywords: checkin-needed
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.
(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"?
Blocks: 780395
You need to log in before you can comment on or make changes to this bug.