Bug 126818

Summary: Add AVKit fullscreen video interface - WebKit1.
Product: WebKit Reporter: Jeremy Jones <jeremyj-wk>
Component: New BugsAssignee: Jeremy Jones <jeremyj-wk>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, eflews.bot, eric.carlson, esprehn+autocc, glenn, gtk-ews, gyuyoung.kim, jer.noble, rego+ews, rniwa, sam, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jeremy Jones 2014-01-11 11:06:17 PST
Add AVKit fullscreen video interface.
Comment 1 Jeremy Jones 2014-01-11 11:39:43 PST
Created attachment 220941 [details]
Patch
Comment 2 Sam Weinig 2014-01-11 11:49:13 PST
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.
Comment 3 Jer Noble 2014-01-11 21:01:18 PST
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);
Comment 4 Sam Weinig 2014-01-11 21:07:38 PST
(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.
Comment 5 EFL EWS Bot 2014-01-12 05:00:12 PST
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
Comment 6 Eric Carlson 2014-01-12 10:48:10 PST
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;
Comment 7 Jeremy Jones 2014-01-13 16:27:47 PST
Created attachment 221086 [details]
Patch
Comment 8 Jeremy Jones 2014-01-13 16:32:06 PST
Created attachment 221087 [details]
Patch
Comment 9 Jon Lee 2014-01-13 19:13:14 PST
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.
Comment 10 Eric Carlson 2014-01-13 20:23:36 PST
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:"
Comment 11 Jeremy Jones 2014-01-15 11:52:11 PST
Created attachment 221290 [details]
Patch
Comment 12 Jer Noble 2014-01-15 12:12:18 PST
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.
Comment 13 Build Bot 2014-01-15 12:26:27 PST
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
Comment 14 Build Bot 2014-01-15 12:45:13 PST
Comment on attachment 221290 [details]
Patch

Attachment 221290 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4890110579441664
Comment 15 Jeremy Jones 2014-01-15 16:56:13 PST
Created attachment 221319 [details]
Patch
Comment 16 EFL EWS Bot 2014-01-15 17:09:44 PST
Comment on attachment 221319 [details]
Patch

Attachment 221319 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/4749109487468544
Comment 17 EFL EWS Bot 2014-01-15 17:14:17 PST
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
Comment 18 Build Bot 2014-01-15 17:23:43 PST
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
Comment 19 Build Bot 2014-01-15 17:45:03 PST
Comment on attachment 221319 [details]
Patch

Attachment 221319 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5073698956509184
Comment 20 kov's GTK+ EWS bot 2014-01-15 18:19:57 PST
Comment on attachment 221319 [details]
Patch

Attachment 221319 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5837646936932352
Comment 21 Jeremy Jones 2014-01-16 20:46:38 PST
Created attachment 221437 [details]
Patch
Comment 22 Eric Carlson 2014-01-17 11:42:38 PST
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?
Comment 23 Jeremy Jones 2014-01-17 11:58:11 PST
(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?
Comment 24 Jer Noble 2014-01-17 13:30:02 PST
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.
Comment 25 Jer Noble 2014-01-17 13:31:10 PST
(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.
Comment 26 Jeremy Jones 2014-01-17 16:02:41 PST
Created attachment 221504 [details]
Patch
Comment 27 WebKit Commit Bot 2014-01-20 14:37:04 PST
Comment on attachment 221504 [details]
Patch

Clearing flags on attachment: 221504

Committed r162379: <http://trac.webkit.org/changeset/162379>
Comment 28 WebKit Commit Bot 2014-01-20 14:37:08 PST
All reviewed patches have been landed.  Closing bug.