Bug 62444

Created attachment 96718 [details] Patch

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) ?

(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 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 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?)

Created attachment 97229 [details] Patch

(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.

(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 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?

(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.

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 on attachment 115519 [details] Patch for BackingStoreCairo. Clearing flags on attachment: 115519 Committed r100583: <http://trac.webkit.org/changeset/100583>

All reviewed patches have been landed. Closing bug.