Bug 158351 – Use the media element's video box when getting the inline video rect in WebVideoFullscreenManager

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Ada Chan]

Use the video box for the video element's client rect in WebVideoFullscreenManager on Mac.

<rdar://problem/26567938>

[Ada Chan]

Created attachment 280457 [details]
Patch

[Tim Horton]

Comment on attachment 280457 [details]
Patch

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

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:62
> +#if PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE)

Why is this different in this case? Seems like something that should be true for all videos.

[Ada Chan]

The way the video fullscreen layer is handled in WebVideoFullscreenInterface between iOS and Mac are different.  I'll see if I can move the platform differences out of WK2 and into WebCore instead.

Thanks for taking a look!

[Darin Adler]

Comment on attachment 280457 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:7072
> +    if (!is<RenderVideo>(renderer()))
> +        return IntRect();
> +
> +    return downcast<RenderVideo>(*renderer()).videoBox();

Please put renderer into a local variable and don’t call it twice.

More importantly, if this was in HTMLVideoElement then we would not need to typecast renderer(), although we would still need to null check it.

> Source/WebCore/html/HTMLMediaElement.h:466
> +    WEBCORE_EXPORT IntRect videoBox() const;

This should be in HTMLVideoElement instead of HTMLMediaElement.

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:63
> +    if (element->isMediaElement()) {

This should be:

    if (is<HTMLVideoElement>(*element)) {

[Ada Chan]

Created attachment 280963 [details]
Patch

[Ada Chan]

Posted an updated patch. It turns out that I can use the same code to get the video rect on both platforms which addresses Tim's feedback. I also addressed Darin's feedback.

[Darin Adler]

Comment on attachment 280963 [details]
Patch

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

I’m saying review+ but I would suggest a few significant changes to make this better before landing.

> Source/WebCore/html/HTMLVideoElement.h:95
> +    WEBCORE_EXPORT IntRect videoBox() const;

I don’t think this should be added as a member function of HTMLVideoElement. A caller that needs this rectangle should fetch the renderer and call videoBox on the renderer.

It’s not a good pattern to hide the fact that a function uses the render tree by adding a function on a DOM object that then turns around and calls a function on the renderer. Functions that use the render tree depend on layout being up to date, and we typically don’t want that hidden behind an abstraction. In fact, normally the caller needs to call some kind of updateLayout function before using a renderer.

Functions like Element::clientRect exist because they are part of the DOM standard but we should not add new ones like those.

See Element::offsetLeft, for an example of a function that correctly calls updateLayoutIgnorePendingStylesheets before getting at the DOM. I think we need something like that here (in the code in WebVideoFullscreenManager) too. I have no idea why Element::clientRect can function correctly without including a call to an updateLayout function. I am guessing it’s a subtle bug in that function.

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:70
> -static IntRect clientRectForElement(HTMLElement* element)
> +static IntRect inlineVideoRectForElement(HTMLElement* element)
>  {
>      if (!element)
>          return IntRect();
>  
> -    return element->clientRect();
> +    IntRect clientRect = element->clientRect();
> +    if (is<HTMLVideoElement>(*element)) {
> +        IntRect videoRect = downcast<HTMLVideoElement>(element)->videoBox();
> +        if (!videoRect.isEmpty()) {
> +            videoRect.moveBy(clientRect.location());
> +            return videoRect;
> +        }
> +    }
> +
> +    return clientRect;
>  }

This function should take an HTMLVideoElement&, not an HTMLElement*. If I am wrong and it needs to take an HTMLElement* for some reason, why is it ever helpful for this to return the clientRect for a non-video element?

As mentioned above, something needs to guarantee layout is up to date. Often that means calling updateLayoutIgnorePendingStylesheets before getting the renderer.

I think it would be better to write something more like this:

    element.document(). updateLayoutIgnorePendingStylesheets();
    auto* renderer = element.renderer();
    if (!renderer)
         return { };
    return renderer->view().contentsToRootView(renderer->videoBox());

rather than using clientRect and moveBy.

I don’t think the function needs “ForElement” in its name.

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:287
> +    IntRect videoRect = inlineVideoRectForElement(&videoElement);

I think auto is better than IntRect here.

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:288
> +    FloatRect videoLayerFrame = FloatRect(0, 0, videoRect.width(), videoRect.height());

Since this trick of using only the size and ignoring the position of the video box is done twice, I suggest making a helper function for this purpose. Just have to figure out what the name of it should be. What is the name for this "at 0,0" version of the video box?

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:486
> +        FloatRect videoRect = inlineVideoRectForElement(model->videoElement());

Seems peculiar to put this IntRect into a FloatRect, just to get values out of it. I think you fixed that above but not here. I suggest we use "auto" here instead of an explicit type.

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:487
> +        bounds = FloatRect(0, 0, videoRect.width(), videoRect.height());

See above about using a helper function.

[Darin Adler]

Sorry I didn’t think of those things the first time I reviewed!

[Ada Chan]

Comment on attachment 280963 [details]
Patch

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

Thanks for all your feedback!  Since I've changed a lot of the original patch, I'll post an updated one soon.

>> Source/WebCore/html/HTMLVideoElement.h:95
>> +    WEBCORE_EXPORT IntRect videoBox() const;
> 
> I don’t think this should be added as a member function of HTMLVideoElement. A caller that needs this rectangle should fetch the renderer and call videoBox on the renderer.
> 
> It’s not a good pattern to hide the fact that a function uses the render tree by adding a function on a DOM object that then turns around and calls a function on the renderer. Functions that use the render tree depend on layout being up to date, and we typically don’t want that hidden behind an abstraction. In fact, normally the caller needs to call some kind of updateLayout function before using a renderer.
> 
> Functions like Element::clientRect exist because they are part of the DOM standard but we should not add new ones like those.
> 
> See Element::offsetLeft, for an example of a function that correctly calls updateLayoutIgnorePendingStylesheets before getting at the DOM. I think we need something like that here (in the code in WebVideoFullscreenManager) too. I have no idea why Element::clientRect can function correctly without including a call to an updateLayout function. I am guessing it’s a subtle bug in that function.

This is good to know.  Thanks for explaining this to me!

>> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:70
>>  }
> 
> This function should take an HTMLVideoElement&, not an HTMLElement*. If I am wrong and it needs to take an HTMLElement* for some reason, why is it ever helpful for this to return the clientRect for a non-video element?
> 
> As mentioned above, something needs to guarantee layout is up to date. Often that means calling updateLayoutIgnorePendingStylesheets before getting the renderer.
> 
> I think it would be better to write something more like this:
> 
>     element.document(). updateLayoutIgnorePendingStylesheets();
>     auto* renderer = element.renderer();
>     if (!renderer)
>          return { };
>     return renderer->view().contentsToRootView(renderer->videoBox());
> 
> rather than using clientRect and moveBy.
> 
> I don’t think the function needs “ForElement” in its name.

It looks like RenderVideo::videoBox() returns a rect that's relative to the renderer's bounding box.  I ended up changing the implementation to this:

static IntRect inlineVideoFrame(HTMLVideoElement& element)
{
    element.document().updateLayoutIgnorePendingStylesheets();
    auto* renderer = element.renderer();
    if (!renderer)
        return element.clientRect();
    auto rect = renderer->videoBox();
    rect.moveBy(renderer->absoluteBoundingBoxRect().location());
    return element.document().view()->contentsToRootView(rect);
}

I tested a video with thick border and padding (and also testing the case where the video is in an iframe) to make sure it works.

>> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:287
>> +    IntRect videoRect = inlineVideoRectForElement(&videoElement);
> 
> I think auto is better than IntRect here.

Fixed to use auto.

>> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:288
>> +    FloatRect videoLayerFrame = FloatRect(0, 0, videoRect.width(), videoRect.height());
> 
> Since this trick of using only the size and ignoring the position of the video box is done twice, I suggest making a helper function for this purpose. Just have to figure out what the name of it should be. What is the name for this "at 0,0" version of the video box?

This trick is done twice in this file.  However, in the first instance (WebVideoFullscreenManager::enterVideoFullscreenForVideoElement()), we do use the videoRect variable later in the code.  If I make a helper function to get the videoLayerFrame and call it here, I'd end up calling inlineVideoRectForElement() twice in this method which I'd like to avoid.  So I think I'll keep this as is.

>> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:486
>> +        FloatRect videoRect = inlineVideoRectForElement(model->videoElement());
> 
> Seems peculiar to put this IntRect into a FloatRect, just to get values out of it. I think you fixed that above but not here. I suggest we use "auto" here instead of an explicit type.

Changed to use auto.

>> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:487
>> +        bounds = FloatRect(0, 0, videoRect.width(), videoRect.height());
> 
> See above about using a helper function.

Since I won't be using this helper method in WebVideoFullscreenManager::enterVideoFullscreenForVideoElement(), such helper method will only be used once here.  So I'll hold off in making a helper method until we need it somewhere else in this file.

[Ada Chan]

Created attachment 281029 [details]
Patch

[Ada Chan]

Created attachment 281030 [details]
Patch

[Darin Adler]

Comment on attachment 281030 [details]
Patch

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

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:61
> +        return element.clientRect();

This should just return an empty rectangle (could use IntRect(), IntRect { }, or { }, depending on which syntax you prefer). That’s what the clientRect function is going to return if the renderer is null.

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:64
> +    auto rect = renderer->videoBox();
> +    rect.moveBy(renderer->absoluteBoundingBoxRect().location());
> +    return element.document().view()->contentsToRootView(rect);

I’m slightly surprised that this rather complex coordinate conversion is correct, but I’m not enough of an expert on rendering to know whether it is or not, so I will take your word for it.

[Ada Chan]

Created attachment 281073 [details]
Patch

[Ada Chan]

(In reply to comment #12)
> Comment on attachment 281030 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281030&action=review
> 
> > Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:61
> > +        return element.clientRect();
> 
> This should just return an empty rectangle (could use IntRect(), IntRect {
> }, or { }, depending on which syntax you prefer). That’s what the clientRect
> function is going to return if the renderer is null.

Fixed to return { }

> 
> > Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:64
> > +    auto rect = renderer->videoBox();
> > +    rect.moveBy(renderer->absoluteBoundingBoxRect().location());
> > +    return element.document().view()->contentsToRootView(rect);
> 
> I’m slightly surprised that this rather complex coordinate conversion is
> correct, but I’m not enough of an expert on rendering to know whether it is
> or not, so I will take your word for it.

I've asked Simon Fraser to take a look just to make sure and he thinks that's right.

Thanks for the review!

[Ada Chan]

Committed:
http://trac.webkit.org/changeset/201963

[Darin Adler]

Comment on attachment 281073 [details]
Patch

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

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:64
> +    return element.document().view()->contentsToRootView(rect);

Darn it, I forgot to comment on this. It’s better not to go through the DOM here, staying in the render tree is more direct, clearer, and the fact that the return value is a reference makes it clearer there is no issue with null:

    return renderer->view().contentsToRootView(rect);

[Ada Chan]

(In reply to comment #16)
> Comment on attachment 281073 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281073&action=review
> 
> > Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:64
> > +    return element.document().view()->contentsToRootView(rect);
> 
> Darn it, I forgot to comment on this. It’s better not to go through the DOM
> here, staying in the render tree is more direct, clearer, and the fact that
> the return value is a reference makes it clearer there is no issue with null:
> 
>     return renderer->view().contentsToRootView(rect);

I did notice you suggested doing this in your first set of feedback, and I did try it and get a compile error (since renderer->view() returns a RenderView which doesn't have a contentsToRootView() method). But now looking at RenderView's header more carefully, maybe what I can do is:

renderer->view().frameView().contentsToRootView(rect);

Rs=you to do this?

Thanks for explaining why going through the render tree is a better option.

[Ada Chan]

(In reply to comment #17)
> (In reply to comment #16)
> > Comment on attachment 281073 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=281073&action=review
> > 
> > > Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:64
> > > +    return element.document().view()->contentsToRootView(rect);
> > 
> > Darn it, I forgot to comment on this. It’s better not to go through the DOM
> > here, staying in the render tree is more direct, clearer, and the fact that
> > the return value is a reference makes it clearer there is no issue with null:
> > 
> >     return renderer->view().contentsToRootView(rect);
> 
> I did notice you suggested doing this in your first set of feedback, and I
> did try it and get a compile error (since renderer->view() returns a
> RenderView which doesn't have a contentsToRootView() method). But now
> looking at RenderView's header more carefully, maybe what I can do is:
> 
> renderer->view().frameView().contentsToRootView(rect);
> 
> Rs=you to do this?

Nevermind, I just saw https://bugs.webkit.org/show_bug.cgi?id=158667.

> 
> Thanks for explaining why going through the render tree is a better option.

[Darin Adler]

Yes, no problem. I will fix this and do some other things to the code in that class and related classes; no rush to do it, I will finish it when I have the time.