Closed Bug 1273766 Opened 4 years ago Closed 4 years ago

Purge NS_*Hint inline functions for nsChangeHints

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jeremychen, Assigned: jeremychen)

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
Attachment #8753677 - Flags: review?(cam)
Attachment #8753678 - Flags: review?(cam)
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.
(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
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.
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/
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/
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)
Attachment #8753677 - Flags: review?(cam)
Attachment #8753678 - Flags: review?(cam)
Attachment #8754702 - Flags: review?(cam)
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 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 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+
(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)
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
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/
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/
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/
Thanks for cleaning this up.

(And I agree that keeping NS_IsHintSubset is probably the right thing to do.)
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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.