[Webkit-unassigned] [Bug 35932] [EFL] Add ewk_view.{cpp, h} to WK/efl/ewk.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=35932


Kenneth Rohde Christiansen <kenneth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #52073|review?                     |review+
               Flag|                            |




--- Comment #10 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2010-04-07 06:10:32 PST ---
(From update of attachment 52073)
Wow, bug patch :-) Generally it looks good. There might be some issues, but
nothing that should block this from the initial release.

You seem to have some settings related things in the view, such as
ewk_view_exceeded_database_quota. I would suggest making a dedicated settings
system. In Qt we have one that is global, but can be specialized per page/view.

 249     const Ewk_View_Smart_Class *api; /**< reference to casted class
instance */
 250     Evas_Object *self; /**< reference to owner object */
 251     Evas_Object *main_frame; /**< reference to main frame object */
 252     Evas_Object *backing_store; /**< reference to backing store */

^ coding style, but I'm ok with that as it is in your public header.

> +#define EWK_VIEW_TYPE_CHECK(o, ...)                                     \
> +    do {                                                                \
> +        const char* _tmp_otype = evas_object_type_get(o);               \
> +        const Evas_Smart* _tmp_s = evas_object_smart_smart_get(o);      \
> +        if (EINA_UNLIKELY(!_tmp_s)) {                                   \
> +            EINA_LOG_CRIT                                               \
> +                ("%p (%s) is not a smart object!", o,                   \
> +                 _tmp_otype ? _tmp_otype : "(null)");                   \
> +            return __VA_ARGS__;                                         \
> +        }                                                               \
> +        const Evas_Smart_Class* _tmp_sc = evas_smart_class_get(_tmp_s); \
> +        if (EINA_UNLIKELY(!_tmp_sc)) {                                  \
> +            EINA_LOG_CRIT                                               \
> +                ("%p (%s) is not a smart object!", o,                   \
> +                 _tmp_otype ? _tmp_otype : "(null)");                   \
> +            return __VA_ARGS__;                                         \
> +        }                                                               \
> +        if (EINA_UNLIKELY(_tmp_sc->data != EWK_VIEW_TYPE_STR)) {        \
> +            EINA_LOG_CRIT                                               \
> +                ("%p (%s) is not of an ewk_view (need %p, got %p)!",    \
> +                 o, _tmp_otype ? _tmp_otype : "(null)",                 \
> +                 EWK_VIEW_TYPE_STR, _tmp_sc->data);                     \
> +            return __VA_ARGS__;                                         \
> +        }                                                               \
> +    } while (0)
> +#endif

As this is C++ you could use templates for this instead, but that is up to you.

> +    // fprintf(stderr, ">>> repaint requested: %d,%d+%dx%d\n", x, y, w, h);

Generally, we don't leave out commented code, though you will find some in
current webkit code


> +// Default Event Handling //////////////////////////////////////////////

Whyt the trailing /// ? :-)

> +            "\t       button: %s", message, defaultValue, *value, ret?"ok":"cancel");

Generally we would suggest spacing here "ok" etc

> +error_history:
> +    // delete priv->main_frame; /* do not delete priv->main_frame */

??

> +error_main_frame:
> +error_settings:
> +    delete priv->page;
> +error_page:
> +    free(priv);
> +    return 0;
> +}

It might make sense for you to use some of our smart pointers for controlling
deletion, such as OwnPtr.

And generally avoid goto.

> +        if (view) {
> +            view->resize(w, h);
> +            view->forceLayout();
> +            view->adjustViewSize();
> +            IntSize size = view->contentsSize();

Maybe you should refactor some of this out in a viewport_size_set method. You
might not always want to layout using the size of the view, at least not if you
want to support something like the viewport meta tag known from the iPhone.

> +/**
> + * Set a fixed layout size to be used, dissociating it from viewport size.
> + *

Oh you already have something like that.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.