Bug 62507 – REGRESSION (full screen video): Watch Again button is obscured after full screen playback ends at Apple trailers page

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

REGRESSION (full screen video): Watch Again button is obscured after full screen playback ends at Apple trailers page

[Jer Noble]

Created attachment 96872 [details]
Patch

[WebKit Review Bot]

Attachment 96872 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/Document.h:1063:  The parameter name "method" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

<rdar://problem/9594595>

[Jer Noble]

Created attachment 96887 [details]
Patch

Fixed style errors.

[Darin Adler]

Comment on attachment 96887 [details]
Patch

My original complaint was driven by the fact that I did not understand this function was called in a loop up the entire parent chain. I thought it was called once, and was puzzled about it. Now that I understand it, here are my two complaints about the existing function:

    - It doesn’t seem good to have a function that operates on all ancestors incorporate the word “recursively”. There’s no recursion here, just a simple loop that goes up all ancestors. In the context of nodes, “recursively” almost always means that all descendants are involved, since recursion is a common way to walk all the descendants.

    - There is no need to have this function be a member of the class Document. This could be a free function, private to Document.cpp and not in the header at all.

    - The operation of walking one step up the parent chain and crossing frame boundaries into owner elements would be clearer as a named function rather than a ? : expression.

I suggest naming this function setContainsFullScreenElementIncludingAncestorsCrossingFrameBoundaries, or a shorter similar name.

I suggest we make a function called something like parentCrossingFrameBoundaries. This can be a free function, private to Document.cpp, or could even be an Element member function. Once we have that, then we can easily just call it like this:

    setContainsFullScreenElementIncludingAncestorsCrossingFrameBoundaries(parentCrossingFrameBoundaries(m_fullScreenElement.get()), false);

Or if you really want to avoid this, you can just put that single in into a named function and use it like this:

    setContainsFullScreenElementOnAncestorsCrossingFrameBoundaries(m_fullScreenElement.get(), false);

I know those are long function names, but I think they are sufficiently clear, and this would all be private to Document.cpp.

r=me but *please* don't use the enum as you do here

Also, I suggest doing the refactoring in a separate patch that does not change behavior, and land the bug fix on its own. I assume the bug fix is adding the new code to Document::fullScreenElementRemoved.

[Jer Noble]

Just a quick reply to a subset of your comments:

(In reply to comment #5)
> (From update of attachment 96887 [details])
>     - There is no need to have this function be a member of the class Document. This could be a free function, private to Document.cpp and not in the header at all.

Actually, the only reason it's in the Document.h header is because it's also used in Element.cpp as well.  The reason I made the enum parameter have a default was so as not to affect those call sites.

As for the rest, I'll make the simple fix here, and file & fix the rest of your comments in a new bug.

> r=me but *please* don't use the enum as you do here

Sure thing.  Thanks!

[Jer Noble]

Filed bug #62528 to cover the refactoring of Document::setContainsFullScreenElementRecursively.

[Jer Noble]

Also filed bug #62529 to track fixing the DRT to catch the regression causing this bug.

[Jer Noble]

Committed r88629: <http://trac.webkit.org/changeset/88629>