1476760 - Port bug 1476030 / bug 1476678 - replace use of document.persist

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

From bug 1476030 comment #18:
1) We're porting `Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);` to `Services.xulStore` (see part 1)
2) We're porting all callers from `document.persist(node.id, attr)` to `Services.xulStore.persist(node, attr)` (see part 3)

Geoff, do you have a few cycles to do this, otherwise I will.

Comment 1

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Seems simple enough. I'll do it.

Comment 2

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Attachment: 1476760-xulstore-1.diff

The change in question hasn't yet landed on central yet, so this is untested.

Comment 3

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Attachment: 1476760-persist-1.diff

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8993269 [details] [diff] [review]
1476760-xulstore-1.diff

Thanks, but I can't review Calendar. Can you please split the patch accordingly.

Also, when you now use Services.xulStore, did you check that Services is in fact available?

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8993270 [details] [diff] [review]
1476760-persist-1.diff

See previous comment.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to Geoff Lankow (:darktrojan) from comment #2)
> The change in question hasn't yet landed on central yet, so this is untested.
Now it has ;-)

Comment 7

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Attachment: 1476760-xulstore-2.diff

Comment 8

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Attachment: 1476760-xulstore-calendar-2.diff

Comment 9

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Attachment: 1476760-persist-2.diff

Comment 10

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Attachment: 1476760-persist-calendar-2.diff

Comment 11

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

(In reply to Jorg K (GMT+2) from comment #4)
> Also, when you now use Services.xulStore, did you check that Services is in
> fact available?

It's available in all those places, I've added it explicitly in the binding I'm not sure about.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8993298 [details] [diff] [review]
1476760-persist-2.diff

Thanks, looks OK by visual inspection.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8993296 [details] [diff] [review]
1476760-xulstore-2.diff

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

::: common/src/Overlays.jsm
@@ +164,5 @@
>        this.loadCSS(sheet);
>      }
>  
>      for (let bar of this._toolbarsToResolve) {
> +      let currentset = Services.xulStore.getValue(this.location, bar.id, "currentset");

calling this repeatedly is not going to make it faster, if you care about speed. I can't see the context, but this.xulStore could be used and initialised somewhere?

@@ +196,3 @@
>              while (attrNames.hasMore()) {
>                let attrName = attrNames.getNext();
> +              let attrValue = Services.xulStore.getValue(this.location, id, attrName);

Here we're in a nested while loop.

::: mail/test/mozmill/migration-to-rdf-ui-2/test-migrate-to-rdf-ui-2.js
@@ +33,1 @@
>    if (xulStore.hasValue(MESSENGER_DOCURL, "folderPaneBox", "collapsed"))

Please remove that variable.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8993296 [details] [diff] [review]
1476760-xulstore-2.diff

(In reply to Jorg K (GMT+2) from comment #13)
> calling this repeatedly is not going to make it faster, if you care about
> speed. I can't see the context, but this.xulStore could be used and
> initialised somewhere?
That's wrong, isn't it? Services.xulStore and this.xulStore are about the same thing: object.attribute.

You can still remove the variable in test-migrate-to-rdf-ui-2.js (although there is bug 1371898 to remove that).

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1476760-xulstore-2.diff - with small tweaks

You're OK with those tweaks?

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2acfc7e3c2d7114e9893b515531ee9e506ba9b53

Comment 16

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Comment on attachment 8993523 [details] [diff] [review]
1476760-xulstore-2.diff - with small tweaks

Not opposed. I can't really see the point in doing that to Overlays.jsm, but whatever.

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to Geoff Lankow (:darktrojan) from comment #16)
> Not opposed. I can't really see the point in doing that to Overlays.jsm, but
> whatever.
Thanks. Well, it was inspired by
-    let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);
+    let xulStore = Services.xulStore;
in mailMigrator.js. And getting a local variable has got to be quicker than getting Services.xulStore, no?

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a7406b1e7296
Port bug 1476030: replace use of document.persist with Services.xulStore.persist. r=jorgk
https://hg.mozilla.org/comm-central/rev/a157a1b0d6e6
Port bug 1476030: use Services.xulStore. r=jorgk
https://hg.mozilla.org/comm-central/rev/ed5da3a2f84a
Port bug 1476030: replace use of document.persist with Services.xulStore.persist in calendar/. r=philipp
https://hg.mozilla.org/comm-central/rev/6037573afc86
Port bug 1476030: use Services.xulStore in calendar/. r=philipp DONTBUILD