Bug 82760 - WebViewImpl doesn't notify the page that the user has canceled fullscreen.
Summary: WebViewImpl doesn't notify the page that the user has canceled fullscreen.
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2012-03-30 11:32 PDT by Bill Budge
Modified: 2012-04-03 00:21 PDT (History)
5 users (show)

See Also:

Proposed Patch (1.95 KB, patch)
2012-03-30 12:35 PDT, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed patch (2.62 KB, patch)
2012-03-30 15:59 PDT, Bill Budge
pnormand: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (2.75 KB, patch)
2012-03-31 12:36 PDT, Bill Budge
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bill Budge 2012-03-30 11:32:26 PDT
Recent changes in WebCore::Document caused a regression in Chromium. The page isn't getting notified when the user presses ESC or clicks the exit fullscreen bubble.
Comment 1 Bill Budge 2012-03-30 12:35:03 PDT
Created attachment 134862 [details]
Proposed Patch
Comment 2 Darin Fisher (:fishd, Google) 2012-03-30 14:38:42 PDT
Comment on attachment 134862 [details]
Proposed Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1337
> +        doc->webkitCancelFullScreen();

I was expecting to see webkitCancelFullScreen called from willExitFullScreen.
The implication of calling webkitCancelFullScreen here is that we will have
already called Document::webkitWillExitFullScreenForElement.  It seems like
that might confuse Document (if not now, then later).
Comment 3 Bill Budge 2012-03-30 15:59:37 PDT
Created attachment 134908 [details]
Proposed patch
Comment 4 Philippe Normand 2012-03-30 18:40:34 PDT
Comment on attachment 134908 [details]
Proposed patch

Attachment 134908 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12247610
Comment 5 Bill Budge 2012-03-31 12:36:54 PDT
Created attachment 134973 [details]
Proposed Patch
Comment 6 Bill Budge 2012-03-31 12:38:14 PDT
Resubmitting. There was some problem with the last patch on GTK.
Comment 7 Darin Fisher (:fishd, Google) 2012-04-02 12:29:03 PDT
Comment on attachment 134973 [details]
Proposed Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1327
> +            doc->webkitCancelFullScreen();

if exiting fullscreen is initiated by JS calling document.webkitCancelFullScreen(),
then we will eventually reach this code again.  it looks like the second call to
webkitCancelFullScreen() will call webkitExitFullscreen().  in this case, do we
have to worry about a second webkitfullscreenchange event being generated?  it
looks like Document::addDocumentToFullScreenChangeEventQueue() could be called
more than once.
Comment 8 Bill Budge 2012-04-02 13:49:53 PDT
When we call webkitCancelFullScreen the second time, it exits at the first if-statement:

    if (!topDocument()->webkitFullscreenElement())

It seems a little fragile, but webkitExitFullscreen() isn't called a second time. If we want to avoid the second call, I can't see a way to distinguish whether the cancel is coming from the page or the embedder at this level. Ideas?
Comment 9 Darin Fisher (:fishd, Google) 2012-04-02 23:25:29 PDT
Comment on attachment 134973 [details]
Proposed Patch

OK, thanks for verifying that!
Comment 10 WebKit Review Bot 2012-04-03 00:21:29 PDT
Comment on attachment 134973 [details]
Proposed Patch

Clearing flags on attachment: 134973

Committed r112991: <http://trac.webkit.org/changeset/112991>
Comment 11 WebKit Review Bot 2012-04-03 00:21:34 PDT
All reviewed patches have been landed.  Closing bug.