Bug 44797 – fullscreen javascript bindings not implemented for v8

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tony Chang]

These were added http://trac.webkit.org/changeset/66251, but need to be implemented for V8.

Of course, the related layout tests are also failing.

cc-ing dglazkov to triage.  I'm going to add to test_expectations for now.

[David Dorwin]

Created attachment 83411 [details]
Patch

[David Dorwin]

With this patch and another small one to Chromium, Chromium supports the WebKit Full Screen APIs and passes all the layout tests when --enable-fullscreen is specified. The layout tests remain disabled until we make this the default.

As mentioned in the patch description, the fullscreen rendering still needs to be implemented.

[Darin Fisher (:fishd, Google)]

Comment on attachment 83411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83411&action=review

I only reviewed the WebKit/chromium changes.

> Source/WebKit/chromium/public/WebSettings.h:114
> +    virtual void setFullScreenEnabled(bool) = 0;

LGTM for this WebKit API change.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:872
> +    // TODO(ddorwin): We may need to call these someplace else when window resizes.

TODO(ddorwin) -> FIXME

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:879
> +    // TODO(ddorwin): We may need to call these someplace else when window resizes.

ditto

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:886
> +    // TODO(ddorwin): Implement.

ditto

[David Dorwin]

Created attachment 83561 [details]
Addressed Darin's feedback. TODO -> FIXME

[Eric Seidel (no email)]

Comment on attachment 83561 [details]
Addressed Darin's feedback. TODO -> FIXME

View in context: https://bugs.webkit.org/attachment.cgi?id=83561&action=review

Seems reasonable.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:886
> +    // FIXME: Implement.

we have a notImplemented() call which may be more useful than a FIXME.

[WebKit Commit Bot]

Comment on attachment 83561 [details]
Addressed Darin's feedback. TODO -> FIXME

Clearing flags on attachment: 83561

Committed r79584: <http://trac.webkit.org/changeset/79584>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This doesn't compile if ENABLE_FULLSCREEN_API is not set to true (which means it's broken on the canaries currently since they use features_overrides.gypi).  At a minimum you need a feature guard inside WebSettingsImpl::setFullScreenEnabled(bool enabled).

[James Robinson]

Reverted r79584 for reason:

[chromium] Patch does not compile if ENABLE_FULLSCREEN_API is not set

Committed r79588: <http://trac.webkit.org/changeset/79588>

[David Dorwin]

Created attachment 83724 [details]
Fixed the build break when ENABLE_FULLSCREEN_API is not 1 and used notImplemented() as Eric suggested.

[James Robinson]

Comment on attachment 83724 [details]
Fixed the build break when ENABLE_FULLSCREEN_API is not 1 and used notImplemented() as Eric suggested.

Looks like you'll get an unused variable warning in WebSettingsImpl::setFullScreenEnabled(bool enabled) if ENABLE(FULLSCREEN_API) is false, which is a compile error on at least some configs.  We have an UNUSED_PARAM() macro for such things.

[David Dorwin]

Created attachment 83726 [details]
Added UNUSED_PARAM().

Thanks for catching that. The Mac Chromium config is apparently not one of them.

[Darin Fisher (:fishd, Google)]

Comment on attachment 83726 [details]
Added UNUSED_PARAM().

View in context: https://bugs.webkit.org/attachment.cgi?id=83726&action=review

R=me, but please fix the nit before committing.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:889
> +  

please do not add gratuitous whitespace.  only one new line here.

[WebKit Review Bot]

http://trac.webkit.org/changeset/79588 might have broken GTK Linux 32-bit Release

[David Dorwin]

Created attachment 83747 [details]
Removed whitespace.

[WebKit Commit Bot]

Comment on attachment 83747 [details]
Removed whitespace.

Clearing flags on attachment: 83747

Committed r79774: <http://trac.webkit.org/changeset/79774>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Broke Chromium Windows:
http://build.webkit.org/builders/Chromium%20Win%20Release/builds/19456/steps/compile-webkit/logs/stdio

[Ryosuke Niwa]

(In reply to comment #18)
> Broke Chromium Windows:
> http://build.webkit.org/builders/Chromium%20Win%20Release/builds/19456/steps/compile-webkit/logs/stdio

I don't think this is a real failure after all.  It's failing on build.webkit.org but building fine on build.chromium.org.  We need to investigate what's happening though.

[David Dorwin]

(In reply to comment #19)
> (In reply to comment #18)
> > Broke Chromium Windows:
> > http://build.webkit.org/builders/Chromium%20Win%20Release/builds/19456/steps/compile-webkit/logs/stdio
> 
> I don't think this is a real failure after all.  It's failing on build.webkit.org but building fine on build.chromium.org.  We need to investigate what's happening though.

I think a clobber of the WebKit Chromium bots should resolve this. Looking at the build output, Document.cpp was not rebuilt even though the value of ENABLE_FULLSCREEN_API changed in Source/WebKit/chromium/features.gypi. The new value is being picked up in ChromeClientImpl.cpp, causing ChromeClientImpl::exitFullScreenForElement() to be compiled.

The full Chromium build still has ENABLE_FULLSCREEN_API=0 until I check in the corresponding Chromium CL.

[David Dorwin]

(In reply to comment #20)

The Chromium Win bot is passing now.