Closed Bug 1476030 Opened 6 years ago Closed 6 years ago

Expose xulStore.persist to use instead of document.persist from chrome HTML documents

Categories

(Toolkit :: General, enhancement)

Product:
Toolkit
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1476030 - Part 1 - Expose xulStore through Services;
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1476030 - Part 2 - Implement xulStore.persist as an alternative to document.persist;
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1476030 - Part 3 - Migrate callers from document.persist to xulStore.persist;
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
document.persist is exposed from XULDocument [0]. This function [1] more or less wraps xulStore.setValue, although there's a special case where it will call xulStore.removeValue if the store already has a value previously but the attribute is empty.

Ultimately, setValue needs the following arguments to work: `docURI, id, attr, value`. These can be derived from the existing API:

  document.persist(id, attr)

But they could also be derived from a new API on xulStore directly like:

  xulStore.persist(node, attr)

We can migrate existing callers from the former to the latter in this bug (which will fix HTML top-level windows that load browser.js and other scripts, and then delete document.persist in a follow up.

[0]: https://searchfox.org/mozilla-central/rev/b0275bc977ad7fda615ef34b822bba938f2b16fd/dom/webidl/XULDocument.webidl#41-42
[1]: https://searchfox.org/mozilla-central/rev/b0275bc977ad7fda615ef34b822bba938f2b16fd/dom/xul/XULDocument.cpp#1056
This is part of making the browser.js scripts work in xhtml (since document.persist is only exposed to XUL docs). Alternatively, we could expose document.persist in HTML documents, but since we already use xulStore directly throughout the codebase anyway I opted to add a new function there.
Comment on attachment 8992392 [details]
Bug 1476030 - Part 1 - Expose xulStore through Services;

https://reviewboard.mozilla.org/r/257258/#review264656

r=me assuming you've also checked that this now throws eslint errors if you still try and use the "old" way of getting the XUL store.

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:228
(Diff revision 4)
> -          let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);
> +          let xulStore = Services.xulStore;
>            return xulStore.getValue(BROWSER_DOCURL, "PersonalToolbar", "collapsed")

Nit: can consolidate this to 1 line.
Attachment #8992392 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8992393 [details]
Bug 1476030 - Part 2 - Implement xulStore.persist as an alternative to document.persist;

https://reviewboard.mozilla.org/r/257260/#review264662

::: toolkit/components/xulstore/XULStore.js:144
(Diff revision 4)
> +    // It may make more sense to drop the `hasValue` check so that
> +    // any time there's an empty attribute it gets removed from the
> +    // store. That said, this is copying behavior from document.persist,
> +    // so callers would need to be updated with that change.

Can you file a follow-up bug for this and mention the bug # here?

::: toolkit/components/xulstore/nsIXULStore.idl:24
(Diff revision 4)
>  interface nsIXULStore: nsISupports
>  {
>    /**
> +   * Sets a value for a specified node's attribute, except in
> +   * the case below (following the original XULDocument::persist):
> +   * If the value is empty and `hasValue` is true, then the

It's a bit confusing to say "`hasValue` is true" because there is no such parameter to the method - can we either describe what you mean ("... and the store does have a value for that attribute for this node"), or be more explicit ("... and if calling `hasValue` with the node's document and ID and the same attribute would return true") ?
Attachment #8992393 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8992394 [details]
Bug 1476030 - Part 3 - Migrate callers from document.persist to xulStore.persist;

https://reviewboard.mozilla.org/r/257262/#review264666

Haven't checked if this is all the callers or not, but this looks OK to me.


Do you intend to remove document.persist elsewhere?
Attachment #8992394 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs (he/him) from comment #14)
> Comment on attachment 8992392 [details]
> Bug 1476030 - Part 1 - Expose xulStore through Services;
> 
> https://reviewboard.mozilla.org/r/257258/#review264656
> 
> r=me assuming you've also checked that this now throws eslint errors if you
> still try and use the "old" way of getting the XUL store.
> 
> ::: browser/components/migration/tests/marionette/test_refresh_firefox.py:228
> (Diff revision 4)
> > -          let xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore);
> > +          let xulStore = Services.xulStore;
> >            return xulStore.getValue(BROWSER_DOCURL, "PersonalToolbar", "collapsed")
> 
> Nit: can consolidate this to 1 line.

Also, given that I'm suddenly looking at this, please add the semicolon that that line is missing. :-)
(In reply to :Gijs (he/him) from comment #16)
> Do you intend to remove document.persist elsewhere?

Yes, I'll file a bug for it now. Jorg and Arshad: we are making the following changes in this bug

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)
Blocks: 1476678
Blocks: 1476680
(In reply to :Gijs (he/him) from comment #14)
> Comment on attachment 8992392 [details]
> Bug 1476030 - Part 1 - Expose xulStore through Services;
> 
> https://reviewboard.mozilla.org/r/257258/#review264656
> 
> r=me assuming you've also checked that this now throws eslint errors if you
> still try and use the "old" way of getting the XUL store.

Yes, in fact eslint caught one place that I missed in an earlier version.
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41765435d9c5
Part 1 - Expose xulStore through Services;r=Gijs
https://hg.mozilla.org/integration/autoland/rev/7f0a7e83b3e1
Part 2 - Implement xulStore.persist as an alternative to document.persist;r=Gijs
https://hg.mozilla.org/integration/autoland/rev/3fc20e8e49f8
Part 3 - Migrate callers from document.persist to xulStore.persist;r=Gijs
https://hg.mozilla.org/mozilla-central/rev/41765435d9c5
https://hg.mozilla.org/mozilla-central/rev/7f0a7e83b3e1
https://hg.mozilla.org/mozilla-central/rev/3fc20e8e49f8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Blocks: 1473165
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: