Bug 75868 - [EFL] Add getter for Ewk_JS_Object::view.
Summary: [EFL] Add getter for Ewk_JS_Object::view.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raphael Kubo da Costa (:rakuco)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-09 10:51 PST by Raphael Kubo da Costa (:rakuco)
Modified: 2012-01-10 05:47 PST (History)
7 users (show)

See Also:

Attachments
Patch (2.84 KB, patch)
2012-01-09 10:51 PST, Raphael Kubo da Costa (:rakuco) 		no flags Details | Formatted Diff | Diff
Coding style fixes (2.84 KB, patch)
2012-01-10 05:13 PST, Raphael Kubo da Costa (:rakuco) 		kling: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Raphael Kubo da Costa (:rakuco) 2012-01-09 10:51:17 PST 
[EFL] Add getter for Ewk_JS_Object::view.
Comment 1 Raphael Kubo da Costa (:rakuco) 2012-01-09 10:51:47 PST 
Created attachment 121687 [details]
Patch
Comment 2 Gyuyoung Kim 2012-01-09 19:18:19 PST 
Comment on attachment 121687 [details]
Patch

LGTM.
Comment 3 KwangHyuk 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.
Comment 4 Grzegorz Czajkowski 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?
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-01-10 05:13:15 PST 
Created attachment 121830 [details]
Coding style fixes
Comment 6 Raphael Kubo da Costa (:rakuco) 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).
Comment 7 KwangHyuk 2012-01-10 05:30:12 PST 
(In reply to comment #5)
> Created an attachment (id=121830) [details]
> Coding style fixes

LGTM. :)
Comment 8 Grzegorz Czajkowski 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.
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-01-10 05:47:42 PST 
Committed r104562: <http://trac.webkit.org/changeset/104562>