[webkit-reviews] review granted: [Bug 208904] Safari sometimes crashes when switch video into PiP mode : [Attachment 393399] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Peng Liu
<peng.liu6 at apple.com>'s request for review:
Bug 208904: Safari sometimes crashes when switch video into PiP mode
https://bugs.webkit.org/show_bug.cgi?id=208904

Attachment 393399: Patch

https://bugs.webkit.org/attachment.cgi?id=393399&action=review




--- Comment #14 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 393399
  --> https://bugs.webkit.org/attachment.cgi?id=393399
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5814
> +	   document().clearMediaElementShowingTextTrack();

I don't see a call to clearMediaElementShowingTextTrack() if the media element
is removed from the document. There probably needs to be another call from
HTMLMediaElement::removedFromAncestor() or something.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1216
> +void MediaControlTextTrackContainerElement::updateTextTrackSizeAndStyle()

This is called "size and style" but it doesn't do anything with style (which is
a good thing).

> Source/WebCore/html/shadow/MediaControlElements.cpp:1237
> +void
MediaControlTextTrackContainerElement::updateTextTrackRepresentationImageIfNeed
ed()
> +{
> +    if (!m_needsPaint)
> +	   return;
> +
> +    m_needsPaint = false;
> +
> +    // We can only call m_textTrackRepresentation->update() after the layout
> +    // is clean, because m_textTrackRepresentation->update() will call
> +    // createTextTrackRepresentationImage() to paint the subtree of
> +    // RenderTextTrackContainerElement to an image buffer.
> +    if (m_textTrackRepresentation)
> +	   m_textTrackRepresentation->update();

m_needsPaint should be named to reflect the functions that get called when it
changes state. That's called "update" here, which is vague. It's more like
"generate text track representation". The fact that it's painting into an image
buffer is implementation detail.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1447
>      updateActiveCuesFontSize();
> -    updateDisplay();
>      updateTextStrokeStyle();

It looks like there's style changing happening here, which should really happen
along with other style changes on the document (before styleResolve). If so,
please file a bug to fix.