Closed Bug 1457749 Opened 6 years ago Closed 6 years ago

Stop using RDF service in add-on manager code

Categories

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

Product:
Toolkit
Component:
Add-ons Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(3 files)

But 1457749: Part 1 - Import RDFDataSource from McCoy and modernize.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1457749: Part 2 - Update UpdateRDFConverter to use RDFDataSource.jsm.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1457749: Part 3 - Migrate install.rdf parser to use RDFDataSource.jsm.
59 bytes, text/x-review-board-request
mossop
: review+
Details 
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
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)
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
status-firefox61: affected → wontfix
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

Creator:
Created:
Updated:
Size: