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

The main reason for doing this is consistency: since APZ is now enabled for B2G apps, the parent process is now the only place where it's _not_ enabled.

Also, after we do this I think we can remove the synchronous scrolling code in BrowserElementPanning.js.

A major issue that's blocking this is that there are some scrollable frames in the parent process which are visibility:hidden, yet get a layer (and therefore an APZC) if APZ is enabled, and that layer is in front of layers for the content actually behind drawn. It's unclear whether this is a bug in Gaia, Layout, or APZ - we should investigate a file a bug in the appropriate component.

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

Reposting Kats' patch from bug 946408 that flips the switch. It should not be committed until the issue described in comment #0 is resolved.

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

(In reply to Botond Ballo [:botond] from comment #0)
> A major issue that's blocking this is that there are some scrollable frames
> in the parent process which are visibility:hidden, yet get a layer (and
> therefore an APZC) if APZ is enabled, and that layer is in front of layers
> for the content actually behind drawn. It's unclear whether this is a bug in
> Gaia, Layout, or APZ - we should investigate a file a bug in the appropriate
> component.

I forgot to mention why this is a problem: it causes touch events to go to the hidden layer's APZC instead of an APZC belonging to the visible content, making parts of the visible content unscrollable.

I also recall now that we talked about this issue and decided it was an APZ bug, specifically a bug in how we do hit testing: we should be doing hit testing based on touch-sensitive regions rather than composition bounds, and the touch-sensitive region of a visibility:hidden element should be empty.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 3 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #0)
> The main reason for doing this is consistency: since APZ is now enabled for
> B2G apps, the parent process is now the only place where it's _not_ enabled.
> 
> Also, after we do this I think we can remove the synchronous scrolling code
> in BrowserElementPanning.js.

We will need to move the double zoom to tap code and some highlighting code into APZC too first ;)

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #3)
> (In reply to Botond Ballo [:botond] from comment #0)
> > The main reason for doing this is consistency: since APZ is now enabled for
> > B2G apps, the parent process is now the only place where it's _not_ enabled.
> > 
> > Also, after we do this I think we can remove the synchronous scrolling code
> > in BrowserElementPanning.js.
> 
> We will need to move the double zoom to tap code and some highlighting code
> into APZC too first ;)

I didn't mean removing all of BrowserElementPanning.js, only the parts that are conditioned on !asyncPanZoomEnabled.

(I have no problem with moving the double tap to zoom code and such into APZC code at some point, but that's a separate task.)

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

Botond and I were discussing this today and he remembered that the other thing preventing us from enabling APZ in the root process is that touch events destined for the root process never get sent to an APZ. In terms of my diagram at https://bug930939.bugzilla.mozilla.org/attachment.cgi?id=827404 the input events take the "Y" exit from the "Target in root process" branch and so never get to a TabParent or to an APZ. This is something we would also have to fix before being able to turn on APZ in the root process.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 6 • 10 years ago

That's something I would like to see for 2.1. It would make all the Gaia codebase using the same code path for scrolling.

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

Clearing nomination since we're not going to get to this for 2.1. Also looks like nobody is actually triaging the noms. I would also like to get this sooner rather than later, but it's complicated because we need better hit-testing in the APZ code and also to reroute input events before we can enable this.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 8 • 10 years ago

This got knocked out of 2.2 with the delivery date changes.

	Kai-Chih Hu [:khu]
	Comment 9 • 10 years ago

Hi, Milan, do we know who could be the best one to own this bug? Thanks.

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

That would probably be me. If you need to have an assignee then assign it to me, but there's dependent bugs that need to be fixed before this can be worked on.

	Kai-Chih Hu [:khu]
	Comment 11 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> That would probably be me. If you need to have an assignee then assign it to
> me, but there's dependent bugs that need to be fixed before this can be
> worked on.

Thank you. 

This bug depends on bug 920036 and 1005815, but both of them don't have assignee. I saw that, this bug was tagged as feature-b2g:2.2+. So, are we targeting to finish it in FxOS 2.2 that plans to use Gecko 37? Thank you.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 12 • 10 years ago

Kevin, you can assume that any 2.2+ bug that is in the graphic team's components (Canvas: 2D, Canvas: WebGL, GFX: Color Management, Graphics, Graphics: Layers, Graphics: Text, Image Blocking, ImageLib, Panning and Zooming) is going to be taken care of whether it has a person assigned or not.  If you see something that is unassigned and it messes up with your process, just assign it to me as a placeholder, to reduce the amount of bugzilla e-mail traffic.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 13 • 10 years ago

(In reply to Kevin Hu [:khu] from comment #11)
> ...
> 
> This bug depends on bug 920036 and 1005815, but both of them don't have
> assignee. I saw that, this bug was tagged as feature-b2g:2.2+. So, are we
> targeting to finish it in FxOS 2.2 that plans to use Gecko 37? Thank you.

Note that the dependencies are sometimes non-blocking dependencies, and we may be able to deliver a bug fix for a particular bug even without fixing all the bugs it depends on.  It's just a nature of how "Depends on" field is used, and very often it does not mean "Blocked on", even though that's what it looks like in the other direction.

Also note that the graphics features for 2.2 are in 2.2 branch of Gecko 37, not in regular Gecko 37, and mostly arriving during the Aurora.  Bhavana has the details.

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

Rebased to master

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

Updated so that BrowerElementPanning.js doesn't keep trying to drive scrolling in the root process as well.

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

Comment on attachment 8546848 [details] [diff] [review]
Enable APZ in the parent process

r?smaug for the docshell change in particular, r?botond for everything. The effect of the docshell change is to stop BEP.js from also responding to touch events in the root process, at [1].

[1] http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js?rev=687318d464a5#35

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

Comment on attachment 8546848 [details] [diff] [review]
Enable APZ in the parent process

I'd use Preferences::AddBoolVarCache, but I guess GetAsyncPanZoomEnabled
isn't in any hot code paths, at least not currently.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 18 • 10 years ago

Comment on attachment 8546848 [details] [diff] [review]
Enable APZ in the parent process

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

::: docshell/base/nsDocShell.cpp
@@ +13621,5 @@
>      if (TabChild* tabChild = TabChild::GetFrom(this)) {
>          *aOut = tabChild->IsAsyncPanZoomEnabled();
>          return NS_OK;
>      }
> +    *aOut = Preferences::GetBool("layers.async-pan-zoom.enabled", false);

I think this could get you in trouble?  The code that uses gfxPrefs::AsyncPanZoomEnabled() evaluates this preference once "on start up", then uses that value, even if the user changes it (restart is required for the new preference to take effect.)  This code, on the other hand, would get the latest value for the preference.  So, if the user changes this preference during the session, you'd end up with code in gfx, layout, dom, image, widget using the original value, and this code using the new value.

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

(In reply to Milan Sreckovic [:milan] from comment #18)
> I think this could get you in trouble?  The code that uses
> gfxPrefs::AsyncPanZoomEnabled() evaluates this preference once "on start
> up", then uses that value, even if the user changes it (restart is required
> for the new preference to take effect.)  This code, on the other hand, would
> get the latest value for the preference.  So, if the user changes this
> preference during the session, you'd end up with code in gfx, layout, dom,
> image, widget using the original value, and this code using the new value.

I guess technically that's true although I don't think this code is ever invoked after startup. I can switch it to using gfxPrefs::AsyncPanZoomEnabled() though for future-proofing and to address comment 17 at the same time.

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

Comment on attachment 8546848 [details] [diff] [review]
Enable APZ in the parent process

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

Looks good, but I'd like to see the patch updated as described in comment 19.

A couple of other things to consider:

  - There's a use of 'Preferences::GetBool("layers.async-pan-zoom.enabled")' here [1].

  - Is the operation done on this line [2] now redundant?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp?rev=901b357d7334#2149
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp?rev=39c448409484#116

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

(In reply to Botond Ballo [:botond] from comment #20)
>   - There's a use of 'Preferences::GetBool("layers.async-pan-zoom.enabled")'
> here [1].

I can update this to use gfxPrefs as well, since this does get called after startup.

>   - Is the operation done on this line [2] now redundant?

Oh, I didn't even see this. I suppose in practice it is probably redundant, but I'd rather leave it as-is for now and file a follow-up to clean it up. Really we should be able to get rid of every method of checking if APZ is enabled except for gfxPrefs, and that includes getting rid of TabChild::IsAsyncPanZoomEnabled(). Once we remove stuff from BrowserElementPanning.js (which we are tantalizingly close to being able to do) then we can even get rid of the docshell method entirely.

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

Updated as per last few comments. Builds and runs fine locally. I can do a try push once the dependencies are done and we're closer to landing this.

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

Comment on attachment 8556127 [details] [diff] [review]
Part 1 - Enable APZ in the parent process (v2)

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

Thanks!

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> Really we should be able to get rid of every method of checking if APZ is
> enabled except for gfxPrefs, and that includes getting rid of
> TabChild::IsAsyncPanZoomEnabled().

Does that mean it will no longer be possible to enable APZ on a per-app basis?

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

(In reply to Botond Ballo [:botond] from comment #23)
> 
> Does that mean it will no longer be possible to enable APZ on a per-app
> basis?

Correct. That was only possible when the APZ pref was disabled globally which I think is no longer a viable option on B2G.

	Botond Ballo [:botond] Assignee
	Comment 25 • 9 years ago

Moved the Part 4 patch from bug 1005815 here, as it's tied to enabling APZ in the parent process pretty tightly (neither patch alone would produce correct behaviour). Carrying r+ from that bug.

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

Reassigning since you did all the heavy lifting here anyway :)

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e710889d4e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcefc7d8d885

	[:fabrice] Fabrice Desré
	Comment 28 • 9 years ago

\o/

	Carsten Book [:Tomcat]
	Comment 29 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/3e710889d4e8
https://hg.mozilla.org/mozilla-central/rev/bcefc7d8d885

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 30 • 9 years ago

\o/ too.

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

Comment on attachment 8556127 [details] [diff] [review]
Part 1 - Enable APZ in the parent process (v2)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): required for parent-process APZ (bug 950934) and input-thread (bug 930939) which are 2.2+ features. I'm tracking the full set of bugs that will need uplifting together at http://mzl.la/1zvKgmk; requesting approval on bugs individually but will wait until everything has approval before I uplift any of it.
User impact if declined: can't have parent-process APZ or input-thread
Testing completed: on master. some of these bugs have been baking for a while; others are more recent.
Risk to taking this patch (and alternatives if risky): there's likely some edge cases that we haven't run into yet and will be uncovered with further testing. Bug 1128887 is the only known issue that we don't yet have a fix for but I consider that relatively minor and something we can fix after uplifting everything else.
String or UUID changes made by this patch: none

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

remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/39dd31c8ad24
remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/d29b62824a19