put different types of pseudos in different atom lists

RESOLVED FIXED in Future

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
16 years ago
11 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Blocks: 1 bug)

Trunk
Future
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Daniel had some similar work in bug 62843.  I also had this partly done in one
of my spare trees...
Created attachment 98432 [details] [diff] [review]
the partial patch that I had in my tree
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → Future
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.
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?
Created attachment 106535 [details] [diff] [review]
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.)
Attachment #98432 - Attachment is obsolete: true
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.
Attachment #106535 - Flags: superreview+
|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...
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.  ;)
Created attachment 106537 [details] [diff] [review]
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
Attachment #106535 - Attachment is obsolete: true
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.
: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.
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 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.
Attachment #106537 - Attachment is obsolete: true
Created attachment 106639 [details]
broken stuff

first-letter and first-line are confused....
I suspect these changes in nsCSSFrameConstructor::GetFirstLineStyle:

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

and in nsCSSFrameConstructor::HaveFirstLineStyle:

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

;)
Oops.  Thanks for catching that.  I just checked in the fix.
No longer blocks: 177539
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.