678392 - [10.7] Add support for swipe animation as in Safari

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[beingalink]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7) AppleWebKit/534.48.3 (KHTML, like Gecko) Version/5.1 Safari/534.48.3

Steps to reproduce:

Going back and forth in browsing history using two-finger horizontal swipe on OSX Lion.


Actual results:

Sites get displayed normally without any animation.


Expected results:

There should be a swipe animation as in Safari under Mac OS 10.7 Lion.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

This bug was spun off from bug 668953.

Comment 4

[Markus Stange [:mstange]]

We need a UX spec here.

Here are some questions. What should we do in these cases, and what does Safari do (if applicable)?
I can't test Safari because I don't have access to my Lion machine this week.

 - If scrollbars are set to always visible, should they move with the page while
   the page is being swiped away, or should they stay fixed?
 - What about notification bars? Should they swipe away or stay fixed?
 - In Safari, if you have a small window, click on a link, make the window
   bigger, and swipe back halfway, is the rendering of the previous page that's
   visible in the background complete?
 - In Safari, does it ever happen that swiping back causes a reload of the
   previous page?
   If so, is the preview image replaced by a half-loaded rendering during the
   process, or does Safari wait for the page to load completely before hiding
   the preview rendering?
 - If the previous page has received POST data, Firefox shows a confirmation
   dialog before going back. (At least it did so in the past, does it still do
   it?) Does Safari have such a dialog, too? If so, at which point is it shown?

I'm also wondering how to implement this efficiently. We need to store a preview image of the last page at some point, and if we do it before every navigation action that's bound to regress page load performance numbers.

Comment 5

[Emanuele Alimonda]

(In reply to Markus Stange from comment #4)
>  - If scrollbars are set to always visible, should they move with the page
> while the page is being swiped away, or should they stay fixed?

They're moved away with the page.  See [1].

>  - What about notification bars? Should they swipe away or stay fixed?

I'm not aware of any notification bars in Safari, are there any?

>  - In Safari, if you have a small window, click on a link, make the window
>    bigger, and swipe back halfway, is the rendering of the previous page
> that's visible in the background complete?

No, it only includes the previous viewport. See [2].

>  - In Safari, does it ever happen that swiping back causes a reload of the
>    previous page?

It does (I believe it always does).  See [3].

>    If so, is the preview image replaced by a half-loaded rendering during the
>    process, or does Safari wait for the page to load completely before hiding
>    the preview rendering?

The page always appears fully loaded, minus possibly script-generated contens.
See the News page behavior in [3].  I need to test this thoroughly with heavier
pages though.

>  - If the previous page has received POST data, Firefox shows a confirmation
>    dialog before going back. (At least it did so in the past, does it still
> do it?) Does Safari have such a dialog, too? If so, at which point is it
> shown?

There is no such dialog.  POST data is not re-submit.  See [4] (and compare it
with the behavior in Firefox, especially regarding the URLs given by the POST
page at every new POST caused by a Back action.)

[1]: http://alimonda.com/misc/firefox/Safari1.webm  [ 846KB]
[2]: http://alimonda.com/misc/firefox/Safari2.webm  [4789KB]
[3]: http://alimonda.com/misc/firefox/Safari3.webm  [2623KB]
[4]: http://alimonda.com/misc/firefox/Safari4.webm  [ 903KB]

Comment 6

[Markus Stange [:mstange]]

Thank you, that was very helpful.

Comment 7

[Markus Stange [:mstange]]

Attachment: wip

Here's the first step. This patch adds code to browser.js that handles the animation overlay. Nothing calls the code yet, but you can test it like this:
After navigating around, open the Error Console (Cmd+Shift+J) and execute:

top.opener.gHistorySwipeAnimation.startAnimation()
top.opener.gHistorySwipeAnimation.updateAnimation(-0.4)
// ... play around with other values between -1 and 1 ...
top.opener.gHistorySwipeAnimation.stopAnimation()

The next step is to call these functions from the right places inside the swipe event tracking block in nsChildView.mm. A good way to do that might be to fire lots of swipe gesture events with sufficient data, and then to handle these events in browser.js gGestureSupport (and call the right gHistorySwipeAnimation functions from there). We might need to add one or two fields to the gesture event interface for that.

Comment 8

[Markus Stange [:mstange]]

Attachment: screenshot

Comment 9

[Emanuele Alimonda]

(In reply to Emanuele Alimonda from comment #5)
> >  - If the previous page has received POST data, Firefox shows a confirmation
> >    dialog before going back. (At least it did so in the past, does it still
> > do it?) Does Safari have such a dialog, too? If so, at which point is it
> > shown?
> 
> There is no such dialog.  POST data is not re-submit. [...]

A small correction to my previous comment.  Said confirmation dialog actually does exist in Safari ([1]), but it appears to be shown only when restoring a session (which, on a side note, would be a pretty neat behavior in my opinion - but that's beyond the scope of this bug).
It remains true, though, that it's never shown when swiping pages back and forward.

[1]: http://alimonda.com/misc/firefox/Safari4.png

Comment 10

[Markus Stange [:mstange]]

Attachment: part 1, v1: add more swipe gesture event types

Comment 11

[Markus Stange [:mstange]]

Attachment: part 2, v1: send swipe events from the swipe tracking handler

Comment 12

[Markus Stange [:mstange]]

Attachment: part 3, v1: animation visuals

This part is very unfinished. Todo:
 - fix code formatting + add comments
 - decide when to take snapshots (currently on "pagehide" event)
 - decide how to take snapshots (currently we also capture any overlays over
   the browser)
 - measure performance
 - fix direction (I think it's flipped at the moment)
 - only activate on Lion (currently activated on all platforms)
   ... and ideally only if the system pref is set
   ([NSEvent isSwipeTrackingFromScrollEventsEnabled])

Comment 13

[Markus Stange [:mstange]]

Attachment: part 4, v1: handle swipe animations to control animation

Comment 14

[Markus Stange [:mstange]]

OK, that's all I've got time for. Somebody else will need to finish these patches, ideally somebody who can also test them :)

I'm available for questions, of course.

These patches are still missing a very important part: If there's no page to go back / forward to, the animation still completes as though there was one. That's because we always pass -1 / +1 to the min / max arguments of -[NSEvent trackSwipeEventWithOptions:dampenAmountThresholdMin:max:usingHandler:].
We need some way for nsChildView.mm to find out whether the selected browser can go back / forward. Maybe we need to add another event, MozSwipeGestureConsiderStarting with write availablePageLeft / availablePageRight fields or something like that.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Thanks, Markus!

I'm not going to have time for this soon, either.  But I hope your work will give us decent animation support a few months down the line.

Comment 16

[Markus Stange [:mstange]]

Here's a tryserver build: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2368eae5dff9/try-macosx64/firefox-9.0a1.en-US.mac.dmg

Please test away.

Comment 17

[beingalink]

This is great. It seems to work well on my side and I'd like this patch checked in as soon as possible to get it thoroughly tested. Just 2 minor things that I observed: 1) Currently you can swipe beyond the first/last page seeing an empty/white page which disappears as soon as you stop swiping. 2) The animation isn't as smooth as in Safari. I guess that's a firefox problem as I can also see this kind of distortion on other pages that use animated DOM (example: http://www.annapialink.de/wp/ )

Comment 18

[beingalink]

Another thing: If you don't wait for the animation to complete and swipe back into the other direction, the animation gets disabled. If you do fast swiping afterwards, you won't see the animation anymore.

Comment 19

[beingalink]

Additionally there seems to be a problem with the Google+ site ( https://plus.google.com/ logged in): Visiting another page afterwards and swiping back to Google+ works, but if you want to go forward again, the Google+ site just gets reloaded.

Comment 20

[Markus Stange [:mstange]]

(In reply to beingalink from comment #19)
> Additionally there seems to be a problem with the Google+ site (
> https://plus.google.com/ logged in): Visiting another page afterwards and
> swiping back to Google+ works, but if you want to go forward again, the
> Google+ site just gets reloaded.

Interesting. I suspect that fixing bug 682115 will fix this.

Comment 21

[Markus Stange [:mstange]]

Here's a new build: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d250e9905569/try-macosx64/firefox-9.0a1.en-US.mac.dmg
It adds the bounce when you can't go back / forward and fixes the problems that occurred when starting a new swipe while the existing animation was still in progress.

Comment 22

[beingalink]

Bouncing when you can't go back/forward seems to work fine. Fast forward/backward (not waiting for animation to complete) still is quirky here. Swiping quickly from A to B to C ignores the swipe to C. Shows A for a short time, then B again and afterwards it goes to C.

Also the animation doesn't look really pleasant. I guess it uses Firfox's own graphics engine and not core animation (or whatever else api safari is using)? I hope that with azure on mac things will get better in this regard.

Comment 23

[astrolox+bugzilla]

Tested out the build posted in comment 21. Working well on my machine (no dev tools installed). Going to use this build as my main Mac OS X browser for a short while.

The animation seemed smooth and pleasant to me - I have one of the higher end MacBook Pros, so perhaps the machine's specification affects this?

Tried to cause issues with swiping very fast and only one made it do something odd. It appeared after two very fast forward swipes from A to B to C, to start the animation with the content for B but then switch to the content for C right at or near the end of the animation. The effect was so quick however that I'm not really sure that I'm believing myself when I write this. Could not reproduce on successive attempts.

Encountered the issue of resuming firefox, then swiping backwards in the history to find I was revealing a blank white page. Once the page was revealed it's content loaded and everything worked fine afterwards. 

Noticed that this includes rubber band scrolling style bouncing for when you try to go past the start or end of the history list. However this is only included for forward/backwards and not for normal scrolling (I think that there was a separate bug for that, but can't currently find it).

Comment 24

[astrolox+bugzilla]

On a side note.. I originally I thought that something such as an animation should be a low priority, nice to have feature. However I recently changed my mind after I started using Nightly as my default browser on my main machine (which is now Mac OS X Lion obviously). 

Going back/forward with the gestures works well, in terms of the page changing. Yet without the animation I keep getting very confused with the results I obtain. This is because Lion has by default changed the direction you have to swipe, that is you now swipe to the right in order to go backwards. The idea being that you are swiping the current page off to the right and revealing the page underneath it. The reverse is true for going forwards. 

So my point is that without the animation the support of the gesture does more harm than not supporting the gesture at all. If at all possible, I'd like to advise that Mozilla does not release the gesture support without having the appropriate animations in place. 

I suspect that if you haven't spent a lot of time using the gesture without the animation this may seem odd to you and so I thought I'd let you know my thoughts.

Comment 25

[beingalink]

I did a screen capture to demonstrate the scraggy looking animation but to my surprise, in the screen recording video everything looks more or less fine. The effect I wanted to show isn't there. I have no idea where the problem might be. Here's the now useless video anyway: http://dl.dropbox.com/u/232093/Bildschirmaufnahme.mov

Comment 26

[beingalink]

Nevertheless you can see that the animation isn't as smooth as with Safari.

Comment 27

[astrolox+bugzilla]

Now I've been using this a few days I have noticed a few things which weren't immediately obvious to start with. sorry, I'm still trying to work out which triggers these behaviors.

The animation does appear to have some jagged edges most of the time, but not always.

Randomly it seems to just stop working and the browser goes back to no animation at all.

After having used the browser for a few minutes on wikipedia, cnet, and sometimes other sites, a black gutter is added to the page on the bottom and right at the start of the animation and removed at the end of the animation.

It gets very confused when navigating in multiple tabs simultaneously. Steps:
1. search in google
2. click one of the links and wait until site is loaded (A)
3. swipe back to google results
4. middle-click (open in a tab in the background) one of the other links (B)
5. swipe forward 
On my machine this will causes a preview of the site which was loaded (A) to be slide in, but part way through the browser changes it to a white page. After this it forgets all navigation history for the tab. No page is loaded. The address bar remains the address of the google search results. No sure if it's coincidence but every time I have triggered this the tab loading site B doesn't complete.

Comment 28

[Emanuele Alimonda]

Is there any chance that the current patch will be landed before the Aurora merge?

The swipe gesture without animation is really confusing, and any animation - even if it's still a little quirky at times - is better than no animation.  A least in my opinion, the tryserver build from comment 21 is far more usable (concerning the one/two finger swipes) than the current Nightly builds.  Even more so it should be to those who haven't used Nightly for the last months.

Comment 29

[astrolox+bugzilla]

I agree that the gesture without animation is really confusing. Unfortunately the animation as implemented so far is very buggy after extensive use.

Comment 30

[Markus Stange [:mstange]]

Sorry for taking so long to get back to this.

Here's a new build:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8a43f85a4f86/try-macosx64/firefox-10.0a1.en-US.mac.dmg

(In reply to astrolox+bugzilla from comment #27)
> The animation does appear to have some jagged edges most of the time, but
> not always.

This is called "tearing" and occurs because we don't use vertical sync, see bug 555834 and bug 689418. It occurs on all animations, it's just more visible for horizontal animations (like the swipe animation here).

> Randomly it seems to just stop working and the browser goes back to no
> animation at all.

This was caused by a small typo in the code that handles discarding of saved snapshots as soon as the maximum of 10 snapshots per tab has been reached. So the animation would break after following 10 links in the same tab, for example.

> After having used the browser for a few minutes on wikipedia, cnet, and
> sometimes other sites, a black gutter is added to the page on the bottom and
> right at the start of the animation and removed at the end of the animation.

I think I've fixed that bug in the meantime.

> It gets very confused when navigating in multiple tabs simultaneously. Steps:

Can you reproduce the bug with the new build? I can't.

Comment 31

[Markus Stange [:mstange]]

Attachment: part 1, v2: add more swipe gesture event types / fields

Comment 32

[Markus Stange [:mstange]]

Attachment: part 2, v2: send swipe events from the swipe tracking handler

The lifecycle issues with the blocks here are interesting, but I think everything works out.

Comment 33

[Markus Stange [:mstange]]

Attachment: part 3, v2: animation visuals

This is the most controversial and tricky part.

We need to take a snapshot of a page right before it's navigated away from. We save the snapshot in a canvas and store up to 10 of them per tab. This snapshot is taken when the page receives the pagehide event.
During the swipe animation, an animation overlay hides the true tab contents. We hide the animation overlay when we know that the navigation in the tab behind it has completed, which is when the pageshow event of the new page fires.

There are several problems here:
 - Memory use for the snapshots.
 - Snapshotting has to paint the whole tab again and does not interface with,
   say, retained layers, so it takes time. This introduces latency to page
   navigation that might not be justified.
 - The pagehide event does not catch navigations caused by pushState.
 - Waiting for the pageshow event before hiding the animation overlay blocks
   interaction with the page for too long.

Adding an event for paint unsuppression might address the last point, but I don't know what to do about the rest. Any ideas?

Comment 34

[Markus Stange [:mstange]]

Attachment: part 4, v2: handle swipe events

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Markus, have you tried to reverse-engineer how Apple supports swipe animation in Safari?  If that's possible, it might be easier (and maybe more reliable) than building support from scratch.

I'll do my review as soon as possible -- hopefully today or tomorrow.  I'll also test all the patches together.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 571313 [details] [diff] [review]
part 1, v2: add more swipe gesture event types / fields

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

Someone else should review these changes.




>+/* attribute unsigned long allowedDirections; */
>+NS_IMETHODIMP nsDOMSimpleGestureEvent::GetAllowedDirections(PRUint32 *aAllowedDirections)
>+{
>+  NS_ENSURE_ARG_POINTER(aAllowedDirections);
>+  *aAllowedDirections = static_cast<nsSimpleGestureEvent*>(mEvent)->allowedDirections;
>+  return NS_OK;
>+}
>+NS_IMETHODIMP nsDOMSimpleGestureEvent::SetAllowedDirections(PRUint32 aAllowedDirections)
>+{
>+  static_cast<nsSimpleGestureEvent*>(mEvent)->allowedDirections = aAllowedDirections;
>+  return NS_OK;
>+}

NS_IMETHODIMP should be in the previous line.
Could you add a newline between methods.

Comment 37

[beingalink]

I'm using the try server build at the moment. It initially worked fine but after a while the animation stopped working and the two finger swipe worked as in the current nightlies. Thanks for telling about bug 555834 and bug 689418. I didn't know about them and I hope they will be fixed soon. I don't think it makes sense landing this feature as long as these aren't fixed.

Comment 38

[Steven Michaud [:smichaud] (Retired)]

Markus, in what order should these patches be applied?

I tried starting with "part 1", but that failed.

Comment 39

[Steven Michaud [:smichaud] (Retired)]

Parts 2, 3 and 4 apply with offsets.  But part 1 fails to apply even after I've applied all the rest.  Looks like you need to update your patches to current mozilla-central code.

Comment 40

[Markus Stange [:mstange]]

Attachment: part 1, v3

Updated to trunk and addressed comment 36.

Comment 41

[Steven Michaud [:smichaud] (Retired)]

When trying to build all patches at once, my build dies with the following error:

RuntimeError: File "linen-pattern.png" not found in /Volumes/Keaton/usr/local/src/Mozilla/hg/bugzilla678392/mozilla-central/browser/themes/pinstripe/browser, /Volumes/Keaton/usr/local/src/Mozilla/hg/bugzilla678392/mozilla-central/obj-x86_64-apple-darwin11.2.0/browser/themes/pinstripe/browser

Comment 42

[Xan]

There's a conflict I've found with horizontal scrolling. 

If you go to http://itunes.apple.com/app/shantae-riskys-revenge/id431535202?mt=8, and try to scroll horizontally to see the next images, you can't scroll horizontally and instead triggers the swipe animation, even after clicking inside the little box. 

And also, thanks for working on this guys, the build with the new scrollbar + swipe animation looks great and the animation works better than Chrome (yay!) and I hope it will land on UX or Nightly soon.

Comment 43

[Emanuele Alimonda]

(In reply to Xan from comment #42)
> There's a conflict I've found with horizontal scrolling. 

I believe the conflict is caused by bug #668953 rather than this, as it's currently present in both Nightly and Aurora builds and absent in Beta and Release builds.

Horizontal scrolling over the pictures moves them just by a few pixels when using a multitouch device (MagicMouse, MagicTrackpad, MBP glass trackpad). Diagonal scrolling works (if you diagonal scroll over the pictures, the page scrolls vertically, while images scroll horizontally).
If you use a legacy mouse or trackpad (i.e. the scrolling-ball Mighty Mouse or the old pre-unibody MacBook trackpad), horizontal scrolling work perfectly.

Comment 44

[Boris Zbarsky [:bzbarsky]]

> We need to take a snapshot of a page right before it's navigated away from. We save the
> snapshot in a canvas and store up to 10 of them per tab.
In order:

1)  Memory.  We're talking, for my case, say, 10 images each 921x936 pixels, right?  That's about 35MB per tab (10 * 921 * 936 * 4), if all 10 image slots are in use.

That does seem like a lot...  Have you measured whether this is in fact about the sort of memory usage you see?

If so, I would be interested in the numbers if instead you take the canvas, call mozGetAsFile on it, then create a filedata URI from that and just use <img> pointed at that URI.  And only create actual HTMLImageElement objects for the ones that you're sticking in mozSetImageElement.  This does need more CPU to encode/decode; perhaps the right way to do it is off an idle timer of some sort?

2)  Snapshotting time:  Please check with roc or tn?  Ccing them. If there's a way we can just get the data out of the layer system that would be ideal.

3)  Pushstate.  We could fire a chrome-only event of some sort there, I guess.  ccing Justin.  But the page may well update itself before calling pushstate....  This is where the layer system might save us, I guess.

4)  The point is that on history navigation we would block interaction until the pageshow?  I agree that adding an async unsuppress notification here would be the way to go.

Comment 45

[Justin Lebar (not reading bugmail)]

> - The pagehide event does not catch navigations caused by pushState.

> 3)  Pushstate.  We could fire a chrome-only event of some sort there, I guess.

> +    // Take a snapshot of a page whenever it's about to be navigated away
> +    // from.
> +    if (event.type == "pagehide")
> +      this._takeSnapshot();

It's hash changes too, right?  The event I think you want is "new session history entry created".

Comment 46

[Justin Lebar (not reading bugmail)]

...or maybe the event you want is "navigating away from this session history entry", since that's when you'd take a screenshot.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Boris Zbarsky (:bz) from comment #44)
> 1)  Memory.  We're talking, for my case, say, 10 images each 921x936 pixels,
> right?  That's about 35MB per tab (10 * 921 * 936 * 4), if all 10 image
> slots are in use.
> 
> That does seem like a lot...  Have you measured whether this is in fact
> about the sort of memory usage you see?

Discard on a timer maybe? there must be some nice heuristic we can use to limit this without much affecting the UX.

> 2)  Snapshotting time:  Please check with roc or tn?  Ccing them. If there's
> a way we can just get the data out of the layer system that would be ideal.

Pass the DRAWWINDOW_USE_WIDGET_LAYERS flag to drawWindow.

Comment 48

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47)
> (In reply to Boris Zbarsky (:bz) from comment #44)
> > 2)  Snapshotting time:  Please check with roc or tn?  Ccing them. If there's
> > a way we can just get the data out of the layer system that would be ideal.
> 
> Pass the DRAWWINDOW_USE_WIDGET_LAYERS flag to drawWindow.

On a content document which isn't a display root this flag will be silently ignored because we don't currently support pulling the layer contents starting at non-root container layers.

Comment 49

[Boris Zbarsky [:bzbarsky]]

> Discard on a timer maybe?

That gives somewhat bizarre behavior where going back will swipe sometimes but not other times depending on how long you spent reading the second page, right?  Hence my proposal to compress to png on a timer...

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Or scale the image down and then compress... Didn't Panorama already deal with all these issues?

(In reply to Timothy Nikkel (:tn) from comment #48)
> On a content document which isn't a display root this flag will be silently
> ignored because we don't currently support pulling the layer contents
> starting at non-root container layers.

True. We should fix that.

Comment 51

[Boris Zbarsky [:bzbarsky]]

> Didn't Panorama already deal with all these issues?

Good question.  Note that panorama did not need to show the images at full page size, and was storing one image per tab, not 10 per tab... but seeing what they did could be useful.

Comment 52

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #41)
> When trying to build all patches at once, my build dies with the following
> error:
> 
> RuntimeError: File "linen-pattern.png" not found in

That file is added by the part 3 patch. Compiling works for me if I apply the patches using hg qimport. Did you use something else? (The normal patch command doesn't work with binary files in diffs.) In any case, if you remove the linen-pattern.png line from jar.mn, compiling should work even if the file isn't present.

Comment 53

[Justin Lebar (not reading bugmail)]

Whatever we're storing in memory, be they compressed or uncompressed images, we should purge on a memory pressure notification.

I think we should also have a global limit on how many images we store.

Comment 54

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 571315 [details] [diff] [review]
part 2, v2: send swipe events from the swipe tracking handler

I can't see anything wrong with this patch.

And now that I realize I need to use hg qimport, I've also built the
whole patch and tested it.  My tests weren't particularly thorough,
and I did notice some problems.  But overall I was very impressed.

The worst problem I saw is that swiping isn't smooth on very busy
pages (e.g. swiping back and forth between http://www.theonion.com/
and any of the "Newswire" links on that page).  But you see the same
jerkiness with my much simpler (and less resource-intensive) patch
from bug 698761.  So I suspect this is some kind of Apple bug, or a
bad interaction between how we manage event loops and how Apple
manages horizontal two-finger swiping.  (The jerkiness doesn't occur
in Safari or Chrome, both of which are (of course) WebKit browsers.)

I've started a tryserver build based on Markus' patches, which should
be available in the next hour or two.

I also noticed another (relatively minor) bug.  But then I noticed the
same thing happens in Safari.  Which may make it a feature, or at
least may make it difficult for us to do anything about it.  Here's
STR:

1) Open 3-4 pages in succession in the same browser window, all of
   which have horizontal scroll bars (in other words, resize the
   window so that this is true).

2) As you swipe	between them, either with Markus' patch	or in Safari,
   you'll notice that if you swipe slowly (if you pause for a few
   seconds between swipes), you won't be able to swipe from page A to
   page B until page A has been scrolled all the way to one side.  But
   if you pause only briefly between swipes, you'll swipe to page B
   even before page A has scrolled all the way to one side.

This doesn't seem right to me.  But Safari's behavior may somehow
legitimize it.  And since we, like Safari, use -[NSEvent
trackSwipeEventWithOptions:...] to track swipe events, there may not
be a lot we can do about it.

Comment 55

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with Markus' latest patches:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e8ec712227b5/try-macosx64/firefox-10.0a1.en-US.mac.dmg

Please try it out and let us know your results.

Comment 56

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #54)

> I also noticed another (relatively minor) bug.  But then I noticed
> the same thing happens in Safari.  Which may make it a feature, or
> at least may make it difficult for us to do anything about it.
> ...

The same thing happens with my patch for bug 698761.

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Steven Michaud from comment #54)
> The worst problem I saw is that swiping isn't smooth on very busy
> pages (e.g. swiping back and forth between http://www.theonion.com/
> and any of the "Newswire" links on that page).  But you see the same
> jerkiness with my much simpler (and less resource-intensive) patch
> from bug 698761.  So I suspect this is some kind of Apple bug, or a
> bad interaction between how we manage event loops and how Apple
> manages horizontal two-finger swiping.  (The jerkiness doesn't occur
> in Safari or Chrome, both of which are (of course) WebKit browsers.)

Probably because those browsers are multiprocess and we aren't (yet); in particular page JS can delay our animation repaints in Firefox, but it can't in those browsers. We need e10s or at least asynchronous compositing.

Comment 58

[Chris Lee [:clee]]

Attachment: webpage does not fill entire browser window

Downloaded the tryserver build and noticed that not all the page loaded for some reason.  See attached screenshot.  

One other observation was that the page appeared to have a slight lag when loading pages on nytimes.com  Not sure if this was due to the animation build or if Fx10 has some other issues.  

I'm on 10.7.2.

Comment 59

[Steven Michaud [:smichaud] (Retired)]

> Downloaded the tryserver build and noticed that not all the page
> loaded for some reason.  See attached screenshot.

Was the page maximized when you saw this?

> One other observation was that the page appeared to have a slight
> lag when loading pages on nytimes.com

As I mentioned in comment #54, I saw severe choppiness and lag with
busy pages like those at http://www.theonion.com/.  And (oddly) these
problems seem just as bad with my (much less resource intensive) patch
for bug 698761.

Is this also true of the problem(s) you saw?  Are they just as bad
with my patch for bug 698761 as they are with Markus' patches?

Comment 60

[Chris Lee [:clee]]

(In reply to Steven Michaud from comment #59)
> > Downloaded the tryserver build and noticed that not all the page
> > loaded for some reason.  See attached screenshot.
> 
> Was the page maximized when you saw this?

Yes.

> 
> > One other observation was that the page appeared to have a slight
> > lag when loading pages on nytimes.com
> 
> As I mentioned in comment #54, I saw severe choppiness and lag with
> busy pages like those at http://www.theonion.com/.  And (oddly) these
> problems seem just as bad with my (much less resource intensive) patch
> for bug 698761.
> 
> Is this also true of the problem(s) you saw?  Are they just as bad
> with my patch for bug 698761 as they are with Markus' patches?
I didn't sense that they were as bad with your patch in bug 698761.  It was much more noticeable in this build for full animation support.

Comment 61

[Steven Michaud [:smichaud] (Retired)]

Attachment: Markus' latest patches, updated to current trunk

Here are Markus' latest patches, updated to current trunk code.  A new tryserver build will follow in a few hours.

Comment 62

[Steven Michaud [:smichaud] (Retired)]

> Markus' latest patches

The most recent of those that Markus attached above.

Comment 63

[Steven Michaud [:smichaud] (Retired)]

Here's the new tryserver build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-dd4193296c37/try-macosx64/firefox-11.0a1.en-US.mac.dmg

Comment 64

[Steven Michaud [:smichaud] (Retired)]

In order to get a rough idea how much further optimization may be
needed for Markus's patch, I just did a few experiments with it and
Safari on OS X 10.7.2.  The impression I get is "not much".  In other
words, the performance of Markus' patch (in terms of memory and CPU
usage) compares pretty well with Safari, and very well with Firefox
without Markus' patch.

In all of these tests I did the following:

1) Started the browser (FF with Markus' patch or Safari) and waited
   about 60 seconds for memory usage to settle down.  Then I measured
   its RSIZE and VSIZE in 'top' (ssh-ed in from another computer).

   For Safari I measured RSIZE and VSIZE for both the "Safari" and
   "WebProcess" processes.

2) Entered the following (partial) URLs, in order, in the location
   bar.  After entering each one, I waited for the page to finish
   loading before I entered the next one.  (None of these pages
   currently loads any plugins.)

   mozilla.org
   bugzilla.mozilla.org
   ibm.com
   microsoft.com
   apple.com

3) Used two-finger swipes or the Back and Forward buttons to do the
   following ten times:  Go back in history to the first page, then go
   forward in history to the last page.

4) Then I waited about 60 seconds for top's RSIZE and VSIZE to
   stabilize again, then measured them again.

VSIZE didn't change much in any of these tests.  But RSIZE roughly
doubled for both Firefox and Safari, *whether or not the two-finger
swipe or the Back and Forward buttons were used*.

Two-finger swiping with Markus's patch does temporarily use lots of
RAM -- RSIZE maxes out at roughly 3.5 times its original size (before
setting down to twice the original size).  But even using the Back and
Forward buttons temporarily triples RSIZE.

Two-finger swiping (with Markus' patch) seems roughly twice as
CPU-intensive in Firefox as using the Back and Forward buttons.  In
Safari, interestingly, they appear to be equally CPU-intensive.  So
maybe there's some room for improvement here.  But we wouldn't want to
go too far -- two-finger swiping with Markus' current patch seems (to
me) quite a bit more responsive than in Safari.  (I'm sure it helps
that none of the pages in my test have plugins.  As we've already
seen, responsiveness is severely degraded with "busy" pages like those
at http://www.theonion.com/ -- presumably because they have lots of
plugins.)

Comment 65

[Lex Robinson]

(In reply to Steven Michaud from comment #63)
> Here's the new tryserver build:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> dd4193296c37/try-macosx64/firefox-11.0a1.en-US.mac.dmg

Hi Steven,
Could you do a new build please?
That one seems to have expired.

Comment 66

[Steven Michaud [:smichaud] (Retired)]

Attachment: Markus' latest patches, updated to current trunk

Here's the build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b18d21ab3fad/try-macosx64/firefox-14.0a1.en-US.mac.dmg

Please download it (and test with it) as soon as possible.  It's likely to vanish off the servers in as little as a week.

Comment 67

[beingalink]

I have still the problem that when swiping back and forth between certain sites, it happens, that the old site gets displayed shortly before the correct site is displayed: http://dl.dropbox.com/u/232093/Bildschirmaufnahme.mov

Another old issue is vertical sync. You can't see it in the video but the animation doesn't look as good as in safari. This is not a problem of this patch but a general one of firefox (on the mac). Same problem appears with scrolling, video playback etc. Would be nice if this problem could be addressed soon, too.

Comment 68

[Valentin Tutu]

I've managed to get stuck in between 2 pages:
http://dl.dropbox.com/u/2311018/temp/2012-03-23_17.54.10.jpg

I can swipe again to remove the 2nd page from the screen, but the swipe remains broken for that tab (after few swipes it behaves in the same way).

I'm using the build posted by Steven.

Comment 69

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #68)

Do you have a way to reproduce this consistently?  One that works at least 50% of the time?

If so, please give us precise steps to reproduce.

Comment 70

[Valentin Tutu]

No, sorry. I will try to see if it happens again. It happened after a few hours of using the new build.

Comment 71

[:Gavin Sharp [email: [e-mail]]]

Markus: Sorry that I haven't gotten around to reviewing this in a reasonable amount of time. Frank said he'd be able to take a look at the updated patch, I'll flag him for feedback.

Comment 72

[Steven Michaud [:smichaud] (Retired)]

Attachment: Markus' latest patches, updated to current trunk

I've updated Markus' patches to current trunk again.  And this time I've taken the liberty of landing them on the ux branch (as Markus' patches for the Lion scrollbar style at bug 636564 were landed a while ago).

http://hg.mozilla.org/projects/ux/rev/52161cd6e3ea

It seems pointless to keep producing tryserver builds that disappear in a week, and which nobody uses.

Comment 73

[Johan Larsson]

The current implementation in the UX branch is a tremendous step up from no animation at all.

It tends to be a bit sluggish on media-heavy sites such as theverge.com but all in all it works quite well.

Comment 74

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #73)

Thanks for your tests.

As for the sluggishness, please try comparing performance on the same sites (and with the same behavior) to that in Safari and Chrome.  If our performance is significantly worse than other browsers, we may need to make some changes.  If it's about the same, we might just decide to live with it (and spend our time on other things).

Comment 75

[beingalink]

Performance is definitely worse than with Safari here. It's quite sluggish like a 1st person shooter on underspecced hardware. I'm on a Macbook Core2Duo 2Ghz, 4GB RAM, NVIDIA GeForce 9400M.

Comment 76

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #75)

I was talking about sluggishness while swiping.  Anything else is another bug.

Comment 77

[beingalink]

Of course I'm talking about the swipe animation.

Comment 78

[Steven Michaud [:smichaud] (Retired)]

You swipe back and forth between pages (in history) while playing a game?!

Comment 79

[beingalink]

Sorry, didn't think my English was that bad. I was just making a comparison. The swipe animation doesn't look ready-to-be-released good. The animation hangs and looks like it would have vsync issues. Someone was pointing me to corresponding bugs that might be related (Comment 30). Perhaps fixing these might make the animation look better. Perhaps it's also just my hardware not being sufficient. In Safari the animation looks mostly fine though.

Comment 80

[Xan]

I agree with beingalink. It's definitely more sluggish than Safari. I have an early 2008 Macbook Pro. Graphics card: 8600M GT. 

The animation definitely looks great but in image heavy sites, the animation gets sluggish. I can't compare it to Chrome since it doesn't have the same style but Safari's animation is much more smoother for me. 

One other thing I noticed is that if I'm on a tab and switch tabs for a while and come back to that tab and do a back animation, the animation shows me something completely different and then takes me to the correct page. 

For example, lets say I have the verge loaded on a specific article and then go back to the front page of the Verge. Then I leave the article to visit other tabs and then come back to the tab and then press back. The animation shows me the article I was on and then loads the front page for me. Does this happen to anyone else?

Maybe this is the reason why it's so sluggish sometimes?

Anyways, I appreciate all the work that's gone into this bug. And like I said, it looks fantastic when it's smooth.

Comment 81

[Xan]

I also made a screen recording showing the bug. When I try the animation, the previous page shows the front page but when I complete the animation, it's actually loads the correct page. It just shows the animation page wrong. 

Dropbox link(24MB screen recording): http://dl.dropbox.com/u/34865/firefox%20animation%20bug.mov

I tried to also show the cpu and ram firefox was taking during the animation and you can see how sluggish it is. If I do the animation again after completing it on the same tab, the animation is smooth again and shows the correct page and loads the correct page.

Comment 82

[Frank Yan (:fryn)]

Comment on attachment 617980 [details] [diff] [review]
Markus' latest patches, updated to current trunk

In the common case, this already works great, at least on my machine. :)

Unfortunately, this glitches when the viewport does not take up the entire width of the browser window, e.g. when a sidebar is open. The snapshot needs to stay at a z-index below any sidebars, and the positioning of the snapshot needs to account for any window width not taken up by the viewport.

I'll give more detailed feedback, including for specific code, later tonight in another comment.

Comment 83

[Frank Yan (:fryn)]

Comment on attachment 617980 [details] [diff] [review]
Markus' latest patches, updated to current trunk

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

This is an edge case, but if tabs get switched from tab A to tab B during a swipe (while the user is in contact with the trackpad), the history navigation gets applied to tab B, even though the animation was displayed on tab A.

::: browser/base/content/browser.js
@@ +1138,5 @@
> +    this._doEnd = function GS__doEnd(aEvent) {
> +      gHistorySwipeAnimation.stopAnimation();
> +
> +      self._doUpdate = function (aEvent) {};
> +      self._doEnd = function (aEvent) {};

Why is `self` needed here?
_doEnd is always called with the proper `this`.

@@ +1477,5 @@
> +  _farthestIndex: function HSA__farthestIndex(indices, fromIndex) {
> +    indices.sort(function numberComparator(a, b) (a - b));
> +    let lowestIndex = indices[0];
> +    let highestIndex = indices[indices.length - 1];
> +    let distanceToHighest = Math.abs(highestIndex - currentIndex);

currentIndex is not defined, so cache eviction is broken in this patch.

@@ +1499,5 @@
> +      // Evict the snapshot for the page that is farthest away from the current page.
> +      // TODO: evict everything on memory pressure notification
> +      let indexToDelete = this._farthestIndex(Object.keys(snapshots), currentIndex);
> +      delete snapshots[indexToDelete];
> +    }

We should profile this to see how much memory this uses over time.

::: browser/themes/pinstripe/browser.css
@@ +1595,5 @@
> +/* History Swipe Animation */
> +
> +#historySwipeAnimationCurrentPage,
> +#historySwipeAnimationNextPage {
> +  box-shadow: 0 3px 6px rgba(0, 0, 0, 0.6);

Our built-in sidebars (bookmarks, etc.) need to be at a higher z-index than these pages, especially for cases in which these pages over-scroll (bounce back).

Also, we should prevent pages from being translated more than their width. To see what I mean in slow motion, open the bookmarks sidebar, click the back button and then start swiping with 2 fingers to the left (to go forward) and then hold them down. Use a third finger (easier with a second hand) to repeatedly swipe to the left until the page slides over the bookmarks sidebar.

Comment 84

[Markus Stange [:mstange]]

Attachment: part 1

Comment 85

[Markus Stange [:mstange]]

Attachment: part 2

Comment 86

[Markus Stange [:mstange]]

Attachment: part 3

Comment 87

[Markus Stange [:mstange]]

Attachment: part 4

Comment 88

[Markus Stange [:mstange]]

Thanks for the review, Frank!

(In reply to Frank Yan (:fryn) from comment #82)
> Unfortunately, this glitches when the viewport does not take up the entire
> width of the browser window, e.g. when a sidebar is open. The snapshot needs
> to stay at a z-index below any sidebars, and the positioning of the snapshot
> needs to account for any window width not taken up by the viewport.

I've fixed this by wrapping the overlay boxes in a <stack> and setting overflow: hidden on that stack. The positioning of the snapshots already took into account the available viewport area, it wasn't based on window dimensions.

I've also removed the backdrop box and set the background pattern on the new container stack.

> This is an edge case, but if tabs get switched from tab A to tab B during a
> swipe (while the user is in contact with the trackpad), the history
> navigation gets applied to tab B, even though the animation was displayed on
> tab A.

Hmm, yes, this is unfortunate but don't see a simple fix here. Do you think it's worth worrying about?

> 
> ::: browser/base/content/browser.js
> @@ +1138,5 @@
> > +    this._doEnd = function GS__doEnd(aEvent) {
> > +      gHistorySwipeAnimation.stopAnimation();
> > +
> > +      self._doUpdate = function (aEvent) {};
> > +      self._doEnd = function (aEvent) {};
> 
> Why is `self` needed here?
> _doEnd is always called with the proper `this`.

Good catch, it's not needed. Fixed.

> 
> @@ +1477,5 @@
> > +  _farthestIndex: function HSA__farthestIndex(indices, fromIndex) {
> > +    indices.sort(function numberComparator(a, b) (a - b));
> > +    let lowestIndex = indices[0];
> > +    let highestIndex = indices[indices.length - 1];
> > +    let distanceToHighest = Math.abs(highestIndex - currentIndex);
> 
> currentIndex is not defined, so cache eviction is broken in this patch.

Ugh, I had noticed that ages ago but apparently never uploaded a fixed patch. Anyway, fixed now by renaming the parameter to currentIndex.

> 
> @@ +1499,5 @@
> > +      // Evict the snapshot for the page that is farthest away from the current page.
> > +      // TODO: evict everything on memory pressure notification
> > +      let indexToDelete = this._farthestIndex(Object.keys(snapshots), currentIndex);
> > +      delete snapshots[indexToDelete];
> > +    }
> 
> We should profile this to see how much memory this uses over time.

I've done a few measurements; we need 3 to 5 MB per snapshot (depending on the viewport size), and we have a default limit of 10 per tab, so that means a maximal memory usage of 30 to 50MB per tab. If that turns out to be too much, browser.snapshots.maxPerTab can be tweaked.

It would be good to have a hard limit on the total number of snapshots, but I haven't thought of a good way to implement that yet. If we add a global snaphot registry, we lose the GC-based discarding of closed tabs' snapshots, so we'd have to discard them manually on tab close. It's probably better than unbounded memory growth, but I'm not sure if it's worth the effort.

> ::: browser/themes/pinstripe/browser.css
> @@ +1595,5 @@
> > +/* History Swipe Animation */
> > +
> > +#historySwipeAnimationCurrentPage,
> > +#historySwipeAnimationNextPage {
> > +  box-shadow: 0 3px 6px rgba(0, 0, 0, 0.6);
> 
> Our built-in sidebars (bookmarks, etc.) need to be at a higher z-index than
> these pages, especially for cases in which these pages over-scroll (bounce
> back).

Fixed with overflow:hidden, see above.

> Also, we should prevent pages from being translated more than their width.
> To see what I mean in slow motion, open the bookmarks sidebar, click the
> back button and then start swiping with 2 fingers to the left (to go
> forward) and then hold them down. Use a third finger (easier with a second
> hand) to repeatedly swipe to the left until the page slides over the
> bookmarks sidebar.

Now that pages can't overlap the sidebar any more, I don't think we need to do anything here.

Comment 89

[Jared Wein [:jaws] (please needinfo? me)]

This patch was backed out of the UX Nightly branch due to code churn. Please update the patch and reland if you would like it to remain on the UX branch.

Comment 90

[Lars Brennicke]

I really don't understand why Mozilla doesn't invest more resources to fix this bug. Lion has been released a year ago. Swipe gestures are nearly unusable without the animation - one often goes back or forward accidentally and doesn't even realize what happened. I know many people who have switched back to Safari because they want to be able to swipe back and forward properly.

Markus quickly created a fantastic patch and noone seems to care. He replied to the review on May 24th. Instead of the Mozilla guys being very happy and doing everything they can to answer the last questions and get this patch into the next version of Firefox, they do: nothing at all.

"This patch was backed out of the UX Nightly branch due to code churn. Please update the patch and reland if you would like it to remain on the UX branch."

And what happens if Markus does the work and updates the patch? Then it's back in the UX branch, great. If noone from Mozilla seems committed to this feature and it will get stuck in the UX branch forever, then we cannot expect from Markus that he invests any more work in his patch which is a real pity.

Comment 91

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We don't have enough Mac developers and those we do have are working on other bugs like bug 674373.

Comment 92

[mcc]

#91 #90 That's exactly the point! 

There is not enough developers and (apparently) the code generated by users are not being used. It doesn't make much sense to me. Since the problem is the lack of developers, user input should be very appreciated rather than ignored.

Of course, it might be necessary some further verifications in the source code and maybe some optimisations. However this is better than working on the ticket from scratch.

Sorry if I judgment is mistaken here. I writing based only on this ticket and on another I submitted a couple years ago.

Comment 93

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #92)

Please read the bug's comments more carefully.  If you do you'll see that, though Markus' work appears close to complete, there are still significant problems.  And Markus' patches reach into many parts of the tree, so it's difficult to predict how long it will take to fix the problems.

Furthermore the current history swipe support is *not* unusable ... though it would certainly be improved by Markus' patches.

Finally, since I'm the only full-time Mac developer, it's impossible for me to spend more than about a week at a time on any one bug.  And it's often difficult to find *any* time when I'm distracted by bugs that are clearly more urgent than this one -- things like topcrashers or particularly bad UI bugs.

Mozilla's been working at hiring more Mac developers.  With luck we'll soon start being able to pay more attention to second-tier bugs like this (ones that it'd be really nice to have fixes for, but which aren't critical).

Comment 95

[Frank Yan (:fryn)]

Comment on attachment 626958 [details] [diff] [review]
part 3

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

Unfortunately, these patches have bitrotted, but the code seems ready for someone to take a look at it.

Comment 96

[astravian]

I just wanted to chime in and say that even though I lack the skills to accomplish this bit of eye candy, I would really appreciate anyone who is able to and would make a donation for the effort.

Comment 97

[Stephen A Pohl [:spohl]]

Attachment: A patch that folds Markus' parts 1-4 into one. Removed all conflicts/rejections with regards to the current nightly tip and tied up loose ends.

I folded all four parts into one patch, removed the bitrot with regards to the current nightly tip and tied up the loose ends. Any feedback is appreciated.

Comment 98

[Stephen A Pohl [:spohl]]

Attachment: A patch that folds Markus' parts 1-4 into one.

Removed a printf and comments used for debugging.

Comment 99

[Steven Michaud [:smichaud] (Retired)]

Stephen, here's what I suggest would be most valuable for you to do:

Use your build (made with your patch) to go through all problems reported here, figure out which ones are still reproducible, and then document them in reasonable detail (conditions required, steps to reproduce, and so forth).

Also (of course) weed out any problems that *aren't* due to Markus' patches (that also happen in current mozilla-central nightly builds).

Even better if you can find fixes for them! :-)

Comment 100

[Stephen A Pohl [:spohl]]

Right, I should probably have mentioned that I won't be able to look into this until the weekend. I thought that fixing up the patch to compile with the current trunk might make it a bit easier for someone else to look into this too, if interested.

I'll be going through the list of issues over the weekend, unless somebody beats me to it. :-)

Comment 101

[Stephen A Pohl [:spohl]]

This is going to be a pretty long comment, so I'm summarizing it here:
I didn't run into any performance problems or other bugs and think this patch is of good quality. However, performance might have been good due to my higher-end system (2.6 GHz i7 + 16GB RAM). Feedback from lower-end systems would be appreciated. There are a few questions that we might want to answer before landing this patch. These questions are listed first, followed by all the performance related issues where feedback would be welcome and the list ends with general bugs that were reported. All issues were either UTR for me, or expected behavior in my opinion.

=== Questions (are these must-haves for this patch?) ===
Q1.  (Boris Zbarsky) Memory.  We're talking, for my case, say, 10 images each 921x936 pixels, right?  That's about 35MB per tab (10 * 921 * 936 * 4), if all 10 image slots are in use. That does seem like a lot...  Have you measured whether this is in fact about the sort of memory usage you see? If so, I would be interested in the numbers if instead you take the canvas, call mozGetAsFile on it, then create a filedata URI from that and just use <img> pointed at that URI.  And only create actual HTMLImageElement objects for the ones that you're sticking in mozSetImageElement.  This does need more CPU to encode/decode; perhaps the right way to do it is off an idle timer of some sort?
- (Stephen) Confirmed not currently implemented.

Q2.  (Boris Zbarsky) Snapshotting time:  Please check with roc or tn?  Ccing them. If there's a way we can just get the data out of the layer system that would be ideal.
- (Stephen) I haven't faced issues with the performance of the latest patch. This may be due to my higher-end system however.

Q3. (Boris Zbarsky) Pushstate.  We could fire a chrome-only event of some sort there, I guess.  ccing Justin.  But the page may well update itself before calling pushstate....  This is where the layer system might save us, I guess.
- (Stephen) Not sure if this feedback went into Markus' patches.

Q4. (Justin Lebar) I think we should also have a global limit on how many images we store. Markus' comment: "It would be good to have a hard limit on the total number of snapshots, but I haven't thought of a good way to implement that yet. If we add a global snaphot registry, we lose the GC-based discarding of closed tabs' snapshots, so we'd have to discard them manually on tab close. It's probably better than unbounded memory growth, but I'm not sure if it's worth the effort"
- (Stephen) Confirmed that we don't currently have a global limit.

Q5. (Justin Lebar) Whatever we're storing in memory, be they compressed or uncompressed images, we should purge on a memory pressure notification.
- Confirmed not currently implemented.

=== Performance issues ===
P1. Animation not as smooth as Safari. However, Comment 17 by [e-mail] said: "I guess that's a firefox problem as I can also see this kind of distortion on other pages that use animated DOM (example: http://www.annapialink.de/wp/ )"
- (Stephen) UTR, might be due to my higher-end system

P2. Jagged edges during animation. Markus' comment: "This is called "tearing" and occurs because we don't use vertical sync, see bug 555834 and bug 689418. It occurs on all animations, it's just more visible for horizontal animations (like the swipe animation here)."
- (Stephen) UTR, might be due to my higher-end system

P3. The worst problem I saw is that swiping isn't smooth on very busy pages (e.g. swiping back and forth between http://www.theonion.com/ and any of the "Newswire" links on that page).  But you see the same jerkiness with my much simpler (and less resource-intensive) patch from bug 698761.  So I suspect this is some kind of Apple bug, or a bad interaction between how we manage event loops and how Apple manages horizontal two-finger swiping.  (The jerkiness doesn't occur in Safari or Chrome, both of which are (of course) WebKit browsers.) According to Robert O'Callahan: Probably because those browsers are multiprocess and we aren't (yet); in particular page JS can delay our animation repaints in Firefox, but it can't in those browsers. We need e10s or at least asynchronous compositing.
- (Stephen) I was unable to reproduce this, but this may very well be due to my higher-end system.

P4. Two-finger swiping with Markus's patch does temporarily use lots of RAM -- RSIZE maxes out at roughly 3.5 times its original size (before setting down to twice the original size).  But even using the Back and Forward buttons temporarily triples RSIZE.
- (Stephen) UTR. Although RAM increases slightly, it doesn't seem to exceed 20% for me with a baseline of 300MB. It settles close to the original 300MB.

P5. Two-finger swiping (with Markus' patch) seems roughly twice as CPU-intensive in Firefox as using the Back and Forward buttons.  In Safari, interestingly, they appear to be equally CPU-intensive.  So maybe there's some room for improvement here.  But we wouldn't want to go too far -- two-finger swiping with Markus' current patch seems (to me) quite a bit more responsive than in Safari.  (I'm sure it helps that none of the pages in my test have plugins.  As we've already seen, responsiveness is severely degraded with "busy" pages like those at http://www.theonion.com/ -- presumably because they have lots of plugins.)
- (Stephen) I may not have used the same method that Steven used to profile this, but I couldn't detect a massive difference between swiping and using the back/forward buttons in terms of CPU usage.

=== Issues and their current status ===
1. swipe beyond first/last page is possible (white page)
- (Stephen) UTR

2. If you don't wait for animation to complete and swipe back into the other direction, the animation gets disabled. Fast swiping afterwards results in no animation anymore.
- (Stephen) UTR

3. Problem with Google+: Leaving Google+ to go to another page, then coming back, then going forward only reloads Google+, not the correct next page (might be due to bug 682115 according to Markus).
- (Stephen) UTR, next page gets loaded correctly

4. History navigation should block interaction until the pageshow.
- (Stephen) Confirmed that page interaction is blocked until the animation completes.

5. Sometimes animation just stops working (fixed according to Markus)
- (Stephen) UTR

6. Sometimes, a black gutter is added to the page on the bottom and right at the start of the animation (removed when animation completes) (Markus believes he has fixed this)
- (Stephen) UTR

7. It gets very confused when navigating in multiple tabs simultaneously. Steps:
a. search in google
b. click one of the links and wait until site is loaded (A)
c. swipe back to google results
d. middle-click (open in a tab in the background) one of the other links (B)
e. swipe forward 
On my machine this will causes a preview of the site which was loaded (A) to be slide in, but part way through the browser changes it to a white page. After this it forgets all navigation history for the tab. No page is loaded. The address bar remains the address of the google search results. No sure if it's coincidence but every time I have triggered this the tab loading site B doesn't complete. (Markus could no longer reproduce this after his most recent changes)
- (Stephen) UTR

8. If you go to http://itunes.apple.com/app/shantae-riskys-revenge/id431535202?mt=8, and try to scroll horizontally to see the next images, you can't scroll horizontally and instead triggers the swipe animation, even after clicking inside the little box. (Emanuele believes that the conflict is caused by bug #668953 rather than this, as it's currently present in both Nightly and Aurora builds and absent in Beta and Release builds.)
- (Stephen) Confirmed that this issue occurs, however, it also occurs in the nightly build without the swipe patch applied. This may very well be due to #668953 as Emanuele suggested.

9. Open 3-4 pages in succession in the same browser window, all of which have horizontal scroll bars (in other words, resize the window so that this is true). As you swipe between them, either with Markus' patch	or in Safari, you'll notice that if you swipe slowly (if you pause for a few seconds between swipes), you won't be able to swipe from page A to page B until page A has been scrolled all the way to one side.  But if you pause only briefly between swipes, you'll swipe to page B even before page A has scrolled all the way to one side. This doesn't seem right to me.  But Safari's behavior may somehow legitimize it.  And since we, like Safari, use -[NSEvent trackSwipeEventWithOptions:...] to track swipe events, there may not be a lot we can do about it.
- (Stephen) Confirmed that we match the behavior in Safari. My personal opinion is that this should be the expected behavior.

10. I have still the problem that when swiping back and forth between certain sites, it happens, that the old site gets displayed shortly before the correct site is displayed: http://dl.dropbox.com/u/232093/Bildschirmaufnahme.mov
- (Stephen) UTR

11. I've managed to get stuck in between 2 pages: http://dl.dropbox.com/u/2311018/temp/2012-03-23_17.54.10.jpg (no consistent repro steps)
- (Stephen) UTR

12. One other thing I noticed is that if I'm on a tab and switch tabs for a while and come back to that tab and do a back animation, the animation shows me something completely different and then takes me to the correct page.
- (Stephen) UTR

13. Unfortunately, this glitches when the viewport does not take up the entire width of the browser window, e.g. when a sidebar is open. The snapshot needs to stay at a z-index below any sidebars, and the positioning of the snapshot needs to account for any window width not taken up by the viewport.
- (Stephen) UTR

Comment 102

[Boris Zbarsky [:bzbarsky]]

As far as must-haves go, I think having at least memory measurements for your typical half-dozen-tabs-reading-the-news case is a must-have.

Comment 103

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 660354 [details] [diff] [review]
A patch that folds Markus' parts 1-4 into one.

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

Thanks, Stephen!

I haven't gone over this in detail (I don't have the time right now).

But I'm inclined to say we can start asking for review(s) as soon as we have some way of measuring this patch's additional memory usage (in a debug build).  Whatever problems remain can be dealt with as separate bugs after the patch has landed.  But we probably want to land this early in a release cycle (to give it maximum baking time before a release).

By the way, when you ask for feedback or a review, you normally need to ask a specific person.

Comment 104

[Mark Jackubosky]

(In reply to comment #93)

Dear Steven, I guess you are the one who hasn't carefully read the previous comments. If you do you'll see that at any moment the user is stating that the the patch is perfect or ready to be included in the trunk. Additionally, he does mention that it is expected that users' patches won't be perfect and will probably need further optimisations, verifications and fixes.

I completely understand that there are more relevant bugs to be solved. I do not understand the tone Mozilla developers use in the tickets. There are several users trying to HELP mozilla to produce better software, and they and up being treated with irony or negligence.

And I am sure that you are doing your best to attend users' demands. Being the *only* Mac developer is not a easy task, and it is not your fault. 

In the last 2 or 3 posts in this ticket, the complaints were related to the lack of appreciation of user feedback. And it is indeed true! They spend their time to produce code to improve the software, or to come up with ideas to improve the software, for FREE. Instead of recognising the effort and giving them proper feedback, they are ignored in most cases.

Back to the topic, current swipe in Firefox, in my opinion, is horrible. That is a relevant factor that made me choose Chrome as my main browser (although Chrome's implementation is not as good as Safari's in this specific case). Additionally, I know other people that don't like it either and don't use Firefox because of that. This might seem silly, but it actually makes Mozilla loose market to other companies. 

Anyway, if there are more relevant tickets to be solved, ok I understand. But it would be nice to hear more often from Mozilla about the progress of tickets like this. As a user who is here contributing for free, just trying to make the software even better, I would really like to have more feedback.

Anyway, this will be my last post in bugzilla with this kind of message, in sick and tired of that. I hope that Mozilla developers start to appreciate more user feedback, but if you don't, be aware that you will loose more and more market. Users DO like to be heard and respected. The ironic "Please read….." in the previous message you post shows the opposite. If I was the user who posted that message, I would never come back and would stop using mozilla at the same moment.

For those who are not interested in this kind of message, I am really sorry. I just replied to this one because of the tone of the previous message. This is my last message of this kind.

Comment 105

[Jet Villegas (inactive)]

Stephen: bz's comment about retrieving the snapshot bitmap from the Layers system will likely address most if not all the memory/performance concerns here. I think we're OK having that be a follow-up bug, provided the current patches don't blow up under low memory. Thanks!

Comment 106

[Stephen A Pohl [:spohl]]

Thanks all for your feedback. What should be the next steps?

1. Measure the memory use as bz suggested? If so, what's the best way to do this?

2. Test low memory conditions? If so, is there a good and reliable way to put a system in that state?

3. Request review by setting the patch to "review?"? If so, who should I ask for a review?

Comment 107

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #106)

We're now (I think) in the 4th week of our 6-week release cycle.  I don't think this bug's patch should be landed until the beginning of a cycle (to give us the maximum amount of baking on pre-release channels).

I currently don't have any time to answer your questions.  I've been eaten alive by the HiDPI bugs.  But I should get spat out again before too long -- in the next week or two.

Whichever of your questions remain unanswered then, I'll try to answer myself.

In the meantime, I think we should plan to land this patch (or something like it) at the beginning of the next release cycle.

Comment 108

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 660354 [details] [diff] [review]
A patch that folds Markus' parts 1-4 into one.

Stephen, I've now taken a closer look at your patch and have some questions about it.

First, changes to the following files seem to be missing from it:

browser/themes/pinstripe/jar.mn
content/events/src/nsDOMEvent.h

Second, you've added a change to the following file:

browser/base/content/tabbrowser.xml

Please explain why this is necessary.

Once these issues are resolved, we can go on to ask for reviews.

Comment 109

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #102 and comment #105)

> As far as must-haves go, I think having at least memory measurements
> for your typical half-dozen-tabs-reading-the-news case is a
> must-have.

> Stephen: bz's comment about retrieving the snapshot bitmap from the
> Layers system will likely address most if not all the
> memory/performance concerns here. I think we're OK having that be a
> follow-up bug, provided the current patches don't blow up under low
> memory. Thanks!

Boris, please expand on this.  What (if anything) do think needs to be
done before this patch lands?

Comment 110

[Boris Zbarsky [:bzbarsky]]

I'm not sure what there is to expand on from comment 102, which you quoted.

The last iteration of this patch I considered (comment 44) seemed to add overhead in the range of tens of megabytes or RAM per tab at run time.  Hundreds of megabytes in an average multi-tab browsing session. 

So a must-have from my point or view is checking how much memory is in fact used by this patch under typical browsing conditions.  Once we have that number, we can see what the next steps are, depending on what the number is.

Comment 111

[Steven Michaud [:smichaud] (Retired)]

So I suppose we'd need some kind of debug logging -- something that could tell us what the memory overhead of this patch was at any one time (possibly triggered by a special key combination), and also periodically (on a 5-10 second timer).

We'd also (of course) need to decide how to measure this "memory overhead".

Comment 112

[Boris Zbarsky [:bzbarsky]]

Well, one possibility is to take some measurements with about:memory with and without this patch.

Comment 113

[Stephen A Pohl [:spohl]]

(In reply to Steven Michaud from comment #108)
> First, changes to the following files seem to be missing from it:
> 
> browser/themes/pinstripe/jar.mn

This was an oversight. This caused the background to be of a solid color when swiping past the history, instead of the linen-pattern. I'll be uploading a fixed patch shortly.

> content/events/src/nsDOMEvent.h

I believe this is no longer necessary, since nsDOMEvent::GetEventName() in nsDOMEvent.cpp now retrieves the name of the event from the list in nsEventNameList.h directly. The patch correctly adds the new events to nsEventNameList.h.

> 
> Second, you've added a change to the following file:
> 
> browser/base/content/tabbrowser.xml

This was necessary because function HSA__addBoxes() in browser.js tries to retrieve this element based on an anonid of "browserStack" in order to append a child element called "historySwipeAnimationContainer". The anonid was missing, so this kept failing.

The delay in my response might have given away that I'm a bit short on time right now. I will do my best to get some memory measurements soon.

Comment 114

[Stephen A Pohl [:spohl]]

Attachment: A patch that folds Markus' parts 1-4 into one.

Fixed an oversight in jar.mn that caused the background to be of solid color when swiping past the browser history, instead of a linen-pattern. Also ensured that this patch is still working with current trunk.

Comment 115

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 671734 [details] [diff] [review]
A patch that folds Markus' parts 1-4 into one.

Looks fine to me.

>> content/events/src/nsDOMEvent.h
>
> I believe this is no longer necessary, since
> nsDOMEvent::GetEventName() in nsDOMEvent.cpp now retrieves the name
> of the event from the list in nsEventNameList.h directly. The patch
> correctly adds the new events to nsEventNameList.h.

Makes sense.

>> Second, you've added a change to the following file:
>>
>> browser/base/content/tabbrowser.xml
>
> This was necessary because function HSA__addBoxes() in browser.js
> tries to retrieve this element based on an anonid of "browserStack"
> in order to append a child element called
> "historySwipeAnimationContainer". The anonid was missing, so this
> kept failing.

Makes sense.

> The delay in my response might have given away that I'm a bit short
> on time right now. I will do my best to get some memory measurements
> soon.

Thanks.  I may have time to do this myself by later this week or early
next week.

Comment 116

[Justin Lebar (not reading bugmail)]

Sorry to jump in here, but if I read this patch correctly, we're not compressing the data from these screenshots at all.

That's likely unacceptable from a memory-usage point of view, unless you set a low cap on how many of these images you're going to store (e.g., 2, globally).

Can we extract the screenshot from the canvas with the new toBlob() function (bug 648610) and compress them?  This will likely save a /lot/ of memory.  It also gives us the ability to write these images out to disk.  For example, we may not need to store history images for all tabs in memory -- we could write some to disk and pull them in when the tab is activated.

You should try both PNG (nice because it's lossless) and JPEG (nice because it's probably smaller and because our JPEG encoder is highly optimized).

You may also be able to save a lot of memory by converting these images to grayscale.  If that's acceptable from a UX PoV, we may want to do that.

Comment 117

[cendrizzi]

Hi,
I stumbled onto this bug since I was wondering the state of getting proper animation support for my favorite Mac browser gesture (and the reason I'm not using FF right now). I am excited that this is getting addressed, but as a web developer do have some questions and concerns I would like to raise:

Since I haven't tried (or know how) to install this fix, I was curious if this fix is following safari (animates going to the previous page) or chrome (simple arrow animation) way of animating. The Safari version is certainly pretty, but also has some unintended problems as well. For example, using the new HTML 5 history stuff if you animate changes between "states" then the safari method has some nasty usability issues that I am not sure how to get around. This is even present on Apples own icloud.com service. On that site you can open and close web applications and see the cool zooming css animations to transition between an opened application and the grid of application icon states (similar to IOS). If you swipe to go back from within an application it goes directly back to grid of icons (the pre-transitions state) and then re-animates the transition back to grid again.

On the other hand the far simpler chrome version works great in these cases, while doing a good enough job letting the user understand the threshold of when they have swiped far enough to go back or forward.

My post here is more to just raise awareness of the potential issues that face the Safari implementation of this animation. I'm not sure if there is a way to address this issue easily, it would be nice if you could detect if the user has done this gesture or not and respond accordingly for example. One thing that can be certain is more and more people are going to adopt the history api stuff since IE is finally getting support for this in the near future (and there is always the hash fallback for older browsers) and people like myself are going to start using css transitions more to offer visually appealing web applications.

Comment 118

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #116)

> Can we extract the screenshot from the canvas with the new toBlob()
> function (bug 648610) and compress them?  This will likely save a
> /lot/ of memory.  It also gives us the ability to write these images
> out to disk.  For example, we may not need to store history images
> for all tabs in memory -- we could write some to disk and pull them
> in when the tab is activated.

Sounds reasonable.  Of course we'd need to make sure this didn't cause
too much of a performance hit.

> You may also be able to save a lot of memory by converting these
> images to grayscale.  If that's acceptable from a UX PoV, we may
> want to do that.

I don't much like this idea, but others may disagree.

Comment 119

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #117)

We've already covered some of this ground -- see bug 698761.

My personal feeling is that Safari-style animation looks a lot better,
and we should use that if at all possible.

> If you swipe to go back from within an application it goes directly
> back to grid of icons (the pre-transitions state) and then
> re-animates the transition back to grid again.

Does this also happen using the Back button?  If it does, it's a
separate problem.

Comment 120

[Josiah Bruner [:jsbruner]]

What version of Firefox are you building this on? I can't get that patches to work on a fresh pull of mozilla-central.

Comment 121

[Josiah Bruner [:jsbruner]]

Never mind, I see why. Also, how far into this are you? How much work still needs to be done?

Comment 122

[Stephen A Pohl [:spohl]]

Revision 112190 "Merge m-c to inbound." broke the patch.

It's a rather large change, but I'll work on updating the patch to work with the current trunk.

@Josiah: The patch is (was) working as expected. One thing that needs improving is memory utilization. This could be achieved by compressing the snapshots (as suggested in Comment 116), unless the actual compression starts to affect performance.

Comment 123

[Justin Lebar (not reading bugmail)]

> Revision 112190 "Merge m-c to inbound." broke the patch.

FYI, we always refer to hg csets using their hash identifier, not the decimal identifier.  The decimal identifier may not be the same across different clones of m-c.

Comment 124

[Stephen A Pohl [:spohl]]

(In reply to Justin Lebar [:jlebar] from comment #123)
> > Revision 112190 "Merge m-c to inbound." broke the patch.
> 
> FYI, we always refer to hg csets using their hash identifier, not the
> decimal identifier.  The decimal identifier may not be the same across
> different clones of m-c.

Thank you for the heads up, Justin. That'd be fb8e7b959173 then.

Comment 125

[Josiah Bruner [:jsbruner]]

What version does the patch work on? If it doesn't work on a fresh pull of Mozilla-Central, then what will work. I would like to take a look at this feature.

Comment 126

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #125)
> What version does the patch work on? If it doesn't work on a fresh pull of
> Mozilla-Central, then what will work. I would like to take a look at this
> feature.

After a fresh pull, update to the revision prior to the one that broke like so:

hg update -C -r 112189

Then, apply the patch like you usually would. That should work.

Comment 127

[Stephen A Pohl [:spohl]]

Attachment: Updated patch for current trunk.

Comment 128

[Stephen A Pohl [:spohl]]

Try builds are available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6904d927d682/

Comment 129

[Josiah Bruner [:jsbruner]]

How do you work the swiping animation. I must have missed something. At the moment nothing happens. Is there some code I need to run in the console?

Comment 130

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #129)
> How do you work the swiping animation. I must have missed something. At the
> moment nothing happens. Is there some code I need to run in the console?

The swipe animation is for Mac OSX and two-finger swipe gestures and you don't need to run any special code. You swipe with two fingers to the right or to the left to navigate through your browsing history. If it doesn't work for you, please check if it works in Safari. If it doesn't work there either, you may have to enable "Swipe between pages" in System Preferences > More Gestures. Let me know if that did it for you or I'll have to investigate.

Comment 131

[Hawken Rives]

I am running OS X 10.8 on a Retina MacBook Pro, with Safari 6. 

Safari is doing its swipe animations, but this build of Firefox is just flipping back and forth between pages.

Comment 132

[Josiah Bruner [:jsbruner]]

Same here. Both the trysever and custom builds are doing nothing. I am also on Mountain Lion. What version of OS X are you on Stephen? Perhaps it is a change with Mountain Lion.

Comment 133

[Stephen A Pohl [:spohl]]

Hmm. I'm on 10.8.2 and I just verified again that the swipe animation works with the build here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6904d927d682/try-macosx64/firefox-20.0a1.en-US.mac.dmg

Clicking the back and forward buttons (intentionally) won't show the animation, but two-finger swipe should...

Comment 134

[Josiah Bruner [:jsbruner]]

Never mind. It works. I downloaded the debug one the first time.

Comment 135

[Josiah Bruner [:jsbruner]]

Alright, the problem is that this feature is quite unreliable. It seems to work if you have a session currently active, but not if you start a new one. Also, if you create a new tab and select something from the list of pages, the swiping feature will become disabled.

Comment 136

[Hawken Rives]

It seems to be something in my profile. I set a clean profile, and the animation looks great! Unlike Josiah, I can't reproduce it on my normal profile.

Comment 137

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #135)
> Alright, the problem is that this feature is quite unreliable. It seems to
> work if you have a session currently active, but not if you start a new one.

I'm not clear about this. When you say "start a new one", do you mean closing firefox and opening it again? I can't reproduce this issue.

> Also, if you create a new tab and select something from the list of pages,
> the swiping feature will become disabled.


I was able to reproduce this problem. Looking into it.

Comment 138

[Josiah Bruner [:jsbruner]]

That is what I meant. But I can no longer reproduce it. It might have been a fluke. But the new tab issue is where the real problem lies.

Comment 139

[beingalink]

Just want to confirm that using the try server build with my default nightly profile doesn't exhibit the swipe animation. I didn't try with a clean profile yet. Just want to point out that when the patch lands, there might be a problems for nightly testers.

Comment 140

[Valentin Tutu]

I can't trigger the swipe also (with my old profile). I'm using the one from comment #133. I'm here if you need to reproduce the problem. I'm on Lion (but I will migrate to Mountain Lion in the next few days).

Comment 141

[Markus Stange [:mstange]]

Comment on attachment 685725 [details] [diff] [review]
Updated patch for current trunk.


>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>+  _addBoxes: function HSA__addBoxes() {
>+    let browserStack =
>+      document.getAnonymousElementByAttribute(gBrowser.getNotificationBox(),
>+                                              "anonid", "browserStack");

Bug 768442 made browserStack use the class attribute instead of the anonid attribute. I think just replacing "anonid" with "class" here should fix the bug.

Comment 142

[Josiah Bruner [:jsbruner]]

Attachment: Screenshot of default page glitch

If you try to swipe on the "default" page, it will create a white border around it. Most likely because when a screenshot is being taken, it does not know to not include the white  background.

Comment 143

[Josiah Bruner [:jsbruner]]

(In reply to Markus Stange from comment #141)
> Comment on attachment 685725 [details] [diff] [review]
> Updated patch for current trunk.
> 
> 
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> >+  _addBoxes: function HSA__addBoxes() {
> >+    let browserStack =
> >+      document.getAnonymousElementByAttribute(gBrowser.getNotificationBox(),
> >+                                              "anonid", "browserStack");
> 
> Bug 768442 made browserStack use the class attribute instead of the anonid
> attribute. I think just replacing "anonid" with "class" here should fix the
> bug.

Yep. Just tried it. Works great, and it is a very easy. Thanks Markus.

Comment 144

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange from comment #141)
> Comment on attachment 685725 [details] [diff] [review]
> 
> Bug 768442 made browserStack use the class attribute instead of the anonid
> attribute. I think just replacing "anonid" with "class" here should fix the
> bug.

New try builds with this fixed are available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6b83f1f57cab/try-macosx64/

Comment 145

[Stephen A Pohl [:spohl]]

Attachment: Patch

Incorporated Markus' feedback

Comment 146

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #142)
> Created attachment 686076 [details]
> Screenshot of default page glitch
> 
> If you try to swipe on the "default" page, it will create a white border
> around it. Most likely because when a screenshot is being taken, it does not
> know to not include the white  background.

Does this still happen with the latest try builds? I could not reproduce.

Comment 147

[Chris Barry [ChrisW_B]]

New build works with a restored session where the old one didn't, at least for me. The one bug Ive found so far is that going back and forth between pages that have been zoomed doesn't quite work- the image is resized to 100%, but only the part visible when zoomed is kept, which means you end up with a large white space and only part of the page in the upper left corner

Comment 148

[Stephen A Pohl [:spohl]]

(In reply to Chris Barry [Christo27] from comment #147)
> New build works with a restored session where the old one didn't, at least
> for me. The one bug Ive found so far is that going back and forth between
> pages that have been zoomed doesn't quite work- the image is resized to
> 100%, but only the part visible when zoomed is kept, which means you end up
> with a large white space and only part of the page in the upper left corner

I think this matches what Safari is doing and we're sort of limited by what's actually visible at the time the snapshot is taken. Or are you saying that the behavior is different from Safari?

Comment 149

[Chris Barry [ChrisW_B]]

Attachment: Screenshot Comparing Safari Zoom and Firefox

Here's a comparison of the two, I tried to zoom to a similar amount on Bing on both

Comment 150

[Stephen A Pohl [:spohl]]

(In reply to Chris Barry [Christo27] from comment #149)
> Created attachment 686414 [details]
> Screenshot Comparing Safari Zoom and Firefox
> 
> Here's a comparison of the two, I tried to zoom to a similar amount on Bing
> on both

Ah, got it. I misread and thought you were referring to resizing the window, not zooming the page. I was able to reproduce the problem with zoomed pages.

Comment 151

[Josiah Bruner [:jsbruner]]

Yeah, that was the problem. Turns out I had the default page zoomed up. Therefore, Chris's problem is what I was experiencing.

Comment 152

[Josiah Bruner [:jsbruner]]

So really what needs to happen is a new screenshot needs to be taken every time the user zooms in/out.

Also, do you think we should have the animation occur when the back and forward buttons are pressed (For all platforms). It would be a quicker animation, but some visual feedback might be appreciated.

Comment 153

[José Jeria]

(In reply to Josiah from comment #152)

> Also, do you think we should have the animation occur when the back and
> forward buttons are pressed (For all platforms). It would be a quicker
> animation, but some visual feedback might be appreciated.

Safari doesn't do this. This sounds more like a separate request that should be dealt with in a separate bug.

Comment 154

[Josiah Bruner [:jsbruner]]

(In reply to José Jeria from comment #153)
> (In reply to Josiah from comment #152)
> 
> > Also, do you think we should have the animation occur when the back and
> > forward buttons are pressed (For all platforms). It would be a quicker
> > animation, but some visual feedback might be appreciated.
> 
> Safari doesn't do this. This sounds more like a separate request that should
> be dealt with in a separate bug.

That is true, but since all other bug requests to add swiping animation have been forwarded here, I figured it would be easiest to implement. Especially since this feature hasn't been pushed to the main mozilla-central. That said, I could create a new bug and copy the patches over.

Another idea is to just wait. I suppose we could implement the OS X feature first. And then after it is merged, maybe add an animation for the back/forward button.

Comment 155

[Steven Michaud [:smichaud] (Retired)]

>> Also, do you think we should have the animation occur when the back
>> and forward buttons are pressed (For all platforms). It would be a
>> quicker animation, but some visual feedback might be appreciated.
>
> Safari doesn't do this. This sounds more like a separate request
> that should be dealt with in a separate bug.

Animation wouldn't be useful/appropriate on the back and forward
buttons ... at least I don't think so.

The reason animation is so useful for swipes is that swipes don't take
place all at once.  It's only when a left or right swipe crosses a
certain threshold that you go back or forward in history.  The
animation shows you the swipe's "progress", both before and after the
threshold has been reached.

And in any case	I agree this should go into a separate bug.  It's
really a separate problem, and this bug already has too many comments
:-)

Comment 156

[Stephen A Pohl [:spohl]]

Attachment: Patch

Fixed zoom issue

Comment 157

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #154)
> (In reply to José Jeria from comment #153)
> > (In reply to Josiah from comment #152)
> > 
> > > Also, do you think we should have the animation occur when the back and
> > > forward buttons are pressed (For all platforms). It would be a quicker
> > > animation, but some visual feedback might be appreciated.
> > 
> > Safari doesn't do this. This sounds more like a separate request that should
> > be dealt with in a separate bug.
> 
> That is true, but since all other bug requests to add swiping animation have
> been forwarded here, I figured it would be easiest to implement. Especially
> since this feature hasn't been pushed to the main mozilla-central. That
> said, I could create a new bug and copy the patches over.
> 
> Another idea is to just wait. I suppose we could implement the OS X feature
> first. And then after it is merged, maybe add an animation for the
> back/forward button.

An animation for the back/forward buttons might look nice, but I agree that it should probably be tracked as a separate bug since it's not strictly related to swipe gestures anymore and could be implemented for all platforms. Users might also want to have a way to turn the animation off.

Comment 158

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #157)
> (In reply to Josiah from comment #154)
> > (In reply to José Jeria from comment #153)
> > > (In reply to Josiah from comment #152)
> > > 
> > > > Also, do you think we should have the animation occur when the back and
> > > > forward buttons are pressed (For all platforms). It would be a quicker
> > > > animation, but some visual feedback might be appreciated.
> > > 
> > > Safari doesn't do this. This sounds more like a separate request that should
> > > be dealt with in a separate bug.
> > 
> > That is true, but since all other bug requests to add swiping animation have
> > been forwarded here, I figured it would be easiest to implement. Especially
> > since this feature hasn't been pushed to the main mozilla-central. That
> > said, I could create a new bug and copy the patches over.
> > 
> > Another idea is to just wait. I suppose we could implement the OS X feature
> > first. And then after it is merged, maybe add an animation for the
> > back/forward button.
> 
> An animation for the back/forward buttons might look nice, but I agree that
> it should probably be tracked as a separate bug since it's not strictly
> related to swipe gestures anymore and could be implemented for all
> platforms. Users might also want to have a way to turn the animation off.

Very well. I will create a new bug after this bug as been merged with the mainstream development.

Comment 159

[beingalink]

I have the problem that after the animation to last/next page in history, I get to see the page of where I came from when the page reloading starts. When the reloading has finished, I get to see the correct page again.

Comment 160

[beingalink]

This only happens with sites with dynamic content of course.

Comment 161

[Mozilla RelEng Bot]

Try run for dd03ec79ffa7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=dd03ec79ffa7
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-dd03ec79ffa7

Comment 162

[Stephen A Pohl [:spohl]]

(In reply to Mozilla RelEng Bot from comment #161)
> Try run for dd03ec79ffa7 is complete.

Try builds with zoom fix available:
http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-dd03ec79ffa7/try-macosx64/

Comment 163

[José Jeria]

(In reply to Stephen Pohl [:spohl] from comment #162)
> Try builds with zoom fix available:
> http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/spohl@mozilla.
> com-dd03ec79ffa7/try-macosx64/

Just tested this. Looks great! Also love the bounce effect when there are no more pages to go back/forward to. Would it be possible (I guess there is already a bug for this) to add this bounce effect when you reach the top/end of the page? Safari does this.

Comment 164

[Josiah Bruner [:jsbruner]]

It is possible. It will have to be part of another bug though. One might exist. We should try to finish one. We now have swiping pages, disappearing scrollbars, and then the elastic effect. If those three things were added to Firefox 20, then I think Firefox would finally have an OS X feel.

Comment 165

[Josiah Bruner [:jsbruner]]

What code would I have to use to implement the animation from somewhere else all at once. I am trying to add the animation to the back button, I know I should ask this on another bug, but I need to know what the necessary code is? That is, with these patches in tact. 

I'm using:

 <method name="goBack">
        <body>
          <![CDATA[
            var webNavigation = this.webNavigation;
            if (webNavigation.canGoBack) {
              try {
                this.userTypedClear++;
                webNavigation.goBack();
              } finally {
                if (this.userTypedClear)
                  this.userTypedClear--;
              }
            }
          ]]>
        </body>
      </method>

So, what do I need to call for the animation to work while going back? Thanks.

Comment 166

[Stephen A Pohl [:spohl]]

(In reply to beingalink from comment #160)
> This only happens with sites with dynamic content of course.

Do you have a specific URL that reproduces this problem?

Comment 167

[Josiah Bruner [:jsbruner]]

(In reply to beingalink from comment #160)
> This only happens with sites with dynamic content of course.

Like Stephen, what URL? I can not reproduce this issue.

Comment 168

[beingalink]

For example on my last.fm profile site. Go to http://www.last.fm/user/TheLink and then to https://encrypted.google.com and then swipe back to the last.fm page.

Comment 169

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #165)
> What code would I have to use to implement the animation from somewhere else
> all at once.

Looking at function HSA_updateAnimation(val) in the patch might give you a start. For swipe gestures, val is obtained via the swipe event and is used to control the animation progress.

Comment 170

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #169)
> (In reply to Josiah from comment #165)
> > What code would I have to use to implement the animation from somewhere else
> > all at once.
> 
> Looking at function HSA_updateAnimation(val) in the patch might give you a
> start. For swipe gestures, val is obtained via the swipe event and is used
> to control the animation progress.

Right, but since this is just a button press, I could use another value correct. That's just an integer right?

Comment 171

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #170)
> (In reply to Stephen Pohl [:spohl] from comment #169)
> > (In reply to Josiah from comment #165)
> > > What code would I have to use to implement the animation from somewhere else
> > > all at once.
> > 
> > Looking at function HSA_updateAnimation(val) in the patch might give you a
> > start. For swipe gestures, val is obtained via the swipe event and is used
> > to control the animation progress.
> 
> Right, but since this is just a button press, I could use another value
> correct. That's just an integer right?

It's a floating point number, representing the event's delta. Markus added this helpful description:
+ * MozSwipeGestureUpdate - Generated periodically while the user is
+ * continuing a horizontal swipe gesture.  The "delta" value represents
+ * the current absolute gesture amount.

In order to get a true animation, you will want to continuously call this (or a similar) function with updated values that represent the animation progress. I think the tricky part will be to time this correctly.

It may also be a good time to file the separate bug we've been talking about to keep this information separate from the swipe bug here.

Comment 172

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #171)
> (In reply to Josiah from comment #170)
> > (In reply to Stephen Pohl [:spohl] from comment #169)
> > > (In reply to Josiah from comment #165)
> > > > What code would I have to use to implement the animation from somewhere else
> > > > all at once.
> > > 
> > > Looking at function HSA_updateAnimation(val) in the patch might give you a
> > > start. For swipe gestures, val is obtained via the swipe event and is used
> > > to control the animation progress.
> > 
> > Right, but since this is just a button press, I could use another value
> > correct. That's just an integer right?
> 
> It's a floating point number, representing the event's delta. Markus added
> this helpful description:
> + * MozSwipeGestureUpdate - Generated periodically while the user is
> + * continuing a horizontal swipe gesture.  The "delta" value represents
> + * the current absolute gesture amount.
> 
> In order to get a true animation, you will want to continuously call this
> (or a similar) function with updated values that represent the animation
> progress. I think the tricky part will be to time this correctly.
> 
> It may also be a good time to file the separate bug we've been talking about
> to keep this information separate from the swipe bug here.

I will want to keep calling what? HSA_updateAnimation, or MozSwipeGestureUpdate? And, I'll go make another bug.

Comment 173

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #172)
> I will want to keep calling what? HSA_updateAnimation, or
> MozSwipeGestureUpdate? And, I'll go make another bug.

You will want to keep calling HSA_updateAnimation. MozSwipeGestureUpdate is just the event that the delta is taken from and then passed to HSA_updateAnimation. If you search for updateAnimation and how it's called in the patch it should make it clearer.

Comment 174

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #173)
> (In reply to Josiah from comment #172)
> > I will want to keep calling what? HSA_updateAnimation, or
> > MozSwipeGestureUpdate? And, I'll go make another bug.
> 
> You will want to keep calling HSA_updateAnimation. MozSwipeGestureUpdate is
> just the event that the delta is taken from and then passed to
> HSA_updateAnimation. If you search for updateAnimation and how it's called
> in the patch it should make it clearer.

Alright, I'll take a look. In case any one is interested, here is the new bug: https://bugzilla.mozilla.org/show_bug.cgi?id=817074

Comment 175

[Josiah Bruner [:jsbruner]]

So, just to make sure I am doing this right... I doubt I am. Before webNavigation.goBack(); I add 

gHistorySwipeAnimation.startAnimation;
gHistorySwipeAnimation.updateAnimation(1)
gHistroySwipeAnimation.stopAnimation;

Obviously, that is way to short to notice any difference, but is that the right idea? Perhaps you could give a very, very rough version of this. Maybe just a few frames, but that will show a little animation when pressing the back button. Once I see how, I can take it from there. 

Feel free to reply in the new bug, that way comments can stop here about this. Thanks.

Comment 176

[Stephen A Pohl [:spohl]]

I've been looking into compressing the snapshot data with both PNG and JPG encoding. I use canvas' toDataURL method to obtain a URL to use as src for Image objects and I use these image objects during animation (instead of the canvas object directly). I found that both PNG and JPG encoding work fast on my system. Also, if a quality of 100% is specified for JPG, both PNG and JPG seem to virtually even out in memory utilization, with considerable variation depending on the type of websites that are being visited (lots of different colors etc.). JPG seems to be a good choice if we use a quality of less than 100%. However, using a quality of less than 100% on a retina display may not be the right choice. It doesn't look very nice.

All these things considered, I'm tempted to favor PNG over JPG.

Another way to limit memory utilization for this feature is the number of pages that we store per tab. We currently have a default of 10, but maybe 4 or 5 would be enough.

Comment 177

[Justin Lebar (not reading bugmail)]

Blobs reduce memory used by images by a factor of ~2.5 compared to data URIs.  I'd highly recommend using the blob api, even though it's more annoying than data URIs.

Some users have hundreds of tabs; we're going to need a global limit on how many images are kept around, instead of or in addition to to a per-tab limit.

Comment 178

[Jared Wein [:jaws] (please needinfo? me)]

We could probably just store 4-5 images of the 3-4 most recently selected tabs.

The 3-4 number comes from this study, which shows the median of tabs open on navigation right around 3 to 4, http://dubroy.com/blog/how-many-tabs-do-people-use-now-with-real-data/

Comment 179

[Stephen A Pohl [:spohl]]

(In reply to Justin Lebar [:jlebar] from comment #177)
> Blobs reduce memory used by images by a factor of ~2.5 compared to data
> URIs.  I'd highly recommend using the blob api, even though it's more
> annoying than data URIs.

I gave this a shot, but I'm running into an important performance problem. The conversion from a Blob back to an Image object seems to take a fair amount of time. When swiping between pages, there is considerable flickering. The conversion currently happens when the animation starts, which is not ideal. There should be as little processing as possible at the start of the animation to make it as smooth as possible.

There might be two ways how we can use blobs anyways:
1. There might be a better/faster way to convert blobs to images. I've tried URL.createObjectURL() to get a URL that is then assigned to src of the image object.
2. Our snapshot-array could hold blobs, but the previous, current and next page would always be available as image objects as well (in a separate array for example). The problem here is that quick swiping would still cause the performance problem described above (flickering).

Comment 180

[Justin Lebar (not reading bugmail)]

> but the previous, current and next page would always be available as image objects as well

Maybe if you did this only for the current tab, that would be OK.  Although you'd probably want some sort of timeout before you start decoding a tab's images, otherwise we'll eat a lot of CPU if you quickly switch between tabs.

I don't see why you need the current snapshot in memory, though.

I'm surprised blobs would perform worse than data: URIs here.  But I don't know the implementation details, so I guess it's possible.

Comment 181

[Stephen A Pohl [:spohl]]

(In reply to Justin Lebar [:jlebar] from comment #180)
> 
> I don't see why you need the current snapshot in memory, though.
> 

The current snapshot is needed because the animation actually consists of two box elements sliding over each other (previous and current, or current and next). The snapshots are used as background images for the box elements. This can be seen by the fact that any dynamic content on the current page stops during the animation (since it's a snapshot of when the animation started).

Comment 182

[Justin Lebar (not reading bugmail)]

Okay, but you're always going to have a decode lag problem with the current screenshot, right?  The current screenshot needs to be taken right before the animation begins, because it needs to reflect the current state of the page (and the current scroll state of the page, for sure).

I guess there's no need to compress the current screenshot, though.

It's not clear to me why you can't pull the prev/next screenshot over the current one, but that's not for me to understand.  :)

Comment 183

[Stephen A Pohl [:spohl]]

(In reply to Justin Lebar [:jlebar] from comment #182)
> Okay, but you're always going to have a decode lag problem with the current
> screenshot, right?  The current screenshot needs to be taken right before
> the animation begins, because it needs to reflect the current state of the
> page (and the current scroll state of the page, for sure).

That's true. Storing canvas objects in the snapshot array definitely has the best performance. toDataURL causes a bit of flickering (and I'm not sure that's acceptable), not as much as toBlob though. I believe toDataURL() performs better in this scenario because it can be directly assigned to src of an image element. We then store these image elements in the snapshot array. After the snapshot is taken, there is no further processing necessary to assign the snapshot as background image of the box element. If we use blobs however, we would be storing blob elements in the snapshot array and we would have to convert them every time we need them during an animation. The URL.createObjectURL call for the conversion seems to be costly and cause the flickering.
 
> It's not clear to me why you can't pull the prev/next screenshot over the
> current one, but that's not for me to understand.  :)

We actually slide the next page over the current one, but we slide the current page away from the previous one. I think of it as a stack of paper. I either add a page (slide the next page onto the stack) or I remove a page (slide the current page away from the current stack). Not sure if this helps (or makes sense to) anybody else, but it sure helped me. :-)

Comment 184

[Justin Lebar (not reading bugmail)]

> The URL.createObjectURL call for the conversion seems to be costly and cause the 
> flickering.

AIUI this is literally just a hashtable lookup.  I would be surprised if it's the slow part.  You can time it to see.

> We actually slide the next page over the current one, but we slide the current page away 
> from the previous one. I think of it as a stack of paper.

There's an obvious disadvantage to this approach, which is that dynamic objects stop animating when you go back.  Also the memory usage is worse.  Do you do it this way because you need to move the current page, and this is the only way to do that?

Comment 185

[Stephen A Pohl [:spohl]]

(In reply to Justin Lebar [:jlebar] from comment #184)
> > The URL.createObjectURL call for the conversion seems to be costly and cause the 
> > flickering.
> 
> AIUI this is literally just a hashtable lookup.  I would be surprised if
> it's the slow part.  You can time it to see.
> 

Good point. I'll look into that.

> > We actually slide the next page over the current one, but we slide the current page away 
> > from the previous one. I think of it as a stack of paper.
> 
> There's an obvious disadvantage to this approach, which is that dynamic
> objects stop animating when you go back.  Also the memory usage is worse. 
> Do you do it this way because you need to move the current page, and this is
> the only way to do that?

I think this approach was chosen because it matches what Safari is doing and what people are used to on OSX, the stoping of dynamic objects included.

I may have a way to use blobs without the flickering, but I need to try this out first.

Comment 186

[Stephen A Pohl [:spohl]]

Attachment: Patch

This patch implements compression via <canvas>.toBlob(). Flickering is avoided by using the raw canvas of the current page during swipe animations. The blobs for the previous and next pages are recovered from the array of snapshots and converted to an image when necessary for animations.

Comment 187

[Mozilla RelEng Bot]

Try run for faaee8083d6a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=faaee8083d6a
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-faaee8083d6a

Comment 188

[José Jeria]

(In reply to Mozilla RelEng Bot from comment #187)
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> faaee8083d6a

This build seems to be sluggish compared to the build in comment 162. It kind of freezes for a milisecond when going back/forward. It is more noticeable when you go forward. The animation kind of stops for a ms before reaching the left. This was tested with www.dn.se and Flash disabled.

Comment 189

[Josiah Bruner [:jsbruner]]

I agree with Jose. The swiping does not seem as smooth as before, in fact, I had quite some problems getting the swiping to complete because of the stuttering. I tested on Cnet.com with Flash. 

Note: I am not using the builds, just the patches, although I believe they should be the same thing.

Comment 190

[Josiah Bruner [:jsbruner]]

To compare this issue. I have started a test on memory use. I am starting with the new build. You can view results here: https://docs.google.com/spreadsheet/ccc?key=0Am1zicD3k5IfdEFZT05rMjFYLTI2WU5zMWFLZS1oYnc

I will update that as I get more results. This is a "normal" browsing experience. However, for testing purposes I will never close tabs. After this test, I will do a similar test on the previous build.

Comment 191

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #189)
> I agree with Jose. The swiping does not seem as smooth as before, in fact, I
> had quite some problems getting the swiping to complete because of the
> stuttering. I tested on Cnet.com with Flash. 
> 
> Note: I am not using the builds, just the patches, although I believe they
> should be the same thing.

Thanks for testing Jose and Josiah. This matches what I've observed as well. I'm currently looking into possible improvements and will be discussing them shortly.

Comment 192

[Josiah Bruner [:jsbruner]]

Just in case anybody wants to compare, here are the first twenty minutes of the previous build: https://docs.google.com/spreadsheet/ccc?key=0Am1zicD3k5IfdFc5OGRTR2h1UE1JWGJqVHYxTF8xV1E

Comment 193

[Stephen A Pohl [:spohl]]

There are two main issues with the current patch:

1. The current snapshot compression causes the animation to become sluggish.
2. There is no global limit to the number of snapshots a browser session could be holding on to in memory.

I'd like to share a few ideas to address these issues. Please provide any feedback that you have and if you have suggestions how/where to implement this, please don't hold back.

1. In order to avoid sluggishness during the animation, I would like to compress the snapshot off the main thread. <canvas>.toBlob() doesn't currently do this (bug 817700), causing the animation to stutter. This might just be the right incentive to look at 817700 more closely once I've addressed the second issue below.

2. If we think of all the snapshots that the browser will take, they seem to be easily representable as a doubly-linked list. This approach has a few advantages:
* If we store a "global" (read: accessible from all tabs) doubly-linked list, the individual nodes would hold data that identifies the tab in which the snapshot was taken as well as the index in the array of snapshots for that tab.
* The ordering of the linked list would be most-recent visited page (head) to oldest (tail).
* The per-tab limit of snapshots could be replaced by a global limit.
* New snapshots are still added to the per-tab array of snapshots, but the tab identifier and array index is also pushed to the head of the linked list.
* Snapshots beyond the limit are deleted from the per-tab array and removed from the tail of the linked list.
* There are some implementation details that would need some thought put into. For example, what happens if the user goes back a few pages and then browses to a different website, with snapshots overwriting existing snapshots in the array that are also referenced from the middle of the linked list. However, nothing strikes me as unsolvable or very costly from a performance standpoint.

The advantage of this linked-list solution is that we have a way of predicting the amount of memory required for this feature to work across all users: users with only a few tabs as well as users with hundreds of tabs. If we're generous and set the global limit to 100 snapshots, and assuming that each snapshot takes 1MB, the maximum amount of memory required would be 100MB (plus some overhead). In a next step we could further decrease this by storing the "older" snapshots on disk rather than in memory while providing a fantastic experience: swipe animations with meaningful snapshots would work across the last 100 pages in history!

Comment 194

[Josiah Bruner [:jsbruner]]

I will examine your other thoughts shortly, but I would like to throw out a suggestion. In order to save some memory, what if "old" snapshots were converted to grayscale, but I mean really old ones. For example, if the user has say 5 tabs, each tab has a website, and in each website they navigate. Then we would have quite a bit of snapshots already. 

Then, let's say the user navigates away to a different page on a new site and then navigates somewhere in there. We can conclude then that most likely, they will not go back all the way to the first page of the first site on that tab. So, we could take quite a few snapshots for the previous site and convert them to grayscale (Perhaps other memory-saving techniques as well.) That way, in case they do go back, an image is there, it just looks faded to the user, not a bad experience in my experience. Then, as the site loads, perhaps we just animate the snapshot away, which will make it look like the site is rebuilding.

This is based off of a browser I used on iOS, K9 browser. However, it used grayscale when it was "closed", then as the page reloads, it animated into a normal page. Of course, I'm not sure how well this would work, as I am mostly an Objective-C person, and ARC handles a lot of this. Let me know what you think.

Comment 195

[Josiah Bruner [:jsbruner]]

Also, for a final version of my testing see here: https://docs.google.com/spreadsheet/ccc?key=0Am1zicD3k5IfdHZHNDEzRFI3MnRTODUzVXNVTHY5aUE

You can see the improvements made with the new swiping compression, but surprisingly it performs worse to the user. This is a result of what Stephen mentioned:

"1. The current snapshot compression causes the animation to become sluggish."

Comment 196

[Josiah Bruner [:jsbruner]]

I like all of your ideas. Also, storing snapshots on the hard drive is something I was going to mention, so that does seem like a good idea. In that case, we could do something like I mentioned. Very old snapshots could be converted to grayscale, then saved on the hard drive. That of course, gives us many more snapshots, but we could read it from the hard drive quicker.

Anyway, I agree with all your suggestions. Creating a doubly-linked list is the way to go. At least for now.

Comment 197

[Stephen A Pohl [:spohl]]

Attachment: Patch

Patch with doubly-linked list implemented. Will need to test some more and write automated tests.

In order to test the eviction of old snapshots, set browser.snapshots.limit to 3 in about:config (the default is currently 100). You should now be able to see that if you swipe past the last three pages in history, older pages will appear blank. Note that this works across tabs. The snapshot limit no longer applies per tab.

I'm going to kick off a try build that includes Markus' latest patch to bug 817700 for async <canvas>.toBlob(). This will bring the performance closer to what we ultimately want.

Comment 198

[Stephen A Pohl [:spohl]]

Attachment: Patch

Removed id property on linked list node (previously used for debugging) and some whitespace.

Comment 199

[Josiah Bruner [:jsbruner]]

Thanks Stephen. I need to re-clone the mozilla source, and then I will apply the patch. Afterward, I will do another memory test and compare to previous results. If needed, I will try again with Markus' patch as well.

Comment 200

[Mozilla RelEng Bot]

Try run for 77a73eb5eeff is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=77a73eb5eeff
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-77a73eb5eeff

Comment 201

[beingalink]

One thing I'd like to annotate, also with regard to my comment 159, is that the page I'm navigating to shouldn't start to get reloaded during the animation process. It should wait till the animation finished and then reload the page if needed. Perhaps this would already solve mentioned issue and possibly improve the overall visual impression.

Comment 202

[Josiah Bruner [:jsbruner]]

To, I finished test results. However, I am not sure how accurate these are, as the different sites, pages, or something else could interfere. Occurring to the test, this new patch made Firefox's memory be pretty close to what it was without any memory management at all. 

Of course, my test could just be messed up, but I do know that it is not using much more memory than before. 

Now, I have seen some problems. First, like beingalink said, I'm not sure if reloading while swiping is a good idea. I do not believe Safari does this, and this might be what is causing the swiping animation to not perform as smoothly. That said, I have yet to try the build with Markus' patch, so perhaps that will improve the swiping. This is slightly odd, as performance is improved, but so far to the user, the swiping seems to be worse. I am hoping Markus' patch fixes this, but if not, then reloading will need to be done after the swipe.

Second, there have been occurrences that even when the screenshot taking limit is set at 100, that certain pages still swipe with a blank page. Actually, they tend to start with an image, but then it disappears. I have had that happen a few times on Cnet.com.

Either way, if you would like to see the memory comparison, see: https://docs.google.com/spreadsheet/ccc?key=0Am1zicD3k5IfdHZHNDEzRFI3MnRTODUzVXNVTHY5aUE#gid=0

Comment 203

[Josiah Bruner [:jsbruner]]

Oh, I'm also having a problem where sometimes swiping back to a page doesn't actually remove the picture from view, but it appears to have done so. What I mean is that it looks like I am on the previous page, but nothing is working. You can't click on anything, scroll, so on and so fourth. 

However, if you slightly slide the page forward and then back again, the page will then load correctly.

Comment 204

[Sergey Reymerov]

I've tried build from comment 200. There are some lags on it (in comparison with Safari 6). Suppose it just debug (or beta) problem and may be doesn't matter for this bug.
Also I've found that there is no take into pixel ratio for Retina Display. So when I swipe fingers current and prev picture makes with big pixels. When I release fingers page view become normal. Support for retina resolved in bug 674373.

Comment 205

[Josiah Bruner [:jsbruner]]

Yes, I just tested with Markus' patch, and although improved, it does not fix the problem. The swiping is still quite lagy, especially when compared to Safari and the swiping build before the memory management was started.

This could still be from the reloading issue. We should try another patch/build where the page reloads after the swiping is complete. Perhaps make it work like Safari, where if the page needs to be reloaded after the swipe, the page fades white for a bit, then turns back to normal after the reload. It is a nice effect and appears to make things work faster.

We could do the same with grayscale or something else.

Comment 206

[Stephen A Pohl [:spohl]]

(In reply to Sergey Reymerov from comment #204)
> I've tried build from comment 200. There are some lags on it (in comparison
> with Safari 6). Suppose it just debug (or beta) problem and may be doesn't
> matter for this bug.

The swipe animation should definitely be tested with the optimized build, not debug. It would be great to know if you're still seeing the lag with the optimized build.

> Also I've found that there is no take into pixel ratio for Retina Display.
> So when I swipe fingers current and prev picture makes with big pixels. When
> I release fingers page view become normal. Support for retina resolved in
> bug 674373.

We're limited by the canvas drawing code. According to bug 780362, HiDPI isn't working right now.

Comment 207

[Stephen A Pohl [:spohl]]

I'll keep looking into

Comment 208

[Stephen A Pohl [:spohl]]

Hmm, my previous comment 207 got cut off. I meant:

I'll keep looking into the "reloading during animation" and "showing of previous page while loading the new page" problems.

It would be great to get some feedback regarding the handling of snapshots across tabs. Is the browser.snapshots.limit property working for you? If you swipe past the limit, do the old pages appear blank during the animation? I highly recommend testing with the optimized try builds (or with the latest patch from bug 817700 applied) to keep animation performance at an acceptable level.

Comment 209

[Josiah Bruner [:jsbruner]]

Yep. That snapshot property is working properly. I still suggest having the white page fade in to the reloaded page, as the page suddenly jumping from blank to content isn't visually appealing. Other than that, the global snapshot limit works like a charm. So far at least.

Comment 210

[Stephen A Pohl [:spohl]]

Attachment: Patch

This patch addresses the "reloading during animation" problem. We will now wait for the animation to complete before reloading the page we navigated to. Just from visual comparison, this seems to match what Safari is doing. You should find that animations are much smoother now and complete fully before the new page is reloaded.

A side-effect of this improvement is that the "showing of previous page while loading the new page" is occurring more frequently. I'm still looking into this.

Also removed some attachments (screenshots) that have become obsolete.

Comment 211

[Stephen A Pohl [:spohl]]

Attachment: Patch

Avoid blank pages during fast swiping when the snapshot compression can't complete quickly enough.

Comment 212

[Josiah Bruner [:jsbruner]]

Very nice. Speed works well, only problem is that old pages show up while the new one is loading. Hopefully Stephen will find a fix soon. As I am sure Stephen will have a better try server build up soon, I just decided to make a quick .dmg with a build containing the latest patch here, and Markus'. I only built once, so I can not be sure it will work on your system, but I believe it should. 

https://www.dropbox.com/s/pw01575l7ucemxe/Firefox-Scroll-4.dmg?dl=1

Comment 213

[Josiah Bruner [:jsbruner]]

Oops. It didn't let me finish. Anyway, after Stephen adds a try build I will remove the file, therefore the link will be broken.

Comment 214

[Josiah Bruner [:jsbruner]]

Attachment: Demonstrates HiDPI problem with swiping.

I also added a screenshot showing the HiDPI problem. However, I don't believe we can do anything about that in this case.

Comment 215

[Stephen A Pohl [:spohl]]

try builds are struggling, but should eventually show up.

I noticed that when swiping in the forward direction, I'm intermittently able to pull the next page further across the previous page than what I should be able to. This causes the previous page to appear on the right. We'll want to make sure that the animation box stops before this happens.

Comment 216

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #215)
> try builds are struggling, but should eventually show up.
> 
> I noticed that when swiping in the forward direction, I'm intermittently
> able to pull the next page further across the previous page than what I
> should be able to. This causes the previous page to appear on the right.
> We'll want to make sure that the animation box stops before this happens.

Right. Also, if you are using a trackpad, perhaps only the magic trackpad, I am able to take a third finger and keep swiping over to move the page. However, there is no limit. That means I am able to actually push the page completely off the view, leaving only the texture background. The animation will need to be limited from both sides.

Comment 217

[Mozilla RelEng Bot]

Try run for ea95ca4398bd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ea95ca4398bd
Results (out of 4 total builds):
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ea95ca4398bd

Comment 218

[Josiah Bruner [:jsbruner]]

(In reply to Mozilla RelEng Bot from comment #217)
> Try run for ea95ca4398bd is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=ea95ca4398bd
> Results (out of 4 total builds):
>     failure: 4
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> ea95ca4398bd

Well... That didn't turn out to well. My build is still up though, but it did go down for a little bit. It should be working now. Hopefully the try server will be up soon.

Comment 219

[Mozilla RelEng Bot]

Try run for 87683cddc69b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=87683cddc69b
Results (out of 2 total builds):
    success: 1
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-87683cddc69b

Comment 220

[Mozilla RelEng Bot]

Try run for ea95ca4398bd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ea95ca4398bd
Results (out of 6 total builds):
    failure: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ea95ca4398bd

Comment 221

[Stephen A Pohl [:spohl]]

Optimized try build is available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-87683cddc69b/try-macosx64/firefox-20.0a1.en-US.mac.dmg

Comment 222

[Mozilla RelEng Bot]

Try run for 933f128d75a5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=933f128d75a5
Results (out of 2 total builds):
    success: 1
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-933f128d75a5

Comment 223

[José Jeria]

(In reply to Stephen Pohl [:spohl] from comment #221)
> Optimized try build is available here:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 87683cddc69b/try-macosx64/firefox-20.0a1.en-US.mac.dmg

This build works great. Very smooth animation. Great!

Comment 224

[Mieszko Ślusarczyk]

Works great here as well!

MacBook Air (2011), OS X 10.8.2

Darwin mieszko-macbook1.home 12.2.0 Darwin Kernel Version 12.2.0: Sat Aug 25 00:48:52 PDT 2012; root:xnu-2050.18.24~1/RELEASE_X86_64 x86_64

Comment 225

[Luca D.]

Works great here too, Macbook Air as ^.

When will it be merged with official Nightly?

Comment 226

[Josiah Bruner [:jsbruner]]

We need Markus's patch to merge either first or with this patch. Most likely we'll wait till his goes through  We should shoot for Firefox 21. Perhaps sooner.

Comment 227

[Stephen A Pohl [:spohl]]

Attachment: Patch

This fixes the issue where we would display the old page while the new page is loading. This is particularly visible by using the sites in comment 168:
1. Go to http://www.last.fm/user/TheLink
2. Then go to https://encrypted.google.com
3. Now swipe back to the last.fm page.

This patch doesn't address the following problems yet:
1. Fast swiping, i.e. starting a new swipe animation before another one ends. This is intentionally broken right now since I'm working on it.
2. Swiping past the bounds of the page, displaying the box underneath.
3. Making swipe animations available if (and only if) [NSEvent isSwipeTrackingFromScrollEventsEnabled].

Comment 228

[Mozilla RelEng Bot]

Try run for 2100b0bd1171 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2100b0bd1171
Results (out of 2 total builds):
    exception: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2100b0bd1171

Comment 229

[Stephen A Pohl [:spohl]]

(In reply to Mozilla RelEng Bot from comment #228)

This try run was cancelled. New one is running now.

Comment 230

[Mozilla RelEng Bot]

Try run for ec7a9b756db6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ec7a9b756db6
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ec7a9b756db6

Comment 231

[Josiah Bruner [:jsbruner]]

Well, it works great so far. Fast, reliable. We just need the few things you (Stephen) mentioned to work. We're getting pretty close to finishing this. Thanks Stephen.

Comment 232

[Josiah Bruner [:jsbruner]]

Unfortunately, I found another bug. If you go to the Add-On page, just try navigating around in there. Go to extensions, then to plugins and so fourth. Then try swiping back and forward to navigate. Sometimes it will work, but almost always it eventually turns all pages into "blank" pages and you then get stuck.

Comment 233

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #232)
> Unfortunately, I found another bug. If you go to the Add-On page, just try
> navigating around in there. Go to extensions, then to plugins and so fourth.
> Then try swiping back and forward to navigate. Sometimes it will work, but
> almost always it eventually turns all pages into "blank" pages and you then
> get stuck.

I've been unable to reproduce this, unless I fast swipe (which I'm still working on). Could you confirm that even when all pages have time to reload after a swipe, this bug still reproduces?

Comment 234

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #233)
> (In reply to Josiah from comment #232)
> > Unfortunately, I found another bug. If you go to the Add-On page, just try
> > navigating around in there. Go to extensions, then to plugins and so fourth.
> > Then try swiping back and forward to navigate. Sometimes it will work, but
> > almost always it eventually turns all pages into "blank" pages and you then
> > get stuck.
> 
> I've been unable to reproduce this, unless I fast swipe (which I'm still
> working on). Could you confirm that even when all pages have time to reload
> after a swipe, this bug still reproduces?

No, I am pretty sure it is another error. I can reproduce it. I do not believe I am swiping fast, and the page appears to load fully. Once my video is uploaded, I will link it here, and you can look at what I did and decide if it is another problem.

Comment 235

[Josiah Bruner [:jsbruner]]

https://www.dropbox.com/s/mggdeho5xhaz0au/Bug.movhttps://www.dropbox.com/s/mggdeho5xhaz0au/Bug.mov

There's the link. Take a look at it yourself.

Comment 236

[Josiah Bruner [:jsbruner]]

https://www.dropbox.com/s/mggdeho5xhaz0au/Bug.mov

Sorry. Messed up the last one. My keyboard is wrecked right now. I can barely type.

Comment 237

[José Jeria]

Does it really make sense to have the swipe animations in the settings page? Swiping the menu as well looks odd.

Comment 238

[Josiah Bruner [:jsbruner]]

(In reply to José Jeria from comment #237)
> Does it really make sense to have the swipe animations in the settings page?
> Swiping the menu as well looks odd.

I have to agree. In many cases swiping looks great, but there I'm not sure it is the way to go. Since the menu was made to be navigated like an app, swiping doesn't look right there. You should still be able to physically swipe, but the animation needs to go.

Comment 239

[Stephen A Pohl [:spohl]]

Attachment: Patch

This patch:
* implements support for fast swiping.
* fixes the issue of pages being swipable past the bounds of the window.
* fixes some instances of flickering when swipe animations stop.

Still to do:
* Support for swipe animations should be conditional on [NSEvent isSwipeTrackingFromScrollEventsEnabled] (most likely requires new component interface).
* Might want to revise the dispatching of swipe events in nsChildView.mm for better fast swiping support.

Try build is running.

Comment 240

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #238)
> (In reply to José Jeria from comment #237)
> > Does it really make sense to have the swipe animations in the settings page?
> > Swiping the menu as well looks odd.
> 
> I have to agree. In many cases swiping looks great, but there I'm not sure
> it is the way to go. Since the menu was made to be navigated like an app,
> swiping doesn't look right there. You should still be able to physically
> swipe, but the animation needs to go.

I was able to track this down to the fact that pageshow and pagehide events aren't being properly dispatched when navigating in the add-ons manager. There might be a workaround for it, but I'm not sure I'll be able to cover this as part of this bug.

If you see this behavior on any real website (i.e. not on a 'built-in' page like the add-ons manager), please speak up.

Comment 241

[Mozilla RelEng Bot]

Try run for ef2fe216bd52 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ef2fe216bd52
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ef2fe216bd52

Comment 242

[Chris Barry [ChrisW_B]]

Is there something in the new build that would affect three finger swipe gestures? I've had three finger swipe up/down set to open panorama, which no longer seems to work, nor does the shift+3 finger to scroll to top/bottom. Running 10.8.2 on a 2012 Macbook Air

Comment 243

[Josiah Bruner [:jsbruner]]

(In reply to Chris Barry [CB_27] from comment #242)
> Is there something in the new build that would affect three finger swipe
> gestures? I've had three finger swipe up/down set to open panorama, which no
> longer seems to work, nor does the shift+3 finger to scroll to top/bottom.
> Running 10.8.2 on a 2012 Macbook Air

Are you talking about swipe gestures from System Preferences? Or from another application? I'm not sure what "open panorama" means.

Comment 244

[Josiah Bruner [:jsbruner]]

I have found another bug that is similar to the Add-On bug, however it has to do with an actual site (Mozilla.org actually). This error appears to happen when a site changes it's content based on changes such as #mobile and #desktop. This causes white pages to appear, on those sites. 

To Reproduce: 

Open a new tab

Go to mozilla.org

Click on Mozilla->Firefox

(You should not be on the Firefox page that shows tabs for mobile and desktop.)

Click on the other tab and then go back and forward.  Then try swiping back, for every change containing #whatever, there is a white page instead of an image. It does this until you leave https://www.mozilla.org/en-US/firefox/fx/.

Comment 245

[Josiah Bruner [:jsbruner]]

Sorry, when I said go back and forward, I mean click on the alternate mobile/desktop tabs.

Comment 246

[Chris Barry [ChrisW_B]]

(In reply to Josiah from comment #243)
> (In reply to Chris Barry [CB_27] from comment #242)

> Are you talking about swipe gestures from System Preferences? Or from
> another application? I'm not sure what "open panorama" means.


Sorry, I had modified the gesture in about:config to open panorama (cmd+shift+e), the default for a 3 finger up/down swipe is to scroll to the top or bottom of the page, which still does not work in a clean profile for this build. However, it still works in other applications, so it seems to be only this build of firefox

Comment 247

[Josiah Bruner [:jsbruner]]

(In reply to Chris Barry [CB_27] from comment #246)
> (In reply to Josiah from comment #243)
> > (In reply to Chris Barry [CB_27] from comment #242)
> 
> > Are you talking about swipe gestures from System Preferences? Or from
> > another application? I'm not sure what "open panorama" means.
> 
> 
> Sorry, I had modified the gesture in about:config to open panorama
> (cmd+shift+e), the default for a 3 finger up/down swipe is to scroll to the
> top or bottom of the page, which still does not work in a clean profile for
> this build. However, it still works in other applications, so it seems to be
> only this build of firefox

Odd. I did not see anything in about:config about three-finger gestures. Three finger swiping never did anything on any version of Firefox for me, however, I do have mission control set with that gesture.

Comment 248

[Chris Barry [ChrisW_B]]

The entry is "browser.gesture.swipe.down", and if I set it to default, the value is "cmd_scrollBottom", but I think it will only work if mission control is set to 4 fingers, and Swipe between pages is set to Swipe with 2 or 3 fingers

Comment 249

[Josiah Bruner [:jsbruner]]

(In reply to Chris Barry [CB_27] from comment #248)
> The entry is "browser.gesture.swipe.down", and if I set it to default, the
> value is "cmd_scrollBottom", but I think it will only work if mission
> control is set to 4 fingers, and Swipe between pages is set to Swipe with 2
> or 3 fingers

Hmm. I can't get the gesture to work even with Mission control off. (On the regular Nightly)

Comment 250

[Chris Barry [ChrisW_B]]

(In reply to Josiah from comment #249)

> 
> Hmm. I can't get the gesture to work even with Mission control off. (On the
> regular Nightly)

Hmm, I only have UX and This version of nightly, so I suppose it could be a problem with the newest Nightly build

Comment 251

[Josiah Bruner [:jsbruner]]

Very possible. When I get a chance I'll try the Aurora build.

Comment 252

[Stephen A Pohl [:spohl]]

(In reply to Chris Barry [CB_27] from comment #242)
> Is there something in the new build that would affect three finger swipe
> gestures? I've had three finger swipe up/down set to open panorama, which no
> longer seems to work, nor does the shift+3 finger to scroll to top/bottom.
> Running 10.8.2 on a 2012 Macbook Air

The latest patch is unlikely to be the reason for this behavior (there were no changes that would intercept/affect these gestures). If you can reproduce in the regular nightly, please file a new bug.

Comment 253

[Josiah Bruner [:jsbruner]]

Also, I can not reproduce this behavior on any version of Firefox. However, if you can get the behavior working on Firefox, but not the Nightly, then you should file a bug.  

http://nightly.mozilla.org/

Comment 254

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #244)
> Click on the other tab and then go back and forward.  Then try swiping back,
> for every change containing #whatever, there is a white page instead of an
> image. It does this until you leave
> https://www.mozilla.org/en-US/firefox/fx/.

I can reproduce this. Will have to look into it. Thanks!

Comment 255

[Chris Barry [ChrisW_B]]

(In reply to Stephen Pohl [:spohl] from comment #252)
> The latest patch is unlikely to be the reason for this behavior (there were
> no changes that would intercept/affect these gestures). If you can reproduce
> in the regular nightly, please file a new bug.

I've just downloaded the latest version of Nightly, and all gestures are working fine, and scroll to the top/bottom when on a clean profile, as they should. I've attached a screenshot of my trackpad settings and my about:config entries

Comment 256

[Chris Barry [ChrisW_B]]

Attachment: Settings to Reproduce 3 Finger Swipe

Comment 257

[Stephen A Pohl [:spohl]]

(In reply to Chris Barry [CB_27] from comment #255)
> (In reply to Stephen Pohl [:spohl] from comment #252)
> I've just downloaded the latest version of Nightly, and all gestures are
> working fine, and scroll to the top/bottom when on a clean profile, as they
> should. I've attached a screenshot of my trackpad settings and my
> about:config entries

Did this start happening with the very latest patch, or do previous try builds in this bug reproduce the same problem?

Comment 258

[Chris Barry [ChrisW_B]]

(In reply to Stephen Pohl [:spohl] from comment #257) 
> Did this start happening with the very latest patch, or do previous try
> builds in this bug reproduce the same problem?

It only started occurring with the latest trybuild, I've been using this daily without problem before

Comment 259

[Stephen A Pohl [:spohl]]

(In reply to Chris Barry [CB_27] from comment #258)
> (In reply to Stephen Pohl [:spohl] from comment #257) 
> > Did this start happening with the very latest patch, or do previous try
> > builds in this bug reproduce the same problem?
> 
> It only started occurring with the latest trybuild, I've been using this
> daily without problem before

And if you go back to previous try builds today, you're still unable to reproduce the problem? I just want to make sure that nothing else has changed on your system and we're really dealing with a new problem in the latest patch.

Comment 260

[Chris Barry [ChrisW_B]]

(In reply to Stephen Pohl [:spohl] from comment #259)
> And if you go back to previous try builds today, you're still unable to
> reproduce the problem? I just want to make sure that nothing else has
> changed on your system and we're really dealing with a new problem in the
> latest patch.

Just downloaded and installed the build in Comment #230, and everything works, I'm assuming I didn't accidentally scroll past a more recent one

Comment 261

[Stephen A Pohl [:spohl]]

(In reply to Stephen Pohl [:spohl] from comment #254)
> (In reply to Josiah from comment #244)
> > Click on the other tab and then go back and forward.  Then try swiping back,
> > for every change containing #whatever, there is a white page instead of an
> > image. It does this until you leave
> > https://www.mozilla.org/en-US/firefox/fx/.
> 
> I can reproduce this. Will have to look into it. Thanks!

Interestingly enough, Safari has difficulties with this page as well. Especially fast swiping causes the page to become unresponsive (i.e. links aren't clickable) and after a few seconds, the page turns blank.

Comment 262

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #261)
> (In reply to Stephen Pohl [:spohl] from comment #254)
> > (In reply to Josiah from comment #244)
> > 
> > I can reproduce this. Will have to look into it. Thanks!
> 
> Interestingly enough, Safari has difficulties with this page as well.
> Especially fast swiping causes the page to become unresponsive (i.e. links
> aren't clickable) and after a few seconds, the page turns blank.

That is odd. Perhaps the website needs some work. In the meantime, it should still be looked at. I think it has to do with javascript. I could be wrong though.

Comment 263

[Stephen A Pohl [:spohl]]

(In reply to Chris Barry [CB_27] from comment #260) 
> Just downloaded and installed the build in Comment #230, and everything
> works, I'm assuming I didn't accidentally scroll past a more recent one

Could you give this build a shot and tell me if it works or not? Thanks!
http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-8094a9b9168c/try-macosx64/firefox-20.0a1.en-US.mac.dmg

Comment 264

[AbdulElah]

@Stephen Pohl

I just tried your build, i don't know how to use patches so i was happy to see your build, its OK but if you go a little bit fast you won't be happy with the animation.

anyway nice work and keep improving it, thanks

Comment 265

[Chris Barry [ChrisW_B]]

(In reply to Stephen Pohl [:spohl] from comment #263)
> Could you give this build a shot and tell me if it works or not? Thanks!
> http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/spohl@mozilla.
> com-8094a9b9168c/try-macosx64/firefox-20.0a1.en-US.mac.dmg

yep, works fine in that build!

Comment 266

[Josiah Bruner [:jsbruner]]

(In reply to Chris Barry [CB_27] from comment #265)
> (In reply to Stephen Pohl [:spohl] from comment #263)
> > Could you give this build a shot and tell me if it works or not? Thanks!
> > http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/spohl@mozilla.
> > com-8094a9b9168c/try-macosx64/firefox-20.0a1.en-US.mac.dmg
> 
> yep, works fine in that build!

@Stephen, What changes were made in that build?

Comment 267

[Stephen A Pohl [:spohl]]

(In reply to Chris Barry [CB_27] from comment #265)
> (In reply to Stephen Pohl [:spohl] from comment #263)
> > Could you give this build a shot and tell me if it works or not? Thanks!
> > http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/spohl@mozilla.
> > com-8094a9b9168c/try-macosx64/firefox-20.0a1.en-US.mac.dmg
> 
> yep, works fine in that build!

Interesting. What about the build below? This should give me enough info to proceed:
http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-b49ce8c01272/try-macosx64/firefox-20.0a1.en-US.mac.dmg

Comment 268

[Stephen A Pohl [:spohl]]

(In reply to AbdulElah from comment #264)
> its OK but if you go a little bit fast you won't be happy
> with the animation.

Thanks for the feedback. Would you mind trying the build at the bottom of comment 267 and let me know if the animation is better during fast swiping?

Comment 269

[Chris Barry [ChrisW_B]]

(In reply to Stephen Pohl [:spohl] from comment #267)
> Interesting. What about the build below? This should give me enough info to
> proceed:
> http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/spohl@mozilla.
> com-b49ce8c01272/try-macosx64/firefox-20.0a1.en-US.mac.dmg

nope, doesnt work in that one

Comment 270

[Stephen A Pohl [:spohl]]

Attachment: Patch

Fixes issue with three finger swipe up/down

Comment 271

[Stephen A Pohl [:spohl]]

(In reply to Chris Barry [CB_27] from comment #269)
> (In reply to Stephen Pohl [:spohl] from comment #267)
> > http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/spohl@mozilla.
> > com-b49ce8c01272/try-macosx64/firefox-20.0a1.en-US.mac.dmg
> 
> nope, doesnt work in that one

I was able to reproduce the problem. This should be fixed with the latest try build:
http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-ce2ffcc18114/try-macosx64/firefox-20.0a1.en-US.mac.dmg

Comment 272

[Josiah Bruner [:jsbruner]]

So far, that build works great. However, one issue still needs to be addressed, although it is not severe. If you have trackpad (I have the magic trackpad) you can swipe with two fingers, but use a third finger to keep pushing the page. I could go on forever. I am able to completely remove the from view leaving nothing but the textured background.

This should not happen. Safari stops you about half way on the window.

Comment 273

[Josiah Bruner [:jsbruner]]

(In reply to Josiah from comment #244)
> I have found another bug that is similar to the Add-On bug, however it has
> to do with an actual site (Mozilla.org actually). This error appears to
> happen when a site changes it's content based on changes such as #mobile and
> #desktop. This causes white pages to appear, on those sites. 
> 
> To Reproduce: 
> 
> Open a new tab
> 
> Go to mozilla.org
> 
> Click on Mozilla->Firefox
> 
> (You should not be on the Firefox page that shows tabs for mobile and
> desktop.)
> 
> Click on the other tab and then go back and forward.  Then try swiping back,
> for every change containing #whatever, there is a white page instead of an
> image. It does this until you leave
> https://www.mozilla.org/en-US/firefox/fx/.

There seems to be a similar issue on Dropbox.

Comment 274

[Josiah Bruner [:jsbruner]]

Also, is this swiping animation completely done through Firefox/Gecko source? Or is this using NSPageController as the backbone?

Comment 275

[Josiah Bruner [:jsbruner]]

Also, I just noticed that Firefox is using 1.88 GB of memory for me. This is with pretty standard use. Is it because I haven't changed the snapshot-limit? Obviously, if it has to do with that, 100 can not be the standard setting.

Comment 276

[Josh Matthews [:jdm]]

about:memory would probably clear up where that memory usage comes from.

Comment 277

[Josiah Bruner [:jsbruner]]

What am I looking for that would contain the snapshots. Explict->images?

Comment 278

[Justin Lebar (not reading bugmail)]

There's no direct explicit -> images memory reporter.  Images are reported under the window which owns them.

Elements in about:memory are sorted by the amount of memory each element uses, so if you have hundreds of mb of image data /somewhere/, it should appear near the top of about:memory.

It's also possible that we don't have good memory reporters for this data and it will show up as heap-unclassified.

Comment 279

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #274)
> Also, is this swiping animation completely done through Firefox/Gecko
> source? Or is this using NSPageController as the backbone?

The animation itself is done in browser.js.

Comment 280

[Stephen A Pohl [:spohl]]

Attachment: Patch for availability check

This patch adds the ability to check whether or not swipe animations are available on a platform/configuration. The changes here are made to nsIDragService under widget. I'm not sure if this functionality should live in a component under widget, and I certainly don't think that it should be part of nsIDragService. I'm wondering:
1. Is there a good existing component to add this functionality to, or should I write an entirely new component?
2. Where should the component live?

nsIDragService was chosen because it demonstrates the ability to have different interface implementations for different platforms and I think this would be the right approach.

Comment 281

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 694540 [details] [diff] [review]
Patch for availability check

Leaving aside the question of which interface it belongs to, this looks fine to me.

But you need to make sure +[NSEvent isSwipeTrackingFromScrollEventsEnabled] is only called on OS X 10.7 and up, since that method doesn't exist on earlier versions of OS X.

Comment 282

[Steven Michaud [:smichaud] (Retired)]

> 2. Where should the component live?

I think it's best to put it somewhere under widget.

> 1. Is there a good existing component to add this functionality to,
>    or should I write an entirely new component?

I can't find an existing interface that fits better than
nsIDragService.  And yes, that's probably not the best match.

We have examples of platform-specific interfaces (for example
nsPIWidgetCocoa).  If you go that way it should be under widget/cocoa.

But I get the impression we'd like to have swiping support (and
support for swiping animation) on more than one platform.  So how
about you create a new nsISwiping interface under widget?

Comment 283

[Stephen A Pohl [:spohl]]

Attachment: Patch for availability check

Adds a new interface nsISwipeService to check for the availability of history swipe animations on a given platform.

Also fixed the method call on NSEvent to ensure that it is only called when available.

Waiting for try builds to finish before asking for feedback/review.

Comment 284

[Stephen A Pohl [:spohl]]

Attachment: Patch for availability check

I think this part of the patch can be reviewed before the rest, but please let me know if you'd rather review everything at the same time.

Comment 285

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 694922 [details] [diff] [review]
Patch for availability check

This looks fine to me.  Though creating platform-specific stubs for os2 and qt is probably overkill.

I've started a tryserver build to make sure this compiles on all platforms.  The results so far are all positive.  When the builds finish (presuming there are no failures), I'll r+ the patch.

Comment 286

[Stephen A Pohl [:spohl]]

Attachment: Patch

Improved the way we deal with fast swiping. This is now done in nsChildView.mm instead of browser.js and helps prevent potential race conditions between initiating the next swipe animation and tearing down the boxes with snapshots.

The last thing I'm going to look at before asking for a review is the issue encountered with named anchors (add-ons manager & mozilla.com website). This might be a simple fix and ideally, we'd want this as part of this patch.

If I'm missing anything crucial for this first patch, please let me know.

Comment 287

[Stephen A Pohl [:spohl]]

Attachment: Patch for availability check

Updated this patch to compile with attachment 695053 [details] [diff] [review]. Carrying over r+ flag since there were no functional changes. Thanks Steven for the review!

Comment 288

[Josiah Bruner [:jsbruner]]

(In reply to Steven Michaud from comment #282)
> We have examples of platform-specific interfaces (for example
> nsPIWidgetCocoa).  If you go that way it should be under widget/cocoa.
> 
> But I get the impression we'd like to have swiping support (and
> support for swiping animation) on more than one platform.  So how
> about you create a new nsISwiping interface under widget?

Yes, do that. We will want the swiping itself on other platform (Perhaps Windows 8, as it seems to be more gesture-based), and just the swiping animation is going to be implemented on all platforms. See bug 817074.

I am running a build now with all three patches, toBlob fix, swipe patch, and compatibility. 

(In reply to Stephen Pohl [:spohl] from comment #286)
> If I'm missing anything crucial for this first patch, please let me know.

I do not believe you are missing anything. As I have said before, a swipe animation limit could be added to keep the page from completely leaving the window, but that is optional. NSPageController that many apps use have this problem as well, and it does not really effect anything, I just find is slightly distracting.

Comment 289

[Stephen A Pohl [:spohl]]

Attachment: Patch

Fixed some more flickering, added function descriptions and improved the way we handle navigation to named anchors and popstate events.

On firefox.com and the add-ons manager, the initial swipe back to a previously visited site will still be blank. However, a swipe forward will now work with proper snapshots. With named anchors, popstate events take the place of pageshow events when swiping. I was unable to identify an event that is consistently dispatched when a named anchor is clicked, but before the anchor is navigated to. This would take the place of pagehide events in the context of named anchors. The consequence is that the pages are initially blank when navigating back to them. The most important thing is that we now always tear down the animation overlay when the swipe gesture ends. Previously, we could run into situations where the overlay remained on top of the page and the user could no longer interact with the page.

I believe this is ready for a first review. We may want to open a separate bug to further improve the navigation to named anchors.

Markus, since you had such a huge part in this patch it would be great if you could have a quick look at it too.

Comment 290

[Stephen A Pohl [:spohl]]

Attachment: Patch for availability check

Updated for latest patch, no functional changes. Carrying r+.

Comment 291

[Stephen A Pohl [:spohl]]

Try builds with latest changes:
http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-25af20f814d4/try-macosx64/

Comment 292

[beingalink]

This is getting ready to land. This try build is the first one where I didn't run into obvious glitches during the first few minutes. I'll do some further testing the next few days but in my opinion this bug is ready to land in the nightlies to become tested on a broader base. Thanks for all the hard work!

Comment 293

[Stephen A Pohl [:spohl]]

Comment on attachment 696097 [details] [diff] [review]
Patch

Adding Madhava for feedback from a UX perspective.

Comment 294

[Stephen A Pohl [:spohl]]

Attachment: Tests in progress

Browser chrome mochitests.
Main focus: doubly-linked list implementation.

Comment 295

[Justin Lebar (not reading bugmail)]

Before we can land this, we need to understand the memory usage effects of this patch.

How many screenshots does this patch cause us to keep in memory?  Are they compressed?  If so, with what algorithm?

We also need to experimentally confirm that the actual memory usage increase from this patch matches our expectations.

Comment 296

[Josiah Bruner [:jsbruner]]

Yes, there is quite a memory problem. I haven't tried the latest build yet, I will soon. Anyway, on the previous build I got up two 2.2 GB of memory. The regular nightly doesn't do that.

Comment 297

[Justin Lebar (not reading bugmail)]

Okay, thanks.  FYI we don't usually ask for review on patches which the author doesn't consider ready to be checked in.

Comment 298

[Stephen A Pohl [:spohl]]

(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #295)
> How many screenshots does this patch cause us to keep in memory?

Right, this may have gone under in the multitude of comments in this bug. We currently have a global limit of 100 snapshots. This number is not set in stone and I'm more than willing to change it to a number that makes sense. 100 is the current number since there haven't been any objections to this limit (yet).

>  Are they compressed?

We use <canvas>.toBlob() to compress the snapshot.

>  If so, with what algorithm?

image/png. My justification for this is that a loss-less algorithm seemed ideal for Retina displays. image/jpg did not result in a significant decrease in memory utilization, unless the quality was reduced. A quality of less than 100% quickly looked ugly on Retina displays.

> We also need to experimentally confirm that the actual memory usage increase
> from this patch matches our expectations.

My limited testing showed that the memory usage increase matched our expectations, but I'll have another focused look at this on Monday.

Comment 299

[Stephen A Pohl [:spohl]]

(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #297)
> Okay, thanks.  FYI we don't usually ask for review on patches which the
> author doesn't consider ready to be checked in.

The latest patch addressed the last outstanding issues (see comment 289). Since I haven't been able to reproduce a memory problem thus far, I considered this patch ready for a review and ultimately a check in. I'll have a closer look at memory utilization on Monday.

Comment 300

[Justin Lebar (not reading bugmail)]

To put this in perspective: Average memory usage is ~600mb for nightly users on Mac.  (Our telemetry numbers on Mac are pretty bad, but the more reliable Windows numbers peg it at ~500mb.)

At 2mb per image, 100 images would be a 1.3x (800/600) increase over the average memory usage.  Granted, I don't know how many browsing sessions would hit the 100 image cap.  But OTOH, I imagine the images would be larger than 2mb, particularly on retina displays.

I'd be much more comfortable with a 5 or 10 image cap to start with, personally.  We can add telemetry to see how often we have a cache miss versus a cache hit so as to tune this size.

Comment 301

[Josiah Bruner [:jsbruner]]

I agree. I would suggest we start the cap at 20 though. I am a pretty average web browser, and I found twenty to be pretty good. 5-10 seems small. 10 is okay. But you are right, telemetry could have us decide later.

Comment 302

[Asa Dotzler [:asa]]

I'd like something a bit better than "I am a pretty average web browser, and I found twenty to be pretty good." 

If we don't have science here, we should get it somehow. In absence of that, we should look at what other browsers do and strive for parity until we have our own science to apply.

Comment 303

[Josiah Bruner [:jsbruner]]

(In reply to Asa Dotzler [:asa] from comment #302)
> I'd like something a bit better than "I am a pretty average web browser, and
> I found twenty to be pretty good." 
> 
> If we don't have science here, we should get it somehow. In absence of that,
> we should look at what other browsers do and strive for parity until we have
> our own science to apply.

Indeed. And as for parity, we do have a problem there. Suppose we discover that Safari holds say about 30 snapshots. What then? Safari also has a much lower memory usage if I remember correctly. As much as I hate to say it, Firefox is a bit of a memory hog, especially on OS X. 

Now, I built again with the newest patches and memory no longer goes above ~600 MB, but that is still high. Anyway, I am just suggesting that always following the lead of other browsers may not help here. Instead, a science is necessary. 

I am quite new at this. So I am not sure how we would go about achieving some kind of test, but it definitely needs to be done. 

(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #300)
> At 2mb per image, 100 images would be a 1.3x (800/600) increase over the
> average memory usage.  Granted, I don't know how many browsing sessions
> would hit the 100 image cap.  But OTOH, I imagine the images would be larger
> than 2mb, particularly on retina displays.

It isn't really retina that is the problem, as the screenshot could still be say 800 x 600. The problem is the larger resolution screens. Obviously, if a user has a maximized window on a Cinema display, then the amount of content would be huge. Retina is more of a challenge, as the user is really displaying twice the content it usually does, assuming you ignore the change in PPI. (Am I correct here? I didn't work on taking screenshots, so I am not positive it works that way.)

Comment 304

[Josiah Bruner [:jsbruner]]

Oh, and what about the idea for using either lower resolution, grayscale, or something else to make images smaller past a point. We could then store about 10 regular images, and perhaps 5 grayscale ones. That would slightly lower the memory usage.

There was also an idea to store images on the hard drive? In light of the new patches, is this still a possibility, would it really help?

Comment 305

[Stephen A Pohl [:spohl]]

I found that about:memory reliably reports the memory utilization for the snapshots in memory under explicit > dom/memory-file-data/large.

A quick experiment has shown that a session with 20 snapshots uses 7.2MB of memory to store the compressed snapshots, or 0.36MB per snapshot on average. I was positively surprised by this, since I expected about the same 2MB per snapshot as Justin. The snapshots range from 0.04MB to 1.47MB (the last one was a complex image found via Google Image search). This is on a MacBook Pro, 15.4" retina display with 2880 x 1800 resolution. The browser was maximized to cause the snapshots to be as large as possible on this system.

Using about:memory, I was also able to confirm that the GC correctly collects old snapshots when they're being overwritten. Overwrites occur when an animation is initiated more than once, but the user ends up staying on the current page. They also occur when a user goes back in history and then navigates to new pages.

What may be working in our favor right now on retina displays is that the <canvas> drawing code doesn't support HiDPI (bug 780362). This may cause snapshots on systems with retina displays to be equal in size as snapshots on non-retina systems.

Having had a closer look at the memory utilization, I believe that it matches our expectations. I believe that the latest patch continues to be ready for a first review, with the exception of the final decision on the global limit of snapshots we want to keep. Safari seems to have a limit of roughly 20 snapshots and I'm inclined to change it to the same.

Personally, I don't think lower resolution images, grayscale or storing images on disk will solve a crucial problem at the moment. In fact, I'm afraid that the additional processing would slow down the browsing experience. I would prefer to set the global limit to something reasonable and that we drop older snapshots.

Comment 306

[Stephen A Pohl [:spohl]]

Attachment: Patch

Changed snapshot limit to 20 and made the list of snapshots a property of gHistorySwipeAnimation.

Comment 307

[Stephen A Pohl [:spohl]]

Attachment: Tests in progress

Updated tests for latest patch.

Comment 308

[Stephen A Pohl [:spohl]]

Try build for latest patch:
http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-2e0f3cf3dd6a/try-macosx64/firefox-20.0a1.en-US.mac.dmg

Comment 309

[Justin Lebar (not reading bugmail)]

FWIW I'm happy with 20 screenshots at < 10mb.  We can add telemetry to try to determine whether we should bump this up or down, but I wouldn't be disappointed if we didn't do that.  (Experiments are hard and shouldn't be undertaken lightly.)

Thanks a lot for checking this out, Stephen.

Comment 310

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated for current trunk.

Comment 311

[Stephen A Pohl [:spohl]]

Attachment: Tests

Some more tests and verified that they pass on platforms that don't support swipe gestures. Steven, I'm wondering if you could give me feedback whether or not these tests cover what we want to test before I ask for a review. I'm not currently testing the actual events, since there are some tests for those in the actual patch (in browser_gestureSupport.js).
The latest try run showed some errors, but they seemed unrelated to my new tests:
https://tbpl.mozilla.org/?tree=Try&rev=54a4c0f7ce47

Comment 312

[Stephen A Pohl [:spohl]]

Attachment: Patch

Fixed warnings in error console caused by GS_handleEvent in browser.js. No functional changes.

Comment 313

[Josiah Bruner [:jsbruner]]

FYI, Availability check patch is broken since the latest merge. The main swipe patch is fine though.

Comment 314

[Stephen A Pohl [:spohl]]

(In reply to Josiah from comment #313)
> FYI, Availability check patch is broken

The patch for bug 817700 was broken. I just updated the patch there.

Comment 315

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #314)
> (In reply to Josiah from comment #313)
> > FYI, Availability check patch is broken
> 
> The patch for bug 817700 was broken. I just updated the patch there.

And is that related to the availability patch?

Comment 316

[Stephen A Pohl [:spohl]]

The patch for the availability check doesn't seem to be broken for me. You rolled three patches into one for bug 817074. One of them was the patch for bug 817700, which was indeed broken.

Also, make sure that you apply the general patch for bug 678392 before you apply the patch for the availability check.

Comment 317

[Josiah Bruner [:jsbruner]]

That makes sense. I tried applying the availability check first.

Comment 318

[Josiah Bruner [:jsbruner]]

There is one potential issue I want to mention. It probably won't matter, but I want to make sure. 

We are taking snapshots of pages before the user leaves the page, and we keep it in memory. But could this cause a security vulnerability? It seems like if we take a snapshot of some confidential data, that any types of viruses could examine the memory and pull the snapshot. So, is this data being encrypted in any way? Does it even matter? 

I am not a security hacker, so I can not be sure of this. But we need to be sure that this is not an issue. Even if it is difficult, millions of people use Firefox. If hackers find a way to access this data, it could be pretty bad.

Comment 319

[Stephen A Pohl [:spohl]]

My understanding is that browser chrome privileges are necessary to access the snapshot arrays directly. With respect to malware, it is true that we keep the snapshots in memory and that malware may be able to access them. I'm hoping that we have general mitigations in place (like ASLR) that would automatically apply to the snapshots in memory as well. It would be great if someone could confirm.

Since performance is so important here it would be great if we could do without encryption.

Comment 320

[Josiah Bruner [:jsbruner]]

(In reply to Stephen Pohl [:spohl] from comment #319)
> My understanding is that browser chrome privileges are necessary to access
> the snapshot arrays directly. With respect to malware, it is true that we
> keep the snapshots in memory and that malware may be able to access them.
> I'm hoping that we have general mitigations in place (like ASLR) that would
> automatically apply to the snapshots in memory as well. It would be great if
> someone could confirm.
> 
> Since performance is so important here it would be great if we could do
> without encryption.

Of course. Most confidential information already has some security that is being displayed, like the ***** in passwords and such. But there are cases when raw information can be found. 

I'm bringing this up because of how hard Firefox pushes privacy. It would not go over well if something like this did happen. So, somebody should check this before it gets merged or at least before it merges to the normal channel.

Comment 321

[Stephen A Pohl [:spohl]]

Attachment: Patch for availability check

Updated patch for current trunk. Carrying r+.

Comment 322

[Josiah Bruner [:jsbruner]]

Here's another bug. If you are in a form page, and you swipe away, it will remove the page from view, but then pull down the sheet asking if you want to navigate away from the page (And lose data), or stay on it. If you choose to stay on it. Nothing happens. 

The "previous" page is shown and you can not get back to the other. If you swipe back, it will poppup the message again, and again, you can choose to stay on the page. Now it appears we are on the "right" page. However, there is no user interaction enabled.

Keep up the good work!

Comment 323

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 697633 [details] [diff] [review]
Patch

This looks fine to me.

I looked at all of it, but (of course) I paid special attention to the Cocoa widgets parts.

There is one fairly major problem -- the mCancelSwipeAnimation block pointer might be used uninitialized.  To fix this you should add "mCancelSwipeAnimation = nil;" just after the following line:

http://hg.mozilla.org/mozilla-central/annotate/b52c02f77cf5/widget/cocoa/nsChildView.mm#l2024

Also the #ifdef __LP64__ blocks around the swipe stuff can now be removed.  They were needed for as long as we still supported OS X 10.5, because 10.5 didn't have support for "blocks".  (We also didn't support 64-bit mode on 10.5, so we *could* support blocks in 64-bit mode, since we only compiled (and ran) 64-bit binaries on 10.6 and up.)  But now that we've stopped supporting 10.5, we can support swiping (and swipe animation) in both 64-bit mode and 32-bit mode.

I did some testing, and found no major problems.  But I'm going to postpone any really serious testing until after this patch has landed.  Then any problems that are found can be reported in separate bugs, rather than risking getting lost among all the comments that have already been made on this bug.

Comment 324

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed Steven's feedback. Gavin, would you have time to review the front-end pieces of this patch?

Comment 325

[Stephen A Pohl [:spohl]]

Attachment: Fix for animation getting stuck (intermittent)

This is a straightforward fix for the animation getting stuck in some scenarios.

Comment 326

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 705002 [details] [diff] [review]
Fix for animation getting stuck (intermittent)

Odd that this should make a difference.  But I don't think it can cause any harm.

Comment 327

[Stephen A Pohl [:spohl]]

(In reply to Steven Michaud from comment #326)
> Odd that this should make a difference.  But I don't think it can cause any
> harm.

Apologies for not providing a better explanation: We have a series of checks that need to pass before we start a new swipe animation. Without this patch, we abort the currently running tracking of gesture events (if any) before performing the checks. This patch ensures that the checks all pass before we abort tracking.

Specifically, when a user swiped slightly sideways (or quickly switched from horizontal to vertical swiping), this if statement would be true and cause the method to return:
  if ((deltaX == 0) || (fabs(deltaX) <= fabs(deltaY) * 8))
    return;

Since we had the tracking of gesture events aborted by the time we got here, the animation would be stuck. Note that just stopping the tracking does not mean that we necessarily tear down the animation overlay to avoid flickering, which is the case here.

I hope this clarifies things.

Comment 328

[Steven Michaud [:smichaud] (Retired)]

> I hope this clarifies things.

It does.  Thanks!

Comment 329

[Stephen A Pohl [:spohl]]

Attachment: Patch for availability check

Updated for current trunk. Carrying r+.

Comment 330

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 704990 [details] [diff] [review]
Patch

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

::: browser/base/content/browser.js
@@ +881,5 @@
> +  _swipeNavigatesHistory: function GS__swipeNavigatesHistory(aEvent) {
> +    return this._getCommand(aEvent, ["swipe", "left"])
> +              == "Browser:BackOrBackDuplicate" &&
> +           this._getCommand(aEvent, ["swipe", "right"])
> +              == "Browser:ForwardOrForwardDuplicate";

How will these handle users in RTL locales? If the app is in RTL mode, then swiping to the right should go back and swiping to the left should go forward.

Comment 331

[Markus Stange [:mstange]]

Comment on attachment 704990 [details] [diff] [review]
Patch

Hi Stephen,

I'm sorry for the delay here, I was on vacation and I'm slowly catching up with the backlog.

Thank you very much for driving this patch into a landable form. The ideas behind the changes are good but I have some problems with some of the implementation details.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+// History Swipe Animation Support (bug 678392)
>+let gHistorySwipeAnimation = {
>+
>+  active: false,
>+
>+  /**
>+   * Initializes the support for history swipe animations, if it is supported
>+   * by the platform/configuration.
>+   */
>+  init: function HSA_init() {
>+    if (!this._isSupported())
>+      return;
>+    
>+    gBrowser.addEventListener("pagehide", this, false);
>+    gBrowser.addEventListener("pageshow", this, false);
>+    gBrowser.addEventListener("popstate", this, false);
>+
>+    this.active = true;
>+    this._snapshotList = this._createDoublyLinkedList();

It's very uncommon to implement your own list data structures in JavaScript. Usually everybody uses normal JS arrays, except in cases where it really does make a noticeable difference in execution speed. I don't think that applies here. So I think you should just make this
this._snapshotList = [];
That also means:
 - remove "prev" and "next" properties from the list nodes
 - list.head becomes list[0], list.tail becomes list[list.length - 1]
 - list.size becomes list.length
 - list.remove(elem) becomes list.splice(list.indexOf(elem), 1)
 - inserting an element to the beginning of the list becomes list.unshift(elem)

Also, I'm not sure if the name "snapshotList" reflects the fact that these are all the snapshots we know to be in memory. Maybe rename it to _availableSnapshots? _trackedSnapshots? _snapshotsCurrentlyKeptInMemory?

>+  /**
>+   * Moves a given box to a given X coordinate position.
>+   *
>+   * @param aBox
>+   *        The box element to position.
>+   * @param aPosition
>+   *        The position (in X coordinates) to move the box element to.
>+   */
>+  _positionBox: function HSA__positionBox(aBox, aPosition) {
>+    let width = this._curBox.getBoundingClientRect().width;
>+    aBox.style.MozTransform = "translateX(" + width * aPosition + "px)";

CSS transforms have been unprefixed, so this can become aBox.style.transform.

>+  /**
>+   * Adds a snapshot to the list and initiates the compression of said snapshot.
>+   * Once the compression is completed, it will replace the uncompressed
>+   * snapshot in the list.
>+   *
>+   * @param aCanvas
>+   *        The snapshot to add to the list and compress.
>+   */
>+  _assignSnapshotToCurrentBrowser:
>+  function HSA__assignSnapshotToCurrentBrowser(aCanvas) {
>+    let browser = gBrowser.selectedBrowser;
>+    let currIndex = browser.webNavigation.sessionHistory.index;
>+
>+    if (!("snapshots" in browser))
>+      browser.snapshots = {};
>+    let snapshots = browser.snapshots;
>+
>+    if(snapshots[currIndex])
>+      this._snapshotList.removeNode(snapshots[currIndex].nodeRef);

I'd get rid of the nodeRef property. We can always find the right node in the list because we know that node.tab == browser and node.index = currIndex.

>+
>+    let newNode = this._addSnapshotNodeToList(currIndex, browser);
>+    snapshots[currIndex] = this._createSnapshotElement(newNode);

Getting rid of nodeRef would also allow you to remove this and just assign the canvas / blob directly to snapshots[currIndex].

>+    // Temporarily store the canvas as the compressed snapshot.
>+    // This avoids a blank page if the user swipes quickly
>+    // between pages before the compression could complete.
>+    snapshots[currIndex].snapshot = aCanvas;
>+    
>+    // Kick off snapshot compression.
>+    aCanvas.toBlob(function(aBlob) {
>+        if(snapshots[currIndex].snapshot)
>+          delete snapshots[currIndex].snapshot;
>+        snapshots[currIndex].snapshot = aBlob;
>+      }, "image/png"
>+    );

Okay, so a snapshot can be both a canvas and a blob...

>+  _installCurrentPageSnapshot:
>+  function HSA__installCurrentPageSnapshot(aCanvas) {
>+    let currSnapshot = aCanvas;
>+    if (!currSnapshot) {
>+      let snapshots = gBrowser.selectedBrowser.snapshots || {};
>+      let currIndex = this._historyIndex;
>+      if (currIndex in snapshots)
>+        currSnapshot = this._convertBlobToImg(snapshots[currIndex].snapshot);

... but here you only handle the case that snapshot is a blob. I know that it's unlikely that we'll arrive here before the canvas encoding has completed, but we should still handle that case. Maybe just rename _convertBlobToImg to _convertToImg and have it return directly if it notices that it has been passed a canvas instead of a blob. (snapshot instanceof HTMLCanvasElement)

>+  _createSnapshotElement: function HSA__createSnapshotElement(aNode) {
>+    let element = new function() {
>+      this.snapshot = null;
>+      this.nodeRef = aNode; // Reference to node in linked list.
>+    };
>+    return element;

The (almost) completely equivalent and preferable version of this would be:
return {
  snapshot: null,
  nodeRef: aNode
};

>+        let snapshots = aNode.tab.snapshots;
>+        if (snapshots[aNode.index]) {
>+          if (snapshots[aNode.index].snapshot)
>+            delete snapshots[aNode.index].snapshot;
>+          delete snapshots[aNode.index].nodeRef;

A quick note on delete: "delete obj.prop;" does NOT mean "destroy the object that obj.prop references". It means "remove the property named 'prop' from the object 'obj'.". JS doesn't have a way of saying "destroy this object"; the only thing that can destroy objects is the GC, when it notices that an object is not referenced anymore. Of course, when you want object D to be destroyed, and the last reference to D is in A.prop, then deleting prop from A will result in the destruction of D during the next GC.

In the code I quoted above, delete snapshots[aNode.index] would make sense. But you're not going to need it if you use array.splice to remove the element.

>+        if(prev)

House style is to have a space between "if" and the opening paren. This happens in several places.

>+  /**
>+   * Creates a node that can be used with the doubly linked list
>+   * for snapshot tracking.
>+   *
>+   * @param aIndex
>+   *        The index in history where a snapshot is stored.
>+   * @param aTab
>+   *        The tab in which the snapshot is stored in.
>+   * @return the newly created node object.
>+   */
>+  _createListNode: function HSA__createListNode(aIndex, aTab) {
>+    let node = new (function() {
>+      this.index = aIndex;
>+      this.tab = aTab;
>+      this.next = null;
>+      this.prev = null;
>+    });
>+    return node;
>+  }

This should become
return {
  tab: aTab,
  index: aIndex
};
... though you could probably pull that up into the calling function.

One problem that hasn't been addressed in this patch is to release snapshots when their tab is closed. At the moment, snapshots are only discarded when a new snapshot is added which would exceed the limit, or when the whole window is closed (which destroys gHistorySwipeAnimation and _snapshotList and so on). The snapshots of closed tabs are kept alive by the snapshot list, for example like this: gHistorySwipeAnimation._snapshotList.head.next.tab.snapshots[0].snapshot.
The more alarming implication is that the browser elements of closed tabs are kept alive, too, but thanks to the hueyfix the browsers' content documents are not.
So you'll need to listen for an event that fires when a tab closes, and remove the references to that tab from the list of open snapshots. Then, when the tab/browser isn't referenced any more, the snapshots it carries aren't, either, and are destroyed together with the tab.

(Before the introduction of the snapshot list, we didn't have that problem, because nothing kept closed browsers alive, so their snapshots were destroyed automatically. That was the main factor that made me reluctant to add a per-window list of snapshots... but it has to be done.)

Further ideas (probably for a follow-up bug):
 - In the CSS rules that apply the snapshots to the XUL boxes, add background-size:contain so that the snapshots are scaled to fit. Might look bad if the window is resized between taking the snapshot and shouwing it.
 - In HiDPI mode (on retina screens), double the canvas size and push a scale transform on the canvas before drawing. Then the snapshots should be high-resolution, too, and are scaled to the right size by the background-size rule.

Comment 332

[Markus Stange [:mstange]]

Comment on attachment 705497 [details] [diff] [review]
Patch for availability check

Sorry to come back to this after you've already got r+ and everything, but I really don't think the introduction of nsSwipeService will be of much use. I think using a system metric would be much simpler, so that you can do window.matchMedia("(-moz-swipe-animation-enabled)").matches.

You can use this patch as a guide:
http://hg.mozilla.org/integration/mozilla-inbound/rev/805cd41a9b9f

Comment 333

[Josiah Bruner [:jsbruner]]

First of all, welcome back Markus! Hope you have a good trip.

Now, unfortunately, I found a pretty significant bug that needs to be addressed. However, I am not exactly sure what triggered it, as I haven't noticed it before. I do know I recently started using tab groups, which may contribute to it, but it is unlikely.

Anyway, at times, if you have two tabs open, and one was recently loaded and can go forward, then you move to the other tab that can't go forward, on that second tab you can still swipe forward, and the entire page moves over. I created a video demonstrating this. 

Note: I before also had this happen in the opposite direction, but could not reproduce it fast enough to keep this video short in time.


https://www.dropbox.com/s/i72715cgj7m65gc/FirefoxSwipeError.mov

Comment 334

[Stephen A Pohl [:spohl]]

Comment on attachment 704990 [details] [diff] [review]
Patch

Removing r? for now.

Comment 335

[Stephen A Pohl [:spohl]]

Attachment: Wip for RTL support

This adds RTL support. Unless I'm told not to, I'll be folding a cleaned-up version of this patch into the general patch when addressing the feedback in comment 331 and 332.

Comment 336

[Stephen A Pohl [:spohl]]

Attachment: Patch

- Addressed all implementation feedback (top portion of comment 331).
- Implemented the release of snapshots for tabs when they're closed by listening for "TabClose" events on gBrowser.tabContainer (bottom of comment 331).
- Addressed bug identified in comment 333.
- Cleaned up "RTL support" patch and folded into this patch.
- Folded "fix for animation getting stuck (intermittent)" patch into this patch (smichaud:r+)

Carrying over the r+ from smichaud for platform code, since no changes were made in platform code.

Asking Felipe for a review of the front-end pieces.

Will be updating the "availability check" patch (addressing comment 332) and tests next.

Comment 337

[Stephen A Pohl [:spohl]]

Attachment: Patch for availability check

Addressed feedback in comment 332.

Comment 338

[Stephen A Pohl [:spohl]]

Attachment: Tests

Updated tests for latest patch and sending for review.

Comment 339

[Stephen A Pohl [:spohl]]

Attachment: Patch

Fixed oversight.

Comment 340

[Stephen A Pohl [:spohl]]

Attachment: Patch

Restoring use of #ifdef __LP64__. The OSX 10.6 SDK has trouble without it.

Comment 341

[Stephen A Pohl [:spohl]]

Try builds with all the latest patches available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2952261d00f8/

Comment 342

[Stephen A Pohl [:spohl]]

Attachment: Patch for availability check

Updated for current trunk.

Comment 343

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated for current trunk.

Comment 344

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange from comment #331)
> Further ideas (probably for a follow-up bug):
>  - In the CSS rules that apply the snapshots to the XUL boxes, add
> background-size:contain so that the snapshots are scaled to fit. Might look
> bad if the window is resized between taking the snapshot and shouwing it.

Could you explain what would be improved by doing this? We already maintain the browser zoom level for the snapshots. Personally, I think scaling would be undesirable.

>  - In HiDPI mode (on retina screens), double the canvas size and push a
> scale transform on the canvas before drawing. Then the snapshots should be
> high-resolution, too, and are scaled to the right size by the
> background-size rule.

Filed bug 836430.

Comment 345

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 707178 [details] [diff] [review]
Patch for availability check

Looks fine to me.

Comment 346

[Stephen A Pohl [:spohl]]

Attachment: Patch

The last patch incorrectly referenced the new linen-pattern.png from jar.mn in winstripe instead of pinstripe. Fixed this by moving it back to jar.mn under pinstripe.

Comment 347

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated for current trunk.

Comment 348

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated for current trunk.

Comment 350

[Stephen A Pohl [:spohl]]

Attachment: Tests

Fixed test failures that went undetected before. Try run looks green with this updated patch now:
https://tbpl.mozilla.org/?tree=Try&rev=6debdef2e066

Comment 351

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 709812 [details] [diff] [review]
Patch

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

This looks very very good! Just a few things as usual with a patch of this size:

You're using the terms "tab" and "browser" interchangeably, but they actually have two different meanings in our front-end code, which made the code a bit confusing to understand. Tab is used for the <tab> element in the tabstrip (which is the target of some of your events, like TabClose), and browser is the <browser> element that holds the content of each page.
An example: you get the tab element in the TabClose event and get its browser through getBrowserForTab, and pass the browser as a parameter to _removeTrackedSnapshot, but that function parameter is named "aTab".
Another case is that the objects stored in your trackedSnapshots ("{index, tab}"), the field name is tab but you're holding a ref to a browser. So please make that naming consistent.


Also, we are trending towards splitting up these big modules out from browser.js and #include them (so technically there's no difference in code behavior, just that browser.js is kept smaller and easier to use).  See here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#147
It'd be great if you could create a new file to host your gHistorySwipeAnimation object and include it. (the other pieces can still go in browser.js).  Bonus points if you also move gGestureSupport to this new file in a separate commit!  (the file could be called browser-gestureSupport.js perhaps).

With this we can also do some trickery to not ship this swipe animation code to Windows/Linux users that will not benefit from it. That can be left for a follow-up if you want.

::: browser/base/content/browser.js
@@ +1276,5 @@
> +   */
> +  startAnimation: function HSA_startAnimation() {
> +    if (this.isAnimationRunning()) {
> +      gBrowser.stop();
> +      this._lastSwipeDir = "RELOAD";

I don't see where RELOAD is used anywhere else.. or is it just to ensure != "" ?

@@ +1287,5 @@
> +      this._canGoBack = this.canGoBack();
> +      this._canGoForward = this.canGoForward();
> +      this._takeSnapshot();
> +      this._installPrevAndNextSnapshots();
> +      this._animatedBrowser = gBrowser.selectedBrowser;

_animatedBrowser is unused

@@ +1373,5 @@
> +    }
> +    else if (aEvent.type == "pagehide") {
> +      // Take a snapshot of a page whenever it's about to be navigated away
> +      // from.
> +      this._takeSnapshot();

you may get a pagehide event for another tab which is not the current one being displayed. So you'll either need to filter this event out to not take a snapshot, or allow the _takeSnapshot (and subsequent functions) to take the tab to operate as a parameter.

Probably the same consideration applies above to the pageshow and popstate case.

@@ +1413,5 @@
> +  canGoBack: function HSA_canGoBack() {
> +    let index = gBrowser.webNavigation.sessionHistory.index;
> +    if (this.isAnimationRunning())
> +      index = this._historyIndex;
> +    return this._doesIndexExistInHistory(index - 1);

IIRC, getEntryAtIndex is considerably slower than sessionHistory.canGoBack().. So for the case where the animation is not running I suppose you can simply call canGoBack/canGoForward

@@ +1475,5 @@
> +   * return true if supported, false otherwise.
> +   */
> +  _isSupported: function HSA__isSupported() {
> +    // Only activate on Lion.
> +    // TODO: Only if [NSEvent isSwipeTrackingFromScrollEventsEnabled]

please have a bug on file to have its bug # on the comment

@@ +1532,5 @@
> +   * @return a new box element with a given identifier.
> +   */
> +  _createBox: function HSA__createBox(aID) {
> +    return this._createElement(aID, "box");
> +  },

this function is too specific, i'd prefer if you just called _createElement with box as a parameter.

@@ +1559,5 @@
> +   * @param aPosition
> +   *        The position (in X coordinates) to move the box element to.
> +   */
> +  _positionBox: function HSA__positionBox(aBox, aPosition) {
> +    let width = this._curBox.getBoundingClientRect().width;

It's safe to assume that during an animation the width of the browser window (and thus the window of the box) won't change. getBoundingClientRect() is quite expensive as it triggers a reflow. So it'd be good if you cached the width at the beginning of the animation and retrieve the cached version in subsequent calls to this function.. and then you can remove the cached val at the end of the animation (or actually you don't need to since you remove the boxes)

@@ +1595,5 @@
> +   * Retrieves the maximum number of snapshots that should be kept in memory.
> +   * This limit is a global limit and is valid across all open tabs.
> +   */
> +  _maxSnapshots: function HSA__maxSnapshots() {
> +    return gPrefService.getIntPref("browser.snapshots.limit");

you should cache this value after first retrieval. I don't think it's important to live update it so there are is no need to listen for pref changes

@@ +1753,5 @@
> +
> +  /**
> +   * Removes the snapshots for the current, previous and next page.
> +   */
> +  _uninstallSnapshots: function HSA__uninstallSnapshots() {

is this necessary, given that you remove the boxes when the animation finishes?

Comment 352

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed feedback.

(In reply to :Felipe Gomes from comment #351)
> Review of attachment 709812 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +1276,5 @@
> > +   */
> > +  startAnimation: function HSA_startAnimation() {
> > +    if (this.isAnimationRunning()) {
> > +      gBrowser.stop();
> > +      this._lastSwipeDir = "RELOAD";
> 
> I don't see where RELOAD is used anywhere else.. or is it just to ensure !=
> "" ?

Right, it's just to ensure != "". Added a comment to clarify.

@@ +1413,5 @@
> > +  canGoBack: function HSA_canGoBack() {
> > +    let index = gBrowser.webNavigation.sessionHistory.index;
> > +    if (this.isAnimationRunning())
> > +      index = this._historyIndex;
> > +    return this._doesIndexExistInHistory(index - 1);
> 
> IIRC, getEntryAtIndex is considerably slower than sessionHistory.canGoBack().. So for the 
> case where the animation is not running I suppose you can simply call
> canGoBack/canGoForward

I'm assuming you meant webNavigation.canGoBack/canGoForward. Fixed.
 
> @@ +1475,5 @@
> > +   * return true if supported, false otherwise.
> > +   */
> > +  _isSupported: function HSA__isSupported() {
> > +    // Only activate on Lion.
> > +    // TODO: Only if [NSEvent isSwipeTrackingFromScrollEventsEnabled]
> 
> please have a bug on file to have its bug # on the comment

This is addressed by patch "Patch for availability check" in this bug and has been r+'d by smichaud.

Comment 353

[Stephen A Pohl [:spohl]]

Attachment: Part 2: Patch for availability check

Updated to build with the latest patch. Carrying over r+.

Comment 354

[Stephen A Pohl [:spohl]]

Attachment: Move gGestureSupport to browser-gestureSupport.js

Comment 355

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 711456 [details] [diff] [review]
Patch

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

::: browser/base/content/browser-gestureSupport.js
@@ +140,5 @@
> +    if (aEvent.type == "TabClose") {
> +      let browser = gBrowser.getBrowserForTab(aEvent.target);
> +      this._removeTrackedSnapshot(-1, browser);
> +    }
> +    else if (this.isAnimationRunning() &&

did you mean to use || here?

Comment 356

[Stephen A Pohl [:spohl]]

Attachment: Patch

Reworked HSA_handleEvent based on discussion with felipe.

Comment 357

[Stephen A Pohl [:spohl]]

Attachment: Tests

Updated for latest patch.

Comment 358

[Stephen A Pohl [:spohl]]

Attachment: Part 4: Tests

Just added the html pages for the tests to the Makefile. Carrying over r+.

Comment 359

[Dão Gottwald [:dao]]

Comment on attachment 711566 [details] [diff] [review]
Patch

Use a WeakMap instead of setting browser.snapshots.

Comment 360

[Markus Stange [:mstange]]

What would be the advantage of that?

Comment 361

[Dão Gottwald [:dao]]

It keeps external data separate from data owned by the code backing browsers.

Comment 362

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Patch

Removed trailing white space. Carrying over r+ and f- flags.

Comment 363

[Stephen A Pohl [:spohl]]

Attachment: Part 3: Move gGestureSupport to browser-gestureSupport.js

Updated for latest patch. Carrying over r+.

Comment 364

[Stephen A Pohl [:spohl]]

Pushed to Mozilla-Inbound:

69c2332c6f77 Patch
d16647e05acd Move gGestureSupport to browser-gestureSupport.js
1f8810ef3a5a Patch for availability check
9a7e608e63e7 Tests

Comment 365

[Stephen A Pohl [:spohl]]

(In reply to Dão Gottwald [:dao] from comment #359)
> Comment on attachment 711566 [details] [diff] [review]
> Patch
> 
> Use a WeakMap instead of setting browser.snapshots.

I will be replacing browser.snapshots by a WeakMap in bug 839549.

Comment 366

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-inbound with links :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/69c2332c6f77
https://hg.mozilla.org/integration/mozilla-inbound/rev/d16647e05acd
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8810ef3a5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a7e608e63e7

Comment 367

[Robert Strong (they/them - no direct email)]

Backed out of mozilla-inbound due to test failures
https://hg.mozilla.org/integration/mozilla-inbound/rev/02430bf44958
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4d767109f2e

Comment 368

[Robert Strong (they/them - no direct email)]

Weird that tests pass on try and not on inbound
https://tbpl.mozilla.org/?tree=Try&rev=0fc8b7799aff
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9a7e608e63e7

Comment 369

[Phil Ringnalda (:philor)]

Weird, but sensible once you remember that dependencies in our build system are quite often best described as "vague hopes", that the tests continue to fail on just one build on your backout.

Add touching http://mxr.mozilla.org/mozilla-central/source/CLOBBER to the patch, set a clobber with https://secure.pub.build.mozilla.org/clobberer/?branch=mozilla-inbound before landing, and keep a close eye out for builds that manage to evade the clobberer anyway, and you can probably reland and make it stick.

Comment 370

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Patch

Updated for current trunk and added touching CLOBBER. Carrying over r+.

Comment 371

[Stephen A Pohl [:spohl]]

Attachment: Part 5: Performance modifications

These are the performance modifications that we discussed:
1. Lower the default number of snapshots from 20 to 5.
2. Ensure that setting the number of snapshots to 0 turns the feature off.

Comment 372

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 714004 [details] [diff] [review]
Part 5: Performance modifications

Looks good to me.

Comment 373

[Josiah Bruner [:jsbruner]]

Part 1 is broken on the current trunk.

Comment 374

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Patch

Updated for current trunk. Carrying over r+.

Comment 375

[Stephen A Pohl [:spohl]]

Attachment: Part 3: Move gGestureSupport to browser-gestureSupport.js

Updated for current trunk. Carrying over r+.

Comment 376

[Stephen A Pohl [:spohl]]

Attachment: Part 5: Performance modifications

Updated for current trunk. Carrying over r+.

Comment 377

[Josiah Bruner [:jsbruner]]

Attachment: Part 1: Patch

Just a quick update for the current trunk. Carrying the review+ flag.

Comment 378

[Josiah Bruner [:jsbruner]]

Attachment: Part 1: Patch

Quick update for the current trunk. Carrying review+ flag.

Comment 379

[Josiah Bruner [:jsbruner]]

Whoops. Sorry, Bugzilla went a little crazy on me. :)

Comment 380

[Stephen A Pohl [:spohl]]

Comment on attachment 718187 [details] [diff] [review]
Part 1: Patch

Please be careful when updating patches (or ask for help when in doubt), especially when carrying over an r+ flag. The updates to CLOBBER were incorrect in this patch.

Comment 381

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Patch

Updated for current trunk. Carrying over r+.

Comment 382

[Stephen A Pohl [:spohl]]

Attachment: Part 6: Fix assert failure

During cancelled swipes, i.e. the user swiped in one direction, then swiped back to stay on the original page, we used to fail the assertion in beginGestureWithEvent because the gesture state hadn't been cleared from the previous swipe. This patch clears the gesture state for this scenario.

Comment 383

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 720958 [details] [diff] [review]
Part 6: Fix assert failure

Looks fine to me.

Comment 384

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Patch

Updated for current trunk. Carrying over r+.

Comment 385

[Stephen A Pohl [:spohl]]

Attachment: Part 7: Clobber

This was included in the patch 'part 1' before. I've split this out to reduce the number of times we have to update part 1 for the current trunk. Carrying over r+.

Comment 386

[Jennifer Morrow [:Boriss] (UX)]

This will need a ui review - I'm happy to do it

Comment 387

[Robert Strong (they/them - no direct email)]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #386)
> This will need a ui review - I'm happy to do it
Thanks and yes please (I believe UX has looked at this previously... it was at least requested a couple of times verbally in the past). Would you like a try build?

spohl, if Boriss would like a try build can you provide one?

Comment 388

[Stephen A Pohl [:spohl]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #387) 
> spohl, if Boriss would like a try build can you provide one?

Sure:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0d386039a3e2/try-macosx64/firefox-22.0a1.en-US.mac.dmg

Comment 389

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Stephen Pohl [:spohl] from comment #388)
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 0d386039a3e2/try-macosx64/firefox-22.0a1.en-US.mac.dmg

Thank you for the test build!

I tried the test build. On my MacBook Air (Mid2012), the swipe animation was very jumpy...  Will we enable this feature by default when we land this patch? I feel it is early in the day from using the test build.

BTW, I think this feature is very good for UX. However, I seem it's better than the swipe animation that the hovering arrow indication like Chromium, because it will not overmuch for rendering processing.)

Comment 390

[Stefan Wrobel]

just downloaded the test build and i'm not seeing any animations on my rMBP. Is there something I need to do to enable them?

Comment 391

[Stephen A Pohl [:spohl]]

(In reply to swrobel from comment #390)
> just downloaded the test build and i'm not seeing any animations on my rMBP.
> Is there something I need to do to enable them?

Comment 130 may help you.

Comment 392

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Forward button not appearing correctly in Comment 388 try build

(In reply to Stephen Pohl [:spohl] from comment #388)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #387) 
> > spohl, if Boriss would like a try build can you provide one?
> 
> Sure:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 0d386039a3e2/try-macosx64/firefox-22.0a1.en-US.mac.dmg

I've been running this build, and I gotta say I'm seeing a lot of performance issues, particularly while using lots of memory.  Rather than being smooth, I'm getting jerky animations and delays.  If it's not possible to be smooth when a machine's running hot, could we detect when performance will be bad and disable the animation in those cases?

I'm also seeing artifact problems on the back button (see attachment).  Is that being addressed in another bug?

Comment 393

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockup: Forward button not appearing correctly in Comment 388 try build

Comment 394

[Josiah Bruner [:jsbruner]]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #392)
> I'm also seeing artifact problems on the back button (see attachment).  Is
> that being addressed in another bug?

I believe that is bug 800443.

Comment 395

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Patch

Updated for current trunk.

Comment 396

[Stephen A Pohl [:spohl]]

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #392)
> I've been running this build, and I gotta say I'm seeing a lot of
> performance issues, particularly while using lots of memory.  Rather than
> being smooth, I'm getting jerky animations and delays.  If it's not possible
> to be smooth when a machine's running hot, could we detect when performance
> will be bad and disable the animation in those cases?

Performance relies heavily on bug 817700, which is still in progress. Maybe the questions to ask here are:
1. Is the overall experience worse with this change than before?
2. Is it acceptable to work on performance issues in follow-up bugs?

Comment 397

[Stephen A Pohl [:spohl]]

Attachment: Part 3: Move gGestureSupport to browser-gestureSupport.js

Updated for current trunk.

Comment 398

[Stephen A Pohl [:spohl]]

It was suggested that I land these patches in disabled state, i.e. by setting the pref for the number of snapshots to 0 to avoid constant bitrotting of the patches.

Unless there are objections I will get the patches ready tomorrow. I would also modify the tests to turn on the feature on the platforms that support it.

We could then turn on the feature as soon as bug 817700 is fixed.

Comment 399

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Patch

Updated for current trunk. Carrying over flags.

Comment 400

[Stephen A Pohl [:spohl]]

Attachment: Part 3: Move gGestureSupport to browser-gestureSupport.js

Updated for current trunk. Carrying over r+.

Comment 401

[Stephen A Pohl [:spohl]]

Attachment: Part 5: Performance modifications

Removed some trailing white space. Carrying over r+.

Comment 402

[Stephen A Pohl [:spohl]]

Attachment: Part 1: Patch

Updated for current trunk and rev'd uuid for nsIDOMSimpleGestureEvent. Carrying over r+.

Comment 403

[Stephen A Pohl [:spohl]]

Attachment: Part 8: Disable feature

Temporarily disables the feature (until bug 817700 is fixed and landed).

Comment 404

[Stephen A Pohl [:spohl]]

Attachment: Part 8: Disable feature

Removed unnecessary else clause.

Comment 405

[Stephen A Pohl [:spohl]]

Attachment: Part 8: Disable feature

Gosh... now removed trailing white space. Sorry about the spam.

Comment 406

[Stephen A Pohl [:spohl]]

Attachment: Part 8: Disable feature

Added missing semicolon. Carrying over r+.

Comment 407

[Stephen A Pohl [:spohl]]

7d6cde6ea21d added test code for gesture image rotation, which is failing when the patches here are applied:
https://tbpl.mozilla.org/?tree=Try&rev=f4fcd13ea8fa

Working on resolving the failures now.

Comment 408

[Stephen A Pohl [:spohl]]

Attachment: Part 9: Fix rotation gestures

While updating the patch for the current trunk in comment 348 I missed the fact that the last argument to initCommandEvent in GS__doCommand had changed from null to aEvent on trunk. This caused rotation gestures to break, since aEvent becomes the sourceEvent of the new event, which is passed to gGestureSupport.rotate (see browser-sets.inc). This wasn't caught earlier since the tests for rotate gestures were only added recently. The tests would try to get the delta property on a null event, which stops execution.

Try run (with all patches applied) is in progress:
https://tbpl.mozilla.org/?tree=Try&rev=323578bb25ab

Comment 409

[Stephen A Pohl [:spohl]]

(In reply to Stephen Pohl [:spohl] from comment #408)
> Try run (with all patches applied) is in progress:
> https://tbpl.mozilla.org/?tree=Try&rev=323578bb25ab

All green.

Comment 410

[Stephen A Pohl [:spohl]]

Attachment: Part 8: Disable feature

Added commit message. Carrying over r+.

Comment 411

[Frank Yan (:fryn)]

One thing we need to fix either in this bug or in a followup is the occasional brief flash of page A before beginning to draw page B when a swipe from page A to page B completes. This happens, especially on slower connections, probably due to us not clearing the content area until the network connection to page B is completed, committing the navigation to page B.

Safari does not have this problem with its swipe animations.

Ideally, we would start loading page B into the content area immediately when the swipe is committed, but this would require quite an invasive reworking of docshell code I imagine.

Comment 412

[Josiah Bruner [:jsbruner]]

(In reply to Frank Yan (:fryn) from comment #411)
> One thing we need to fix either in this bug or in a followup is the
> occasional brief flash of page A before beginning to draw page B when a
> swipe from page A to page B completes. This happens, especially on slower
> connections, probably due to us not clearing the content area until the
> network connection to page B is completed, committing the navigation to page
> B.
> 
> Safari does not have this problem with its swipe animations.
> 
> Ideally, we would start loading page B into the content area immediately
> when the swipe is committed, but this would require quite an invasive
> reworking of docshell code I imagine.

If not, one thing Safari does that we are not doing is an animated flash if sites are taking a while to load. When swiping back, sometimes Safari can tell a site needs to be reloaded longer and will animate the entire view white for some time, until the site is somewhat responsive. This could be an alternative approach that doesn't look so ugly, but does tell the user that a page needs to be reloaded.

Comment 413

[beingalink]

I really really hope that these patches can finally land. I don't understand where the harm should be if it stays disabled at first. It is so tedious seeing how these patches get updated over and over again and then ... nothing. It would really be nice to have that stuff (together with the new scrollbars and other stuff) in firefox before the 3rd iteration of OS X, where these styles are the "new" default look, comes out. I could go on how Mac support seems to have much too low priority but that wouldn't lead anywhere anyway. Sorry for abusing this place with my rant.

Comment 414

[Stephen A Pohl [:spohl]]

Attachment: Part 7: Clobber

Updated for current trunk and added commit message. Carrying over r+.

Comment 415

[Stephen A Pohl [:spohl]]

Attachment: Part 8: Disable feature

Updated commit message. Carrying over r+.

Comment 416

[Steven Michaud [:smichaud] (Retired)]

Attachment: All parts (save clobber)

Stephen, here are all your patches but one combined into one large patch.  Please check that it doesn't contain any surprises.

If you have any corrections I'll make them to the combined patch.  Then I'll land it on mozilla-inbound.

I didn't include part 7 (clobber).  That part bitrots every few hours (every time someone updates the CLOBBER file).  So I'll need to update it by hand when I land the rest on mozilla-inbound.

That part's very simple, so it shouldn't be a problem.

Comment 417

[Stephen A Pohl [:spohl]]

Comment on attachment 735264 [details] [diff] [review]
All parts (save clobber)

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

I folded all the patches on my side and diff'd it with your combined patch. No differences were found. We are good to go.

Comment 418

[Steven Michaud [:smichaud] (Retired)]

Mozilla-inbound is currently closed (busted).  I'll wait until it reopens to land.

Comment 419

[Steven Michaud [:smichaud] (Retired)]

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4ccad71f40

Hurray!

Swipe animation is turned off by default.  To turn it on set browser.snapshots.limit to '5' (or another non-zero value).

Since animation is still off by default, this bug still isn't fixed, exactly.  But let's allow this bug to be marked FIXED when this patch lands on mozilla-central.

For further discussion of issues with this patch, please open new bugs.  Please also open new bugs for such discussion as has already happened here.

Comment 420

[Stephen A Pohl [:spohl]]

Please note that by enabling this feature (by setting the snapshot limit to something > 0) bug 770626 will partially regress until the patch for bug 673875 has landed. Although there will not be an infinite loop of onbeforeunload events as in bug 770626, pressing the "stay on page" button will fail to tear down the animation overlay.

Comment 421

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5e4ccad71f40

Comment 422

[beingalink]

Is this already in today's nightly? Setting browser.snapshots.limit to 5 doesn't seem to do anything.

Comment 423

[Scoobidiver (away)]

(In reply to beingalink from comment #422)
> Is this already in today's nightly?
No. You need to wait tomorrow.

Comment 424

[Scoobidiver (away)]

(In reply to Scoobidiver from comment #423)
> (In reply to beingalink from comment #422)
> > Is this already in today's nightly?
> No. You need to wait tomorrow.
There's a second today's build, 23.0a1/20130410065939, where it's included.

Comment 425

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

PRUint32, eh.

Comment 426

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/firefox/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.

Comment 427

[u279076]

Adding this to the sign-off criteria for Firefox 23.0b1.

Comment 428

[Stephen A Pohl [:spohl]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #427)
> Adding this to the sign-off criteria for Firefox 23.0b1.

Please note that this is currently disabled by default (see bug 860493).

Comment 429

[u279076]

(In reply to Stephen Pohl [:spohl] from comment #428)
> (In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #427)
> > Adding this to the sign-off criteria for Firefox 23.0b1.
> 
> Please note that this is currently disabled by default (see bug 860493).

Lukas, can we update the status/tracking/relnote flags to reflect this? I'm dropping this from my radar for Firefox 23. Please add qawanted if the situation changes and this needs testing.