826961 - Revise SVGPreserveAspectRatio constructor and callers so that the silly cast is not needed

Status: RESOLVED FIXED

Description

[Masatoshi Kimura [:emk]]

I think some enum types would be even better than featureless uint_8.

Comment 1

[Masatoshi Kimura [:emk]]

Created attachment 698231 [details] [diff] [review]
patch

I also renamed getters because they are infallible.

Comment 2

[David Zbarsky (:dzbarsky)]

Comment on attachment 698231 [details] [diff] [review]
patch

I'm not an SVG peer.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 698231 [details] [diff] [review]
patch

Thanks for filing & posting a patch for this!  Patch mostly looks great -- I just have 2 concerns that I wanted to bring up.

>+class SVGPreserveAspectRatio MOZ_FINAL
> {
[...]
>-  uint8_t mAlign;
>-  uint8_t mMeetOrSlice;
>+  SVGAlign mAlign;
>+  SVGMeetOrSlice mMeetOrSlice;
>   bool mDefer;
> };

(1) While I'm in favor of the migration to enum values, I think we should double-check that it doesn't bring a penalty from failing to pack adjacent member-variables.  I can imagine that one of our compilers might refuse to pack mAlign and mMeetOrSlice into the same word, after your patch, because they're different types (even though they're still both backed by uint8_t).  So it might be worth doing two TryServer runs (one with your patch, one without), with a debugging printf inserted somewhere to print the size of the classes in question, to be sure there's no silly compiler-imposed penalty on the size of this struct.

>-  if (val.GetDefer()) {
>+  if (val.Defer()) {

(2) I don't think I'm in favor of this "remove 'Get' prefix" part.  My impression (which could be wrong) is that we're fine with "GetFoo()"-style getters, and we (usually) only drop the "Get" prefix if the method returns a pointer (which could be null). And in this case, I think that removing the prefix actually makes the code less clear -- the existing "GetDefer()" is clearly a getter, whereas "Defer()" sounds like a function that defers something.  (To a casual reader, "if (val.Defer())" sounds like we're deferring val (whatever that means), and proceeding if that succeeds)

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> (1) While I'm in favor of the migration to enum values, I think we should
> double-check that it doesn't bring a penalty from failing to pack adjacent
> member-variables.  I can imagine that one of our compilers might refuse to
> pack mAlign and mMeetOrSlice into the same word, after your patch, because
> they're different types (even though they're still both backed by uint8_t). 
> So it might be worth doing two TryServer runs (one with your patch, one
> without), with a debugging printf inserted somewhere to print the size of
> the classes in question, to be sure there's no silly compiler-imposed
> penalty on the size of this struct.

static assert would be much faster to catch this :)
https://tbpl.mozilla.org/?tree=Try&rev=37997e806c38

> (2) I don't think I'm in favor of this "remove 'Get' prefix" part.  My
> impression (which could be wrong) is that we're fine with "GetFoo()"-style
> getters, and we (usually) only drop the "Get" prefix if the method returns a
> pointer (which could be null). And in this case, I think that removing the
> prefix actually makes the code less clear -- the existing "GetDefer()" is
> clearly a getter, whereas "Defer()" sounds like a function that defers
> something.  (To a casual reader, "if (val.Defer())" sounds like we're
> deferring val (whatever that means), and proceeding if that succeeds)

I'll revert renaming once the try-run passed.

Comment 5

[Masatoshi Kimura [:emk]]

B2g compiler wasn't so smart, now testing with uint8_t version to verify things getting worse.
https://tbpl.mozilla.org/?tree=Try&rev=98a2b85e6aeb

Comment 6

[Masatoshi Kimura [:emk]]

Created attachment 698288 [details] [diff] [review]
patch v2

Comment 7

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b4ce0db7ecb

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0b4ce0db7ecb

Comment 9

[Cameron Kaiser [:spectre]]

What's with the static assert? For example, on PowerPC bool is word-size, so it won't build as written.

Comment 10

[Daniel Holbert [:dholbert]]

I believe the static assert was there to be sure we didn't regress the size of SVGPreserveAspectRatio. (because this bug's patch switched it from using two adjacent uint8_t's to two adjacent uint8_t-backed enums, which could conceivably be packed less efficiently depending on how the compiler handles enums.)

Given that it didn't fail, I think we can safely remove it, if it's causing problems on other platforms.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Given that it didn't fail

...on tier 1 platforms, of course. :)  (It sounds like it *is* failing on powerpc, but I'd expect it always would've failed, since it's the bool that's taking up all the space there, and this bug didn't change the bool.)

Comment 12

[Cameron Kaiser [:spectre]]

Fair enough. I can file a separate bug or put a patch here, whatever is preferred.

Comment 13

[Daniel Holbert [:dholbert]]

Sounds great!  I'd prefer that it happen in a separate bug, since it's been months since this bug's original patch landed.

CC me; I'll grant swift r+. :)

Comment 14

[Cameron Kaiser [:spectre]]

Done (bug 847804). Thanks!