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 :-)

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.

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.

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