1150144 - Make HTMLVideoElement::GetVideoSize use CSSIntSize instead of the unitless nsIntSize

Status: NEW

(Core :: Layout, defect, P4)

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b85d7b914dcb

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8586897 [details] [diff] [review]
fix

Review of attachment 8586897 [details] [diff] [review]:
-----------------------------------------------------------------

r=me regardless; two optional observations/nits below:

::: dom/html/HTMLVideoElement.h
@@ +48,2 @@
>    // If there is no video frame, returns NS_ERROR_FAILURE.
> +  nsresult GetVideoSize(CSSIntSize* size);

I'm a little mixed about using "CSS Pixels" in the documentation here. Do we have documentation on what "CSS Pixels" means somewhere?

I suspect the returned size is really in terms of "pixels of actual native video data". (e.g. if this is a 300x300 video, rendered in <video style="height:100px;width:50px">, I think (?) this method would still return 300 x 300, not the CSS-specified size -- but the use of the term "CSS pixels" is a bit ambiguous about which of these behaviors someone should expect.)

So, the documentation might be clearer just saying "in pixels" [implying pixels of the actual video-frame]? Or maybe not. Whatever you think WFM.

::: layout/generic/nsVideoFrame.cpp
@@ +161,5 @@
>  {
>    nsRect area = GetContentRectRelativeToSelf() + aItem->ToReferenceFrame();
>    HTMLVideoElement* element = static_cast<HTMLVideoElement*>(GetContent());
>  
> +  CSSIntSize videoSizeInPx;

Looks like this file (nsVideoFrame) doesn't have an #include for "Units.h" -- maybe we should add one? (I suppose it'll get one indirectly via HTMLVideoElement.h, so maybe that's fine.)

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.