68034 2011-09-13 15:06:49 -0700 Update media-controls.js so it can find the timeline slider 2011-09-14 12:27:27 -0700 1 1 1 Unclassified WebKit Media 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 68038 1 eric.carlson eric.carlson webkit-bug-importer oldest_to_newest 466586 0 eric.carlson 2011-09-13 15:06:49 -0700 The mediaControlsButtonCoordinates() function in LayoutTests/media/media-controls.js returns the center point of an element in the media controller so it is possible to write cross-platform tests that use the controls. It doesn't work with the timeline slider or volume slider because it assumes that all of the elements in the controller are siblings. 466591 1 107239 eric.carlson 2011-09-13 15:12:53 -0700 Created attachment 107239 Proposed patch. 466682 2 107239 darin 2011-09-13 16:45:08 -0700 Comment on attachment 107239 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. 466744 3 eric.carlson 2011-09-13 18:03:16 -0700 (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! 466780 4 webkit-bug-importer 2011-09-13 19:10:56 -0700 <rdar://problem/10120776> 467236 5 eric.carlson 2011-09-14 12:27:27 -0700 http://trac.webkit.org/changeset/95111 107239 2011-09-13 15:12:53 -0700 2011-09-13 16:45:08 -0700 Proposed patch. patch_1.txt text/plain 2367 eric.carlson SW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9D aGFuZ2VMb2cJKHJldmlzaW9uIDk1MDQ3KQorKysgTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3Jr aW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTEtMDktMTMgIEVyaWMgQ2FybHNvbiAgPGVy aWMuY2FybHNvbkBhcHBsZS5jb20+CisKKyAgICAgICAgVXBkYXRlIG1lZGlhLWNvbnRyb2xzLmpz IHNvIGl0IGNhbiBmaW5kIHRoZSB0aW1lbGluZSBzbGlkZXIKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTY4MDM0CisKKyAgICAgICAgRG9uJ3QgYXNzdW1l IHRoYXQgYWxsIGVsZW1lbnRzIGluIHRoZSBtZWRpYSBjb250cm9sbGVyIHBzZXVkbyBET00gYXJl IHNpYmxpbmdzLgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgICogbWVkaWEvbWVkaWEtY29udHJvbHMuanM6CisgICAgICAgIChtZWRpYUNvbnJvbHNFbGVt ZW50KToKKyAgICAgICAgKG1lZGlhQ29udHJvbHNCdXR0b25Db29yZGluYXRlcyk6CisKIDIwMTEt MDktMTMgIFRpbSBIb3J0b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CiAKICAgICAgICAg UkVHUkVTU0lPTiAoNjQyNzUpOiBTaGFwZSBwYXR0ZXJuLWltYWdlIGZpbGwgdHVybnMgYmxhY2sK SW5kZXg6IExheW91dFRlc3RzL21lZGlhL21lZGlhLWNvbnRyb2xzLmpzCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IExheW91dFRlc3RzL21lZGlhL21lZGlhLWNvbnRyb2xzLmpzCShyZXZpc2lvbiA5NDc2MSkKKysr IExheW91dFRlc3RzL21lZGlhL21lZGlhLWNvbnRyb2xzLmpzCSh3b3JraW5nIGNvcHkpCkBAIC0x LDE3ICsxLDMxIEBACiAKLWZ1bmN0aW9uIG1lZGlhQ29udHJvbHNCdXR0b25Db29yZGluYXRlcyhl bGVtZW50LCBpZCkKK2Z1bmN0aW9uIG1lZGlhQ29ucm9sc0VsZW1lbnQocGFyZW50LCBpZCkKIHsK LSAgICB2YXIgYnV0dG9uOwotICAgIHZhciBjb250cm9sc1NoYWRvdyA9IGludGVybmFscy5zaGFk b3dSb290KGVsZW1lbnQpLmZpcnN0Q2hpbGQuZmlyc3RDaGlsZDsKLSAgICBmb3IgKGNoaWxkID0g Y29udHJvbHNTaGFkb3cuZmlyc3RDaGlsZDsgY2hpbGQ7IGNoaWxkID0gY2hpbGQubmV4dFNpYmxp bmcpIHsKLSAgICAgICAgaWYgKGludGVybmFscy5zaGFkb3dQc2V1ZG9JZChjaGlsZCkgPT0gIi13 ZWJraXQtbWVkaWEtY29udHJvbHMtIiArIGlkKSB7Ci0gICAgICAgICAgICBidXR0b24gPSBjaGls ZDsKLSAgICAgICAgICAgIGJyZWFrOworICAgIGlmIChwYXJlbnQubm9kZVR5cGUgIT09IE5vZGUu RUxFTUVOVF9OT0RFKQorICAgICAgICByZXR1cm4gbnVsbDsKKworICAgIGNvbnRyb2xJRCA9ICIt d2Via2l0LW1lZGlhLWNvbnRyb2xzLSIgKyBpZDsKKyAgICBmb3IgKHZhciBlbGVtZW50ID0gcGFy ZW50OyBlbGVtZW50OyBlbGVtZW50ID0gZWxlbWVudC5uZXh0U2libGluZykgeworICAgICAgICBp ZiAoaW50ZXJuYWxzLnNoYWRvd1BzZXVkb0lkKGVsZW1lbnQpID09IGNvbnRyb2xJRCkKKyAgICAg ICAgICAgIHJldHVybiBlbGVtZW50OworCisgICAgICAgIGlmIChlbGVtZW50LmZpcnN0Q2hpbGQp IHsKKyAgICAgICAgICAgIHZhciBjaGlsZEVsZW1lbnQgPSBtZWRpYUNvbnJvbHNFbGVtZW50KGVs ZW1lbnQuZmlyc3RDaGlsZCwgaWQpOworICAgICAgICAgICAgaWYgKGNoaWxkRWxlbWVudCkKKyAg ICAgICAgICAgICAgICByZXR1cm4gY2hpbGRFbGVtZW50OwogICAgICAgICB9CiAgICAgfQogCisg ICAgcmV0dXJuIG51bGw7Cit9CisKKworZnVuY3Rpb24gbWVkaWFDb250cm9sc0J1dHRvbkNvb3Jk aW5hdGVzKGVsZW1lbnQsIGlkKQoreworICAgIHZhciBjb250cm9sc1NoYWRvdyA9IGludGVybmFs cy5zaGFkb3dSb290KGVsZW1lbnQpLmZpcnN0Q2hpbGQuZmlyc3RDaGlsZDsKKyAgICB2YXIgYnV0 dG9uID0gbWVkaWFDb25yb2xzRWxlbWVudChjb250cm9sc1NoYWRvdywgaWQpOwogICAgIGlmICgh YnV0dG9uKQotICAgICAgICB0aHJvdyAiRmFpbGVkIHRvIGZpbmQgYnV0dG9uICIgKyBpZDsKKyAg ICAgICAgdGhyb3cgIkZhaWxlZCB0byBmaW5kIG1lZGlhIGNvbnRyb2wgZWxlbWVudCBJRCAnIiAr IGlkICsgIiciOwogCiAgICAgdmFyIGJ1dHRvbkJvdW5kaW5nUmVjdCA9IGJ1dHRvbi5nZXRCb3Vu ZGluZ0NsaWVudFJlY3QoKTsKICAgICB2YXIgeCA9IGJ1dHRvbkJvdW5kaW5nUmVjdC5sZWZ0ICsg YnV0dG9uQm91bmRpbmdSZWN0LndpZHRoIC8gMjsK