Open Bug 1433624 Opened 6 years ago Updated 2 years ago

Use rust-url's form-urlencoded parser instead of implementing it in C++

Categories

(Core :: DOM: Core & HTML, enhancement)

Product:
Core
Component:
DOM: Core & HTML
Version:
60 Branch
Type:
enhancement

Tracking

()

Tracking Flags:
Tracking Status
firefox60 --- affected

People

(Reporter: nox, Unassigned)

Details

Attachments

(3 files)

Bug 1433624 - Move URLParams' Parse and Extract methods to MozURL;
59 bytes, text/x-review-board-request
valentin
: review+
Details
Bug 1433624 - Use rust-url's form-urlencoded parser;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1433624 - Do not convert to UTF-16 when calling ForEachFormUrlencoded::Process;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
      No description provided.
Comment on attachment 8945998 [details]
Bug 1433624 - Move URLParams' Parse and Extract methods to MozURL;

https://reviewboard.mozilla.org/r/216066/#review221858


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


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


::: netwerk/base/MozURL.cpp:162
(Diff revision 1)
> +          ASCII_HEX_DIGIT(*first) && ASCII_HEX_DIGIT(*second)) {
> +        unescaped.Append(HEX_DIGIT(first) * 16 + HEX_DIGIT(second));
> +        start = ++second;
> +        continue;
> +
> +      } else {

Warning: Do not use 'else' after 'continue' [clang-tidy: readability-else-after-return]
Comment on attachment 8945998 [details]
Bug 1433624 - Move URLParams' Parse and Extract methods to MozURL;

https://reviewboard.mozilla.org/r/216066/#review221858

> Warning: Do not use 'else' after 'continue' [clang-tidy: readability-else-after-return]

This code was only moved around and is killed in the next patch.
Comment on attachment 8945999 [details]
Bug 1433624 - Use rust-url's form-urlencoded parser;

https://reviewboard.mozilla.org/r/216068/#review222220

r=me, though I expect Valentin could have reviewed this just as well, fwiw.

::: netwerk/base/MozURL.cpp:19
(Diff revision 1)
> +  nsAutoString& mValue;
> +  mozilla::MozURL::ForEachFormUrlencoded& mProcessor;
> +};
> +
> +bool
> +CallForEach(const nsACString* aName, const nsACString* aValue, void* aContext)

This should be static, I would think.
Attachment #8945999 - Flags: review?(bzbarsky) → review+
Comment on attachment 8946000 [details]
Bug 1433624 - Do not convert to UTF-16 when calling ForEachFormUrlencoded::Process;

https://reviewboard.mozilla.org/r/216070/#review222222

It's not quite clear what the benefit of this change is.  The commit message should probably explain that...

::: caps/OriginAttributes.cpp:170
(Diff revision 1)
>  
>  class MOZ_STACK_CLASS PopulateFromSuffixIterator final
>    : public MozURL::ForEachFormUrlencoded
>  {
>  public:
> -  explicit PopulateFromSuffixIterator(OriginAttributes* aOriginAttributes)
> +  explicit PopulateFromSuffixIterator(OriginAttributes& aOriginAttributes)

Would be nice to separate mass-changes like this from substantive changed, in the future.

::: caps/OriginAttributes.cpp:179
(Diff revision 1)
>      // then it will remain >0 when it should be 0 according to the suffix. Set to 0 before
>      // iterating to fix this.
> -    mOriginAttributes->mPrivateBrowsingId = 0;
> +    mOriginAttributes.mPrivateBrowsingId = 0;
>    }
>  
> -  bool Process(const nsAString& aName, const nsAString& aValue) override
> +  bool Process(const nsACString& aName, const nsACString& aValue) override

Needs to define what charset aName/aValue are in (or document that they're just byte sequences if that's what they are).

::: dom/base/BodyUtil.cpp:53
(Diff revision 1)
>  class MOZ_STACK_CLASS FillFormIterator final
>    : public net::MozURL::ForEachFormUrlencoded
>  {
>  public:
> -  explicit FillFormIterator(FormData* aFormData)
> -    : mFormData(aFormData)
> +  explicit FillFormIterator(FormData* aFormData,
> +                            nsAutoString& aName,

You generally don't want to require callers to have an nsAutoString.  Why can't this just be nsAString&?

If you really want to require a string with an inline buffer, why does it need to be pased in by the caller?  Why can't you just make the autostrings members of this object, since they're really an implementation detail of it?

::: dom/base/BodyUtil.cpp:60
(Diff revision 1)
> +    : mFormData(aFormData), mName(aName), mValue(aValue)
>    {
>      MOZ_ASSERT(aFormData);
>    }
>  
> -  bool Process(const nsAString& aName, const nsAString& aValue) override
> +  bool Process(const nsACString& aName, const nsACString& aValue) override

Needs to document assumptions around charsets.

::: dom/url/URLSearchParams.cpp:107
(Diff revision 1)
>  class MOZ_STACK_CLASS PopulateIterator final
>    : public net::MozURL::ForEachFormUrlencoded
>  {
>  public:
> -  explicit PopulateIterator(URLParams* aParams)
> -    : mParams(aParams)
> +  explicit PopulateIterator(URLParams& aParams,
> +                            nsAutoString& aName,

Again, why do callers need to provide this?

::: netwerk/base/MozURL.h:63
(Diff revision 1)
>    nsresult GetOrigin(nsACString& aOrigin);
>  
>    class ForEachFormUrlencoded
>    {
>    public:
> -    virtual bool Process(const nsAString& aName, const nsAString& aValue) = 0;
> +    virtual bool Process(const nsACString& aName, const nsACString& aValue) = 0;

Needs to be documented wrt charsets.

::: netwerk/base/MozURL.h:69
(Diff revision 1)
>    };
>  
>    static bool ParseFormUrlencoded(const nsACString& aInput,
>                                    ForEachFormUrlencoded& aProcessor);
>  
>    static bool Extract(const nsACString& aInput,

Needs to be documented wrt charsets.  Especially since that first nsACString is a byte bag but the other two are presumably UTF-8?  In any case, needto make it very clear what they are.

::: netwerk/base/MozURL.cpp:131
(Diff revision 1)
>  
>  class MOZ_STACK_CLASS ExtractURLParam final
>    : public MozURL::ForEachFormUrlencoded
>  {
>  public:
> -  explicit ExtractURLParam(const nsAString& aName, nsAString& aValue)
> +  explicit ExtractURLParam(const nsACString& aName, nsACString& aValue)

Needs to document charsets.

::: netwerk/base/MozURL.cpp:137
(Diff revision 1)
>      : mName(aName)
>      , mValue(aValue)
>    {
>    }
>  
> -  bool Process(const nsAString& aName, const nsAString& aValue) override
> +  bool Process(const nsACString& aName, const nsACString& aValue) override

Document.

::: netwerk/base/MozURL.cpp:147
(Diff revision 1)
> -  const nsAString& mName;
> -  nsAString& mValue;
> +  const nsACString& mName;
> +  nsACString& mValue;

Document.

::: netwerk/base/MozURL.cpp:160
(Diff revision 1)
> -                const nsAString& aName,
> -                nsAString& aValue)
> +                const nsACString& aName,
> +                nsACString& aValue)

Document charsets.
Attachment #8946000 - Flags: review?(bzbarsky) → review-
Comment on attachment 8945998 [details]
Bug 1433624 - Move URLParams' Parse and Extract methods to MozURL;

https://reviewboard.mozilla.org/r/216066/#review222350
Attachment #8945998 - Flags: review?(valentin.gosu) → review+
Priority: -- → P2
Comment on attachment 8946000 [details]
Bug 1433624 - Do not convert to UTF-16 when calling ForEachFormUrlencoded::Process;

https://reviewboard.mozilla.org/r/216070/#review222378

::: caps/OriginAttributes.cpp:179
(Diff revision 1)
>      // then it will remain >0 when it should be 0 according to the suffix. Set to 0 before
>      // iterating to fix this.
> -    mOriginAttributes->mPrivateBrowsingId = 0;
> +    mOriginAttributes.mPrivateBrowsingId = 0;
>    }
>  
> -  bool Process(const nsAString& aName, const nsAString& aValue) override
> +  bool Process(const nsACString& aName, const nsACString& aValue) override

I documented the virtual method in the header.

::: dom/base/BodyUtil.cpp:53
(Diff revision 1)
>  class MOZ_STACK_CLASS FillFormIterator final
>    : public net::MozURL::ForEachFormUrlencoded
>  {
>  public:
> -  explicit FillFormIterator(FormData* aFormData)
> -    : mFormData(aFormData)
> +  explicit FillFormIterator(FormData* aFormData,
> +                            nsAutoString& aName,

It now has `nsAutoString` fields.

::: dom/base/BodyUtil.cpp:60
(Diff revision 1)
> +    : mFormData(aFormData), mName(aName), mValue(aValue)
>    {
>      MOZ_ASSERT(aFormData);
>    }
>  
> -  bool Process(const nsAString& aName, const nsAString& aValue) override
> +  bool Process(const nsACString& aName, const nsACString& aValue) override

I documented the virtual method in the header.

::: dom/url/URLSearchParams.cpp:107
(Diff revision 1)
>  class MOZ_STACK_CLASS PopulateIterator final
>    : public net::MozURL::ForEachFormUrlencoded
>  {
>  public:
> -  explicit PopulateIterator(URLParams* aParams)
> -    : mParams(aParams)
> +  explicit PopulateIterator(URLParams& aParams,
> +                            nsAutoString& aName,

Now in fields.

::: netwerk/base/MozURL.cpp:137
(Diff revision 1)
>      : mName(aName)
>      , mValue(aValue)
>    {
>    }
>  
> -  bool Process(const nsAString& aName, const nsAString& aValue) override
> +  bool Process(const nsACString& aName, const nsACString& aValue) override

Documented in the header.
Comment on attachment 8945998 [details]
Bug 1433624 - Move URLParams' Parse and Extract methods to MozURL;

https://reviewboard.mozilla.org/r/216066/#review222384


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


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


::: netwerk/base/MozURL.cpp:162
(Diff revision 2)
> +          ASCII_HEX_DIGIT(*first) && ASCII_HEX_DIGIT(*second)) {
> +        unescaped.Append(HEX_DIGIT(first) * 16 + HEX_DIGIT(second));
> +        start = ++second;
> +        continue;
> +
> +      } else {

Warning: Do not use 'else' after 'continue' [clang-tidy: readability-else-after-return]
I'd still like to see an explanation, in the commit message, of what the goal is.  We seem to be trading off one centralized conversion for a bunch of scattered ones...
Comment on attachment 8945998 [details]
Bug 1433624 - Move URLParams' Parse and Extract methods to MozURL;

https://reviewboard.mozilla.org/r/216066/#review222642


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


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


::: netwerk/base/MozURL.cpp:162
(Diff revision 3)
> +          ASCII_HEX_DIGIT(*first) && ASCII_HEX_DIGIT(*second)) {
> +        unescaped.Append(HEX_DIGIT(first) * 16 + HEX_DIGIT(second));
> +        start = ++second;
> +        continue;
> +
> +      } else {

Warning: Do not use 'else' after 'continue' [clang-tidy: readability-else-after-return]
Comment on attachment 8946000 [details]
Bug 1433624 - Do not convert to UTF-16 when calling ForEachFormUrlencoded::Process;

https://reviewboard.mozilla.org/r/216070/#review222910

Thank you for the updates.  Sorry for the lag...

::: caps/OriginAttributes.cpp:228
(Diff revision 3)
> -      mOriginAttributes->mFirstPartyDomain.Assign(aValue);
> +      NS_ConvertUTF8toUTF16 value(aValue);
> +      mOriginAttributes.mFirstPartyDomain.Assign(value);

CopyUTF8toUTF16(aValue, mOriginAttributes.mFirstPartyDomain);
    
will avoid creating extra copies.

::: netwerk/base/MozURL.h:75
(Diff revision 3)
>    };
>  
>    static bool ParseFormUrlencoded(const nsACString& aInput,
>                                    ForEachFormUrlencoded& aProcessor);
>  
>    static bool Extract(const nsACString& aInput,

This still needs documentation about the charsets of aInput, aName, aValue.

::: netwerk/base/MozURL.cpp:123
(Diff revision 3)
>  }
>  
>  /* static */ bool
> +/**
> + * Parses a form-urlencoded blob of bytes.
> + * @param aInput The input to parse, lossly decoded as UTF-8.

"lossly"?

It's not clear to me whether the input _is_ already "lossily decoded", or whether the callee will do that.  Please make that clear.

::: netwerk/base/MozURL.cpp:136
(Diff revision 3)
>  
>  class MOZ_STACK_CLASS ExtractURLParam final
>    : public MozURL::ForEachFormUrlencoded
>  {
>  public:
> -  explicit ExtractURLParam(const nsAString& aName, nsAString& aValue)
> +  explicit ExtractURLParam(const nsACString& aName, nsACString& aValue)

Need to document encodings of these arguments...
Attachment #8946000 - Flags: review?(bzbarsky) → review+
Comment on attachment 8945998 [details]
Bug 1433624 - Move URLParams' Parse and Extract methods to MozURL;

https://reviewboard.mozilla.org/r/216066/#review223500


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


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


::: netwerk/base/MozURL.cpp:162
(Diff revision 4)
> +          ASCII_HEX_DIGIT(*first) && ASCII_HEX_DIGIT(*second)) {
> +        unescaped.Append(HEX_DIGIT(first) * 16 + HEX_DIGIT(second));
> +        start = ++second;
> +        continue;
> +
> +      } else {

Warning: Do not use 'else' after 'continue' [clang-tidy: readability-else-after-return]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has priority 'P2'.
:hsinyi, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: nox → nobody
Flags: needinfo?(htsai)
Flags: needinfo?(htsai)
Priority: P2 → --
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: