Bug 126818 – Add AVKit fullscreen video interface - WebKit1.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Jones]

Add AVKit fullscreen video interface.

[Jeremy Jones]

Created attachment 220941 [details]
Patch

[Sam Weinig]

Comment on attachment 220941 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:42
> +@property (assign) WebView *webView;

WebCore is not allowed to know about WebView's existence. If you need the host view, you should use some abstraction.

[Jer Noble]

Comment on attachment 220941 [details]
Patch

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

r=me, with some nits. Go ahead and upload a new patch with a cq? flag and someone will cq+ it.

> Source/WebCore/html/HTMLMediaElement.cpp:4916
> +    if (document().settings() && document().settings()->mediaPlaybackFullscreenAVKit()) {
> +        if (m_platformLayerBorrowed)

I think the if (m_platformLayerBorrowed) check here is sufficient.

>> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:42
>> +@property (assign) WebView *webView;
> 
> WebCore is not allowed to know about WebView's existence. If you need the host view, you should use some abstraction.

It looks like this is dead code.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:131
> +        _mediaElement.get()->exitFullscreen();

RefPtr has an operator->, so no need to call .get() here.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:140
> +        _mediaElement.get()->play();

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:148
> +        _mediaElement.get()->pause();

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:156
> +        _mediaElement.get()->togglePlayState();

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:171
> +            _mediaElement.get()->play();
> +        else
> +            _mediaElement.get()->pause();

Double ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:183
> +        ExceptionCode exceptionCode {};

ExceptionCode is an integer, so it can be initialized with `= 0`;

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:184
> +        _mediaElement.get()->setCurrentTime(time, exceptionCode);

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:190
> +    NSTimeInterval anchorTimeStamp = ([self rate] == 0.f) ? NAN : [classAVValueTiming currentTimeStamp];

No need for the `.f`.  In fact, that conditional should be `![self rate]`.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:192
> +      anchorTimeStamp:anchorTimeStamp rate:0];

Weird indenting here.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:251
> +    self.playerLayer = (CALayer<AVPlayerLayer> *)_mediaElement.get()->borrowPlatformLayer();

Again, no need for .get().  This should have something like:

ASSERT([self.playerLayer isKindOfClass:classAVPlayerLayer]);

To ensure that we catch if anyone starts returning an unexpected layer type here.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:254
> +    [self updateTimingWithCurrentTime:_mediaElement.get()->currentTime()];

No need for .get().

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:257
> +        HTMLVideoElement *ve = toHTMLVideoElement(_mediaElement.get());

Please use a more descriptive variable name here, like "videoElement".

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:286
> +        [self updateTimingWithCurrentTime:_mediaElement.get()->currentTime()];

No need for .get().

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:979
> +#if PLATFORM(IOS)
> +    if (Settings::mediaPlaybackFullscreenAVKit())
> +        return isHTMLVideoElement(node);
> +    else
> +        return false;
> +#else

Just to clean things up a little, this could be:

#if PLATFORM(IOS)
    if (!Settings::mediaPlaybackFullscreenAVKit())
        return false;
#endif
    return isHTMLVideoElement(node);

[Sam Weinig]

(In reply to comment #2)
> (From update of attachment 220941 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220941&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:42
> > +@property (assign) WebView *webView;
> 
> WebCore is not allowed to know about WebView's existence. If you need the host view, you should use some abstraction.

Please don't land with this layering violation in place.

[EFL EWS Bot]

Comment on attachment 220941 [details]
Patch

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

[Eric Carlson]

Comment on attachment 220941 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:247
> +    self.loadedTimeRanges = @[ @0, @(duration) ];

We don't always have the entire file loaded. minTimeSeekable() and maxTimeSeekable() are probably what you want. This is probably fine for the initial patch, but please file a bug to fix it and include a "FIXME:" here that references the bug.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:275
> +        self.maxTime = duration;

Is maxTime the maximum time loaded or the maximum time seekable? In either case, this isn't correct. Please file a bug and reference it here if you aren't going to fix it in this patch.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:280
> +        self.loadedTimeRanges = @[
> +            @0,
> +            @(duration)
> +        ];

Ditto the comment about loadedTimeRanges above.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:305
> +    if ((self = [super init])) {

if (!(self = [super init]))
        return nil;

[Jeremy Jones]

Created attachment 221086 [details]
Patch

[Jeremy Jones]

Created attachment 221087 [details]
Patch

[Jon Lee]

Comment on attachment 221087 [details]
Patch

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

> Source/WebCore/page/Settings.cpp:101
> +bool Settings::gMediaPlaybackFullscreenAVKit = false;

This name could be improved. "gMediaPlayerFullscreenAVKitEnabled"? or just "gAVKitEnabled"? Also, this should propagate across all the settings-related names in this patch.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY

This is the wrong license to use. (What's Apple Computer, Inc?)

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:25
> +

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:162
> +    return ([self rate] != 0.);

Remove parentheses, please.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:183
> +        ExceptionCode exceptionCode {};

ExceptionCode ec = 0;

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:184
> +        _mediaElement.get()->setCurrentTime(time, exceptionCode);

setCurrentTime() takes one parameter here...

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:265
> +    Event *event = core(evnt);

Event* event.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:280
> +        ];

Small enough to combine into one line without losing legibility.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:283
> +            || event->type() == eventNames().ratechangeEvent)

I believe we indent 4 spaces, and not align the conditionals.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:305
> +    if ((self = [super init])) {

Would be better to do an early return to get rid of a scope.

[Eric Carlson]

Comment on attachment 221087 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:131
> +        _mediaElement.get()->exitFullscreen();

A Jer noted in the earlier review, RefPtr has an operator-> so no need to call .get() here.

Here and throughout much of the rest of the patch.

>> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:162
>> +    return ([self rate] != 0.);
> 
> Remove parentheses, please.

No need to compare against 0: "return [self rate];"

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:190
> +    NSTimeInterval anchorTimeStamp = ([self rate] == 0.f) ? NAN : [classAVValueTiming currentTimeStamp];

No need for the ".f": 

NSTimeInterval anchorTimeStamp = ![self rate] ? NAN : [classAVValueTiming currentTimeStamp];

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:247
> +    self.loadedTimeRanges = @[ @0, @(duration) ];

This is wrong. Please file a bug about fixing it in a followup patch and include the bug number here in a "FIXME:" comment.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:251
> +    self.playerLayer = (CALayer<AVPlayerLayer> *)_mediaElement.get()->borrowPlatformLayer();

As Jer noted before: "ASSERT([self.playerLayer isKindOfClass:classAVPlayerLayer])"

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:257
> +        HTMLVideoElement *ve = toHTMLVideoElement(_mediaElement.get());

Letters are cheap, don't abbreviate: "HTMLVideoElement *videoElement"

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:279
> +        self.loadedTimeRanges = @[
> +            @0,
> +            @(duration)

Ditto the comment above about adding a "FIXME:"

[Jeremy Jones]

Created attachment 221290 [details]
Patch

[Jer Noble]

Comment on attachment 221290 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:4883
> +        if (document().settings() && document().settings()->avKitEnabled()) {

This could be replaced with `document().page()->chrome().client().supportsFullscreenForNode(this)`, but this can be done in a follow-up patch.

In fact, you could remove the entire #if !PLATFORM(IOS) check, since the default chromeClient will answer true for HTMLMediaElements.

[Build Bot]

Comment on attachment 221290 [details]
Patch

Attachment 221290 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4701501385605120

[Build Bot]

Comment on attachment 221290 [details]
Patch

Attachment 221290 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4890110579441664

[Jeremy Jones]

Created attachment 221319 [details]
Patch

[EFL EWS Bot]

Comment on attachment 221319 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 221319 [details]
Patch

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

[Build Bot]

Comment on attachment 221319 [details]
Patch

Attachment 221319 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5671146624122880

[Build Bot]

Comment on attachment 221319 [details]
Patch

Attachment 221319 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5073698956509184

[kov's GTK+ EWS bot]

Comment on attachment 221319 [details]
Patch

Attachment 221319 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5837646936932352

[Jeremy Jones]

Created attachment 221437 [details]
Patch

[Eric Carlson]

Comment on attachment 221437 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:250
> +    if (_mediaElement == mediaElement)
> +        return;

You need to stash "mediaElement" in a local RefPtr since you use it twice, dereferencing a PassRefPtr nulls the internal ptr (see http://www.webkit.org/coding/RefPtr.html).

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:258
> +        _mediaElement->removeEventListener(eventNames().durationchangeEvent, _listener.get(), false);
> +        _mediaElement->removeEventListener(eventNames().pauseEvent, _listener.get(), false);
> +        _mediaElement->removeEventListener(eventNames().playEvent, _listener.get(), false);
> +        _mediaElement->removeEventListener(eventNames().ratechangeEvent, _listener.get(), false);
> +        _mediaElement->removeEventListener(eventNames().timeupdateEvent, _listener.get(), false);

Do you need _listener.get(), it isn't necessary in the addEventListener() calls below?

[Jeremy Jones]

(In reply to comment #22)
> (From update of attachment 221437 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221437&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:250
> > +    if (_mediaElement == mediaElement)
> > +        return;
> 
> You need to stash "mediaElement" in a local RefPtr since you use it twice, dereferencing a PassRefPtr nulls the internal ptr (see http://www.webkit.org/coding/RefPtr.html).

Good catch. I'll fix this.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:258
> > +        _mediaElement->removeEventListener(eventNames().durationchangeEvent, _listener.get(), false);
> > +        _mediaElement->removeEventListener(eventNames().pauseEvent, _listener.get(), false);
> > +        _mediaElement->removeEventListener(eventNames().playEvent, _listener.get(), false);
> > +        _mediaElement->removeEventListener(eventNames().ratechangeEvent, _listener.get(), false);
> > +        _mediaElement->removeEventListener(eventNames().timeupdateEvent, _listener.get(), false);
> 
> Do you need _listener.get(), it isn't necessary in the addEventListener() calls below?

addEventListener takes a PassRefPtr, removeEventListener takes a naked pointer. Maybe so removes can happen during destruction?

[Jer Noble]

Comment on attachment 221437 [details]
Patch

r=me.  Looks like you've got all the bots passing, so whenever you're ready, just mark it as cq? and I'll cq+ it.

[Jer Noble]

(In reply to comment #24)
> (From update of attachment 221437 [details])
> r=me.  Looks like you've got all the bots passing, so whenever you're ready, just mark it as cq? and I'll cq+ it.

Or you can put up another patch to address Eric's comments, but add the "Reviewed by Jer Noble." string to the ChangeLog, and pass the '-no-obselete' flag to webkit-patch.

[Jeremy Jones]

Created attachment 221504 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 221504 [details]
Patch

Clearing flags on attachment: 221504

Committed r162379: <http://trac.webkit.org/changeset/162379>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.