836604 - Merge nsStyleSet::GetContext()'s boolean parameters into a single 'flags' parameter

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Daniel Holbert [:dholbert]]

(sorry, forgot the bug description)

I'd like to add another boolean variable to nsStyleSet::GetContext, in bug 812822, but I don't want it to have four mysterious "..., true, false, ..., true, true)" boolean args in each invocation. Better to collapse the bools into a flags bitfield. Filing this bug on that.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 708484 [details] [diff] [review]
fix v1

>-                                                aIsVisitedLink);
>+                                                aFlags & eIsLink);

eIsVisitedLink?

>+        aFlags & eIsLink ?
>+        aFlags & eIsVisitedLink
>+        : (aParentContext && aParentContext->RelevantLinkVisited());

This would benefit from some parens and the bit operations, I think.

>+                          aFlags & ^eDoAnimation);

That should be a '~'.  I assume the above doesn't compile, right?


>                     // For pseudos, |data.IsLink()| being true means that
>                     // our parent node is a link.

That comment should probably move to above the |flags = eNoFlags;|, both times.

>+  if ((aStyleContext->IsLinkContext() &&
>+       aStyleContext->RelevantLinkVisited()) ||
>+      aNewParentContext->RelevantLinkVisited()) {

No, this is wrong.  Say aStyleContext->IsLinkContext() is true, aStyleContext->RelevantLinkVisited() is false, and aNewParentContext->RelevantLinkVisited() is true.  The new code returns true, while the old code correctly returned false.

r=me with those issues fixed.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v2

(In reply to Boris Zbarsky (:bz) from comment #3)
> eIsVisitedLink?

Oops, thanks! Fixed.

> >+        aFlags & eIsLink ?
> >+        aFlags & eIsVisitedLink
> >+        : (aParentContext && aParentContext->RelevantLinkVisited());
> 
> This would benefit from some parens and the bit operations, I think.

I added parens around bit operations, and I pulled the first chunk (before the '?') up to the previous line -- hopefully it's clearer now.

> >+                          aFlags & ^eDoAnimation);
> 
> That should be a '~'.  I assume the above doesn't compile, right?

Yup (& it doesn't compile -- sorry about that. Previously, I just was incorrectly passing 'aFlags' straight through, and *that* compiled, but I noticed just before posting that I needed to invert the eDoAnimation bit, and of course I used the wrong operator-symbol. :))

Fixed.

> >                     // For pseudos, |data.IsLink()| being true means that
> >                     // our parent node is a link.
> 
> That comment should probably move to above the |flags = eNoFlags;|, both
> times.

Fixed.

> >+  if ((aStyleContext->IsLinkContext() &&
> >+       aStyleContext->RelevantLinkVisited()) ||
> >+      aNewParentContext->RelevantLinkVisited()) {
> 
> No, this is wrong.  Say aStyleContext->IsLinkContext() is true,
> aStyleContext->RelevantLinkVisited() is false, and
> aNewParentContext->RelevantLinkVisited() is true.  The new code returns
> true, while the old code correctly returned false.

Ah, right -- technically I should be checking "!aStyleContext->IsLinkContext()" on that last line there, to preserve all of the original logic.

Instead of that, though, I just reverted this change back to the original code, w/ 'bool relevantLinkVisited', and then I set the flag if relevantLinkVisited is true.

> r=me with those issues fixed.

Thanks!

Comment 5

[Daniel Holbert [:dholbert]]

Try run (all tests on linux64 debug & opt):
 https://tbpl.mozilla.org/?tree=Try&rev=e55242bf2a17

Comment 6

[Daniel Holbert [:dholbert]]

Try run is looking good! I'll land this tomorrow.

Comment 7

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7245eecea323

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7245eecea323