1188225 - Implement ChromeProcessController::HandleDoubleTap

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

For Fennec we'll need to implement the HandleDoubleTap callback in ChromeProcessController. It should be invoking the C++ code botond added in bug 1131359, possibly with a little bit of glue as needed.

Comment 1

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1188225-Implement-ChromeProcessController-HandleDoubleTap-15080712-37ea5c8.patch

Comment 2

[Randall Barker [:rbarker]]

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

Comment 3

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1188225-Implement-ChromeProcessController-HandleDoubleTap-15081114-a259310.patch

Rebased.

Comment 4

[Botond Ballo [:botond]]

Comment on attachment 8646586 [details] [diff] [review]
0001-Bug-1188225-Implement-ChromeProcessController-HandleDoubleTap-15081114-a259310.patch

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

Looks pretty good, thanks! I'd like to see an updated patch that addresses the comments before giving the r+.

::: gfx/layers/apz/util/APZCCallbackHelper.h
@@ +84,5 @@
>         that it accepts future scroll offset updates from APZ. */
>      static void AcknowledgeScrollUpdate(const FrameMetrics::ViewID& aScrollId,
>                                          const uint32_t& aScrollGeneration);
>  
> +    /* Get the nsIPresShell associated with the nsIContent */

"Get the pres shell associated with the root content document enclosing |aContent|."

::: gfx/layers/apz/util/ChromeProcessController.cpp
@@ +125,2 @@
>  already_AddRefed<nsIDOMWindowUtils>
>  ChromeProcessController::GetDOMWindowUtils() const

It appears no one calls this function - let's remove it.

@@ +144,5 @@
> +                          aPoint, aModifiers, aGuid));
> +    return;
> +  }
> +
> +  nsRefPtr<layers::APZCTreeManager> treeManager = CompositorParent::GetAPZCTreeManager(aGuid.mLayersId);

I'd rather have the ChromeProcessController hold a reference to the APZCTreeManager rather than look it up each time (both are unique per widget).

We can modify nsBaseWidget::CreateRootContentController() to take an APZCTreeManager* parameter, and pass in mAPZC when calling it in nsBaseWidget::ConfigureAPZCTreeManager().

@@ +150,5 @@
> +  if (treeManager.get()) {
> +    CSSPoint point = APZCCallbackHelper::ApplyCallbackTransform(aPoint, aGuid);
> +    nsCOMPtr<nsIDocument> document = GetRootContentDocument(aGuid.mScrollId);
> +    CSSRect zoomToRect = CalculateRectToZoomTo(document, point);
> +    treeManager->ZoomToRect(aGuid, zoomToRect);

We need to use the guid of the root content document's root scroll frame, rather than the guid that was passed in. Please see how TabChild does this [1].

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=abd120a5b640#1727

Comment 5

[Randall Barker [:rbarker]]

(In reply to Botond Ballo [:botond] from comment #4)
> Comment on attachment 8646586 [details] [diff] [review] 
> @@ +150,5 @@
> > +  if (treeManager.get()) {
> > +    CSSPoint point = APZCCallbackHelper::ApplyCallbackTransform(aPoint, aGuid);
> > +    nsCOMPtr<nsIDocument> document = GetRootContentDocument(aGuid.mScrollId);
> > +    CSSRect zoomToRect = CalculateRectToZoomTo(document, point);
> > +    treeManager->ZoomToRect(aGuid, zoomToRect);
> 
> We need to use the guid of the root content document's root scroll frame,
> rather than the guid that was passed in. Please see how TabChild does this
> [1].
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.
> cpp?rev=abd120a5b640#1727

I updated the HandleDoubleTap function as follows:

void                                                                             
ChromeProcessController::HandleDoubleTap(const mozilla::CSSPoint& aPoint,        
                                         Modifiers aModifiers,                   
                                         const ScrollableLayerGuid& aGuid)       
{                                                                                
  if (MessageLoop::current() != mUILoop) {                                       
    mUILoop->PostTask(                                                           
        FROM_HERE,                                                               
        NewRunnableMethod(this, &ChromeProcessController::HandleDoubleTap,       
                          aPoint, aModifiers, aGuid));                           
    return;                                                                      
  }                                                                              
                                                                                 
RLOG("HandleDoubleTap layer=%d ps=%d view=%d", (int)aGuid.mLayersId, (int)aGuid.mPresShellId, (int)aGuid.mScrollId);
  nsRefPtr<layers::APZCTreeManager> treeManager = CompositorParent::GetAPZCTreeManager(aGuid.mLayersId);
  if (!treeManager.get()) {                                                      
RLOG("No treeManager");                                                          
    return;                                                                      
  }                                                                              
                                                                                 
  nsCOMPtr<nsIDocument> document = GetRootContentDocument(aGuid.mScrollId);      
  if (!document.get()) {                                                         
RLOG("No document");                                                             
    return;                                                                      
  }                                                                              
                                                                                 
  CSSPoint point = APZCCallbackHelper::ApplyCallbackTransform(aPoint, aGuid);    
  CSSRect zoomToRect = CalculateRectToZoomTo(document, point);                   
                                                                                 
  uint32_t presShellId;                                                          
  FrameMetrics::ViewID viewId;                                                   
  if (APZCCallbackHelper::GetOrCreateScrollIdentifiers(                          
      nsLayoutUtils::FindContentFor(aGuid.mScrollId), &presShellId, &viewId)) {  
RLOG("ZoomToRect layer=%d ps=%d view=%d", (int)aGuid.mLayersId, (int)presShellId, (int)viewId);
    treeManager->ZoomToRect(                                                     
      ScrollableLayerGuid(aGuid.mLayersId, presShellId, viewId), zoomToRect);    
  }                                                                              
}

However, the values I get back from APZCCallbackHelper::GetOrCreateScrollIdentifiers are the same presShellId and viewId as were contained in aGuid. Am I doing something wrong? In what cases would you expect them to be different?

Comment 6

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1188225-Implement-ChromeProcessController-HandleDoubleTap-15081216-097a933.patch

Updated to address comments.

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8647238 [details] [diff] [review]
0001-Bug-1188225-Implement-ChromeProcessController-HandleDoubleTap-15081216-097a933.patch

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

LGTM! r+ with the remaining (minor) comments addressed.

::: gfx/layers/apz/util/ChromeProcessController.cpp
@@ +32,4 @@
>    , mUILoop(MessageLoop::current())
>  {
>    // Otherwise we're initializing mUILoop incorrectly.
>    MOZ_ASSERT(NS_IsMainThread());

Please assert that aAPZCTreeManager is not null.

::: gfx/layers/apz/util/ChromeProcessController.h
@@ +6,5 @@
>  #ifndef mozilla_layers_ChromeProcessController_h
>  #define mozilla_layers_ChromeProcessController_h
>  
>  #include "mozilla/layers/GeckoContentController.h"
> +#include "mozilla/layers/APZCTreeManager.h"

It should be sufficient to just forward-declare APZCTreeManager in this file; there's no need to include APZCTreeManager.h. (Keeping header files lean in this way helps keep compile times down.)

::: widget/android/AndroidContentController.h
@@ +6,5 @@
>  #ifndef AndroidContentController_h__
>  #define AndroidContentController_h__
>  
>  #include "mozilla/layers/ChromeProcessController.h"
> +#include "mozilla/layers/APZCTreeManager.h"

Likewise here, it should be sufficient to forward-declare APZCTreeManager.

Comment 8

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1188225-Implement-ChromeProcessController-HandleDoubleTap-15081316-7008b80.patch

Carry forward r+ from :botond

Comment 9

[Randall Barker [:rbarker]]

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

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae48bb7aad1b

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/ae48bb7aad1b