Bug 35184 – Fix chromium iframe shims

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[John Abd-El-Malek]

Add another test case to the iframes-shims test

[John Abd-El-Malek]

Created attachment 49108 [details]
Patch

[Simon Fraser (smfr)]

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]

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]

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 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]

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

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]

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]

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

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]

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]

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]

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]

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]

Created attachment 49486 [details]
Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions()

fix style booboos

[Simon Fraser (smfr)]

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 on attachment 49486 [details]
Ensure that Widget is notified via a new method widgetPositionsUpdated() during RenderView::updateWidgetPositions()

> Index: LayoutTests/ChangeLog
...
> [phone]  Thatcher Ulrich  <[e-mail]>
> +
> +        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]

Created attachment 49505 [details]
fix tabs in ChangeLog

[Simon Fraser (smfr)]

Please update the title of this bug to reflect the real issue, so I can find it in future.

[WebKit Commit Bot]

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]

Created attachment 49728 [details]
fix test expected whitespace

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.