WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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.
Top of Page
Format For Printing
XML
Clone This Bug