Bug 52317 - Flip input[type=range] to use the new shadow DOM model.
Summary: Flip input[type=range] to use the new shadow DOM model.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on: 52382 52399
Blocks: 44907
  Show dependency treegraph
 
Reported: 2011-01-12 12:09 PST by Dimitri Glazkov (Google)
Modified: 2011-01-13 16:14 PST (History)
5 users (show)

See Also:


Attachments
Patch (22.51 KB, patch)
2011-01-12 12:17 PST, Dimitri Glazkov (Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-01-12 12:09:10 PST
Flip input[type=range] to use the new shadow DOM model.
Comment 1 Dimitri Glazkov (Google) 2011-01-12 12:17:57 PST
Created attachment 78720 [details]
Patch
Comment 2 Darin Adler 2011-01-12 17:44:47 PST
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?
Comment 3 Dimitri Glazkov (Google) 2011-01-13 09:53:48 PST
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.
Comment 4 Dimitri Glazkov (Google) 2011-01-13 10:35:14 PST
Committed r75725: <http://trac.webkit.org/changeset/75725>
Comment 5 WebKit Review Bot 2011-01-13 11:29:02 PST
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
Comment 6 WebKit Review Bot 2011-01-13 13:11:51 PST
http://trac.webkit.org/changeset/75728 might have broken Leopard Intel Release (Tests)
Comment 7 Dimitri Glazkov (Google) 2011-01-13 13:19:53 PST
Fascinating. I've forgotten the case of transferring shadow DOM between documents. Fixing.
Comment 8 Dimitri Glazkov (Google) 2011-01-13 16:14:57 PST
Committed r75749: <http://trac.webkit.org/changeset/75749>