Last Comment Bug 659594 - Use MozAfterPaint to redraw thumbnails
: Use MozAfterPaint to redraw thumbnails
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 14
Assigned To: Raymond Lee [:raymondlee]
:
:
Mentors:
: 598435 (view as bug list)
Depends on:
Blocks: 633096 665906 691740 692706 692707
  Show dependency treegraph
 
Reported: 2011-05-25 03:09 PDT by :aceman
Modified: 2016-04-12 14:00 PDT (History)
11 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot (126.03 KB, image/png)
2011-05-25 03:09 PDT, :aceman
no flags Details
Actually when I do exactly those step, even the 2nd thumbnail can get garbled. (24.22 KB, image/png)
2011-05-26 05:19 PDT, :aceman
no flags Details
v1 (1.55 KB, patch)
2011-05-27 03:42 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v2 (7.28 KB, patch)
2011-05-27 12:35 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v3 (10.71 KB, patch)
2011-06-01 08:01 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback-
Details | Diff | Splinter Review
v4 (9.28 KB, patch)
2011-08-17 07:22 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v5 (7.03 KB, patch)
2011-08-18 03:44 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v6 (8.40 KB, patch)
2011-08-18 09:49 PDT, Raymond Lee [:raymondlee]
dao+bmo: feedback+
Details | Diff | Splinter Review
v7 (14.89 KB, patch)
2011-08-23 07:12 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v8 (16.09 KB, patch)
2011-08-30 01:06 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v9 (17.15 KB, text/plain)
2011-10-18 04:07 PDT, Raymond Lee [:raymondlee]
no flags Details
v10 (17.10 KB, text/plain)
2011-10-22 03:58 PDT, Raymond Lee [:raymondlee]
no flags Details
v11 (16.39 KB, patch)
2011-11-22 00:38 PST, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (16.35 KB, patch)
2011-11-29 05:20 PST, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v13 (16.43 KB, patch)
2011-12-08 11:02 PST, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v14 (21.04 KB, patch)
2012-04-12 07:50 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v15 (21.18 KB, patch)
2012-04-13 03:24 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v16 (20.06 KB, patch)
2012-04-17 11:12 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (19.42 KB, patch)
2012-04-18 10:41 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description :aceman 2011-05-25 03:09:24 PDT
Created attachment 535014 [details]
screenshot

Some error pages in panorama do not have thumbnails draws. It can be seen in the attached screenshot, one error page has proper thumbnail, the other one doesn't. Both of them are the 'offline mode' errors. The missing thumbnail does not appear after visiting its tab via normal browser window. It does appear only when clicking on it directly inside panorama. Then it is quickly rendered and the zoom effect goes to the page.
Comment 1 Raymond Lee [:raymondlee] 2011-05-26 02:16:30 PDT
Do you have exact steps to reproduce it?
Comment 2 :aceman 2011-05-26 03:10:21 PDT
1. open firefox
2. set offline mode
3. type URL (that is not in your cache, even nonexistent)
4. open new tab
5. type URL (that is not in your cache, even nonexistent)
6. enter panorama

Result:
2nd tab has thumbnail, 1st one does not.
Comment 3 :aceman 2011-05-26 05:19:07 PDT
Created attachment 535304 [details]
Actually when I do exactly those step, even the 2nd thumbnail can get garbled.
Comment 4 Tim Taubert [:ttaubert] 2011-05-27 02:24:06 PDT
bugspam
Comment 5 Tim Taubert [:ttaubert] 2011-05-27 02:29:04 PDT
bugspam
Comment 6 Raymond Lee [:raymondlee] 2011-05-27 03:42:09 PDT
Created attachment 535600 [details] [diff] [review]
v1
Comment 7 Tim Taubert [:ttaubert] 2011-05-27 04:44:25 PDT
Comment on attachment 535600 [details] [diff] [review]
v1

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

The patch looks good but we should add a test for this.

::: browser/base/content/tabview/tabitems.js
@@ +933,5 @@
> +
> +    let readyState =
> +      tab.linkedBrowser.contentDocument.readyState;
> +    let url =
> +      tab.linkedBrowser.contentDocument.URL;

Nit: Let's just add "let contentDocument = tab.linkedBrowser.contentDocument;" before these lines so they don't have to wrap.
Comment 8 Raymond Lee [:raymondlee] 2011-05-27 12:35:06 PDT
Created attachment 535714 [details] [diff] [review]
v2

I've modified the patch and added test.
Comment 9 Tim Taubert [:ttaubert] 2011-05-31 14:58:49 PDT
Comment on attachment 535714 [details] [diff] [review]
v2

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

::: browser/base/content/tabview/tabitems.js
@@ +1039,5 @@
> +        // fired so a delay is needed to ensure the page is fully loaded.
> +        if (this.isSpecialPageLoaded(tab)) {
> +           if (!tab._tabViewTabItem.resumeUpdateTime) {
> +             tab._tabViewTabItem.resumeUpdateTime =
> +               Date.now() + this._SPECIAL_PAGE_LOAD_INTERVAL;

Hmm... This doesn't really look like the right way to do it. We can't ensure that these pages are "loaded" after the given 500ms. Maybe we can use nsIWebProgressListener to keep track of the special page states?

::: browser/base/content/test/tabview/browser_tabview_bug659594.js
@@ +12,5 @@
> +    originalTab.linkedBrowser.loadURI("http://www.example.com");
> +    let testTab = win.gBrowser.addTab("http://www.example.com");
> +
> +    registerCleanupFunction(function () {
> +      BrowserOffline.toggleOfflineStatus();

Nit: We should add the condition "if (Services.io.offline)" here to not toggle when the first attempt failed (somehow).

@@ +23,5 @@
> +      let contentWindow = win.TabView.getContentWindow();
> +      let groupItem = contentWindow.GroupItems.groupItems[0];
> +
> +      groupItem.getChildren().forEach(function (tabItem) {
> +        tabItem.addSubscriber(tabItem, "updated", function () {

We should add those tabItems to the update queue manually by calling TabItems.update(tabItem) just to make sure they get updated (just in case they've already been).

@@ +34,5 @@
> +    }, win);
> +  });
> +}
> +
> +function newWindow(callback) {

You could just use newWindowWithTabView(onShow, onLoad) here.
Comment 10 Raymond Lee [:raymondlee] 2011-06-01 08:01:49 PDT
Created attachment 536620 [details] [diff] [review]
v3

(In reply to comment #9)
> ::: browser/base/content/tabview/tabitems.js
> @@ +1039,5 @@
> > +        // fired so a delay is needed to ensure the page is fully loaded.
> > +        if (this.isSpecialPageLoaded(tab)) {
> > +           if (!tab._tabViewTabItem.resumeUpdateTime) {
> > +             tab._tabViewTabItem.resumeUpdateTime =
> > +               Date.now() + this._SPECIAL_PAGE_LOAD_INTERVAL;
> 
> Hmm... This doesn't really look like the right way to do it. We can't ensure
> that these pages are "loaded" after the given 500ms. Maybe we can use
> nsIWebProgressListener to keep track of the special page states?
> 

I've added a progress listener. However, even the listener receives a STOP state, the special page isn't loaded completely. E.g. netError.html calls initPage() at the end of the page.  Therefore, a delay is still needed when the progress listener receives a STOP state.  I've tried different setTimeout second and one second works out the best for local special pages. 

See http://mxr.mozilla.org/mozilla-central/source/docshell/resources/content/netError.xhtml#394


> ::: browser/base/content/test/tabview/browser_tabview_bug659594.js
> @@ +12,5 @@
> > +    originalTab.linkedBrowser.loadURI("http://www.example.com");
> > +    let testTab = win.gBrowser.addTab("http://www.example.com");
> > +
> > +    registerCleanupFunction(function () {
> > +      BrowserOffline.toggleOfflineStatus();
> 
> Nit: We should add the condition "if (Services.io.offline)" here to not
> toggle when the first attempt failed (somehow).

Added

> 
> @@ +23,5 @@
> > +      let contentWindow = win.TabView.getContentWindow();
> > +      let groupItem = contentWindow.GroupItems.groupItems[0];
> > +
> > +      groupItem.getChildren().forEach(function (tabItem) {
> > +        tabItem.addSubscriber(tabItem, "updated", function () {
> 
> We should add those tabItems to the update queue manually by calling
> TabItems.update(tabItem) just to make sure they get updated (just in case
> they've already been).
> 

Added

> @@ +34,5 @@
> > +    }, win);
> > +  });
> > +}
> > +
> > +function newWindow(callback) {
> 
> You could just use newWindowWithTabView(onShow, onLoad) here.

Fixed
Comment 11 Raymond Lee [:raymondlee] 2011-06-28 02:36:02 PDT
@tim: please also feedback on this.
Comment 12 Tim Taubert [:ttaubert] 2011-06-29 04:19:17 PDT
Comment on attachment 536620 [details] [diff] [review]
v3

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

So I think this isn't the right approach to do this. We can't just pick an arbitrary interval and hope that the page is loaded after that. We should not work around the ideal solution if there's currently no way to do it. We should rather talk to other devs whether there is way to get notified that error pages have been rendered. If there isn't we need talk with them about how a solution could look like.

::: browser/base/content/tabview/tabitems.js
@@ +785,5 @@
>  // ##########
>  // Class: TabItems
>  // Singleton for managing <TabItem>s
>  let TabItems = {
> +  _SPECIAL_PAGE_LOAD_INTERVAL: 1000, // milliseconds

This interval works on your machine but may not be sufficient on slower machines or the test servers. One second is pretty long so that on faster machines we're waiting more time than needed.

::: browser/base/content/tabview/thumbnailStorage.js
@@ +120,5 @@
>    // Should be called when window is unloaded.
>    uninit: function ThumbnailStorage_uninit() {
>      gBrowser.removeTabsProgressListener(this);
>      Services.prefs.removeObserver(this.PREF_DISK_CACHE_SSL, this);
> +    gBrowser.removeTabsProgressListener(this);

That line is a duplicate.
Comment 13 Tim Taubert [:ttaubert] 2011-07-24 18:40:04 PDT
bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
Comment 14 :aceman 2011-08-09 13:42:18 PDT
Can bug 665906 have the same cause as this one? That one does not get an error page, but the load is user interrupted. The thumbnail still becomes missing.
Comment 15 Raymond Lee [:raymondlee] 2011-08-09 19:46:32 PDT
Ehsan: please see comment 12.  

At the end of the below pages (LOAD_BACKGROUND pages), initPage() is called to manipulate the DOM since onload handlers would not be executed.  Is there any way we can detect the webpage is completely rendered after initPage() is called?  Any suggestions?

http://mxr.mozilla.org/mozilla-central/source/docshell/resources/content/netError.xhtml
http://mxr.mozilla.org/mozilla-central/source/browser/components/certerror/content/aboutCertError.xhtml
Comment 16 :Ehsan Akhgari 2011-08-15 17:05:14 PDT
Does MozBeforePaint get dispatched for these pages?
Comment 17 Raymond Lee [:raymondlee] 2011-08-15 21:00:19 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #16)
> Does MozBeforePaint get dispatched for these pages?

Tested them and the MozBeforePaint event doesn't get dispatched. :(
Comment 18 Raymond Lee [:raymondlee] 2011-08-17 03:03:02 PDT
(In reply to Raymond Lee [:raymondlee] from comment #17)
> (In reply to Ehsan Akhgari [:ehsan] from comment #16)
> > Does MozBeforePaint get dispatched for these pages?
> 
> Tested them and the MozBeforePaint event doesn't get dispatched. :(

MozAfterPaint seems to work for us.
Comment 19 Raymond Lee [:raymondlee] 2011-08-17 07:22:33 PDT
Created attachment 553760 [details] [diff] [review]
v4
Comment 20 Dão Gottwald [:dao] 2011-08-17 08:04:08 PDT
Comment on attachment 553760 [details] [diff] [review]
v4

>+  // ----------
>+  // Implements progress listener interface.
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
>+                                         Ci.nsISupportsWeakReference,
>+                                         Ci.nsISupports]),

This is unnecessary junk, please remove it. You're only talking to tabbrowser here, without XPCOM being involved.

This whole patch looks like a gigantic hack. If you can't rely on the load event, why not use progress listeners and MozAfterPaint for all pages?
Comment 21 Tim Taubert [:ttaubert] 2011-08-17 08:15:15 PDT
Comment on attachment 553760 [details] [diff] [review]
v4

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

> This whole patch looks like a gigantic hack. If you can't rely on the load
> event, why not use progress listeners and MozAfterPaint for all pages?

Yeah, that was my first thought, too. TabItems.update(tab) is quite prepared to be called very often. So why not listen for MozAfterPaint on all tabs/browsers instead of listening for "attrModified"? I even think we can do that without using progress listeners. That should solve this bug and bug 665906, too. (Investigating the use of MozAfterPaint has been on my todo list for some time. It would be great if this turns out to be useful.)
Comment 22 Raymond Lee [:raymondlee] 2011-08-18 03:44:09 PDT
Created attachment 554027 [details] [diff] [review]
v5

> Yeah, that was my first thought, too. TabItems.update(tab) is quite prepared
> to be called very often. So why not listen for MozAfterPaint on all
> tabs/browsers instead of listening for "attrModified"? I even think we can
> do that without using progress listeners. That should solve this bug and bug
> 665906, too. (Investigating the use of MozAfterPaint has been on my todo
> list for some time. It would be great if this turns out to be useful.)

Added MozAfterPaint for all tabs.  However, we can't remove "attrModified".  If we remove "attrModified", we would actually introduce bug 674794 

This patch should also fixes Bug 665906.
Comment 23 Dão Gottwald [:dao] 2011-08-18 08:27:35 PDT
Comment on attachment 554027 [details] [diff] [review]
v5

>--- a/browser/base/content/tabview/modules/AllTabs.jsm
>+++ b/browser/base/content/tabview/modules/AllTabs.jsm
>@@ -112,23 +112,24 @@ __defineGetter__("browserWindows", funct
>   let browserWindows = [];
>   let windows = Services.wm.getEnumerator("navigator:browser");
>   while (windows.hasMoreElements())
>     browserWindows.push(windows.getNext());
>   return browserWindows;
> });
> 
> let events = {
>-  attrModified: "TabAttrModified",
>-  close:        "TabClose",
>-  move:         "TabMove",
>-  open:         "TabOpen",
>-  select:       "TabSelect",
>-  pinned:       "TabPinned",
>-  unpinned:     "TabUnpinned"
>+  attrModified:  "TabAttrModified",
>+  close:         "TabClose",
>+  move:          "TabMove",
>+  open:          "TabOpen",
>+  select:        "TabSelect",
>+  pinned:        "TabPinned",
>+  unpinned:      "TabUnpinned",
>+  mozAfterPaint: "MozAfterPaint"
> };
> let eventListeners = {};
> 
> function registerBrowserWindow(browserWindow) {
>   for each (let event in events)
>     browserWindow.addEventListener(event, tabEventListener, true);
> 
>   browserWindow.addEventListener("unload", unregisterBrowserWindow, false);

This doesn't belong in here, at least not this way. If you look further below, you'll see that tabEventListener expects a tab as the event target.
Comment 24 Raymond Lee [:raymondlee] 2011-08-18 09:49:32 PDT
Created attachment 554120 [details] [diff] [review]
v6

Dao: Is it ok to do it this way?
Comment 25 Dão Gottwald [:dao] 2011-08-19 01:15:54 PDT
Comment on attachment 554120 [details] [diff] [review]
v6

>-  isComplete: function TabItems_update(tab) {
>+  _isComplete: function TabItems__isComplete(tab) {
>     // If our readyState is complete, but we're showing about:blank,
>     // and we're not loading about:blank, it means we haven't really
>     // started loading. This can happen to the first few tabs in a
>     // page.
>     Utils.assertThrow(tab, "tab");
>+
>+    let contentDocument = tab.linkedBrowser.contentDocument;
>     return (
>-      tab.linkedBrowser.contentDocument.readyState == 'complete' &&
>-      !(tab.linkedBrowser.contentDocument.URL == 'about:blank' &&
>-        tab._tabViewTabItem.url != 'about:blank')
>+      ((contentDocument.readyState == 'complete' ||
>+       !tab.linkedBrowser.webProgress.isLoadingDocument) &&
>+       !(contentDocument.URL == 'about:blank' && 
>+       tab._tabViewTabItem.url != 'about:blank')) ||
>+      (contentDocument.readyState == "interactive" &&
>+        /^about:/.test(contentDocument.URL))
>     );

This is hard to read, partly because it's wrongly indented, partly because it's a generally messy expression. It would be nice to get rid of the about: special-casing here as well. Seemingly this method isn't e10s-ready either.

But as far as the MozAfterPaint handling is concerned, this patch seems to make more sense at a first glance.
Comment 26 Tim Taubert [:ttaubert] 2011-08-19 01:52:08 PDT
(In reply to Dão Gottwald [:dao] from comment #25)
> But as far as the MozAfterPaint handling is concerned, this patch seems to
> make more sense at a first glance.

Yeah, this is much better. Alas, this isn't e10s ready either, but with e10s we can have that a lot simpler:

Bug 626455 and bug 674926 (applied in that order) introduce a tabview/content.js file that is the content script for Panorama. If you add a "MozAfterPaint" event listener there and send an async "Panorama:MozAfterPaint" message to the chrome process then you just need to register a message listener once in UI.init().

See https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=626455&attachment=547931 for implementation details (look at ui.js + content.js).

Sorry that I didn't land those patches yet, but bug 626455 is giving me hard time figuring out that leak :|
Comment 27 Tim Taubert [:ttaubert] 2011-08-22 03:10:16 PDT
Comment on attachment 554120 [details] [diff] [review]
v6

Bug 626455 landed. Bug 674926 still needs to be applied (sorry for that again). Please ask me for review again when MozAfterPaint is handled in our content script.
Comment 28 Raymond Lee [:raymondlee] 2011-08-23 07:12:31 PDT
Created attachment 555096 [details] [diff] [review]
v7

> -const Cu = Components.utils;
> -
> -Cu.import("resource:///modules/tabview/utils.jsm");
> +Components.utils.import("resource:///modules/tabview/utils.jsm");

In the content.js, it throws redecoration Cu.  If I remove it, everything works fine but the mochitests complains that Cu is undefined.  However, I made the above change.
Comment 29 Tim Taubert [:ttaubert] 2011-08-29 07:34:12 PDT
Comment on attachment 555096 [details] [diff] [review]
v7

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

The approach looks good, though some questions and issues remain that need to be addressed.

::: browser/base/content/tabview/content.js
@@ +35,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
>  "use strict";
>  
> +Components.utils.import("resource:///modules/tabview/utils.jsm");

Yeah, that's because of bug 673569. Don't think we need to change that.

@@ +93,5 @@
>    // Function: isDocumentLoaded
>    // Checks if the currently active document is loaded.
>    isDocumentLoaded: function WMH_isDocumentLoaded(cx) {
> +    let isLoaded = (content.document.readyState == "complete"||
> +                    !webProgress.isLoadingDocument);

Why that change? According to https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/thumbnailStorage.js#106 this needs to be "&&", right?

::: browser/base/content/tabview/tabitems.js
@@ +817,5 @@
>    },
>  
>    // ----------
> +  // Function: _isComplete
> +  // Checks whether the xul:tab has fully loaded and makes a callback with a 

Better: "calls" a callback. Please describe what the "boolean" exactly is.

@@ +837,5 @@
> +        // page.
> +        let aboutBlankURI = gWindow.makeURI("about:blank");
> +        isComplete =
> +          !(gWindow.makeURI(cx.json.url).equalsExceptRef(aboutBlankURI) &&
> +            !gWindow.makeURI(tab._tabViewTabItem.url).equalsExceptRef(aboutBlankURI));

Hmm. IIRC the whole 'about:blank' checking here was introduced in bug 628701 to not update the tab canvas too early (because that would throw away the cached image and we have nothing good to show). Couldn't we just obsolete this with tracking if a MozAfterPaint event has occured in the tab, yet? Once we received MozAfterPaint there should be something visible so we can start drawing, I think (even when navigating to different pages afterwards).

@@ +939,5 @@
>        }
>  
>        // ___ Make sure the tab is complete and ready for updating.
> +      let self = this;
> +      this._isComplete(tab, function(isComplete) {

Please name that anonymous function.

@@ +940,5 @@
>  
>        // ___ Make sure the tab is complete and ready for updating.
> +      let self = this;
> +      this._isComplete(tab, function(isComplete) {
> +        if (!isComplete && (!options || !options.force)) {

If options.force=true we don't need to call ._isComplete() and wait for it to return.

::: browser/base/content/tabview/ui.js
@@ +246,5 @@
>        let mm = gWindow.messageManager;
>        let callback = this._onDOMWillOpenModalDialog.bind(this);
>        mm.addMessageListener("Panorama:DOMWillOpenModalDialog", callback);
> +      let paintedCallback = TabItems.onMozAfterPaint.bind(TabItems);
> +      mm.addMessageListener("Panorama:MozAfterPaint", paintedCallback);

If you have to bind(TabItems) then please add this to TabItems.(un)init.

::: browser/base/content/test/tabview/browser_tabview_bug597248.js
@@ +141,1 @@
>      contentWindow.TabItems._update(tabItem.tab);

We should call .update() here.

::: browser/base/content/test/tabview/browser_tabview_bug659594.js
@@ +26,5 @@
> +    ok(Services.io.offline, "It is now offline");
> +
> +    let originalTab = win.gBrowser.tabs[0];
> +    originalTab.linkedBrowser.loadURI("http://www.example.com/foo");
> +    let testTab = win.gBrowser.addTab("http://www.example.com/bar");

Nit: we don't need the variable "testTab" here.

::: browser/base/content/test/tabview/head.js
@@ +80,5 @@
> +        tabItem.removeSubscriber("updated", onUpdated);
> +        if (--counter == 0)
> +          callback();
> +      });
> +      tabItems._update(tab);

We should call .update() here.
Comment 30 Raymond Lee [:raymondlee] 2011-08-30 01:06:59 PDT
Created attachment 556772 [details] [diff] [review]
v8

(In reply to Tim Taubert [:ttaubert] from comment #29)
> Comment on attachment 555096 [details] [diff] [review]
> v7
> 
> Review of attachment 555096 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The approach looks good, though some questions and issues remain that need
> to be addressed.
> 
> ::: browser/base/content/tabview/content.js
> @@ +35,5 @@
> >   * ***** END LICENSE BLOCK ***** */
> >  
> >  "use strict";
> >  
> > +Components.utils.import("resource:///modules/tabview/utils.jsm");
> 
> Yeah, that's because of bug 673569. Don't think we need to change that.
> 

OK, undid the change.

> @@ +93,5 @@
> >    // Function: isDocumentLoaded
> >    // Checks if the currently active document is loaded.
> >    isDocumentLoaded: function WMH_isDocumentLoaded(cx) {
> > +    let isLoaded = (content.document.readyState == "complete"||
> > +                    !webProgress.isLoadingDocument);
> 
> Why that change? According to
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/
> thumbnailStorage.js#106 this needs to be "&&", right?
> 

This is actually for fixing bug 665906.  When loading a long page and user stops the page load, readyState remains "loading" and webProgress.isLoadingDocument is false.  However, I think there might be a better to fix that, I just leave it to that bug.  Therefore, I just added content.document.readyState == "interactive" to the validation for what we need here.

N.B. opening a blank tab in background.  The isLoaded would be false because the readyState would be "uninitialized"

> ::: browser/base/content/tabview/tabitems.js
> @@ +817,5 @@
> >    },
> >  
> >    // ----------
> > +  // Function: _isComplete
> > +  // Checks whether the xul:tab has fully loaded and makes a callback with a 
> 
> Better: "calls" a callback. Please describe what the "boolean" exactly is.
> 
Fixed

> @@ +837,5 @@
> > +        // page.
> > +        let aboutBlankURI = gWindow.makeURI("about:blank");
> > +        isComplete =
> > +          !(gWindow.makeURI(cx.json.url).equalsExceptRef(aboutBlankURI) &&
> > +            !gWindow.makeURI(tab._tabViewTabItem.url).equalsExceptRef(aboutBlankURI));
> 
> Hmm. IIRC the whole 'about:blank' checking here was introduced in bug 628701
> to not update the tab canvas too early (because that would throw away the
> cached image and we have nothing good to show). Couldn't we just obsolete
> this with tracking if a MozAfterPaint event has occured in the tab, yet?
> Once we received MozAfterPaint there should be something visible so we can
> start drawing, I think (even when navigating to different pages afterwards).

Tested and removed.

> 
> @@ +939,5 @@
> >        }
> >  
> >        // ___ Make sure the tab is complete and ready for updating.
> > +      let self = this;
> > +      this._isComplete(tab, function(isComplete) {
> 
> Please name that anonymous function.
Fixed

> 
> @@ +940,5 @@
> >  
> >        // ___ Make sure the tab is complete and ready for updating.
> > +      let self = this;
> > +      this._isComplete(tab, function(isComplete) {
> > +        if (!isComplete && (!options || !options.force)) {
> 
> If options.force=true we don't need to call ._isComplete() and wait for it
> to return.
> 
Fixed

> ::: browser/base/content/tabview/ui.js
> @@ +246,5 @@
> >        let mm = gWindow.messageManager;
> >        let callback = this._onDOMWillOpenModalDialog.bind(this);
> >        mm.addMessageListener("Panorama:DOMWillOpenModalDialog", callback);
> > +      let paintedCallback = TabItems.onMozAfterPaint.bind(TabItems);
> > +      mm.addMessageListener("Panorama:MozAfterPaint", paintedCallback);
> 
> If you have to bind(TabItems) then please add this to TabItems.(un)init.
> 
Done

> ::: browser/base/content/test/tabview/browser_tabview_bug597248.js
> @@ +141,1 @@
> >      contentWindow.TabItems._update(tabItem.tab);
> 
> We should call .update() here.
> 
Fixed

> ::: browser/base/content/test/tabview/browser_tabview_bug659594.js
> @@ +26,5 @@
> > +    ok(Services.io.offline, "It is now offline");
> > +
> > +    let originalTab = win.gBrowser.tabs[0];
> > +    originalTab.linkedBrowser.loadURI("http://www.example.com/foo");
> > +    let testTab = win.gBrowser.addTab("http://www.example.com/bar");
> 
> Nit: we don't need the variable "testTab" here.
>
Fixed
 
> ::: browser/base/content/test/tabview/head.js
> @@ +80,5 @@
> > +        tabItem.removeSubscriber("updated", onUpdated);
> > +        if (--counter == 0)
> > +          callback();
> > +      });
> > +      tabItems._update(tab);
> 
> We should call .update() here.
Fixed
Comment 31 Raymond Lee [:raymondlee] 2011-09-27 21:22:40 PDT
Tim: could you review this patch please? thanks!
Comment 32 Tim Taubert [:ttaubert] 2011-09-29 18:50:51 PDT
Comment on attachment 556772 [details] [diff] [review]
v8

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

::: browser/base/content/tabview/tabitems.js
@@ +828,5 @@
>  
>    // ----------
> +  // Function: _isComplete
> +  // Checks whether the xul:tab has fully loaded and calls a callback with a 
> +  // boolean indicates whether the tab is loaded or not.

Nit: "with a boolean indicating...".

@@ +838,5 @@
> +
> +    mm.addMessageListener(message, function onMessage(cx) {
> +      mm.removeMessageListener(cx.name, onMessage);
> +      callback(cx.json.isLoaded);
> +    });

Now that TabItems._isComplete() is asynchronous we need to fix TabItems._checkHeartbeat(). The latter assumes that TabItems._update() runs synchronously to measure how many items can be updated between heartbeats.

Do we even need TabItems._isComplete() anymore? Now that we listen for MozAfterPaint our thumbnails should always be accurate no matter if the page has finished loading, is loading or loading has been cancelled, right? I did a quick test on my machine - the tests pass and a manual test looks pretty good. Am I missing anything?

@@ +937,5 @@
>        }
>  
>        // ___ Make sure the tab is complete and ready for updating.
> +      let self = this;
> +      let updateCanvas = function TabItems__update_updateCanvas(tabItem) {

Let's make that "function updateCanvas(...".

::: browser/base/content/test/tabview/browser_tabview_bug594958.js
@@ +56,5 @@
>  
> +    let mm = tab.linkedBrowser.messageManager;
> +
> +    mm.addMessageListener("Panorama:DOMContentLoaded", function onLoad(cx) {
> +      mm.removeMessageListener(cx.name, onLoad);

Isn't it enough to wait for "updated" only here?

::: browser/base/content/test/tabview/browser_tabview_bug597248.js
@@ +142,5 @@
>    });
>  
>    // clean up and finish
>    restoredWin.close();
>    finish();

We're now waiting for the tabs to be updated asynchronously. So we should finish only when all tabItems have been updated.

::: browser/base/content/test/tabview/browser_tabview_expander.js
@@ +25,5 @@
>  
>    // procreate!
>    contentWindow.UI.setActive(group);
>    for (var i=0; i<7; i++) {
> +    win.gBrowser.loadOneTab('http://example.com#' + i, {inBackground: true});

Why that change?
Comment 33 Tim Taubert [:ttaubert] 2011-09-29 19:11:42 PDT
(In reply to Tim Taubert [:ttaubert] from comment #32)
> Do we even need TabItems._isComplete() anymore? Now that we listen for
> MozAfterPaint our thumbnails should always be accurate no matter if the page
> has finished loading, is loading or loading has been cancelled, right? I did
> a quick test on my machine - the tests pass and a manual test looks pretty
> good. Am I missing anything?

Ok, so the one thing where we still need it is for yet unrestored tabs. Without that isComplete() check the cached thumbnail gets replaced with a blank one. We're listening for MozAfterPaint which is solely related to any content that was rendered, so we could add another method named TabItems.updateThumbnail() that is only called from the MozAfterPaint event handler. We'd need a combined TabPriorityQueue that holds "update" and "updateThumbnail" commands that are processed or delayed combined with the heartbeat.

I think we should maybe file a separate bug to have TabItems.updateThumbnail() implemented and let it block this one. So we could approach that solution in a cleaner way.
Comment 34 Tim Taubert [:ttaubert] 2011-10-01 05:10:52 PDT
(In reply to Tim Taubert [:ttaubert] from comment #33)
> (In reply to Tim Taubert [:ttaubert] from comment #32)
> I think we should maybe file a separate bug to have
> TabItems.updateThumbnail() implemented and let it block this one. So we
> could approach that solution in a cleaner way.

I'm working on a patch for this and will file a bug soon. These two patches should then only land together.
Comment 35 Raymond Lee [:raymondlee] 2011-10-03 03:17:25 PDT
(In reply to Tim Taubert [:ttaubert] from comment #34)
> I'm working on a patch for this and will file a bug soon. These two patches
> should then only land together.

Great!  I will update this one when your patch is ready. Thanks!
Comment 36 Tim Taubert [:ttaubert] 2011-10-05 02:57:20 PDT
*** Bug 598435 has been marked as a duplicate of this bug. ***
Comment 37 Raymond Lee [:raymondlee] 2011-10-18 04:07:59 PDT
Created attachment 567706 [details]
v9

unrotted
Comment 38 Raymond Lee [:raymondlee] 2011-10-22 03:58:15 PDT
Created attachment 568861 [details]
v10

Updated the patch for the new directory of Panorama.
Comment 39 Raymond Lee [:raymondlee] 2011-11-22 00:38:53 PST
Created attachment 576097 [details] [diff] [review]
v11

Minor update

Passed Try
https://tbpl.mozilla.org/?tree=Try&rev=e71d91fd8445
Comment 40 Raymond Lee [:raymondlee] 2011-11-25 09:53:45 PST
Comment on attachment 576097 [details] [diff] [review]
v11

Now, the patch for bug 691740 is ready.  We need to get a review for this one as well in order to check in both patches
Comment 41 Tim Taubert [:ttaubert] 2011-11-28 06:32:35 PST
Comment on attachment 576097 [details] [diff] [review]
v11

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

Looks good!

r=me with the question below answered, thanks!

::: browser/components/tabview/content.js
@@ +99,5 @@
>    // Function: isDocumentLoaded
>    // Checks if the currently active document is loaded.
>    isDocumentLoaded: function WMH_isDocumentLoaded(cx) {
> +    let isLoaded = ((content.document.readyState == "complete" ||
> +                     content.document.readyState == "interactive") &&

Nit: please add a 'readyState' variable to save some property lookups.

@@ +100,5 @@
>    // Checks if the currently active document is loaded.
>    isDocumentLoaded: function WMH_isDocumentLoaded(cx) {
> +    let isLoaded = ((content.document.readyState == "complete" ||
> +                     content.document.readyState == "interactive") &&
> +                     !webProgress.isLoadingDocument);

So 'complete' is for normal pages and 'interactive' is for error pages, right? As you mentioned, there is bug 665906 which would not be fixed because pages remain in readyState 'loading'. Could we maybe alter this to be

isLoaded = (readyState != 'uninitalized' && !webProgress.isLoadingDocument)

So we'd cover interrupted page loads, normal pages and error pages that have finished loading. Blank tabs that aren't loaded or restored yet, are not considered 'loaded'. Is there anything I might oversee?
Comment 42 Raymond Lee [:raymondlee] 2011-11-29 05:20:33 PST
Created attachment 577561 [details] [diff] [review]
Patch for checkin

(In reply to Tim Taubert [:ttaubert] from comment #41)
> Comment on attachment 576097 [details] [diff] [review] [diff] [details] [review]
> v11
> 
> Review of attachment 576097 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Looks good!
> 
> r=me with the question below answered, thanks!
> 
> ::: browser/components/tabview/content.js
> @@ +99,5 @@
> >    // Function: isDocumentLoaded
> >    // Checks if the currently active document is loaded.
> >    isDocumentLoaded: function WMH_isDocumentLoaded(cx) {
> > +    let isLoaded = ((content.document.readyState == "complete" ||
> > +                     content.document.readyState == "interactive") &&
> 
> Nit: please add a 'readyState' variable to save some property lookups.

Done

> 
> @@ +100,5 @@
> >    // Checks if the currently active document is loaded.
> >    isDocumentLoaded: function WMH_isDocumentLoaded(cx) {
> > +    let isLoaded = ((content.document.readyState == "complete" ||
> > +                     content.document.readyState == "interactive") &&
> > +                     !webProgress.isLoadingDocument);
> 
> So 'complete' is for normal pages and 'interactive' is for error pages,
> right? As you mentioned, there is bug 665906 which would not be fixed
> because pages remain in readyState 'loading'. Could we maybe alter this to be
> 
> isLoaded = (readyState != 'uninitalized' && !webProgress.isLoadingDocument)
> 
> So we'd cover interrupted page loads, normal pages and error pages that have
> finished loading. Blank tabs that aren't loaded or restored yet, are not
> considered 'loaded'. Is there anything I might oversee?

Yes, you are right.  It works fine for this bug and also fixes bug 665906.


Passed Try!
https://tbpl.mozilla.org/?tree=Try&rev=f7f20c55e12a
Comment 43 Raymond Lee [:raymondlee] 2011-11-29 05:22:58 PST
Please also check in the patch for bug 691740 at the same time
Comment 44 Tim Taubert [:ttaubert] 2011-11-30 04:43:28 PST
https://hg.mozilla.org/integration/fx-team/rev/54346920e122
Comment 45 Tim Taubert [:ttaubert] 2011-11-30 12:35:37 PST
Backed out - https://hg.mozilla.org/integration/fx-team/rev/a47c5dab113b

browser_tabview_bug594958.js failed on at least Win dbg. Maybe also on Linux dbg. Unfortunately the log for this build somehow disappeared...
Comment 46 Tim Taubert [:ttaubert] 2011-11-30 22:28:15 PST
Logs are back! It's only on Win dbg...

https://tbpl.mozilla.org/php/getParsedLog.php?id=7659879&tree=Fx-Team
Comment 47 Raymond Lee [:raymondlee] 2011-12-08 11:02:16 PST
Created attachment 580107 [details] [diff] [review]
v13

Minor update
Comment 48 Raymond Lee [:raymondlee] 2012-04-12 07:50:00 PDT
Created attachment 614373 [details] [diff] [review]
v14

Updated the patch again and hope this passes try
Comment 49 Raymond Lee [:raymondlee] 2012-04-13 03:24:39 PDT
Created attachment 614718 [details] [diff] [review]
v15

Finally, this latest patch passed try.

https://tbpl.mozilla.org/?tree=Try&rev=1882ffb4442a
Comment 50 Tim Taubert [:ttaubert] 2012-04-16 06:20:43 PDT
Comment on attachment 614718 [details] [diff] [review]
v15

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

Thanks for picking this up again! This looks all very good, no big issues, mainly some cleanup.

::: browser/components/tabview/tabitems.js
@@ +1061,5 @@
>        tabItem.$container.attr("title", label + "\n" + tabUrl);
>  
>        // ___ Make sure the tab is complete and ready for updating.
> +      let self = this;
> +      let updateCanvas = function TabItems__update_updateCanvas(tabItem) {

Please make this a separate function |TabItem.updateCanvas()|.

@@ +1087,3 @@
>  
> +        // ___ notify subscribers that a full update has completed.
> +        tabItem._sendToSubscribers("updated");

Hint: We should keep this line in |TabItems._update()| as this shouldn't be in |TabItem.updateCanvas()|.

@@ +1090,5 @@
> +      };
> +      if (options && options.force)
> +        updateCanvas(tabItem);
> +      else
> +        this._isComplete(tab, function TabItems__update_isComplete(isComplete) {

We need to check that the tab to update is still valid and not pinned before continuing.

::: browser/components/tabview/test/browser_tabview_bug594958.js
@@ +56,5 @@
>  
> +    let mm = tab.linkedBrowser.messageManager;
> +
> +    mm.addMessageListener("Panorama:DOMContentLoaded", function onLoad(cx) {
> +      mm.removeMessageListener(cx.name, onLoad);

Please don't use 'Panorama:DOMContentLoaded'. The only client so far is the storage policy which we're going to remove soon (bug 745303). We should stick with the normal load event here.

::: browser/components/tabview/test/browser_tabview_bug610242.js
@@ +77,5 @@
>  
> +      if (browser.currentURI.spec == "about:blank" ||
> +          browser.contentDocument.readyState != "complete" ||
> +          browser.webProgress.isLoadingDocument)
> +        return;

Can't we just remove this check if we add 'iconUpdated' subscribers and after that call |afterAllTabItemsUpdated(function () {}, win)| just to force all tabItems to update themselves?

::: browser/components/tabview/test/browser_tabview_bug631662.js
@@ +16,5 @@
>    }
>  
>    let actionAddTab = function () {
>      storeTimestamp();
> +    gBrowser.addTab("about:robots");

What's this change exactly for?

::: browser/components/tabview/test/browser_tabview_bug685476.js
@@ +6,5 @@
>  
> +  newWindowWithTabView(function (newWin) {
> +    registerCleanupFunction(function () newWin.close());
> +
> +    let tab = newWin.gBrowser.addTab();

The only difference is that we're now opening a new window but I don't see why we need that? Do we?
Comment 51 Raymond Lee [:raymondlee] 2012-04-17 11:12:11 PDT
Created attachment 615797 [details] [diff] [review]
v16
Comment 52 Raymond Lee [:raymondlee] 2012-04-17 11:15:23 PDT
Comment on attachment 615797 [details] [diff] [review]
v16

(In reply to Tim Taubert [:ttaubert] from comment #50)
> Comment on attachment 614718 [details] [diff] [review]
> v15
> 
> Review of attachment 614718 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for picking this up again! This looks all very good, no big issues,
> mainly some cleanup.
> 
> ::: browser/components/tabview/tabitems.js
> @@ +1061,5 @@
> >        tabItem.$container.attr("title", label + "\n" + tabUrl);
> >  
> >        // ___ Make sure the tab is complete and ready for updating.
> > +      let self = this;
> > +      let updateCanvas = function TabItems__update_updateCanvas(tabItem) {
> 
> Please make this a separate function |TabItem.updateCanvas()|.

Done

> 
> @@ +1087,3 @@
> >  
> > +        // ___ notify subscribers that a full update has completed.
> > +        tabItem._sendToSubscribers("updated");
> 
> Hint: We should keep this line in |TabItems._update()| as this shouldn't be
> in |TabItem.updateCanvas()|.
> 

Done.

> @@ +1090,5 @@
> > +      };
> > +      if (options && options.force)
> > +        updateCanvas(tabItem);
> > +      else
> > +        this._isComplete(tab, function TabItems__update_isComplete(isComplete) {
> 
> We need to check that the tab to update is still valid and not pinned before
> continuing.
> 

Added check

> ::: browser/components/tabview/test/browser_tabview_bug594958.js
> @@ +56,5 @@
> >  
> > +    let mm = tab.linkedBrowser.messageManager;
> > +
> > +    mm.addMessageListener("Panorama:DOMContentLoaded", function onLoad(cx) {
> > +      mm.removeMessageListener(cx.name, onLoad);
> 
> Please don't use 'Panorama:DOMContentLoaded'. The only client so far is the
> storage policy which we're going to remove soon (bug 745303). We should
> stick with the normal load event here.

Done.

> 
> ::: browser/components/tabview/test/browser_tabview_bug610242.js
> @@ +77,5 @@
> >  
> > +      if (browser.currentURI.spec == "about:blank" ||
> > +          browser.contentDocument.readyState != "complete" ||
> > +          browser.webProgress.isLoadingDocument)
> > +        return;
> 
> Can't we just remove this check if we add 'iconUpdated' subscribers and
> after that call |afterAllTabItemsUpdated(function () {}, win)| just to force
> all tabItems to update themselves?

It seems to work fine.

> 
> ::: browser/components/tabview/test/browser_tabview_bug631662.js
> @@ +16,5 @@
> >    }
> >  
> >    let actionAddTab = function () {
> >      storeTimestamp();
> > +    gBrowser.addTab("about:robots");
> 
> What's this change exactly for?

Tested that again and reverted it.

> 
> ::: browser/components/tabview/test/browser_tabview_bug685476.js
> @@ +6,5 @@
> >  
> > +  newWindowWithTabView(function (newWin) {
> > +    registerCleanupFunction(function () newWin.close());
> > +
> > +    let tab = newWin.gBrowser.addTab();
> 
> The only difference is that we're now opening a new window but I don't see
> why we need that? Do we?

Tested that again and we don't need that.


Pushed to try and it looks good so far.
https://tbpl.mozilla.org/?tree=Try&rev=0bc553dc5178
Comment 53 Tim Taubert [:ttaubert] 2012-04-18 02:30:03 PDT
Comment on attachment 615797 [details] [diff] [review]
v16

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

Thanks looks good! Please don't mark the new patch as checkin-needed, we still need to fix bug 691740 to land them both at the same time.

r=me with all the nits fixed.

::: browser/components/tabview/tabitems.js
@@ +813,5 @@
> +      let w = $canvas.width();
> +      let h = $canvas.height();
> +      if (w != this.$canvas[0].width || h != this.$canvas[0].height) {
> +        this.$canvas[0].width = w;
> +        this.$canvas[0].height = h;

Nit: please use |$canvas| instead of |this.$canvas| here.

::: browser/components/tabview/test/browser_tabview_bug594958.js
@@ +59,5 @@
> +     this.removeEventListener("load", onLoad, true);
> +     stillToLoad--;
> +     if (!stillToLoad)
> +       executeSoon(callback);
> +   }

Not sure what this code is for? Seems unused to me.

@@ +76,2 @@
>  
> +/*

Please remove the code you commented out.

::: browser/components/tabview/test/browser_tabview_bug610242.js
@@ +76,3 @@
>  
> +      if (++iconUpdateCounter == len) {
> +        afterAllTabItemsUpdated(function() {

I meant it like this (sorry if I was unclear):

children.forEach(function(tabItem) {
 ...
});

afterAllTabItemsUpdated(function() {}, win);
Comment 54 Tim Taubert [:ttaubert] 2012-04-18 03:09:26 PDT
(In reply to Tim Taubert [:ttaubert] from comment #53)
> Thanks looks good! Please don't mark the new patch as checkin-needed, we
> still need to fix bug 691740 to land them both at the same time.

I was actually referring to comment #33 but as we now keep the _isComplete() check this patch landable. We'll first remove the check in bug 691740. So please do mark your new patch as checkin-needed :)
Comment 55 Raymond Lee [:raymondlee] 2012-04-18 10:41:30 PDT
Created attachment 616202 [details] [diff] [review]
Patch for checkin

(In reply to Tim Taubert [:ttaubert] from comment #53)
> Comment on attachment 615797 [details] [diff] [review]
> v16
> 
> Review of attachment 615797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks looks good! Please don't mark the new patch as checkin-needed, we
> still need to fix bug 691740 to land them both at the same time.
> 
> r=me with all the nits fixed.
> 
> ::: browser/components/tabview/tabitems.js
> @@ +813,5 @@
> > +      let w = $canvas.width();
> > +      let h = $canvas.height();
> > +      if (w != this.$canvas[0].width || h != this.$canvas[0].height) {
> > +        this.$canvas[0].width = w;
> > +        this.$canvas[0].height = h;
> 
> Nit: please use |$canvas| instead of |this.$canvas| here.

Done

> 
> ::: browser/components/tabview/test/browser_tabview_bug594958.js
> @@ +59,5 @@
> > +     this.removeEventListener("load", onLoad, true);
> > +     stillToLoad--;
> > +     if (!stillToLoad)
> > +       executeSoon(callback);
> > +   }
> 
> Not sure what this code is for? Seems unused to me.

Removed.

> 
> @@ +76,2 @@
> >  
> > +/*
> 
> Please remove the code you commented out.

Removed.

> 
> ::: browser/components/tabview/test/browser_tabview_bug610242.js
> @@ +76,3 @@
> >  
> > +      if (++iconUpdateCounter == len) {
> > +        afterAllTabItemsUpdated(function() {
> 
> I meant it like this (sorry if I was unclear):
> 
> children.forEach(function(tabItem) {
>  ...
> });
> 
> afterAllTabItemsUpdated(function() {}, win);

Updated.

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=67fd6188e152
Comment 56 Raymond Lee [:raymondlee] 2012-04-18 19:32:05 PDT
> > ::: browser/components/tabview/test/browser_tabview_bug610242.js
> children.forEach(function(tabItem) {
>  ...
> });
> 
> afterAllTabItemsUpdated(function() {}, win);

The try failed for window debug using the above solution.  We should switch back to use below.  Tim: what do you think?

> > @@ +77,5 @@
> > >  
> > > +      if (browser.currentURI.spec == "about:blank" ||
> > > +          browser.contentDocument.readyState != "complete" ||
> > > +          browser.webProgress.isLoadingDocument)
> > > +        return;
> >
Comment 57 Tim Taubert [:ttaubert] 2012-04-18 23:29:21 PDT
(In reply to Raymond Lee [:raymondlee] from comment #56)
> The try failed for window debug using the above solution.  We should switch
> back to use below. Tim: what do you think?

We still need to wait for all tabs to load before we put them in the update queue. I added this little fix and two try runs were green:

https://tbpl.mozilla.org/?tree=Try&rev=5f27e057a57e
https://tbpl.mozilla.org/?tree=Try&rev=4a939a93d8fd

Pushed to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/5d69cf5ce278

Thank you for picking this up again and finishing it!
Comment 58 Tim Taubert [:ttaubert] 2012-04-19 01:53:21 PDT
https://hg.mozilla.org/mozilla-central/rev/5d69cf5ce278

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