Bug 117667 – Expose a getMediaType method in WebKit

• Component: Media
• Status: RESOLVED FIXED

[Ruth Fong]

Provide hooks in WebKit to detect whether a media element is a <video> element.

[Radar WebKit Bug Importer]

<rdar://problem/14163408>

[Ruth Fong]

Created attachment 204752 [details]
Patch

[Brady Eidson]

Comment on attachment 204752 [details]
Patch

Style seems fine here, but I'm not convinced this API is the way to go.

Asking a WKBundleHitTestResult if the media is video for a non-media element is kind of weird.  Of course it's not video - It's a div, or a img!

The client would have to first determine that the hit test result represents a media element by getting the media src URL (which is kind of a weird way to determine that...) and then use this API.

The new API should probably be a general purpose "what kind of media element is this?" call.  To start with it could return:
kWKMediaTypeAudio
kWKMediaTypeVideo
kWKMediaTypeNone

This removes some burden on the client and is also expandable in the future when we add <smell>, <taste>, or <touch>.

[Ruth Fong]

Created attachment 204851 [details]
Patch

[Brady Eidson]

Comment on attachment 204851 [details]
Patch

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;

[Ruth Fong]

Created attachment 204859 [details]
Patch

[Ruth Fong]

Created attachment 204860 [details]
Patch

[Brady Eidson]

Comment on attachment 204860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204860&action=review

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:111
> +    if (node->nodeType() != Node::ELEMENT_NODE)
> +        return kWKBundleHitTestResultMediaTypeNone;

This should probably be if (node->isElementNode()) instead.  Sorry for not catching that earlier.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleHitTestResult.h:32
>  #include <WebCore/HitTestResult.h>
> +#include <WebKit2/WKBundleHitTestResult.h>
>  #include <wtf/Forward.h>

Seeing this in the flesh set off a red flag - We shouldn't be directly including API headers in implementation files when we can avoid it.

I've suggested to Ruth on IRC a path forward with an API set of enums, an impl set of enums, and API casting.

[Ruth Fong]

Created attachment 204866 [details]
Patch

[EFL EWS Bot]

Comment on attachment 204866 [details]
Patch

Attachment 204866 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/921395

[Ruth Fong]

Created attachment 204867 [details]
Patch

[EFL EWS Bot]

Comment on attachment 204867 [details]
Patch

Attachment 204867 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/884135

[Ruth Fong]

Created attachment 204869 [details]
Patch

[EFL EWS Bot]

Comment on attachment 204869 [details]
Patch

Attachment 204869 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/950175

[Brady Eidson]

Comment on attachment 204869 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204869&action=review

> Source/WebKit2/UIProcess/API/C/WKAPICast.h:176
> +    
> +    return kWKBundleHitTestResultMediaTypeNone;

ASSERT_NOT_REACHED() here also, right?

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleHitTestResult.h:32
> +#include <WebKit2/InjectedBundleHitTestResultMediaType.h>
> +#include <WebKit2/WKBundleHitTestResult.h>

These headers are local to the WK2 project, so it should just be #include "InjectedBundleHitTestResultMediaType.h" under the APIObject include.

And the WKBundleHitTestResult.h include isn't needed at all - that's what we're avoiding with all these casts and such!

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleHitTestResult.cpp:84
> +    return toImpl(hitTestResultRef)->getMediaType();

This needs to be:
return toAPI(toImpl(hitTestResultRef)->getMediaType()) to perform the InjectedBundle* -> WKBundle* cast

[Ruth Fong]

Comment on attachment 204869 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204869&action=review

>> Source/WebKit2/UIProcess/API/C/WKAPICast.h:176
>> +    return kWKBundleHitTestResultMediaTypeNone;
> 
> ASSERT_NOT_REACHED() here also, right?

None of the other toAPI methods include the ASSERT_NOT_REACHED() (not sure why)

>> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleHitTestResult.h:32
>> +#include <WebKit2/WKBundleHitTestResult.h>
> 
> These headers are local to the WK2 project, so it should just be #include "InjectedBundleHitTestResultMediaType.h" under the APIObject include.
> 
> And the WKBundleHitTestResult.h include isn't needed at all - that's what we're avoiding with all these casts and such!

Got it.

>> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleHitTestResult.cpp:84
>> +    return toImpl(hitTestResultRef)->getMediaType();
> 
> This needs to be:
> return toAPI(toImpl(hitTestResultRef)->getMediaType()) to perform the InjectedBundle* -> WKBundle* cast

Got it (it seemed to work without the cast...)

[Ruth Fong]

Created attachment 204931 [details]
Patch

[Ruth Fong]

Created attachment 204934 [details]
Patch

[EFL EWS Bot]

Comment on attachment 204934 [details]
Patch

Attachment 204934 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/948047

[Beth Dakin]

Committed Ruth's patch: http://trac.webkit.org/changeset/151699

[Brady Eidson]

(In reply to comment #19)
> (From update of attachment 204934 [details])
> Attachment 204934 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/948047

This build failure is bizarre.  It's in WebKitTestRunner and TestWebKitAPI with regards to WKAPICast.h.

In those failures, WKAPICast.h includes other WK2 internal headers.  For the other ~6 it includes, they are found without issue.  For the newly added header, it is not found.

There's no build system file we can find that includes those other headers.  It doesn't seem like we missed a build system.

Our only theory is that EFL copies all the Shared/ headers (and therefore gets those other ones) but doesn't copy the InjectedBundle/ headers.

If that's what's going on, that's a failure with the EFL build system, as this new header is in the right place.

[Darin Adler]

Comment on attachment 204934 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204934&action=review

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleHitTestResult.h:56
> +    BundleHitTestResultMediaType getMediaType() const;

WebKit coding style says this should be named mediaType(), not getMediaType().

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleHitTestResult.h:55
> +WK_EXPORT WKBundleHitTestResultMediaType WKBundleHitTestResultGetMediaType(WKBundleHitTestResultRef hitTestResult);

Here it is fine to use the name WKBundleHitTestResultGetMediaType.

[Csaba Osztrogonác]

(In reply to comment #20)
> Committed Ruth's patch: http://trac.webkit.org/changeset/151699

It broke the !ENABLE(VIDEO) platforms because of a missing ifdef guard:
/home/webkitbuildbot/slaves/armQt5Release-buildonly/buildslave/qt5-linux-armv7-release/build/Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp: In member function 'WebKit::BundleHitTestResultMediaType WebKit::InjectedBundleHitTestResult::getMediaType() const':
/home/webkitbuildbot/slaves/armQt5Release-buildonly/buildslave/qt5-linux-armv7-release/build/Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleHitTestResult.cpp:113:27: error: 'class WebCore::Element' has no member named 'isMediaElement'


Could you check and fix it, please?

[Csaba Osztrogonác]

(In reply to comment #20)
> Committed Ruth's patch: http://trac.webkit.org/changeset/151699

The patch landed, so can we close the bug?

[Ruth Fong]

Closed this bug.

Bug 117765 is now tracking the revisions to this patch: namely the renaming of the getMediaType functions and fixing the patch for !ENABLE(VIDEO) platforms.