1131359 - Port the code in BrowserElementPanning.js for handling double-taps to C++

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

The relevant code in BEP.js:

  - maintains some state about viewport metrics
  - listens to "Viewport:Change" messages and updates this state
  - listens to "Gesture:DoubleTap" message; upon receiving one:
     - uses the double-tap coordinates, together with its
       stored metrics, to decide on an area of the screen to
       zoom in on
     - sends a browser-zoom-to-rect message with that area

Porting this code to C++ should be the final piece of the puzzle (along with bug 1131358) to get rid of BEP.js.

Comment 1

[Botond Ballo [:botond]]

I'm going to take this to advance apz-fennec work.

Comment 2

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1131359 - Port the double-tap-to-zoom functionality of BrowserElementPanning.js to C++. r=kats

Bug 1131359 - Port the double-tap-to-zoom functionality of BrowserElementPanning.js to C++. r=kats

Comment 3

[Botond Ballo [:botond]]

This is how I envision the component that replaces the JS double-tap-to-zoom code to look at the interface level and fit into the surrounding code. The actual implementation of its functionality remains to be done.

Comment 4

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

https://reviewboard.mozilla.org/r/13347/#review11981

::: gfx/layers/apz/util/DoubleTapToZoomer.h:23
(Diff revision 1)
> +  void NotifyViewportChange(const CSSIntPoint& aScrollOffset,

This looks pretty reasonable. I think we might even be able to get rid of the NotifyViewportChange API and just pass in the scrollId to ProcessDoubleTap. ProcessDoubleTap can then use that to get whatever it needs. It would save having to keep pushing updates via NotifyViewportChange when most of the time they're not really needed.

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8634219 [details]
MozReview Request: Bug 1131359 - Port the double-tap-to-zoom functionality of BrowserElementPanning.js to C++. r=kats

Bug 1131359 - Port the double-tap-to-zoom functionality of BrowserElementPanning.js to C++. r=kats

Comment 6

[Botond Ballo [:botond]]

Posting what I have so far. I haven't had a chance to test it yet, but all the pieces should be in place.

Kats, you were right that we could get rid of NotifyViewportChange altogether; it simplified things quite a bit, especially now that with bug 1178847 landed, RefreshViewportSize() would have had to call it too.

Comment 7

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

Cool, the shape of the new patch looks pretty good! I like how everything is well encapsulated.

Comment 8

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1131359 - Expose a basic FrameMetrics calculations in nsLayoutUtils. r=kats

Bug 1131359 - Expose a basic FrameMetrics calculations in nsLayoutUtils. r=kats

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8634219 [details]
MozReview Request: Bug 1131359 - Port the double-tap-to-zoom functionality of BrowserElementPanning.js to C++. r=kats

Bug 1131359 - Port the double-tap-to-zoom functionality of BrowserElementPanning.js to C++. r=kats

Comment 10

[Botond Ballo [:botond]]

The updated patches fix some issues found during testing and clean up the code a bit.

Some commentary:

  - The purpose of exposing CalculateFrameMetricsForDisplayPort() (renamed to
    CalculateBasicFrameMetrics()) is to avoid duplicating the code that
    calculates the composition size in the CSS pixels of the scrolled content.
    (Trying to just use nsIScrolledFrame::CalculateCompositionSizeForFrame()
    as I did originally is wrong, because that returns the composition size
    in the CSS pixels of the parent frame).

  - I did some manual testing of the patch and it seems to be working well
    (matching the previous behaviour) well. I intend to turn some of my test
    cases into mochitests as a follow-up.

  - I tried to follow the calculations done in BrowserElementPanning.js as
    closely as possible, but there are two cases where I deviated from it to
    do things more efficiently:

     (1) ElementTouchHelper.anyElementFromPoint() would call 
         nsDocument::ElementFromPoint(), which calls 
         nsLayoutUtils::GetFrameForPoint() with IGNORE_CROSS_DOC to avoid
         recursing into subdocuments, and then recurse into subdocuments
         manually.

         In the C++ port, I just called GetFrameForPoint() directly without
         IGNORE_CROSS_DOC, to get the recursion into subdocuments for free.


     (2) ElementTouchHelper.getBoundingContentRect() would call
         Element.getBoundingClientRect(), which returns a rect relative to
         the containing document, and then walk up the chain of iframes
         to convert that to a rect relative to the root content document.

         In the C++ port, I used layout APIs to get the rect relative to
         the root content document directly.

Comment 11

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceb1466d2779

Comment 12

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceb1466d2779

Had a unified build error on Windows, repushing build-only with fix:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7102d6892c3b

Comment 13

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1131359 - Fix an include-what-you-use error in AsyncCompositionManager.cpp. r=kats

Bug 1131359 - Fix an include-what-you-use error in AsyncCompositionManager.cpp. r=kats

Comment 14

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

Comment on attachment 8639499 [details]
MozReview Request: Bug 1131359 - Expose a basic FrameMetrics calculations in nsLayoutUtils. r=kats

https://reviewboard.mozilla.org/r/14205/#review12849

Comment 15

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

Comment on attachment 8634219 [details]
MozReview Request: Bug 1131359 - Port the double-tap-to-zoom functionality of BrowserElementPanning.js to C++. r=kats

https://reviewboard.mozilla.org/r/13347/#review12843

Looks pretty good - thanks for pointing out the differences to the JS version, it made it easier to review.

::: dom/ipc/TabChild.cpp:2152
(Diff revision 3)
> +    APZCCallbackHelper::GetOrCreateScrollIdentifiers(document->GetDocumentElement(), &presShellId, &viewId);

We should either check the return value of this call, or assert that it's true.

::: gfx/layers/apz/util/DoubleTapToZoom.h:23
(Diff revision 3)
> +CSSRect CalculateRectToZoomTo(const nsCOMPtr<nsIDocument>& aRootContentDocumentt,

extra trailing "t"

::: gfx/layers/apz/util/DoubleTapToZoom.cpp:120
(Diff revision 3)
> +  // Am empty rect as return value is interpreted as "zoom out".

s/Am/An/

::: gfx/layers/apz/util/DoubleTapToZoom.cpp:67
(Diff revision 3)
> +  if (nsCOMPtr<nsIDOMHTMLLIElement> li = do_QueryInterface(aElement)) {

Probably better to do aElement->IsAnyOfHTMLElements(nsGkAtoms::li, nsGkAtoms::quote)

Comment 16

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

Comment on attachment 8639614 [details]
MozReview Request: Bug 1131359 - Fix an include-what-you-use error in AsyncCompositionManager.cpp. r=kats

https://reviewboard.mozilla.org/r/14223/#review12851

Comment 17

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> I intend to turn some of my test cases into mochitests as a follow-up.

Filed bug 1188955 for this.

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8639614 [details]
MozReview Request: Bug 1131359 - Fix an include-what-you-use error in AsyncCompositionManager.cpp. r=kats

Bug 1131359 - Fix an include-what-you-use error in AsyncCompositionManager.cpp. r=kats

Comment 19

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1131359 - Expose a basic FrameMetrics calculations in nsLayoutUtils. r=kats

Bug 1131359 - Expose a basic FrameMetrics calculations in nsLayoutUtils. r=kats

Comment 20

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1131359 - Port the double-tap-to-zoom functionality of BrowserElementPanning.js to C++. r=kats

Bug 1131359 - Port the double-tap-to-zoom functionality of BrowserElementPanning.js to C++. r=kats

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8640580 [details]
MozReview Request: Bug 1131359 - Expose a basic FrameMetrics calculations in nsLayoutUtils. r=kats

Rebased, carrying r+.

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8640581 [details]
MozReview Request: Bug 1131359 - Port the double-tap-to-zoom functionality of BrowserElementPanning.js to C++. r=kats

Addressed review comments, carrying r+.

Comment 23

[Botond Ballo [:botond]]

(Try pushes are in comment 11 and comment 12.)

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2fc1cd10a39
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b0ef3b98b72
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d65dafbdce5

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/f2fc1cd10a39
https://hg.mozilla.org/mozilla-central/rev/4b0ef3b98b72
https://hg.mozilla.org/mozilla-central/rev/0d65dafbdce5