Last Comment Bug 1329182 - Make newURI's last two parameters optional
: Make newURI's last two parameters optional
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: 53 Branch
: Unspecified Unspecified
-- enhancement (vote)
: mozilla53
Assigned To: Florian Quèze [:florian] [:flo]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on: 1332910 1334261
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-06 05:04 PST by Florian Quèze [:florian] [:flo]
Modified: 2017-02-14 08:22 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed


Attachments
Patch (2.41 KB, patch)
2017-01-06 05:08 PST, Florian Quèze [:florian] [:flo]
mcmanus: review+
jcristau: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Test (3.44 KB, patch)
2017-01-06 06:58 PST, Florian Quèze [:florian] [:flo]
no flags Details | Diff | Splinter Review
Test v2 (3.70 KB, patch)
2017-01-07 13:26 PST, Florian Quèze [:florian] [:flo]
no flags Details | Diff | Splinter Review
xpcshell script (3.89 KB, text/plain)
2017-01-07 13:37 PST, Florian Quèze [:florian] [:flo]
no flags Details
cleanup XBL bindings (3.41 KB, patch)
2017-01-07 13:40 PST, Florian Quèze [:florian] [:flo]
jaws: review+
Details | Diff | Splinter Review
cleanup browser/ (109.74 KB, patch)
2017-01-07 13:41 PST, Florian Quèze [:florian] [:flo]
jaws: review+
Details | Diff | Splinter Review
cleanup toolkit/ (98.31 KB, patch)
2017-01-07 13:42 PST, Florian Quèze [:florian] [:flo]
jaws: review+
Details | Diff | Splinter Review
cleanup devtools/ (33.42 KB, patch)
2017-01-07 13:43 PST, Florian Quèze [:florian] [:flo]
jaws: review+
Details | Diff | Splinter Review
cleanup the rest of the tree (292.73 KB, patch)
2017-01-07 13:47 PST, Florian Quèze [:florian] [:flo]
jaws: review+
Details | Diff | Splinter Review
cleanup addon-sdk/source/lib/sdk/indexed-db.js (1.29 KB, patch)
2017-01-07 13:49 PST, Florian Quèze [:florian] [:flo]
jaws: review+
Details | Diff | Splinter Review

Description User image Florian Quèze [:florian] [:flo] 2017-01-06 05:04:10 PST
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 User image Florian Quèze [:florian] [:flo] 2017-01-06 05:08:13 PST
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.
Comment 2 User image Florian Quèze [:florian] [:flo] 2017-01-06 06:58:06 PST
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 3 User image Patrick McManus [:mcmanus] 2017-01-06 14:37:18 PST
Comment on attachment 8824391 [details] [diff] [review]
Patch

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

this is good. thanks
Comment 4 User image Florian Quèze [:florian] [:flo] 2017-01-07 11:50:19 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47ff31f5d83e
Comment 5 User image Florian Quèze [:florian] [:flo] 2017-01-07 13:23:09 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0258dab76e85
Comment 6 User image Florian Quèze [:florian] [:flo] 2017-01-07 13:26:17 PST
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.
Comment 7 User image Florian Quèze [:florian] [:flo] 2017-01-07 13:37:14 PST
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.
Comment 8 User image Florian Quèze [:florian] [:flo] 2017-01-07 13:40:09 PST
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).
Comment 9 User image Florian Quèze [:florian] [:flo] 2017-01-07 13:41:10 PST
Created attachment 8824719 [details] [diff] [review]
cleanup browser/

Automatic changes, except for browser/base/content/newtab/sites.js.
Comment 10 User image Florian Quèze [:florian] [:flo] 2017-01-07 13:42:40 PST
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
Comment 11 User image Florian Quèze [:florian] [:flo] 2017-01-07 13:43:52 PST
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.
Comment 12 User image Florian Quèze [:florian] [:flo] 2017-01-07 13:47:22 PST
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.
Comment 13 User image Florian Quèze [:florian] [:flo] 2017-01-07 13:49:28 PST
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.
Comment 14 User image Jared Wein [:jaws] (please needinfo? me) 2017-01-09 10:52:21 PST
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 User image Jared Wein [:jaws] (please needinfo? me) 2017-01-09 11:00:12 PST
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 User image Jared Wein [:jaws] (please needinfo? me) 2017-01-09 11:02:14 PST
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 User image Florian Quèze [:florian] [:flo] 2017-01-09 11:15:56 PST
(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 User image Florian Quèze [:florian] [:flo] 2017-01-09 11:28:25 PST
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 20 User image Ryan VanderMeulen [:RyanVM] 2017-01-12 17:19:50 PST
(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 User image Florian Quèze [:florian] [:flo] 2017-01-13 03:18:30 PST
(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 User image Ryan VanderMeulen [:RyanVM] 2017-01-13 05:33:42 PST
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 User image :Gijs 2017-02-08 15:09:04 PST
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 User image Florian Quèze [:florian] [:flo] 2017-02-08 15:16:27 PST
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 User image Julien Cristau [:jcristau] 2017-02-09 06:51:09 PST
Comment on attachment 8824391 [details] [diff] [review]
Patch

make future cherry-picks to 52 easier, beta52+
Comment 26 User image Ryan VanderMeulen [:RyanVM] 2017-02-09 09:28:28 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/b888fcaaa839
Comment 27 User image Ryan VanderMeulen [:RyanVM] 2017-02-14 08:22:57 PST
https://hg.mozilla.org/releases/mozilla-esr52/rev/b888fcaaa839

Note You need to log in before you can comment on or make changes to this bug.