Bug 62528 - Rename Document::setContainsFullScreenElementRecursively
Summary: Rename Document::setContainsFullScreenElementRecursively
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
Depends on:
Reported: 2011-06-12 22:21 PDT by Jer Noble
Modified: 2011-06-18 13:35 PDT (History)
2 users (show)

See Also:

Patch (7.07 KB, patch)
2011-06-13 01:14 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-06-12 22:21:30 PDT
Document::setContainsFullScreenElementRecursively is named poorly, as it does not actually use recursion.  Also, make explicit that the function walks up the ancestor chain and crosses frame boundaries.

(See Darin's comments in https://bugs.webkit.org/show_bug.cgi?id=62507#c5.)
Comment 1 Jer Noble 2011-06-13 01:14:45 PDT
Created attachment 96933 [details]
Comment 2 Jer Noble 2011-06-13 01:18:53 PDT
In the end, I made setContainsFullScreenElementOnAncestorsCrossingFrameBoundaries() a member function in Element (alongside setContainsFullScreenElement()), so it could be used so:

Comment 3 Darin Adler 2011-06-13 08:19:30 PDT
Comment on attachment 96933 [details]

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.
Comment 4 WebKit Review Bot 2011-06-18 13:35:10 PDT
Comment on attachment 96933 [details]

Clearing flags on attachment: 96933

Committed r89205: <http://trac.webkit.org/changeset/89205>
Comment 5 WebKit Review Bot 2011-06-18 13:35:14 PDT
All reviewed patches have been landed.  Closing bug.