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

• Component: Platform
• Status: RESOLVED FIXED

[Leandro Pereira]

+++ This bug was initially created as a clone of Bug #35059 +++

The attached patch adds ewk_view.cpp and ewk_view.h to WebKit/efl/ewk.

[Leandro Pereira]

Created attachment 50331 [details]
Add ewk_view to WK/efl/ewk.

[Eric Seidel (no email)]

Comment on attachment 50331 [details]
Add ewk_view to WK/efl/ewk.

I don't understand how anyone is supposed to review this.

I assume EFL/EWK are not supposed to conform to webkit.org style?

[Eric Seidel (no email)]

I don't think any webkit.org reviewer knows EFL/EWK, so the best we could offer is a rubber-stamp.

[Gustavo Sverzut Barbieri]

Hi Eric,

There are lots of EFL specific bits, yes... but I'd like to have your review on couple of critical functions, specially those that have PassPtr and RefPtr in their signature... I did these based on GTK, Qt and couple of others, I guess they are all fine, but extra review would be awesome! :-) These functions should have not much of EFL, it's more about WebKit.

The rest just feel free to ignore if you wish, we wrote it like most of other EFL code, so it's very C-ish and all. But it works like a charm :-)

[Gustavo Noronha (kov)]

Comment on attachment 50331 [details]
Add ewk_view to WK/efl/ewk.

 169 static inline void _ewk_view_smart_changed(Ewk_View_Smart_Data* sd)

There are several inline markers in this code. In WebKit land we're averse to premature, unmeasured optimization. If you have measured that this helps, this should be OK, but if you haven't I'd recommend leaving this up to the compiler.

 478 static inline WTF::PassRefPtr<WebCore::Frame> _ewk_view_core_frame_new(Ewk_View_Smart_Data* sd, Ewk_View_Private_Data* priv, WebCore::HTMLFrameOwnerElement* owner)
[...]
 488     return WebCore::Frame::create(priv->page, owner, flc).get();

This is wrong. Frame::create will adopt the ref a RefPtr is holding, and return a PassRefPtr already. What you're doing here is you are getting the raw pointer, and returning it. When the returned PassRefPtr goes out of scope the frame will likely be deleted.

[Leandro Pereira]

Created attachment 51927 [details]
Add ewk_view to WK/efl/ewk.

Revised patch, with requested changes.

[WebKit Review Bot]

Attachment 51927 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/efl/ewk/ewk_view.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/efl/ewk/ewk_view.cpp:30:  "EditorClientEfl.h" already included at WebKit/efl/ewk/ewk_view.cpp:28  [build/include] [4]
WebKit/efl/ewk/ewk_view.cpp:545:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/efl/ewk/ewk_view.cpp:2269:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/efl/ewk/ewk_view.cpp:2270:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/efl/ewk/ewk_view.cpp:2277:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/efl/ewk/ewk_view.cpp:2984:  Declaration has space between type name and * in Evas_Object *frame  [whitespace/declaration] [3]
WebKit/efl/ewk/ewk_view.cpp:3042:  Declaration has space between type name and * in Evas_Object *frame  [whitespace/declaration] [3]
WebKit/efl/ewk/ewk_view.cpp:3117:  Declaration has space between type name and * in Evas_Object *ewk_view_window_create  [whitespace/declaration] [3]
WebKit/efl/ewk/ewk_view.cpp:3119:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/efl/ewk/ewk_view.cpp:3146:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/efl/ewk/ewk_view.cpp:3428:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKit/efl/ewk/ewk_view.cpp:3431:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/efl/ewk/ewk_view.cpp:3434:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKit/efl/ewk/ewk_view.cpp:3434:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/efl/ewk/ewk_view.cpp:3553:  Should have a space between // and comment  [whitespace/comments] [4]
WebKit/efl/ewk/ewk_view.cpp:3555:  Missing spaces around =  [whitespace/operators] [4]
WebKit/efl/ewk/ewk_view.cpp:3556:  Declaration has space between type name and * in Ewk_Menu_Item *item  [whitespace/declaration] [3]
WebKit/efl/ewk/ewk_view.cpp:3599:  Declaration has space between type name and * in void *itemv  [whitespace/declaration] [3]
WebKit/efl/ewk/ewk_view.cpp:3601:  Declaration has space between type name and * in Ewk_Menu_Item *item  [whitespace/declaration] [3]
WebKit/efl/ewk/ewk_view.cpp:3606:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 21 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Leandro Pereira]

Created attachment 51933 [details]
Add ewk_view to WK/efl/ewk.

[Leandro Pereira]

Created attachment 52073 [details]
Add ewk_view to WK/efl/ewk.

I've overlooked some coding style issues in previous patch. Here's an updated version.

[Kenneth Rohde Christiansen]

Comment on attachment 52073 [details]
Add ewk_view to WK/efl/ewk.

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.

[Gustavo Sverzut Barbieri]

(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!

[WebKit Commit Bot]

Comment on attachment 52073 [details]
Add ewk_view to WK/efl/ewk.

Clearing flags on attachment: 52073

Committed r57212: <http://trac.webkit.org/changeset/57212>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.