Bug 135200

Summary: compile window-inactive and fullscreen pseudo classes
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: CSSAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
benjamin: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 none

Description Alex Christensen 2014-07-23 09:21:58 PDT
I fixed a bug with window-inactive yesterday, so why not compile it, too?
Comment 1 Alex Christensen 2014-07-23 09:23:42 PDT
Created attachment 235353 [details]
Patch
Comment 2 Alex Christensen 2014-07-23 10:04:59 PDT
I was on a roll and compiled the fullscreen pseudo classes, too.
Comment 3 Alex Christensen 2014-07-23 10:12:23 PDT
Created attachment 235355 [details]
Patch
Comment 4 Alex Christensen 2014-07-23 10:13:29 PDT
Created attachment 235356 [details]
Patch
Comment 5 Alex Christensen 2014-07-23 10:38:58 PDT
Created attachment 235358 [details]
Patch
Comment 6 Alex Christensen 2014-07-23 10:40:51 PDT
https://i.imgflip.com/ajjz5.jpg
Comment 7 Benjamin Poulain 2014-07-23 11:05:37 PDT
Comment on attachment 235358 [details]
Patch

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

Awesome!

r- because I have serious doubt about PseudoClassAnyLink, the patch looks good otherwise.

> Source/WebCore/css/SelectorChecker.cpp:484
> +        } else if (selector->pseudoClassType() == CSSSelector::PseudoClassWindowInactive)
> +            return isWindowInactive(element);

Can't you juste remove this code? This is handled properly by the switch() below.

> Source/WebCore/cssjit/SelectorCompiler.cpp:409
> +            return FunctionType::SimpleSelectorChecker;

Indentation issue here.

> Source/WebCore/cssjit/SelectorCompiler.cpp:415
> +            return FunctionType::SimpleSelectorChecker;

ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:-417
> -        fragment.pseudoClasses.add(CSSSelector::PseudoClassLink);
> -        return FunctionType::SimpleSelectorChecker;
> -

This does not look right. There is no handler for PseudoClassAnyLink, it is aliased with the handler of PseudoClassLink.

> Source/WebCore/cssjit/SelectorCompiler.cpp:573
> +        return FunctionType::CannotCompile;

I think CannotMatchAnything would make sense.
Comment 8 Alex Christensen 2014-07-23 11:12:28 PDT
Created attachment 235361 [details]
Patch
Comment 9 Build Bot 2014-07-23 12:17:00 PDT
Comment on attachment 235361 [details]
Patch

Attachment 235361 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6743218150440960

New failing tests:
media/track/add-and-remove-track.html
Comment 10 Build Bot 2014-07-23 12:17:02 PDT
Created attachment 235366 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 11 Alex Christensen 2014-07-23 13:20:59 PDT
test failure probably not from this change.

https://trac.webkit.org/r171486