RESOLVED FIXED 85588
[EFL] Refactor ewk_view_context_paint code.
https://bugs.webkit.org/show_bug.cgi?id=85588
Summary [EFL] Refactor ewk_view_context_paint code.
Tomasz Morawski
Reported 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.
Attachments
ewk_view_context_paint related changes (45.97 KB, patch)
2012-05-04 01:07 PDT, Tomasz Morawski
no flags
ewk_view_context_paint related changes 2 (45.94 KB, patch)
2012-05-04 01:21 PDT, Tomasz Morawski
no flags
ewk_view_context_paint related changes 3 (42.22 KB, patch)
2012-05-08 07:00 PDT, Tomasz Morawski
no flags
ewk_view_context_paint related changes 4 (42.21 KB, patch)
2012-06-01 02:53 PDT, Tomasz Morawski
no flags
Tomasz Morawski
Comment 1 2012-05-04 01:07:18 PDT
Created attachment 140176 [details] ewk_view_context_paint related changes
WebKit Review Bot
Comment 2 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.
Tomasz Morawski
Comment 3 2012-05-04 01:21:20 PDT
Created attachment 140178 [details] ewk_view_context_paint related changes 2
Grzegorz Czajkowski
Comment 4 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?
Tomasz Morawski
Comment 5 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.
Tomasz Morawski
Comment 6 2012-05-08 07:00:32 PDT
Created attachment 140711 [details] ewk_view_context_paint related changes 3
Gyuyoung Kim
Comment 7 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.
Grzegorz Czajkowski
Comment 8 2012-05-08 23:54:53 PDT
LGTM from my side. CC'ing backing store developers.
Tomasz Morawski
Comment 9 2012-06-01 02:53:34 PDT
Created attachment 145252 [details] ewk_view_context_paint related changes 4
Tomasz Morawski
Comment 10 2012-06-01 03:02:01 PDT
Patch rebased.
Hajime Morrita
Comment 11 2012-06-04 00:36:00 PDT
Comment on attachment 145252 [details] ewk_view_context_paint related changes 4 Rubberstamping.
WebKit Review Bot
Comment 12 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>
WebKit Review Bot
Comment 13 2012-06-04 01:46:00 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.