Bug 57400 - Fullscreen code assumes all layers use GPU compositing when USE(ACCELERATED_COMPOSITING)
Summary: Fullscreen code assumes all layers use GPU compositing when USE(ACCELERATED_C...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-29 16:32 PDT by David Dorwin
Modified: 2011-04-05 10:41 PDT (History)
4 users (show)

See Also:


Attachments
Check isComposited() before using the backing and remove the asserts. (2.83 KB, patch)
2011-03-29 16:50 PDT, David Dorwin
no flags Details | Formatted Diff | Diff
Fixed indent and ChangeLog. (2.84 KB, patch)
2011-04-04 14:45 PDT, David Dorwin
no flags Details | Formatted Diff | Diff
Merged with head (2.84 KB, patch)
2011-04-05 09:30 PDT, David Dorwin
no flags Details | Formatted Diff | Diff
Merged with head (2.86 KB, patch)
2011-04-05 09:48 PDT, David Dorwin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Dorwin 2011-03-29 16:32:23 PDT
Document::webkitWillEnterFullScreenForElement, Document::webkitDidEnterFullScreenForElement, and Document::webkitWillExitFullScreenForElement all assert that m_fullScreenRenderer->layer()->backing() is valid inside the #if USE(ACCELERATED_COMPOSITING) blocks. However, accelerated compositing may be compiled in but disabled at runtime or for some elements. This is the case for Chromium, which supports a command line switch and does not currently accelerate all types of elements. In addition, the layout tests do not run with GPU acceleration enabled by default.

These asserts and subsequent use of a NULL backing cause the fullscreen tests to crash in the non-GPU-enabled runs of DumpRenderTree and when Chrome is run without GPU acceleration enabled.

I have a fix.
Comment 1 David Dorwin 2011-03-29 16:50:46 PDT
Created attachment 87432 [details]
Check isComposited() before using the backing and remove the asserts.
Comment 2 Jer Noble 2011-03-29 16:54:08 PDT
Looks good to me.
Comment 3 David Levin 2011-04-04 14:20:09 PDT
Comment on attachment 87432 [details]
Check isComposited() before using the backing and remove the asserts.

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

> Source/WebCore/ChangeLog:9
> +        Accelerated compositing may be compiled in but disabled at runtime or for so

This sentence looks incomplete.

> Source/WebCore/dom/Document.cpp:4924
> +          page()->chrome()->client()->setRootFullScreenLayer(m_fullScreenRenderer->layer()->backing()->graphicsLayer());

Your indent is incorrect. (4 space indent in WK unlike chromium with a 2 space indent).

> Source/WebCore/dom/Document.cpp:4949
> +          page()->chrome()->client()->setRootFullScreenLayer(m_fullScreenRenderer->layer()->backing()->graphicsLayer());

Ditto.
Comment 4 David Dorwin 2011-04-04 14:45:41 PDT
Created attachment 88134 [details]
Fixed indent and ChangeLog.
Comment 5 WebKit Commit Bot 2011-04-04 19:11:36 PDT
Comment on attachment 88134 [details]
Fixed indent and ChangeLog.

Rejecting attachment 88134 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2

Last 500 characters of output:
mmit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Parsed 2 diffs from patch file(s).
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/dom/Document.cpp
Hunk #1 succeeded at 4849 (offset -71 lines).
Hunk #2 FAILED at 4861.
Hunk #3 FAILED at 4874.
2 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/dom/Document.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8330185
Comment 6 David Dorwin 2011-04-05 09:30:51 PDT
Created attachment 88254 [details]
Merged with head
Comment 7 David Dorwin 2011-04-05 09:48:01 PDT
Created attachment 88257 [details]
Merged with head
Comment 8 WebKit Review Bot 2011-04-05 09:51:24 PDT
Comment on attachment 88257 [details]
Merged with head

Rejecting attachment 88257 [details] from commit-queue.

ddorwin@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 9 WebKit Commit Bot 2011-04-05 10:40:54 PDT
Comment on attachment 88257 [details]
Merged with head

Clearing flags on attachment: 88257

Committed r82956: <http://trac.webkit.org/changeset/82956>
Comment 10 WebKit Commit Bot 2011-04-05 10:41:00 PDT
All reviewed patches have been landed.  Closing bug.