Bug 43099

Created attachment 62784 [details] FullScreen-WebCore

Created attachment 62785 [details] FullScreen-WebKit

Created attachment 62786 [details] FullScreen-WebKitTools

Created attachment 62787 [details] FullScreen-LayoutTests

Attachment 62784 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/html/HTMLMediaElement.cpp:2016: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/dom/WebKitFullScreenChangeEvent.cpp:38: Code inside a namespace should not be indented. [whitespace/indent] [4] WebCore/dom/WebKitFullScreenChangeEvent.h:35: Code inside a namespace should not be indented. [whitespace/indent] [4] WebCore/dom/WebKitFullScreenChangeEvent.h:56: Should have a space between // and comment [whitespace/comments] [4] WebCore/dom/EventNames.h:163: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 5 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 62785 [details] did not build on mac: Build output: http://queues.webkit.org/results/3619254

Attachment 62784 [details] did not build on qt: Build output: http://queues.webkit.org/results/3585620

Attachment 62784 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3638043

Created attachment 62793 [details] FullScreen-WebCore Fixed a few style errors (except for the one in EventNames.h).

Attachment 62793 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/EventNames.h:163: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 62793 [details] did not build on qt: Build output: http://queues.webkit.org/results/3636054

Comment on attachment 62793 [details] FullScreen-WebCore Cancelling review+ while I work on fixing the qt and gtk compiles.

Created attachment 62797 [details] FullScreen-WebCore Changed the way that the new full screen symbols are exported from WebCore. Also, the HTMLVideo and MediaElements retain their original implementations if ENABLE_FULLSCREEN is not turned on.

Created attachment 62799 [details] FullScreen-WebCore Recreated patch after resolving conflicts in the FeatureDefines.xcconfig file.

Attachment 62799 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/html/HTMLMediaElement.cpp:2021: Tab found; better to use spaces [whitespace/tab] [1] WebCore/dom/EventNames.h:163: Code inside a namespace should not be indented. [whitespace/indent] [4] WebCore/html/HTMLVideoElement.cpp:142: Tab found; better to use spaces [whitespace/tab] [1] WebCore/html/HTMLVideoElement.cpp:145: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 4 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.

I'll fix the tabs next time I upload the FullScreen-WebCore patch.

Attachment 62799 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3588550

Attachment 62799 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3643004

Created attachment 62833 [details] FullScreen-WebCore Fixed the mismatched braces in HTMLElement.cpp when ENABLE_FULLSCREEN is turned off.

Created attachment 62835 [details] FullScreen-WebCore Fixed the merge conflicts resulting from <r64210.

Attachment 62835 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/EventNames.h:163: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 62835 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3642031

Attachment 62835 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3588563

Created attachment 62874 [details] FullScreen-WebCore Unprotected ChromeClient::enter/exit/supportsFullScreenForElement().

Attachment 62874 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/EventNames.h:163: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 62954 [details] FullScreen-WebCore Figured out the way the new exports mechanism works. Added the symbols to WebCore.exp.in instead of ExportFileGenerator.cpp directly. Also, modified HTMLIFrameElement.idl so the allowsFullScreen attribute is a boolean type (instead of a string).

Comment on attachment 62954 [details] FullScreen-WebCore Clearing review? due to merge conflicts.

Created attachment 62968 [details] FullScreen-WebCore Resolved merge conflicts.

Attachment 62968 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/EventNames.h:163: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 62968 [details] FullScreen-WebCore 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 > +@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.

(In reply to comment #30) First, much of your review focuses on changes you would like seen made to the Mozilla proposal. I don't believe this is the correct forum for those changes. The patch, as it is, hews very closely to the current Mozilla API proposal purposefully, so as to better understand the shortcomings of the actual proposal. I don't foresee this being the final implementation; the proposal will surely change with feedback from the community and the implementation will have to change to match. > 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 I'll work on adding those tests to the LayoutTests patch. > I think this needs a slightly more descriptive name, like ENABLE_FULLSCREEN_API. OK. > > +#if defined(ENABLE_FULLSCREEN) && ENABLE_FULLSCREEN > > +@property(assign) BOOL webkitAllowFullScreen; > > +#endif > > @end > > Does +#if ENABLE(FULLSCREEN) not work here? PublicDOMInterfaces.h does not include any files, and cannot therefore guarantee the ENABLE() macro has been defined. > 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). This is a (relatively) strict implementation of the Mozilla spec. I deliberately did not make any design or naming changes, apart from the -webkit prefix. > We use "full-page" elsewhere; should "full-screen-doc" be "full-screen-page"? Seems like a good point for discussion with the what-wg. > It's not obvious to me that we want to load fullscreen rules in loadSimpleDefaultStyle(). OK. > > 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. OK. > > \ No newline at end of file > > Fix this. OK. > > Index: WebCore/dom/Document.cpp > > =================================================================== > > +#if ENABLE(FULLSCREEN) > > + else if (eventType == eventNames().webkitfullscreenchangeEvent) > > + addListenerType(FULLSCREEN_LISTENER); > > +#endif > > Should FULLSCREEN_LISTENER be FULLSCREEN_CHANGE_LISTENER? OK. (FULLSCREENCHANGE_LISTENER, to match the existing definitions.) > > +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? OK. > > + 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. Whoops, m_fullScreenElement should not be set in webkitRequestFullScreenForElementWithKeys. > > +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. OK. > > 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.) Again, this is a discussion to have with the what-wg or Mozilla. > I'm surprised these 'request fullscreen' methods don't return a bool. They are not specced to return a bool. > > + void webkitCancelFullScreen(); > > Does "cancel" mean "exit"? From the Mozilla proposal: "Requests that the UA exit fullscreen mode. The UA is not required to honor this, for example the UA might require that only a Document that last triggered fullscreen can cancel it." > > +#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. It could be changed to m_areKeysEnabledInFullScreen; > > + Element* m_fullScreenElement; > > You may want to make this a RefPtr<> (taking care to avoid ref cycles). OK. > > 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? They aren't specced to. From the proposal: "Typically the user agent would react by transitioning the Document to the fullscreen state, or by presenting asynchronous confirmation UI and transitioning to the fullscreen state if/when the user responds affirmatively." > > + readonly attribute boolean webkitIsFullScreen; > > + readonly attribute boolean webkitIsFullScreenWithKeysEnabled; > > + readonly attribute Element webkitCurrentFullScreenElement; > > + void webkitCancelFullScreen(); > > 'cancel' should be 'exit'. That's not the spec. > > 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. They are exposed to content, by way of the DOMHTMLIFrameElement and JSHTMLIFrameElement. > > +#if ENABLE(FULLSCREEN) > > + bool m_allowFullScreen; > > +#endif > > I think m_allowsFullScreen would be better. OK. > > Index: WebCore/page/Settings.h > > =================================================================== > > > +#if ENABLE(FULLSCREEN) > > + bool m_fullScreenEnabled : 1; > > m_fullScreenEnabled would be better as m_fullScreenAPIEnabled. OK.

Created attachment 63360 [details] FullScree-LayoutTests

Created attachment 63361 [details] FullScreen-WebCore

Attachment 63361 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/EventNames.h:163: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 63397 [details] FullScreen-WebCore Fixed some merge conflicts that were confusing the EWS bots.

Attachment 63397 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/EventNames.h:163: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 63476 [details] FullScreen-WebCore Updated patch after speaking with Robert O'Callahan about the Mozilla proposal.

Created attachment 63477 [details] FullScreen-LayoutTests Updated LayoutTests to match changes to the Mozilla proposal.

Attachment 63476 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/EventNames.h:163: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 63476 [details] FullScreen-WebCore I agree with Simon's comment above that it would be nice to break this into several smaller patches, this is a lot for one patch! https://bugs.webkit.org/show_bug.cgi?id=34942 change all of Webkit to use "FullScreen" instead of "Fullscreen" and this goes back to using inner caps... I realize that these names are from the Mozilla proposal, but this is one more thing to discuss on the mailing list. > + (WebCore::Event::isWebKitFullScreenChangeEvent): Overrided by WebKitFullScreenChangeEvent. Typo, you want "overridden" I think. > + (WebCore::HTMLMediaElement::HTMLMediaElement): > + (WebCore::HTMLMediaElement::parseMappedAttribute): > + (WebCore::HTMLMediaElement::removedFromDocument): > + (WebCore::HTMLMediaElement::documentWillBecomeInactive): > + (WebCore::HTMLMediaElement::isFullscreen): > + (WebCore::HTMLMediaElement::enterFullscreen): > + (WebCore::HTMLMediaElement::exitFullscreen): No comments about what changed. > + (WebCore::HTMLVideoElement::supportsFullscreen): Ditto. > +void Document::webkitRequestFullScreenForElementWithKeys(Element* element, bool keysEnabled) > +{ > + if (!page() || !page()->settings()->fullScreenEnabled()) > + return; > + > + if (!element) > + element = documentElement(); > + > + if (page()->chrome()->client()->supportsFullscreenForElement(element)) { Might as well return early if we can't go fullscreen for the element. > +void Document::webkitCancelFullScreen() > +{ > + if (!page() || !page()->settings()->fullScreenEnabled()) > + return; > + > + if (page()->chrome()->client()->supportsFullscreenForElement(m_fullScreenElement.get())) > + page()->chrome()->client()->exitFullscreenForElement(m_fullScreenElement.get()); > +} You definitely shouldn't bail if fullscreen is disabled, and presumably m_fullScreenElement can not exist unless supportsFullscreenForElement() returned true for it previously, so maybe you should have something like this instead: if (!page() || !m_fullScreenElement) return; page()->chrome()->client()->exitFullscreenForElement(m_fullScreenElement.get()); > Index: WebCore/html/HTMLMediaElement.cpp > =================================================================== > --- WebCore/html/HTMLMediaElement.cpp (revision 64653) > +++ WebCore/html/HTMLMediaElement.cpp (working copy) > @@ -312,7 +312,7 @@ void HTMLMediaElement::removedFromDocume > { > if (m_networkState > NETWORK_EMPTY) > pause(processingUserGesture()); > - if (m_isFullscreen) > + if (isFullscreen()) > exitFullscreen(); > HTMLElement::removedFromDocument(); > } > @@ -1852,7 +1852,7 @@ void HTMLMediaElement::userCancelledLoad > > void HTMLMediaElement::documentWillBecomeInactive() > { > - if (m_isFullscreen) > + if (isFullscreen()) > exitFullscreen(); Why does the media element need to exit manually when ENABLE(FULLSCREEN_API) is true?

(In reply to comment #40) > (From update of attachment 63476 [details]) > I agree with Simon's comment above that it would be nice to break this into > several smaller patches, this is a lot for one patch! Okay, I'll see what I can do. > https://bugs.webkit.org/show_bug.cgi?id=34942 change all of Webkit to use > "FullScreen" instead of "Fullscreen" and this goes back to using inner > caps... I realize that these names are from the Mozilla proposal, but > this is one more thing to discuss on the mailing list. Noted. :) > > + (WebCore::Event::isWebKitFullScreenChangeEvent): Overrided by WebKitFullScreenChangeEvent. > > Typo, you want "overridden" I think. OK. > > + (WebCore::HTMLMediaElement::HTMLMediaElement): > > + (WebCore::HTMLMediaElement::parseMappedAttribute): > > + (WebCore::HTMLMediaElement::removedFromDocument): > > + (WebCore::HTMLMediaElement::documentWillBecomeInactive): > > + (WebCore::HTMLMediaElement::isFullscreen): > > + (WebCore::HTMLMediaElement::enterFullscreen): > > + (WebCore::HTMLMediaElement::exitFullscreen): > > No comments about what changed. > > > + (WebCore::HTMLVideoElement::supportsFullscreen): > > Ditto. I'll fill these out with more detail. > > +void Document::webkitRequestFullScreenForElementWithKeys(Element* element, bool keysEnabled) > > +{ > > + if (!page() || !page()->settings()->fullScreenEnabled()) > > + return; > > + > > + if (!element) > > + element = documentElement(); > > + > > + if (page()->chrome()->client()->supportsFullscreenForElement(element)) { > > Might as well return early if we can't go fullscreen for the element. OK. > > +void Document::webkitCancelFullScreen() > > +{ > > + if (!page() || !page()->settings()->fullScreenEnabled()) > > + return; > > + > > + if (page()->chrome()->client()->supportsFullscreenForElement(m_fullScreenElement.get())) > > + page()->chrome()->client()->exitFullscreenForElement(m_fullScreenElement.get()); > > +} > > You definitely shouldn't bail if fullscreen is disabled, and presumably m_fullScreenElement > can not exist unless supportsFullscreenForElement() returned true for it previously, so maybe > you should have something like this instead: > > if (!page() || !m_fullScreenElement) > return; > page()->chrome()->client()->exitFullscreenForElement(m_fullScreenElement.get()); OK. > > Index: WebCore/html/HTMLMediaElement.cpp > > =================================================================== > > --- WebCore/html/HTMLMediaElement.cpp (revision 64653) > > +++ WebCore/html/HTMLMediaElement.cpp (working copy) > > @@ -312,7 +312,7 @@ void HTMLMediaElement::removedFromDocume > > { > > if (m_networkState > NETWORK_EMPTY) > > pause(processingUserGesture()); > > - if (m_isFullscreen) > > + if (isFullscreen()) > > exitFullscreen(); > > HTMLElement::removedFromDocument(); > > } > > @@ -1852,7 +1852,7 @@ void HTMLMediaElement::userCancelledLoad > > > > void HTMLMediaElement::documentWillBecomeInactive() > > { > > - if (m_isFullscreen) > > + if (isFullscreen()) > > exitFullscreen(); > > Why does the media element need to exit manually when ENABLE(FULLSCREEN_API) is true? I was trying to disrupt the media element as little as possible on this round of patches. I'll break the media/video portion out into its own patch and address this comment there.

Created attachment 63623 [details] FullScreen-WebCore-ChangeLogs Splitting up FullScreen-WebCore.patch into 5 parts. With this break up, individual patches will not compile autonomously. Such is the price to pay to get patches reviewed.

Created attachment 63624 [details] FullScreen-WebCore-Config

Created attachment 63626 [details] FullScreen-WebCore-IDL

Created attachment 63627 [details] FullScreen-WebCore-MediaElements

Created attachment 63628 [details] FullScreen-WebCore

Attachment 63628 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/EventNames.h:163: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 62786 [details] FullScreen-WebKitTools > +- (BOOL)webView:(WebView *)wv supportsFullscreenForElement:(DOMElement*)element > +- (void)webView:(WebView *)wv enterFullscreenForElement:(DOMElement*)element listener:(NSObject<WebKitFullScreenListener>*)listener > +- (void)webView:(WebView *)wv exitFullscreenForElement:(DOMElement*)element listener:(NSObject<WebKitFullScreenListener>*)listener Please write "webView" in full here, not "wv".

(In reply to comment #48) > (From update of attachment 62786 [details]) > > +- (BOOL)webView:(WebView *)wv supportsFullscreenForElement:(DOMElement*)element > > +- (void)webView:(WebView *)wv enterFullscreenForElement:(DOMElement*)element listener:(NSObject<WebKitFullScreenListener>*)listener > > +- (void)webView:(WebView *)wv exitFullscreenForElement:(DOMElement*)element listener:(NSObject<WebKitFullScreenListener>*)listener > > Please write "webView" in full here, not "wv". OK. Thanks!

Comment on attachment 62785 [details] FullScreen-WebKit > Index: WebKit/mac/Configurations/FeatureDefines.xcconfig > =================================================================== You should probably land the xconfig changes for all projects at the same time. > Index: WebKit/mac/WebCoreSupport/WebChromeClient.mm > =================================================================== > -#if ENABLE(VIDEO) > +#if ENABLE(FULLSCREEN) These (and other) changes regress apps that get video fullscreen now, but won't have generic fullscreen enabled unless they set the preference. I think we should keep video fullscreen the way it works currently, and only switch over to generic fullscreen when we're ready to do so with zero regressions. > +#if ENABLE(FULLSCREEN) > +@implementation WebKitFullScreenListener > +- (id)initWithElement:(Element*)element > +{ > + if (!(self = [super init])) > + return nil; > + > + _element = element; > + return self; > +} > + > +- (void)webkitWillEnterFullScreen > +{ > + if (_element) > + _element->document()->webkitWillEnterFullScreenForElement(_element.get()); > +} > + > +- (void)webkitDidEnterFullScreen > +{ > + if (_element) > + _element->document()->webkitDidEnterFullScreenForElement(_element.get()); > +} > + > +- (void)webkitWillExitFullScreen > +{ > + if (_element) > + _element->document()->webkitWillExitFullScreenForElement(_element.get()); > +} > + > +- (void)webkitDidExitFullScreen > +{ > + if (_element) > + _element->document()->webkitDidExitFullScreenForElement(_element.get()); > +} > + > +@end I'm not sure I see the benefit of having this object that just turns around and calls back into the document. Why not make the WebView the UI delegate? Also fullscreen/fullScreen inconsistency here. > +#if ENABLE(FULLSCREEN) > +- (void)setFullScreenEnabled:(BOOL)flag > +{ > + [self _setBoolValue:flag forKey:WebKitFullScreenEnabledPreferenceKey]; > +} > + > +- (BOOL)fullScreenEnabled > +{ > + return [self _boolValueForKey:WebKitFullScreenEnabledPreferenceKey]; > +} > +#endif Fullscreen/fullScreen inconsistency here (and in the pref name). > Index: WebKit/mac/WebView/WebUIDelegatePrivate.h > =================================================================== > > +#if ENABLE(FULLSCREEN) > +@protocol WebKitFullScreenListener<NSObject> > +- (void)webkitWillEnterFullScreen; > +- (void)webkitDidEnterFullScreen; > +- (void)webkitWillExitFullScreen; > +- (void)webkitDidExitFullScreen; > +@end Fullscreen/fullScreen inconsistency. > +#if ENABLE(FULLSCREEN) > +- (BOOL)webView:(WebView *)sender supportsFullscreenForElement:(DOMElement *)element; > +- (void)webView:(WebView *)sender enterFullscreenForElement:(DOMElement *)element; > +- (void)webView:(WebView *)sender exitFullscreenForElement:(DOMElement *)element; > +#endif Fullscreen/fullScreen inconsistency. > @end > Index: WebKit/mac/WebView/WebView.mm > =================================================================== > --- WebKit/mac/WebView/WebView.mm (revision 64185) > +++ WebKit/mac/WebView/WebView.mm (working copy) > @@ -1434,6 +1434,9 @@ - (void)_preferencesChangedNotification: > settings->setHTML5ParserEnabled([preferences html5ParserEnabled]); > settings->setHTML5TreeBuilderEnabled_DO_NOT_USE([preferences html5TreeBuilderEnabled]); > settings->setPaginateDuringLayoutEnabled([preferences paginateDuringLayoutEnabled]); > +#if ENABLE(FULLSCREEN) > + settings->setFullScreenEnabled([preferences fullScreenEnabled]); > +#endif > settings->setMemoryInfoEnabled([preferences memoryInfoEnabled]); > } Fullscreen/fullScreen inconsistency. r- for breaking the existing video fullscreen.

Attachment 63627 [details] did not build on qt: Build output: http://queues.webkit.org/results/3559953

Comment on attachment 63477 [details] FullScreen-LayoutTests > Index: LayoutTests/ChangeLog > =================================================================== > + * fullscreen: Added. > + * fullscreen/full-screen-api-expected.txt: Added. > + * fullscreen/full-screen-api.html: Added. > + * fullscreen/full-screen-css-expected.txt: Added. > + * fullscreen/full-screen-css.html: Added. > + * fullscreen/full-screen-request-expected.txt: Added. > + * fullscreen/full-screen-request.html: Added. > + * fullscreen/full-screen-test.js: Added. > + (disableFullTestDetailsPrinting): > + (logConsole): > + (testAndEnd): > + (test): > + (testExpected): > + (reportExpected): > + (runSilently): > + (run): > + (waitForEventAndEnd): > + (waitForEvent._eventCallback): > + (waitForEvent): > + (waitForEventTestAndEnd): > + (waitForEventAndFail): > + (waitForEventAndTest._eventCallback): > + (waitForEventAndTest): > + (testException): > + (endTest): > + (endTestLater): > + (failTestIn): > + (failTest): > + (logResult): > + (consoleWrite): > + (relativeURL): > + (isInTimeRanges): You can remove the method listing from the Changelog for new files. > Index: LayoutTests/fullscreen/full-screen-css-expected.txt > =================================================================== > --- LayoutTests/fullscreen/full-screen-css-expected.txt (revision 0) > +++ LayoutTests/fullscreen/full-screen-css-expected.txt (revision 0) > @@ -0,0 +1,13 @@ > +EXPECTED (document.defaultView.getComputedStyle(span, null).getPropertyValue('background-color') != 'rgb(0, 255, 0)') OK > +EXPECTED (document.defaultView.getComputedStyle(document.documentElement, null).getPropertyValue('background-color') != 'rgb(255, 0, 0)') OK > +EXPECTED (document.defaultView.getComputedStyle(document.documentElement, null).getPropertyValue('color') != 'rgb(0, 0, 255)') OK > +EVENT(webkitfullscreenchange) > +EXPECTED (document.defaultView.getComputedStyle(span, null).getPropertyValue('background-color') == 'rgb(0, 255, 0)') OK > +EXPECTED (document.defaultView.getComputedStyle(document.documentElement, null).getPropertyValue('background-color') == 'rgb(255, 0, 0)') OK > +EXPECTED (document.defaultView.getComputedStyle(document.documentElement, null).getPropertyValue('color') == 'rgb(0, 0, 255)') OK > +EVENT(webkitfullscreenchange) > +EXPECTED (document.defaultView.getComputedStyle(span, null).getPropertyValue('background-color') == 'rgb(0, 255, 0)') OK > +EXPECTED (document.defaultView.getComputedStyle(document.documentElement, null).getPropertyValue('background-color') != 'rgb(255, 0, 0)'), OBSERVED 'rgb(255, 0, 0)' FAIL You have a FAIL in the results here. > Index: LayoutTests/fullscreen/full-screen-css.html > =================================================================== > +<script src=full-screen-test.js></script> Please quote the src attribute. > Index: LayoutTests/fullscreen/full-screen-remove-ancestor.html > =================================================================== > --- LayoutTests/fullscreen/full-screen-remove-ancestor.html (revision 0) > +++ LayoutTests/fullscreen/full-screen-remove-ancestor.html (revision 0) > @@ -0,0 +1,30 @@ > +<body> > +<script src=full-screen-test.js></script> > +<div><span></span></div> > +<script> > + var callback; > + var fullscreenChanged = function(event) > + { > + if (callback) > + callback(event) > + }; > + waitForEvent(document, 'webkitfullscreenchange', fullscreenChanged); > + > + var span = document.getElementsByTagName('span')[0]; > + var div = span.parentNode; > + > + var spanIsFullScreen = function(event) { > + callback = documentIsFullScreen; > + testExpected("document.webkitCurrentFullScreenElement", span); > + document.body.removeChild(div); > + }; spanIsFullScreen is a confusing name for the variable; maybe spanEnteredFullScreen. I don't see anything actually being tested.Shouldn't you test that you came out of fullscreen? > Index: LayoutTests/fullscreen/full-screen-remove.html > =================================================================== > +<body> > +<script src=full-screen-test.js></script> > +<div><span></span></div> > +<script> > + var callback; > + var fullscreenChanged = function(event) > + { > + if (callback) > + callback(event) > + }; > + waitForEvent(document, 'webkitfullscreenchange', fullscreenChanged); > + > + var span = document.getElementsByTagName('span')[0]; > + > + var spanIsFullScreen = function(event) { > + callback = documentIsFullScreen; > + testExpected("document.webkitCurrentFullScreenElement", span); > + span.parentNode.removeChild(span); > + }; Ditto. > Index: LayoutTests/fullscreen/full-screen-twice.html > =================================================================== > +<body> > +<script src=full-screen-test.js></script> > +<span></span> > +<script> > + var callback; > + var fullscreenChanged = function(event) > + { > + if (callback) > + callback(event) > + }; > + waitForEvent(document, 'webkitfullscreenchange', fullscreenChanged); > + > + var span = document.getElementsByTagName('span')[0]; > + > + var spanIsFullScreen = function() { > + testExpected("document.webkitCurrentFullScreenElement", span); > + callback = documentIsFullScreen; > + document.webkitRequestFullScreen(); > + }; Same comments about it being unclear what is being tested. If you run all the LayoutTests, you'll fine some others whose results dump all properties of the Document object. You'll have to supply new results for these tests, for platforms which enable the FULLSCREEN_API #ifdef.

Attachment 63627 [details] did not build on mac: Build output: http://queues.webkit.org/results/3583897

Comment on attachment 63624 [details] FullScreen-WebCore-Config > Index: JavaScriptCore/wtf/Platform.h > =================================================================== > --- JavaScriptCore/wtf/Platform.h (revision 64653) > +++ JavaScriptCore/wtf/Platform.h (working copy) > @@ -594,6 +594,7 @@ > #endif > #define HAVE_READLINE 1 > #define HAVE_RUNLOOP_TIMER 1 > +#define ENABLE_FULLSCREEN_API 1 I think this should go along with the things like: #if !defined(ENABLE_DRAG_SUPPORT) #define ENABLE_DRAG_SUPPORT 1 #endif and default to 0. It will get enabled via the xconfig files on platforms that want it. The rest of this patch doesn't stand alone, because it's adding new files to the projects without supplying the new files.

Comment on attachment 63623 [details] FullScreen-WebCore-ChangeLogs It doesn't really make sense to break out the changelogs into their own patch.

Attachment 63627 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3643341

Comment on attachment 63626 [details] FullScreen-WebCore-IDL > Index: WebCore/dom/WebKitFullScreenChangeEvent.idl > =================================================================== > +module events { > + > + interface [ > + Conditional=WORKERS&FULLSCREEN_API, Why WORKERS? > + NoStaticTables > + ] WebKitFullScreenChangeEvent : Event { > + > + void initWebKitFullScreenChangeEvent(in DOMString eventTypeArg, > + in boolean canBubbleArg, > + in boolean cancelableArg); > + }; > + > +} > Index: WebCore/html/HTMLElement.idl > =================================================================== > --- WebCore/html/HTMLElement.idl (revision 64653) > +++ WebCore/html/HTMLElement.idl (working copy) > @@ -64,6 +64,11 @@ module html { > #if defined(LANGUAGE_OBJECTIVE_C) && LANGUAGE_OBJECTIVE_C > readonly attribute DOMString titleDisplayString; > #endif > + > +#if defined(ENABLE_FULLSCREEN_API) && ENABLE_FULLSCREEN_API > + void webkitRequestFullScreen(); > + void webkitRequestFullScreenWithKeys(); > +#endif Why do you need these here? HTMLElement derives from Element.

Comment on attachment 63627 [details] FullScreen-WebCore-MediaElements I think we have to figure out how to get the generic fullscreen code in without breaking video element fullscreen, so this patch isn't really suitable.

Comment on attachment 63628 [details] FullScreen-WebCore > +#if ENABLE(FULLSCREEN_API) > + ASSERT(n); > + if (n->contains(m_fullScreenElement.get())) { > + ASSERT(n != documentElement()); > + m_fullScreenElement = documentElement(); > + m_fullScreenElement->dispatchEvent(WebKitFullScreenChangeEvent::create()); > + } > +#endif Who's actually telling the UA that it's leaving fullscreen in this case? Also, what happens when an <iframe>, <object>, or <embed> is fullscreen, then one of its parent nodes is removed? I think this deserves a testcase. > +void Document::webkitRequestFullScreenForElementWithKeys(Element* element, bool keysEnabled) > +{ > + if (!page() || !page()->settings()->fullScreenEnabled()) > + return; > + > + if (!element) > + element = documentElement(); > + > + if (!page()->chrome()->client()->supportsFullscreenForElement(element)) > + return; > + > + m_fullScreenElement = 0; > + m_areKeysEnabledInFullScreen = keysEnabled; > + page()->chrome()->client()->enterFullscreenForElement(element); > +} Should this method bail earlier if the doc is already fullscreen? > +void Document::webkitWillEnterFullScreenForElement(Element* element) > +{ > + ASSERT(page() && page()->settings()->fullScreenEnabled()); > + > + m_fullScreenElement = element; > + m_isFullScreen = true; > + documentElement()->setNeedsStyleRecalc(FullStyleChange); I'd like to see a comment saying that Document::isFullscreen() applies some pseudoclasses, so we need to do the style recalc. > + if (m_fullScreenElement) Hadn't we better always have a m_fullScreenElement here, so maybe ASSERT()? > Index: WebCore/html/HTMLIFrameElement.cpp > =================================================================== > --- WebCore/html/HTMLIFrameElement.cpp (revision 64653) > +++ WebCore/html/HTMLIFrameElement.cpp (working copy) > @@ -38,6 +38,9 @@ using namespace HTMLNames; > > inline HTMLIFrameElement::HTMLIFrameElement(const QualifiedName& tagName, Document* document) > : HTMLFrameElementBase(tagName, document) > +#if ENABLE(FULLSCREEN_API) > + , m_allowsFullScreen(false) > +#endif > { > ASSERT(hasTagName(iframeTag)); > } > @@ -132,6 +135,10 @@ void HTMLIFrameElement::parseMappedAttri > else if (attr->name() == sandboxAttr) > setSandboxFlags(parseSandboxAttribute(attr)); > #endif > +#if ENABLE(FULLSCREEN_API) > + if (attr->name() == webkitallowfullscreenAttr) > + m_allowsFullScreen = true; > +#endif > else > HTMLFrameElementBase::parseMappedAttribute(attr); I think we should postpone the frame/object/emebed-related stuff for a later patch. There are lots of complexities we need to figure out, and make tests for. I think this is generally OK, but I'd like to see more consideration of actions that should force us to leave fullscreen.

Attachment 63628 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3590938

(In reply to comment #50) > (From update of attachment 62785 [details]) > > Index: WebKit/mac/Configurations/FeatureDefines.xcconfig > > =================================================================== > > You should probably land the xconfig changes for all projects at the same time. Will Do. > > Index: WebKit/mac/WebCoreSupport/WebChromeClient.mm > > =================================================================== > > > -#if ENABLE(VIDEO) > > +#if ENABLE(FULLSCREEN) > > These (and other) changes regress apps that get video fullscreen now, but won't have generic fullscreen enabled unless they set the preference. > > I think we should keep video fullscreen the way it works currently, and only switch over to generic fullscreen when we're ready to do so with zero regressions. I'm going to update this patch to do precisely that. Unless a client of WebKit specifically requests the new full screen API, they'll get the old behavior. > > +#if ENABLE(FULLSCREEN) > > +@implementation WebKitFullScreenListener > > +- (id)initWithElement:(Element*)element > > +{ > > + if (!(self = [super init])) > > + return nil; > > + > > + _element = element; > > + return self; > > +} > > + > > +- (void)webkitWillEnterFullScreen > > +{ > > + if (_element) > > + _element->document()->webkitWillEnterFullScreenForElement(_element.get()); > > +} > > + > > +- (void)webkitDidEnterFullScreen > > +{ > > + if (_element) > > + _element->document()->webkitDidEnterFullScreenForElement(_element.get()); > > +} > > + > > +- (void)webkitWillExitFullScreen > > +{ > > + if (_element) > > + _element->document()->webkitWillExitFullScreenForElement(_element.get()); > > +} > > + > > +- (void)webkitDidExitFullScreen > > +{ > > + if (_element) > > + _element->document()->webkitDidExitFullScreenForElement(_element.get()); > > +} > > + > > +@end > > I'm not sure I see the benefit of having this object that just turns around and calls back into the document. Why not make the WebView the UI delegate? The geolocation feature already does something very similar with a listener object. Since it's seen by clients as a protocol, there's nothing keeping the WebView from passing itself instead of a separate listener object. But, since there's no specific need for a WebView to be involved; a lightweight object like the WebKitFullScreenListener is all that's needed. > Also fullscreen/fullScreen inconsistency here. Whoops! Definitely will fix this. > > +#if ENABLE(FULLSCREEN) > > +- (void)setFullScreenEnabled:(BOOL)flag > > +{ > > + [self _setBoolValue:flag forKey:WebKitFullScreenEnabledPreferenceKey]; > > +} > > + > > +- (BOOL)fullScreenEnabled > > +{ > > + return [self _boolValueForKey:WebKitFullScreenEnabledPreferenceKey]; > > +} > > +#endif > > Fullscreen/fullScreen inconsistency here (and in the pref name). OK. > > Index: WebKit/mac/WebView/WebUIDelegatePrivate.h > > =================================================================== > > > > +#if ENABLE(FULLSCREEN) > > +@protocol WebKitFullScreenListener<NSObject> > > +- (void)webkitWillEnterFullScreen; > > +- (void)webkitDidEnterFullScreen; > > +- (void)webkitWillExitFullScreen; > > +- (void)webkitDidExitFullScreen; > > +@end > > Fullscreen/fullScreen inconsistency. OK. > > +#if ENABLE(FULLSCREEN) > > +- (BOOL)webView:(WebView *)sender supportsFullscreenForElement:(DOMElement *)element; > > +- (void)webView:(WebView *)sender enterFullscreenForElement:(DOMElement *)element; > > +- (void)webView:(WebView *)sender exitFullscreenForElement:(DOMElement *)element; > > +#endif > > Fullscreen/fullScreen inconsistency. OK. > > @end > > Index: WebKit/mac/WebView/WebView.mm > > =================================================================== > > --- WebKit/mac/WebView/WebView.mm (revision 64185) > > +++ WebKit/mac/WebView/WebView.mm (working copy) > > @@ -1434,6 +1434,9 @@ - (void)_preferencesChangedNotification: > > settings->setHTML5ParserEnabled([preferences html5ParserEnabled]); > > settings->setHTML5TreeBuilderEnabled_DO_NOT_USE([preferences html5TreeBuilderEnabled]); > > settings->setPaginateDuringLayoutEnabled([preferences paginateDuringLayoutEnabled]); > > +#if ENABLE(FULLSCREEN) > > + settings->setFullScreenEnabled([preferences fullScreenEnabled]); > > +#endif > > settings->setMemoryInfoEnabled([preferences memoryInfoEnabled]); > > } > > Fullscreen/fullScreen inconsistency. OK. > r- for breaking the existing video fullscreen. New patch is coming.

(In reply to comment #52) > (From update of attachment 63477 [details]) > > Index: LayoutTests/ChangeLog ... > You can remove the method listing from the Changelog for new files. OK. > > Index: LayoutTests/fullscreen/full-screen-css-expected.txt > > =================================================================== > > --- LayoutTests/fullscreen/full-screen-css-expected.txt (revision 0) > > +++ LayoutTests/fullscreen/full-screen-css-expected.txt (revision 0) > > @@ -0,0 +1,13 @@ > > +EXPECTED (document.defaultView.getComputedStyle(span, null).getPropertyValue('background-color') != 'rgb(0, 255, 0)') OK > > +EXPECTED (document.defaultView.getComputedStyle(document.documentElement, null).getPropertyValue('background-color') != 'rgb(255, 0, 0)') OK > > +EXPECTED (document.defaultView.getComputedStyle(document.documentElement, null).getPropertyValue('color') != 'rgb(0, 0, 255)') OK > > +EVENT(webkitfullscreenchange) > > +EXPECTED (document.defaultView.getComputedStyle(span, null).getPropertyValue('background-color') == 'rgb(0, 255, 0)') OK > > +EXPECTED (document.defaultView.getComputedStyle(document.documentElement, null).getPropertyValue('background-color') == 'rgb(255, 0, 0)') OK > > +EXPECTED (document.defaultView.getComputedStyle(document.documentElement, null).getPropertyValue('color') == 'rgb(0, 0, 255)') OK > > +EVENT(webkitfullscreenchange) > > +EXPECTED (document.defaultView.getComputedStyle(span, null).getPropertyValue('background-color') == 'rgb(0, 255, 0)') OK > > +EXPECTED (document.defaultView.getComputedStyle(document.documentElement, null).getPropertyValue('background-color') != 'rgb(255, 0, 0)'), OBSERVED 'rgb(255, 0, 0)' FAIL > > You have a FAIL in the results here. That's definitely not right. It looks like it should be a pass; both are rgb(255, 0, 0). One is probably a string and the other an object or something. I'll figure out why that test isn't passing. > > Index: LayoutTests/fullscreen/full-screen-css.html > > =================================================================== > > > +<script src=full-screen-test.js></script> > > Please quote the src attribute. OK. > > Index: LayoutTests/fullscreen/full-screen-remove-ancestor.html > > =================================================================== > > --- LayoutTests/fullscreen/full-screen-remove-ancestor.html (revision 0) > > +++ LayoutTests/fullscreen/full-screen-remove-ancestor.html (revision 0) > > @@ -0,0 +1,30 @@ > > +<body> > > +<script src=full-screen-test.js></script> > > +<div><span></span></div> > > +<script> > > + var callback; > > + var fullscreenChanged = function(event) > > + { > > + if (callback) > > + callback(event) > > + }; > > + waitForEvent(document, 'webkitfullscreenchange', fullscreenChanged); > > + > > + var span = document.getElementsByTagName('span')[0]; > > + var div = span.parentNode; > > + > > + var spanIsFullScreen = function(event) { > > + callback = documentIsFullScreen; > > + testExpected("document.webkitCurrentFullScreenElement", span); > > + document.body.removeChild(div); > > + }; > > spanIsFullScreen is a confusing name for the variable; maybe > spanEnteredFullScreen. OK. > I don't see anything actually being tested.Shouldn't you test that you came out > of fullscreen? The line: "callback = documentIsFullScreen" changes which function is called when the 'webkitfullscreenchange' event is caught. In this case, if that function isn't called in response to document.body.removeChild(div), the test will fail. > If you run all the LayoutTests, you'll fine some others whose results dump all properties of the Document object. You'll have to supply new results for these tests, for platforms which enable the FULLSCREEN_API #ifdef. OK.

(In reply to comment #54) > (From update of attachment 63624 [details]) > > Index: JavaScriptCore/wtf/Platform.h > > =================================================================== > > --- JavaScriptCore/wtf/Platform.h (revision 64653) > > +++ JavaScriptCore/wtf/Platform.h (working copy) > > @@ -594,6 +594,7 @@ > > #endif > > #define HAVE_READLINE 1 > > #define HAVE_RUNLOOP_TIMER 1 > > +#define ENABLE_FULLSCREEN_API 1 > > I think this should go along with the things like: > > #if !defined(ENABLE_DRAG_SUPPORT) > #define ENABLE_DRAG_SUPPORT 1 > #endif > > and default to 0. It will get enabled via the xconfig files on platforms that want it. OK.

(In reply to comment #57) > (From update of attachment 63626 [details]) > > > Index: WebCore/dom/WebKitFullScreenChangeEvent.idl > > =================================================================== > > > +module events { > > + > > + interface [ > > + Conditional=WORKERS&FULLSCREEN_API, > > Why WORKERS? No good reason. Removed. > > Index: WebCore/html/HTMLElement.idl > > =================================================================== > > --- WebCore/html/HTMLElement.idl (revision 64653) > > +++ WebCore/html/HTMLElement.idl (working copy) > > @@ -64,6 +64,11 @@ module html { > > #if defined(LANGUAGE_OBJECTIVE_C) && LANGUAGE_OBJECTIVE_C > > readonly attribute DOMString titleDisplayString; > > #endif > > + > > +#if defined(ENABLE_FULLSCREEN_API) && ENABLE_FULLSCREEN_API > > + void webkitRequestFullScreen(); > > + void webkitRequestFullScreenWithKeys(); > > +#endif > > Why do you need these here? HTMLElement derives from Element. My first pass at the API put these functions here. It appears I never removed them, which I'll do now.

Created attachment 63936 [details] Part 1: FullScreen-ChangeLogs Changed the layout of the patches for this bug. I've separated them out so that each one will build independently (if applied incrementally).

Created attachment 63937 [details] Part 2: FullScreen-WebCore-NewFiles

Created attachment 63938 [details] Part 3: FullScreen-WebCore-NewAPI

Created attachment 63939 [details] Part 4: FullScreen-WebCore-CSS

Created attachment 63941 [details] Part 5: FullScreen-WebCore-MediaElements

Created attachment 63943 [details] Part 6: FullScreen-WebKit

Attachment 63938 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/EventNames.h:167: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 63949 [details] Part 7: FullScreen-LayoutTests These layout tests were previously review+, but they've been moved into LayoutTests/platform/mac/fullscreen. Also, other tests which count the number of properties on an HTML element have been modified to remove the specific count.

Created attachment 63955 [details] Part 3: FullScreen-WebCore-NewAPI

Attachment 63955 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/EventNames.h:167: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 63477 [details] FullScreen-LayoutTests Cleared Simon Fraser's review+ from obsolete attachment 63477 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Comment on attachment 63628 [details] FullScreen-WebCore Cleared Simon Fraser's review+ from obsolete attachment 63628 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Comment on attachment 63936 [details] Part 1: FullScreen-ChangeLogs It is a bit strange for me that there is a patch just with the ChangeLogs. Is this because you have to land all the other patches together? If not, please add one ChangeLog for each of your other patches so that they can be landed part per part.

(In reply to comment #77) > (From update of attachment 63936 [details]) > It is a bit strange for me that there is a patch just with the ChangeLogs. Is this because you have to land all the other patches together? Yes, I plan on landing all these patches at once.

Comment on attachment 63937 [details] Part 2: FullScreen-WebCore-NewFiles You shouldn't need to define an entirely new event type since it doesn't have fields not in Event. Just define the event name(s) and pass a name to Event::create().

Comment on attachment 63941 [details] Part 5: FullScreen-WebCore-MediaElements > + if (document()->page()->settings()->fullScreenEnabled()) { Document has a settings() method so you don't have to go through page. settings() can return NULL if there isn't a frame. r=me with these changes.

Comment on attachment 63955 [details] Part 3: FullScreen-WebCore-NewAPI > +#if ENABLE(FULLSCREEN_API) > +#include "WebKitFullScreenChangeEvent.h" > +#endif Not necessary. > +#if ENABLE(FULLSCREEN_API) > + ASSERT(n); > + if (n->contains(m_fullScreenElement.get())) { > + ASSERT(n != documentElement()); > + m_fullScreenElement = documentElement(); > + m_fullScreenElement->setNeedsStyleRecalc(); > + m_fullScreenElement->dispatchEvent(WebKitFullScreenChangeEvent::create()); > + } It took me a minute to figure out why this is necessary, so a comment would be nice. > +#if ENABLE(FULLSCREEN_API) > + else if (eventType == "WebKitFullScreenChange") > + event = WebKitFullScreenChangeEvent::create(); > +#endif Not necessary. > +#if ENABLE(FULLSCREEN_API) > + else if (eventType == eventNames().webkitfullscreenchangeEvent) > + addListenerType(FULLSCREENCHANGE_LISTENER); > +#endif Ditto. +void Document::webkitRequestFullScreenForElementWithKeys(Element* element, bool keysEnabled) Shouldn't this take the flags word so we don't have to create a new method once there are new flags? +void Document::webkitRequestFullScreenForElementWithKeys(Element* element, bool keysEnabled) This should be "webkitRequestFullScreenForElementWithFlags". > +void Document::webkitWillEnterFullScreenForElement(Element* element) > +{ > + ASSERT(page() && page()->settings()->fullScreenEnabled()); > + > + m_fullScreenElement = element; > + m_isFullScreen = true; > + documentElement()->setNeedsStyleRecalc(FullStyleChange); > + if (m_fullScreenElement) > + m_fullScreenElement->setNeedsStyleRecalc(FullStyleChange); > + updateStyleIfNeeded(); > + > + if (m_fullScreenElement) > + m_fullScreenElement->dispatchEvent(WebKitFullScreenChangeEvent::create()); > + else > + documentElement()->dispatchEvent(WebKitFullScreenChangeEvent::create()); > +} It shouldn't be legal to pass a NULL element, I would ASSERT and remove support for it. > +void Document::webkitDidExitFullScreenForElement(Element* element) > +{ > + m_isFullScreen = false; > + m_areKeysEnabledInFullScreen = false; > + // m_fullScreenElement has already been cleared; recalc the style of > + // the passed in element instead. > + > + if (element) > + element->setNeedsStyleRecalc(FullStyleChange); > + documentElement()->setNeedsStyleRecalc(FullStyleChange); > + updateStyleIfNeeded(); > + > + if (element) > + element->dispatchEvent(WebKitFullScreenChangeEvent::create()); > + else > + documentElement()->dispatchEvent(WebKitFullScreenChangeEvent::create()); > +} Ditto. > + FULLSCREENCHANGE_LISTENER = 0x8000, Not necessary once you get rid of the custom event type. > +void Element::webkitRequestFullScreen(unsigned short flags) > +{ > + document()->webkitRequestFullScreenForElement(this, flags == ALLOW_KEYBOARD_INPUT); > +} Should just pass the flags through to webkitRequestFullScreenForElement > +#if ENABLE(FULLSCREEN_API) > +bool Event::isWebKitFullScreenChangeEvent() const > +{ > + return false; > +} Not needed. > +#if ENABLE(FULLSCREEN_API) > + virtual bool isWebKitFullScreenChangeEvent() const; > +#endif Ditto. r= for now since a fair amount of cleanup is still needed.

Created attachment 64545 [details] Part 1-3: FullScreen-WebCore-NewAPI This patch makes obsolete and integrates patches Part 1, 2, & 3 above. WebKitFullScreenChangeEvent has been removed, and Eric's reviews of Parts 2 & 3 have been addressed: (In reply to comment #81) > (From update of attachment 63955 [details]) > > +#if ENABLE(FULLSCREEN_API) > > +#include "WebKitFullScreenChangeEvent.h" > > +#endif > > Not necessary. Removed. > > +#if ENABLE(FULLSCREEN_API) > > + ASSERT(n); > > + if (n->contains(m_fullScreenElement.get())) { > > + ASSERT(n != documentElement()); > > + m_fullScreenElement = documentElement(); > > + m_fullScreenElement->setNeedsStyleRecalc(); > > + m_fullScreenElement->dispatchEvent(WebKitFullScreenChangeEvent::create()); > > + } > > It took me a minute to figure out why this is necessary, so a comment would be nice. Added a comment here. > > +#if ENABLE(FULLSCREEN_API) > > + else if (eventType == "WebKitFullScreenChange") > > + event = WebKitFullScreenChangeEvent::create(); > > +#endif > > Not necessary. Removed. > > +#if ENABLE(FULLSCREEN_API) > > + else if (eventType == eventNames().webkitfullscreenchangeEvent) > > + addListenerType(FULLSCREENCHANGE_LISTENER); > > +#endif > > Ditto. Removed. > +void Document::webkitRequestFullScreenForElementWithKeys(Element* element, bool keysEnabled) > > Shouldn't this take the flags word so we don't have to create a new method once there are new flags? > > +void Document::webkitRequestFullScreenForElementWithKeys(Element* element, bool keysEnabled) > > This should be "webkitRequestFullScreenForElementWithFlags". Actually, in a previous review, it was decided drop the "WithFlags/WithKeys" part entirely. Regardless, the Document::webkitRequestFullScreenForElement() function now takes a flags parameter. > > +void Document::webkitWillEnterFullScreenForElement(Element* element) > > +{ > > + ASSERT(page() && page()->settings()->fullScreenEnabled()); > > + > > + m_fullScreenElement = element; > > + m_isFullScreen = true; > > + documentElement()->setNeedsStyleRecalc(FullStyleChange); > > + if (m_fullScreenElement) > > + m_fullScreenElement->setNeedsStyleRecalc(FullStyleChange); > > + updateStyleIfNeeded(); > > + > > + if (m_fullScreenElement) > > + m_fullScreenElement->dispatchEvent(WebKitFullScreenChangeEvent::create()); > > + else > > + documentElement()->dispatchEvent(WebKitFullScreenChangeEvent::create()); > > +} > > It shouldn't be legal to pass a NULL element, I would ASSERT and remove support for it. Done. > > +void Document::webkitDidExitFullScreenForElement(Element* element) > > +{ > > + m_isFullScreen = false; > > + m_areKeysEnabledInFullScreen = false; > > + // m_fullScreenElement has already been cleared; recalc the style of > > + // the passed in element instead. > > + > > + if (element) > > + element->setNeedsStyleRecalc(FullStyleChange); > > + documentElement()->setNeedsStyleRecalc(FullStyleChange); > > + updateStyleIfNeeded(); > > + > > + if (element) > > + element->dispatchEvent(WebKitFullScreenChangeEvent::create()); > > + else > > + documentElement()->dispatchEvent(WebKitFullScreenChangeEvent::create()); > > +} > > Ditto. Done. > > + FULLSCREENCHANGE_LISTENER = 0x8000, > > Not necessary once you get rid of the custom event type. Removed. > > +void Element::webkitRequestFullScreen(unsigned short flags) > > +{ > > + document()->webkitRequestFullScreenForElement(this, flags == ALLOW_KEYBOARD_INPUT); > > +} > > Should just pass the flags through to webkitRequestFullScreenForElement Done. > > +#if ENABLE(FULLSCREEN_API) > > +bool Event::isWebKitFullScreenChangeEvent() const > > +{ > > + return false; > > +} > > Not needed. Removed. > > +#if ENABLE(FULLSCREEN_API) > > + virtual bool isWebKitFullScreenChangeEvent() const; > > +#endif > > Ditto. Removed.

Attachment 64545 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/EventNames.h:167: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 63939 [details] Part 4: FullScreen-WebCore-CSS > @@ -553,6 +553,14 @@ static void loadFullDefaultStyle() > String quirksRules = String(quirksUserAgentStyleSheet, sizeof(quirksUserAgentStyleSheet)) + RenderTheme::defaultTheme()->extraQuirksStyleSheet(); > CSSStyleSheet* quirksSheet = parseUASheet(quirksRules); > defaultQuirksStyle->addRulesFromSheet(quirksSheet, screenEval()); > + > +#if ENABLE(FULLSCREEN_API) > + // Full-screen rules. > + String fullscreenRules = String(fullscreenUserAgentStyleSheet, sizeof(fullscreenUserAgentStyleSheet)) + RenderTheme::defaultTheme()->extraDefaultStyleSheet(); > + CSSStyleSheet* fullscreenSheet = parseUASheet(fullscreenRules); > + defaultStyle->addRulesFromSheet(fullscreenSheet, screenEval()); > + defaultQuirksStyle->addRulesFromSheet(fullscreenSheet, screenEval()); > +#endif All of the other style sheets added for optionally compiled features are added in CSSStyleSelector::styleForElement. Is there a reason to load this in loadFullDefaultStyle instead? > + case CSSSelector::PseudoFullScreen: > + // While a Document is in the fullscreen state, and the document's current fullscreen > + // element is an element in the document, the 'full-screen' pseudoclass applies to > + // that element. Also, an <iframe>, <object> or <embed> element whose child browsing > + // context's Document is in the fullscreen state has the 'full-screen' pseudoclass applied. > + if (!e->document()->webkitFullScreen()) > + return false; > + if (e != e->document()->webkitCurrentFullScreenElement()) > + return false; Is the "... an <iframe>, <object> or <embed> element whose child browsing context's ..." comment specifically relevant here? r=me if you address these comments.

Comment on attachment 63949 [details] Part 7: FullScreen-LayoutTests You should put these tests in LayoutTests/media/fullscreen and add "fullscreen/" to all of the other port's skiplist files instead of putting them in platform/mac now and moving them later. r-

Comment on attachment 64545 [details] Part 1-3: FullScreen-WebCore-NewAPI > +++ WebCore/ChangeLog (working copy) > > + * html/HTMLIFrameElement.h: Looks like this is left over from a previous version. > + * rendering/MediaControlElements.cpp: > + (WebCore::MediaControlFullscreenButtonElement::defaultEventHandler): The full screen button now toggles full screen mode (previously, it only entered). > + * rendering/style/RenderStyleConstants.h: Added new style constants. Ditto. > + > +#if ENABLE(FULLSCREEN_API) > +void Document::webkitRequestFullScreenForElement(Element* element, unsigned short flags) > +{ > + if (!page() || !page()->settings()->fullScreenEnabled()) > + return; > + > + if (!element) > + element = documentElement(); > + > + if (!page()->chrome()->client()->supportsFullScreenForElement(element)) > + return; > + > + m_fullScreenElement = 0; > + m_areKeysEnabledInFullScreen = flags & Element::ALLOW_KEYBOARD_INPUT; > + page()->chrome()->client()->enterFullScreenForElement(element); If enterFullScreenForElement fails and m_fullScreenElement is non-NULL, we won't ever fire a webkitfullscreenchange event. Is this worth worrying about? > +void Document::webkitDidExitFullScreenForElement(Element* element) > +{ > + ASSERT(element); > + m_isFullScreen = false; > + m_areKeysEnabledInFullScreen = false; > + // m_fullScreenElement has already been cleared; recalc the style of > + // the passed in element instead. > + > + element->setNeedsStyleRecalc(FullStyleChange); The blank line should go above the comment instead of below it. > Index: WebCore/dom/Element.cpp > =================================================================== > --- WebCore/dom/Element.cpp (revision 65463) > +++ WebCore/dom/Element.cpp (working copy) > @@ -1441,7 +1441,7 @@ void Element::normalizeAttributes() > attr->normalize(); > } > } > - > + Don't need the new white space. r=m with these minor changes.

(In reply to comment #86) > (From update of attachment 64545 [details]) > > +++ WebCore/ChangeLog (working copy) > > > > + * html/HTMLIFrameElement.h: > > Looks like this is left over from a previous version. Whoops, will delete. > > + * rendering/MediaControlElements.cpp: > > + (WebCore::MediaControlFullscreenButtonElement::defaultEventHandler): The full screen button now toggles full screen mode (previously, it only entered). > > + * rendering/style/RenderStyleConstants.h: Added new style constants. > Ditto. This is technically a comment for the Part5: FullScreen-WebCore-MediaElements patch. They will all be committed at the same time, so I didn't break out this minor part of the ChangeLog into it's own patch. > > + > > +#if ENABLE(FULLSCREEN_API) > > +void Document::webkitRequestFullScreenForElement(Element* element, unsigned short flags) > > +{ > > + if (!page() || !page()->settings()->fullScreenEnabled()) > > + return; > > + > > + if (!element) > > + element = documentElement(); > > + > > + if (!page()->chrome()->client()->supportsFullScreenForElement(element)) > > + return; > > + > > + m_fullScreenElement = 0; > > + m_areKeysEnabledInFullScreen = flags & Element::ALLOW_KEYBOARD_INPUT; > > + page()->chrome()->client()->enterFullScreenForElement(element); > > If enterFullScreenForElement fails and m_fullScreenElement is non-NULL, we won't ever fire a webkitfullscreenchange event. Is this worth worrying about? The Mozilla spec says: "When a Document enters or leaves the fullscreen state, the user agent must queue a task to dispatch [the fullscreenchange event]." So, because the fullscreen state isn't changing, the event shouldn't be dispatched. Granted, this case occurs only when another element was previously full screen, which is not specifically addressed in the documentation. > > +void Document::webkitDidExitFullScreenForElement(Element* element) > > +{ > > + ASSERT(element); > > + m_isFullScreen = false; > > + m_areKeysEnabledInFullScreen = false; > > + // m_fullScreenElement has already been cleared; recalc the style of > > + // the passed in element instead. > > + > > + element->setNeedsStyleRecalc(FullStyleChange); > > The blank line should go above the comment instead of below it. Will change. > > Index: WebCore/dom/Element.cpp > > =================================================================== > > --- WebCore/dom/Element.cpp (revision 65463) > > +++ WebCore/dom/Element.cpp (working copy) > > @@ -1441,7 +1441,7 @@ void Element::normalizeAttributes() > > attr->normalize(); > > } > > } > > - > > + > Don't need the new white space. Will remove. > r=m with these minor changes. Thanks!

(In reply to comment #87) > (In reply to comment #86) > > > + m_fullScreenElement = 0; > > > + m_areKeysEnabledInFullScreen = flags & Element::ALLOW_KEYBOARD_INPUT; > > > + page()->chrome()->client()->enterFullScreenForElement(element); > > > > If enterFullScreenForElement fails and m_fullScreenElement is non-NULL, we won't ever fire a webkitfullscreenchange event. Is this worth worrying about? > > The Mozilla spec says: "When a Document enters or leaves the fullscreen state, the user agent must queue a task to dispatch [the fullscreenchange event]." So, because the fullscreen state isn't changing, the event shouldn't be dispatched. Granted, this case occurs only when another element was previously full screen, which is not specifically addressed in the documentation. On second thought, setting m_fullScreenElement to 0 here seems unnecessary. If the UA decides to ignore the new full screen request, we should leave the existing m_fullScreenElement alone. I'll remove that "m_fullScreenElement = 0;" line.

Created attachment 64997 [details] Part 7: FullScreen-LayoutTests

Comment on attachment 63943 [details] Part 6: FullScreen-WebKit > // These are private both because callers should be using the cover methods and because the > Index: WebKit/mac/WebView/WebPreferences.h > =================================================================== > --- WebKit/mac/WebView/WebPreferences.h (revision 65005) > +++ WebKit/mac/WebView/WebPreferences.h (working copy) > @@ -437,6 +437,28 @@ caching behavior. > */ > - (WebCacheModel)cacheModel; > > +#if ENABLE(FULLSCREEN_API) > +/*! > + @method setFullScreenEnabled: > + > + @abstract > + > + @param > + > + @discussion > + */ > +- (void)setFullScreenEnabled:(BOOL)flag; > + > +/*! > + @method fullScreenEnabled: > + > + @abstract > + > + @result > + */ > +- (BOOL)fullScreenEnabled; > +#endif > + > @end > You should fill in @abstract and @result for both of these. r=me with this change.

Comment on attachment 63943 [details] Part 6: FullScreen-WebKit > Index: WebKit/mac/WebView/WebPreferences.h > =================================================================== > --- WebKit/mac/WebView/WebPreferences.h (revision 65005) > +++ WebKit/mac/WebView/WebPreferences.h (working copy) > @@ -437,6 +437,28 @@ caching behavior. > */ > - (WebCacheModel)cacheModel; > > +#if ENABLE(FULLSCREEN_API) > +/*! > + @method setFullScreenEnabled: > + > + @abstract > + > + @param > + > + @discussion > + */ > +- (void)setFullScreenEnabled:(BOOL)flag; > + > +/*! > + @method fullScreenEnabled: > + > + @abstract > + > + @result > + */ > +- (BOOL)fullScreenEnabled; > +#endif > + > Dan reminds me that #if ENABLE() can't be in a public header.

Created attachment 65593 [details] Part 6: FullScreen-WebKit Moved the new API for setting fullscreen preferences into WebPreferencesPrivate.h, and removed the #if ENABLE(FULLSCREEN_API) guards.

Comment on attachment 64997 [details] Part 7: FullScreen-LayoutTests > +2010-08-19 Jer Noble <jer.noble@apple.com> > + > + Reviewed by NOBODY (OOPS!). > + ... > + * fullscreen/full-screen-twice.html: Added. > + > +2010-08-09 Jer Noble <jer.noble@apple.com> > + These two entries should be combined. > + Add JavaScript API to allow a page to go fullscreen. > + rdar://problem/6867795 > + https://bugs.webkit.org/show_bug.cgi?id=43099 > + > + New tests for the new full screen API: > + > + full-screen-api.html: tests for the presence of the new full screen apis. > + full-screen-request.html: tests that clients can request the browser enter full screen mode successfully. > + full-screen-css.html: tests that the new full screen pseudo classes are applied once full screen mode is entered. > + full-screen-remove.html: tests that the document's current full screen element is changed when that element is removed from the DOM tree. > + full-screen-remove-ancestor.html: tests that the document's current full screen element is changed an ancestor of that element is removed from the DOM tree. > + full-screen-twice.html: tests that entering full screen mode on two different elements in a row does not fail. Please wrap the long lines so they are readable without horizontal scrolling in a reasonable size window. > =================================================================== > --- LayoutTests/fullscreen/full-screen-api.html (revision 0) > +++ LayoutTests/fullscreen/full-screen-api.html (revision 0) > @@ -0,0 +1,13 @@ > +<body> > + <script src="full-screen-test.js"></script> > +<span></span> > +<script> > + span = document.getElementsByTagName('span')[0]; > + testExpected("document.webkitFullScreen", false); > + testExpected("document.webkitCancelFullScreen", undefined, "!="); > + testExpected("document.webkitCurrentFullScreenElement", null); > + testExpected("document.onwebkitfullscreenchange", null) > + testExpected("span.webkitRequestFullScreen", undefined, "!="); > + testExpected("span.onwebkitfullscreenchange", null) > + endTest(); > +</script> Ack, tabs! > Index: LayoutTests/fullscreen/full-screen-css.html > +<script> > + // Bail out early if the full screen API is not enabled or is missing: > + if (Element.prototype.webkitRequestFullScreen == undefined) > + endTest(); Shouldn't this be an explicit fail? > Index: LayoutTests/fullscreen/full-screen-remove-ancestor.html > +<script> > + // Bail out early if the full screen API is not enabled or is missing: > + if (Element.prototype.webkitRequestFullScreen == undefined) > + endTest(); Here too. > Index: LayoutTests/fullscreen/full-screen-remove.html > +<script> > + // Bail out early if the full screen API is not enabled or is missing: > + if (Element.prototype.webkitRequestFullScreen == undefined) > + endTest(); And here. > Index: LayoutTests/fullscreen/full-screen-request.html > +<script> > + // Bail out early if the full screen API is not enabled or is missing: > + if (Element.prototype.webkitRequestFullScreen == undefined) > + endTest(); And here. > + case '<': success = observed < expected; break; > + case '<=': success = observed <= expected; break; > + case '>': success = observed > expected; break; > + case '>=': success = observed >= expected; break; > + case '!=': success = observed != expected; break; > + case '==': success = observed == expected; break; Strange spacing here... > +function waitForEventAndEnd(eventName, funcString) > +function waitForEventAndFail(element, eventName) > +function waitForEventAndTest(element, eventName, testFuncString, endit) These aren't used, you should remove them. > Index: LayoutTests/fullscreen/full-screen-twice.html > +<script> > + // Bail out early if the full screen API is not enabled or is missing: > + if (Element.prototype.webkitRequestFullScreen == undefined) > + endTest(); Fail here. > Index: LayoutTests/platform/android-v8/Skipped > Index: LayoutTests/platform/android/Skipped > Index: LayoutTests/platform/chromium-linux/Skipped > Index: LayoutTests/platform/chromium-linux/Skipped > Index: LayoutTests/platform/chromium-mac/Skipped > Index: LayoutTests/platform/chromium-win-vista/Skipped > Index: LayoutTests/platform/chromium-win-xp/Skipped > Index: LayoutTests/platform/chromium-win/Skipped > Index: LayoutTests/platform/chromium/Skipped > Index: LayoutTests/platform/google-chrome-linux64/Skipped > Index: LayoutTests/platform/gtk/Skipped > Index: LayoutTests/platform/qt-linux/Skipped > Index: LayoutTests/platform/qt-mac/Skipped > Index: LayoutTests/platform/qt-win/Skipped > Index: LayoutTests/platform/qt/Skipped > Index: LayoutTests/platform/win-wk2/Skipped > Index: LayoutTests/platform/win/Skipped You should only need to add the test the top level Skipped file for each port (eg. LayoutTests/platform/chromium/Skipped skips all chromium). You should add a comment to each Skipped file about why the tests are disabled. r=me with these changes.

Comment on attachment 65593 [details] Part 6: FullScreen-WebKit > Index: WebKit/WebKit.xcodeproj/project.pbxproj > =================================================================== > --- WebKit/WebKit.xcodeproj/project.pbxproj (revision 65463) > +++ WebKit/WebKit.xcodeproj/project.pbxproj (working copy) > @@ -1623,7 +1623,6 @@ > isa = PBXProject; > buildConfigurationList = 149C283208902B0F008A9EFC /* Build configuration list for PBXProject "WebKit" */; > compatibilityVersion = "Xcode 2.4"; > - developmentRegion = English; > hasScannedForEncodings = 1; > knownRegions = ( > English, Don't need this change. > Index: WebKit/mac/WebCoreSupport/WebSystemInterface.mm > =================================================================== > --- WebKit/mac/WebCoreSupport/WebSystemInterface.mm (revision 65463) > +++ WebKit/mac/WebCoreSupport/WebSystemInterface.mm (working copy) > @@ -84,6 +84,7 @@ void InitWebCoreSystemInterface(void) > INIT(SignalCFReadStreamHasBytes); > INIT(QTIncludeOnlyModernMediaFileTypes); > INIT(QTMovieDataRate); > + INIT(QTMovieDisableComponent); > INIT(QTMovieMaxTimeLoaded); > INIT(QTMovieMaxTimeLoadedChangeNotification); > INIT(QTMovieMaxTimeSeekable); This is unrelated to this patch. r=me with the unrelated changes removed.

Committed r66251: <http://trac.webkit.org/changeset/66251>

Committed r66267: <http://trac.webkit.org/changeset/66267>

http://trac.webkit.org/changeset/66267 might have broken Qt Linux Release The following changes are on the blame list: http://trac.webkit.org/changeset/66267 http://trac.webkit.org/changeset/66268 http://trac.webkit.org/changeset/66271

(In reply to comment #93) > (From update of attachment 64997 [details]) > > Index: LayoutTests/platform/android-v8/Skipped > > Index: LayoutTests/platform/android/Skipped > > Index: LayoutTests/platform/chromium-linux/Skipped > > Index: LayoutTests/platform/chromium-linux/Skipped > > Index: LayoutTests/platform/chromium-mac/Skipped > > Index: LayoutTests/platform/chromium-win-vista/Skipped > > Index: LayoutTests/platform/chromium-win-xp/Skipped > > Index: LayoutTests/platform/chromium-win/Skipped > > Index: LayoutTests/platform/chromium/Skipped > > Index: LayoutTests/platform/google-chrome-linux64/Skipped > > Index: LayoutTests/platform/gtk/Skipped > > Index: LayoutTests/platform/qt-linux/Skipped > > Index: LayoutTests/platform/qt-mac/Skipped > > Index: LayoutTests/platform/qt-win/Skipped > > Index: LayoutTests/platform/qt/Skipped > > Index: LayoutTests/platform/win-wk2/Skipped > > Index: LayoutTests/platform/win/Skipped > > You should only need to add the test the top level Skipped file for each port (eg. > LayoutTests/platform/chromium/Skipped skips all chromium). You should add a comment > to each Skipped file about why the tests are disabled. Chromium doesn't use skip lists. I'm not sure if Android does or not. In the future, if you're adding a Skipped file, there's a good chance you're doing something wrong or the skipped list isn't upstreamed. In this case, chromium uses a test-expectations file (because we run tests even when we expect them to fail). The top of the file describes how it works. If anything, please don't create skip list files since they'll only serve to confuse people.

Android does not used a Skipped list. I removed it in http://trac.webkit.org/changeset/66793

Comment on attachment 63939 [details] Part 4: FullScreen-WebCore-CSS View in context: https://bugs.webkit.org/attachment.cgi?id=63939&action=review > WebCore/rendering/style/RenderStyleConstants.h:82 > + FULL_SCREEN, FULL_SCREEN_DOCUMENT, This line should be put *before* AFTER_LAST_INTERNAL_PSEUDOID.

(In reply to comment #100) > (From update of attachment 63939 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=63939&action=review > > > WebCore/rendering/style/RenderStyleConstants.h:82 > > + FULL_SCREEN, FULL_SCREEN_DOCUMENT, > > This line should be put *before* AFTER_LAST_INTERNAL_PSEUDOID.