Botond Ballo [:botond] Assignee
	Description • 11 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 2 • 11 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 3 • 11 years ago

updated patch

	Botond Ballo [:botond] Assignee
	Comment 4 • 11 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 5 • 11 years ago

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

	Timothy Nikkel (:tnikkel)
	Comment 6 • 11 years ago

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

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

	Botond Ballo [:botond] Assignee
	Comment 7 • 11 years ago

Corrected documentation patch.

	Botond Ballo [:botond] Assignee
	Comment 8 • 11 years ago

(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.

	Timothy Nikkel (:tnikkel)
	Comment 9 • 11 years ago

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

Thanks!

	Botond Ballo [:botond] Assignee
	Comment 10 • 11 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 11 • 11 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 13 • 11 years ago

Updated patch to handle multi-APZC.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 14 • 11 years ago

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).

	Botond Ballo [:botond] Assignee
	Comment 15 • 11 years ago

Updated patch to address review comments.

	Botond Ballo [:botond] Assignee
	Comment 16 • 11 years ago

(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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 11 years ago

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)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 18 • 11 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 19 • 11 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 20 • 11 years ago

Removed some debug output that snuck into the last patch.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 21 • 11 years ago

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?

	Botond Ballo [:botond] Assignee
	Comment 23 • 11 years ago

(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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 24 • 11 years ago

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.

	Timothy Nikkel (:tnikkel)
	Comment 25 • 11 years ago

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

Should be fine.

	Botond Ballo [:botond] Assignee
	Comment 26 • 11 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 27 • 11 years ago

Updated patch to fix a assertion failure.

	Botond Ballo [:botond] Assignee
	Comment 28 • 11 years ago

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

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

	Botond Ballo [:botond] Assignee
	Comment 29 • 11 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 34 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 11 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 36 • 11 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 37 • 11 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 38 • 11 years ago

(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"?