[webkit-reviews] review granted: [Bug 68034] Update media-controls.js so it can find the timeline slider : [Attachment 107239] Proposed patch.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 68034: Update media-controls.js so it can find the timeline slider
https://bugs.webkit.org/show_bug.cgi?id=68034

Attachment 107239: Proposed patch.
https://bugs.webkit.org/attachment.cgi?id=107239&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.