[webkit-reviews] review denied: [Bug 43099] Add JavaScript API to allow a page to go fullscreen : [Attachment 62968] FullScreen-WebCore
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jul 29 12:32:47 PDT 2010
Simon Fraser (smfr) <simon.fraser at apple.com> has denied Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 43099: Add JavaScript API to allow a page to go fullscreen
https://bugs.webkit.org/show_bug.cgi?id=43099
Attachment 62968: FullScreen-WebCore
https://bugs.webkit.org/attachment.cgi?id=62968&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
In general, I think it might be helpful to split this commit up into multiple
commits:
1. Add non-working support for the DOM API and events.
2. Add the security-related logic.
3. Add the code to make them work, with tests
4. Add the iframe-related logic
5. Convert the existing video fullscreen to use this
Tests I would like to see:
1. Test that takes an element fullscreen, then removes that element from the
DOM
2. Test that takes an element fullscreen, then removes an ancestor of that
element from the DOM
3. Test that takes an element fullscreen, then sets display:none on that
element
4. Test that takes the document fullscreen, and then navigates
5. Test that takes the document fullscreen, then tries to take an element
fullscreen
> Index: JavaScriptCore/wtf/Platform.h
> ===================================================================
> --- JavaScriptCore/wtf/Platform.h (revision 64291)
> +++ JavaScriptCore/wtf/Platform.h (working copy)
> @@ -597,6 +597,7 @@
> #endif
> #define HAVE_READLINE 1
> #define HAVE_RUNLOOP_TIMER 1
> +#define ENABLE_FULLSCREEN 1
I think this needs a slightly more descriptive name, like
ENABLE_FULLSCREEN_API.
> Index: WebCore/bindings/objc/PublicDOMInterfaces.h
> ===================================================================
> --- WebCore/bindings/objc/PublicDOMInterfaces.h (revision 64291)
> +++ WebCore/bindings/objc/PublicDOMInterfaces.h (working copy)
> @@ -156,6 +156,11 @@ @interface DOMDocument : DOMNode WEBKIT_
> - (DOMNodeList *)getElementsByClassName:(NSString *)tagname
AVAILABLE_IN_WEBKIT_VERSION_4_0;
> - (DOMElement *)querySelector:(NSString *)selectors
AVAILABLE_IN_WEBKIT_VERSION_4_0;
> - (DOMNodeList *)querySelectorAll:(NSString *)selectors
AVAILABLE_IN_WEBKIT_VERSION_4_0;
> +#if defined(ENABLE_FULLSCREEN) && ENABLE_FULLSCREEN
> +- (void)webkitRequestFullScreen AVAILABLE_IN_WEBKIT_VERSION_4_0;
> +- (void)webkitRequestFullScreenWithKeys AVAILABLE_IN_WEBKIT_VERSION_4_0;
> +- (void)webkitCancelFullScreen AVAILABLE_IN_WEBKIT_VERSION_4_0;
> +#endif
> @end
>
> @interface DOMDocumentFragment : DOMNode WEBKIT_VERSION_1_3
> @@ -224,6 +229,10 @@ @interface DOMElement : DOMNode WEBKIT_V
> - (DOMNodeList *)getElementsByClassName:(NSString *)name
AVAILABLE_IN_WEBKIT_VERSION_4_0;
> - (DOMElement *)querySelector:(NSString *)selectors
AVAILABLE_IN_WEBKIT_VERSION_4_0;
> - (DOMNodeList *)querySelectorAll:(NSString *)selectors
AVAILABLE_IN_WEBKIT_VERSION_4_0;
> +#if defined(ENABLE_FULLSCREEN) && ENABLE_FULLSCREEN
> +- (void)webkitRequestFullScreen AVAILABLE_IN_WEBKIT_VERSION_4_0;
> +- (void)webkitRequestFullScreenWithKeys AVAILABLE_IN_WEBKIT_VERSION_4_0;
> +#endif
> @end
>
> @interface DOMEntity : DOMNode WEBKIT_VERSION_1_3
> @@ -569,6 +578,9 @@ @interface DOMHTMLIFrameElement : DOMHTM
> @property(copy) NSString *width;
> @property(readonly, retain) DOMDocument *contentDocument;
> @property(readonly, retain) DOMAbstractView *contentWindow
AVAILABLE_IN_WEBKIT_VERSION_4_0;
> +#if defined(ENABLE_FULLSCREEN) && ENABLE_FULLSCREEN
> + at property(assign) BOOL webkitAllowFullScreen;
> +#endif
> @end
Does +#if ENABLE(FULLSCREEN) not work here?
> Index: WebCore/css/CSSSelector.cpp
> ===================================================================
> +#if ENABLE(FULLSCREEN)
> + DEFINE_STATIC_LOCAL(AtomicString, fullScreen, ("-webkit-full-screen"));
> + DEFINE_STATIC_LOCAL(AtomicString, fullScreenDoc,
("-webkit-full-screen-doc"));
> + DEFINE_STATIC_LOCAL(AtomicString, fullScreenRootWithTarget,
("-webkit-full-screen-root-with-target"));
> +#endif
I think -webkit-full-screen-doc should not abbreviate "document' (also applies
to all the variable and type names).
This would require feedback on the Mozilla proposal (which should happen in
what-wg, not just via the wiki).
We use "full-page" elsewhere; should "full-screen-doc" be "full-screen-page"?
> Index: WebCore/css/CSSStyleSelector.cpp
> ===================================================================
> static void loadSimpleDefaultStyle()
> @@ -563,6 +571,13 @@ static void loadSimpleDefaultStyle()
> defaultStyle->addRulesFromSheet(simpleDefaultStyleSheet, screenEval());
>
> // No need to initialize quirks sheet yet as there are no quirk rules
for elements allowed in simple default style.
> +
> +#if ENABLE(FULLSCREEN)
> + // Full-screen rules.
> + String fullscreenRules = String(fullscreenUserAgentStyleSheet,
sizeof(fullscreenUserAgentStyleSheet)) +
RenderTheme::defaultTheme()->extraDefaultStyleSheet();
> + CSSStyleSheet* fullscreenSheet = parseUASheet(fullscreenRules);
> + defaultStyle->addRulesFromSheet(fullscreenSheet, screenEval());
> +#endif
It's not obvious to me that we want to load fullscreen rules in
loadSimpleDefaultStyle().
> Index: WebCore/css/fullscreen.css
> ===================================================================
> +:-webkit-full-screen {
> + position:fixed;
> + top:0;
> + left:0;
> + right:0;
> + bottom:0;
> +}
> +:-webkit-full-screen-root-with-target {
> + overflow:hidden;
> +}
> +
> +video:-webkit-full-screen {
> + width: 100%;
> + height: 100%
> + image-fit: fill;
Mixed 2- and 4-space indentation here.
> \ No newline at end of file
Fix this.
> Index: WebCore/dom/Document.cpp
> ===================================================================
> +#if ENABLE(FULLSCREEN)
> + else if (eventType == eventNames().webkitfullscreenchangeEvent)
> + addListenerType(FULLSCREEN_LISTENER);
> +#endif
Should FULLSCREEN_LISTENER be FULLSCREEN_CHANGE_LISTENER?
> +void Document::webkitWillEnterFullScreenForElement(Element* element)
> +{
> + if (!page() || !page()->settings()->fullScreenEnabled())
> + return;
I find it odd that this does these checks again; shouldn't it just assert?
> + m_fullScreenElement = element ? element : documentElement();
Why is m_fullScreenElement being set again? It seems like the role of this
method is
being confused with the "request" methods. By this time, you should have
everything
set up already.
> +void Document::webkitDidExitFullScreenForElement(Element*)
> +{
> + if (!page())
> + return;
> +
> + if (!page()->settings()->fullScreenEnabled())
> + return;
Seems bad to bail here too. If you're in fullscreen, you sure need to be able
to come out.
> Index: WebCore/dom/Document.h
> ===================================================================
> +#if ENABLE(FULLSCREEN)
> + bool webkitIsFullScreen() const { return m_isFullScreen; }
> + bool webkitIsFullScreenWithKeysEnabled() const { return m_isFullScreen
&& m_areKeysEnabled; }
I don't really like the "keys enabled" nomenclature. "keys" is too generic; it
should be
"key input". (This is feedback for the mozilla proposal too.)
> + Element* webkitCurrentFullScreenElement() const { return
m_fullScreenElement; }
> + void webkitRequestFullScreen();
> + void webkitRequestFullScreenWithKeys();
Should be webkitRequestFullScreenAllowingKeyboardInput().
I'm surprised these 'request fullscreen' methods don't return a bool.
> + void webkitRequestFullScreenForElementWithKeys(Element*, bool
keysEnabled);
Ditto.
> + void webkitCancelFullScreen();
Does "cancel" mean "exit"?
> +#if ENABLE(FULLSCREEN)
> + bool m_isFullScreen;
> + bool m_areKeysEnabled;
The variable name should be qualified by fullscreen (otherwise it reads like it
affects
the Document all the time), or you combine this and m_isFullScreen in a enum.
> + Element* m_fullScreenElement;
You may want to make this a RefPtr<> (taking care to avoid ref cycles).
> Index: WebCore/dom/Document.idl
> ===================================================================
> --- WebCore/dom/Document.idl (revision 64291)
> +++ WebCore/dom/Document.idl (working copy)
> @@ -245,6 +245,15 @@ module core {
> [DontEnum] void initializeWMLPageState();
> #endif
>
> +#if defined(ENABLE_FULLSCREEN) && ENABLE_FULLSCREEN
> + void webkitRequestFullScreen();
> + void webkitRequestFullScreenWithKeys();
Why don't these return bools?
> + readonly attribute boolean webkitIsFullScreen;
> + readonly attribute boolean webkitIsFullScreenWithKeysEnabled;
> + readonly attribute Element webkitCurrentFullScreenElement;
> + void webkitCancelFullScreen();
'cancel' should be 'exit'.
> Index: WebCore/dom/EventNames.h
> ===================================================================
> + macro(webkitfullscreenchange)
> + \
It's unfortunate that we use both "foochange" and "foochanged" for events. I
find
the latter more readable.
> Index: WebCore/html/HTMLIFrameElement.h
> ===================================================================
> +#if ENABLE(FULLSCREEN)
> + bool webkitAllowFullScreen() const { return m_allowFullScreen; }
> + void setWebkitAllowFullScreen(bool value) { m_allowFullScreen = value; }
> +#endif
These methods should not use the webkit prefix. The only places you have to use
the prefix
are those exposed to content, otherwise, when the prefix is removed, you have
to make
lots of extra changes.
> +#if ENABLE(FULLSCREEN)
> + bool m_allowFullScreen;
> +#endif
I think m_allowsFullScreen would be better.
> Index: WebCore/page/Settings.h
> ===================================================================
> +#if ENABLE(FULLSCREEN)
> + bool m_fullScreenEnabled : 1;
m_fullScreenEnabled would be better as m_fullScreenAPIEnabled.
More information about the webkit-reviews
mailing list