1431449 - Some URLParams values are created only to be thrown away afterwards

Status: RESOLVED FIXED

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

Description

[Anthony Ramine [:nox]]

In many call sites, URLParams values are created only to call URLParams::ParseInput and traverse the result with URLParams::ForEach afterwards. This could be avoided by invoking the URLParams::ForEachIterator callback directly when parsing.

Comment 1

[Anthony Ramine [:nox]]

Attachment: Bug 1431449 - Kill URLSearchParams::ForEach;

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

Comment 2

[Anthony Ramine [:nox]]

Attachment: Bug 1431449 - Introduce URLParams::Parse;

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

Comment 3

[Anthony Ramine [:nox]]

Attachment: Bug 1431449 - Make URLParams::DecodeString static;

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

Comment 4

[Anthony Ramine [:nox]]

Attachment: Bug 1431449 - Move nsTString::ToInteger* methods to nsTSubstring;

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

Comment 5

[Anthony Ramine [:nox]]

Attachment: Bug 1431449 - Make URLParams::ParseInput call URLParams::Parse instead of the opposite;

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8943679 [details]
Bug 1431449 - Kill URLSearchParams::ForEach;

https://reviewboard.mozilla.org/r/214036/#review219824

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8943680 [details]
Bug 1431449 - Introduce URLParams::Parse;

https://reviewboard.mozilla.org/r/214038/#review219828

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8943681 [details]
Bug 1431449 - Make URLParams::DecodeString static;

https://reviewboard.mozilla.org/r/214040/#review219850

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8943682 [details]
Bug 1431449 - Move nsTString::ToInteger* methods to nsTSubstring;

https://reviewboard.mozilla.org/r/214042/#review219852

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8943683 [details]
Bug 1431449 - Make URLParams::ParseInput call URLParams::Parse instead of the opposite;

https://reviewboard.mozilla.org/r/214044/#review219856

r=me, though maybe it would be OK to leave the ForEach; might come in handy.

Also, we're basically replacing object construction with more virtual calls in the Parse/ParseInput case.  That's probably a win just due to less allocation, but it's not obvious....  Might be worth having the commit message explain why this is better, since the bug doesn't really.

Comment 11

[Anthony Ramine [:nox]]

I'm not sure what introduces more virtual calls, could you explain?

Comment 12

[Boris Zbarsky [:bzbarsky]]

In the cases that did ParseInput (not Parse), we now have a virtual call for each name/value pair, via aIterator.URLParamsIterator, that didn't use to be there, right?

Comment 13

[Anthony Ramine [:nox]]

Oh yes, you are absolutely right. I'll add a note in the commit message, given how fast I forgot about it.

Comment 14

[Anthony Ramine [:nox]]

Comment on attachment 8943683 [details]
Bug 1431449 - Make URLParams::ParseInput call URLParams::Parse instead of the opposite;

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

Comment 15

[Anthony Ramine [:nox]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c834e5f789da2b7070d4ce00bce63a913432bd7

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b802a492170
Kill URLSearchParams::ForEach; r=bz
https://hg.mozilla.org/integration/autoland/rev/ddb98ad66aa0
Introduce URLParams::Parse; r=bz
https://hg.mozilla.org/integration/autoland/rev/d86e47eb88db
Make URLParams::DecodeString static; r=bz
https://hg.mozilla.org/integration/autoland/rev/ecc091d9860d
Move nsTString::ToInteger* methods to nsTSubstring; r=bz
https://hg.mozilla.org/integration/autoland/rev/c1b5b46f8558
Make URLParams::ParseInput call URLParams::Parse instead of the opposite; r=bz

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/3b802a492170
https://hg.mozilla.org/mozilla-central/rev/ddb98ad66aa0
https://hg.mozilla.org/mozilla-central/rev/d86e47eb88db
https://hg.mozilla.org/mozilla-central/rev/ecc091d9860d
https://hg.mozilla.org/mozilla-central/rev/c1b5b46f8558