938334 - Use "Type" as parameter in nsCSSPseudoElements methods instead of "nsIAtom*" when it makes sense to do so

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Arnaud Bienner]

As discussed in bug 875275 comment 31.

Comment 1

[Arnaud Bienner]

Attachment: type_atom.patch

Comment 2

[Arnaud Bienner]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1bc106b86bd1

Comment 3

[Arnaud Bienner]

Try build almost OK (except one test on Android, but I don't think it happens because of my changes).

Comment 4

[Arnaud Bienner]

Comment on attachment 831791 [details] [diff] [review]
type_atom.patch

David, could you please review this patch?
As stated above, this is a follow-up from your remarks in bug 875275 comment 31 (I finally had time to work on this).
Feel free to delegate the review to someone else if you're busy.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 831791 [details] [diff] [review]
type_atom.patch

> /* static */ uint32_t
>-nsCSSPseudoElements::FlagsForPseudoElement(nsIAtom *aAtom)
>+nsCSSPseudoElements::FlagsForPseudoElement(const Type aType)
> {
>-  uint32_t i;
>-  for (i = 0; i < ArrayLength(CSSPseudoElements_info); ++i) {
>-    if (*CSSPseudoElements_info[i].mAtom == aAtom) {
>-      break;
>-    }
>-  }
>-  NS_ASSERTION(i < ArrayLength(CSSPseudoElements_info),
>+  NS_ASSERTION(aType < ePseudo_NotPseudoElement,
>                "argument must be a pseudo-element");

Could you keep the right side of this assertion using ArrayLength() (although ..._flags is probably better than ..._info), in other words, make it aType < ArrayLength(CSSPseudoElements_flags)?

>-  return CSSPseudoElements_flags[i];
>+  return CSSPseudoElements_flags[aType];
> }

r=dbaron with that, though I suspect even more conversion is possible (followup patch please, though)

Comment 6

[Arnaud Bienner]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #5)
> r=dbaron with that, though I suspect even more conversion is possible
> (followup patch please, though)

What do you mean "more conversion is possible"?

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Arnaud Bienner from comment #6)
> (In reply to David Baron [:dbaron] (needinfo? me) from comment #5)
> > r=dbaron with that, though I suspect even more conversion is possible
> > (followup patch please, though)
> 
> What do you mean "more conversion is possible"?

I suspect there might be even more things that could converted from nsIAtom* to nsCSSPseudoElements::Type.

Comment 8

[Arnaud Bienner]

Attachment: type_atom.patch v2

Updated version of the patch, with David's comments applied.

Comment 9

[Arnaud Bienner]

This patch (attachment 832973 [details] [diff] [review]) can be landed, but I will keep the bug opened for now as, as stated in comment 7, there might be other places when we can use nsCSSPseudoElements::Type instead of nsIAtom*.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Busted on Windows.
https://tbpl.mozilla.org/php/getParsedLog.php?id=30610992&tree=Try

Comment 11

[Arnaud Bienner]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #10)
> Busted on Windows.
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30610992&tree=Try

Ah yes: I didn't ask for building "Windows Debug" on my push to try.
I'm not so surprised the compiler doesn't like this: actually I pushed to try also to be sure mixing array indexes/Enum the way I did was fine on all platforms/configurations.

So, how to fix this?
Two solution I believe:
1) Perform a similar check to the one done in "GetPseudoAtom", i.e. something similar to what I initially did: "aType < nsCSSPseudoElements::ePseudo_PseudoElementCount" (I should have do this instead of "< ePseudo_NotPseudoElement" in my first patch).
This way, we stay consistent.
2) static_cast<uint>(aType); this sounds clearer to me (and we keep the current check on ArrayLength).

I will check the second one is working well.

Comment 12

[Arnaud Bienner]

https://tbpl.mozilla.org/?tree=Try&rev=1c98a1cdc013

Comment 13

[Arnaud Bienner]

https://tbpl.mozilla.org/?tree=Try&rev=7dfe9e25f00e
This one should be better (I forgot that uint isn't a standard type).

Comment 14

[Arnaud Bienner]

Attachment: type_atom.patch v3

Last push to try looks fine :)

David, could you please check this slightly different patch is OK for you? I just added a static_cast to make things clearer, and compilers happy (see comment 11).

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 833327 [details] [diff] [review]
type_atom.patch v3

Could you use size_t rather than unsigned int?  (Both in the cast, and the type of i?)  And maybe also rename i to index?

r=dbaron

Comment 16

[Arnaud Bienner]

Attachment: type_atom.patch v4

s/unsigned int/size_t
s/i/index
Marking it as r+ directly as I just applied David's comments.

Pushed to try, looks fine:
https://tbpl.mozilla.org/?tree=Try&rev=af036d44f914

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc3679786f70

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc3679786f70

Comment 19

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Remaining changes I see:

 1. make ElementRestyler::RestyleSelf pass pseudoType instead of pseudoTag to nsCSSPseudoElements::PseudoElementSupportsStyleAttribute

 2. (after 1) remove the nsIAtom* version of nsCSSPseudoElements::PseudoElementSupportsStyleAttribute

Comment 20

[Arnaud Bienner]

I had a quick look to remaining things, but didn't take the time to carefully review every cases and to update this bug.

>  1. make ElementRestyler::RestyleSelf pass pseudoType instead of pseudoTag
> to nsCSSPseudoElements::PseudoElementSupportsStyleAttribute

Indeed, you're right. I wasn't sure we have a Type which correspond to this nsIAtom* here.

>  2. (after 1) remove the nsIAtom* version of
> nsCSSPseudoElements::PseudoElementSupportsStyleAttribute

Indeed. We had for convenience, but I don't think it's worth keeping it now it's not used anywhere.

I don't think we can get rid of the remaining methods which take nsIAtom* as parameter:
- IsCSS2PseudoElement because IsAnonBox needs nsIAtom*
- IsPseudoElement: we might check "< ePseudo_PseudoElementCount" instead of nsAtomListUtils::IsMember. But this might make the code less readable and anyway, the only place where we use it (apart from IsCSS2PseudoElement) is in nsComputedDOMStyle, but we have only nsIAtom* if I'm right, so we will have to call GetPseudoType anyway.


Long story short: I don't think we can do other changes that the one you pointed out.

I will write a patch for this.

Comment 21

[Arnaud Bienner]

Attachment: type_atom_part2.patch

David, could you please review this small patch?
I made the changes you suggested in comment 19.
Also, as I said in comment 20, I don't think there are other places where we need to replace nsAItom* by Type.
So once this patch will be landed, I guess this bug can be closed, don't you think?

Comment 22

[Arnaud Bienner]

Pushed to try and looks good:
https://tbpl.mozilla.org/?tree=Try&rev=022418cb7731

Comment 23

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8338835 [details] [diff] [review]
type_atom_part2.patch

r=dbaron, and yes, this bug can be closed now

Comment 24

[Arnaud Bienner]

Thanks David for the review :)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8338835 [details] [diff] [review]
type_atom_part2.patch

This is bitrotted. Please rebase.

Comment 26

[Arnaud Bienner]

Attachment: type_atom_part2 v2

Updated version of the patch, rebase against current tree. Hope next time it will be integrated before someone else change something it modifies ;)

Cameron, my patch didn't applied because of your changes, so as I needed to modify a bit what you've done [1], I would like to be sure it's OK i.e.:
- it's fine to use pseudoType instead of pseudoTag (as my first patch did when calling PseudoElementSupportsStyleAttribute)
- it's fine to remove (now unused) PseudoElementSupportsUserActionState(nsIAtom*)

David, you already r+ the previous patch, but as I made some extra changes, would you mind quickly reviewing this new patch?

I pushed it to try just in case, but I'm confident it will be green: https://tbpl.mozilla.org/?tree=Try&rev=680687e23869

[1]: http://hg.mozilla.org/mozilla-central/rev/ae82eacc5bff

Comment 27

[Arnaud Bienner]

Attachment: type_atom_part2 v2: interdiff

The interdiff.

Comment 28

[Cameron McCormack (:heycam)]

(In reply to Arnaud Bienner from comment #26)
> Cameron, my patch didn't applied because of your changes, so as I needed to
> modify a bit what you've done [1], I would like to be sure it's OK i.e.:
> - it's fine to use pseudoType instead of pseudoTag (as my first patch did
> when calling PseudoElementSupportsStyleAttribute)
> - it's fine to remove (now unused)
> PseudoElementSupportsUserActionState(nsIAtom*)

Yep that's fine.

Comment 29

[Arnaud Bienner]

OK great :)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce7a0d8e44c

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ce7a0d8e44c