100812 – [Shadow] Implement custom pseudo elements styling

RESOLVED FIXED 100812

[Shadow] Implement custom pseudo elements styling

https://bugs.webkit.org/show_bug.cgi?id=100812

Summary [Shadow] Implement custom pseudo elements styling

Shinya Kawanaka

Reported 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.

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Shinya Kawanaka

Comment 1 2012-10-30 23:51:25 PDT

I would like to implement this like shadowPseudoId, since this also breaks shadow boundary.

Shinya Kawanaka

Comment 2 2012-10-31 02:06:33 PDT

Created attachment 171601 [details] WIP

Shinya Kawanaka

Comment 3 2012-10-31 02:08:02 PDT

This contains a lot of debug-printf... But it succeeds in styling custom pseudo elements.

Dimitri Glazkov (Google)

Comment 4 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.

Dimitri Glazkov (Google)

Comment 5 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?

Shinya Kawanaka

Comment 6 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...

Shinya Kawanaka

Comment 7 2012-10-31 22:36:51 PDT

Created attachment 171769 [details] Patch

Shinya Kawanaka

Comment 8 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.

Build Bot

Comment 9 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

WebKit Review Bot

Comment 10 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

Early Warning System Bot

Comment 11 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

Peter Beverloo (cr-android ews)

Comment 12 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

Early Warning System Bot

Comment 13 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

EFL EWS Bot

Comment 14 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

Build Bot

Comment 15 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

Shinya Kawanaka

Comment 16 2012-11-02 02:10:52 PDT

Created attachment 172009 [details] Patch

Hajime Morrita

Comment 17 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?

Early Warning System Bot

Comment 18 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

Hajime Morrita

Comment 19 2012-11-02 02:26:51 PDT

Comment on attachment 172009 [details] Patch (cq- for now just in case.)

Hajime Morrita

Comment 20 2012-11-02 02:27:27 PDT

Comment on attachment 172009 [details] Patch hmm. let's fix build breakage...

Shinya Kawanaka

Comment 21 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

Shinya Kawanaka

Comment 22 2012-11-02 23:13:06 PDT

Created attachment 172210 [details] Patch

Shinya Kawanaka

Comment 23 2012-11-04 17:54:45 PST

Created attachment 172251 [details] Patch

WebKit Review Bot

Comment 24 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>

WebKit Review Bot

Comment 25 2012-11-04 19:05:28 PST

All reviewed patches have been landed. Closing bug.

Ojan Vafai

Comment 26 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

Ojan Vafai

Comment 27 2012-11-06 10:56:22 PST

Another graph that shows the regression: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/intl1/report.html?rev=166182&graph=V8.MemoryHeapSampleTotalUsed_0.75&trace=V8.MemoryHeapSampleTotalUsed_0.75&history=150 In theory, the regression could also be from http://trac.webkit.org/changeset/133429/, but I don't see how.

Ojan Vafai

Comment 28 2012-11-07 09:50:28 PST

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

Dimitri Glazkov (Google)

Comment 29 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? :)

Ojan Vafai

Comment 30 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?

Shinya Kawanaka

Comment 31 2012-11-07 17:55:17 PST

Hmm... It's hard for me to imagine this causes such memory regression...

Shinya Kawanaka

Comment 32 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.

Ojan Vafai

Comment 33 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.

Shinya Kawanaka

Comment 34 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.

Shinya Kawanaka

Comment 35 2012-11-07 20:53:48 PST

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

Ojan Vafai

Comment 36 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/.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component CSS

Assignee

Shinya Kawanaka

Reported

2012-10-30 21:56 PDT

Modified

2012-11-07 22:10 PST History

CC List

10 users Show

URL

Keywords

Depends on

100831

Blocks

101170 101533

Dependencies

tree graph