Make newURI's last two parameters optional

RESOLVED FIXED in Firefox 52

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

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

Firefox Tracking Flags

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

Details

Attachments

(9 attachments, 1 obsolete attachment)

Assignee

Description

2 years 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

2 years ago
Posted patch PatchSplinter Review
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

2 years ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
Assignee

Comment 2

2 years ago
Posted patch Test (obsolete) — Splinter Review
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 6

2 years ago
Posted patch Test v2Splinter Review
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

2 years ago
Attachment #8824424 - Attachment is obsolete: true
Assignee

Comment 7

2 years ago
Posted file 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

2 years ago
Attachment #8824717 - Attachment mime type: application/binary → text/plain
Assignee

Comment 8

2 years ago
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

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

Comment 10

2 years ago
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

2 years ago
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

2 years ago
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

2 years ago
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)
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+
Assignee

Comment 17

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

2 years 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 years ago
Depends on: 1334261

Comment 23

2 years 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 years 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+
You need to log in before you can comment on or make changes to this bug.