Closed Bug 598832 Opened 9 years ago Closed 9 years ago

Consider eliminating RuleProcessorData

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(16 files, 18 obsolete files)

29.08 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.15 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
12.33 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
9.99 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
21.62 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
9.42 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
17.50 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.20 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
13.79 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
8.22 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
29.02 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
11.71 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
14.07 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
10.84 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
32.60 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
19.40 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Except for the nth-* stuff, everything else can be gotten off the node quickly enough nowadays to not regress matching on "real" pages, and some things speed up due to not having to create and destroy these structs.
I have patches to do this, but they've likely bitrotted.  I'll try to dust them off once the fx4 dust settles.
Depends on: 598833
Blocks: 608648
I'm going to post my wip queue here, merged to tip.  Not asking for review yet, because there are unresolved issues with the nth-child cache stuff and intrinsic state.
Attached patch Add RuleMatchingContext (obsolete) — Splinter Review
Attached patch Move mCompatMode (obsolete) — Splinter Review
Attached patch Remove mNthIndices (obsolete) — Splinter Review
Attached patch Kill mLanguage (obsolete) — Splinter Review
Attached patch kill mContentState (obsolete) — Splinter Review
Attached patch Kill mContentTag (obsolete) — Splinter Review
Depends on: 631529
Blocks: 631837
Comment on attachment 497475 [details] [diff] [review]
Remove mNthIndices

>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.

MoFo
Yes.  And that patch is getting rewritten anyway to deal with bug 631529.
Depends on: 632137
Blocks: 631529
No longer depends on: 631529
Attachment #497471 - Attachment is obsolete: true
Attachment #497472 - Attachment is obsolete: true
Attachment #497473 - Attachment is obsolete: true
Attachment #497470 - Attachment is obsolete: true
Attachment #497474 - Attachment is obsolete: true
Attachment #497475 - Attachment is obsolete: true
Attachment #497476 - Attachment is obsolete: true
Attachment #497477 - Attachment is obsolete: true
Attachment #497478 - Attachment is obsolete: true
Attachment #497479 - Attachment is obsolete: true
Attachment #497480 - Attachment is obsolete: true
Attachment #497481 - Attachment is obsolete: true
Attachment #497482 - Attachment is obsolete: true
Attachment #497483 - Attachment is obsolete: true
Attachment #497484 - Attachment is obsolete: true
Whiteboard: [need review]
Attachment #510897 - Attachment is obsolete: true
Attachment #510897 - Flags: review?(dbaron)
Attachment #510898 - Attachment is obsolete: true
Attachment #510898 - Flags: review?(dbaron)
Attachment #511647 - Attachment is obsolete: true
Attachment #511647 - Flags: review?(dbaron)
Comment on attachment 510864 [details] [diff] [review]
part 1.  Move TreeMatchContext out to where things other than the rule processor can get at it.

>+ * A |TreeMatchContext| has data about a matching operation.  This
>+ * data is not node-specific but is an invariant of the DOM tree the

s/This data is/The data are/

>+  // Reset this data for :visited matching.  This should only be

s/for :visited matching/for matching for the style-if-:visited/, perhaps?

Also, I don't see why ResetForVisitedMatching sets mHaveRelevantLink
to false.  It's only called *after* mHaveRelevantLink is tested, as far
as I can tell.


I'm a little uncomfortable with the way the RuleProcessorData constructor
initializes mVisitedHandling in the case where there isn't a rule walker --
it's assuming a default -- but I'm hoping that goes away in a later patch.

r=dbaron with that
Attachment #510864 - Flags: review?(dbaron) → review+
Comment on attachment 510866 [details] [diff] [review]
part 2.  Move tracking of the scope root to the TreeMatchContext.

r=dbaron
Attachment #510866 - Flags: review?(dbaron) → review+
Comment on attachment 510867 [details] [diff] [review]
part 3.  Stop keeping track of whether the node or document is HTML in the RuleProcessorData.

r=dbaron
Attachment #510867 - Flags: review?(dbaron) → review+
Comment on attachment 510868 [details] [diff] [review]
part 4.  Move tracking of whether we're in quirks mode to the TreeMatchContext.

r=dbaron
Attachment #510868 - Flags: review?(dbaron) → review+
> s/This data is/The data are/
> s/for :visited matching/for matching for the style-if-:visited/, perhaps?

Done.

> Also, I don't see why ResetForVisitedMatching sets mHaveRelevantLink
> to false.  It's only called *after* mHaveRelevantLink is tested.

Not quite: we test mHaveRelevantLink in selector matching too.  But more importantly, once all is said and done we'll be reusing these objects for multiple matching operations, so I wanted to reset them "fully".

> but I'm hoping that goes away in a later patch.

Yeah, it basically does.
Comment on attachment 510869 [details] [diff] [review]
part 5.  Move caching of nth-index stuff to the TreeMatchContext.

I think I should probably be on the Contributors line in the new files
too, since it is based on the existing code from bug 75375.

We do have a whole bunch of tests for this code, so I didn't feel like I needed to be *really* careful reviewing it.

r=dbaron
Attachment #510869 - Flags: review?(dbaron) → review+
Comment on attachment 510870 [details] [diff] [review]
part 6.  Stop caching the language of elements; just reget it from the DOM as needed.

r=dbaron

(I hope you're indentation's right.  I turned this into a diff -w.)
Attachment #510870 - Flags: review?(dbaron) → review+
Comment on attachment 510874 [details] [diff] [review]
part 7.  Stop caching the content state of elements; just reget it from the DOM as needed.

r=dbaron
Attachment #510874 - Flags: review?(dbaron) → review+
Comment on attachment 510890 [details] [diff] [review]
part 8.  Move NodeMatchContext higher up the callstack, so that we don't have to call IsLink as much.

r=dbaron

(I found the Pseudo and XULTree cases a little confusing, but I think that's largely due to my memory of how the code used to work.  I guess it's relatively straightforward that we'd be constructing a node match context for the actual node; I'm just not used to it.)
Attachment #510890 - Flags: review?(dbaron) → review+
Comment on attachment 510891 [details] [diff] [review]
part 9.  Stop caching the element's local name; just get it from the DOM as needed.

>+  if (mTagTable.entryCount) {
>     RuleHashTableEntry *entry = static_cast<RuleHashTableEntry*>
>-                                           (PL_DHashTableOperate(&mTagTable, aTag, PL_DHASH_LOOKUP));
>+                                           (PL_DHashTableOperate(&mTagTable, tag, PL_DHASH_LOOKUP));

Assert that |tag| is non-null?

> /* virtual */ nsRestyleHint
> nsHTMLStyleSheet::HasStateDependentStyle(StateRuleProcessorData* aData)
> {
>   if (aData->mElement->IsHTML() &&
>-      aData->mContentTag == nsGkAtoms::a &&
>+      aData->mElement->Tag() == nsGkAtoms::a &&

>@@ nsHTMLStyleSheet::HasAttributeDependentS
>       element->IsHTML() &&
>-      aData->mContentTag == nsGkAtoms::a) {
>+      aData->mElement->Tag() == nsGkAtoms::a) {
...
>         element->IsHTML() &&
>-        aData->mContentTag == nsGkAtoms::table) {
>+        element->Tag() == nsGkAtoms::table) {

In all three of these, use nsIContent::IsHTML(nsIAtom*) to combine the IsHTML and Tag() == checks.

r=dbaron with that
Attachment #510891 - Flags: review?(dbaron) → review+
Comment on attachment 510892 [details] [diff] [review]
part 10.  Stop caching information about the element's attributes; just get it from the DOM as needed.

r=dbaron
Attachment #510892 - Flags: review?(dbaron) → review+
Comment on attachment 510893 [details] [diff] [review]
part 11.  Stop caching the parent content pointer and the element pointer; just use the passed-in aElement.

r=dbaron
Attachment #510893 - Flags: review?(dbaron) → review+
Comment on attachment 510894 [details] [diff] [review]
part 12.  Stop using RuleProcessorData in SelectorMatchesTree.

>-    if (! data) {
>+    if (! element) {

Fix the out-of-style space while you're here.

r=dbaron
Attachment #510894 - Flags: review?(dbaron) → review+
Comment on attachment 510895 [details] [diff] [review]
part 13.  Stop using RuleProcessorData in the DOM-exposed selector-matching methods.

Can and should we assert that the document parameter to TreeMatchContext's constructor is non-null?

r=dbaron
Attachment #510895 - Flags: review?(dbaron) → review+
Comment on attachment 510896 [details] [diff] [review]
part 14.  Make RuleProcessorData a stack-only class.

r=dbaron
Attachment #510896 - Flags: review?(dbaron) → review+
Comment on attachment 511645 [details] [diff] [review]
part 15.  Make RuleProcessorData not inherit from TreeMatchContext anymore, so we can decouple the lifetimes.

Avoid the extra blank line at the start of nsStyleSet::HasAttributeDependentStyle

Also, I tend to wrap comments at tw=72... which explains some of the extra blame you're taking for parts of comments you're not modifying.  Rewrapping back to 72 might be nice.
Attachment #511645 - Flags: review?(dbaron) → review+
Comment on attachment 511754 [details] [diff] [review]
part 16.  Use a single TreeMatchContext for all the style resolution that the frame constructor does as part of a single frame construction batch.

r=dbaron
Attachment #511754 - Flags: review?(dbaron) → review+
> I think I should probably be on the Contributors line in the new files

Good point.  Done.

> Assert that |tag| is non-null?

Added an NS_ABORT_IF_FALSE to that effect after getting all the stuff up front in ContentEnumFunc.

> In all three of these, use nsIContent::IsHTML(nsIAtom*)

Done.

> Fix the out-of-style space while you're here.

Done.

> Can and should we assert that the document parameter to TreeMatchContext's
> constructor is non-null?

The constructor will crash if it's not, since it calls aDocument->IsHTML().  Not much point asserting it, I think, given that.

> Avoid the extra blank line at the start of
> nsStyleSet::HasAttributeDependentStyle

Done.

> Rewrapping back to 72 might be nice.

Done.
Whiteboard: [need review] → [need bug 598833 fixed?]
Though that might have been from bug 631837, which had this bug as a prerequisite.
Blocks: 630480
Depends on: 667520
You need to log in before you can comment on or make changes to this bug.