Tim Taubert [:ttaubert] (inactive) Assignee
	Description • 10 years ago

.... and throws "TypeError: content is null".

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 1 • 10 years ago

Seized the chance to add a proper uninit() function. I would have not added listeners at all but the comment at the top of the file seems to indicate that we want to cancel events.

	Josh Matthews [:jdm]
	Comment 2 • 10 years ago

Comment on attachment 8489135 [details] [diff] [review]
0001-Bug-1067128-_setupSwipeGesture-tries-to-access-conte.patch

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

::: browser/base/content/browser-gestureSupport.js
@@ +44,5 @@
> +      return;
> +    }
> +
> +    for (let name of this.GESTURE_EVENTS) {
> +      addEventListener(`Moz${name}`, this, true);

What's this?

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 3 • 10 years ago

Does this mean that we won't/can't support swipe animations in e10s?

	Josh Matthews [:jdm]
	Comment 4 • 10 years ago

Comment on attachment 8489135 [details] [diff] [review]
0001-Bug-1067128-_setupSwipeGesture-tries-to-access-conte.patch

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

Sorry, I have no idea what happened.

::: browser/base/content/browser-gestureSupport.js
@@ +44,5 @@
> +      return;
> +    }
> +
> +    for (let name of this.GESTURE_EVENTS) {
> +      addEventListener(`Moz${name}`, this, true);

Oh, http://tc39wiki.calculist.org/es6/template-strings/ I guess.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 5 • 10 years ago

(In reply to Josh Matthews [:jdm] from comment #2)
> > +    for (let name of this.GESTURE_EVENTS) {
> > +      addEventListener(`Moz${name}`, this, true);

Yes, those are template strings :)

(In reply to Stephen Pohl [:spohl] from comment #3)
> Does this mean that we won't/can't support swipe animations in e10s?

This means we need to rewrite it to support e10s.

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 6 • 10 years ago

> (In reply to Stephen Pohl [:spohl] from comment #3)
> > Does this mean that we won't/can't support swipe animations in e10s?
> 
> This means we need to rewrite it to support e10s.

I'm assuming that we're going with this workaround because e10s is on a tight deadline? Is there already a bug on file to implement this for e10s? IIRC this also affects history swipe navigation without animations that some people have come to rely on.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 7 • 10 years ago

Yeah, we're going with lots of workarounds because we want to make basic browsing in e10s work while iteratively fixing all the stuff that hasn't been written with e10s in mind.

Bug 863514 was filed to fix this. Are you talking about the back/fwd navigation with two-finger swipes? I of course don't want that to break and I think I tested that with the given patch applied. I really just want to get rid of that error for now when scrolling.

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 8 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #7)
> Bug 863514 was filed to fix this.

Thanks! I wasn't aware of this bug but will make sure to CC myself.

> Are you talking about the back/fwd
> navigation with two-finger swipes? I of course don't want that to break and
> I think I tested that with the given patch applied. I really just want to
> get rid of that error for now when scrolling.

Yes, I was expecting back/forward navigation with two-finger swipes to break with this patch and I still get this impression after quickly reading through browser-gestureSupport.js. It might be better to just leave isVerticalSwipe set to false when gMultiProcessBrowser is true in _setupSwipeGesture. If I'm mistaken and regular two-finger swipe navigation is indeed unbroken, please ignore my comment.

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 9 • 10 years ago

(In reply to Stephen Pohl [:spohl] from comment #8)
> It might be better to just leave isVerticalSwipe set to false when
> gMultiProcessBrowser is true in _setupSwipeGesture.

... and skip over the part that's accessing 'content', of course.

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 10 • 10 years ago

I would expect something like this to work:

diff --git a/browser/base/content/browser-gestureSupport.js b/browser/base/content/browser-gestureSupport.js
--- a/browser/base/content/browser-gestureSupport.js
+++ b/browser/base/content/browser-gestureSupport.js
@@ -184,22 +184,22 @@ let gGestureSupport = {
    */
   _setupSwipeGesture: function GS__setupSwipeGesture(aEvent) {
     if (!this._swipeNavigatesHistory(aEvent)) {
       return false;
     }
 
     let isVerticalSwipe = false;
     if (aEvent.direction == aEvent.DIRECTION_UP) {
-      if (content.pageYOffset > 0) {
+      if (gMultiProcessBrowser || content.pageYOffset > 0) {
         return false;
       }
       isVerticalSwipe = true;
     } else if (aEvent.direction == aEvent.DIRECTION_DOWN) {
-      if (content.pageYOffset < content.scrollMaxY) {
+      if (gMultiProcessBrowser || content.pageYOffset < content.scrollMaxY) {
         return false;
       }
       isVerticalSwipe = true;
     }
     if (isVerticalSwipe) {
       // Vertical overscroll has been temporarily disabled until bug 939480 is
       // fixed.
       return false;

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 11 • 10 years ago

Yeah that sounds like the right solution for now.

	Stephen A Pohl [:spohl] (OOO until 9/8)
	Comment 13 • 10 years ago

Comment on attachment 8489588 [details] [diff] [review]
0001-Bug-1067128-e10s-_setupSwipeGesture-tries-to-access-.patch, v2

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

Thanks!

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 14 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/3dd054c861f5

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 15 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/3dd054c861f5