RESOLVED FIXED75868
[EFL] Add getter for Ewk_JS_Object::view. 
https://bugs.webkit.org/show_bug.cgi?id=75868
Summary [EFL] Add getter for Ewk_JS_Object::view.
Raphael Kubo da Costa (:rakuco)
Reported 2012-01-09 10:51:17 PST
[EFL] Add getter for Ewk_JS_Object::view.
Attachments
Patch (2.84 KB, patch)
2012-01-09 10:51 PST, Raphael Kubo da Costa (:rakuco) 		no flags
DetailsFormatted DiffDiff
Coding style fixes (2.84 KB, patch)
2012-01-10 05:13 PST, Raphael Kubo da Costa (:rakuco) 		kling: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Raphael Kubo da Costa (:rakuco)
Comment 1 2012-01-09 10:51:47 PST
Created attachment 121687 [details] Patch
Gyuyoung Kim
Comment 2 2012-01-09 19:18:19 PST
Comment on attachment 121687 [details] Patch LGTM.
KwangHyuk
Comment 3 2012-01-10 00:03:58 PST
> Source/WebKit/efl/ewk/ewk_js.cpp:615 > +Evas_Object* ewk_js_object_view_get(const Ewk_JS_Object *jsObject) As far as I know, renewed coding style doesn't like '*' in front of parameter, Would you check the position of '*' in front of jsObject ? > Source/WebKit/efl/ewk/ewk_js.cpp:718 > +Evas_Object* ewk_js_object_view_get(const Ewk_JS_Object *jsObject) Ditto.
Grzegorz Czajkowski
Comment 4 2012-01-10 00:24:52 PST
Wouldn't it be better to move checking NETSCAPE_PLUGIN_API macro to functions definition instead of duplicate them in source depending on macro value? It is known practice in WebKit and EFL port. Reviewing patches like this may confuse reviewer if he sees the same API in the same file whose return value and implementation are different (especially if macro isn't included in patch). What do you think about it?
Raphael Kubo da Costa (:rakuco)
Comment 5 2012-01-10 05:13:15 PST
Created attachment 121830 [details] Coding style fixes
Raphael Kubo da Costa (:rakuco)
Comment 6 2012-01-10 05:14:43 PST
(In reply to comment #4) > Wouldn't it be better to move checking NETSCAPE_PLUGIN_API macro to functions definition instead of duplicate them in source depending on macro value? Yeah, but it makes more sense to do that in a separate patch IMO. Doing so here would make the patch bigger and the function inconsistent with the rest of the file (the other option, fixing everything here, would be even worse).
KwangHyuk
Comment 7 2012-01-10 05:30:12 PST
(In reply to comment #5) > Created an attachment (id=121830) [details] > Coding style fixes LGTM. :)
Grzegorz Czajkowski
Comment 8 2012-01-10 05:33:51 PST
(In reply to comment #6) > (In reply to comment #4) > > Wouldn't it be better to move checking NETSCAPE_PLUGIN_API macro to functions definition instead of duplicate them in source depending on macro value? > > Yeah, but it makes more sense to do that in a separate patch IMO. Doing so here would make the patch bigger and the function inconsistent with the rest of the file (the other option, fixing everything here, would be even worse). LGTM.
Raphael Kubo da Costa (:rakuco)
Comment 9 2012-01-10 05:47:42 PST
Committed r104562: <http://trac.webkit.org/changeset/104562>
Note You need to log in before you can comment on or make changes to this bug.