RESOLVED FIXED 35184
Fix chromium iframe shims
https://bugs.webkit.org/show_bug.cgi?id=35184
Summary Fix chromium iframe shims
John Abd-El-Malek
Reported 2010-02-19 15:57:48 PST
Add another test case to the iframes-shims test
Attachments
Patch (4.55 KB, patch)
2010-02-19 16:01 PST, John Abd-El-Malek
simon.fraser: review-
Ensure that Widget is notified (via setWidgetGeometry) during RenderWidget::updateWidgetPosition() (728 bytes, patch)
2010-02-22 13:01 PST, Thatcher Ulrich
no flags
Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions() (2.96 KB, patch)
2010-02-22 14:00 PST, Thatcher Ulrich
no flags
Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions() (7.75 KB, patch)
2010-02-25 06:42 PST, Thatcher Ulrich
no flags
Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions() (9.86 KB, patch)
2010-02-25 06:52 PST, Thatcher Ulrich
no flags
Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions() (9.90 KB, patch)
2010-02-25 07:14 PST, Thatcher Ulrich
fishd: review+
fishd: commit-queue-
fix tabs in ChangeLog (9.94 KB, patch)
2010-02-25 10:07 PST, Thatcher Ulrich
fishd: review+
commit-queue: commit-queue-
fix test expected whitespace (10.29 KB, patch)
2010-03-01 09:07 PST, Thatcher Ulrich
no flags
John Abd-El-Malek
Comment 1 2010-02-19 16:01:20 PST
Simon Fraser (smfr)
Comment 2 2010-02-19 16:33:21 PST
Will this testcase start to fail on Mac now? We can't take a change that makes a test start to fail; we'll have to make the patch at the same time.
John Abd-El-Malek
Comment 3 2010-02-19 16:45:12 PST
I haven't tested latest Safari or Chrome/Mac builds. I don't think it should fail on Macs since there aren't windowed plugins there, but it's up to whoever fixes this to verify of course.
Thatcher Ulrich
Comment 4 2010-02-22 13:01:05 PST
Created attachment 49237 [details] Ensure that Widget is notified (via setWidgetGeometry) during RenderWidget::updateWidgetPosition() On Chrome, windows plugins need to watch for other iframes on the page in order to implement iframe-shim cutout behavior. This patch fixes the bug by ensuring that an existing plugin is notified when a new iframe is added to the page.
Simon Fraser (smfr)
Comment 5 2010-02-22 13:06:18 PST
Comment on attachment 49237 [details] Ensure that Widget is notified (via setWidgetGeometry) during RenderWidget::updateWidgetPosition() It seems wrong that it is necessary to call setFrameRect() even when the frame has not changed. setFrameRect() should not have side effects. Whatever needs doing for windows plugins should be separated out into a method that is called at the appropriate times.
Thatcher Ulrich
Comment 6 2010-02-22 13:18:38 PST
(In reply to comment #5) > (From update of attachment 49237 [details]) > It seems wrong that it is necessary to call setFrameRect() even when the frame > has not changed. setFrameRect() should not have side effects. Whatever needs > doing for windows plugins should be separated out into a method that is called > at the appropriate times. frameRectsChanged() is an existing call and would work here. But I'm not sure if that conflicts with the intended semantics of frameRectsChanged(). I can invent a new virtual function in Widget if you think that makes more sense.
Thatcher Ulrich
Comment 7 2010-02-22 14:00:56 PST
Created attachment 49241 [details] Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions() A different approach using a new explicit Widget method, widgetPositionsUpdated()
John Abd-El-Malek
Comment 8 2010-02-22 16:10:09 PST
btw, this also breaks video ads on Google. i.e. http://www.google.com/search?aq=f&sourceid=chrome&ie=UTF-8&q=droid If you click on the + for the video ad, you'll see a white screen until you resize.
Thatcher Ulrich
Comment 9 2010-02-24 12:39:55 PST
(In reply to comment #8) > btw, this also breaks video ads on Google. i.e. > > http://www.google.com/search?aq=f&sourceid=chrome&ie=UTF-8&q=droid > > If you click on the + for the video ad, you'll see a white screen until you > resize. Just to clarify -- the most recent patch I attached fixes this problem. Simon -- have you had a chance to look at this yet?
Darin Fisher (:fishd, Google)
Comment 10 2010-02-24 13:06:13 PST
Comment on attachment 49241 [details] Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions() This change looks good to me. I think this is a cleaner way of addressing the issue. However, it needs a proper ChangeLog entry, and it would be good to include jam's testcase in this patch. That way they get committed together. Thanks!
John Abd-El-Malek
Comment 11 2010-02-24 17:18:12 PST
one small note: I tested latest webkit trunk with Safari on Windows. The video ads are working. The iframe shim stuff isn't, which is to be expected, since WebKit plugin implementation on Windows doesn't take care of the cutout rectangles. So, the layout test I added above should be sufficient.
Thatcher Ulrich
Comment 12 2010-02-25 06:42:04 PST
Created attachment 49484 [details] Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions() added ChangeLog entries and combined with jam's test case patch
WebKit Review Bot
Comment 13 2010-02-25 06:48:25 PST
Attachment 49484 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:14: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:15: Line contains tab character. [whitespace/tab] [5] Total errors found: 7 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Thatcher Ulrich
Comment 14 2010-02-25 06:52:55 PST
Created attachment 49485 [details] Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions() Oops, fix tabs and include the WebKit/chromium changes
WebKit Review Bot
Comment 15 2010-02-25 07:01:56 PST
Attachment 49485 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/chromium/src/WebPluginContainerImpl.cpp:175: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebKit/chromium/src/WebPluginContainerImpl.cpp:176: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebKit/chromium/ChangeLog:5: Line contains tab character. [whitespace/tab] [5] WebKit/chromium/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] WebKit/chromium/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] WebKit/chromium/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] WebKit/chromium/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] WebKit/chromium/ChangeLog:14: Line contains tab character. [whitespace/tab] [5] Total errors found: 8 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Thatcher Ulrich
Comment 16 2010-02-25 07:14:27 PST
Created attachment 49486 [details] Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions() fix style booboos
Simon Fraser (smfr)
Comment 17 2010-02-25 09:09:42 PST
Comment on attachment 49108 [details] Patch You need to land the testcase change along with the patch. No need for this as a separate patch.
Darin Fisher (:fishd, Google)
Comment 18 2010-02-25 09:11:50 PST
Comment on attachment 49486 [details] Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions() > Index: LayoutTests/ChangeLog ... > +2010-02-25 Thatcher Ulrich <tulrich@google.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Fix chromium iframe shims. Add another test case to the > + iframes-shims test. After r53637, the plugin widget doesn't get > + moved every paint. This used to hide the bug that if an iframe > + gets added, the plugin's cutout rectangles don't get updated until > + a layout happens. ^^^ these tabs need to be replaced with spaces. otherwise, R=me
Thatcher Ulrich
Comment 19 2010-02-25 10:07:53 PST
Created attachment 49505 [details] fix tabs in ChangeLog
Simon Fraser (smfr)
Comment 20 2010-02-25 10:49:42 PST
Please update the title of this bug to reflect the real issue, so I can find it in future.
WebKit Commit Bot
Comment 21 2010-02-26 01:36:22 PST
Comment on attachment 49505 [details] fix tabs in ChangeLog Rejecting patch 49505 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 12229 test cases. plugins/iframe-shims.html -> failed Exiting early after 1 failures. 9143 tests run. 198.89s total testing time 9142 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 7 test cases (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/313382
Thatcher Ulrich
Comment 22 2010-03-01 09:07:38 PST
Created attachment 49728 [details] fix test expected whitespace
WebKit Commit Bot
Comment 23 2010-03-01 14:51:33 PST
Comment on attachment 49728 [details] fix test expected whitespace Clearing flags on attachment: 49728 Committed r55381: <http://trac.webkit.org/changeset/55381>
WebKit Commit Bot
Comment 24 2010-03-01 14:51:39 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.