Closed
Bug 826961
Opened 10 years ago
Closed 10 years ago
Revise SVGPreserveAspectRatio constructor and callers so that the silly cast is not needed
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: emk, Assigned: emk)
References
Details
Attachments
(1 file, 1 obsolete file)
7.69 KB,
patch
|
longsonr
:
review+
|
Details | Diff | Splinter Review |
I think some enum types would be even better than featureless uint_8.
Assignee | ||
Comment 1•10 years ago
|
||
I also renamed getters because they are infallible.
Comment 2•10 years ago
|
||
Comment on attachment 698231 [details] [diff] [review] patch I'm not an SVG peer.
Attachment #698231 -
Flags: review?(dzbarsky) → review?(longsonr)
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
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
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #698231 -
Attachment is obsolete: true
Attachment #698231 -
Flags: review?(longsonr)
Attachment #698288 -
Flags: review?(longsonr)
Updated•10 years ago
|
Attachment #698288 -
Flags: review?(longsonr) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b4ce0db7ecb
Flags: in-testsuite-
Updated•10 years ago
|
Version: unspecified → Trunk
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0b4ce0db7ecb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 9•10 years ago
|
||
What's with the static assert? For example, on PowerPC bool is word-size, so it won't build as written.
Comment 10•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
Fair enough. I can file a separate bug or put a patch here, whatever is preferred.
Comment 13•10 years ago
|
||
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•10 years ago
|
||
Done (bug 847804). Thanks!
You need to log in
before you can comment on or make changes to this bug.
Description
•