62447 – dump-as-markup.js should support shadow tree

RESOLVED FIXED 62447

dump-as-markup.js should support shadow tree

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

Summary dump-as-markup.js should support shadow tree

Hajime Morrita

Reported 2011-06-10 03:55:12 PDT

In some case, we need to dump shadow tree as a part of markup dump.

Attachments
Patch (6.02 KB, patch) 2011-07-04 22:16 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (6.06 KB, patch) 2011-07-05 01:13 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (6.18 KB, patch) 2011-07-05 01:55 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (6.19 KB, patch) 2011-07-05 18:54 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (6.11 KB, patch) 2011-07-06 21:26 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (9.95 KB, patch) 2011-07-07 00:34 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Asking for cq+ (10.25 KB, patch) 2011-07-07 18:52 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Patch (10.25 KB, patch) 2011-07-07 19:04 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Hajime Morrita

Comment 1 2011-07-04 22:16:26 PDT

Created attachment 99669 [details] Patch

Hajime Morrita

Comment 2 2011-07-04 22:17:05 PDT

Ryosuke, Dimitri, could you take a look?

Ryosuke Niwa

Comment 3 2011-07-04 23:46:37 PDT

Comment on attachment 99669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99669&action=review > LayoutTests/ChangeLog:6 > + Added Markup.showShadows() to enable shadow-related dump. Long description goes after "Reviewed by" > LayoutTests/resources/dump-as-markup.js:214 > + var pseudoId = internals.shadowPseudoId(node); > + if (pseudoId) You should check the existence of internals so that opening the test manually doesn't throw errors.

Hajime Morrita

Comment 4 2011-07-05 01:13:22 PDT

Created attachment 99676 [details] Patch

Hajime Morrita

Comment 5 2011-07-05 01:14:37 PDT

Comment on attachment 99669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99669&action=review HI Ryosuke, thank you for reviewing quickly! I updated the patch to address the points. >> LayoutTests/ChangeLog:6 >> + Added Markup.showShadows() to enable shadow-related dump. > > Long description goes after "Reviewed by" Oops. Fixed. >> LayoutTests/resources/dump-as-markup.js:214 >> + if (pseudoId) > > You should check the existence of internals so that opening the test manually doesn't throw errors. Fixed.

Ryosuke Niwa

Comment 6 2011-07-05 01:25:42 PDT

Comment on attachment 99676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99676&action=review > LayoutTests/fast/dom/HTMLMeterElement/meter-element-markup.html:1 > +<html> DOCTYPE? > LayoutTests/fast/dom/HTMLProgressElement/progress-element-markup.html:1 > +<html> Ditto about DOCTYPE. > LayoutTests/resources/dump-as-markup.js:224 > + str += "<@shadowRoot>"; I'm not convinced that prepending @ is a good syntax here. Maybe use shadow as a namespace as in <shadow:root shadow:pseudoId=id>?

Hajime Morrita

Comment 7 2011-07-05 01:55:14 PDT

Created attachment 99683 [details] Patch

Hajime Morrita

Comment 8 2011-07-05 01:56:58 PDT

Thanks for the second round... (In reply to comment #6) > (From update of attachment 99676 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99676&action=review > > > LayoutTests/fast/dom/HTMLMeterElement/meter-element-markup.html:1 > > +<html> > > DOCTYPE? Done. > > > LayoutTests/fast/dom/HTMLProgressElement/progress-element-markup.html:1 > > +<html> > > Ditto about DOCTYPE. Done. > > > LayoutTests/resources/dump-as-markup.js:224 > > + str += "<@shadowRoot>"; > > I'm not convinced that prepending @ is a good syntax here. Maybe use shadow as a namespace as in <shadow:root shadow:pseudoId=id>? I have no good idea here.... Okay, let's adopt the namespace style.

Ojan Vafai

Comment 9 2011-07-05 16:10:34 PDT

Comment on attachment 99683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99683&action=review Should we just always dump shadows? How many existing tests would be affected by doing that? > LayoutTests/resources/dump-as-markup.js:129 > +Markup.showShadows = function() > +{ > + Markup._showShadows = true; Bikeshed nit: I find showShadows a bit confused. show implies to me that it's messing with visibility or display state. Maybe call this dumpShadows?

Hajime Morrita

Comment 10 2011-07-05 18:54:35 PDT

Created attachment 99776 [details] Patch

Hajime Morrita

Comment 11 2011-07-05 18:57:15 PDT

> > LayoutTests/resources/dump-as-markup.js:129 > > +Markup.showShadows = function() > > +{ > > + Markup._showShadows = true; > > Bikeshed nit: I find showShadows a bit confused. show implies to me that it's messing with visibility or display state. Maybe call this dumpShadows? Done. I have a opinion here, but I'd just pick my bike today ;-o

Ojan Vafai

Comment 12 2011-07-06 16:18:38 PDT

Comment on attachment 99776 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99776&action=review Are you opposed to trying just always dumping the shadow tree? I really don't see why we need a flag to only do it sometimes. > LayoutTests/resources/dump-as-markup.js:215 > + str += Markup._indent(depth + 1) + 'shadow:shadowPseudoId="' + pseudoId + '"'; Can this be shadow:pseudoId and shadow:root as Ryosuke suggested? Repeating "shadow" seems unnecessary.

Hajime Morrita

Comment 13 2011-07-06 21:26:45 PDT

Created attachment 99926 [details] Patch

Hajime Morrita

Comment 14 2011-07-06 21:29:01 PDT

(In reply to comment #12) > (From update of attachment 99776 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99776&action=review > > Are you opposed to trying just always dumping the shadow tree? I really don't see why we need a flag to only do it sometimes. Yes for the backward compatibility - almost all <input> element have shadows, So enabling shadow dump by default will break existing expectation.. > > > LayoutTests/resources/dump-as-markup.js:215 > > + str += Markup._indent(depth + 1) + 'shadow:shadowPseudoId="' + pseudoId + '"'; > > Can this be shadow:pseudoId and shadow:root as Ryosuke suggested? Repeating "shadow" seems unnecessary. Totally makes sense. Renamed.

Ryosuke Niwa

Comment 15 2011-07-06 21:30:01 PDT

(In reply to comment #14) > (In reply to comment #12) > > (From update of attachment 99776 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=99776&action=review > > > > Are you opposed to trying just always dumping the shadow tree? I really don't see why we need a flag to only do it sometimes. > Yes for the backward compatibility - almost all <input> element have shadows, > So enabling shadow dump by default will break existing expectation.. Can we rebaseline them? I bet there aren't that many tests with input element that uses dump-as-markup.js

Ojan Vafai

Comment 16 2011-07-06 21:51:56 PDT

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #12) > > > (From update of attachment 99776 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=99776&action=review > > > > > > Are you opposed to trying just always dumping the shadow tree? I really don't see why we need a flag to only do it sometimes. > > Yes for the backward compatibility - almost all <input> element have shadows, > > So enabling shadow dump by default will break existing expectation.. > > Can we rebaseline them? I bet there aren't that many tests with input element that uses dump-as-markup.js That's what I was thinking.

Hajime Morrita

Comment 17 2011-07-07 00:34:47 PDT

Created attachment 99952 [details] Patch

Hajime Morrita

Comment 18 2011-07-07 00:36:00 PDT

(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > (In reply to comment #12) > > > > (From update of attachment 99776 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=99776&action=review > > > > > > > > Are you opposed to trying just always dumping the shadow tree? I really don't see why we need a flag to only do it sometimes. > > > Yes for the backward compatibility - almost all <input> element have shadows, > > > So enabling shadow dump by default will break existing expectation.. > > > > Can we rebaseline them? I bet there aren't that many tests with input element that uses dump-as-markup.js > > That's what I was thinking. Well, so I made it default and updated expectations. Actually the number of affected test are smaller than I imagined.

Ryosuke Niwa

Comment 19 2011-07-07 09:37:14 PDT

Comment on attachment 99952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99952&action=review r=me provided you address the following points. > LayoutTests/ChangeLog:8 > + - Added Markup.showShadows() to enable shadow-related dump. Please update ChangeLog. > LayoutTests/fast/dom/HTMLMeterElement/meter-element-markup-expected.txt:1 > + You should explain what output you'd expect to see. > LayoutTests/fast/dom/HTMLProgressElement/progress-element-markup-expected.txt:1 > + Ditto.

Hajime Morrita

Comment 20 2011-07-07 18:52:21 PDT

Created attachment 100063 [details] Asking for cq+

Ryosuke Niwa

Comment 21 2011-07-07 18:54:52 PDT

Comment on attachment 100063 [details] Asking for cq+ View in context: https://bugs.webkit.org/attachment.cgi?id=100063&action=review > LayoutTests/fast/dom/HTMLMeterElement/meter-element-markup-expected.txt:2 > +Both meter elements should have a nested shadow box with a width specified.: Period before colon? > LayoutTests/fast/dom/HTMLProgressElement/progress-element-markup-expected.txt:2 > +A progress element should have a nested shadow box with a width specified.: Ditto.

Hajime Morrita

Comment 22 2011-07-07 19:04:28 PDT

Created attachment 100064 [details] Patch

WebKit Review Bot

Comment 23 2011-07-07 19:46:54 PDT

Comment on attachment 100064 [details] Patch Clearing flags on attachment: 100064 Committed r90608: <http://trac.webkit.org/changeset/90608>

WebKit Review Bot

Comment 24 2011-07-07 19:47:00 PDT

All reviewed patches have been landed. Closing bug.

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 Tools / Tests

Assignee

Hajime Morrita

Reported

2011-06-10 03:55 PDT

Modified

2011-07-07 19:47 PDT History

CC List

4 users Show

URL

Keywords

Depends on

Blocks

59802

Dependencies

tree graph