Closed
Bug 1329182
Opened 7 years ago
Closed 7 years ago
Make newURI's last two parameters optional
Categories
(Core :: Networking, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: florian, Assigned: florian)
References
Details
Attachments
(9 files, 1 obsolete file)
2.41 KB,
patch
|
mcmanus
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
3.70 KB,
patch
|
Details | Diff | Splinter Review | |
3.89 KB,
text/plain
|
Details | |
3.41 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
109.74 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
98.31 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
33.42 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
292.73 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
1.29 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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•7 years ago
|
Assignee: nobody → florian
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47ff31f5d83e
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0258dab76e85
Assignee | ||
Comment 6•7 years ago
|
||
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•7 years ago
|
Attachment #8824424 -
Attachment is obsolete: true
Assignee | ||
Comment 7•7 years ago
|
||
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•7 years ago
|
Attachment #8824717 -
Attachment mime type: application/binary → text/plain
Assignee | ||
Comment 8•7 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•7 years ago
|
||
Automatic changes, except for browser/base/content/newtab/sites.js.
Attachment #8824719 -
Flags: review?(jaws)
Assignee | ||
Comment 10•7 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•7 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•7 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•7 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)
Updated•7 years ago
|
Attachment #8824718 -
Flags: review?(jaws) → review+
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
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 16•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8824722 -
Flags: review?(jaws) → review+
Updated•7 years ago
|
Attachment #8824723 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 17•7 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.
Assignee | ||
Comment 18•7 years 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•7 years 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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 20•7 years ago
|
||
(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•7 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)
Comment 22•7 years ago
|
||
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•6 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•6 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 25•6 years ago
|
||
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•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/b888fcaaa839
status-firefox52:
--- → fixed
Comment 27•6 years ago
|
||
bugherder uplift |
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.
Description
•