1424468 - Stop using Encoding::ForName in HTML parser

Status: RESOLVED FIXED

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

Description

[Masatoshi Kimura [:emk]]

This is the last consumer of Encoding::ForName in m-c/c-c.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Bug 1424468 - Stop using Encoding::ForName in HTML parser.

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

Comment 2

[Henri Sivonen (:hsivonen)]

Comment on attachment 8935994 [details]
Bug 1424468 - Stop using Encoding::ForName in HTML parser.

https://reviewboard.mozilla.org/r/206844/#review212658

Thank you and hooray for making `Encoding::ForName` obsolete! This looks correct to me, but it's unclear to me if using nsString in a union is something that we are supposed to allow in the codebase, so requesting review from the owner of XPCOM strings, too.

Comment 3

[Henri Sivonen (:hsivonen)]

Comment on attachment 8935994 [details]
Bug 1424468 - Stop using Encoding::ForName in HTML parser.

Somehow the additional review request wasn't reflected to Bugzilla.

Comment 4

[Masatoshi Kimura [:emk]]

FYI, non-trivial class in union is a new feature since C++11.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think I'd pass that question on to Nathan, since I think it's the general question stated in comment 4.

(I'm also curious how it works under the hood, and what the performance characteristics are.)

Comment 6

[Nathan Froyd [:froydnj]]

Non-trivial classes in unions work more-or-less like you expect them to: you need to manually construct them with placement new when they become active and manually destruct them when they become inactive.  There should be virtually no performance difference.

I wonder if we shouldn't be using mozilla::Variant to represent this, since that's the type-safe way to do unions in our codebase.  Ideally, we'd tie it to the value of mOpCode as well, so we couldn't have the values of the union being out-of-sync with the value of mOpCode (or vice versa), but maybe that'd be too big of a project to tackle here.  It'd be larger, and maybe slightly slower, but perhaps that'd be OK for this code?  It looks like we'd have to use Maybe<Variant<const Encoding*, nsString>>, though, because the union isn't really initialized from the moment the object is constructed?  Is that correct?

Comment 7

[Masatoshi Kimura [:emk]]

Variant<const Encoding*, nsString> will not take less space than const Encoding* + nsString.
I'd just add a `const Encoding*` member to nsHtml5SpeculativeLoad if a union is undesiable and the size increase is acceptable for HTML5 parser. Henri, what do you think?

Comment 8

[Henri Sivonen (:hsivonen)]

Size increase is undesirable but not totally out of the question. Still, Variant doesn't make sense if it doesn't subsume mOpCode into the variant discriminant, and doing that seems messy in terms of code readability.

My preference is landing the patch as-is, considering that comment 6 doesn't appear to rule that option out.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7398a75d48d
Stop using Encoding::ForName in HTML parser. r=hsivonen

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c7398a75d48d

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8935994 [details]
Bug 1424468 - Stop using Encoding::ForName in HTML parser.

https://reviewboard.mozilla.org/r/206844/#review217992

ok, r=dbaron, although it seems like it might be safer to use only a small set of getters/setters to access those two members in a single place rather than spreading the accesses around