[webkit-reviews] review granted: [Bug 61220] Video looks squished when animating to full screen. : [Attachment 94302] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 61220: Video looks squished when animating to full screen.
https://bugs.webkit.org/show_bug.cgi?id=61220

Attachment 94302: Patch
https://bugs.webkit.org/attachment.cgi?id=94302&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=94302&action=review

> Source/WebCore/ChangeLog:13
> +	   Added a new css pseudo-class: -webkit-animating-full-screen.  During
the transition animation, this
> +	   pseudo-class will be applied to the current full-screen element. 
Styles have been added to 
> +	   fullscreenQuickTime.css to hide the video element's built-in
controller during the full-screen
> +	   animation.

How does removing the controller relate to “video looks squished”? I understand
that all this new code makes the controller disappear during the transition to
full screen, but I don’t have the slightest idea why that is a good idea, and
the change log does not tell me.

> Source/WebCore/css/CSSSelector.cpp:327
> +    DEFINE_STATIC_LOCAL(AtomicString, animatingFullScreen,
("-webkit-animating-full-screen"));

The name “animating full screen” doesn’t seem like clear language to me. That
term makes it sound like we’re animating something that’s covering an entire
screen. To someone not working on this it does not immediately point to the
full screen transition animation. Instead I would prefer a name that is
explicitly mentions the transition into or out of full screen rather than
simply mentioning full screen and also animation. Maybe even two different
pseudo-classes, one for the animation into and the other for the animation out
of the full screen state.

> Source/WebCore/css/CSSStyleSelector.cpp:2909
>		   if (e != e->document()->webkitCurrentFullScreenElement())
>		       return false;
>		   return true;

I think it's easier to read something like this as a boolean expression:

    return e == e->document()->webkitCurrentFullScreenElement();

> Source/WebCore/css/CSSStyleSelector.cpp:2913
> +		   if (e != e->document()->webkitCurrentFullScreenElement())
> +		       return false;
> +		   return e->document()->isAnimatingFullScreen();

I think it's easier to read something like this as a boolean expression:

    return e == e->document()->webkitCurrentFullScreenElement() &&
e->document()->isAnimatingFullScreen();

> Source/WebCore/dom/Document.cpp:4898
> +    m_fullScreenElement->recalcStyle(Force);

Normally when a pseudo-style has changed we don’t call recalcStyle directly,
and we certainly don’t call it directly on a single specific element. I’d
expect to instead see a call here to scheduleStyleRecalc (or possibly
scheduleForcedStyleRecalc, but I think that would be wrong). I’m surprised by
this, and I think it’s probably wrong.

> Source/WebKit2/ChangeLog:11
> +	   * WebProcess/FullScreen/mac/WebFullScreenManagerMac.mm:
> +	   (WebKit::WebFullScreenManagerMac::beginEnterFullScreenAnimation):
Set the destination frame
> +	       to be the content box of the current full screen element.
> +	   (WebKit::WebFullScreenManagerMac::beginExitFullScreenAnimation):
Ditto.

I don’t understand why these changes are here, and how they relate to the
WebCore changes above. The change log states the change being made, but neither
the change log nor the code touch on the *why* aspect. Was there something
wrong before? What was the symptom? How does this change help?