[webkit-reviews] review granted: [Bug 62528] Rename Document::setContainsFullScreenElementRecursively : [Attachment 96933] 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 62528: Rename Document::setContainsFullScreenElementRecursively
https://bugs.webkit.org/show_bug.cgi?id=62528

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

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

> Source/WebCore/dom/Element.cpp:996
> -    if (parentElement() && containsFullScreenElement() &&
!parentElement()->containsFullScreenElement())
> -	   document()->setContainsFullScreenElementRecursively(parentElement(),
true);
> +    if (containsFullScreenElement() && parentElement() &&
!parentElement()->containsFullScreenElement())
> +	  
setContainsFullScreenElementOnAncestorsCrossingFrameBoundaries(true);

I don’t think the additional checks in this if statement are needed or helpful.
I see no compelling reason to optimize for this case, and it seems it could
cause problems if the element being inserted is the top level element; we’ll
miss out on the code that follows the ownerElement pointer. But it may be that
the top level element is never inserted into a tree at a time where this
matters.

> Source/WebCore/dom/Element.cpp:1931
> +void
Element::setContainsFullScreenElementOnAncestorsCrossingFrameBoundaries(bool
flag)

I probably would not make this a member function, just because I don’t want to
have to modify Element.h every time I change it. But I think it’s OK as one.