802668 - Use Service.jsm in toolkit/content/contentAreaUtils.js

Status: RESOLVED FIXED

(Firefox :: General, defect)

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 672360 [details] [diff] [review]
patch

>diff --git a/toolkit/content/contentAreaUtils.js b/toolkit/content/contentAreaUtils.js

> var ContentAreaUtils = {
>+
>+  // this is backwards /compatibility.
>   get ioService() {
>-    delete this.ioService;

You need to keep this, otherwise the getter stays in place, I think.

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 672360 [details] [diff] [review]
patch

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

This will have to be reviewed by somebody else; I'm not a toolkit peer. See https://wiki.mozilla.org/Modules/Toolkit for options.

Comment 4

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> 
> > var ContentAreaUtils = {
> >+
> >+  // this is backwards /compatibility.
> >   get ioService() {
> >-    delete this.ioService;
> 
> You need to keep this, otherwise the getter stays in place, I think.

Do you means "not delete 'delete this.ioService;'" ?


(In reply to Josh Matthews [:jdm] from comment #3)
> This will have to be reviewed by somebody else; I'm not a toolkit peer. See
> https://wiki.mozilla.org/Modules/Toolkit for options.

Oops. I'm sorry in every times.

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 672360 [details] [diff] [review]
patch

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

That being said, this looks fine to me (with gavin's comment addressed). Dao, is my review sufficient?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 672360 [details] [diff] [review]
patch

Just a couple of minor nits...

>diff --git a/toolkit/content/contentAreaUtils.js b/toolkit/content/contentAreaUtils.js

> function urlSecurityCheck(aURL, aPrincipal, aFlags)

>+  var secMan = Services.scriptSecurityManager;
>+  if (aFlags === undefined) {
>+    aFlags = Components.interfaces.nsIScriptSecurityManager.STANDARD;

You could use secMan.STANDARD here.

>+// This is for backwords compatibility.
> function getPrefsBrowserDownload(branch)

I don't think we need to keep this; it's very specifically-named, and https://mxr.mozilla.org/addons/search?string=getPrefsBrowserDownload returns no useful hits (just wholesale copies of the file).

r=me too with those addressed.

Comment 7

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v2

Thank you for your speedy reviews. I reflect them.
How about this?

Comment 8

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> Comment on attachment 672360 [details] [diff] [review]
> patch
> 
> >diff --git a/toolkit/content/contentAreaUtils.js b/toolkit/content/contentAreaUtils.js
> 
> > var ContentAreaUtils = {
> >+
> >+  // this is backwards /compatibility.
> >   get ioService() {
> >-    delete this.ioService;
> 
> You need to keep this, otherwise the getter stays in place, I think.

This still isn't addressed.

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v3

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 672638 [details] [diff] [review]
patch v3

I guess this is fine too, this is just a compat shim after all (and getter overhead is negligible).

Comment 11

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/389e0c33ddef

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/389e0c33ddef