Last Comment Bug 674926 - refactor the webProgressListener used to keep track of whether to save tab thumbnails
: refactor the webProgressListener used to keep track of whether to save tab th...
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 9
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 671101 685104 689042
Blocks: fxe10s 677596
  Show dependency treegraph
 
Reported: 2011-07-28 08:40 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
9 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (26.20 KB, patch)
2011-07-28 08:45 PDT, Tim Taubert [:ttaubert]
raymond: feedback+
Details | Diff | Splinter Review
patch v2 (28.18 KB, patch)
2011-08-09 09:58 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v3 (28.15 KB, patch)
2011-08-11 17:20 PDT, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review
patch v4 (30.95 KB, patch)
2011-08-22 03:56 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v5 (30.96 KB, patch)
2011-08-22 04:58 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v6 (31.18 KB, patch)
2011-08-25 02:00 PDT, Tim Taubert [:ttaubert]
dietrich: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2011-07-28 08:40:06 PDT
We should refactor this because:

1) It's currently included in the ThumbnailStorage and this is more about policy than actual storage. So we could move it to a separate object/file and clean up the code.

2) The current code does not remove excluded browsers from the internal list when tabs get closed and keeps those objects alive.

3) The current code accesses .contentDocument, .webProgress and does some other stuff that is not e10s safe. Make it future-proof.
Comment 1 Tim Taubert [:ttaubert] 2011-07-28 08:45:29 PDT
Created attachment 549126 [details] [diff] [review]
patch v1

Here's what I did:

* removed all storage policy related code from ThumbnailStorage
* added the StoragePolicy object
* refactored tabview/content.js and added e10s code related to the storage policy
* updated the test for bug 627239
Comment 2 Raymond Lee [:raymondlee] 2011-07-28 10:47:34 PDT
Comment on attachment 549126 [details] [diff] [review]
patch v1

Looks good
Comment 3 Tim Taubert [:ttaubert] 2011-08-09 09:58:09 PDT
Created attachment 551808 [details] [diff] [review]
patch v2

Renamed browser_tabview_bug627239.js to browser_tabview_storage_policy.js so that we have a named test for the storage policy object.
Comment 4 Tim Taubert [:ttaubert] 2011-08-11 17:20:19 PDT
Created attachment 552553 [details] [diff] [review]
patch v3

Fixed a DOMDocument leak in browser/base/content/tabview/content.js.
Comment 5 Dietrich Ayala (:dietrich) 2011-08-11 18:28:26 PDT
Comment on attachment 552553 [details] [diff] [review]
patch v3

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

r=me with a few minor changes.

::: browser/base/content/tabview/content.js
@@ +57,5 @@
>    }
> +};
> +
> +let MessageHandler = {
> +  isDocumentLoaded: function MessageHandler_isDocumentLoaded(cx) {

add a short comment summarizing why sending these?

@@ +65,5 @@
> +    sendAsyncMessage(cx.name, {isLoaded: isLoaded});
> +  }
> +};
> +
> +let WebProgressListener = {

add summary comment about what the progress listener is doing and why.

@@ +105,5 @@
> +  },
> +
> +  _isNoStoreResponse: function WebProgressListener__isNoStoreResponse(req) {
> +    try {
> +      return req.isNoStoreResponse();

does this really throw?

also, maybe we need a utils.tryThisAndReturnFalseOnException(function).

@@ +135,5 @@
> +// add message listeners
> +addMessageListener("Panorama:isDocumentLoaded", MessageHandler.isDocumentLoaded);
> +
> +// add web progress listener
> +webProgress.addProgressListener(WebProgressListener, Ci.nsIWebProgress.NOTIFY_STATE_ALL);

Code organization nits:

* s/EventHandler/WindowEventHandler/
* s/MessageListener/WindowMessageListener/

Also, I find it clearer to have the listener code and the add*Listener calls adjacent.

These organization nits are optional fixes.

::: browser/base/content/tabview/storagePolicy.js
@@ +92,5 @@
> +  // Function: _initializeBrowser
> +  // Initializes the given browser and checks if we need to add it to our
> +  // internal exclusion list.
> +  _initializeBrowser: function StoragePolicy__initializeBrowser(browser) {
> +    let self = this;

you're bind()ing above, which should remove the need for this right?

@@ +131,5 @@
> +  },
> +
> +  // ----------
> +  // Function: uninit
> +  // Should be called when window is unloaded.

"should" is a funny word to use here. can you comment on what code exactly calls it?

@@ +186,5 @@
> +
> +  // ----------
> +  // Function: canStoreThumbnailFor
> +  // Returns whether we're allowed to store the thumbnail of the given tab.
> +  canStoreThumbnailFor: function StoragePolicy_canStoreThumbnailFor(tab) {

canStoreThumbnailForTab would be clearer. but longer. or are you planning on supporting other things to be passed?

@@ +192,5 @@
> +  },
> +
> +  // ----------
> +  // Function: observe
> +  // Observe pref changes.

Someday, we should make a module for binding vars to prefs, instead of duplicating this pref observer infrastructure all over the tree. Something like:

Cu.import('prefbindery.js').bind('pref.name', myLocalVar)
Comment 6 Tim Taubert [:ttaubert] 2011-08-14 17:33:43 PDT
(In reply to Dietrich Ayala (:dietrich) from comment #5)
> > +    try {
> > +      return req.isNoStoreResponse();
> 
> does this really throw?

Yeah, I've seen lots of errors in old test logs. Also documented in https://developer.mozilla.org/en/NsIHttpChannel#isNoStoreResponse%28%29

> also, maybe we need a utils.tryThisAndReturnFalseOnException(function).

Added Utils.attempt() that accepts any number of functions. It returns immediately the return value of the first non-failed function without executing successive functions, or null.

> > +  _initializeBrowser: function StoragePolicy__initializeBrowser(browser) {
> > +    let self = this;
> 
> you're bind()ing above, which should remove the need for this right?

No, we still need it for checkExclusion() and waitForDocumentLoad() which are defined in _initializeBrowser().

> > +  // Function: canStoreThumbnailFor
> > +  // Returns whether we're allowed to store the thumbnail of the given tab.
> > +  canStoreThumbnailFor: function StoragePolicy_canStoreThumbnailFor(tab) {
> 
> canStoreThumbnailForTab would be clearer. but longer. or are you planning on
> supporting other things to be passed?

Actually, no. Renamed to canStoreThumbnailForTab.

> Someday, we should make a module for binding vars to prefs, instead of
> duplicating this pref observer infrastructure all over the tree. Something
> like:
> 
> Cu.import('prefbindery.js').bind('pref.name', myLocalVar)

+1. That would make lots of code much easier. This pattern is repeated all over the code base...
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-19 21:04:46 PDT
>+// add web progress listener
>+webProgress.addProgressListener(WebProgressListener, Ci.nsIWebProgress.NOTIFY_STATE_ALL);

You don't actually use ALL state messages. We found that tightening the filter yielded a Tp improvement in bug 632005.
Comment 8 Tim Taubert [:ttaubert] 2011-08-20 06:16:15 PDT
(In reply to Mark Finkle (:mfinkle) from comment #7)
> You don't actually use ALL state messages. We found that tightening the
> filter yielded a Tp improvement in bug 632005.

Good point! Updated from NOTIFY_STATE_ALL to NOTIFY_STATE_WINDOW (that saves an "if" as well, nice). Thanks!
Comment 9 Tim Taubert [:ttaubert] 2011-08-21 10:05:44 PDT
http://hg.mozilla.org/integration/fx-team/rev/7837b186b10b
Comment 10 Tim Taubert [:ttaubert] 2011-08-21 13:48:25 PDT
Backed out because of bug 671101:

http://hg.mozilla.org/integration/fx-team/rev/eb8e9468f409
Comment 11 Raymond Lee [:raymondlee] 2011-08-22 03:51:44 PDT
Comment on attachment 552553 [details] [diff] [review]
patch v3

+  // ----------
+  // Function: _initializeBrowser
+  // Initializes the given browser and checks if we need to add it to our
+  // internal exclusion list.
+  _initializeBrowser: function StoragePolicy__initializeBrowser(browser) {
+    let self = this;
+
+    function checkExclusion() {
+      if (browser.currentURI.schemeIs("https"))
+        self._deniedBrowsers.push(browser);
+    }
+
+    function waitForDocumentLoad() {
+      mm.addMessageListener("Panorama:Load", function onLoad(cx) {
+        mm.removeMessageListener(cx.name, onLoad);
+        checkExclusion(browser);
+      });

mm isn't defined within _initializeBrowser()?

+    }
+
+    this._isDocumentLoaded(browser, function (isLoaded) {
+      if (isLoaded)
+        checkExclusion();
+      else
+        waitForDocumentLoad();
+    });
+  },
Comment 12 Tim Taubert [:ttaubert] 2011-08-22 03:56:29 PDT
Created attachment 554816 [details] [diff] [review]
patch v4

(In reply to Raymond Lee [:raymondlee] from comment #11)
> +    function waitForDocumentLoad() {
> +      mm.addMessageListener("Panorama:Load", function onLoad(cx) {
> +        mm.removeMessageListener(cx.name, onLoad);
> +        checkExclusion(browser);
> +      });
> 
> mm isn't defined within _initializeBrowser()?

Oops :/ my bad. Fixed.
Comment 13 Tim Taubert [:ttaubert] 2011-08-22 04:58:11 PDT
Created attachment 554826 [details] [diff] [review]
patch v5

Added missing "capture" argument to addEventListener() calls.
Comment 14 Tim Taubert [:ttaubert] 2011-08-25 02:00:02 PDT
Created attachment 555676 [details] [diff] [review]
patch v6

Changes:

1) Wrapped the variable "webProgress" in a getter to not leak docShells (see bug 671101). Approach tested on try - http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=14f76836536d.

2) Replaced the "load" event with "DOMContentLoaded" because we actually want to know when the content has loaded, the "load" event fires way too late.
Comment 15 Tim Taubert [:ttaubert] 2011-09-01 05:25:12 PDT
http://hg.mozilla.org/integration/fx-team/rev/63becbe85737
Comment 16 Rob Campbell [:rc] (:robcee) 2011-09-02 05:37:09 PDT
Comment on attachment 555676 [details] [diff] [review]
patch v6

http://hg.mozilla.org/mozilla-central/rev/63becbe85737

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