1470044 - Copy persisted attributes from xulStore.json when loading overlays

Status: RESOLVED FIXED

(MailNews Core :: XUL Replacements, enhancement)

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1470044-persist-1.diff

As mentioned in bug 1448808 comment 40, this patch applies the persisted attributes to the nodes before adding them to the document. I'm not 100% sure I've done this at the right point, but it does seem to work.

Comment 2

[Patrick Brunschwig]

Comment on attachment 8986642 [details] [diff] [review]
1470044-persist-1.diff

Seems to be alright (without testing it)

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8986642 [details] [diff] [review]
1470044-persist-1.diff

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

r=me with comments considered

::: common/src/Overlays.jsm
@@ +56,4 @@
>      this.window = window;
> +
> +    this.location = window.location.protocol + "//" +
> +        window.location.host + window.location.pathname;

Could you use window.location.origin + window.location.pathname ?

@@ +61,5 @@
> +    this.persistenceIDs = [];
> +    let ids = xulStoreService.getIDsEnumerator(this.location);
> +    while (ids.hasMore()) {
> +      this.persistenceIDs.push(ids.getNext());
> +    }

Loading all ids seems like it could be a performance hit, does it make sense to cache/reuse this in some way?

@@ +262,5 @@
>     * @param {Element} node          The node to insert.
>     */
>    _insertElement(parent, node) {
> +    let loadPersistentAttributes = (element) => {
> +      if (this.persistenceIDs.includes(element.id)) {

If persistenceIDs is an array this is O(n), maybe you could use new Set() instead to make it O(1)

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: 1470044-persist-2.diff

Unfortunately my code was running too early and didn't quite work for some things, so I've moved it. It's also a lot tidier here.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8989308 [details] [diff] [review]
1470044-persist-2.diff

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

I believe this needs another round, please see the following comments:

::: common/src/Overlays.jsm
@@ +15,5 @@
>  ChromeUtils.defineModuleGetter(this, "setTimeout", "resource://gre/modules/Timer.jsm");
>  
>  Cu.importGlobalProperties(["XMLHttpRequest"]);
>  
> +let xulStoreService = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);

Can you make this a lazy getter?

@@ +172,5 @@
>        // Now here is where it gets a little tricky. The subscript loader is synchronous, but the
>        // script itself may have some asynchronous side-effects (xbl bindings attached). Throwing in a
>        // 1500ms timeout before we fire the load handlers seems to help, even though it is an ugly hack.
>        setTimeout(() => {
> +        let ids = xulStoreService.getIDsEnumerator(this.location);

Note this code only runs when the window is already loaded, which was originally thought for loading overlays on the fly into already open windows. The latest ext-legacy.js code should be loading the overlay before the window load is complete, because it only triggers on startup and before dialogs are completely loaded.

Could you test this for both cases and also find the right time to load the xul store state for not yet loaded windows?

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Also, see previous review comments from comment 3. Let me know if I missed something.

Comment 7

[Geoff Lankow (:darktrojan)]

(In reply to Philipp Kewisch [:Fallen]  from comment #5)
> Note this code only runs when the window is already loaded, which was
> originally thought for loading overlays on the fly into already open
> windows. The latest ext-legacy.js code should be loading the overlay before
> the window load is complete, because it only triggers on startup and before
> dialogs are completely loaded.

Unless I'm reading it wrong, the overlays are never added before the window loads. The extensionSupport window listener waits for the load event before calling back.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Hmm you are right, it does. This could explain some of the race conditions I was experiencing. When I developed the patch, the code was mostly getting called before the readyState was completed, and before some other load handlers like the one that takes the toolbarpalette out of the window was not called. I guess it was all a coincidence, but it was helpful since the random timeout was not great.

Maybe it would be worth trying to execute the overlay loader code a little earlier, e.g. on DOMContentLoaded.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1470044-persist-3.diff

Please have another look at this after you've reviewed bug 1474248. This is actually earlier on my patch queue, but I think what I'm doing there answers the concerns you had here.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8991253 [details] [diff] [review]
1470044-persist-3.diff

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

I recall reading some of this in another patch. Looks fine :)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/70ce87dcbe19
Copy persisted attributes from xulStore.json when loading overlays. r=philipp DONTBUILD

Comment 12

[Jorg K (CEST = GMT+2)]

Beta (TB 62):
https://hg.mozilla.org/releases/comm-beta/rev/ba4452fb46ee4988525d703da7db58b4c56426fa