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

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: tetsuharu, Assigned: tetsuharu)

Tracking

Trunk
Firefox 19
x86
macOS
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

No description provided.
Posted patch patch (obsolete) — Splinter Review
Attachment #672360 - Flags: review?(josh)
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 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.
Attachment #672360 - Flags: review?(josh) → review?(dao)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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 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?
Attachment #672360 - Flags: review?(dao) → review+
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.
Attachment #672360 - Flags: review+
Posted patch patch v2 (obsolete) — Splinter Review
Thank you for your speedy reviews. I reflect them.
How about this?
Attachment #672360 - Attachment is obsolete: true
Attachment #672399 - Flags: review?(gavin.sharp)
Attachment #672399 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Posted patch patch v3Splinter Review
Attachment #672399 - Attachment is obsolete: true
Attachment #672638 - Flags: review?(gavin.sharp)
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).
Attachment #672638 - Flags: review?(gavin.sharp) → review+
Assignee: nobody → saneyuki.s.snyk
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/389e0c33ddef
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.