[Webkit-unassigned] [Bug 117667] Expose the mediaIsVideo method in WebKit

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=117667


Brady Eidson <beidson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #204851|review?                     |review-
               Flag|                            |




--- Comment #5 from Brady Eidson <beidson at apple.com>  2013-06-17 14:35:29 PST ---
(From update of attachment 204851)
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;

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.