Last Comment Bug 721208 - Tab screenshots can hose the gecko event loop
: Tab screenshots can hose the gecko event loop
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86_64 Linux
-- normal (vote)
: Firefox 12
Assigned To: Brad Lassey [:blassey] (use needinfo?)
: Sebastian Kaspari (:sebastian)
Depends on:
  Show dependency treegraph
Reported: 2012-01-25 14:17 PST by Brad Lassey [:blassey] (use needinfo?)
Modified: 2012-01-30 11:37 PST (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (2.97 KB, patch)
2012-01-25 14:17 PST, Brad Lassey [:blassey] (use needinfo?)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Brad Lassey [:blassey] (use needinfo?) 2012-01-25 14:17:51 PST
Created attachment 591604 [details] [diff] [review]
Comment 1 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-01-25 22:27:15 PST
Comment on attachment 591604 [details] [diff] [review]

I'm not a big fan of the actual impl, but I understand the reason we would need this. This pass is a "can I make myself like this better" review

>diff -r 5f6d25767d30 -r e6336e3e1e8c mobile/android/base/

>+        GeckoAppShell.sendEventToGecko(new GeckoEvent("Tab:CancelScreenshots",""));

nit: "Tab:Screenshot:Cancel"

>diff -r 5f6d25767d30 -r e6336e3e1e8c mobile/android/chrome/content/browser.js

>     Services.obs.addObserver(this, "Tab:Screenshot", false);
>+    Services.obs.addObserver(this, "Tab:ScreenshotNext", false);

I'd like to switch to use the thread manager and dispatching to continue the queue, so let's remove this, but you do need to add "Tab:Screenshot:Cancel"

>   screenshotTab: function screenshotTab(aData) {
>+      if (this.screenshotQueue == null) {
>+          this.screenShotQueue = new Array();

nit: use []
            this.screenShotQueue = [];

>+  doNextScreenshot: function() {
>+      if (this.screenshotQueue == null || this.screenshotQueue.length == 0) {
>+          this.screenshotQueue = null;
>+          return

nit: add ;

>+    } else if (aTopic == "Tab:ScreenshotNext") {
>+      this.doNextScreenshot();


>+    } else if (aTopic == "Tab:CancelScreenshots") {


>       sendMessageToJava(message);
>+      Services.obs.notifyObservers(this, "Tab:ScreenshotNext", "");

This is kinda convenient, but it's also a bit of an abuse. Can we use this code instead: {

r+, but I'll keep thinking of ways to make this impl more likable
Comment 2 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-01-25 22:27:58 PST
Like maybe making a simple JS class for the ScreenshotQueue instead of adding it to BrowserApp
Comment 3 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-25 23:59:49 PST
Comment 4 User image Ed Morley [:emorley] 2012-01-26 04:25:00 PST
Comment 5 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-26 20:42:49 PST
Comment on attachment 591604 [details] [diff] [review]

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
the gecko event loop can be bogged down by the screenshot creation even when is no longer needed
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
pretty safe, but if something were to go wrong, the we'd have blank thumbnails.
Comment 6 User image Alex Keybl [:akeybl] 2012-01-27 16:17:09 PST
Comment on attachment 591604 [details] [diff] [review]

[Triage Comment]
Mobile only - approved for Aurora.
Comment 7 User image Brad Lassey [:blassey] (use needinfo?) 2012-01-30 11:37:37 PST

Note You need to log in before you can comment on or make changes to this bug.