Closed Bug 1673682 Opened 1 year ago Closed 1 year ago

Move URLParams to a more general place

Categories

(Core :: DOM: Core & HTML, task, P3)

task

Tracking

()

RESOLVED FIXED
84 Branch
Tracking Status
firefox84 --- fixed

People

(Reporter: sg, Assigned: sg)

Details

Attachments

(9 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

URLParams doesn't have to be in dom/url, but could be moved to a more general place, perhaps mod. ReadStructuredClone/WriteStructuredClone, so that it can be used at more places without introducing extra dependencies.

When doing this, the Parse could also be changed to use a functor rather than a virtual method for the iterator.

DOM:Networking is for XHR and Fetch stuff.
DOM: Core & HTML seems to be a better component.

Component: DOM: Networking → DOM: Core & HTML

(In reply to Simon Giesecke [:sg] [he/him] from comment #0)

URLParams doesn't have to be in dom/url, but could be moved to a more general place, perhaps mod.

mod? Which folder do you mean?

Flags: needinfo?(sgiesecke)

I meant that the target situation might be that a base class of URLParams is placed at a more general place, and a subclass of that remains in dom/url which contains the ReadStructuredClone/WriteStructuredClone member functions.

I am not sure where exactly the base class could be placed yet. At first sight, it depends on nothing but XPCOM, so it could be somewhere in XPCOM, e.g.

Flags: needinfo?(sgiesecke)
Severity: -- → N/A
Priority: -- → P3

It's closely related to the functions in netwerk/base/nsURLHelper.{h,cpp}, so I will provide a patch to move it there.

Move ReadStructuredClone/WriteStructuredClone to URLSearchParams.

Adapt all uses of URLParams.

Depends on D96415

Assignee: nobody → sgiesecke
Status: NEW → ASSIGNED
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a258e2c1f86d
Move URLParams to nsURLHelper.h. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/b80bc0d6a60f
Don't copy substrings in URLParams::Parse. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/c1bcc2a715c9
Use StableSort in URLParams::Sort. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/82bcc3804b87
Remove unused nsresult return value from URLParams::Sort. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/11117a58e809
Accept functors rather than a virtual function override in URLParams::Parse. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/5d3793aeba07
Use std::any_of/find_if in URLParams::Has/Get. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/94d647472989
Remove uses of deprecated reading iterators in URLParams::DecodeStringURLParams::DecodeString. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/178b89edf803
Add documentation to several URLParam member functions. r=valentin,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/bf3fb5a5c418
Remove redundant URLParam ctor/dtor. r=valentin,necko-reviewers
Pushed by abutkovits@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a82edf73587
Add missing include. r=fix. CLOSED TREE

This seems like a lot to land during a soft freeze. What's the risk of regressions from this as 84 goes to Beta in the next couple days?

Flags: needinfo?(sgiesecke)

Sorry, copying from our Slack discussion: The first patch is purely mechanical, as it moves the type to different files. Most of the other patches simplify the implementations of some methods, which might be consider a somewhat higher risk. Then there's one patch that changes the API, but in a rather mechanical way, to use functors rather than a base class with virtual method overrides.

I now was able to check the code coverage (https://coverage.moz.tools/#revision=latest&path=netwerk%2Fbase%2FnsURLHelper.cpp&view=file) and all functions are well-covered by tests, so I don't think this is a significant risk overall.

Flags: needinfo?(sgiesecke)
You need to log in before you can comment on or make changes to this bug.