Bug 100812 - [Shadow] Implement custom pseudo elements styling
Summary: [Shadow] Implement custom pseudo elements styling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on: 100831
Blocks: 101170 101533
  Show dependency treegraph
 
Reported: 2012-10-30 21:56 PDT by Shinya Kawanaka
Modified: 2012-11-07 22:10 PST (History)
10 users (show)

See Also:


Attachments
WIP (12.34 KB, patch)
2012-10-31 02:06 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (4.27 KB, patch)
2012-10-31 22:36 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (4.27 KB, patch)
2012-11-02 02:10 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (4.94 KB, patch)
2012-11-02 23:13 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (4.96 KB, patch)
2012-11-04 17:54 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-10-30 21:56:51 PDT
Let's consider the following tree.

div#host -- SR
             |-span (pseudo='x-foo')

When we have a CSS selector #host:x-foo, it should match the span.
Comment 1 Shinya Kawanaka 2012-10-30 23:51:25 PDT
I would like to implement this like shadowPseudoId, since this also breaks shadow boundary.
Comment 2 Shinya Kawanaka 2012-10-31 02:06:33 PDT
Created attachment 171601 [details]
WIP
Comment 3 Shinya Kawanaka 2012-10-31 02:08:02 PDT
This contains a lot of debug-printf... But it succeeds in styling custom pseudo elements.
Comment 4 Dimitri Glazkov (Google) 2012-10-31 10:22:04 PDT
I don't understand why you're implementing this again? UnknownPseudoElements and shadowPseudoIds are exactly what you need already.
Comment 5 Dimitri Glazkov (Google) 2012-10-31 10:28:16 PDT
Comment on attachment 171601 [details]
WIP

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

> Source/WebCore/css/CSSSelector.cpp:395
> +    if (name.startsWith("x-"))

I could be wrong, but it should be easier to make this work at the attribute-setting time, right?
Comment 6 Shinya Kawanaka 2012-10-31 18:33:25 PDT
First I thought I would like to use the shadowPseudoId, but I was afraid that it causes some confusion.
But maybe it's OK to use it anyway...
Comment 7 Shinya Kawanaka 2012-10-31 22:36:51 PDT
Created attachment 171769 [details]
Patch
Comment 8 Shinya Kawanaka 2012-10-31 22:37:32 PDT
This patch depends on https://bugs.webkit.org/show_bug.cgi?id=100831, so it won't run until it's landed.
Comment 9 Build Bot 2012-10-31 22:42:21 PDT
Comment on attachment 171769 [details]
Patch

Attachment 171769 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14692355
Comment 10 WebKit Review Bot 2012-10-31 22:43:24 PDT
Comment on attachment 171769 [details]
Patch

Attachment 171769 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14677391
Comment 11 Early Warning System Bot 2012-10-31 22:45:27 PDT
Comment on attachment 171769 [details]
Patch

Attachment 171769 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14697180
Comment 12 Peter Beverloo (cr-android ews) 2012-10-31 22:47:10 PDT
Comment on attachment 171769 [details]
Patch

Attachment 171769 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14678390
Comment 13 Early Warning System Bot 2012-10-31 22:47:34 PDT
Comment on attachment 171769 [details]
Patch

Attachment 171769 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14684376
Comment 14 EFL EWS Bot 2012-10-31 22:51:52 PDT
Comment on attachment 171769 [details]
Patch

Attachment 171769 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14687379
Comment 15 Build Bot 2012-10-31 23:05:08 PDT
Comment on attachment 171769 [details]
Patch

Attachment 171769 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14561501
Comment 16 Shinya Kawanaka 2012-11-02 02:10:52 PDT
Created attachment 172009 [details]
Patch
Comment 17 Hajime Morrita 2012-11-02 02:25:12 PDT
Comment on attachment 172009 [details]
Patch

The change looks good.
In my understanding, we currently allows arbitrary name for pseudos. What is our plan to reject invalid ones?
Comment 18 Early Warning System Bot 2012-11-02 02:26:07 PDT
Comment on attachment 172009 [details]
Patch

Attachment 172009 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14686775
Comment 19 Hajime Morrita 2012-11-02 02:26:51 PDT
Comment on attachment 172009 [details]
Patch

(cq- for now just in case.)
Comment 20 Hajime Morrita 2012-11-02 02:27:27 PDT
Comment on attachment 172009 [details]
Patch

hmm. let's fix build breakage...
Comment 21 Shinya Kawanaka 2012-11-02 22:42:40 PDT
(In reply to comment #17)
> (From update of attachment 172009 [details])
> The change looks good.
> In my understanding, we currently allows arbitrary name for pseudos. What is our plan to reject invalid ones?

Yes.

I would like to do some refactoring (https://bugs.webkit.org/show_bug.cgi?id=100826) first.
Then it should become easy to reject invalid ones. I'll work it in https://bugs.webkit.org/show_bug.cgi?id=100919
Comment 22 Shinya Kawanaka 2012-11-02 23:13:06 PDT
Created attachment 172210 [details]
Patch
Comment 23 Shinya Kawanaka 2012-11-04 17:54:45 PST
Created attachment 172251 [details]
Patch
Comment 24 WebKit Review Bot 2012-11-04 19:05:22 PST
Comment on attachment 172251 [details]
Patch

Clearing flags on attachment: 172251

Committed r133428: <http://trac.webkit.org/changeset/133428>
Comment 25 WebKit Review Bot 2012-11-04 19:05:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Ojan Vafai 2012-11-06 10:19:09 PST
I'm not sure how, but I think this patch may have caused a 2% increase in memory usage on the Chromium page cyclers.

http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl2/report.html?rev=165925&graph=ws_single_peak_r&history=100

Regression range: http://trac.webkit.org/log/?verbose=on&rev=133430&stop_rev=133428
Comment 29 Dimitri Glazkov (Google) 2012-11-07 09:55:04 PST
(In reply to comment #28)
> Another regressions that are either from this patch or http://trac.webkit.org/changeset/133429/:
> 
> http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?rev=166198&graph=CreateNodes&history=100

Time for some speculative rollouts? :)
Comment 30 Ojan Vafai 2012-11-07 10:28:42 PST
I tried to roll this out, but couldn't due to conflicts with http://trac.webkit.org/changeset/133749. Could someone more familiar with these patches give it a shot?
Comment 31 Shinya Kawanaka 2012-11-07 17:55:17 PST
Hmm...
It's hard for me to imagine this causes such memory regression...
Comment 32 Shinya Kawanaka 2012-11-07 18:00:42 PST
(In reply to comment #31)
> Hmm...
> It's hard for me to imagine this causes such memory regression...

Let me try to roll this out anyway.
I hope this is innocent, though.
Comment 33 Ojan Vafai 2012-11-07 18:27:46 PST
Thanks! If it turns out to be Dimitri's patch, r=me on relanding this. We'll know once the rollout goes in.
Comment 34 Shinya Kawanaka 2012-11-07 20:05:49 PST
Reverted.
https://bugs.webkit.org/show_bug.cgi?id=101533

Let's see the page cycler for a while.
Comment 36 Ojan Vafai 2012-11-07 22:10:48 PST
(In reply to comment #35)
> This patch seems innocent?

Yes. Sorry for the misplaced blame. Thanks for trying the rollout. It must be http://trac.webkit.org/changeset/133429/.