580954 - Replace the heartbeat with something smarter

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Ed Lee :Mardak]

In tracking down BrowserTab usage in bug 580937, I noticed .url and .raw were constantly getting called and turns out to be the heartbeat. It is scheduled to run every 100 milliseconds.

This is probably contributing a good portion to the Ts impact.

I'm guessing this is used to make sure the mirrors are kept up-to-date?

Perhaps this could leverage the AllTabs.onChange I'm adding to the tabs module in bug 580952.

Comment 1

[Ian Gilman [:iangilman]]

Yes, the heartbeat was written to work around the fact that we weren't getting reliable information about when the tabs were changing. If we can get reliable events, we should definitely use them instead. 

That said, the other function that the heartbeat performs is to throttle thumbnail updating; if you've got a lot of tabs with dynamic content, you don't want them updating constantly or all at the same time. 

So I think the ideal system would be triggered by AllTabs.onChange, but would then be throttled as needed.

Comment 2

[(no longer active)]

Hmm, Firefox (on Windows only?) has some code to grab tab screenshots in canvas elements.  Maybe TabCandy can share the same code with the rest of Firefox?

Comment 3

[(no longer active)]

This is the code that I'm talking about:

<http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-tabPreviews.js>

Comment 4

[Rob Arnold [:robarnold]]

The AeroPeek code obtains full-size canvas representations and incrementally updates them. Relevant bits of the code start here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/wintaskbar/WindowsPreviewPerTab.jsm#197

Comment 5

[Ian Gilman [:iangilman]]

Cool! I wonder if either of these can be generalized for all platforms?

By the way, we've got another bug, bug 581038, for discussing alternatives to how we currently thumbnail. This bug is more about how/when we trigger the updates of the thumbnails as well as tab titles, favIcons, etc.

Comment 6

[Rob Arnold [:robarnold]]

(In reply to comment #5)
> Cool! I wonder if either of these can be generalized for all platforms?
> 
> By the way, we've got another bug, bug 581038, for discussing alternatives to
> how we currently thumbnail. This bug is more about how/when we trigger the
> updates of the thumbnails as well as tab titles, favIcons, etc.

The code I linked does all this in a way that is, I believe, cross platform. It doesn't handle zooming, but there's a patch posted in bug 520943 that does.

Comment 7

[Ian Gilman [:iangilman]]

Rob, thanks! 

Mardak, I'll take a whack at refactoring heartbeat once AllTabs has landed.

Comment 8

[Ian Gilman [:iangilman]]

The first part of this is in now: 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/b08bcefd2895

It still needs to be smarter, though: 

* We're hiding the cached thumbnails too soon
* We shouldn't update at all when the Tab Candy UI isn't visible (except for the current tab, to facilitate the zoom down)
* If we get a bunch of updates all at the same time, spread them out so we don't get a big stutter

Comment 9

[Ian Gilman [:iangilman]]

With this change: 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/d6d4d80a0dea

... we're now not updating when the UI isn't visible, and we're spreading things out when it is. Remaining: 

* Hiding cached thumbnails too soon
* An exception for the current tab

Comment 10

[Ian Gilman [:iangilman]]

With this change: 

http://hg.mozilla.org/users/edward.lee_engineering.uiuc.edu/tabcandy-central/rev/674fb15553b8

... we're now updating the thumbnail for the current tab even if the UI isn't visible (so the zoom down works). Remaining: 

* Hiding cached thumbnails too soon

Also, we might want to do some tests to see whether triggering repainting the thumbnail on every onChange is too much.

Comment 11

[(no longer active)]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

"Something smarter" may include using -moz-element... just a thought.

Comment 13

[Hans Schmucker]

Maybe I'm overlooking something, but wouldn't listening for load and MozAfterPaint events help? Let both events set a dirty state. When the heartbeat-controlled drawing part comes across a thumbnail that's dirty it will redraw, otherwise the old one is kept.

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #13)
> Maybe I'm overlooking something, but wouldn't listening for load and
> MozAfterPaint events help? Let both events set a dirty state. When the
> heartbeat-controlled drawing part comes across a thumbnail that's dirty it will
> redraw, otherwise the old one is kept.

That was actually what we had earlier, but we moved away from it, I believe for performance reasons. I'll let Ian chime in.

Comment 15

[Ian Gilman [:iangilman]]

(In reply to comment #13)
> Maybe I'm overlooking something, but wouldn't listening for load and
> MozAfterPaint events help? Let both events set a dirty state. When the
> heartbeat-controlled drawing part comes across a thumbnail that's dirty it will
> redraw, otherwise the old one is kept.

Early on we had reliability issues with MozAfterPaint, but it's probably worth revisiting it (along with load) for use instead of TabAttrModified. I've now filed bug 598435 to look into this. 

I'm going to close this bug, as all of the remaining items now have their own bugs: 

Bug 597258 - Explore using -moz-element instead of the periodic canvas painting for Panorama thumbnails 

Bug 581038 - Should we be using an existing thumbnailing system from elsewhere in Firefox? 

Bug 597248 - Make sure Panorama's thumbnail cache is solid