[Webkit-unassigned] [Bug 85588] [EFL] Refactor ewk_view_context_paint code.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 7 05:01:54 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=85588
--- Comment #5 from Tomasz Morawski <t.morawski at samsung.com> 2012-05-07 05:01:53 PST ---
(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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list