[webkit-reviews] review granted: [Bug 62507] REGRESSION (full screen video): Watch Again button is obscured after full screen playback ends at Apple trailers page : [Attachment 96887] 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 62507: REGRESSION (full screen video): Watch Again button is obscured after
full screen playback ends at Apple trailers page
https://bugs.webkit.org/show_bug.cgi?id=62507

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

------- Additional Comments from Darin Adler <darin at apple.com>
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(parentCro
ssingFrameBoundaries(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_fullScreenElem
ent.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.