refactor the webProgressListener used to keep track of whether to save tab thumbnails

RESOLVED FIXED in Firefox 9

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 9
Dependency tree / graph

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
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
Attachment #549126 - Flags: feedback?(raymond)
Comment on attachment 549126 [details] [diff] [review]
patch v1

Looks good
Attachment #549126 - Flags: feedback?(raymond) → feedback+
(Assignee)

Updated

6 years ago
Attachment #549126 - Flags: review?(dietrich)
(Assignee)

Comment 3

6 years ago
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.
Attachment #549126 - Attachment is obsolete: true
Attachment #549126 - Flags: review?(dietrich)
Attachment #551808 - Flags: review?(dietrich)
(Assignee)

Updated

6 years ago
Blocks: 677596
(Assignee)

Comment 4

6 years ago
Created attachment 552553 [details] [diff] [review]
patch v3

Fixed a DOMDocument leak in browser/base/content/tabview/content.js.
Attachment #551808 - Attachment is obsolete: true
Attachment #551808 - Flags: review?(dietrich)
Attachment #552553 - Flags: review?(dietrich)
Blocks: 653064
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)
Attachment #552553 - Flags: review?(dietrich) → review+
(Assignee)

Comment 6

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

Comment 8

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

Comment 9

6 years ago
http://hg.mozilla.org/integration/fx-team/rev/7837b186b10b
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 10

6 years ago
Backed out because of bug 671101:

http://hg.mozilla.org/integration/fx-team/rev/eb8e9468f409
Depends on: 671101
Whiteboard: [fixed-in-fx-team]
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();
+    });
+  },
(Assignee)

Comment 12

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

Comment 13

6 years ago
Created attachment 554826 [details] [diff] [review]
patch v5

Added missing "capture" argument to addEventListener() calls.
Attachment #554816 - Attachment is obsolete: true
(Assignee)

Comment 14

6 years ago
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.
Attachment #554826 - Attachment is obsolete: true
Attachment #555676 - Flags: review?(dietrich)
Attachment #555676 - Flags: review?(dietrich) → review+
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/integration/fx-team/rev/63becbe85737
Whiteboard: [fixed-in-fx-team]
Comment on attachment 555676 [details] [diff] [review]
patch v6

http://hg.mozilla.org/mozilla-central/rev/63becbe85737
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
(Assignee)

Updated

6 years ago
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

6 years ago
Depends on: 685104

Updated

6 years ago
Depends on: 689042
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.