The default bug view has changed. See this FAQ.

Make newURI's last two parameters optional

RESOLVED FIXED in Firefox 52

Status

()

Core
Networking
--
enhancement
RESOLVED FIXED
3 months ago
a month ago

People

(Reporter: florian, Assigned: florian)

Tracking

53 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed, firefox-esr52 fixed)

Details

Attachments

(9 attachments, 1 obsolete attachment)

(Assignee)

Description

3 months ago
Most calls to Services.io.newURI pass null for the last two parameters. This is annoying to the point that the NetUtil.newURI wrapper is sometimes used just for the benefit of omitting the last 2 parameters.

I suspect the only reasons for these last 2 parameters not being optional is that this interface was written and frozen before xpidl got support for optional parameters.
(Assignee)

Comment 1

3 months ago
Created attachment 8824391 [details] [diff] [review]
Patch

I made the changes to both nsIProtocolHandler and nsIIOService for consistency, but it's really only nsIIOService that I care about.

I'm willing to work on a script to mass remove the ", null, null" from existing JS code and get patches up for review for at least browser/ and toolkit/, but I think this will be better as separate patches, and probably in separate follow-up bugs. Just making the interface changes is enough to avoid the frustration when writing new code.
Attachment #8824391 - Flags: review?(mcmanus)
(Assignee)

Updated

3 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 months ago
Created attachment 8824424 [details] [diff] [review]
Test

This adapts an existing test to report how many of the newURI callers we have in the .js and .jsm files shipped in omni.ja are not using the 2nd and 3rd parameters, the result is 189 out of 226 callers (more than 80%).
Comment on attachment 8824391 [details] [diff] [review]
Patch

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

this is good. thanks
Attachment #8824391 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 4

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47ff31f5d83e
(Assignee)

Comment 5

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0258dab76e85
(Assignee)

Comment 6

3 months ago
Created attachment 8824716 [details] [diff] [review]
Test v2

Test matching what I pushed to try in comment 5. I don't think we want to land this test, but I could be convinced otherwise. I'm attaching it mostly because it could be intereting to refer to this in the future to do a similar analysis on another method.
(Assignee)

Updated

3 months ago
Attachment #8824424 - Attachment is obsolete: true
(Assignee)

Comment 7

3 months ago
Created attachment 8824717 [details]
xpcshell script

This is the xpcshell script I used to generate patches removing the trailing null parameters on newURI calls.

I put this file at the top of an m-c tree, and invoke it like this:
<objdir>/dist/bin/xpcshell newURI.js

It modifies automatically all the .js and .jsm files it finds in the tree that contain the string '.newURI('.

Multi-line removals are handled correctly, for example:

-        var uri = ios.newURI("data:text/plain;charset=utf-8," + text,
-                             null, null);
+        var uri = ios.newURI("data:text/plain;charset=utf-8," + text);

-          let documentURI = Services.io.newURI(contentWindow.document.documentURI,
-                                               null,
-                                               null);
+          let documentURI = Services.io.newURI(contentWindow.document.documentURI);



Running the script takes about 2 minutes on my macbook. Most of the time is spent walking the m-c tree. If I restrict it to browser/ or toolkit/ it finishes within 5s.


It failed to parse the following files due to preprocessing directives:
  toolkit/components/url-classifier/content/trtable.js
  toolkit/components/jsdownloads/src/DownloadIntegration.jsm
  testing/specialpowers/content/specialpowersAPI.js
  layout/tools/reftest/reftest.jsm
  browser/base/content/newtab/sites.js

I hand edited these files, as for so few files it was easier than attempting to workaround the preprocessor in the script.
(Assignee)

Updated

3 months ago
Attachment #8824717 - Attachment mime type: application/binary → text/plain
(Assignee)

Comment 8

3 months ago
Created attachment 8824718 [details] [diff] [review]
cleanup XBL bindings

There were not enough newURI calls in XBL bindings for this to be worth scripting, I edited these by hand (but used a test locally to verify I wasn't missing any occurence).
Attachment #8824718 - Flags: review?(jaws)
(Assignee)

Comment 9

3 months ago
Created attachment 8824719 [details] [diff] [review]
cleanup browser/

Automatic changes, except for browser/base/content/newtab/sites.js.
Attachment #8824719 - Flags: review?(jaws)
(Assignee)

Comment 10

3 months ago
Created attachment 8824720 [details] [diff] [review]
cleanup toolkit/

Automatic changes, except for:
  toolkit/components/url-classifier/content/trtable.js
  toolkit/components/jsdownloads/src/DownloadIntegration.jsm
Attachment #8824720 - Flags: review?(jaws)
(Assignee)

Comment 11

3 months ago
Created attachment 8824721 [details] [diff] [review]
cleanup devtools/

Automatic changes. Feel free to forward to someone else if you aren't comfortable reviewing the devtools patch.
Attachment #8824721 - Flags: review?(jaws)
(Assignee)

Comment 12

3 months ago
Created attachment 8824722 [details] [diff] [review]
cleanup the rest of the tree

Automatic changes except for:
  testing/specialpowers/content/specialpowersAPI.js
  layout/tools/reftest/reftest.jsm

Let me know if you aren't comfortable reviewing this, and we'll add more eyes on it. What makes me confident is that try looks green.
Attachment #8824722 - Flags: review?(jaws)
(Assignee)

Comment 13

3 months ago
Created attachment 8824723 [details] [diff] [review]
cleanup addon-sdk/source/lib/sdk/indexed-db.js

My script missed this occurence because '.' and 'newURI(' are on different lines, but the test (attachment 8824716 [details] [diff] [review]) caught it on the try push in comment 4.
Attachment #8824723 - Flags: review?(jaws)
Attachment #8824718 - Flags: review?(jaws) → review+
Comment on attachment 8824719 [details] [diff] [review]
cleanup browser/

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

::: browser/components/places/tests/unit/test_clearHistory_shutdown.js
@@ +178,2 @@
>                          Ci.nsICacheStorage.OPEN_READONLY,
>                          checkCacheListener);

Please fix the indentation of these two lines while you're here.
Attachment #8824719 - Flags: review?(jaws) → review+
Comment on attachment 8824720 [details] [diff] [review]
cleanup toolkit/

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

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +482,5 @@
>      // Log the event if required by parental controls settings.
>      if (isEnabled && gParentalControlsService.loggingEnabled) {
>        gParentalControlsService.log(gParentalControlsService.ePCLog_FileDownload,
>                                     shouldBlock,
> +                                   NetUtil.newURI(aDownload.source.url));

note, the removal of 'null' here is not related to removing the extra parameters to newURI but looks like something that was just cleaned up since the fourth argument to log() is optional. I hope this wasn't done by the automated script because there may be arguments to other functions that were removed accidentally.

::: toolkit/modules/RemoteWebProgress.jsm
@@ +12,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  function newURI(spec) {
>      return Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService)
> +                                                    .newURI(spec);

nit, please fix the indentation here so that the periods line up vertically.
Attachment #8824720 - Flags: review?(jaws) → review+
Comment on attachment 8824721 [details] [diff] [review]
cleanup devtools/

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

::: devtools/client/webconsole/webconsole.js
@@ +1502,1 @@
>                        .QueryInterface(Ci.nsIURL);

nit, please fix the indentation here so that the periods line up vertically (should line up with the last period of line 1501).

::: devtools/shared/touch/simulator-core.js
@@ +17,1 @@
>        .prePath;

nit, please fix the indentation here too.
Attachment #8824721 - Flags: review?(jaws) → review+
Attachment #8824722 - Flags: review?(jaws) → review+
Attachment #8824723 - Flags: review?(jaws) → review+
(Assignee)

Comment 17

3 months ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)

> ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
> @@ +482,5 @@
> >      // Log the event if required by parental controls settings.
> >      if (isEnabled && gParentalControlsService.loggingEnabled) {
> >        gParentalControlsService.log(gParentalControlsService.ePCLog_FileDownload,
> >                                     shouldBlock,
> > +                                   NetUtil.newURI(aDownload.source.url));
> 
> note, the removal of 'null' here is not related to removing the extra
> parameters to newURI but looks like something that was just cleaned up since
> the fourth argument to log() is optional. I hope this wasn't done by the
> automated script because there may be arguments to other functions that were
> removed accidentally.

Great catch! It was done by hand (one of the files the script couldn't handle due to preprocessing directives) and was an accident; I had scanned so many ', null' that evening already that any null at the end of a call looked like something to remove.
(Assignee)

Comment 18

3 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/52f8121f00650836089d6f309aee266c7b5f878d
Bug 1329182 - Make newURI's last two parameters optional, r=mcmanus.

https://hg.mozilla.org/integration/mozilla-inbound/rev/35cb8eb1856d275ca3f2e849974e5b33343ebab2
Bug 1329182 - remove trailing newURI null parameters in XBL bindings, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0d77afb2e04228f6878c228cab954eb793540e
Bug 1329182 - remove trailing newURI null parameters in browser/, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4a26954062015731b0643f4575935581d9e545
Bug 1329182 - remove trailing newURI null parameters in toolkit/, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fca014f3f8aaf1932fa243c0f7f17fe9057f6c12
Bug 1329182 - remove trailing newURI null parameters in devtools/, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d816f539d536fcb917e1ce8ba012e7f2873bc3e6
Bug 1329182 - remove trailing newURI null parameters in the rest of the tree, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/79c7e34346304105ab56ff302a070c4a13329627
Bug 1329182 - remove trailing newURI null parameters in addon-sdk's indexed-db.js, r=jaws.

Comment 19

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/52f8121f0065
https://hg.mozilla.org/mozilla-central/rev/35cb8eb1856d
https://hg.mozilla.org/mozilla-central/rev/3e0d77afb2e0
https://hg.mozilla.org/mozilla-central/rev/ae4a26954062
https://hg.mozilla.org/mozilla-central/rev/fca014f3f8aa
https://hg.mozilla.org/mozilla-central/rev/d816f539d536
https://hg.mozilla.org/mozilla-central/rev/79c7e3434630
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(In reply to Carsten Book [:Tomcat] from comment #19)
> https://hg.mozilla.org/mozilla-central/rev/3e0d77afb2e0

In the future, please make sure that the changes to pdf.js get upstreamed as well to avoid accidental clobbering on the next update. I've gone ahead and filed the upstream issue for it this time.
Flags: needinfo?(florian)
(Assignee)

Comment 21

3 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> (In reply to Carsten Book [:Tomcat] from comment #19)
> > https://hg.mozilla.org/mozilla-central/rev/3e0d77afb2e0
> 
> In the future, please make sure that the changes to pdf.js get upstreamed as
> well to avoid accidental clobbering on the next update. I've gone ahead and
> filed the upstream issue for it this time.

Thanks!

Is there a document somewhere indicating which files in our tree are upstream and have a special process associated to modifying them? I started preparing another automated mass-cleanup for addEventListener calls, and for now I'm just relying on my best guesses to decide which files I should ignore or handle differently.
Flags: needinfo?(florian)
Not that I know of. In general, I'd be wary of anything in browser/extensions, though. (At least worth a glance at the commit history within those subdirs)
Depends on: 1332910
(Assignee)

Updated

2 months ago
Depends on: 1334261

Comment 23

2 months ago
Could we take the patch modifying the actual implementation / idl, and only that patch, on 52? This is going to make any uplift that calls newURI break when uplifted to 52 (which will be an ESR). Just happened to me in bug 1335272 and it won't be the last time, given that eslint now enforces not passing the extra parameters on m-c. :-(
Flags: needinfo?(florian)
(Assignee)

Comment 24

2 months ago
Comment on attachment 8824391 [details] [diff] [review]
Patch

Approval Request Comment
[Reason for requesting uplift]: simplify future uplifts to the ESR52 branch, per comment 23
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only makes parameters optional.
[String changes made/needed]: none
Flags: needinfo?(florian)
Attachment #8824391 - Flags: approval-mozilla-beta?
Comment on attachment 8824391 [details] [diff] [review]
Patch

make future cherry-picks to 52 easier, beta52+
Attachment #8824391 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 26

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/b888fcaaa839
status-firefox52: --- → fixed

Comment 27

a month ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/b888fcaaa839
status-firefox-esr52: --- → fixed
You need to log in before you can comment on or make changes to this bug.