WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Ensure that Widget is notified (via setWidgetGeometry) during RenderWidget::updateWidgetPosition()
(728 bytes, patch)
2010-02-22 13:01 PST
,
Thatcher Ulrich
no flags
Details
Formatted Diff
Diff
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
Details
Formatted Diff
Diff
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
Details
Formatted Diff
Diff
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
Details
Formatted Diff
Diff
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-
Details
Formatted Diff
Diff
fix tabs in ChangeLog
(9.94 KB, patch)
2010-02-25 10:07 PST
,
Thatcher Ulrich
fishd
: review+
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
fix test expected whitespace
(10.29 KB, patch)
2010-03-01 09:07 PST
,
Thatcher Ulrich
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
John Abd-El-Malek
Comment 1
2010-02-19 16:01:20 PST
Created
attachment 49108
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug