Bug 35184

Summary: Fix chromium iframe shims
Product: WebKit Reporter: John Abd-El-Malek <jam>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fishd, simon.fraser, tulrich, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
simon.fraser: review-
Ensure that Widget is notified (via setWidgetGeometry) during RenderWidget::updateWidgetPosition()
none
Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions()
none
Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions()
none
Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions()
none
Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions()
fishd: review+, fishd: commit-queue-
fix tabs in ChangeLog
fishd: review+, commit-queue: commit-queue-
fix test expected whitespace none

Description John Abd-El-Malek 2010-02-19 15:57:48 PST
Add another test case to the iframes-shims test
Comment 1 John Abd-El-Malek 2010-02-19 16:01:20 PST
Created attachment 49108 [details]
Patch
Comment 2 Simon Fraser (smfr) 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.
Comment 3 John Abd-El-Malek 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.
Comment 4 Thatcher Ulrich 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.
Comment 5 Simon Fraser (smfr) 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.
Comment 6 Thatcher Ulrich 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.
Comment 7 Thatcher Ulrich 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()
Comment 8 John Abd-El-Malek 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.
Comment 9 Thatcher Ulrich 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?
Comment 10 Darin Fisher (:fishd, Google) 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!
Comment 11 John Abd-El-Malek 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.
Comment 12 Thatcher Ulrich 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
Comment 13 WebKit Review Bot 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.
Comment 14 Thatcher Ulrich 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
Comment 15 WebKit Review Bot 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.
Comment 16 Thatcher Ulrich 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
Comment 17 Simon Fraser (smfr) 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.
Comment 18 Darin Fisher (:fishd, Google) 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
Comment 19 Thatcher Ulrich 2010-02-25 10:07:53 PST
Created attachment 49505 [details]
fix tabs in ChangeLog
Comment 20 Simon Fraser (smfr) 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.
Comment 21 WebKit Commit Bot 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
Comment 22 Thatcher Ulrich 2010-03-01 09:07:38 PST
Created attachment 49728 [details]
fix test expected whitespace
Comment 23 WebKit Commit Bot 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>
Comment 24 WebKit Commit Bot 2010-03-01 14:51:39 PST
All reviewed patches have been landed.  Closing bug.