Bug 43099 - Add JavaScript API to allow a page to go fullscreen
Summary: Add JavaScript API to allow a page to go fullscreen
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jer Noble
URL: https://wiki.mozilla.org/index.php?ti...
Keywords: InRadar
Depends on:
Blocks: 49481
  Show dependency treegraph
 
Reported: 2010-07-27 18:51 PDT by Jer Noble
Modified: 2013-08-13 10:44 PDT (History)
19 users (show)

See Also:


Attachments
FullScreen-WebCore (62.13 KB, patch)
2010-07-27 19:12 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebKit (12.46 KB, patch)
2010-07-27 19:12 PDT, Jer Noble
simon.fraser: review-
Details | Formatted Diff | Diff
FullScreen-WebKitTools (3.02 KB, patch)
2010-07-27 19:13 PDT, Jer Noble
simon.fraser: review+
Details | Formatted Diff | Diff
FullScreen-LayoutTests (12.31 KB, patch)
2010-07-27 19:14 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebCore (61.98 KB, patch)
2010-07-27 20:36 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebCore (55.32 KB, patch)
2010-07-28 00:09 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebCore (55.34 KB, patch)
2010-07-28 00:37 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebCore (55.82 KB, patch)
2010-07-28 09:22 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebCore (56.07 KB, patch)
2010-07-28 09:41 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebCore (56.29 KB, patch)
2010-07-28 14:04 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebCore (56.93 KB, patch)
2010-07-29 09:05 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebCore (56.93 KB, patch)
2010-07-29 11:02 PDT, Jer Noble
simon.fraser: review-
Details | Formatted Diff | Diff
FullScree-LayoutTests (17.80 KB, patch)
2010-08-03 11:35 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebCore (60.82 KB, patch)
2010-08-03 11:43 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebCore (60.88 KB, patch)
2010-08-03 17:48 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebCore (60.70 KB, patch)
2010-08-04 12:06 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-LayoutTests (19.65 KB, patch)
2010-08-04 12:07 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
FullScreen-WebCore-ChangeLogs (8.36 KB, patch)
2010-08-05 13:38 PDT, Jer Noble
simon.fraser: review-
Details | Formatted Diff | Diff
FullScreen-WebCore-Config (13.14 KB, patch)
2010-08-05 13:38 PDT, Jer Noble
simon.fraser: review-
Details | Formatted Diff | Diff
FullScreen-WebCore-IDL (4.92 KB, patch)
2010-08-05 13:39 PDT, Jer Noble
simon.fraser: review-
Details | Formatted Diff | Diff
FullScreen-WebCore-MediaElements (4.45 KB, patch)
2010-08-05 13:39 PDT, Jer Noble
simon.fraser: review-
Details | Formatted Diff | Diff
FullScreen-WebCore (26.66 KB, patch)
2010-08-05 13:40 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Part 1: FullScreen-ChangeLogs (6.97 KB, patch)
2010-08-09 15:27 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Part 2: FullScreen-WebCore-NewFiles (19.54 KB, patch)
2010-08-09 15:28 PDT, Jer Noble
eric.carlson: review-
Details | Formatted Diff | Diff
Part 3: FullScreen-WebCore-NewAPI (17.89 KB, patch)
2010-08-09 15:29 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Part 4: FullScreen-WebCore-CSS (6.26 KB, patch)
2010-08-09 15:29 PDT, Jer Noble
eric.carlson: review+
eric.carlson: commit-queue-
Details | Formatted Diff | Diff
Part 5: FullScreen-WebCore-MediaElements (1.51 KB, patch)
2010-08-09 15:31 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff
Part 6: FullScreen-WebKit (12.41 KB, patch)
2010-08-09 15:31 PDT, Jer Noble
eric.carlson: review-
Details | Formatted Diff | Diff
Part 7: FullScreen-LayoutTests (23.41 KB, patch)
2010-08-09 16:27 PDT, Jer Noble
eric.carlson: review-
Details | Formatted Diff | Diff
Part 3: FullScreen-WebCore-NewAPI (17.82 KB, patch)
2010-08-09 17:04 PDT, Jer Noble
eric.carlson: review-
Details | Formatted Diff | Diff
Part 1-3: FullScreen-WebCore-NewAPI (27.88 KB, patch)
2010-08-16 18:25 PDT, Jer Noble
eric.carlson: review+
eric.carlson: commit-queue-
Details | Formatted Diff | Diff
Part 7: FullScreen-LayoutTests (29.65 KB, patch)
2010-08-20 14:58 PDT, Jer Noble
eric.carlson: review+
eric.carlson: commit-queue-
Details | Formatted Diff | Diff
Part 6: FullScreen-WebKit (13.40 KB, patch)
2010-08-26 11:53 PDT, Jer Noble
eric.carlson: review+
eric.carlson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2010-07-27 18:51:58 PDT
Mozilla has an API proposal for allowing arbitrary HTML elements to enter and exit full screen mode.
Comment 1 Jer Noble 2010-07-27 19:12:22 PDT
Created attachment 62784 [details]
FullScreen-WebCore
Comment 2 Jer Noble 2010-07-27 19:12:52 PDT
Created attachment 62785 [details]
FullScreen-WebKit
Comment 3 Jer Noble 2010-07-27 19:13:33 PDT
Created attachment 62786 [details]
FullScreen-WebKitTools
Comment 4 Jer Noble 2010-07-27 19:14:01 PDT
Created attachment 62787 [details]
FullScreen-LayoutTests
Comment 5 WebKit Review Bot 2010-07-27 19:18:38 PDT
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.
Comment 6 Eric Seidel (no email) 2010-07-27 19:22:06 PDT
Attachment 62785 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3619254
Comment 7 Early Warning System Bot 2010-07-27 19:29:34 PDT
Attachment 62784 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3585620
Comment 8 WebKit Review Bot 2010-07-27 20:21:49 PDT
Attachment 62784 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3638043
Comment 9 Jer Noble 2010-07-27 20:36:21 PDT
Created attachment 62793 [details]
FullScreen-WebCore

Fixed a few style errors (except for the one in EventNames.h).
Comment 10 WebKit Review Bot 2010-07-27 20:42:53 PDT
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.
Comment 11 Early Warning System Bot 2010-07-27 20:49:34 PDT
Attachment 62793 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3636054
Comment 12 Jer Noble 2010-07-27 21:12:21 PDT
Comment on attachment 62793 [details]
FullScreen-WebCore

Cancelling review+ while I work on fixing the qt and gtk compiles.
Comment 13 Jer Noble 2010-07-28 00:09:20 PDT
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.
Comment 14 Jer Noble 2010-07-28 00:37:28 PDT
Created attachment 62799 [details]
FullScreen-WebCore

Recreated patch after resolving conflicts in the FeatureDefines.xcconfig file.
Comment 15 WebKit Review Bot 2010-07-28 00:39:39 PDT
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.
Comment 16 Jer Noble 2010-07-28 00:41:39 PDT
I'll fix the tabs next time I upload the FullScreen-WebCore patch.
Comment 17 WebKit Review Bot 2010-07-28 02:10:49 PDT
Attachment 62799 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3588550
Comment 18 WebKit Review Bot 2010-07-28 04:12:28 PDT
Attachment 62799 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3643004
Comment 19 Jer Noble 2010-07-28 09:22:00 PDT
Created attachment 62833 [details]
FullScreen-WebCore

Fixed the mismatched braces in HTMLElement.cpp when ENABLE_FULLSCREEN is turned off.
Comment 20 Jer Noble 2010-07-28 09:41:29 PDT
Created attachment 62835 [details]
FullScreen-WebCore

Fixed the merge conflicts resulting from <r64210.
Comment 21 WebKit Review Bot 2010-07-28 09:42:46 PDT
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.
Comment 22 WebKit Review Bot 2010-07-28 11:31:50 PDT
Attachment 62835 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3642031
Comment 23 WebKit Review Bot 2010-07-28 13:36:12 PDT
Attachment 62835 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3588563
Comment 24 Jer Noble 2010-07-28 14:04:25 PDT
Created attachment 62874 [details]
FullScreen-WebCore

Unprotected ChromeClient::enter/exit/supportsFullScreenForElement().
Comment 25 WebKit Review Bot 2010-07-28 14:07:16 PDT
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.
Comment 26 Jer Noble 2010-07-29 09:05:57 PDT
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 27 Jer Noble 2010-07-29 09:15:41 PDT
Comment on attachment 62954 [details]
FullScreen-WebCore

Clearing review? due to merge conflicts.
Comment 28 Jer Noble 2010-07-29 11:02:05 PDT
Created attachment 62968 [details]
FullScreen-WebCore

Resolved merge conflicts.
Comment 29 WebKit Review Bot 2010-07-29 11:05:16 PDT
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 30 Simon Fraser (smfr) 2010-07-29 12:32:48 PDT
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.
Comment 31 Jer Noble 2010-07-29 14:10:23 PDT
(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.
Comment 32 Jer Noble 2010-08-03 11:35:21 PDT
Created attachment 63360 [details]
FullScree-LayoutTests
Comment 33 Jer Noble 2010-08-03 11:43:27 PDT
Created attachment 63361 [details]
FullScreen-WebCore
Comment 34 WebKit Review Bot 2010-08-03 11:47:42 PDT
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.
Comment 35 Jer Noble 2010-08-03 17:48:31 PDT
Created attachment 63397 [details]
FullScreen-WebCore

Fixed some merge conflicts that were confusing the EWS bots.
Comment 36 WebKit Review Bot 2010-08-03 17:52:12 PDT
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.
Comment 37 Jer Noble 2010-08-04 12:06:58 PDT
Created attachment 63476 [details]
FullScreen-WebCore

Updated patch after speaking with Robert O'Callahan about the Mozilla proposal.
Comment 38 Jer Noble 2010-08-04 12:07:41 PDT
Created attachment 63477 [details]
FullScreen-LayoutTests

Updated LayoutTests to match changes to the Mozilla proposal.
Comment 39 WebKit Review Bot 2010-08-04 12:09:21 PDT
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 40 Eric Carlson 2010-08-05 12:18:05 PDT
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?
Comment 41 Jer Noble 2010-08-05 12:49:01 PDT
(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.
Comment 42 Jer Noble 2010-08-05 13:38:17 PDT
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.
Comment 43 Jer Noble 2010-08-05 13:38:49 PDT
Created attachment 63624 [details]
FullScreen-WebCore-Config
Comment 44 Jer Noble 2010-08-05 13:39:26 PDT
Created attachment 63626 [details]
FullScreen-WebCore-IDL
Comment 45 Jer Noble 2010-08-05 13:39:59 PDT
Created attachment 63627 [details]
FullScreen-WebCore-MediaElements
Comment 46 Jer Noble 2010-08-05 13:40:33 PDT
Created attachment 63628 [details]
FullScreen-WebCore
Comment 47 WebKit Review Bot 2010-08-05 13:45:47 PDT
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 48 Simon Fraser (smfr) 2010-08-05 13:57:27 PDT
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".
Comment 49 Jer Noble 2010-08-05 14:04:20 PDT
(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 50 Simon Fraser (smfr) 2010-08-05 14:12:46 PDT
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.
Comment 51 Early Warning System Bot 2010-08-05 14:19:56 PDT
Attachment 63627 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3559953
Comment 52 Simon Fraser (smfr) 2010-08-05 14:22:08 PDT
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.
Comment 53 Eric Seidel (no email) 2010-08-05 14:27:26 PDT
Attachment 63627 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3583897
Comment 54 Simon Fraser (smfr) 2010-08-05 14:27:53 PDT
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 55 Simon Fraser (smfr) 2010-08-05 14:28:32 PDT
Comment on attachment 63623 [details]
FullScreen-WebCore-ChangeLogs

It doesn't really make sense to break out the changelogs into their own patch.
Comment 56 WebKit Review Bot 2010-08-05 14:30:28 PDT
Attachment 63627 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3643341
Comment 57 Simon Fraser (smfr) 2010-08-05 14:31:00 PDT
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 58 Simon Fraser (smfr) 2010-08-05 14:33:01 PDT
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 59 Simon Fraser (smfr) 2010-08-05 14:47:02 PDT
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.
Comment 60 WebKit Review Bot 2010-08-05 15:02:58 PDT
Attachment 63628 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3590938
Comment 61 Jer Noble 2010-08-05 15:45:55 PDT
(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.
Comment 62 Jer Noble 2010-08-05 15:50:34 PDT
(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.
Comment 63 Jer Noble 2010-08-05 15:54:40 PDT
(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.
Comment 64 Jer Noble 2010-08-05 15:58:43 PDT
(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.
Comment 65 Jer Noble 2010-08-09 15:27:57 PDT
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).
Comment 66 Jer Noble 2010-08-09 15:28:39 PDT
Created attachment 63937 [details]
Part 2: FullScreen-WebCore-NewFiles
Comment 67 Jer Noble 2010-08-09 15:29:11 PDT
Created attachment 63938 [details]
Part 3: FullScreen-WebCore-NewAPI
Comment 68 Jer Noble 2010-08-09 15:29:43 PDT
Created attachment 63939 [details]
Part 4: FullScreen-WebCore-CSS
Comment 69 Jer Noble 2010-08-09 15:31:12 PDT
Created attachment 63941 [details]
Part 5: FullScreen-WebCore-MediaElements
Comment 70 Jer Noble 2010-08-09 15:31:58 PDT
Created attachment 63943 [details]
Part 6: FullScreen-WebKit
Comment 71 WebKit Review Bot 2010-08-09 15:36:28 PDT
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.
Comment 72 Jer Noble 2010-08-09 16:27:18 PDT
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.
Comment 73 Jer Noble 2010-08-09 17:04:44 PDT
Created attachment 63955 [details]
Part 3: FullScreen-WebCore-NewAPI
Comment 74 WebKit Review Bot 2010-08-09 17:06:26 PDT
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 75 Eric Seidel (no email) 2010-08-10 03:16:33 PDT
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 76 Eric Seidel (no email) 2010-08-10 03:16:41 PDT
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 77 Kenneth Rohde Christiansen 2010-08-14 07:44:57 PDT
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.
Comment 78 Jer Noble 2010-08-14 12:24:24 PDT
(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 79 Eric Carlson 2010-08-16 13:18:18 PDT
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 80 Eric Carlson 2010-08-16 13:53:57 PDT
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 81 Eric Carlson 2010-08-16 14:52:42 PDT
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.
Comment 82 Jer Noble 2010-08-16 18:25:10 PDT
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.
Comment 83 WebKit Review Bot 2010-08-16 18:28:56 PDT
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 84 Eric Carlson 2010-08-19 14:20:08 PDT
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 85 Eric Carlson 2010-08-19 14:42:16 PDT
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 86 Eric Carlson 2010-08-19 15:09:20 PDT
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.
Comment 87 Jer Noble 2010-08-19 15:28:45 PDT
(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!
Comment 88 Jer Noble 2010-08-19 15:31:57 PDT
(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.
Comment 89 Jer Noble 2010-08-20 14:58:43 PDT
Created attachment 64997 [details]
Part 7: FullScreen-LayoutTests
Comment 90 Eric Carlson 2010-08-26 11:23:55 PDT
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 91 Eric Carlson 2010-08-26 11:31:43 PDT
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.
Comment 92 Jer Noble 2010-08-26 11:53:21 PDT
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 93 Eric Carlson 2010-08-26 11:59:37 PDT
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 94 Eric Carlson 2010-08-27 11:33:31 PDT
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.
Comment 95 Jer Noble 2010-08-27 13:49:27 PDT
Committed r66251: <http://trac.webkit.org/changeset/66251>
Comment 96 Jer Noble 2010-08-27 15:40:01 PDT
Committed r66267: <http://trac.webkit.org/changeset/66267>
Comment 97 WebKit Review Bot 2010-08-27 16:52:52 PDT
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
Comment 98 Jeremy Orlow 2010-09-04 06:04:39 PDT
(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.
Comment 99 Steve Block 2010-09-04 11:08:06 PDT
Android does not used a Skipped list. I removed it in http://trac.webkit.org/changeset/66793
Comment 100 Kent Tamura 2010-10-14 01:47:31 PDT
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.
Comment 101 Nicky Robinson 2013-08-13 10:44:57 PDT
(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.