Bug 142703 - Use filterRootId in SelectorQuery even if CSS JIT is not enabled
Summary: Use filterRootId in SelectorQuery even if CSS JIT is not enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 142826
  Show dependency treegraph
 
Reported: 2015-03-14 18:01 PDT by Yusuke Suzuki
Modified: 2015-03-18 21:06 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.85 KB, patch)
2015-03-14 20:11 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (8.02 KB, patch)
2015-03-14 20:18 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (1.92 KB, patch)
2015-03-18 19:57 PDT, Yusuke Suzuki
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-03-14 18:01:49 PDT
Use filterRootId in SelectorQuery even if CSS JIT is not enabled
Comment 1 Yusuke Suzuki 2015-03-14 20:11:54 PDT
Created attachment 248661 [details]
Patch
Comment 2 Yusuke Suzuki 2015-03-14 20:18:51 PDT
Created attachment 248662 [details]
Patch
Comment 3 Benjamin Poulain 2015-03-15 14:54:13 PDT
Comment on attachment 248662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248662&action=review

I guess that could be useful when the JIT is disabled.

> Source/WebCore/ChangeLog:16
> +        - fast/selectors/filter-root-node-with-selector-contains-adjacents.html
> +        - fast/selectors/querySelector-id-filtering.html
> +        - fast/selectors/querySelector-scope-filtered-root.html

If that's not already covered, it would be good to add tests for:
-Failures on selectorForIdLookup()'s rootNode.inDocument() (Element.matches() on a detached element).
-Failures on selectorForIdLookup()'s rootNode.document().inQuirksMode().
-Tests for CompilableSingleWithRootFilter and CompilableSingle.
with cases that compiles with the CSS JIT, and cases that fail to compile.

> Source/WebCore/dom/SelectorQuery.cpp:535
> +
> +

Two blank lines here.
Comment 4 Yusuke Suzuki 2015-03-16 20:43:53 PDT
Comment on attachment 248662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248662&action=review

Thank you for your review!

>> Source/WebCore/ChangeLog:16
>> +        - fast/selectors/querySelector-scope-filtered-root.html
> 
> If that's not already covered, it would be good to add tests for:
> -Failures on selectorForIdLookup()'s rootNode.inDocument() (Element.matches() on a detached element).
> -Failures on selectorForIdLookup()'s rootNode.document().inQuirksMode().
> -Tests for CompilableSingleWithRootFilter and CompilableSingle.
> with cases that compiles with the CSS JIT, and cases that fail to compile.

Great! I'll add them.

>> Source/WebCore/dom/SelectorQuery.cpp:535
>> +
> 
> Two blank lines here.

Oops, thank you!
Comment 5 Yusuke Suzuki 2015-03-18 10:31:09 PDT
http://trac.webkit.org/changeset/181699 Landed with tests.
Accidentally webkit-patch crashed, I've closed this issue manually.
Comment 6 Alexey Proskuryakov 2015-03-18 12:10:10 PDT
This broke the build with some compilers, because it doesn't make sense to FALLTHROUGH after an ASSERT.

Tim made a temporary fix in http://trac.webkit.org/changeset/181706, please fix this properly.
Comment 7 Yusuke Suzuki 2015-03-18 18:57:30 PDT
(In reply to comment #6)
> This broke the build with some compilers, because it doesn't make sense to
> FALLTHROUGH after an ASSERT.
> 
> Tim made a temporary fix in http://trac.webkit.org/changeset/181706, please
> fix this properly.

Thank you! I'll fix it soon.
Comment 8 Yusuke Suzuki 2015-03-18 19:09:59 PDT
OK, I've found that, when !ENABLE(CSS_SELECTOR_JIT) and --debug, both FALLTHROUGH and ASSERT_NOT_REACHED are materialized and raise this compile error.

Here, ASSERT_NOT_REACHED is correct because this CompiledSingle and CompiledSingleWithRootFilter is not set when !ENABLE(CSS_SELECTOR_JIT).
So I'll fix it by dropping this FALLTHROUGH in !ENABLE(CSS_SELECTOR_JIT) build.
Comment 9 Yusuke Suzuki 2015-03-18 19:57:29 PDT
Created attachment 249009 [details]
Patch
Comment 10 Yusuke Suzuki 2015-03-18 21:06:32 PDT
Committed r181725: <http://trac.webkit.org/changeset/181725>