Closed Bug 1131359 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

(Whiteboard: gfx-noted)

Attachments

(3 files, 2 obsolete files)

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.
Whiteboard: gfx-noted
I'm going to take this to advance apz-fennec work.
Assignee: nobody → botond
Bug 1131359 - Port the double-tap-to-zoom functionality of BrowserElementPanning.js to C++. r=kats
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.
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 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
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.
Cool, the shape of the new patch looks pretty good! I like how everything is well encapsulated.
Bug 1131359 - Expose a basic FrameMetrics calculations in nsLayoutUtils. r=kats
Attachment #8639499 - Flags: review?(bugmail.mozilla)
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
Attachment #8634219 - Flags: review?(bugmail.mozilla)
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.
(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
Bug 1131359 - Fix an include-what-you-use error in AsyncCompositionManager.cpp. r=kats
Attachment #8639614 - Flags: review?(bugmail.mozilla)
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
Attachment #8639499 - Flags: review?(bugmail.mozilla) → review+
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)
Attachment #8634219 - Flags: review?(bugmail.mozilla) → review+
Attachment #8639614 - Flags: review?(bugmail.mozilla) → review+
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
Blocks: 1188955
(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 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
Attachment #8639499 - Attachment is obsolete: true
Attachment #8634219 - Attachment is obsolete: true
Bug 1131359 - Expose a basic FrameMetrics calculations in nsLayoutUtils. r=kats
Attachment #8640580 - Flags: review?(bugmail.mozilla)
Bug 1131359 - Port the double-tap-to-zoom functionality of BrowserElementPanning.js to C++. r=kats
Attachment #8640581 - Flags: review?(bugmail.mozilla)
Comment on attachment 8640580 [details]
MozReview Request: Bug 1131359 - Expose a basic FrameMetrics calculations in nsLayoutUtils. r=kats

Rebased, carrying r+.
Attachment #8640580 - Flags: review?(bugmail.mozilla) → review+
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+.
Attachment #8640581 - Flags: review?(bugmail.mozilla) → review+
(Try pushes are in comment 11 and comment 12.)
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.