Bug 85588

Summary: [EFL] Refactor ewk_view_context_paint code.
Product: WebKit Reporter: Tomasz Morawski <t.morawski>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: antognolli+webkit, g.czajkowski, gyuyoung.kim, leandro, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
ewk_view_context_paint related changes
none
ewk_view_context_paint related changes 2
none
ewk_view_context_paint related changes 3
none
ewk_view_context_paint related changes 4 none

Description Tomasz Morawski 2012-05-04 01:00:36 PDT
The main purpose of this change is reduce size of ewk_view.h/cpp files and make use of 
ewk_view_context_paint object by ewk_view_single and ewk_view_tiled instead of direct 
operations on cairo. The code is more objective and looks very simple now. Remove 
ewk_view_context_paint code from ewk_view h/cpp file and move it to separate new 
ewk_context_paint file. The ewk_view_context_paint has been renamed to 
ewk_context_paint and it is not binded to view on creation time.
Comment 1 Tomasz Morawski 2012-05-04 01:07:18 PDT
Created attachment 140176 [details]
ewk_view_context_paint related changes
Comment 2 WebKit Review Bot 2012-05-04 01:10:09 PDT
Attachment 140176 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit/ChangeLog', u'..." exit_code: 1
Source/WebKit/efl/ewk/ewk_paint_context_private.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/efl/ewk/ewk_paint_context.cpp:23:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/efl/ewk/ewk_paint_context.cpp:47:  Declaration has space between type name and * in Ewk_Paint_Context *context  [whitespace/declaration] [3]
Source/WebKit/efl/ewk/ewk_paint_context.cpp:171:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Tomasz Morawski 2012-05-04 01:21:20 PDT
Created attachment 140178 [details]
ewk_view_context_paint related changes 2
Comment 4 Grzegorz Czajkowski 2012-05-07 03:45:29 PDT
Generally I'm fine with those changes. We should be aware that this patch introduces a new private file for paint_context component.

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

> Source/WebKit/efl/ewk/ewk_paint_context.cpp:30
> +    Ewk_Paint_Context* context = new Ewk_Paint_Context;

What about using smart pointers here?
OwnPtr<Ewk_Paint_Context> context = adoptPtr(new Ewk_Paint_Context)

> Source/WebKit/efl/ewk/ewk_paint_context.cpp:103
> +    if (context->image && context->pixels)

Braces should be added here:
if (condition) {
    // Some comment
    doIt();
}

> Source/WebKit/efl/ewk/ewk_paint_context.cpp:153
> +    WebCore::IntRect rect(*area);

Can you use a more descriptive variable for example, areaToPaint?

> Source/WebKit/efl/ewk/ewk_paint_context.cpp:166
> +    WebCore::IntRect rect(*area);

Ditto.

> Source/WebKit/efl/ewk/ewk_paint_context_private.h:23
> + * @brief   Describes the paint context API.

I wouldn't rather mention about API here. It's an internal file that won't be exported to an application layer. Please use different name: functions, internals etc.

> Source/WebKit/efl/ewk/ewk_paint_context_private.h:41
> + * @internal

In my opinion all definitions in this file should have @internal tag in first line of documentation.

> Source/WebKit/efl/ewk/ewk_paint_context_private.h:67
> +EAPI Ewk_Paint_Context* ewk_paint_context_new(cairo_t* cairo);

EAPI doesn't looks well in private files. We need to find better way before landing this patch.

> Source/WebKit/efl/ewk/ewk_paint_context_private.h:72
> + * @param image to use as paint destination

Please mention that it must not be 0.

> Source/WebKit/efl/ewk/ewk_paint_context_private.h:85
> + * @param pixel pointer to pixel buffer

Ditto.

> Source/WebKit/efl/ewk/ewk_private.h:180
> +EAPI Eina_Bool ewk_view_paint(Ewk_View_Private_Data* priv, Ewk_Paint_Context* context, const Eina_Rectangle* area);

I understand that those functions are used by DumpRenderTree. Is there any better way to use them internally and remove EAPI prefix?
Comment 5 Tomasz Morawski 2012-05-07 05:01:53 PDT
(In reply to comment #4)
> Generally I'm fine with those changes. We should be aware that this patch introduces a new private file for paint_context component.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=140178&action=review
> 
> > Source/WebKit/efl/ewk/ewk_paint_context.cpp:30
> > +    Ewk_Paint_Context* context = new Ewk_Paint_Context;
> 
> What about using smart pointers here?
> OwnPtr<Ewk_Paint_Context> context = adoptPtr(new Ewk_Paint_Context)
> 
> > Source/WebKit/efl/ewk/ewk_paint_context.cpp:103
> > +    if (context->image && context->pixels)
> 
> Braces should be added here:
> if (condition) {
>     // Some comment
>     doIt();
> }
> 
> > Source/WebKit/efl/ewk/ewk_paint_context.cpp:153
> > +    WebCore::IntRect rect(*area);
> 
> Can you use a more descriptive variable for example, areaToPaint?
> 
> > Source/WebKit/efl/ewk/ewk_paint_context.cpp:166
> > +    WebCore::IntRect rect(*area);
> 
> Ditto.
> 
> > Source/WebKit/efl/ewk/ewk_paint_context_private.h:23
> > + * @brief   Describes the paint context API.
> 
> I wouldn't rather mention about API here. It's an internal file that won't be exported to an application layer. Please use different name: functions, internals etc.
> 
> > Source/WebKit/efl/ewk/ewk_paint_context_private.h:41
> > + * @internal
> 
> In my opinion all definitions in this file should have @internal tag in first line of documentation.
> 
> > Source/WebKit/efl/ewk/ewk_paint_context_private.h:67
> > +EAPI Ewk_Paint_Context* ewk_paint_context_new(cairo_t* cairo);
> 
> EAPI doesn't looks well in private files. We need to find better way before landing this patch.
> 
> > Source/WebKit/efl/ewk/ewk_paint_context_private.h:72
> > + * @param image to use as paint destination
> 
> Please mention that it must not be 0.
> 
> > Source/WebKit/efl/ewk/ewk_paint_context_private.h:85
> > + * @param pixel pointer to pixel buffer
> 
> Ditto.
> 
> > Source/WebKit/efl/ewk/ewk_private.h:180
> > +EAPI Eina_Bool ewk_view_paint(Ewk_View_Private_Data* priv, Ewk_Paint_Context* context, const Eina_Rectangle* area);
> 
> I understand that those functions are used by DumpRenderTree. Is there any better way to use them internally and remove EAPI prefix?

Thank you for your review. I will try to remove EAPI prefix from that function.
Comment 6 Tomasz Morawski 2012-05-08 07:00:32 PDT
Created attachment 140711 [details]
ewk_view_context_paint related changes 3
Comment 7 Gyuyoung Kim 2012-05-08 23:25:11 PDT
Comment on attachment 140711 [details]
ewk_view_context_paint related changes 3

Looks fantastic !! Though this patch is a little huge, I think efl port needs to land this patch.
Comment 8 Grzegorz Czajkowski 2012-05-08 23:54:53 PDT
LGTM from my side. CC'ing backing store developers.
Comment 9 Tomasz Morawski 2012-06-01 02:53:34 PDT
Created attachment 145252 [details]
ewk_view_context_paint related changes 4
Comment 10 Tomasz Morawski 2012-06-01 03:02:01 PDT
Patch rebased.
Comment 11 Hajime Morrita 2012-06-04 00:36:00 PDT
Comment on attachment 145252 [details]
ewk_view_context_paint related changes 4

Rubberstamping.
Comment 12 WebKit Review Bot 2012-06-04 01:45:54 PDT
Comment on attachment 145252 [details]
ewk_view_context_paint related changes 4

Clearing flags on attachment: 145252

Committed r119374: <http://trac.webkit.org/changeset/119374>
Comment 13 WebKit Review Bot 2012-06-04 01:46:00 PDT
All reviewed patches have been landed.  Closing bug.