Closed Bug 1432187 Opened 6 years ago Closed 6 years ago

Add new nsIStandardURLMutator interface

Categories

(Core :: Networking, enhancement)

Product:
Core
Component:
Networking
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(3 files)

Bug 1432187 - Add nsIStandardURLMutator interface
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1432187 - Change code to use nsIStandardURLMutator.{init,setDefaultPort}
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1432187 - Add missing SubstitutingURL::Mutator
59 bytes, text/x-review-board-request
mayhemer
: review+
Details 
      No description provided.
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]
Blocks: 1432257
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?
Attachment #8944482 - Flags: review?(honzab.moz) → review+
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?
Attachment #8944483 - Flags: review?(honzab.moz) → review+
Comment on attachment 8944484 [details]
Bug 1432187 - Add missing SubstitutingURL::Mutator

https://reviewboard.mozilla.org/r/214678/#review220930
Attachment #8944484 - Flags: review?(honzab.moz) → review+
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 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.
Pushed by valentin.gosu@gmail.com:
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
Blocks: 1432928
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: