Use MozAfterPaint to redraw thumbnails

RESOLVED FIXED in Firefox 14

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: aceman, Assigned: raymondlee)

Tracking

Trunk
Firefox 14
Dependency tree / graph

Details

Attachments

(3 attachments, 16 obsolete attachments)

126.03 KB, image/png
Details
24.22 KB, image/png
Details
19.42 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
Blocks: 653099
(Assignee)

Comment 1

6 years ago
Do you have exact steps to reproduce it?
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Comment 3

6 years ago
Created attachment 535304 [details]
Actually when I do exactly those step, even the 2nd thumbnail can get garbled.
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
(Assignee)

Comment 6

6 years ago
Created attachment 535600 [details] [diff] [review]
v1
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #535600 - Flags: feedback?(tim.taubert)
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.
OS: Windows XP → All
Hardware: x86 → All
Version: 4.0 Branch → Trunk
(Assignee)

Comment 8

6 years ago
Created attachment 535714 [details] [diff] [review]
v2

I've modified the patch and added test.
Attachment #535600 - Attachment is obsolete: true
Attachment #535600 - Flags: feedback?(tim.taubert)
(Assignee)

Updated

6 years ago
Attachment #535714 - Flags: feedback?(tim.taubert)
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.
Attachment #535714 - Flags: feedback?(tim.taubert)
(Assignee)

Comment 10

6 years ago
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
Attachment #535714 - Attachment is obsolete: true
Attachment #536620 - Flags: feedback?
(Assignee)

Updated

6 years ago
Attachment #536620 - Flags: feedback? → feedback?(tim.taubert)
(Assignee)

Comment 11

6 years ago
@tim: please also feedback on this.
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.
Attachment #536620 - Flags: feedback?(tim.taubert) → feedback-
bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
No longer blocks: 660175
(Reporter)

Comment 14

6 years ago
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.
(Assignee)

Comment 15

6 years ago
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
Does MozBeforePaint get dispatched for these pages?
(Assignee)

Comment 17

6 years ago
(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. :(
(Assignee)

Comment 18

6 years ago
(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.
(Assignee)

Comment 19

6 years ago
Created attachment 553760 [details] [diff] [review]
v4
Attachment #536620 - Attachment is obsolete: true
Attachment #553760 - Flags: review?(tim.taubert)
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 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.)
Attachment #553760 - Flags: review?(tim.taubert)
(Assignee)

Comment 22

6 years ago
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.
Attachment #554027 - Flags: review?(tim.taubert)
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.
(Assignee)

Comment 24

6 years ago
Created attachment 554120 [details] [diff] [review]
v6

Dao: Is it ok to do it this way?
Attachment #553760 - Attachment is obsolete: true
Attachment #554027 - Attachment is obsolete: true
Attachment #554027 - Flags: review?(tim.taubert)
Attachment #554120 - Flags: feedback?(dao)
(Assignee)

Updated

6 years ago
Attachment #554120 - Flags: review?(tim.taubert)
(Assignee)

Updated

6 years ago
Blocks: 598435
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.
Attachment #554120 - Flags: feedback?(dao) → feedback+
(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 :|
(Reporter)

Updated

6 years ago
Blocks: 665906
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.
Attachment #554120 - Flags: review?(tim.taubert)
(Assignee)

Comment 28

6 years ago
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.
Attachment #554120 - Attachment is obsolete: true
Attachment #555096 - Flags: review?(tim.taubert)
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.
Attachment #555096 - Flags: review?(tim.taubert)
(Assignee)

Updated

6 years ago
Depends on: 673569
(Assignee)

Comment 30

6 years ago
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
Attachment #555096 - Attachment is obsolete: true
Attachment #556772 - Flags: review?(tim.taubert)
(Assignee)

Comment 31

6 years ago
Tim: could you review this patch please? thanks!
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?
Attachment #556772 - Flags: review?(ttaubert)
(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.
Summary: error pages in panorama sometimes do not have thumbnails → Use MozAfterPaint to redraw thumbnails
(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.
(Assignee)

Comment 35

6 years ago
(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!
Depends on: 691740
No longer blocks: 598435
Duplicate of this bug: 598435
(Assignee)

Updated

6 years ago
Blocks: 692707
(Assignee)

Updated

6 years ago
Blocks: 692706
(Assignee)

Comment 37

6 years ago
Created attachment 567706 [details]
v9

unrotted
Attachment #556772 - Attachment is obsolete: true
(Assignee)

Comment 38

6 years ago
Created attachment 568861 [details]
v10

Updated the patch for the new directory of Panorama.
Attachment #567706 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 691740
No longer depends on: 691740
(Assignee)

Comment 39

6 years ago
Created attachment 576097 [details] [diff] [review]
v11

Minor update

Passed Try
https://tbpl.mozilla.org/?tree=Try&rev=e71d91fd8445
Attachment #568861 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #576097 - Attachment description: v10 → v11
(Assignee)

Comment 40

6 years ago
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
Attachment #576097 - Flags: review?(ttaubert)
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?
Attachment #576097 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 42

6 years ago
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
Attachment #576097 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Comment 43

6 years ago
Please also check in the patch for bug 691740 at the same time
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/54346920e122
Whiteboard: [fixed-in-fx-team]
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...
Whiteboard: [fixed-in-fx-team]
Logs are back! It's only on Win dbg...

https://tbpl.mozilla.org/php/getParsedLog.php?id=7659879&tree=Fx-Team
(Assignee)

Comment 47

6 years ago
Created attachment 580107 [details] [diff] [review]
v13

Minor update
Attachment #577561 - Attachment is obsolete: true
(Assignee)

Comment 48

5 years ago
Created attachment 614373 [details] [diff] [review]
v14

Updated the patch again and hope this passes try
Attachment #580107 - Attachment is obsolete: true
(Assignee)

Comment 49

5 years ago
Created attachment 614718 [details] [diff] [review]
v15

Finally, this latest patch passed try.

https://tbpl.mozilla.org/?tree=Try&rev=1882ffb4442a
Attachment #614373 - Attachment is obsolete: true
Attachment #614718 - Flags: review?(ttaubert)
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?
Attachment #614718 - Flags: review?(ttaubert)
No longer depends on: 673569
(Assignee)

Comment 51

5 years ago
Created attachment 615797 [details] [diff] [review]
v16
Attachment #615797 - Flags: review?(ttaubert)
(Assignee)

Comment 52

5 years ago
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
(Assignee)

Updated

5 years ago
Attachment #614718 - Attachment is obsolete: true
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);
Attachment #615797 - Flags: review?(ttaubert) → review+
(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 :)
(Assignee)

Comment 55

5 years ago
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
Attachment #615797 - Attachment is obsolete: true
(Assignee)

Comment 56

5 years ago
> > ::: 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;
> >
(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!
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/5d69cf5ce278
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Blocks: 633096
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.