Bug 62447

Summary: dump-as-markup.js should support shadow tree
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: Tools / TestsAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, ojan, rniwa, webkit.review.bot
Priority: P2    
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

Description Hajime Morrita 2011-06-10 03:55:12 PDT 
In some case, we need to dump shadow tree as a part of markup dump.
Comment 1 Hajime Morrita 2011-07-04 22:16:26 PDT 
Created attachment 99669 [details]
Patch
Comment 2 Hajime Morrita 2011-07-04 22:17:05 PDT 
Ryosuke, Dimitri, could you take a look?
Comment 3 Ryosuke Niwa 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.
Comment 4 Hajime Morrita 2011-07-05 01:13:22 PDT 
Created attachment 99676 [details]
Patch
Comment 5 Hajime Morrita 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.
Comment 6 Ryosuke Niwa 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>?
Comment 7 Hajime Morrita 2011-07-05 01:55:14 PDT 
Created attachment 99683 [details]
Patch
Comment 8 Hajime Morrita 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.
Comment 9 Ojan Vafai 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?
Comment 10 Hajime Morrita 2011-07-05 18:54:35 PDT 
Created attachment 99776 [details]
Patch
Comment 11 Hajime Morrita 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
Comment 12 Ojan Vafai 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.
Comment 13 Hajime Morrita 2011-07-06 21:26:45 PDT 
Created attachment 99926 [details]
Patch
Comment 14 Hajime Morrita 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.
Comment 15 Ryosuke Niwa 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
Comment 16 Ojan Vafai 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.
Comment 17 Hajime Morrita 2011-07-07 00:34:47 PDT 
Created attachment 99952 [details]
Patch
Comment 18 Hajime Morrita 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.
Comment 19 Ryosuke Niwa 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.
Comment 20 Hajime Morrita 2011-07-07 18:52:21 PDT 
Created attachment 100063 [details]
Asking for cq+
Comment 21 Ryosuke Niwa 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.
Comment 22 Hajime Morrita 2011-07-07 19:04:28 PDT 
Created attachment 100064 [details]
Patch
Comment 23 WebKit Review Bot 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>
Comment 24 WebKit Review Bot 2011-07-07 19:47:00 PDT 
All reviewed patches have been landed.  Closing bug.