659594 - Use MozAfterPaint to redraw thumbnails

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[:aceman]

Attachment: 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]]

Do you have exact steps to reproduce it?

Comment 2

[:aceman]

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]

Attachment: Actually when I do exactly those step, even the 2nd thumbnail can get garbled.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: v2

I've modified the patch and added test.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: 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]]

@tim: please also feedback on this.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

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] (inactive)]

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]

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]]

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

[(no longer active)]

Does MozBeforePaint get dispatched for these pages?

Comment 17

[Raymond Lee [:raymondlee]]

(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]]

(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]]

Attachment: v4

Comment 20

[Dão Gottwald [:dao]]

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] (inactive)]

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]]

Attachment: 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]]

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]]

Attachment: v6

Dao: Is it ok to do it this way?

Comment 25

[Dão Gottwald [:dao]]

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] (inactive)]

(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] (inactive)]

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]]

Attachment: 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] (inactive)]

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]]

Attachment: 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]]

Tim: could you review this patch please? thanks!

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

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] (inactive)]

(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] (inactive)]

(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]]

(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 37

[Raymond Lee [:raymondlee]]

Attachment: v9

unrotted

Comment 38

[Raymond Lee [:raymondlee]]

Attachment: v10

Updated the patch for the new directory of Panorama.

Comment 39

[Raymond Lee [:raymondlee]]

Attachment: v11

Minor update

Passed Try
https://tbpl.mozilla.org/?tree=Try&rev=e71d91fd8445

Comment 40

[Raymond Lee [:raymondlee]]

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] (inactive)]

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]]

Attachment: 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]]

Please also check in the patch for bug 691740 at the same time

Comment 44

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/54346920e122

Comment 45

[Tim Taubert [:ttaubert] (inactive)]

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] (inactive)]

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]]

Attachment: v13

Minor update

Comment 48

[Raymond Lee [:raymondlee]]

Attachment: v14

Updated the patch again and hope this passes try

Comment 49

[Raymond Lee [:raymondlee]]

Attachment: v15

Finally, this latest patch passed try.

https://tbpl.mozilla.org/?tree=Try&rev=1882ffb4442a

Comment 50

[Tim Taubert [:ttaubert] (inactive)]

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]]

Attachment: v16

Comment 52

[Raymond Lee [:raymondlee]]

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] (inactive)]

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] (inactive)]

(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]]

Attachment: 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]]

> > ::: 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] (inactive)]

(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] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/5d69cf5ce278