WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
ewk_view_context_paint related changes 2
(45.94 KB, patch)
2012-05-04 01:21 PDT
,
Tomasz Morawski
no flags
Details
Formatted Diff
Diff
ewk_view_context_paint related changes 3
(42.22 KB, patch)
2012-05-08 07:00 PDT
,
Tomasz Morawski
no flags
Details
Formatted Diff
Diff
ewk_view_context_paint related changes 4
(42.21 KB, patch)
2012-06-01 02:53 PDT
,
Tomasz Morawski
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug