Bug 68969 – [GTK] need iframe shim support for Gtk webkit

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKitGTK

chromium iframe shim code has been moved to cross-platform file, and supported it in Qt webkit. We are using Gtk webkit, and need to put html overlaying html on windowed plugin.Can you support iframe shim for Gtk webkit?

[Robert Hogan]

Why not experiment with adding the getPluginOcclusions() call to PluginViewGtk.cpp. PluginView::updateWidgetAllocationAndClip() looks like the place to start.

I will try,thanks a lot.
(In reply to comment #1)
> Why not experiment with adding the getPluginOcclusions() call to PluginViewGtk.cpp. PluginView::updateWidgetAllocationAndClip() looks like the place to start.

(In reply to comment #2)
> I will try,thanks a lot.
> (In reply to comment #1)
> > Why not experiment with adding the getPluginOcclusions() call to PluginViewGtk.cpp. PluginView::updateWidgetAllocationAndClip() looks like the place to start.

I added getPluginOcclusions() call in  PluginView::updateWidgetAllocationAndClip() in PluginViewGtk.cpp and it works well to use iframe shims to put html overlaying html on windowed plugin.

Created attachment 111572 [details]
iframe shims support in Gtk webkit

This patch use getPluginOcclusions() in IFrameShimSupport.cpp to support  overlay html on windowed plugin in Gtk webkit using iframe shims.

[WebKit Review Bot]

Comment on attachment 111572 [details]
iframe shims support in Gtk webkit

Rejecting attachment 111572 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Review Bot]

Comment on attachment 111572 [details]
iframe shims support in Gtk webkit

Rejecting attachment 111572 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Martin Robinson]

Comment on attachment 111572 [details]
iframe shims support in Gtk webkit

View in context: https://bugs.webkit.org/attachment.cgi?id=111572&action=review

Thanks for your contribution! This looks like a good change. Are there any tests that cover it that we can unskip? Your patch is also missing a ChangeLog, see http://www.webkit.org/coding/contributing.html.

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:568
> +		// Cut out areas of the plugin occluded by iframe shims
> +		Vector<IntRect> cutOutRects;
> +		getPluginOcclusions(m_element, this->parent(), frameRect(), cutOutRects);

You indentation looks off here. Be sure to use spaces. All comments that are complete sentences should end with a period.

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:574
> +				cutOutRects[i].move(-frameRect().x(), -frameRect().y());
> +				GdkRectangle cutOutRect=cutOutRects[i];
> +				gdk_region_subtract(clipRegion,gdk_region_rectangle(&cutOutRect));

Looks like the indentation is off below as well.

[Martin Robinson]

Oh, by the way, once you have uploaded your patch, set the review flag to ? and the commit-queue flag to ?. This says: "This patch needs a review and if it passes review should be landed by the commit bot."

Created attachment 111723 [details]
iframe shims support in Gtk webkit.

(In reply to comment #8)
> Oh, by the way, once you have uploaded your patch, set the review flag to ? and the commit-queue flag to ?. This says: "This patch needs a review and if it passes review should be landed by the commit bot."

I test my build with iframe-shims.html in LayoutTests/plugins/iframe-shims.html and it works.(In reply to comment #7)
> (From update of attachment 111572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111572&action=review
> 
> Thanks for your contribution! This looks like a good change. Are there any tests that cover it that we can unskip? Your patch is also missing a ChangeLog, see http://www.webkit.org/coding/contributing.html.
> 
> > Source/WebCore/plugins/gtk/PluginViewGtk.cpp:568
> > +		// Cut out areas of the plugin occluded by iframe shims
> > +		Vector<IntRect> cutOutRects;
> > +		getPluginOcclusions(m_element, this->parent(), frameRect(), cutOutRects);
> 
> You indentation looks off here. Be sure to use spaces. All comments that are complete sentences should end with a period.
> 
> > Source/WebCore/plugins/gtk/PluginViewGtk.cpp:574
> > +				cutOutRects[i].move(-frameRect().x(), -frameRect().y());
> > +				GdkRectangle cutOutRect=cutOutRects[i];
> > +				gdk_region_subtract(clipRegion,gdk_region_rectangle(&cutOutRect));
> 
> Looks like the indentation is off below as well.

Thanks, I've changed the comment and the indentation.You can unskip iframe-shims.html in LayoutTests/plugins/iframe-shims.html to test it, I run this case by hand. I didn't see any test case available for iframe shims in Gtk webkit.

(In reply to comment #8)
> Oh, by the way, once you have uploaded your patch, set the review flag to ? and the commit-queue flag to ?. This says: "This patch needs a review and if it passes review should be landed by the commit bot."

I test my build with iframe-shims.html in LayoutTests/plugins/iframe-shims.html and it works.(In reply to comment #7)
> (From update of attachment 111572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111572&action=review
> 
> Thanks for your contribution! This looks like a good change. Are there any tests that cover it that we can unskip? Your patch is also missing a ChangeLog, see http://www.webkit.org/coding/contributing.html.
> 
> > Source/WebCore/plugins/gtk/PluginViewGtk.cpp:568
> > +		// Cut out areas of the plugin occluded by iframe shims
> > +		Vector<IntRect> cutOutRects;
> > +		getPluginOcclusions(m_element, this->parent(), frameRect(), cutOutRects);
> 
> You indentation looks off here. Be sure to use spaces. All comments that are complete sentences should end with a period.
> 
> > Source/WebCore/plugins/gtk/PluginViewGtk.cpp:574
> > +				cutOutRects[i].move(-frameRect().x(), -frameRect().y());
> > +				GdkRectangle cutOutRect=cutOutRects[i];
> > +				gdk_region_subtract(clipRegion,gdk_region_rectangle(&cutOutRect));
> 
> Looks like the indentation is off below as well.

Thanks, I've changed the comment and the indentation.You can unskip iframe-shims.html in LayoutTests/plugins/iframe-shims.html to test it, I run this case by hand. I didn't see any test case available for iframe shims in Gtk webkit.

[Robert Hogan]

The annoying thing about the layout test is that it passes even without the shin support. See bug 3778. I think it needs a layouttestcontroller.repaint test to test the paint areas properly.

[Eric Seidel (no email)]

Comment on attachment 111572 [details]
iframe shims support in Gtk webkit

Please obsolete your old patches when uploading new ones, or use webkit-patch upload which will do that for you. :)

[Eric Seidel (no email)]

Comment on attachment 111723 [details]
iframe shims support in Gtk webkit.

View in context: https://bugs.webkit.org/attachment.cgi?id=111723&action=review

> Source/WebCore/ChangeLog:8
> +        Amend PluginViewGtk use IFframeShimSupport.h and IFframeShimSupport.cpp to handle shims correctly.

IFframe?

> Source/WebCore/GNUmakefile.list.am:4086
> +	Source/WebCore/plugins/IFrameShimSupport.cpp \
> +	Source/WebCore/plugins/IFrameShimSupport.h

Are these new files?  If so, they're missing from your patch.

[Robert Hogan]

(In reply to comment #14)
> > Source/WebCore/GNUmakefile.list.am:4086
> > +	Source/WebCore/plugins/IFrameShimSupport.cpp \
> > +	Source/WebCore/plugins/IFrameShimSupport.h
> 
> Are these new files?  If so, they're missing from your patch.

No, they're only used by chromium and qt at the moment though. Note that the layout tests don't actually work for this shim testing, on any platform. They pass even without iframe shims implemented. The test plugin needs to paint itself in order to show up properly in the hit testing I think.

[Martin Robinson]

Comment on attachment 111723 [details]
iframe shims support in Gtk webkit.

View in context: https://bugs.webkit.org/attachment.cgi?id=111723&action=review

I think with the typo fix and the minor change below this patch looks okay.

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:586
> +            cairo_region_t* cutOutRegion = cairo_region_create_rectangle(&cutOutRect);
> +            cairo_region_subtract(clipRegion, cutOutRegion);

Why not just do:
cairo_rectangle_int_t cutOutRect = cutOutRects[i];
cairo_region_subtract_rectangle(clipRegion, cutOutRegion);

Created attachment 140648 [details]
iframe shims support in Gtk webkit

Replace cairo_region_subtract with cairo_region_subtract_rectangle according to Martin Robinson's advice, thanks a lot.

[Martin Robinson]

Comment on attachment 140648 [details]
iframe shims support in Gtk webkit

View in context: https://bugs.webkit.org/attachment.cgi?id=140648&action=review

Sorry for letting this one go for so long. Do you mind fixing the ChangeLog so that I can land it with the commit-queue.

> ChangeLog:13
> +10	        * GNUmakefile.list.am:
> +11	        * plugins/gtk/PluginViewGtk.cpp:
> +12	        (WebCore::PluginView::updateWidgetAllocationAndClip):
> +

There are line numbers in the ChangeLog here.

[Xan Lopez]

Created attachment 169487 [details]
shims.diff

Went ahead and made a proper ChangeLog for the patch, plus kept the files in alphabetical order in the Makefile.

[kov's GTK+ EWS bot]

Comment on attachment 169487 [details]
shims.diff

Attachment 169487 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14460166

[Xan Lopez]

OK, seems this actually needed more than a new ChangeLog.

[Martin Robinson]

Xan, thanks for fixing the ChangeLog. Looks like we cannot cq+ this because it broke the build though.

No, I don't mind.Please go ahead.

(In reply to comment #18)
> (From update of attachment 140648 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140648&action=review
> 
> Sorry for letting this one go for so long. Do you mind fixing the ChangeLog so that I can land it with the commit-queue.
> 
> > ChangeLog:13
> > +10	        * GNUmakefile.list.am:
> > +11	        * plugins/gtk/PluginViewGtk.cpp:
> > +12	        (WebCore::PluginView::updateWidgetAllocationAndClip):
> > +
> 
> There are line numbers in the ChangeLog here.

[Eric Seidel (no email)]

Comment on attachment 140648 [details]
iframe shims support in Gtk webkit

Cleared Martin Robinson's review+ from obsolete attachment 140648 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Philippe Normand]

Ping Xan!

[Anders Carlsson]

Comment on attachment 169487 [details]
shims.diff

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Martin Robinson]

*** This bug has been marked as a duplicate of bug 132180 ***