Bug 71350 - Added TileCairo and TiledBackingStoreBackendCairo files
Summary: Added TileCairo and TiledBackingStoreBackendCairo files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 71352
  Show dependency treegraph
 
Reported: 2011-11-02 00:54 PDT by Tomasz Morawski
Modified: 2011-11-11 14:18 PST (History)
6 users (show)

See Also:


Attachments
Initial implementation (14.63 KB, patch)
2011-11-02 00:59 PDT, Tomasz Morawski
mrobinson: review-
Details | Formatted Diff | Diff
Updated (14.60 KB, patch)
2011-11-04 00:51 PDT, Tomasz Morawski
mrobinson: review-
Details | Formatted Diff | Diff
Updated according to suggestions (15.69 KB, patch)
2011-11-10 05:27 PST, Tomasz Morawski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomasz Morawski 2011-11-02 00:54:37 PDT
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>
Comment 1 Tomasz Morawski 2011-11-02 00:59:52 PDT
Created attachment 113287 [details]
Initial implementation
Comment 2 Simon Hausmann 2011-11-02 01:15:13 PDT
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.
Comment 3 Tomasz Morawski 2011-11-02 01:42:21 PDT
(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 4 Martin Robinson 2011-11-02 09:45:57 PDT
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!
Comment 5 Tomasz Morawski 2011-11-03 01:50:33 PDT
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.
Comment 6 Tomasz Morawski 2011-11-04 00:51:55 PDT
Created attachment 113630 [details]
Updated

Updated according to Martin Robinson's suggestions.
Comment 7 Martin Robinson 2011-11-09 14:08:37 PST
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 8 Joone Hur 2011-11-09 19:21:10 PST
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)?
Comment 9 Martin Robinson 2011-11-09 19:27:19 PST
(In reply to comment #8)

> How about using checkerColorDarkGrey(0xff555555) and checkerColorLightGrey(0xffaaaaaa)?

That would definitely be an improvement.
Comment 10 Tomasz Morawski 2011-11-10 05:27:07 PST
Created attachment 114477 [details]
Updated according to suggestions
Comment 11 Tomasz Morawski 2011-11-10 05:51:47 PST
(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.
Comment 12 Tomasz Morawski 2011-11-10 05:58:01 PST
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 13 Martin Robinson 2011-11-10 08:25:25 PST
Comment on attachment 114477 [details]
Updated according to suggestions 

Thanks. This looks fine to me. :)
Comment 14 Joone Hur 2011-11-10 20:50:26 PST
(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 15 WebKit Review Bot 2011-11-11 14:18:44 PST
Comment on attachment 114477 [details]
Updated according to suggestions 

Clearing flags on attachment: 114477

Committed r100021: <http://trac.webkit.org/changeset/100021>
Comment 16 WebKit Review Bot 2011-11-11 14:18:51 PST
All reviewed patches have been landed.  Closing bug.