1939658 - Replace new URL usage with URL.parse/URL.canParse where possible

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[kernp25]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:133.0) Gecko/20100101 Firefox/133.0

Actual results:

Instead of using try { new URL(...) } catch (ex) {} just use URL.parse/URL.canParse.

Comment 1

[kernp25]

Attachment: Bug 1939658 - Replace new URL usage with URL.parse/URL.canParse where possible. r?robwu,Standard8,Gijs,mtigley,pdahiya,mak,dwalker,adw,ochameau

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c11770a86dc
Replace new URL usage with URL.parse/URL.canParse where possible. r=robwu,Standard8,Gijs,mtigley,pdahiya,settings-reviewers,credential-management-reviewers,devtools-reviewers,tabbrowser-reviewers,places-reviewers,omc-reviewers,migration-reviewers,firefox-desktop-core-reviewers ,home-newtab-reviewers,webcompat-reviewers,urlbar-reviewers,twisniewski,mossop,dao,nchevobbe,webdriver-reviewers,whimboo,issammani,mconley,nbarrett,beth

Comment 4

[Atila Butkovits]

Backed out for causing failures at test_JsonSchemaValidator.js.

Backout link: https://hg.mozilla.org/integration/autoland/rev/67af6fcfd17c320191d5424212161d520a6584f2

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=2c11770a86dc84a158d2b41a2b1aeea75df7baea&selectedTaskRun=DkGNUFp-QAO4YHtIY3e_qQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=493181619&repo=autoland&lineNumber=4740

Comment 5

[kernp25]

Do you know why this test is failing? The test must be changed? It says parsedValue - null === "".

Comment 6

[Mark Banner (:standard8)]

(In reply to kernp25 from comment #5)

Do you know why this test is failing? The test must be changed? It says parsedValue - null === "".

At the start of the function, there's this line: let parsedParam = param;. In this case, I think it is setting parsedParam to the empty string. Your patch changes the code from try { parsedParam = new URI(param) } catch ... to parsedParam = URL.parse(param);.

Due to that change, if the param cannot be parsed, we're now returning null rather than "".

Looking at this documentation it might be acceptable to change the return type there - the result isn't valid, so we shouldn't be looking at the parsedParam anyway.

However, I don't know the code well enough here to know if that's acceptable, so forwarding the NI to :dmose who hopefully will have an eye..

Comment 7

[kernp25]

Should I change the parsedParam = URL.parse(param); line to parsedParam = URL.parse(param) ?? "";?

Comment 8

[kernp25]

Or better, I should reset the parsedParam variable to parsedParam = param; here?

Comment 9

[Mark Banner (:standard8)]

The options here are why I'm asking :dmose. I don't know enough to know if it is safe to change what we return (and hence we can update the test) or change the code to match the test. Whilst we could change the code rather than the test, that is then extra code that we'll need to explain.

Comment 10

[kernp25]

Can you now push again? The other case will be handled in bug 1946723.

Comment 11

[Mark Banner (:standard8)]

(In reply to kernp25 from comment #10)

Can you now push again? The other case will be handled in bug 1946723.

Thank you, that's a good way of handling this, I've just trigged landing again.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d3dea2426af
Replace new URL usage with URL.parse/URL.canParse where possible. r=robwu,Standard8,Gijs,mtigley,pdahiya,settings-reviewers,credential-management-reviewers,devtools-reviewers,tabbrowser-reviewers,places-reviewers,omc-reviewers,migration-reviewers,firefox-desktop-core-reviewers ,home-newtab-reviewers,webcompat-reviewers,urlbar-reviewers,twisniewski,mossop,dao,nchevobbe,webdriver-reviewers,whimboo,issammani,mconley,nbarrett,beth

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/8d3dea2426af