1329182 - Make newURI's last two parameters optional

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Florian Quèze [:florian]]

Attachment: 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.

Comment 2

[Florian Quèze [:florian]]

Attachment: 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 3

[Patrick McManus [:mcmanus]]

Comment on attachment 8824391 [details] [diff] [review]
Patch

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

this is good. thanks

Comment 4

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47ff31f5d83e

Comment 5

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0258dab76e85

Comment 6

[Florian Quèze [:florian]]

Attachment: 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.

Comment 7

[Florian Quèze [:florian]]

Attachment: 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.

Comment 8

[Florian Quèze [:florian]]

Attachment: 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).

Comment 9

[Florian Quèze [:florian]]

Attachment: cleanup browser/

Automatic changes, except for browser/base/content/newtab/sites.js.

Comment 10

[Florian Quèze [:florian]]

Attachment: cleanup toolkit/

Automatic changes, except for:
  toolkit/components/url-classifier/content/trtable.js
  toolkit/components/jsdownloads/src/DownloadIntegration.jsm

Comment 11

[Florian Quèze [:florian]]

Attachment: cleanup devtools/

Automatic changes. Feel free to forward to someone else if you aren't comfortable reviewing the devtools patch.

Comment 12

[Florian Quèze [:florian]]

Attachment: 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.

Comment 13

[Florian Quèze [:florian]]

Attachment: 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.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 17

[Florian Quèze [:florian]]

(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.

Comment 18

[Florian Quèze [:florian]]

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

[Carsten Book [:Tomcat]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(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.

Comment 21

[Florian Quèze [:florian]]

(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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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)

Comment 23

[:Gijs (he/him)]

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. :-(

Comment 24

[Florian Quèze [:florian]]

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

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8824391 [details] [diff] [review]
Patch

make future cherry-picks to 52 easier, beta52+

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b888fcaaa839

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/b888fcaaa839