Closed Bug 938334 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: arnaud.bienner, Assigned: arnaud.bienner)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 5 obsolete files)

As discussed in bug 875275 comment 31.
Assignee: nobody → arnaud.bienner
Status: NEW → ASSIGNED
Attached patch type_atom.patch (obsolete) — Splinter Review
Try build almost OK (except one test on Android, but I don't think it happens because of my changes).
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.
Attachment #831791 - Flags: review?(dbaron)
Depends on: 831791
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)
Attachment #831791 - Flags: review?(dbaron) → review+
(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"?
Flags: needinfo?(dbaron)
(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.
Flags: needinfo?(dbaron)
Attached patch type_atom.patch v2 (obsolete) — Splinter Review
Updated version of the patch, with David's comments applied.
Attachment #831791 - Attachment is obsolete: true
Attachment #832973 - Flags: review+
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*.
Keywords: checkin-needed
Whiteboard: [leave open]
(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.
https://tbpl.mozilla.org/?tree=Try&rev=7dfe9e25f00e
This one should be better (I forgot that uint isn't a standard type).
Attached patch type_atom.patch v3 (obsolete) — Splinter Review
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).
Attachment #832973 - Attachment is obsolete: true
Attachment #833327 - Flags: review?(dbaron)
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
Attachment #833327 - Flags: review?(dbaron) → review+
Attached patch type_atom.patch v4 (obsolete) — Splinter Review
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
Attachment #833327 - Attachment is obsolete: true
Attachment #8333506 - Flags: review+
Keywords: checkin-needed
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
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.
Attached patch type_atom_part2.patch (obsolete) — Splinter Review
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?
Attachment #8338835 - Flags: review?(dbaron)
Pushed to try and looks good:
https://tbpl.mozilla.org/?tree=Try&rev=022418cb7731
Comment on attachment 8338835 [details] [diff] [review]
type_atom_part2.patch

r=dbaron, and yes, this bug can be closed now
Attachment #8338835 - Flags: review?(dbaron) → review+
Thanks David for the review :)
Keywords: checkin-needed
Whiteboard: [leave open]
Attachment #8338835 - Flags: checkin?
Comment on attachment 8338835 [details] [diff] [review]
type_atom_part2.patch

This is bitrotted. Please rebase.
Attachment #8338835 - Attachment is obsolete: true
Attachment #8338835 - Flags: checkin?
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
Attachment #8333506 - Attachment is obsolete: true
Attachment #8341273 - Flags: review?(dbaron)
Attachment #8341273 - Flags: feedback?(cam)
The interdiff.
Attachment #8341273 - Flags: review?(dbaron) → review+
(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.
OK great :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5ce7a0d8e44c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Attachment #8341273 - Flags: feedback?(cam) → feedback+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.