Closed Bug 1426501 Opened 6 years ago Closed 6 years ago

Change code that uses nsIURI.spec setter to use nsIURIMutator

Categories

(Core :: Networking, enhancement, P2)

Product:
Core
Component:
Networking
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(4 files)

Bug 1426501 - Add nsINetUtil.notImplemented() method that always throws
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1426501 - Change nsIURIMutator to call set spec on cloned URI if available
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1426501 - Change JS code to use nsIURIMutator instead of the nsIURI.spec setter
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1426501 - Change C++ code to use NS_MutateURI when changing URI
59 bytes, text/x-review-board-request
mayhemer
: review+
Details 
      No description provided.
Summary: Change code that uses nsIURI.setSpec to use nsIURIMutator → Change code that uses nsIURI.spec setter to use nsIURIMutator
Attachment #8938171 - Flags: review?(honzab.moz)
Attachment #8938172 - Flags: review?(honzab.moz)
Attachment #8938173 - Flags: review?(honzab.moz)
Attachment #8938174 - Flags: review?(honzab.moz)
Comment on attachment 8938171 [details]
Bug 1426501 - Add nsINetUtil.notImplemented() method that always throws

https://reviewboard.mozilla.org/r/208890/#review215398
Attachment #8938171 - Flags: review?(honzab.moz) → review+
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?
Attachment #8938172 - Flags: review?(honzab.moz) → review+
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?
Attachment #8938173 - Flags: review?(honzab.moz) → review+
Comment on attachment 8938174 [details]
Bug 1426501 - Change C++ code to use NS_MutateURI when changing URI

https://reviewboard.mozilla.org/r/208896/#review215434
Attachment #8938174 - Flags: review?(honzab.moz) → review+
Priority: -- → P2
(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
Depends on: 1420714
No longer depends on: 1420714
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: