Closed Bug 1457749 Opened 7 years ago Closed 7 years ago

Stop using RDF service in add-on manager code

Categories

(Toolkit :: Add-ons Manager, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(3 files)

The RDF service is pretty bad, and is going away anyway. Our uses of it are also going away, but in the mean time, it's a pretty simple task to migrate them to a pure JS implementation.
Comment on attachment 8971843 [details] But 1457749: Part 1 - Import RDFDataSource from McCoy and modernize. https://reviewboard.mozilla.org/r/240588/#review246430 Do we actually care about serializing the data back to disk anywhere? If not there is substantial clean-up we could do here. Started to review but then realised I'm basically reviewing code I wrote. Did you effectively review the code I wrote in the first place before making the changes to it? ::: toolkit/mozapps/extensions/internal/RDFDataSource.jsm:3 (Diff revision 2) > +/* > +# ***** BEGIN LICENSE BLOCK ***** > +# Version: MPL 1.1/GPL 2.0/LGPL 2.1 I believe that as the author of the entire code that this is based on I can say that it is ok to relicense this under MPL 2 ::: toolkit/mozapps/extensions/internal/RDFDataSource.jsm:67 (Diff revision 2) > +const NS_RDF = "http://www.w3.org/1999/02/22-rdf-syntax-ns#"; > +const NS_NC = "http://home.netscape.com/NC-rdf#"; > + > +/* eslint prefer-template: 1 */ > + > +function raw(strings) { Nice! ::: toolkit/mozapps/extensions/internal/RDFDataSource.jsm:204 (Diff revision 2) > +function RDF_R(name) { > + return NS_RDF + name; > +} > + > +/** > + * Because Mozilla doesn't support all of DOM3 yet :( Not sure what I was referring to here but I don't see anything in the spec that would allow this so I suggest just removing this comment. ::: toolkit/mozapps/extensions/internal/RDFDataSource.jsm:218 (Diff revision 2) > + newdomnode.appendChild(domnode.firstChild); > + for (let attr of domnode.attributes) { > + domnode.removeAttributeNode(attr); > + newdomnode.setAttributeNode(attr); > + } > + newdomnode.parentNode.removeChild(domnode); Either domnode.remove() or just replace the add/remove with domnode.parentNode.replaceChild(...) ::: toolkit/mozapps/extensions/internal/RDFDataSource.jsm:232 (Diff revision 2) > + return attr; > + } > + throw new Error("cannot rename node of this type"); > +} > + > +function predicateOrder(a, b) { This could be simplified as a.getPredicate().localeCompare(b.getPredicate()) ::: toolkit/mozapps/extensions/internal/RDFDataSource.jsm:306 (Diff revision 2) > + if (this._isSubjectElement) > + return this._DOMnode; > + return this._DOMnode.parentNode; > + } > + > + getSubject() { Maybe use getters for these. Not that bothered though. ::: toolkit/mozapps/extensions/internal/RDFDataSource.jsm:347 (Diff revision 2) > + > + /** > + * Compares this literal to another node. > + */ > + equals(rdfnode) { > + if (rdfnode instanceof RDFLiteral) I think if you compare constructors here you won't have to replicate this method so much. ::: toolkit/mozapps/extensions/internal/RDFDataSource.jsm:1395 (Diff revision 2) > + * Guess the indent level within the given Element. The method looks for > + * elements that are preceeded by whitespace including a newline. The > + * whitespace following the newline is presumed to be the indentation for the > + * element. > + * If the indentation cannot be guessed then it recurses up the document > + * hierarchy until it can guess the indent or untl the Document is reached. Nit: s/untl/until/
Comment on attachment 8971843 [details] But 1457749: Part 1 - Import RDFDataSource from McCoy and modernize. https://reviewboard.mozilla.org/r/240588/#review246430 I don't really care about serializing, no. I mostly left that code because it didn't seem worth the trouble to remove it. I reviewed most of the code that was relevant to me (i.e., not the serialization parts). I didn't pay much attention to the RDF logic, and mostly decided to trust tests to tell me if it was sane.
Priority: -- → P2
Comment on attachment 8971843 [details] But 1457749: Part 1 - Import RDFDataSource from McCoy and modernize. https://reviewboard.mozilla.org/r/240588/#review248002 Probably some cleanups to be done here since we don't care about serialization, but that can be a follow-up.
Attachment #8971843 - Flags: review?(dtownsend) → review+
Comment on attachment 8971844 [details] Bug 1457749: Part 2 - Update UpdateRDFConverter to use RDFDataSource.jsm. https://reviewboard.mozilla.org/r/240590/#review248010
Attachment #8971844 - Flags: review?(dtownsend) → review+
Comment on attachment 8971845 [details] Bug 1457749: Part 3 - Migrate install.rdf parser to use RDFDataSource.jsm. https://reviewboard.mozilla.org/r/240592/#review248020
Attachment #8971845 - Flags: review?(dtownsend) → review+
Comment on attachment 8971843 [details] But 1457749: Part 1 - Import RDFDataSource from McCoy and modernize. https://reviewboard.mozilla.org/r/240588/#review246430 > Either domnode.remove() or just replace the add/remove with domnode.parentNode.replaceChild(...) TIL: https://developer.mozilla.org/en-US/docs/Web/API/ChildNode/replaceWith
https://hg.mozilla.org/integration/mozilla-inbound/rev/bef795f8c45c7cb33ab59c02348ad40e35660195 Bug 1457749: Part 1 - Import RDFDataSource from McCoy and modernize. r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/7f657f4d30880395e20f990c5c97ece275a61de2 Bug 1457749: Part 2 - Update UpdateRDFConverter to use RDFDataSource.jsm. r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/292cf80054b4734a0d7c84e987f68e229f2ccc24 Bug 1457749: Part 3 - Migrate install.rdf parser to use RDFDataSource.jsm. r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/e640d678c9881de3f465257aac6858e34c76b034 Bug 1457749: Follow-up: Fix invalid install manifest in tabpaint test add-on. r=bustage CLOSED TREE
Flags: needinfo?(kmaglione+bmo)
Backout by dluca@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5f85b4722429 Backed out 4 changesets for making T-e10s(o) hang
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc90a032770b4c4dfc80191da989b770ac3fc2cc Bug 1457749: Part 1 - Import RDFDataSource from McCoy and modernize. r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/f470abadfa78fc38e8ecd394bb893e1a1a4c71e3 Bug 1457749: Part 2 - Update UpdateRDFConverter to use RDFDataSource.jsm. r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/a2bc8b64659ae9e3387aa7b599aa38be773041e5 Bug 1457749: Part 3 - Migrate install.rdf parser to use RDFDataSource.jsm. r=Mossop
Flags: needinfo?(kmaglione+bmo)
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag. Thanks!
Flags: needinfo?(kmaglione+bmo)
Probably does not need additional QA. We don't support install.rdf manifests aside from system add-ons and a few special cases, anymore. Might be a good idea to test non-AMO update.rdf manifests, if we knew which ones to check. But we don't.
Flags: needinfo?(kmaglione+bmo) → qe-verify-
Depends on: 1475289
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: