Remove nsIURI::originCharset

RESOLVED FIXED in Firefox 57

Status

()

Core
Networking
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: hsivonen, Assigned: emk)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 affected, firefox57 fixed)

Details

(Whiteboard: [necko-next])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

It seems that nsIURI::originCharset has two use cases:
 1) Dealing with the spec-incompliant feature of escapes in the hash (reference) part of the URL.
 2) For UI display of non-UTF-8 URLs.

If per bug 817374 comment 18 we no longer care to have the complexity of having pretty display of query strings on legacy-encoded pages and if our hash part handling is reaching spec compliance, it seems we could remove nsIURI::originCharset.
(Assignee)

Comment 1

2 years ago
If I read the source correctly, we already stop decoding the hash part.

GettersDecodeURLHash() will return true only if the "dom.url.getters_decode_hash" is true.[1][2]
"dom.url.getters_decode_hash" is false by default[3] and we have no UI to flip this pref (except about:config).

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/nsContentUtils.h#2079-2082
[2] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/nsContentUtils.cpp#586-587
[3] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/modules/libpref/init/all.js#220
(Assignee)

Comment 2

2 years ago
And I would like to keep the first parameter until we drop support for XUL/XPCOM addons.
(Assignee)

Comment 3

2 years ago
Oh, I didn't read the bug title correctly.

Comment #2 was about the first parameter of unEscapeURIForUI.
Valentin, I seem to remember you having some hands in this. Can you take a look and re-triage or work on as appropriate? Thanks.
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-next]
(Assignee)

Updated

a year ago
Blocks: 1347507
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
Comment on attachment 8893231 [details]
Bug 1322874 - Get rid of nsIURI.originCharset. .gosu

https://reviewboard.mozilla.org/r/164270/#review169736

Just a few minor nits. 
r=me Thanks!

::: commit-message-4c731:3
(Diff revision 1)
> +Bug 1322874 - Get rid of nsIURI.originCharset. r?valentin.gosu
> +
> +MozReview-Commit-ID: 3tHd0VCWSqF

Please add a detailed description message:
* what originCharset was used for
* why we don't need it anymore

::: commit-message-4c731:3
(Diff revision 1)
> +Bug 1322874 - Get rid of nsIURI.originCharset. r?valentin.gosu
> +
> +MozReview-Commit-ID: 3tHd0VCWSqF

Please add a detailed description message:
* what originCharset was used for
* why we don't need it anymore

::: netwerk/base/nsStandardURL.cpp:3597
(Diff revision 1)
>      if (NS_FAILED(rv)) return rv;
>  
>      rv = ReadSegment(stream, mRef);
>      if (NS_FAILED(rv)) return rv;
>  
> -    rv = NS_ReadOptionalCString(stream, mOriginCharset);
> +    nsAutoCString old_originCharset;

nit: let's not mix camelCase with snake_case :)
Attachment #8893231 - Flags: review?(valentin.gosu) → review+
Comment hidden (mozreview-request)

Comment 9

a year ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/8ff32bdb356d
Get rid of nsIURI.originCharset. r=valentin.gosu
Flags: needinfo?(valentin.gosu)

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8ff32bdb356d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

10 months ago
Depends on: 1405696
You need to log in before you can comment on or make changes to this bug.