Bug 75705 - [Chromium] Disable per-tile painting support on Windows.
Summary: [Chromium] Disable per-tile painting support on Windows.
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Critical
Assignee: David Reveman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-06 08:54 PST by David Reveman
Modified: 2012-02-15 11:32 PST (History)
4 users (show)

See Also:


Attachments
Patch (3.03 KB, patch)
2012-01-06 08:58 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (1.75 KB, patch)
2012-01-06 12:13 PST, David Reveman
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Reveman 2012-01-06 08:54:03 PST
Per-tile painting on WIN32 is not currently supported. Prevent it from being used and add ASSERT to reveal causes for it being enabled.
Comment 1 David Reveman 2012-01-06 08:55:59 PST
This is to fix http://code.google.com/p/chromium/issues/detail?id=107461 which still seem to be happening.
Comment 2 David Reveman 2012-01-06 08:58:21 PST
Created attachment 121436 [details]
Patch
Comment 3 Adrienne Walker 2012-01-06 09:10:40 PST
Isn't perTilePainting exposed in about:flags? Wouldn't that be the cause of it being turned on?

(That's really sad that per-tile painting is not supported on Windows.  Are there bugs open to address this? I was really hoping that we could get that to be the default texture uploader sometime soon.)
Comment 4 David Reveman 2012-01-06 09:22:00 PST
(In reply to comment #3)
> Isn't perTilePainting exposed in about:flags? Wouldn't that be the cause of it being turned on?

Oh, that's right. We're probably still seeing this crash because users explicitly enable it.

> 
> (That's really sad that per-tile painting is not supported on Windows.  Are there bugs open to address this? I was really hoping that we could get that to be the default texture uploader sometime soon.)

I haven't been able to determine the exact reason for why it's not working as don't have access to a Windows machine. I know that skia on Windows uses a HDC for text rendering and that breaks when using SkPictures. It still shouldn't crash the way it's currently doing.
Comment 5 Adrienne Walker 2012-01-06 10:38:35 PST
(In reply to comment #4)

> I haven't been able to determine the exact reason for why it's not working as don't have access to a Windows machine. I know that skia on Windows uses a HDC for text rendering and that breaks when using SkPictures. It still shouldn't crash the way it's currently doing.

If a bug isn't filed, can you file one?
Comment 6 David Reveman 2012-01-06 10:49:51 PST
(In reply to comment #5)
> (In reply to comment #4)
> 
> > I haven't been able to determine the exact reason for why it's not working as don't have access to a Windows machine. I know that skia on Windows uses a HDC for text rendering and that breaks when using SkPictures. It still shouldn't crash the way it's currently doing.
> 
> If a bug isn't filed, can you file one?

Done.

https://bugs.webkit.org/show_bug.cgi?id=75715
Comment 7 James Robinson 2012-01-06 11:25:29 PST
Please disable the about:flags entry on windows on the chrome side if it doesn't work. That might be sufficient for this issue.
Comment 8 James Robinson 2012-01-06 11:29:18 PST
Comment on attachment 121436 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:127
> +#if defined(WIN32)

in WebKit we use OS(WINDOWS)

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:128
> +    ASSERT(!host->settings().perTilePainting);

not sure this is useful to have. if we don't want to support the flag, then we should just ignore it

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:99
> +#if defined(WIN32)
> +    ASSERT(!m_settings.perTilePainting); // No per-tile painting support on Windows.
> +#endif

how is this helpful? it won't help us with crashes in the field, since ASSERT()s are only active in debug builds
Comment 9 David Reveman 2012-01-06 12:13:39 PST
Created attachment 121464 [details]
Patch
Comment 10 James Robinson 2012-01-06 12:36:51 PST
Comment on attachment 121464 [details]
Patch

This looks good. It will make life a little harder for people trying to fix per-tile painting, so would you mind waiting to see if the about:flags entry gets the crashrate down low enough on canaries such that we don't need to do this before landing it?

R=me
Comment 11 David Reveman 2012-01-06 12:41:06 PST
(In reply to comment #10)
> (From update of attachment 121464 [details])
> This looks good. It will make life a little harder for people trying to fix per-tile painting, so would you mind waiting to see if the about:flags entry gets the crashrate down low enough on canaries such that we don't need to do this before landing it?
> 
> R=me

yup, I think that's a good idea.