1298886 - Zoom Level doesn't increase/decrease with fingers touch (with e10s on)

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Kanchan Kumari QA]

[ Tested Version]: 
Build Id 20160829030202
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

[Tested touch enabled platforms]: 
Surface Pro 4  
Aspire SW3-013 
ASUS TP500L    

[Pre-requisite]: 
javascript.options.asyncstack; false
browser.tabs.remote.force-enable; true
browser.tabs.remote.autostart.2; true 
layers.async-pan-zoom.enabled; true 
About:support
Multiprocess Windows 1/1 (Enabled by user)
Asynchronous Pan/Zoom wheel input enabled;

[Steps to reproduce]:
1.Launch Firefox on Nightly 51.0a1 by double tapping the icon with  finger	
2.Tap the Location bar and use the onscreen keyboard to type 
  https://en.wikipedia.org/wiki/Peach. 
3.Apply your fingers to increase the zoom level
4.Apply your fingers to decrease the zoom level	

[Expected result]:
Zoom level is increased/decreased smoothly.

[Actual result]:
With e10s enabled: Zoom level doesn’t get increased or decreased, page scrolls down when fingers are used to change the zoom level.

[Note]:
It works fine with e10s disabled.

Comment 1

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

Looking into fixing this.

Comment 2

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

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

Comment 3

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

Now with modifier support:

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

Comment 4

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

Attachment: Bug 1298886 - Convert the PinchGestureInput span fields from float to ParentLayerCoord.

Review commit: https://reviewboard.mozilla.org/r/80844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80844/

Comment 5

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

Attachment: Bug 1298886 - Fire magnify gesture events in the parent process if APZ is enabled but APZ zooming is disabled.

Review commit: https://reviewboard.mozilla.org/r/80846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80846/

Comment 6

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

Comment on attachment 8794358 [details]
Bug 1298886 - Fire magnify gesture events in the parent process if APZ is enabled but APZ zooming is disabled.

https://reviewboard.mozilla.org/r/80846/#review79516

::: gfx/layers/apz/util/APZCCallbackHelper.h:195
(Diff revision 1)
>      IsScrollInProgress(nsIScrollableFrame* aFrame);
> +
> +    /* Notify content of the progress of a pinch gesture that APZ won't do
> +     * zooming for (because the apz.allow_zooming pref is false). This function
> +     * will dispatch appropriate WidgetSimpleGestureEvent events to gecko.
> +     * The widget provided must be non-null.

Actually the widget can be null, and the code will handle it fine. I'll remove this line from the comment block.

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8794357 [details]
Bug 1298886 - Convert the PinchGestureInput span fields from float to ParentLayerCoord.

https://reviewboard.mozilla.org/r/80844/#review79552

::: gfx/layers/apz/src/GestureEventListener.cpp:52
(Diff revision 1)
> -float GetCurrentSpan(const MultiTouchInput& aEvent)
> +ParentLayerCoord GetCurrentSpan(const MultiTouchInput& aEvent)
>  {
>    const ParentLayerPoint& firstTouch = aEvent.mTouches[0].mLocalScreenPoint;
>    const ParentLayerPoint& secondTouch = aEvent.mTouches[1].mLocalScreenPoint;
>    ParentLayerPoint delta = secondTouch - firstTouch;
>    return delta.Length();

Can we change BasePoint::Length() to return Coord, while we're at it?

(I originally didn't do that for the reason mentioned in bug 1055741 comment 11, but I've since been convinced otherwise (bug 1055741 comment 12)).

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8794358 [details]
Bug 1298886 - Fire magnify gesture events in the parent process if APZ is enabled but APZ zooming is disabled.

https://reviewboard.mozilla.org/r/80846/#review79556

r+ on the APZ parts. Going to defer to :dvander on the IPC-related parts.

::: gfx/layers/apz/public/GeckoContentController.h:69
(Diff revision 1)
>                           uint64_t aInputBlockId) = 0;
>  
>    /**
> +   * When the apz.allow_zooming pref is set to false, the APZ will not
> +   * translate pinch gestures to actual zooming. Instead, it will call this
> +   * method to notify gecko of the pinch gesture, and allow it to deal with

deal with it

::: gfx/layers/apz/public/GeckoContentController.h:83
(Diff revision 1)
> +   *        For a SCALE event, this is the difference in span between the
> +   *        previous state and the new state.
> +   * @param aModifiers The keyboard modifiers depressed during the pinch.
> +   */
> +  virtual void NotifyPinchGesture(PinchGestureInput::PinchGestureType aType,
> +                                  const ScrollableLayerGuid& aGuid,

This alignment, where the second argument type is one column before the first argument type, elsewhere is a result of refactorings that changed method names. Let's please not propagate it as a deliberate style choice :)

Applies to other header files modified in the patch too.

::: gfx/layers/apz/public/GeckoContentController.h:84
(Diff revision 1)
> +   *        previous state and the new state.
> +   * @param aModifiers The keyboard modifiers depressed during the pinch.
> +   */
> +  virtual void NotifyPinchGesture(PinchGestureInput::PinchGestureType aType,
> +                                  const ScrollableLayerGuid& aGuid,
> +                                  LayoutDeviceCoord aSpan,

I'd call this aSpanChange

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8794358 [details]
Bug 1298886 - Fire magnify gesture events in the parent process if APZ is enabled but APZ zooming is disabled.

https://reviewboard.mozilla.org/r/80846/#review79566

Looks good on the IPC side.

::: gfx/layers/apz/util/ContentProcessController.cpp:154
(Diff revision 1)
> +                        const ScrollableLayerGuid& aGuid,
> +                        LayoutDeviceCoord aSpan,
> +                        Modifiers aModifiers)
> +{
> +  // This should never get called
> +  MOZ_ASSERT(false);

nit: MOZ_ASSERT_UNREACHABLE

::: gfx/layers/ipc/RemoteContentController.cpp:101
(Diff revision 1)
> +  // For now we only ever want to handle this NotifyPinchGesture message in
> +  // the parent process, even if the APZ is sending it to a content process.
> +
> +  // If we're in the GPU process, try to find a handle to the parent process
> +  // and send it there.
> +  if (XRE_GetProcessType() == GeckoProcessType_GPU) {

note: We have XRE_IsGPUProcess now

Comment 10

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

(In reply to Botond Ballo [:botond] from comment #7)
> Can we change BasePoint::Length() to return Coord, while we're at it?
> 
> (I originally didn't do that for the reason mentioned in bug 1055741 comment
> 11, but I've since been convinced otherwise (bug 1055741 comment 12)).

I filed bug 1305201 for this, in case it snowballs into something larger.

(In reply to Botond Ballo [:botond] from comment #8)
> > +  virtual void NotifyPinchGesture(PinchGestureInput::PinchGestureType aType,
> > +                                  const ScrollableLayerGuid& aGuid,
> 
> This alignment, where the second argument type is one column before the
> first argument type, elsewhere is a result of refactorings that changed
> method names. Let's please not propagate it as a deliberate style choice :)
> 
> Applies to other header files modified in the patch too.

I'm not sure what you mean by this - the argument types are all lined up on the same column. Maybe the font MozReview is using for you makes this look wrong, or maybe I'm misunderstanding what you mean?

Agreed with the other comments, will fix before landing.

Comment 11

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> (In reply to Botond Ballo [:botond] from comment #8)
> > > +  virtual void NotifyPinchGesture(PinchGestureInput::PinchGestureType aType,
> > > +                                  const ScrollableLayerGuid& aGuid,
> > 
> > This alignment, where the second argument type is one column before the
> > first argument type, elsewhere is a result of refactorings that changed
> > method names. Let's please not propagate it as a deliberate style choice :)
> > 
> > Applies to other header files modified in the patch too.
> 
> I'm not sure what you mean by this - the argument types are all lined up on
> the same column. Maybe the font MozReview is using for you makes this look
> wrong, or maybe I'm misunderstanding what you mean?

Hmm, yeah, looking at the raw diff, the alignment is fine. It must be a bug in MozReview that they don't look aligned there. My bad!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3437f7cc1db2
Convert the PinchGestureInput span fields from float to ParentLayerCoord. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd20b9ac007d
Fire magnify gesture events in the parent process if APZ is enabled but APZ zooming is disabled. r=botond,dvander

Comment 13

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/3437f7cc1db2
https://hg.mozilla.org/mozilla-central/rev/bd20b9ac007d

Comment 14

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #11)
> Hmm, yeah, looking at the raw diff, the alignment is fine. It must be a bug
> in MozReview that they don't look aligned there.

Filed bug 1305464 about the MozReview bug.