[chromium] fix for animated gif layout tests
Created attachment 78150 [details] Patch
This is a fix for http://code.google.com/p/chromium/issues/detail?id=62433
Are we killing test_shell entirely? If not, it might be nice to fix it as well, even if layout tests won't be affected.
(In reply to comment #3) > Are we killing test_shell entirely? If not, it might be nice to fix it as well, even if layout tests won't be affected. I don't think we plan on killing test_shell, just removing the layout test specific code there. Sure, I can look into porting this to test_shell as well, although I thought a paint would be triggered automatically when the window is displayed on the screen (i.e., as long as the window isn't behind another window).
Comment on attachment 78150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78150&action=review > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1447 > + postDelayedTask(new WebViewHostPaintTask(this), 0); Instead of needing a new class, can this use webkit_support::PostDelayedTask(painInvalidatedRegion, static_cast<void*>(this), 0); (the way closeWidgetSoon works?)
(In reply to comment #5) > (From update of attachment 78150 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78150&action=review > > > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1447 > > + postDelayedTask(new WebViewHostPaintTask(this), 0); > > Instead of needing a new class, can this use webkit_support::PostDelayedTask(painInvalidatedRegion, static_cast<void*>(this), 0); (the way closeWidgetSoon works?) I tried that, but it crashes on some tests. The task is queued up and the WebViewHost is deleted before the task is run. TaskList handles this by canceling the task if WebViewHost goes away. http/tests/appcache/crash-when-navigating-away-then-back.html is an example of a test that triggers this crash.
Comment on attachment 78150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78150&action=review >>> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1447 >> >> Instead of needing a new class, can this use webkit_support::PostDelayedTask(painInvalidatedRegion, static_cast<void*>(this), 0); (the way closeWidgetSoon works?) > > I tried that, but it crashes on some tests. The task is queued up and the WebViewHost is deleted before the task is run. TaskList handles this by canceling the task if WebViewHost goes away. > > http/tests/appcache/crash-when-navigating-away-then-back.html is an example of a test that triggers this crash. Would it make sense to retrofit webkit_support::PostDelayedTask to also use a TaskList so that it's more resilient (beyond the scope of this patch though)?
Committed r75398: <http://trac.webkit.org/changeset/75398>
(In reply to comment #8) > Committed r75398: <http://trac.webkit.org/changeset/75398> I reverted this in r75433 because of crashes on chromium win. I don't have a win box handy (waiting for a new machine) to test, so this will have to wait.
Created attachment 84304 [details] Patch
Here's a new patch that no longer crashes on Windows. However, it turns out that the extra paint calls revealed bug 55514, so I'm going to punt on this for now.
Comment on attachment 84304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84304&action=review > Tools/DumpRenderTree/chromium/WebThemeEngineDRTWin.cpp:233 > default: > - ASSERT_NOT_REACHED(); > + ctype = WebThemeControlDRTWin::CheckedRadioType; > + cstate = WebThemeControlDRTWin::IndeterminateState; > break; What value does 'state' have in this case? Setting IndeterminateState looks wrong because radio buttons shouldn't be indeterminate state.
(In reply to comment #12) > (From update of attachment 84304 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84304&action=review > > > Tools/DumpRenderTree/chromium/WebThemeEngineDRTWin.cpp:233 > > default: > > - ASSERT_NOT_REACHED(); > > + ctype = WebThemeControlDRTWin::CheckedRadioType; > > + cstate = WebThemeControlDRTWin::IndeterminateState; > > break; > > What value does 'state' have in this case? Setting IndeterminateState looks wrong because radio buttons shouldn't be indeterminate state. RenderThemeChromiumWin::determineState returned 9 for some tests. I'm not sure why the radio button had an indeterminate state.
Comment on attachment 84304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84304&action=review >>> Tools/DumpRenderTree/chromium/WebThemeEngineDRTWin.cpp:233 >>> break; >> >> What value does 'state' have in this case? Setting IndeterminateState looks wrong because radio buttons shouldn't be indeterminate state. > > RenderThemeChromiumWin::determineState returned 9 for some tests. I'm not sure why the radio button had an indeterminate state. Ah, I understand. RenderThemeChromiumWin adds 8 to the state value even if it's a radio button. We should add 8 only if appearance==CheckoxPart in RenderThemeChromiumWin::determineState().
Test related bugs being marked WontFix. TestExpectations will contain bug if still relevant.