Summary: | Update media-controls.js so it can find the timeline slider | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 68038 | ||||||
Attachments: |
|
Description
Eric Carlson
2011-09-13 15:06:49 PDT
Created attachment 107239 [details]
Proposed patch.
Comment on attachment 107239 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=107239&action=review r=me -- this code seems like it will work but there are some things worth fixing > LayoutTests/media/media-controls.js:2 > +function mediaConrolsElement(parent, id) Typo here: "conrols". Not sure that naming this argument “parent” is good; the code iterates the siblings of what’s passed in, not its children. > LayoutTests/media/media-controls.js:5 > + if (parent.nodeType !== Node.ELEMENT_NODE) > + return null; I don’t think this is needed. Not sure why you are checking the type of the first element in a sibling list but not the types of the rest. As far as I can tell no checking is needed. > LayoutTests/media/media-controls.js:7 > + controlID = "-webkit-media-controls-" + id; Should use var here. > LayoutTests/media/media-controls.js:13 > + var childElement = mediaConrolsElement(element.firstChild, id); This will work, but there are also non-recursive ways to walk an entire hierarchy. > LayoutTests/media/media-controls.js:25 > + var controlsShadow = internals.shadowRoot(element).firstChild.firstChild; Would be nice to have a why comment here explaining why we are descending two levels deep with firstChild. (In reply to comment #2) > (From update of attachment 107239 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107239&action=review > > r=me -- this code seems like it will work but there are some things worth fixing > > > LayoutTests/media/media-controls.js:2 > > +function mediaConrolsElement(parent, id) > > Typo here: "conrols". > > Not sure that naming this argument “parent” is good; the code iterates the siblings of what’s passed in, not its children. > Fixed both, thanks. > > LayoutTests/media/media-controls.js:5 > > + if (parent.nodeType !== Node.ELEMENT_NODE) > > + return null; > > I don’t think this is needed. Not sure why you are checking the type of the first element in a sibling list but not the types of the rest. As far as I can tell no checking is needed. > It was there because internals.shadowPseudoId() throws an exception if called for an element that doesn't have a shadow ID and the TEXT_NODE nodes don't, but a better way to deal with that is with a comment and a try block. Fixed. > > LayoutTests/media/media-controls.js:7 > > + controlID = "-webkit-media-controls-" + id; > > Should use var here. > Thanks, fixed. > > LayoutTests/media/media-controls.js:13 > > + var childElement = mediaConrolsElement(element.firstChild, id); > > This will work, but there are also non-recursive ways to walk an entire hierarchy. > The media controls hierarchy is small and shallow, so I left this as is for now. > > LayoutTests/media/media-controls.js:25 > > + var controlsShadow = internals.shadowRoot(element).firstChild.firstChild; > > Would be nice to have a why comment here explaining why we are descending two levels deep with firstChild. > Good point, there isn't any reason to hard code it at two levels deep. Fixed. Thanks! |