1432187 - Add new nsIStandardURLMutator interface

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1432187 - Add nsIStandardURLMutator interface

Review commit: https://reviewboard.mozilla.org/r/214674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214674/

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1432187 - Change code to use nsIStandardURLMutator.{init,setDefaultPort}

Review commit: https://reviewboard.mozilla.org/r/214676/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214676/

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1432187 - Add missing SubstitutingURL::Mutator

* Makes the implementation of nsStandardURL::Mutator into a template called TemplatedMutator<T>
* Makes both nsStandardURL::Mutator and SubstitutingURL::Mutator extend TemplatedMutator<T>

Review commit: https://reviewboard.mozilla.org/r/214678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214678/

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8944483 [details]
Bug 1432187 - Change code to use nsIStandardURLMutator.{init,setDefaultPort}

https://reviewboard.mozilla.org/r/214676/#review220314


Static analysis found 2 defects in this patch.
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: netwerk/test/unit/test_large_port.js:14
(Diff revision 1)
> -const StandardURL = Components.Constructor("@mozilla.org/network/standard-url;1",
> -                                           "nsIStandardURL",
> -                                           "init");
>  function run_test()
>  {
> +    let mutator = Cc["@mozilla.org/network/standard-url-mutator;1"]

Error: 'mutator' is assigned a value but never used. Allowed unused vars must match /^Cc|Ci|Cu|Cr|EXPORTED_SYMBOLS/. [eslint: no-unused-vars]

::: netwerk/test/unit/test_large_port.js:18
(Diff revision 1)
>  {
> +    let mutator = Cc["@mozilla.org/network/standard-url-mutator;1"]
> +                    .createInstance(Ci.nsIURIMutator);
>      // Bug 1301621 makes invalid ports throw
>      Assert.throws(() => {
> -        new StandardURL(Ci.nsIStandardURL.URLTYPE_AUTHORITY, 65536,
> +        let url = Cc["@mozilla.org/network/standard-url-mutator;1"]

Error: 'url' is assigned a value but never used. Allowed unused vars must match /^Cc|Ci|Cu|Cr|EXPORTED_SYMBOLS/. [eslint: no-unused-vars]

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8944482 [details]
Bug 1432187 - Add nsIStandardURLMutator interface

https://reviewboard.mozilla.org/r/214674/#review220894

::: netwerk/base/nsIStandardURL.idl:9
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsIMutable.idl"
>  
>  interface nsIURI;
> +interface nsIURIMutator;

do you really need this?

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8944483 [details]
Bug 1432187 - Change code to use nsIStandardURLMutator.{init,setDefaultPort}

https://reviewboard.mozilla.org/r/214676/#review220896

::: dom/url/URLWorker.cpp:659
(Diff revision 1)
> +                                          NS_ConvertUTF16toUTF8(aURL),
> +                                          nullptr, baseURL, nullptr)
> +            .Finalize(uri);
> +    aRv = rv;
> +    if (NS_SUCCEEDED(rv)) {
> +      mStdURL = static_cast<nsStandardURL*>(uri.get());

it would be so nice if the Finalize() method returned nsStandardURL*, I really strongly dislike any static_cast these patches has to add..

but if we want to think of anything better, let's do it in followups

::: netwerk/protocol/file/nsFileProtocolHandler.cpp:172
(Diff revision 1)
>                                nsIURI *baseURI,
>                                nsIURI **result)
>  {
> -    nsCOMPtr<nsIStandardURL> url = new nsStandardURL(true);
> +    nsCOMPtr<nsIURI> url = new nsStandardURL(true);
>      if (!url)
>          return NS_ERROR_OUT_OF_MEMORY;

can be removed

::: netwerk/protocol/file/nsFileProtocolHandler.cpp:187
(Diff revision 1)
> -    nsresult rv = url->Init(nsIStandardURL::URLTYPE_NO_AUTHORITY, -1,
> -                            *specPtr, charset, baseURI);
> -    if (NS_FAILED(rv)) return rv;
> -
> -    return CallQueryInterface(url, result);
> +    return NS_MutateURI(url)
> +             .Apply<nsIStandardURLMutator>(&nsIStandardURLMutator::Init,
> +                                           nsIStandardURL::URLTYPE_NO_AUTHORITY, -1,
> +                                           buf, charset, baseURI,
> +                                           nullptr)
> +             .Finalize(result);

any reason why you are mutating existing URL instance instead of letting NS_MutateURI create it only once?

::: netwerk/protocol/ftp/nsFtpProtocolHandler.cpp:180
(Diff revision 1)
> -    nsCOMPtr<nsIStandardURL> url = do_CreateInstance(NS_STANDARDURL_CONTRACTID, &rv);
> -    if (NS_FAILED(rv)) return rv;
> -
> -    rv = url->Init(nsIStandardURL::URLTYPE_AUTHORITY, 21, aSpec, aCharset, aBaseURI);
> -    if (NS_FAILED(rv)) return rv;
> -
> +    return NS_MutateURI(NS_STANDARDURLMUTATOR_CONTRACTID)
> +             .Apply<nsIStandardURLMutator>(&nsIStandardURLMutator::Init,
> +                                           nsIStandardURL::URLTYPE_AUTHORITY, 21,
> +                                           nsCString(aSpec), aCharset, aBaseURI,
> +                                           nullptr)
> +             .Finalize(result);

here you do it..

::: netwerk/test/unit/test_large_port.js:28
(Diff revision 1)
> +                    .finalize();
>      }, "invalid port during creation");
> -    let url = new StandardURL(Ci.nsIStandardURL.URLTYPE_AUTHORITY, 65535,
> +
> +    let url = Cc["@mozilla.org/network/standard-url-mutator;1"]
> +                    .createInstance(Ci.nsIURIMutator)
> +                    .QueryInterface(Ci.nsIStandardURLMutator)

nit: I think you can just :

.createInstance(Ci.nsIStandardURLMutator)

::: netwerk/test/unit/test_standardurl_default_port.js
(Diff revision 1)
>  
>    // Create a nsStandardURL:
>    var origUrlStr = "http://foo.com/";
>    var stdUrl = stringToURL(origUrlStr);
> -  var stdUrlAsUri = stdUrl.QueryInterface(Ci.nsIURI);
> +  Assert.equal(-1, stdUrl.port);
> -  Assert.equal(-1, stdUrlAsUri.port);

hmm.. why is the QI no longer needed here?  or wasn't it ever needed?

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8944484 [details]
Bug 1432187 - Add missing SubstitutingURL::Mutator

https://reviewboard.mozilla.org/r/214678/#review220930

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8944483 [details]
Bug 1432187 - Change code to use nsIStandardURLMutator.{init,setDefaultPort}

https://reviewboard.mozilla.org/r/214676/#review220896

> any reason why you are mutating existing URL instance instead of letting NS_MutateURI create it only once?

Yes. The URL that we create has to have mSupportsFileURL = true.
I will try to fix this in a followup patch.

> hmm.. why is the QI no longer needed here?  or wasn't it ever needed?

It was never needed.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8944482 [details]
Bug 1432187 - Add nsIStandardURLMutator interface

https://reviewboard.mozilla.org/r/214674/#review220894

> do you really need this?

Yes. The mutator methods return nsIURIMutator to allow chaining of methods.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8944482 [details]
Bug 1432187 - Add nsIStandardURLMutator interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214674/diff/1-2/

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8944483 [details]
Bug 1432187 - Change code to use nsIStandardURLMutator.{init,setDefaultPort}

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214676/diff/1-2/

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8944484 [details]
Bug 1432187 - Add missing SubstitutingURL::Mutator

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214678/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c575454e11e
Add nsIStandardURLMutator interface r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/42c5d8b64356
Change code to use nsIStandardURLMutator.{init,setDefaultPort} r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/04b754324774
Add missing SubstitutingURL::Mutator r=mayhemer

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/2c575454e11e
https://hg.mozilla.org/mozilla-central/rev/42c5d8b64356
https://hg.mozilla.org/mozilla-central/rev/04b754324774