Bug 53117

Created attachment 80105 [details] Proposed patch.

Comment on attachment 80105 [details] Proposed patch. This needs a regression test, although it does look correct.

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

Created attachment 81248 [details] Patch with regression test

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!

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

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

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

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

Created attachment 81691 [details] Patch

> > #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 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+.

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

Created attachment 81726 [details] Patch

> 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 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 on attachment 81726 [details] Patch Clearing flags on attachment: 81726 Committed r78010: <http://trac.webkit.org/changeset/78010>

All reviewed patches have been landed. Closing bug.

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?

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.

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