Closed Bug 416317 (querySelector) Opened 16 years ago Closed 16 years ago

Implement W3C Selectors API (querySelector and querySelectorAll)

Categories

(Core :: DOM: Core & HTML, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: bedney, Assigned: bzbarsky)

References

()

Details

(Keywords: perf)

Attachments

(1 file, 6 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-us) AppleWebKit/523.10.6 (KHTML, like Gecko) Version/3.0.4 Safari/523.10.6
Build Identifier: 

The W3C Selectors API has been unleashed. The fine folks over at the WebKit project have now implemented it :-).

http://bugs.webkit.org/show_bug.cgi?id=16587

Sure makes things faster to have this stuff in C code, rather than using a JS library :-).

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Keywords: perf
Status: UNCONFIRMED → NEW
Ever confirmed: true
Jumping the gun a little, aren't they?  It's not a Candidate Recommendation or Proposed Recommendation, but a Working Draft.
Ok folks -

The IE8 beta1 release notes made available today show that the Microsoft folks have implemented this natively for IE8 (querySelector and querySelectorAll - they didn't do the namespace resolver).

Any takers?? There could be a bottle of nice wine in it for you ;-).

Cheers,

- Bill
I'm going to do a compliance test suite once I finish up my current performance work unless, of course, someone else wants to tackle it.
Attached patch Rough draft (obsolete) — Splinter Review
I couldn't sleep, sadly.

Things left to do:

1)  Get clarifications on the various spec issues that came up when I was doing
    this.  Make changes to the implementation as needed based on those
    resolutions.
2)  Clean up the CSSParser namespace changes to not be quite so cut-and-paste.
3)  Verify that the various error conditions (esp. said namespace changes)
    throw the right exceptions.
4)  Change the NodeList implementation to be cycle-collected, since the current
    code can leak pretty easily.
5)  Decide whether we want this to work for documents with no presentation (I
    think yes) and rewrite the allocation code for RuleProcessorData to deal.
6)  Don't flag nodes as having slow selectors if those selectors come via this
    API.
7)  Write exhaustive tests.  I found the slickspeed querySelectorAll test that
    the webkit folks published, but the querySelector code is basically
    untested, as is everything to do with the NSResolvers.

If someone wants to make this happen faster, mochitests covering item 7 would be very welcome.
Oh, the other thing that would make things faster is getting the spec clarifications, of course.
For my future reference, a sorta test for querySelectorAll: http://webkit.org/perf/slickspeed
(In reply to comment #7)
> For my future reference, a sorta test for querySelectorAll:
> http://webkit.org/perf/slickspeed

That doesn't actually test for compliance, as you noted. As far as I know jQuery has the most advanced JS-based CSS selector compliance test suite:
http://dev.jquery.com/browser/trunk/jquery/test/unit/selector.js#L43

Using that, in conjunction with the official CSS selector test suite, should give a good level of coverage.
Alias: querySelector
Depends on: 422865
In addition to those, we need to make sure that we're also testing disconnected subtrees, as well as some invalid selectors in various ways.  Not to mention various errors on the part of the NSResolver.

Also, I believe there are no namespace-aware tests in the Selectors test suite.  I don't see any in the jquery test suite either (which makes sense).  So we'd need to create those, right?
Depends on: 422868
Summary: Implement W3C Selectors API → Implement W3C Selectors API (querySelector and querySelectorAll)
(In reply to comment #9)
> Also, I believe there are no namespace-aware tests in the Selectors test suite.
>  I don't see any in the jquery test suite either (which makes sense).  So we'd
> need to create those, right?

Correct, there aren't any in the jQuery suite.
Also for my reference, the real spec text to be reading:
http://dev.w3.org/2006/webapi/selectors-api/
Attached patch Slightly less rough (obsolete) — Splinter Review
This addresses items 1, 4, 5, 6 from comment 5.
Attachment #308988 - Attachment is obsolete: true
For my even later future reference, if we want to optimize things a little more once bug 75375 lands, we'd want to keep track of more previous sibling data (either on the heap, or with some stack finagling or something) and add the following to RuleProcessorData::GetNthIndex after the slot check:

  if (mPreviousSiblingData &&
      (!aIsOfType ||
       (mPreviousSiblingData->mNameSpaceID == mNameSpaceID &&
        mPreviousSiblingData->mContentTag == mContentTag))) {
    slot = mPreviousSiblingData->mNthIndices[aIsOfType][aIsFromEnd];
    if (slot != 0) {
      slot += (aIsFromEnd ? -1 : 1);
      NS_ASSERTION(slot != 0, "How did that happen?");
      return slot;
    }
  }
IE8 Beta 1's implementation of the selectors API ignores :link and :visited [1].  This seems to be allowed by the spec [2].  We should probably do the same or make the API pretend all links are unvisited, so if/when we fix bug 147777, we don't end up breaking sites that use the selectors API.

[1] http://code.msdn.microsoft.com/Release/ProjectReleases.aspx?ProjectName=ie8whitepapers&ReleaseId=574

[2] http://www.w3.org/TR/selectors-api/#privacy
I don't think we want to do what MS is doing, because we do want to give authors a way to ask for a list of all links.

I suppose we could make either :link match all links, or :link and :visited both match all links.  But I'd be tempted to just match the way we normally match, and change if/when bug 147777 gets fixed.

David, thoughts?
(In reply to comment #15)
> I suppose we could make either :link match all links, or :link and :visited
> both match all links.  But I'd be tempted to just match the way we normally
> match, and change if/when bug 147777 gets fixed.

I think matching as normal makes sense -- pages shouldn't break if we later change that to :link matching all links and :visited matching none.
Sounds reasonable.
Depends on: 429744
Attached patch Maybe even correct (obsolete) — Splinter Review
This still needs serious testing, and removal of all the XXX comments as tests are created.  But at this point it _might_ work correctly (including all the namespace/parsing exceptions as defined in the spec).
Attachment #310680 - Attachment is obsolete: true
Guys -

Would really like to get this targeted for mozilla1.9.1.

Any objections? I guess Boris would need to chime in here as to whether he thinks that is realistic. I can't help with the code, but I will send beer, gourmet coffee, etc. :-).

Cheers,

- Bill
The code is done.  What needs to happen is (in that order):

1)  The spec stops mutating, especially where the namespace resolver is involved.
2)  The tests for this get written.
3)  Any errors in the code found by said tests (I don't expect any, honestly)
    get fixed.

Sadly, neither IE nor Safari implement the namespace stuff at all, so the namespace resolver thing in the spec is only a burning issue for us, apparently.
I have a test suite nearly completed (working through the namespace resolver stuff now) - it'll bring about 2000 tests to the table. It should be ready today or tomorrow. I've been working with the Opera team, as well, to make sure that the suite is working correctly and that that it matches the spec.

I'm nominating for both wanted and blocking to 1.9.1 - it's pretty much essential that we get this out this year - all other browsers will have it on the table (in IE 8, in Safari 3, and will be in Opera 10) and not having it will make Firefox look really, really, bad in speed comparisons.
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Assignee: nobody → bzbarsky
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Priority: -- → P1
(In reply to comment #18)
> Created an attachment (id=316502) [details]

I'm having trouble applying this patch. Against the old (CVS) tree it applies but doesn't build - against the new (Mercurial) tree the patch doesn't apply. Which should I be working against - is there possible bitrot here?

The patch can't be applied in a vacuum to the CVS tree: you need to apply the three patches in the bugs this one depends on first.

And yes, it's definitely bitrotted against trunk by dbaron's selector checkins.  I plan to land the dependencies and update it to tip once I finally get an hg tree set up (today or tomorrow).
(In reply to comment #23)

Ok - that sounds great. Here's the test suite that I've pulled together (at a URL for now - I'll be happy to convert it into a Mochitest when the time comes):
http://ejohn.org/apps/selectortest/

It has about 2600 tests and covers all aspects of the specification:
http://dev.w3.org/2006/webapi/selectors-api-testsuite/Criteria.txt

I worked with Lachlan to make sure the suite was testing, and passing, as it should. The current numbers are:
 - Safari 3.1 (49.3% - No Fragment or Namespace support)
 - WebKit Nightly (51% - No Fragment or Namespace support)
 - Opera Gogi - "ACID3 Build" (76.7% - No Fragment support)
 - IE 8 (Can't run - the file is proper XHTML so it tries to download it.)

It's already caught a number of bugs in both Safari's and Opera's implementations (some in their CSS selector implementation, some in their querySelector implementation).
My tree is showing 73.8% on that page, but it doesn't include some of dbaron's selector fixes.  I'll look into the failures.  I assume I should just mail you and Lachlan directly about any that I think are due to bugs in the testsuite?
Just let me know directly - Lachlan was just my sounding board to make sure I was matching the spec properly.
So quick look says this about the failures:

6, 10, 546, 550: We convert |undefined| to "undefined", not to "".
7, 11, 547, 551: We don't allow skipping non-optional args.  I could fix this,
                 but I think this test is wrong.  Nothing in the spec requires
                 treating a missing argument as undefined.
75, 93, 192-200, 207-218, 225-229, 324, 328, 340-342, 387-394, 398, 400, 401:
   dbaron's patches should have fixed that on trunk.  I'll retest once I merge.
399: not sure (the test labeling is not very clear here), but might be fixed by
     dbaron as well.
599, 602, 605, 608, 622, 626, 634, 649, 653, 657, 661, 680, 684, 688, 703, 706, 709, 712, whole bunch of other tests that have a '|' before every tag name in
the selector: The tests are wrong.  There is no reason to resolve the default
              namespace in these tests, so we don't, but the test expects us
              to.
610-620, 637-647, 664-674, 691-701, other "bad resolver" tests: Not sure yet. 
     Having a way to run an individual test would be helpful here, but I can
     manage.
1020-1026: no idea what's up here; if I run those queries by hand I get
           SYNTAX_ERR.  Will look again after merging.

All the same applies to Document, of course.
(In reply to comment #27)
> 6, 10, 546, 550: We convert |undefined| to "undefined", not to "".
> 7, 11, 547, 551: We don't allow skipping non-optional args.  I could fix this,
>                  but I think this test is wrong.  Nothing in the spec requires
>                  treating a missing argument as undefined.

Both of these points are discussed here (below the block of green text):
http://dev.w3.org/2006/webapi/selectors-api/#nodeselector

> 75, 93, 192-200, 207-218, 225-229, 324, 328, 340-342, 387-394, 398, 400, 401:
>    dbaron's patches should have fixed that on trunk.  I'll retest once I merge.
> 399: not sure (the test labeling is not very clear here), but might be fixed by dbaron as well.

Most likely all part of the same grouping - but yeah, the labeling of some of these is a little off - I'll see if I can tweak it.

> 599, 602, 605, 608, 622, 626, 634, 649, 653, 657, 661, 680, 684, 688, 703, 706,
> 709, 712, whole bunch of other tests that have a '|' before every tag name in
> the selector: The tests are wrong.  There is no reason to resolve the default
>               namespace in these tests, so we don't, but the test expects us
>               to.

That's reasonable - I've changed the test to no longer take that into account (for those following along, this changes the resulting test numbers).

> 610-620, 637-647, 664-674, 691-701, other "bad resolver" tests: Not sure yet. 
>      Having a way to run an individual test would be helpful here, but I can
>      manage.

The bad namespace resolvers are, in order:
  {},
  null,
  undefined,
  function(){},
  function(){ return ""; },
  function(){ return null; },
  function(){ throw {message: "bad ERROR"} },
  { lookupNamespaceURI: function(){} },
  { lookupNamespaceURI: function(){ return ""; } },
  { lookupNamespaceURI: function(){ return null; } },
  { lookupNamespaceURI: function(){ throw {message: "bad ERROR"}; } }

All of which should throw NAMESPACE_ERR exceptions (except for the ones that actually throw exceptions - in which case those pass through).
> Both of these points are discussed here (below the block of green text):

The first one is.  And I agree that as the spec is written we have a bug and need to fix it.  The second point is not covered by the spec, nor do I think the behavior tested for here is "correct" (inferring undefined for non-optional args of host object methods).

> All of which should throw NAMESPACE_ERR exceptions

I'm not sure that {} should, in fact.  The spec doesn't cover this case very well, but here's the spec text from http://dev.w3.org/2006/webapi/selectors-api/#resolving as of today:

  An unresolvable namespace is a namespace that cannot be resolved because
  there was no NSResolver provided or the given NSResolver returns an empty
  string (or equivalent) for a prefix. When an unresolvable namespace is
  encountered, the implementation must raise a NAMESPACE_ERR exception ([DOM-
  LEVEL-3-CORE], section 1.4). 

This doesn't apply, since in this case there _is_ an NSResolver and it does NOT return empty string or equivalent.  Later on the spec says:

  If an exception is raised by the NSResolver while resolving namespaces,
  processing must be aborted and the exception propagated to the caller. 

That's what we do.  We get the "lookupNamespaceURI" property, get undefined, try to call it as a function, get an exception, and propagate it out.  We do throw NAMESPACE_ERR for null, return "", return null, but the test flags us as failing those anyway, for some reason.  Haven't sorted out why yet.  The "bad ERROR" case is technically a bug per spec in our code: we throw a "error thrown" exception instead of propagating through the one that the resolver threw; changing that will be fun xpconnect hackery...
(In reply to comment #29)
> The second point is not covered by the spec, nor do I think
> the behavior tested for here is "correct" (inferring undefined for non-optional
> args of host object methods).

What would you like the spec to define in this instance?  My original intention was for missing arguments to be treated as undefined, but this doesn't have to be the case.  Is there any specific error handling you think would be more appropriate here?

>> All of which should throw NAMESPACE_ERR exceptions
> 
> I'm not sure that {} should, in fact.  The spec doesn't cover this case very
> well, but here's the spec text from
> http://dev.w3.org/2006/webapi/selectors-api/#resolving as of today:
> 
>   An unresolvable namespace is a namespace that cannot be resolved because
>   there was no NSResolver provided or the given NSResolver returns an empty
>   string (or equivalent) for a prefix. When an unresolvable namespace is
>   encountered, the implementation must raise a NAMESPACE_ERR exception ([DOM-
>   LEVEL-3-CORE], section 1.4). 

DOM 3 Core defines a TYPE_MISMATCH_ERR which may be appropriate in this case, since the object passed doesn't match the expected type requirements.  If you agree, I can update the spec to require this.  If not, let me know what you would like the spec to say.
Honestly, I think missing non-optional arguments should throw an exception, but there are no existing standards as to what exception should be thrown, or even whether this is the right behavior.  Nor do I think there should be, necessarily.  If this spec insists on defining the behavior of this case I'm not sure we can implement it without seriously rearchitecting the way we do JS-to-C++ calls.

Note that in all cases here there _will_ be an exception; the only question is exactly what exception it is, and whether anyone cares.

> DOM 3 Core defines a TYPE_MISMATCH_ERR which may be appropriate in this case

How exactly do I tell that the object "doesn't match the type requirements"?  Would it match if there were a property called lookupNamespaceURI which wasn't a function?  If it were a function which I'm not allowed to call?  If it were a function but calling it threw an exception in the JS engine (before I ever got into the function body)?  Should these cases behave differently?  Why or why not?  For that matter, how do I tell which of those cases I'm in?

Note that all we're worrying about here is the exact exception code.  No matter what, there will be an exception.  Does the exact exception code matter?
(In reply to comment #30)
> (In reply to comment #29)
> > The second point is not covered by the spec, nor do I think
> > the behavior tested for here is "correct" (inferring undefined for non-optional
> > args of host object methods).
> 
> What would you like the spec to define in this instance?  My original intention
> was for missing arguments to be treated as undefined, but this doesn't have to
> be the case.  Is there any specific error handling you think would be more
> appropriate here?

I strongly agree with Boris that the Selectors API spec is the wrong place to define this; it should be defined in ECMAScript + http://www.w3.org/TR/DOM-Bindings/ .
(In reply to comment #31)
> Honestly, I think missing non-optional arguments should throw an exception, but
> there are no existing standards as to what exception should be thrown, or even
> whether this is the right behavior.  Nor do I think there should be,
> necessarily.  If this spec insists on defining the behavior of this case I'm
> not sure we can implement it without seriously rearchitecting the way we do
> JS-to-C++ calls.

I just want to know what you want the spec to say.  If leaving the spec as is
isn't acceptable and defining exactly what exception to throw isn't either,
then what is?  Perhaps this is something that should be addressed in WebIDL or
ECMAScript instead.

> > DOM 3 Core defines a TYPE_MISMATCH_ERR which may be appropriate in this case
> 
> How exactly do I tell that the object "doesn't match the type requirements"? 
> Would it match if there were a property called lookupNamespaceURI which wasn't
> a function?  If it were a function which I'm not allowed to call?  If it were a
> function but calling it threw an exception in the JS engine (before I ever got
> into the function body)?  Should these cases behave differently?  Why or why
> not?  For that matter, how do I tell which of those cases I'm in?

Good question.  That seems like it would be something for DOM Core to define. 
Since it doesn't, it probably isn't such a good idea to try and use it.

> Note that all we're worrying about here is the exact exception code.  No matter
> what, there will be an exception.  Does the exact exception code matter?

It probably doesn't matter, I can't imagine it being a major interop issue
having different exception codes for this edge case.  Would it be acceptable to
define that if any other error is encountered, an implementation specific
exception may be thrown?
> I just want to know what you want the spec to say.

If it feels the need to say something, I would be happy to have it say that an exception is raised if querySelector is called with no arguments.  But honestly, I don't think the spec needs changing here.  What I _don't_ want is the tests testing for something that isn't required by the spec: a specific exception code.

> Perhaps this is something that should be addressed in WebIDL or ECMAScript
> instead.

That sounds great to me.

> That seems like it would be something for DOM Core to define.

Agreed.  Again, I'm not saying the spec needs changing, just that the tests test things that are not necessarily defined yet.

> Would it be acceptable to define that if any other error is encountered, an
> implementation specific exception may be thrown?

Sounds fine to me.  ;)
I've made two changes to the test suite:
 - querySelector() and querySelectorAll() are just expected to throw exceptions (not a particular type of exception).
 - In order to get the default namespace from a resolver, null must now be passed in (instead of ""). This is in accordance with a change to the specification. 
John, you're still testing for the specific exceptions in cases when the spec requires them (SYNTAX_ERR and NAMESPACE_ERR), right?  As well as the "propagate the NSResolver exception" behavior?

As far as getting the default namespace, I'm not sure I see where that's tested.  None of the resolvers in goodNamespace actually return anything for null.  Is there some test here that actually has a namespace resolver that returns a non-empty default namespace and then checks that the right nodes end up returned by querySelector?

By the way, if I haven't said this yet, thanks a ton for getting these tests together!
(In reply to comment #36)
> John, you're still testing for the specific exceptions in cases when the spec
> requires them (SYNTAX_ERR and NAMESPACE_ERR), right?  As well as the "propagate
> the NSResolver exception" behavior?

Correct, yes, I'm still testing for all of those. The only case that I changed was there qs or qsa were called with no arguments (I expect an exception, just not a specific one).

Amusingly, this now means that we pass 16 tests in Firefox 3 (since we "correctly" throw an exception when trying to access that method). I guess I could determine if it's a "method not found" error and exclude that - but I'm not sure if we want to dictate that.

> As far as getting the default namespace, I'm not sure I see where that's
> tested.  None of the resolvers in goodNamespace actually return anything for
> null.  Is there some test here that actually has a namespace resolver that
> returns a non-empty default namespace and then checks that the right nodes end
> up returned by querySelector?

Good point! I just doubled the number of "Good Resolver" tests. There are now 4 that have no default handler (return undefined) and 4 that have a default handler (return "http://www.w3.org/1999/xhtml"). To get the default handler you must pass in null, as mentioned before.

> By the way, if I haven't said this yet, thanks a ton for getting these tests
> together!

Thanks - no problem!

The only use case that I hadn't explicitly covered was the ability to query DOMs that were disconnected from the document. I had done this with DocumentFragment but not with disconnected elements. I just added this to the suite, increasing the number of tests to 4200+. In doing so I caught a bug with how I was doing some of the query analysis (I was checking the computed style of the elements - which wouldn't work for elements that are disconnected from the DOM) and fixed it, as well.
Attached patch Ready for review (obsolete) — Splinter Review
This passes about 91% of John's tests.  The ones it fails have to do mostly with the way xpconnect stringifies |undefined| to "undefined", whereas selectors API expects it to stringify to "".  The other issues are that we throw a custom exception if the resolver is {} and that we don't properly propagate the exception from the resolver.  We pass everything else.

Oh, and we're about 15% faster than Webkit on <http://webkit.org/perf/slickspeed/>.  ;)

Review notes:  This patch has the CSS changes first, then the DOM changes that make use of them.  I'd like jst to look over the latter especially, and David over the former.  Other than the CSS parser changes and glue to call selector parsing from the DOM, the CSS changes have to do with either allowing RuleProcessorDatas to be allocated without a prescontext (needed to make this all work in display:none iframes) or with a performance optimization for :nth-of-type selectors (the GetNthIndex changes).
Attachment #316502 - Attachment is obsolete: true
Attachment #329168 - Flags: superreview?
Attachment #329168 - Flags: review?(dbaron)
Attachment #329168 - Flags: superreview? → superreview?(jst)
(In reply to comment #38)
> This passes about 91% of John's tests.  The ones it fails have to do mostly
> with the way xpconnect stringifies |undefined| to "undefined", whereas
> selectors API expects it to stringify to "".  The other issues are that we

That seems like an odd expectation (unless I'm misunderstanding something), given that ECMA-262 ed. 3, section 9.8, says that converting undefined to a string yields "undefined" (see table at the top of page 42).
Yeah, well.  http://dev.w3.org/2006/webapi/selectors-api/#convert-value says:

  While resolving either a prefix or the default namespace, if the
  lookupNamespaceURI() method returns undefined or an empty string, the
  implementation must act as if null had been returned.

Of course since the return value is a DOMString, it's not clear to me what "returns undefined" would really mean.  I brought this up on the mailing list. If you feel that this requirement is unreasonable, it might be worth mentioning that too.
Attached patch Rip out namespace resolver (obsolete) — Splinter Review
Attachment #329168 - Attachment is obsolete: true
Attachment #329727 - Flags: superreview?(jst)
Attachment #329727 - Flags: review?(dbaron)
Attachment #329168 - Flags: superreview?(jst)
Attachment #329168 - Flags: review?(dbaron)
Comment on attachment 329727 [details] [diff] [review]
Rip out namespace resolver

- In ParseSelectorList():

+  rv = parser->ParseSelectorString(aSelectorString,
+                                   doc->GetDocumentURI(),
+                                   aSelectorList);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // ParseSelectorString can execute script.  Throw if we got moved to
+  // a different document.
+  if (doc != aNode->GetOwnerDoc()) {
+    return NS_ERROR_DOM_SECURITY_ERR;

We should probably release *aSelectorList here and null it out, just in case.

Other than that this looks really good! I'm assuming we're working on getting the tests into mochitest, or some way to run them on tinderboxes right?
Attachment #329727 - Flags: superreview?(jst) → superreview+
Comment on attachment 329727 [details] [diff] [review]
Rip out namespace resolver

It might be good to at least comment that ParseSelectorString should
have a line-number parameter, if not actually add it.  (We really ought
to propagate line numbers from script to these methods at some point
soon...)

I wonder whether the InitScanner in ParseSelectorString (and in
ParseColorString) should actually be passing null for the sheet URL.

+  /* Find and return the correct namespace ID for the prefix aPrefix.
+     If the prefix cannot be resolved to a namespace, this method will
+     report the error and return false without changing aErrorCode.
+     In error conditions, it will set aErrorCode and return false.
+
+     This method never returns kNameSpaceID_Unknown or
+     kNameSpaceID_None for aNameSpaceID while returning true.
+  */ 

This seems a little garbled.  In particular, it claims that aErrorCode
will be untouched on a false return (lines 2-3), and then claims that
aErrorCode is set on all false returns (line 4), when in fact it touches
aErrorCode iff it returns false and mUnresolvablePrefixException is
true.

I'm guessing this made more sense back with the NSResolver stuff, but it
seems like SetDefaultNamespaceOnSelector should return void and not take
an aErrorCode parameter.  The comment describing it should also be fixed.

+    return aSlot;;

only need one ";"

+  PRInt32 GetNthIndex(PRBool aIsOfType, PRBool aIsFromEnd);

don't add a declaration for a method overload that you're not defining

(and while you're there, how about keeping GetLang after all the
allocation methods, where it used to be, rather than moving it?)

[ I'm guessing these were merge problems. ]

Don't the calls to IsHTMLLink and IsLink in RuleProcessorData's
constructor need dto deal with mPresContext being null (and pass a null
link handler if it is)?

In RuleProcessorData::GetNthIndex, please assert that
!mPreviousSiblingData ||
mPreviousSiblingData->mContent->IsNodeOfType(nsINode::eELEMENT).

I think I would prefer removing the default "= PR_TRUE" for the
aForStyling parameters (to both SelectorMatches and
SelectorMatchesTree); having default parameters means you risk not
passing it through from place-to-place.

It seems like nsNodeSelectorTearoff could have a private and non-virtual
destructor (rather than protected and virtual).  And probably the same
for nsStaticContentList.

nsStaticContentList::GetLength and Item should follow local bracing
style with the { opening the function on its own line.

+  // ParseSelectorString can execute script.  Throw if we got moved to
+  // a different document.

Dare I ask how it can execute script?

The huge DO_FOR_EACH_ELEMENT macro in nsGenericElement.cpp is pretty
ugly.  Why not make it a function that's given a callback?  The function
pointer overhead shouldn't be significant given the number of other
virtual function calls, and the code will be smaller.  (The return value
for the callback says whether to continue.)  And while you're there,
could you make this code use its own stack rather than recursion, so
that it can handle deep content trees?

I'd like to have one last look over the replacement of
DO_FOR_EACH_ELEMENT -- or if there's a good reason to keep it -- a
closer look at it.

(I wonder why nsDocumentFragment.cpp has the Node3Tearoff MAP_ENTRY when
it would inherit that from nsGenericElement.)

Are there any interesting issues with running selector matching on
nodes inside document fragments?

And yes, we do want the tests as mochitests!
Attachment #329727 - Flags: review?(dbaron) → review+
> We should probably release *aSelectorList here and null it out, just in case.

As dbaron points out, this code is no longer needed, so I've removed it.

And yes, we're working on getting the test into mochitests.
(In reply to comment #43)
> It might be good to at least comment that ParseSelectorString should
> have a line-number parameter, if not actually add it.

Added.

> I wonder whether the InitScanner in ParseSelectorString (and in
> ParseColorString) should actually be passing null for the sheet URL.

I wonder whether we should be doing error reporting here at all, to be honest.  But if we are, we want to report it for the calling script URI, right?  I'm using the document URI for now, but maybe I should change that.

> This seems a little garbled.  In particular, it claims that aErrorCode
> will be untouched on a false return (lines 2-3), and then claims that
> aErrorCode is set on all false returns (line 4)

I'll rip out more of the NSResolver legacy here.

> I'm guessing this made more sense back with the NSResolver stuff, but it
> seems like SetDefaultNamespaceOnSelector should return void

Done.

> only need one ";"

Done.

> +  PRInt32 GetNthIndex(PRBool aIsOfType, PRBool aIsFromEnd);

Removed, and fixed GetLang().  And yes, this was a merge issue.

> Don't the calls to IsHTMLLink and IsLink in RuleProcessorData's
> constructor need dto deal with mPresContext being null (and pass a null
> link handler if it is)?

Yep.  They do.

> In RuleProcessorData::GetNthIndex, please assert that
> !mPreviousSiblingData ||
> mPreviousSiblingData->mContent->IsNodeOfType(nsINode::eELEMENT).

Done.

> I think I would prefer removing the default "= PR_TRUE"

Done.

> It seems like nsNodeSelectorTearoff could have a private and non-virtual
> destructor (rather than protected and virtual).  And probably the same
> for nsStaticContentList.

Yeah, indeed.

> nsStaticContentList::GetLength and Item should follow local bracing
> style with the { opening the function on its own line.

Fixed.

> +  // ParseSelectorString can execute script.  Throw if we got moved to
> +  // a different document.
> 
> Dare I ask how it can execute script?

NSResolver leftovers.  Removed.

> The huge DO_FOR_EACH_ELEMENT macro in nsGenericElement.cpp is pretty
> ugly.  Why not make it a function that's given a callback?

Done.  I wrote it as I did due to performance worries, but I think that was silly, as you point out.  I tested, and there is no appreciable performance difference.

> And while you're there,
> could you make this code use its own stack rather than recursion, so
> that it can handle deep content trees?

I'd prefer to do this as a separate patch, if that's ok.  We have a lot of recursive code that walks the DOM tree, and it could all use some sort of combined solution.  The obvious one involves either lots of reallocation or heap allocation or something clever, because the kids of a node need to come after the node and before its next sibling, so as you go through the tree you have to insert new things in the middle of the list of nodes to process.

> (I wonder why nsDocumentFragment.cpp has the Node3Tearoff MAP_ENTRY when
> it would inherit that from nsGenericElement.)

Looks like that line didn't get removed when the INHERITING thing was added.  It can probably just go.

> Are there any interesting issues with running selector matching on
> nodes inside document fragments?

There shouldn't be, other than matching of :root.

I'll post the updated patch, then work on getting the tests into mochitestable form.
Attached patch Updated patch (obsolete) — Splinter Review
David, can you take a look at the DO_FOR_EACH_ELEMENT replacement?
Attachment #329727 - Attachment is obsolete: true
Attachment #330655 - Flags: review?(dbaron)
Comment on attachment 330655 [details] [diff] [review]
Updated patch

>+    if (aParentData) {
>+      data->mParentData = nsnull;
>+    }

Shouldn't you clear the mParentData of prevSiblingData instead?  The mParentData of the mPrevivousSiblingData could be useful.

You'd then need to do the same clearing before the
+    prevSibling->~RuleProcessorData();
at the very end of the function, too, though.

It took me a while to wrap my head around the object management in TryMatchingElementsInSubtree, but I guess I can live with it.

r+sr=dbaron on the new nsGenericElement changes
Attachment #330655 - Flags: superreview+
Attachment #330655 - Flags: review?(dbaron)
Attachment #330655 - Flags: review+
Then again, the change I suggest probably isn't worth the trouble, since it would only matter in pretty rare cases.
> The mParentData of the mPrevivousSiblingData could be useful.

Ah, indeed.  I'll make that change before checking in.
The tests still need serious work, but they're what I have right now.
Attachment #330655 - Attachment is obsolete: true
Checked in.  jresig said he'll look at the tests some tomorrow.
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
The status messages for these tests confuse the tinderbox error parser:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1216713585.1216719279.12712.gz

Could we change the format slightly? You're matching /\WError: / here:
http://mxr.mozilla.org/mozilla/source/webtools/tinderbox/ep_unittest.pl#31

So simply changing "Syntax Error:" to "SyntaxError:" would make this go away.
Made that change.
Depends on: 448161
Depends on: 453307
Depends on: 482394
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.