Bug 76735 - [chromium] Tiled layers that get skipped once never get to draw again
Summary: [chromium] Tiled layers that get skipped once never get to draw again
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vangelis Kokkevis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-20 13:48 PST by Vangelis Kokkevis
Modified: 2012-01-24 19:28 PST (History)
4 users (show)

See Also:


Attachments
Patch (7.77 KB, patch)
2012-01-20 13:57 PST, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Patch (7.77 KB, patch)
2012-01-20 16:10 PST, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Resync-ed and resolved conflicts (7.71 KB, patch)
2012-01-20 18:14 PST, Vangelis Kokkevis
jamesr: review+
jamesr: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vangelis Kokkevis 2012-01-20 13:48:57 PST
If m_skipsDraw is set on a tiled layer because it's been unable to reserve a texture, the flag will stick with it for as long as the layer is around.  The issue is that TiledLayerChromium::drawsContent() will return false and the layer will never end up in a RS's layer list.  As a result, prepareToUpdate() will never be called on it and m_skipsDraw won't be reset.

This is a big issue for root layers as they stick around even after reloading the page.
Comment 1 Vangelis Kokkevis 2012-01-20 13:57:24 PST
Created attachment 123372 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-20 14:00:33 PST
Attachment 123372 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Adrienne Walker 2012-01-20 15:55:56 PST
Comment on attachment 123372 [details]
Patch

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

Nice catch! I guess this must have regressed when I changed which layers got added to render surface layer lists.

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:416
> +    // Neet to set the max limit again as it gets ovewritten by updateLayers().

nit: typos.

Also, it's really too bad you have to do this.  Maybe when we separate out the texture manager's responsibilities more it'll be possible to have a test interact with it more reasonably.
Comment 4 Vangelis Kokkevis 2012-01-20 16:10:19 PST
Created attachment 123401 [details]
Patch
Comment 5 Vangelis Kokkevis 2012-01-20 16:10:57 PST
(In reply to comment #4)
> Created an attachment (id=123401) [details]
> Patch

Fixed typos (oops!) and order of include files.
Comment 6 WebKit Review Bot 2012-01-20 16:11:56 PST
Attachment 123401 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
 + baa86bc...bf9dada master     -> origin/master  (forced update)
	M	Source/WebKit/chromium/ChangeLog
	M	Source/WebKit/chromium/DEPS
	M	Source/WebKit/mac/Plugins/WebNetscapePluginView.mm
	M	Source/WebKit/mac/Plugins/Hosted/WebHostedNetscapePluginView.mm
	M	Source/WebKit/mac/Plugins/WebBasePluginPackage.mm
	M	Source/WebKit/mac/Plugins/WebBaseNetscapePluginView.mm
	M	Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm
	M	Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm
	M	Source/WebKit/mac/ChangeLog
	M	Source/WebKit/mac/Misc/WebElementDictionary.mm
	M	Source/WebKit/mac/Misc/WebIconDatabase.mm
	M	Source/WebKit/mac/History/WebHistoryItem.mm
	M	Source/WebKit/mac/History/WebBackForwardList.mm
	M	Source/WebKit/mac/Carbon/CarbonWindowAdapter.mm
	M	Source/WebKit/mac/WebView/WebArchive.mm
	M	Source/WebKit/mac/WebView/WebViewData.mm
	M	Source/WebKit/mac/WebView/WebView.mm
	M	Source/WebKit/mac/WebView/WebResource.mm
	M	Source/WebKit/mac/WebView/WebDataSource.mm
	M	Source/WebKit/mac/WebView/WebTextIterator.mm
	M	Source/WebKit/mac/WebView/WebHTMLView.mm
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/page/DOMWindow.cpp
	M	Source/WebCore/platform/RunLoop.cpp
	M	Source/WebCore/platform/mac/RunLoopMac.mm
	M	Source/WebCore/platform/RunLoop.h
r105554 = 507f56cd2ce6237ed37916e4d44446183ee0a24d (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Applying: Make WebCore RunLoop work for WebKit1
Using index info to reconstruct a base tree...
<stdin>:41: trailing whitespace.
        
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Make WebCore RunLoop work for WebKit1

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 170.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Vangelis Kokkevis 2012-01-20 18:14:37 PST
Created attachment 123422 [details]
Resync-ed and resolved conflicts
Comment 8 James Robinson 2012-01-23 12:52:07 PST
Comment on attachment 123422 [details]
Resync-ed and resolved conflicts

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

> Source/WebCore/ChangeLog:8
> +        with the layer for the remainign of its lifetime.

typo: remainign -> remainder

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:99
> +class FakeCCLayerTreeHostClient : public CCLayerTreeHostClient {

this is very nearly the same thing as FakeCCLayerTreeHostClient in LayerChromiumTest.cpp. Could you unify them and put it in a shared location? Seems like a quite handy thing to have for unit tests.
Comment 9 Vangelis Kokkevis 2012-01-24 19:28:26 PST
Committed r105845: <http://trac.webkit.org/changeset/105845>