147887 - put different types of pseudos in different atom lists

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

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

I'd like to put different types of pseudos (elements, classes, and our special
globals that we use as pseudo-elements with null content) into three different
lists, with an additional static member function relative to the current atom
lists that tests whether something is an atom in one of the lists.  This will
help with:
 * showing what's going on by making the type distinction clearer
 * allowing callers to distinguish between the types, which will help to fix a
number of bugs that I'm going to add as dependencies.

Comment 1

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

Daniel had some similar work in bug 62843.  I also had this partly done in one
of my spare trees...

Comment 2

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

Attachment: the partial patch that I had in my tree

Comment 3

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

I think nsCSSAnonBoxPseudos would be better than nsCSSPseudoGlobals, since
they're really pseudos that we use to style anonymous boxes.

Also, we can define classes:

class nsICSSPseudoElement : public nsIAtom {};
class nsICSSPseudoClass : public nsIAtom {};
class nsICSSAnonBoxPseudo : public nsIAtom {};

and have the atom lists composed of these, and use them for parameter types in
functions.  This could be a second pass after the first round of changes.

Comment 4

[Hixie (not reading bugmail)]

Could we get a blanket r=/sr= on this idea so David can move forward with it
without having to check back for every minor fixup?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh yeah. r/sr=roc+moz

Comment 6

[Boris Zbarsky [:bzbarsky]]

sr=bzbarsky as well.

Comment 7

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

Attachment: patch

This should be pretty much complete.  (My tree is still building, though, since
I just did some search-replace on the patch and reapplied.)  I'm not checking
it in tonight (it's late), so if you want to really review the patch, feel
free.  (It's probably good if you look at the changes in content/shared/.  The
rest are just wrote substitution, with occasional reindenting.)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 106535 [details] [diff] [review]
patch

> + * This file contains the list of all CSS nsIAtoms and their values. 

This part of the comment isn't really true for the new files. ;)  

MPL please, not NPL for the licenses...

Any reason to keep ":not" named "notPseudo" instead of just "not"?

Other than those nits, looks great.

Comment 9

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

|not| is a reserved word (it's an alternative representation of ! -- see C++
2.11 clause 2 and 2.5).

I'm a little hesitant about changing licenses when splitting files, but I guess
Netscape has agreed to NPL->MPL conversion (is there an official announcement of
that somewhere)?

I'll fix the comments...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah, I was having a sneaking suspicion that it must be reserved.  Good point
about the file splitting; do whichever is less work with the licenses.  ;)

Comment 11

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

Attachment: patch

This fixes the licenses and the comments in the list files.  I also put the
lists in a more "logical" order (grouped by function), and found one unused
anon-box-pseudo (:-moz-wrapper-frame, nice and descriptive), and removed the
corresponding rule from html.css

Comment 12

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

Comment on attachment 106537 [details] [diff] [review]
patch

A few other things to note:
 * I just change IsAnonBoxPseudo to IsAnonBox in my tree.
 * This removes IsPseudoClass from nsCSSParser.cpp and replaces it with
nsCSSPseudoClasses::IsPseudoClass, which also returns true for ":not". 
However, I think this is OK.  I'll test it once my tree finishes building.

Comment 13

[Hixie (not reading bugmail)]

:not() cannot contait :not() and has a different specificity (or rather, takes
its specificity from its argument). Those would be two things to look at in
particular when checking that that change doesn't matter.

Comment 14

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

The only changes I made that affected :not were in the parser, and I ran a
significant portion of the Selectors test suite, so I think I'm ok.

Comment 15

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

Comment on attachment 106537 [details] [diff] [review]
patch

Fix checked in, 2002-11-17 07:37 PDT.

I still want to use this bug to use the new types for parameters to some
functions.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: broken stuff

first-letter and first-line are confused....

Comment 17

[Boris Zbarsky [:bzbarsky]]

I suspect these changes in nsCSSFrameConstructor::GetFirstLineStyle:

-                                 nsHTMLAtoms::firstLinePseudo,
+                                 nsCSSPseudoElements::firstLetter,

and in nsCSSFrameConstructor::HaveFirstLineStyle:

-                        nsHTMLAtoms::firstLinePseudo);
+                        nsCSSPseudoElements::firstLetter);

;)

Comment 18

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

Oops.  Thanks for catching that.  I just checked in the fix.

Comment 19

[Boris Zbarsky [:bzbarsky]]

What's left to do here?