Bug 52317

Created attachment 78720 [details] Patch

Comment on attachment 78720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78720&action=review Looks good. > Source/WebCore/html/InputType.h:181 > virtual bool canBeSuccessfulSubmitButton(); > > + // Shadow tree handling > + virtual void createShadowSubtree(); > + void destroyShadowSubtree(); > + > // Miscellaneous functions > > virtual bool rendererIsNeeded(); Formatting here is inconsistent. Number of blank lines (I prefer to never use more than one). Whether there is a blank line after a comment before the paragraph of functions it describes. > Source/WebCore/html/shadow/SliderThumbElement.cpp:44 > +// FIXME: Find a way to cascade appearance and get rid of this class. I think this would be clearer if you were a bit more explicit and specific. > Source/WebCore/rendering/MediaControlElements.cpp:474 > + // FIXME: Remove this once all media controls are transitioned to use the regular > + // style calculation. Seems a bit cryptic, but maybe someone more expert than I would understand. > Source/WebCore/rendering/RenderSlider.h:49 > + // FIXME: Eventually, accessing sliderThumbElement should not be necessary in this class. It would be good to say why it would not be necessary. What is it currently used for? Why would that no longer be needed?

Thanks for review! (In reply to comment #2) > (From update of attachment 78720 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78720&action=review > > Looks good. > > > Source/WebCore/html/InputType.h:181 > > virtual bool canBeSuccessfulSubmitButton(); > > > > + // Shadow tree handling > > + virtual void createShadowSubtree(); > > + void destroyShadowSubtree(); > > + > > // Miscellaneous functions > > > > virtual bool rendererIsNeeded(); > > Formatting here is inconsistent. Number of blank lines (I prefer to never use more than one). Whether there is a blank line after a comment before the paragraph of functions it describes. Will fix. > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:44 > > +// FIXME: Find a way to cascade appearance and get rid of this class. > > I think this would be clearer if you were a bit more explicit and specific. Yeah, you're right. I'll add better comment around. > > > Source/WebCore/rendering/MediaControlElements.cpp:474 > > + // FIXME: Remove this once all media controls are transitioned to use the regular > > + // style calculation. > > Seems a bit cryptic, but maybe someone more expert than I would understand. Will expand comment, too. > > > Source/WebCore/rendering/RenderSlider.h:49 > > + // FIXME: Eventually, accessing sliderThumbElement should not be necessary in this class. > > It would be good to say why it would not be necessary. What is it currently used for? Why would that no longer be needed? And here too.

Committed r75725: <http://trac.webkit.org/changeset/75725>

http://trac.webkit.org/changeset/75725 might have broken SnowLeopard Intel Release (Tests) The following tests are not passing: fast/css/pseudo-in-range-invalid-value.html fast/css/pseudo-in-range.html fast/forms/form-collection-elements.html fast/forms/range-keyoperation.html

http://trac.webkit.org/changeset/75728 might have broken Leopard Intel Release (Tests)

Fascinating. I've forgotten the case of transferring shadow DOM between documents. Fixing.

Committed r75749: <http://trac.webkit.org/changeset/75749>