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)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla62
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
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/
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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.
Updated•7 years ago
|
Priority: -- → P2
Comment 9•7 years ago
|
||
mozreview-review |
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 10•7 years ago
|
||
mozreview-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 11•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 13•7 years ago
|
||
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
Assignee | ||
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
Backed out 4 changesets (bug 1457749) for making T-e10s(o) hang
Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=177417195&repo=mozilla-inbound
Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=292cf80054b4734a0d7c84e987f68e229f2ccc24
Backout:
Backed out 4 changesets (bug 1457749) for making T-e10s(o) hang
Flags: needinfo?(kmaglione+bmo)
Comment 16•7 years ago
|
||
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f85b4722429
Backed out 4 changesets for making T-e10s(o) hang
Assignee | ||
Comment 17•7 years ago
|
||
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
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bc90a032770b
https://hg.mozilla.org/mozilla-central/rev/f470abadfa78
https://hg.mozilla.org/mozilla-central/rev/a2bc8b64659a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•7 years ago
|
Flags: needinfo?(kmaglione+bmo)
Comment 19•7 years ago
|
||
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)
Assignee | ||
Comment 20•7 years ago
|
||
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-
You need to log in
before you can comment on or make changes to this bug.
Description
•