Bug 53117 – Invalidate rect doesn't work for windowless plugins on Chromium

• Component: New Bugs
• Status: RESOLVED FIXED

[Sailesh Agrawal]

Steps:
1. Enable "GPU Accelerated Compositing" in about:flags
2. Go to http://apple.com/appletv
3. Abut half way down the page, click on the "See what's new with Apple TV" link.

Expected:
The "What's new with Apple TV" movie should play smoothly.

Actual:
The video doesn't update unless you resize the window.

This can be reproduced on Mac OS 10.6 and Windows 7.

Also see the Chromium bug:
http://crbug.com/64370

[Sailesh Agrawal]

Created attachment 80105 [details]
Proposed patch.

[James Robinson]

Comment on attachment 80105 [details]
Proposed patch.

This needs a regression test, although it does look correct.

[Sailesh Agrawal]

(In reply to comment #2)
> (From update of attachment 80105 [details])
> This needs a regression test, although it does look correct.

Hi James. I'm new to this code base. What would be a good regression test? Does that mean adding a unit test for this bug? Thanks.

[Sailesh Agrawal]

Created attachment 81248 [details]
Patch with regression test

[Sailesh Agrawal]

Added a new patch with a regression test.

The regression test doesn't work yet. There are two things I need help with:

#1 Where should I place the test HTML file? Is there a way to get it automatically executed with --platform chromium-gpu?

#2 I can't seem to get any paint event to the pugin in the DumpRenderTree app. I think this is due to a difference between Chromium and DumpRenderTree. To work around this I changed webkit/plugins/npapi/webplugin_delegate_impl_mac.mm WebPluginDelegateImpl::WindowlessPaint() to not to check for buffer_context_. I'm not sure what the correct fix is.

Thanks!

[Sailesh Agrawal]

Created attachment 81249 [details]
removed debugging code form HTML file

[Kenneth Russell]

(In reply to comment #5)
> Added a new patch with a regression test.
> 
> The regression test doesn't work yet. There are two things I need help with:
> 
> #1 Where should I place the test HTML file? Is there a way to get it automatically executed with --platform chromium-gpu?

It should go under LayoutTests/plugins/, I think, and run on all platforms. Right now platform/chromium-gpu/test_expectations.txt skips all of the plugin tests, but you could explicitly enable this one. Note though that this test_expectations.txt is in the process of being merged with the main one, and new "CPU"/"GPU" flags are being added. I am not 100% sure where this work stands. CC'ing dpranke and mihaip.

> #2 I can't seem to get any paint event to the pugin in the DumpRenderTree app. I think this is due to a difference between Chromium and DumpRenderTree. To work around this I changed webkit/plugins/npapi/webplugin_delegate_impl_mac.mm WebPluginDelegateImpl::WindowlessPaint() to not to check for buffer_context_. I'm not sure what the correct fix is.

You'll need to do more debugging. Nobody on the CC: list is an expert in plugins in DRT to the best of my knowledge. You might try contacting Tony Chang.

[Kenneth Russell]

Comment on attachment 81249 [details]
removed debugging code form HTML file

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

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:136
> +  renderer->repaintRectangle(dirtyRect);

Indentation doesn't follow WebKit style in this file.

> LayoutTests/platform/chromium-gpu/plugin/invalidate_rect-expected.txt:8
>    + LF

You should disable "enable-auto-props = yes" in ~/.subversion/config when adding these new files to the WebKit repository.

[Mihai Parparita]

(In reply to comment #7)
> It should go under LayoutTests/plugins/, I think, and run on all platforms. Right now platform/chromium-gpu/test_expectations.txt skips all of the plugin tests, but you could explicitly enable this one. Note though that this test_expectations.txt is in the process of being merged with the main one, and new "CPU"/"GPU" flags are being added. I am not 100% sure where this work stands. CC'ing dpranke and mihaip.

The merging of files just landed (http://trac.webkit.org/changeset/77666). If you want to include a new directory in the GPU suite, you'll need to modify the list in chromium_gpy.py (http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py?rev=77678#L74)

[Dirk Pranke]

(In reply to comment #9)
> (In reply to comment #7)
> > It should go under LayoutTests/plugins/, I think, and run on all platforms. Right now platform/chromium-gpu/test_expectations.txt skips all of the plugin tests, but you could explicitly enable this one. Note though that this test_expectations.txt is in the process of being merged with the main one, and new "CPU"/"GPU" flags are being added. I am not 100% sure where this work stands. CC'ing dpranke and mihaip.
> 
> The merging of files just landed (http://trac.webkit.org/changeset/77666). If you want to include a new directory in the GPU suite, you'll need to modify the list in chromium_gpy.py (http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py?rev=77678#L74)

You can also just list the one file, but it seems like directory at a time is probably a better strategy.

[Sailesh Agrawal]

Created attachment 81691 [details]
Patch

[Sailesh Agrawal]

> > #2 I can't seem to get any paint event to the pugin in the DumpRenderTree app. I think this is due to a difference between Chromium and DumpRenderTree. To work around this I changed webkit/plugins/npapi/webplugin_delegate_impl_mac.mm WebPluginDelegateImpl::WindowlessPaint() to not to check for buffer_context_. I'm not sure what the correct fix is.
> 
> You'll need to do more debugging. Nobody on the CC: list is an expert in plugins in DRT to the best of my knowledge. You might try contacting Tony Chang.

I haven't been able to get in touch with Tony yet.

With the current patch the test should work on all platforms except chromium-gpu. Would it be ok if I filed a bug to fix this on chromium-gpu and skipped the test on that platform for now?

[Kenneth Russell]

Comment on attachment 81691 [details]
Patch

Yes, it's fine to skip the test on chromium-gpu for the moment. Note that test_expectations.txt has been merged, so you'll need to use the new GPU tag in LayoutTests/platform/chromium/test_expectations.txt. I assume you'll need to generate a new patch so I'm only marking this r+ for now, not cq+.

[Tony Chang]

(In reply to comment #13)
> (From update of attachment 81691 [details])
> Yes, it's fine to skip the test on chromium-gpu for the moment. Note that test_expectations.txt has been merged, so you'll need to use the new GPU tag in LayoutTests/platform/chromium/test_expectations.txt. I assume you'll need to generate a new patch so I'm only marking this r+ for now, not cq+.

Sailesh and I talked on IRC and he said it's failing on chromium gpu and non-gpu.  I'm not really familiar with WebPluginContainerImpl.cpp, but jam is.  Maybe he has some insight as to why this isn't working?

[Sailesh Agrawal]

Created attachment 81726 [details]
Patch

[Sailesh Agrawal]

> With the current patch the test should work on all platforms except chromium-gpu. Would it be ok if I filed a bug to fix this on chromium-gpu and skipped the test on that platform for now?

I was wrong about this. This patch doesn't work on chromium-mac with and without GPU. I've added an exception in test_expectations.txt for this. I've also filed bug 54051 to fix this.

Thanks.

[Kenneth Russell]

Comment on attachment 81726 [details]
Patch

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

Looks good overall. Thanks for persevering with this fix.

> LayoutTests/ChangeLog:11
> +        * plugins/invalidate_rect-expected.txt: Added.
> +        * plugins/invalidate_rect.html: Added.

The ChangeLog ideally should have been regenerated to contain the update to platform/chromium/test_expectations.txt. However since we've gone back and forth so many times on this patch I'm just going to cq+ it as is.

[WebKit Commit Bot]

Comment on attachment 81726 [details]
Patch

Clearing flags on attachment: 81726

Committed r78010: <http://trac.webkit.org/changeset/78010>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nico Weber]

Sail & reviewers,

how risky is this patch? To me, it looks reasonably simple – should we try to merge it to m10, given that we want to enable the compositor in m10, which will trigger this problem more often?

[Brian Weinstein]

This test has been failing on Windows since it landed, added Windows-specific failing results in r78082.

If you could investigate and figure out the issue that is causing this test to fail on Windows, that would be great.

https://bugs.webkit.org/show_bug.cgi?id=54122 tracks the failure.

[Sailesh Agrawal]

(In reply to comment #21)
> This test has been failing on Windows since it landed, added Windows-specific failing results in r78082.
> 
> If you could investigate and figure out the issue that is causing this test to fail on Windows, that would be great.
> 
> https://bugs.webkit.org/show_bug.cgi?id=54122 tracks the failure.

Working on it right now. Thanks.