Bug 71350

Created attachment 113287 [details] Initial implementation

Comment on attachment 113287 [details] Initial implementation In principle I think this looks pretty good actually. But are you sure that you don't want to have a back and a front buffer to benefit from atomic tile switching? It's rather simple to do I'd say.

(In reply to comment #2) > (From update of attachment 113287 [details]) > In principle I think this looks pretty good actually. But are you sure that you don't want to have a back and a front buffer to benefit from atomic tile switching? It's rather simple to do I'd say. Yes, this is a very good feature. Due to that I still don't know if this patch will be applied. I would like to commit this patch as is it firstly. Next, new features can be added to existing code.

Comment on attachment 113287 [details] Initial implementation View in context: https://bugs.webkit.org/attachment.cgi?id=113287&action=review > Source/WebCore/platform/graphics/cairo/OwnPtrCairo.cpp:56 > +template <> void deleteOwnedPtr<cairo_region_t>(cairo_region_t* ptr) > +{ > + if (ptr) > + cairo_region_destroy(ptr); > +} cairo_region_t should not be wrapped in an OwnPtr because it's reference counted. Instead you should use RefPtrCairo. > Source/WebCore/platform/graphics/cairo/OwnPtrCairo.cpp:62 > +template <> void deleteOwnedPtr<cairo_rectangle_t>(cairo_rectangle_t* ptr) > +{ > + if (ptr) > + free(ptr); > +} I think you can aovid this, because the default implementation calls delete. > Source/WebCore/platform/graphics/cairo/TileCairo.cpp:78 > + OwnPtr<cairo_rectangle_int_t> rects = adoptPtr(new cairo_rectangle_int_t[rectCount]); It's probably better here to use OwnFastMallocPtr and fastAlloc. > Source/WebCore/platform/graphics/cairo/TileCairo.cpp:91 > + m_buffer = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, m_backingStore->tileSize().width(), m_backingStore->tileSize().height())); This line is pretty long. I'd prefer it shorter, at least. m_buffer = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, m_backingStore->tileSize().width(), m_backingStore->tileSize().height())); > Source/WebCore/platform/graphics/cairo/TileCairo.cpp:126 > + PlatformContextCairo* platformContext = context->platformContext(); > + cairo_t* cr = platformContext->cr(); No need to cache platformContext here. You can just do: cairo_t* cr = context->platformContext()->cr(); > Source/WebCore/platform/graphics/cairo/TileCairo.cpp:127 > + cairo_set_source_surface(cr, m_buffer.get(), target.x() - source.x(), target.y() - source.y()); Okay. The result is target.x() - source.x() source.x() = target.x() - m_rect.x() Therefore the result is target.x() - target.x() + m_rect.x() Am I missing something here or can you just write m_rect.x() and m_rect.y() and get rid of source altogether? > Source/WebCore/platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp:46 > +static const unsigned checkerColor1 = 0xff555555; > +static const unsigned checkerColor2 = 0xffaaaaaa; What colors are these? > Source/WebCore/platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp:63 > + if (alternate) > + setSourceRGBAFromColor(cr.get(), checkerColor1); > + else > + setSourceRGBAFromColor(cr.get(), checkerColor2); I think it's much clearer to call cairo_set_source_rgba directly here!

Thanks for review. I will put a new version of patch soon. > > Source/WebCore/platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp:46 > > +static const unsigned checkerColor1 = 0xff555555; > > +static const unsigned checkerColor2 = 0xffaaaaaa; > > What colors are these? I am sorry I don't know names of these colors (I am a men) but it are generally grey (light and dark). These colors are defined in TileQt also. May be the better idea will be to have one checker color definition for all port. > > Source/WebCore/platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp:63 > > + if (alternate) > > + setSourceRGBAFromColor(cr.get(), checkerColor1); > > + else > > + setSourceRGBAFromColor(cr.get(), checkerColor2); > > I think it's much clearer to call cairo_set_source_rgba directly here! Yes, it could be done. But if the above colors definition will stay the use of setSourceRGBAFromColor is simpler then cairo_set_source_rgba.

Created attachment 113630 [details] Updated Updated according to Martin Robinson's suggestions.

Comment on attachment 113630 [details] Updated View in context: https://bugs.webkit.org/attachment.cgi?id=113630&action=review This looks pretty close, but I have a few more comments. Sorry for the delay in reviewing. > Source/WebCore/ChangeLog:12 > + Added TileCairo and TiledBackingStoreBackend files needed by Tiled Backing Store implementation that uses cairo > + library eg. EFL and Gtk WebKit port. > + This patch contains some parts of rebased and updated code from 45423 bug. Originals authors of the code are: > + Julien Chaffraix <jchaffraix@codeaurora.org>, Zaheer Ahmad <zahmad@codeaurora.org>, > + Joone Hur <joone.hur@collabora.co.uk> In the future please try to align these lines into a paragraph. > Source/WebCore/ChangeLog:35 > + * platform/graphics/cairo/RefPtrCairo.cpp: > + (WTF::refIfNotNull): > + (WTF::derefIfNotNull): > + * platform/graphics/cairo/RefPtrCairo.h: > + * platform/graphics/cairo/TileCairo.cpp: Added. > + (WebCore::TileCairo::TileCairo): > + (WebCore::TileCairo::~TileCairo): > + (WebCore::TileCairo::isDirty): > + (WebCore::TileCairo::isReadyToPaint): > + (WebCore::TileCairo::invalidate): > + (WebCore::TileCairo::updateBackBuffer): > + (WebCore::TileCairo::swapBackBufferToFront): > + (WebCore::TileCairo::paint): > + (WebCore::TileCairo::resize): > + * platform/graphics/cairo/TileCairo.h: Added. > + (WebCore::TileCairo::create): > + (WebCore::TileCairo::coordinate): > + (WebCore::TileCairo::rect): > + * platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp: Added. > + (WebCore::TiledBackingStoreBackend::createTile): > + (WebCore::checkeredSurface): > + (WebCore::TiledBackingStoreBackend::paintCheckerPattern): It's a good idea to fill in these as well. Take a look at other ChangeLog entries for an example. > Source/WebCore/platform/graphics/cairo/RefPtrCairo.cpp:116 > - > #endif > - > } It seems these lines were removed accidentally. > Source/WebCore/platform/graphics/cairo/TileCairo.cpp:120 > + cairo_t* cr = context->platformContext()->cr(); Extra space after '=' > Source/WebCore/platform/graphics/cairo/TileCairo.h:3 > +/* > + Copyright (C) 2011 Samsung Electronics > + Doesn't it make sense for this file to have the same license as TileCairo.cpp? I think it's pretty weird to give them different licenses randomly. I'm unsure exactly what legal problems this could pose, but it's best to make them the same. > Source/WebCore/platform/graphics/cairo/TileCairo.h:39 > + static PassRefPtr<Tile> create(TiledBackingStore* backingStore, const Coordinate& tileCoordinate) { return adoptRef(new TileCairo(backingStore, tileCoordinate)); } Please reformat this to be across multiple lines. It's quite a long line right now. > Source/WebCore/platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp:46 > +static const unsigned checkerColor1 = 0xff555555; > +static const unsigned checkerColor2 = 0xffaaaaaa; My issue with this is that it's really hard to tell just by looking at this lines what the colors are. I think they should be renamed to reflect what colors they are. > Source/WebCore/platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp:68 > + for (unsigned y = 0; y < checkerSize; y += checkerSize / 2) { > + bool alternate = y % checkerSize; > + for (unsigned x = 0; x < checkerSize; x += checkerSize / 2) { > + if (alternate) > + setSourceRGBAFromColor(cr.get(), checkerColor1); > + else > + setSourceRGBAFromColor(cr.get(), checkerColor2); > + cairo_rectangle(cr.get(), x, y, checkerSize / 2, checkerSize / 2); > + cairo_fill(cr.get()); > + alternate = !alternate; > + } > + } I think it will be much more readable to unroll this loop: unsigned halfCheckerSize = checkerSize / 2; cairo_rectangle(cr.get(), 0, 0, halfCheckerSize, halfCheckerSize); cairo_rectangle(cr.get(), halfCheckerSize, halfCheckerSize, halfCheckerSize, halfCheckerSize); setSourceRGBAFromColor(cr.get(), checkerColor1); cairo_fill(cr.get()); cairo_rectangle(cr.get(), halfCheckerSize, 0, halfCheckerSize, halfCheckerSize); cairo_rectangle(cr.get(), 0, halfCheckerSize, halfCheckerSize, halfCheckerSize); setSourceRGBAFromColor(cr.get(), checkerColor2); cairo_fill(cr.get()); It's even fewer lines!

Comment on attachment 113630 [details] Updated View in context: https://bugs.webkit.org/attachment.cgi?id=113630&action=review >> Source/WebCore/platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp:46 >> +static const unsigned checkerColor2 = 0xffaaaaaa; > > My issue with this is that it's really hard to tell just by looking at this lines what the colors are. I think they should be renamed to reflect what colors they are. How about using checkerColorDarkGrey(0xff555555) and checkerColorLightGrey(0xffaaaaaa)?

(In reply to comment #8) > How about using checkerColorDarkGrey(0xff555555) and checkerColorLightGrey(0xffaaaaaa)? That would definitely be an improvement.

Created attachment 114477 [details] Updated according to suggestions

(In reply to comment #7) > (From update of attachment 113630 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113630&action=review > > This looks pretty close, but I have a few more comments. Sorry for the delay in reviewing. I realy thank you for your review. After your and Joone Hur's comments the code looks much better and it has chance to be commited. > Doesn't it make sense for this file to have the same license as TileCairo.cpp? The license issue was due to that I tried to save Joone Hur's license in code that was orginaly written by him. Yes, it was poor solution. I have unified the licens to BSD for all files. I have uploaded a patch that contains all your suggestions.

Hello Joone Hur, I have changed slightly a license that you have used in your patch. It is still BSD license but I have changed: THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS insted of THIS SOFTWARE IS PROVIDED BY THE AUTHOR and IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS insted of IN NO EVENT SHALL THE AUTHOR Do you agree for this?

Comment on attachment 114477 [details] Updated according to suggestions Thanks. This looks fine to me. :)

(In reply to comment #12) > Hello Joone Hur, > I have changed slightly a license that you have used in your patch. It is still BSD license but I have changed: > > THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS insted of > THIS SOFTWARE IS PROVIDED BY THE AUTHOR > > and > IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS insted of > IN NO EVENT SHALL THE AUTHOR > > Do you agree for this? This license was written by Julien or Zaheer. Anyway, thank you for your contribution. :-)

Comment on attachment 114477 [details] Updated according to suggestions Clearing flags on attachment: 114477 Committed r100021: <http://trac.webkit.org/changeset/100021>

All reviewed patches have been landed. Closing bug.