Bug 62444

Summary: [WK2] Move gtk/BackingStoreGtk.cpp to cairo/BackingStoreCairo.cpp to share with EFL port.
Product: WebKit Reporter: Eunmi Lee <enmi.lee>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: antognolli+webkit, bfulgham, gyuyoung.kim, joone.hur, kenneth, leandro, mrobinson, ryuan.choi, sangseok.lim, tonikitoo, webkit.review.bot, youngtaeck.song
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61838    
Attachments:
Description Flags
Patch
none
Patch
leandro: review-, leandro: commit-queue-
Patch for BackingStoreCairo. none

Description Eunmi Lee 2011-06-10 02:34:36 PDT
I added BackingStoreEfl for WebKit2 EFL port.
Comment 1 Eunmi Lee 2011-06-10 02:36:47 PDT
Created attachment 96718 [details]
Patch
Comment 2 Ryuan Choi 2011-06-10 22:03:42 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=96718&action=review

> Source/WebKit2/UIProcess/BackingStore.h:78
> -#elif PLATFORM(GTK)
> +#elif PLATFORM(GTK) || PLATFORM(EFL)
>      typedef cairo_t* PlatformGraphicsContext;
>  #endif

Hello, Martin.
Could you give your opinion about changing it to #elif USE(CAIRO) ?
Comment 3 Martin Robinson 2011-06-12 15:55:48 PDT
(In reply to comment #2)
> View in context: https://bugs.webkit.org/attachment.cgi?id=96718&action=review
> 
> > Source/WebKit2/UIProcess/BackingStore.h:78
> > -#elif PLATFORM(GTK)
> > +#elif PLATFORM(GTK) || PLATFORM(EFL)
> >      typedef cairo_t* PlatformGraphicsContext;
> >  #endif
> 
> Hello, Martin.
> Could you give your opinion about changing it to #elif USE(CAIRO) ?

I don't think that's the right thing to do for WinCairo. CCing Brent Fulgham who probably knows better than I do.
Comment 4 Gyuyoung Kim 2011-06-13 20:09:06 PDT
Comment on attachment 96718 [details]
Patch

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

> Source/WebKit2/ChangeLog:7
> +

Missing summary for this patch.
Comment 5 Brent Fulgham 2011-06-13 21:43:33 PDT
Comment on attachment 96718 [details]
Patch

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

There is a lot of Cairo functionality here that is improperly limited to EFL.  Can this patch be combined with some of our GTK/WinCairo stuff for better reuse?

> Source/WebKit2/UIProcess/BackingStore.h:44
> +#if PLATFORM(GTK) || PLATFORM(EFL)

This needs to be based on the Cairo feature, not GTK/EFL.  A patch to this effect was already submitted IIRC by mrobinson, but I'm not sure the state.

> Source/WebKit2/UIProcess/BackingStore.h:76
> +#elif PLATFORM(GTK) || PLATFORM(EFL)

Ditto:  This is PLATFORM(CAIRO) (or is it FEATURE(CAIRO) now?)
Comment 6 Eunmi Lee 2011-06-14 21:56:07 PDT
Created attachment 97229 [details]
Patch
Comment 7 Eunmi Lee 2011-06-14 22:04:38 PDT
(In reply to comment #5)
> (From update of attachment 96718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96718&action=review
> 
> There is a lot of Cairo functionality here that is improperly limited to EFL.  Can this patch be combined with some of our GTK/WinCairo stuff for better reuse?
> 
I think that EFL's BackingStore and GTK's one can be combined if the EFL's BackingStore can use GtkWidgetBackingStoreCairo.
In order to do that, we have to change the GtkWidgetBackingStore to common one firstly.
I will investigate GtkWidgetBackingStore and try to make patch for that. 

> > Source/WebKit2/UIProcess/BackingStore.h:44
> > +#if PLATFORM(GTK) || PLATFORM(EFL)
> 
> This needs to be based on the Cairo feature, not GTK/EFL.  A patch to this effect was already submitted IIRC by mrobinson, but I'm not sure the state.
> 

Done. I've change that to USE(CAIRO)

> > Source/WebKit2/UIProcess/BackingStore.h:76
> > +#elif PLATFORM(GTK) || PLATFORM(EFL)
> 
> Ditto:  This is PLATFORM(CAIRO) (or is it FEATURE(CAIRO) now?)

Done. Ditto.
Comment 8 Eunmi Lee 2011-06-14 22:06:18 PDT
(In reply to comment #4)
> (From update of attachment 96718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96718&action=review
> 
> > Source/WebKit2/ChangeLog:7
> > +
> 
> Missing summary for this patch.

Done. I've added summary.
Comment 9 Leandro Pereira 2011-06-15 09:07:55 PDT
Comment on attachment 97229 [details]
Patch

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

Informal r-.

> Source/WebKit2/UIProcess/efl/BackingStoreEfl.cpp:40
> +void BackingStore::paint(cairo_t* context, const IntRect& rect)

Can't this be shared with other Cairo-using ports?

> Source/WebKit2/UIProcess/efl/BackingStoreEfl.cpp:48
> +void BackingStore::incorporateUpdate(ShareableBitmap* bitmap, const UpdateInfo& updateInfo)

If the platform-independent part of this function is also similar to other Cairo-using ports, consider splitting this method into two, and doing something along the lines of:

   if (!m_backingStoreSurface)
      platformCreateBackingStoreSurface();

   scroll(...); ...

I'd talk with WinCairo and GTK+ devs before attempting this, though.

> Source/WebKit2/UIProcess/efl/BackingStoreEfl.cpp:77
> +void BackingStore::scroll(const IntRect& scrollRect, const IntSize& scrollOffset)

This method could also be shared with GTK+ or other Cairo-using ports: the only part here that depends on Evas is obtaining the dimentions of the view object, so perhaps a little refactoring would make this platform-independent?
Comment 10 Eunmi Lee 2011-06-16 01:39:53 PDT
(In reply to comment #9)
> (From update of attachment 97229 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97229&action=review
> 
> Informal r-.
> 
> > Source/WebKit2/UIProcess/efl/BackingStoreEfl.cpp:40
> > +void BackingStore::paint(cairo_t* context, const IntRect& rect)
> 
> Can't this be shared with other Cairo-using ports?
> 
> > Source/WebKit2/UIProcess/efl/BackingStoreEfl.cpp:48
> > +void BackingStore::incorporateUpdate(ShareableBitmap* bitmap, const UpdateInfo& updateInfo)
> 
> If the platform-independent part of this function is also similar to other Cairo-using ports, consider splitting this method into two, and doing something along the lines of:
> 
>    if (!m_backingStoreSurface)
>       platformCreateBackingStoreSurface();
> 
>    scroll(...); ...
> 
> I'd talk with WinCairo and GTK+ devs before attempting this, though.
> 
> > Source/WebKit2/UIProcess/efl/BackingStoreEfl.cpp:77
> > +void BackingStore::scroll(const IntRect& scrollRect, const IntSize& scrollOffset)
> 
> This method could also be shared with GTK+ or other Cairo-using ports: the only part here that depends on Evas is obtaining the dimentions of the view object, so perhaps a little refactoring would make this platform-independent?

I think it is not a little refactoring to make platform-independent.
Because, 
We use the cairo_surface directly from evas_object_image, 
but GTK+ uses the WebCore::GtkWidgetBackingStore and cairo operations are delegated to that class.
The functions in the BackingStoreEfl.cpp and BackingStoreGtk seem similar, but they have different implementation.
Comment 11 Eunmi Lee 2011-11-16 21:40:26 PST
Created attachment 115519 [details]
Patch for BackingStoreCairo.

I've renamed gtk/BackingStoreGtk.cpp to cairo/BackingStoreCairo.cpp to use in the EFL port instead of adding BackingStoreEfl.cpp.
Comment 12 WebKit Review Bot 2011-11-17 01:28:02 PST
Comment on attachment 115519 [details]
Patch for BackingStoreCairo.

Clearing flags on attachment: 115519

Committed r100583: <http://trac.webkit.org/changeset/100583>
Comment 13 WebKit Review Bot 2011-11-17 01:28:07 PST
All reviewed patches have been landed.  Closing bug.