Comment on attachment 8938171 [details] Bug 1426501 - Add nsINetUtil.notImplemented() method that always throws https://reviewboard.mozilla.org/r/208890/#review215398

Comment on attachment 8938172 [details] Bug 1426501 - Change nsIURIMutator to call set spec on cloned URI if available https://reviewboard.mozilla.org/r/208892/#review215402 ::: netwerk/base/nsIURIMutator.idl:81 (Diff revision 1) > + } > + > + rv = uri->SetSpec(aSpec); > if (NS_FAILED(rv)) { > return rv; > } in other words, when SetSpec fails, we end up with mURI being null regardless it was initially null or not, right?

Comment on attachment 8938173 [details] Bug 1426501 - Change JS code to use nsIURIMutator instead of the nsIURI.spec setter https://reviewboard.mozilla.org/r/208894/#review215404 r+ with comments fixed (some are critical) ::: chrome/test/unit/test_no_remote_registration.js:28 (Diff revision 2) > defaultPort: -1, > allowPort: () => false, > newURI(aSpec, aCharset, aBaseURI) { > - let uri = Cc["@mozilla.org/network/standard-url;1"]. > - createInstance(Ci.nsIURI); > - uri.spec = aSpec; > + let mutator = Cc["@mozilla.org/network/standard-url-mutator;1"] > + .createInstance(Ci.nsIURIMutator); > + if (aBaseURI) { the original condition was a bit different, are sure this is right? ::: dom/base/test/chrome/test_bug682305.html:123 (Diff revision 2) > allowPort: function allowPort() { > return false; > }, > newURI: function newURI(spec, charset, baseURI) { > - var uri = SimpleURI.createInstance(Ci.nsIURI) > - uri.spec = spec; > + return Cc["@mozilla.org/network/simple-uri-mutator;1"] > + .createInstance(Ci.nsIURIMutator) nit: you could have Cc (a constructor) for the mutator here, but it's cosmetic only. ::: dom/xslt/tests/XSLTMark/XSLTMark-static.js:12 (Diff revision 2) > const IOSERVICE_CTRID = "@mozilla.org/network/io-service;1"; > const nsIIOService = Components.interfaces.nsIIOService; > const SIS_CTRID = "@mozilla.org/scriptableinputstream;1"; > const nsISIS = Components.interfaces.nsIScriptableInputStream; > const nsIFilePicker = Components.interfaces.nsIFilePicker; > -const STDURL_CTRID = "@mozilla.org/network/standard-url;1"; > +const STDURL_MUTID = "@mozilla.org/network/standard-url-mutator;1"; to be consistent, this should probably be STDURLMUT_CTRID (yeah, I don't like this kind of indention too...) ::: netwerk/test/unit/test_bug894586.js:18 (Diff revision 2) > function ProtocolHandler() { > - this.uri = Cc["@mozilla.org/network/simple-uri;1"]. > - createInstance(Ci.nsIURI); > - this.uri.spec = this.scheme + ":dummy"; > + this.uri = Cc["@mozilla.org/network/simple-uri-mutator;1"] > + .createInstance(Ci.nsIURIMutator) > + .setSpec(this.scheme + ":dummy") > + .finalize(); > this.uri.QueryInterface(Ci.nsIMutable).mutable = false; still needed? ::: widget/tests/unit/test_taskbar_jumplistitems.js:61 (Diff revision 2) > > function test_hashes() > { > var link = Cc["@mozilla.org/windows-jumplistlink;1"] > .createInstance(Ci.nsIJumpListLink); > - var uri1 = Cc["@mozilla.org/network/simple-uri;1"] > + var uri1 = Cc["@mozilla.org/network/simple-uri-mutate;1"] mutatOR ? ::: widget/tests/unit/test_taskbar_jumplistitems.js:65 (Diff revision 2) > .createInstance(Ci.nsIJumpListLink); > - var uri1 = Cc["@mozilla.org/network/simple-uri;1"] > - .createInstance(Ci.nsIURI); > + var uri1 = Cc["@mozilla.org/network/simple-uri-mutate;1"] > + .createInstance(Ci.nsIURIMutator) > + .setSpec("http://www.123.com/") > + .finalize(); > var uri2 = Cc["@mozilla.org/network/simple-uri;1"] simple-uri-mutator;1?

Comment on attachment 8938174 [details] Bug 1426501 - Change C++ code to use NS_MutateURI when changing URI https://reviewboard.mozilla.org/r/208896/#review215434

(In reply to Honza Bambas (:mayhemer) from comment #10) > Comment on attachment 8938172 [details] > in other words, when SetSpec fails, we end up with mURI being null > regardless it was initially null or not, right? That is correct.

(In reply to Honza Bambas (:mayhemer) from comment #11) > Comment on attachment 8938173 [details] > > + if (aBaseURI) { > > the original condition was a bit different, are sure this is right? > > this.uri.QueryInterface(Ci.nsIMutable).mutable = false; > > still needed? I intend to change it in a follow-up patch. > ::: widget/tests/unit/test_taskbar_jumplistitems.js:61 > > + var uri1 = Cc["@mozilla.org/network/simple-uri-mutate;1"] > mutatOR ? Good catch! > ::: widget/tests/unit/test_taskbar_jumplistitems.js:65 > > var uri2 = Cc["@mozilla.org/network/simple-uri;1"] > simple-uri-mutator;1? Yes. Missed that one.

(In reply to Valentin Gosu [:valentin] (PTO until Jan 8) from comment #14) > (In reply to Honza Bambas (:mayhemer) from comment #11) > > Comment on attachment 8938173 [details] > > > + if (aBaseURI) { > > > > the original condition was a bit different, are sure this is right? Yes, this is the common behaviour for newURI. I don't know why it was originally written like that.

Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a1e8bf62f2da Add nsINetUtil.notImplemented() method that always throws r=mayhemer https://hg.mozilla.org/integration/autoland/rev/a3baf4893b8d Change nsIURIMutator to call set spec on cloned URI if available r=mayhemer https://hg.mozilla.org/integration/autoland/rev/ad96d6e27750 Change JS code to use nsIURIMutator instead of the nsIURI.spec setter r=mayhemer https://hg.mozilla.org/integration/autoland/rev/0ca777d3a396 Change C++ code to use NS_MutateURI when changing URI r=mayhemer

https://hg.mozilla.org/mozilla-central/rev/a1e8bf62f2da https://hg.mozilla.org/mozilla-central/rev/a3baf4893b8d https://hg.mozilla.org/mozilla-central/rev/ad96d6e27750 https://hg.mozilla.org/mozilla-central/rev/0ca777d3a396