[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





--- Comment #11 from Gustavo Sverzut Barbieri <barbieri at profusion.mobi>  2010-04-07 06:21:47 PST ---
(In reply to comment #10)
> (From update of attachment 52073 [details])
> 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.

We considered it as well, but opted to go per-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.

Yes, that will be used by EFL/C applications and thus should follow their
rules.


> > +#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.

In this case this is not possible. We're dealing with Evas_Object and we really
don't know if it is an image, text or anything else other than ewk_view. I'm
currently introducing hierarchy types and thus type-checking in Evas, but this
is new and needs some testing, later on I can move this type check macro to use
the other check instead.


> > +    // 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

/me looks at Rafael Antognolli ;-)


> > +// Default Event Handling //////////////////////////////////////////////
> 
> Whyt the trailing /// ? :-)

it's just to make the "section" easier to spot.


> > +            "\t       button: %s", message, defaultValue, *value, ret?"ok":"cancel");
> 
> Generally we would suggest spacing here "ok" etc

/me looks at Lucas De Marchi and his space saving style. 


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

My initial implementation did that delete, and it segfaulted. Later on I went
to realize the reason, since the main_frame was being used and deleted by view,
thus we should not delete it there. I left the code and the comment to make
clear I or any other developer fall in the same trap again.


> > +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.

For priv->page that could be, for priv itself, it should not as that is stored
directly by evas_object_smart_data_set() and then things would be more
complicated.


> And generally avoid goto.

EFL error checking style.


> > +        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.

Interesting to know about this meta-tag.


> > +/**
> > + * Set a fixed layout size to be used, dissociating it from viewport size.
> > + *
> 
> Oh you already have something like that.

Yes, but it is not a meta tag, rather being set explicitly by browser. The
purpose is to have different zoom levels to scale based on the viewport. So the
zoom level 2.0 would have the viewport 480x800 at 560x1600. This webkit part
does that.

Thanks for your review!

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