Bug 53117 - Invalidate rect doesn't work for windowless plugins on Chromium
Summary: Invalidate rect doesn't work for windowless plugins on Chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Sailesh Agrawal
URL:
Keywords:
Depends on:
Blocks: 54051 54122
  Show dependency treegraph
 
Reported: 2011-01-25 12:37 PST by Sailesh Agrawal
Modified: 2011-02-09 10:55 PST (History)
10 users (show)

See Also:


Attachments
Proposed patch. (2.20 KB, patch)
2011-01-25 12:47 PST, Sailesh Agrawal
jamesr: review-
Details | Formatted Diff | Diff
Patch with regression test (11.08 KB, patch)
2011-02-04 11:10 PST, Sailesh Agrawal
no flags Details | Formatted Diff | Diff
removed debugging code form HTML file (10.40 KB, patch)
2011-02-04 11:16 PST, Sailesh Agrawal
no flags Details | Formatted Diff | Diff
Patch (13.43 KB, patch)
2011-02-08 14:13 PST, Sailesh Agrawal
no flags Details | Formatted Diff | Diff
Patch (13.95 KB, patch)
2011-02-08 17:51 PST, Sailesh Agrawal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sailesh Agrawal 2011-01-25 12:37:54 PST
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
Comment 1 Sailesh Agrawal 2011-01-25 12:47:55 PST
Created attachment 80105 [details]
Proposed patch.
Comment 2 James Robinson 2011-01-25 14:38:49 PST
Comment on attachment 80105 [details]
Proposed patch.

This needs a regression test, although it does look correct.
Comment 3 Sailesh Agrawal 2011-01-25 14:49:35 PST
(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.
Comment 4 Sailesh Agrawal 2011-02-04 11:10:48 PST
Created attachment 81248 [details]
Patch with regression test
Comment 5 Sailesh Agrawal 2011-02-04 11:11:13 PST
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!
Comment 6 Sailesh Agrawal 2011-02-04 11:16:54 PST
Created attachment 81249 [details]
removed debugging code form HTML file
Comment 7 Kenneth Russell 2011-02-04 12:23:24 PST
(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.
Comment 8 Kenneth Russell 2011-02-04 12:24:43 PST
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.
Comment 9 Mihai Parparita 2011-02-04 14:38:00 PST
(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)
Comment 10 Dirk Pranke 2011-02-04 15:37:58 PST
(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.
Comment 11 Sailesh Agrawal 2011-02-08 14:13:53 PST
Created attachment 81691 [details]
Patch
Comment 12 Sailesh Agrawal 2011-02-08 14:18:16 PST
> > #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?
Comment 13 Kenneth Russell 2011-02-08 14:29:26 PST
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+.
Comment 14 Tony Chang 2011-02-08 14:36:37 PST
(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?
Comment 15 Sailesh Agrawal 2011-02-08 17:51:05 PST
Created attachment 81726 [details]
Patch
Comment 16 Sailesh Agrawal 2011-02-08 17:53:11 PST
> 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.
Comment 17 Kenneth Russell 2011-02-08 18:09:55 PST
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.
Comment 18 WebKit Commit Bot 2011-02-08 19:30:47 PST
Comment on attachment 81726 [details]
Patch

Clearing flags on attachment: 81726

Committed r78010: <http://trac.webkit.org/changeset/78010>
Comment 19 WebKit Commit Bot 2011-02-08 19:30:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Nico Weber 2011-02-08 22:06:57 PST
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?
Comment 21 Brian Weinstein 2011-02-09 10:51:08 PST
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.
Comment 22 Sailesh Agrawal 2011-02-09 10:53:37 PST
(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.