Quoting Vivien from https://bugzilla.mozilla.org/show_bug.cgi?id=976605#c49: "Can you open a followup to split this file into 2 separated parts ? A big part of the code of this file is only relevant if panning is handled by it ... That would also makes it easier to identify which parts of the code will be removed once APZ is enabled into the parent process, and what left to be converted from JS to cpp."

This sounds like it might make a good mentored bug. I'm happy to be the mentor, though I'll probably need some tips from Vivien.

On what basis this file should be split? I mean what kind or related to what, one file should contain and what should go into the other file?

(In reply to sssarvjeet27 from comment #2) > On what basis this file should be split? I mean what kind or related to > what, one file should contain and what should go into the other file? Hey, thanks for your interest :) I believe the general idea is to extract out parts that are only run if APZ (async pan zoom) is disabled. Notice that _setupListenersForPanning() is only called if APZ is disabled [1]. This means that handleEvent() will only receive touch and mouse events if APZ is disabled, and so the functions that handle these events, such as onTouchStart() and onTouchEnd(), are only called if APZ is disabled. So, these functions, and their helpers, can be extracted out into a new file. The remaining functions (such as _unloadHandler, _recvViewportChange, _recvDoubleTap, and _handleVisibilityChange), which get called regardless of whether APZ is enabled, can stay in BrowserElementPanning.js. Let me know if that's enough information to get started based on, otherwise I can go into more detail tomorrow. Once we extracted the parts specific to APZ being disabled into a new file, we'll have to hook up the new file somehow. We can ask Vivien for help with that part. [1] http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#36

Actually, given that bug 950934 enables APZ everywhere on B2G (and BrowserElementPlanning.js is only used on B2G), we can do something simpler: we can just *remove* the parts of BEP.js that are only used when APZ is disabled (i.e. the parts that we were going to split out into a new file).

I think its enough to get started. Thanks.

Comment on attachment 8560879 [details] [diff] [review] bug-995394-fix-v1.patch Review of attachment 8560879 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch - it looks great! Just a couple of comments: - As I mentioned in comment 4, we don't actually need the parts of BEP.js that are only run in APZ is disabled now that bug 950934 has enabled APZ everywhere on B2G, so we don't need the file 'browserElementPanningAPZDisabled.js'. - Please add a commit message of the form: Bug <bug number> - <description>. r=<reviewer> (The reviewer in this case is me, so put "botond"). If you could upload a new patch with these changes, that would be great! I will then submit it to our automated test server, and assuming it passes, commit it. ::: dom/browser-element/BrowserElementPanning.js @@ -26,3 @@ > > // Are mouse events being delivered to this content along with touch > // events, in violation of spec? Since you removed the line of code this comment applies to, please remove the comment as well.

I'm assigning the bug to you to indicate that you're working on it.

(In reply to Botond Ballo [:botond] from comment #7) > - As I mentioned in comment 4, we don't actually need the parts of BEP.js > that are only run in APZ is disabled now that bug 950934 has enabled APZ > everywhere on B2G, so we don't need the file > 'browserElementPanningAPZDisabled.js'. Hmm, bug 990904 seems to indicate that we do still need the synchronous scrolling code in some pathological cases. Kats, can you confirm if this is the case?

(In reply to Botond Ballo [:botond] from comment #9) > Hmm, bug 990904 seems to indicate that we do still need the synchronous > scrolling code in some pathological cases. Kats, can you confirm if this is > the case? There are cases where we need "synchronous" scrolling, but we can make those work using the existing sync-scrolling codepaths and APZ. That is, we can build an APZ for on a dummy layer, and that APZ can handle touch events and send scroll requests to main-thread gecko which will actually do the scrolling. We shouldn't need the code in BEP.js any more, and I'm happy to get rid of it.

Made changes as asked in comment 7.

Comment on attachment 8560947 [details] [diff] [review] bug-995394-fix-v3.patch Review of attachment 8560947 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! I pushed the patch to our test infrastructure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9ce8eb1c93c

This was or is my 3rd bug. Could you please suggest me next bug to take on?

(In reply to sssarvjeet27 from comment #13) > This was or is my 3rd bug. Could you please suggest me next bug to take on? Assuming you'd like to continue working in Javascript, there aren't really any bugs I can recommend personally, as I mostly work on C++ parts of the codebase. However, there are plenty of mentored bugs listed here [1]. You can filter them by feature / part of the codebase, and see if you find one you're interested in. If, by chance, you'd like to venture into C++, I can recommend some bugs for that. [1] http://www.joshmatthews.net/bugsahoy/?js=1

(In reply to Botond Ballo [:botond] from comment #12) > I pushed the patch to our test infrastructure: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9ce8eb1c93c The test run shows a failing test on B2G Desktop (apps/system/test/marionette/edges_gesture_test.js). I believe this is because on B2G Desktop (which is basically a setup where the B2G user interface is run on desktop) we don't have APZ enabled, but it still relies on BrowserElementPanning.js. So to fix this, I believe we'll have to keep BrowserElementPanningAPZDisabled.js after all. (Sorry to have gone back and forth on this!) sarvjeet, could you post a new patch with: - The file added back. (Start the name with a capital 'B' for consistency.) - Code to hook up the new file (i.e. make sure it gets loaded when APZ is disabled). Here's what I believe needs to be done for that: - Add BrowserElementPanningAPZDisabled.js to the manifest file [1]. - In BrowserElementChild.js, where we load BrowserElementPanning.js [2], also load BrowserElementPanningAPZDisabled.js *if* APZ is disabled. You can test for APZ being enabled with 'docShell.asyncPanZoomEnabled'. - There's another place where we load BrowserElementPanning.js [3] (looks like some optimization to pre-load certain files). Here too, also load the APZ-disabled version if APZ disabled. Here, you can test for APZ being enabled by checking the pref "layers.async-pan-zoom.enabled" (see the code a few lines above which checks other prefs). Thanks! [1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/jar.mn?rev=a80140872ea5#10 [2] http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChild.js?rev=6e90fd9bf6e2#45 [3] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/preload.js?rev=9151efa94f1f#94

Comment on attachment 8561551 [details] [diff] [review] bug-995394-fix-v6.patch Review of attachment 8561551 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks pretty good! I have a few comments. To be safe, I'd also like :kats and :vingtetun to look over some parts: - kats: the mechanisms used to check whether APZ is enabled (in BrowserElementChild.js and preload.js) - vingtetun: the script loading bits ::: dom/browser-element/BrowserElementChild.js @@ +42,5 @@ > } > } > > + if (docShell.asyncPanZoomEnabled === false) { > + Services.scriptloader.loadSubScript("chrome://global/content/BrowserElementPanningAPZDisabled.js"); We'll also need to call init() on the ContentPanningAPZDisabled object here (see my comment further below). @@ +50,5 @@ > ContentPanning.init(); > > Services.scriptloader.loadSubScript("chrome://global/content/BrowserElementChildPreload.js"); > } else { > ContentPanning.init(); In this branch, the scripts were loaded in preload.js, but we still need to call the init() functions here, so we'll need something like: if (docShell.asyncPanZoomEnabled === false) { ContentPanningAPZDisabled.init(); } ::: dom/browser-element/BrowserElementPanningAPZDisabled.js @@ +5,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > +dump("############################### browserElementPanning.js loaded\n"); Please update the comment to talk about "BrowserElementPanningAPZDisabled.js". @@ +13,5 @@ > +Cu.import("resource://gre/modules/Geometry.jsm"); > + > +var global = this; > + > +const ContentPanning = { ContentPanning here (and in BrowserElementPanning.js) is a global object. We don't want two global objects with the same name, so let's rename this one, for example to ContentPanningAPZDisabled. @@ +19,5 @@ > + watchedEventsType: '', > + > + // Are mouse events being delivered to this content along with touch > + // events, in violation of spec? > + hybridEvents: false, nit: too much indentation on this line ::: dom/ipc/preload.js @@ +90,5 @@ > } > } catch (e) { > } > > + if (Services.prefs.getBoolPref("layers.async-pan-zoom.enabled") === false){ Please enclose this in a try/catch block like the checks above (the reason this is necessary is that getBoolPref() can fail with an exception).

(Restoring original title as that's what we're doing after all.)

Comment on attachment 8561551 [details] [diff] [review] bug-995394-fix-v6.patch Review of attachment 8561551 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok to me, but I'm not that familiar with this stuff. If it passes try we can be reasonably sure it's good, I think.

Comment on attachment 8562026 [details] [diff] [review] bug-995394-fix-v6.patch Review of attachment 8562026 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, sarvjeet! Just one more small issue remaining. Carrying forward the feedback request from Vivien about the script loading bits from the previous patch. ::: dom/browser-element/BrowserElementPanningAPZDisabled.js @@ +423,5 @@ > + if (!firstScroll) { > + return false; > + } > + > + current = ContentPanning._findPannable(targetParent(current)); Here we also need to update ContentPanning to ContentPanningAPZDisabled.

Comment on attachment 8562830 [details] [diff] [review] bug-995394-fix-v6.patch Review of attachment 8562830 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Carrying feedback request from previous version. In the meantime I'll push the new patch to the test server.

(In reply to Botond Ballo [:botond] from comment #23) > In the meantime I'll push the new patch to the test server. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d05f2b2bce3c

(In reply to Botond Ballo [:botond] from comment #24) > (In reply to Botond Ballo [:botond] from comment #23) > > In the meantime I'll push the new patch to the test server. > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=d05f2b2bce3c Hmm, it seems like we somehow made things worse: most of the B2G Desktop tests are failing. I'll investigate when I get a chance.

Hey, sorry for the delay in following up on this! The logs for the failing test runs contains errors of the form: 10:29:05 INFO - JavaScript error: chrome://global/content/BrowserElementPanningAPZDisabled.js, line 35: SyntaxError: illegal character These are likely the cause of the failures. The line in question is: #ifdef MOZ_WIDGET_GONK The patch moves this line from BrowserElementPanning.js, where it didn't causing this error, to BrowserElementPanninAPZDisabled.js, where it does. Looking into this a bit, it looks like the #ifdef is not native JS syntax, but rather it's processed by a text preprocessor that our build system runs over JS code [1]. JS files that wish to use this preprocessor need to opt into them by having a '*' in front of their name in the manifest file. Notice how BrowserElementPanning.js had such a star [2]. The fix, therefore, should be to also add such a star in front of BrowserElementPanningAPZDisabled.js in the manifest file. I went ahead and made this change for you (didn't want to make you upload a new patch just for a one-character change), and pushed the updated patch to the Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe93d88fb8c7 [1] https://dxr.mozilla.org/mozilla-central/source/build/docs/preprocessor.rst [2] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/jar.mn?rev=a80140872ea5#10

could you please vouch for me?

(In reply to sssarvjeet27 from comment #27) > could you please vouch for me? Are you referring to getting Level 1 commit access? If so, sure - have you filed a bug for it?

(In reply to Botond Ballo [:botond] from comment #26) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe93d88fb8c7 Two B2G desktop tests (apps/calendar/test/marionette/month_view_test.js and apps/system/test/marionette/edges_gesture_test.js) - the same ones as in the Try run from comment 12 - are still failing. It's not immediately clear to me why.

(In reply to Botond Ballo [:botond] from comment #29) > Two B2G desktop tests (apps/calendar/test/marionette/month_view_test.js and > apps/system/test/marionette/edges_gesture_test.js) - the same ones as in the > Try run from comment 12 - are still failing. It's not immediately clear to > me why. Getting these tests to run locally, and getting a hold of enough logging output to be able to debug them turned out to be surprisingly difficult, but I managed it eventually. The test is failing because a scroll that's supposed to be happening, isn't happening. It looks like ContentPanningAPZDisabled is being successfully initialized and is successfully getting touch events that are meant to cause the scroll, but it takes some early-exit path before getting to actually invoke a scroll. I'll need to investigate a bit more to figure out why.

The problem was that the '_setActive()' function used 'this._domUtils', but the '_domUtils' getter was not defined in BrowserElementPanningAPZDisabled.js, only in BrowserElementPanning.js. Since both files use it, I copied it to BrowserElementPanningAPZDisabled.js. With that change, the failing test is passing for me locally. Try run with updated patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd553e67c391

(In reply to Botond Ballo [:botond] from comment #31) > Try run with updated patch: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd553e67c391 Looking much better! Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/da37f3c9b4ce

https://hg.mozilla.org/integration/mozilla-inbound/rev/da37f3c9b4ce#l1.22 This line has a typo: asynPanZoomEnabled

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #33) > https://hg.mozilla.org/integration/mozilla-inbound/rev/da37f3c9b4ce#l1.22 > > This line has a typo: asynPanZoomEnabled Nice catch! Fixed in follow-up: https://hg.mozilla.org/integration/mozilla-inbound/rev/a250f116b563

Both backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/577937f4dce7 for mochitest-devtools failures: https://treeherder.mozilla.org/logviewer.html#?job_id=7043950&repo=mozilla-inbound

Based on IRC discussion in #developers, it looks like this bug wasn't the cause of the mochitest-devtools failures, after all. Relanded (with the typo-fix patch folded in): https://hg.mozilla.org/integration/mozilla-inbound/rev/f38c426f4f28

\o/ Thanks, sarvjeet, for your work on this bug!

(In reply to Botond Ballo [:botond] from comment #28) > (In reply to sssarvjeet27 from comment #27) > > could you please vouch for me? > > Are you referring to getting Level 1 commit access? If so, sure - have you > filed a bug for it? It just occurred to me that perhaps you meant vouching for you on your mozillians.org profile. I went ahead and did that.

Comment on attachment 8562830 [details] [diff] [review] bug-995394-fix-v6.patch Clearing old feedback flag.