Add a Cairo implementation to complement the patch from bug 90116.
Created attachment 173539 [details] Patch
Attachment 173539 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:44: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 173539 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173539&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:365 > + cairo_set_source_rgb(cr, 1, 0, 0); Qt has comment about blue color that it will turn red because R+B are not swapped. Will it result blue or red here? If it will result in different color from specified, IMHO, it requires comment here. > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:114 > + return (String(getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS")) == "1"); Should we use EFL prefix as Qt does?
> > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedBackingStore.cpp:114 > > + return (String(getenv("WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS")) == "1"); > > Should we use EFL prefix as Qt does? I don't think that's needed, since Cairo is not EFL specific.
Comment on attachment 173539 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173539&action=review Looks sane to me. >> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:365 >> + cairo_set_source_rgb(cr, 1, 0, 0); > > Qt has comment about blue color that it will turn red because R+B are not swapped. > Will it result blue or red here? If it will result in different color from specified, IMHO, it requires comment here. You probably want to do the same thing as Qt above here. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:372 > + cairo_move_to(cr, 2, pointSize); Is this 2 pixels just padding?
(In reply to comment #3) > You probably want to do the same thing as Qt above here. For Cairo, this patch will paint in red, just like the code above for Qt. > Is this 2 pixels just padding? Yes, could be anything.
(In reply to comment #6) > (In reply to comment #3) > > You probably want to do the same thing as Qt above here. > > For Cairo, this patch will paint in red, just like the code above for Qt. Yes, it seems it will be red, because updateContents always swizzles.
Created attachment 174259 [details] Fix style
Comment on attachment 174259 [details] Fix style View in context: https://bugs.webkit.org/attachment.cgi?id=174259&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:357 > + // cairo_text_extents() requires a cairo_t, so dimensions need to be guesstimated. no way of getting hold of that? > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:361 > + cairo_surface_t* surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, width, height); why not use RefPtr they work for cairo_surface_t! etc > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:362 > + cairo_t* cr = cairo_create(surface); Here you have one, a bit too late :) > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:363 > + cairo_surface_destroy(surface); Is this safe here? cr stays valid?
(In reply to comment #9) > (From update of attachment 174259 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174259&action=review > > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:357 > > + // cairo_text_extents() requires a cairo_t, so dimensions need to be guesstimated. > > no way of getting hold of that? AFAIK no. We need the width and height to create the surface, which we need to create the context, which we need to use the text extents, which we need to create the surface in the first place. In Qt, QFontMetrics don't need a QPainter. There might be a way in Cairo, but I'm not aware of it. Of course I could create a separate context just for the purpose, but I think there's no point. > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:361 > > + cairo_surface_t* surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, width, height); > > why not use RefPtr they work for cairo_surface_t! etc I know, I chose to explicitly destroy it locally with no smart pointer allocation because its life cycle is pretty tiny and controlled. > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:363 > > + cairo_surface_destroy(surface); > > Is this safe here? cr stays valid? From http://www.cairographics.org/manual/cairo-cairo-t.html#cairo-create : "This function references target, so you can immediately call cairo_surface_destroy() on it if you don't need to maintain a separate reference to it."
Comment on attachment 174259 [details] Fix style Clearing flags on attachment: 174259 Committed r134676: <http://trac.webkit.org/changeset/134676>
All reviewed patches have been landed. Closing bug.
Can this new code be combined with TextureMapperLayer::drawRepaintCounter somehow?