Bug 62447

Summary:

dump-as-markup.js should support shadow tree

Product:

WebKit

Reporter:

Hajime Morrita <morrita>

Component:

Tools / Tests

Assignee:

Hajime Morrita <morrita>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, ojan, rniwa, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

59802

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Asking for cq+	none
Patch	none

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.