[webkit-reviews] review denied: [Bug 117667] Expose the mediaIsVideo method in WebKit : [Attachment 204851] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Ruth Fong <ruthiecftg at gmail.com>'s
request for review:
Bug 117667: Expose the mediaIsVideo method in WebKit
https://bugs.webkit.org/show_bug.cgi?id=117667

Attachment 204851: Patch
https://bugs.webkit.org/attachment.cgi?id=204851&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=204851&action=review


Great first swipe.

My feedback should be pretty straightforward to address.

How did you test this?	I have a *vague* concern that we're getting at the
wrong Node from the hit test result, but am not sure.

> Source/WebCore/ChangeLog:4
> +	   Expose the mediaIsVideo method in WebKit
> +	   https://bugs.webkit.org/show_bug.cgi?id=117667

s/in WebKit/to WebKit/

> Source/WebCore/ChangeLog:8
> +	   No new tests added.

Generally isn't necessary with patches that obviously have no behavior change
in WebCore, especially when they don't even touch actual source files in
WebCore.

> Source/WebKit2/ChangeLog:4
> +	   Expose the mediaIsVideo method in WebKit
> +	   https://bugs.webkit.org/show_bug.cgi?id=117667

The WK2 API being exposed is no longer mediaIsVideo.

> Source/WebKit2/ChangeLog:10
> +	   * WebProcess/InjectedBundle/API/c/WKBundleHitTestResult.cpp:
> +	   (WKBundleHitTestResultGetMediaType): Added to return what type, if
any,
> +	   a media element is.

The WK* functions that implement the API are generally meant to be as thin and
simple as possible.  Usually they just call directly to their impl class.

So this function should just call through...

> Source/WebKit2/ChangeLog:16
> +	   * WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:
> +	   (WebKit::InjectedBundleHitTestResult::mediaIsVideo): Added to hook
into WebCore
> +	   method HitTestResult::mediaIsVideo().

...to this function, which should be InjectedBundleHitTestResult::getMediaType

Also please adjust the descriptions as necessary.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:106
> +bool InjectedBundleHitTestResult::mediaIsVideo() const

As mentioned above, this should be renamed and most the work moved here.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleHitTestResult.cpp:85
> +WKBundleHitTestResultMediaType
WKBundleHitTestResultGetMediaType(WKBundleHitTestResultRef hitTestResultRef)

As mentioned above, this should just be a thin layer that calls directly to
InjectedBundleHitTest

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleHitTestResult.cpp:92
> +    WebCore::Node* node =
toImpl(hitTestResultRef)->coreHitTestResult().innerNonSharedNode();
> +    if (node->nodeType() != Node::ELEMENT_NODE)
> +	   return kWKBundleHitTestResultMediaTypeNone;
> +    
> +    if (!((Element*)node)->isMediaElement())
> +	   return kWKBundleHitTestResultMediaTypeNone;

After we banged this out, I recalled there's a convenience method in WebCore
for casting Node->Element

See "toElement()" in WebCore/Element.h.   Use that instead.  :)

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleHitTestResult.cpp:97
> +    if (toImpl(hitTestResultRef)->mediaIsVideo())
> +	   return kWKBundleHitTestResultMediaTypeVideo;
> +    
> +    return kWKBundleHitTestResultMediaTypeAudio;

This is fine.

An alternate way to write it would be:

return toImpl(hitTestResultRef)->mediaIsVideo() ?
kWKBundleHitTestResultMediaTypeVideo : kWKBundleHitTestResultMediaTypeAudio;