Closed
Bug 1273766
Opened 8 years ago
Closed 8 years ago
Purge NS_*Hint inline functions for nsChangeHints
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: chenpighead, Assigned: chenpighead)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
Inline operators for nsChangeHints have been available for a while (see [1]). Although some of the style structs have been using them, some have not. We should be able to fully take advantage of these operators. The coding style would be more consistent as well. [1] https://dxr.mozilla.org/mozilla-central/rev/a884b96685aa13b65601feddb24e5f85ba861561/layout/base/nsChangeHint.h#221
Comment hidden (spam) |
Comment hidden (spam) |
Assignee | ||
Updated•8 years ago
|
Attachment #8753677 -
Flags: review?(cam)
Attachment #8753678 -
Flags: review?(cam)
Assignee | ||
Comment 3•8 years ago
|
||
Per bug 1169440 comment 9, seems using operator is preferred than inline functions. I should've read this first. Would request review after I re-write a patch to use operator instead of inline functions.
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #3) > Per bug 1169440 comment 9, seems using operator is preferred than inline > functions. I should've read this first. Would request review after I > re-write a patch to use operator instead of inline functions. We shall gain some readability after doing so, since the listing of nsChangeHint parameters are much more clear.
Summary: Use inline operators on nsChangeHints for style structs → Purge NS_*Hint inline functions for nsChangeHints
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/54194/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/54194/
Attachment #8753677 -
Attachment description: MozReview Request: Bug 1273766 - part1: fix comments in nsChangeHint.h. r=heycam → MozReview Request: Bug 1273766 - part1: purge NS_CombineHint inline.
Attachment #8753678 -
Attachment description: MozReview Request: Bug 1273766 - part2: use inlined nsChangeHint operators for style structs. r=heycam → MozReview Request: Bug 1273766 - part2: purge NS_SubtractHint inline.
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8753677 [details] MozReview Request: Bug 1273766 - part1: purge NS_CombineHint inline. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53436/diff/1-2/
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8753678 [details] MozReview Request: Bug 1273766 - part2: purge NS_SubtractHint inline. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53438/diff/1-2/
Assignee | ||
Comment 8•8 years ago
|
||
Hi Cameron, could you take a look at these patches? With these changes, I think the coding style would be more consistent, and code could be easier to read/write. There're 2 more questions I may need your feedback: 1. As to the only left NS_*Hint, NS_IsHintSubset in [1], I'm not sure if purging this could gain us any good, since this may complicate the code and affect the readability. Do you think it's okay to keep NS_IsHintSubset alive in this bug? 2. While writing patches for this bug, I was hit by a thought that maybe we should use enum class for both nsChangeHint and nsRestyleHint? In this way, we could get rid of those inline operators as well. You kind of mention this in bug 1169440 comment 16. Do you think it is a good idea to file a bug for that? [1] https://dxr.mozilla.org/mozilla-central/rev/1806d405c8715949b39fa3a4fc142d14a60df590/layout/base/nsChangeHint.h#243
Flags: needinfo?(cam)
Assignee | ||
Updated•8 years ago
|
Attachment #8753677 -
Flags: review?(cam)
Attachment #8753678 -
Flags: review?(cam)
Attachment #8754702 -
Flags: review?(cam)
Comment 9•8 years ago
|
||
Comment on attachment 8753677 [details] MozReview Request: Bug 1273766 - part1: purge NS_CombineHint inline. https://reviewboard.mozilla.org/r/53436/#review51100 ::: layout/style/nsStyleStruct.h:1422 (Diff revision 2) > FreeByObjectID(mozilla::eArenaObjectID_nsStyleList, this); > } > > nsChangeHint CalcDifference(const nsStyleList& aOther) const; > static nsChangeHint MaxDifference() { > - return NS_CombineHint(NS_STYLE_HINT_FRAMECHANGE, > + return NS_STYLE_HINT_FRAMECHANGE | nsChangeHint_NeutralChange; I think one change hint per line is clearer (and in a couple of other places in this patch).
Attachment #8753677 -
Flags: review?(cam) → review+
Comment 10•8 years ago
|
||
Comment on attachment 8753678 [details] MozReview Request: Bug 1273766 - part2: purge NS_SubtractHint inline. https://reviewboard.mozilla.org/r/53438/#review51102
Attachment #8753678 -
Flags: review?(cam) → review+
Comment 11•8 years ago
|
||
Comment on attachment 8754702 [details] MozReview Request: Bug 1273766 - part3: purge NS_UpdateHint inline. https://reviewboard.mozilla.org/r/54194/#review51104
Attachment #8754702 -
Flags: review?(cam) → review+
Comment 12•8 years ago
|
||
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #8) > There're 2 more questions I may need your feedback: > 1. As to the only left NS_*Hint, NS_IsHintSubset in [1], I'm not sure if > purging this could gain us any good, since this may complicate the code and > affect the readability. Do you think it's okay to keep NS_IsHintSubset alive > in this bug? Yes, I think it's fine to leave it. > 2. While writing patches for this bug, I was hit by a thought that maybe we > should use enum class for both nsChangeHint and nsRestyleHint? In this way, > we could get rid of those inline operators as well. You kind of mention this > in bug 1169440 comment 16. Do you think it is a good idea to file a bug for > that? Yes, feel free to file a bug for that. It's probably not as important as the cleanups you've done in this bug, though. (Using enum classes gets us a small amount of safety, since we can't accidentally treat an nsChangeHint as an integer, and we would get to remove the inline operators in favour of using MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS, but that is all I think.)
Flags: needinfo?(cam)
Assignee | ||
Comment 13•8 years ago
|
||
Thank you for the review. :) > > 2. While writing patches for this bug, I was hit by a thought that maybe we > > should use enum class for both nsChangeHint and nsRestyleHint? In this way, > > we could get rid of those inline operators as well. You kind of mention this > > in bug 1169440 comment 16. Do you think it is a good idea to file a bug for > > that? > > Yes, feel free to file a bug for that. It's probably not as important as > the cleanups you've done in this bug, though. (Using enum classes gets us a > small amount of safety, since we can't accidentally treat an nsChangeHint as > an integer, and we would get to remove the inline operators in favour of > using MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS, but that is all I think.) Filed Bug 1274891
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8753677 [details] MozReview Request: Bug 1273766 - part1: purge NS_CombineHint inline. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53436/diff/2-3/
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8753678 [details] MozReview Request: Bug 1273766 - part2: purge NS_SubtractHint inline. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53438/diff/2-3/
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8754702 [details] MozReview Request: Bug 1273766 - part3: purge NS_UpdateHint inline. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54194/diff/1-2/
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fc7ba5055b0 https://hg.mozilla.org/integration/mozilla-inbound/rev/6d7c672dab42 https://hg.mozilla.org/integration/mozilla-inbound/rev/743b83a94cd4
Thanks for cleaning this up. (And I agree that keeping NS_IsHintSubset is probably the right thing to do.)
Comment hidden (spam) |
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4fc7ba5055b0 https://hg.mozilla.org/mozilla-central/rev/6d7c672dab42 https://hg.mozilla.org/mozilla-central/rev/743b83a94cd4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1277128
You need to log in
before you can comment on or make changes to this bug.
Description
•