Bug 52008 – [chromium] fix for animated gif layout tests

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Tony Chang]

[chromium] fix for animated gif layout tests

[Tony Chang]

Created attachment 78150 [details]
Patch

[Tony Chang]

This is a fix for http://code.google.com/p/chromium/issues/detail?id=62433

[Peter Kasting]

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.

[Tony Chang]

(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).

[Mihai Parparita]

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?)

[Tony Chang]

(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.

[Mihai Parparita]

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)?

[Tony Chang]

Committed r75398: <http://trac.webkit.org/changeset/75398>

[Tony Chang]

(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.

[Tony Chang]

Created attachment 84304 [details]
Patch

[Tony Chang]

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.

[Kent Tamura]

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.

[Tony Chang]

(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.

[Kent Tamura]

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().

[Stephen Chenney]

Test related bugs being marked WontFix. TestExpectations will contain bug if still relevant.