Summary: | [EFL] Exporting JavaScript objects for EFL port | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonas Gastal <jgastal> | ||||||||||||||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | andersca, antognolli+webkit, bdilly, darin, eric, flavio.ceolin, g.czajkowski, gyuyoung.kim, hausmann, kenneth, leandro, lucas.de.marchi, ryuan.choi, tkent, tonikitoo, webkit.review.bot | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Other | ||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||
Attachments: |
|
Description
Jonas Gastal
2010-08-03 14:38:20 PDT
Created attachment 63381 [details]
Patch
Attachment 63381 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/efl/ewk/ewk_view.h:26: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 63383 [details]
Patch
Attachment 63383 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/efl/EWebLauncher/main.c:86: Extra space before last semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5]
WebKit/efl/EWebLauncher/main.c:94: This { should be at the end of the previous line [whitespace/braces] [4]
WebKit/efl/EWebLauncher/main.c:99: This { should be at the end of the previous line [whitespace/braces] [4]
WebKit/efl/EWebLauncher/main.c:104: This { should be at the end of the previous line [whitespace/braces] [4]
WebKit/efl/EWebLauncher/main.c:112: js_meta is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 5 in 1 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 64592 [details]
Adds support for exporting a JS API to EFL port.
Created attachment 64593 [details]
Example code for injecting JS API.
Attachment 64592 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/efl/ewk/ewk_js.cpp:23: Alphabetical sorting problem. [build/include_order] [4]
WebKit/efl/ewk/ewk_js.cpp:389: Use 0 instead of NULL. [readability/null] [4]
Total errors found: 2 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 64593 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/efl/EWebLauncher/main.c:93: This { should be at the end of the previous line [whitespace/braces] [4]
WebKit/efl/EWebLauncher/main.c:98: This { should be at the end of the previous line [whitespace/braces] [4]
WebKit/efl/EWebLauncher/main.c:103: This { should be at the end of the previous line [whitespace/braces] [4]
WebKit/efl/EWebLauncher/main.c:111: js_meta is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 4 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
As far as I can tell all the style complaints from check-webkit-style are false positives. Comment on attachment 64592 [details] Adds support for exporting a JS API to EFL port. View in context: https://bugs.webkit.org/attachment.cgi?id=64592&action=review > WebKit/efl/ChangeLog:171 > * DefaultTheme/widget/slider/slider_v.png: Copied from WebKit/efl/DefaultTheme/widget/scrollbar/scrollbar_v.png. > > +2010-08-03 Jonas M. Gastal <jgastal@profusion.mobi> > + This should be the first entry of the ChangeLog Created attachment 71165 [details]
Adds support for exporting a JS API to EFL port.
Fixed ChangeLog as request by kenneth.
Created attachment 71166 [details]
Example code for injecting JS API.
Fixed ChangeLog as requested by kenneth.
Could some one please review these patches? Comment on attachment 71165 [details]
Adds support for exporting a JS API to EFL port.
The patch looks not to be applied to the trunk cleanly. Would you update it please?
Comment on attachment 71166 [details]
Example code for injecting JS API.
ditto.
Hi Jonas. When you have time, please upload a new patch. I'm closing this one now, feel free to re-open it later. Thanks Created attachment 97602 [details]
Patch
It's the Jonas Gastal's patch rebased with the upstream and with small fixes to be possible build with NETSCAPE_PLUGIN_API disabled.
Could some one please review this patch ?
Regards, Ceolin.
Attachment 97602 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/CMakeLists.txt', u'Source/We..." exit_code: 1
Source/WebKit/efl/ewk/ewk_js.cpp:28: Alphabetical sorting problem. [build/include_order] [4]
Source/WebKit/efl/ewk/ewk_js.cpp:127: Declaration has space between type name and * in JavaScriptObject *jso [whitespace/declaration] [3]
Source/WebKit/efl/ewk/ewk_js.cpp:404: Use 0 or null instead of NULL (even in *comments*). [readability/null] [4]
Source/WebKit/efl/ewk/ewk_js.cpp:716: Declaration has space between type name and * in Eina_Hash *ewk_js_object_properties_get [whitespace/declaration] [3]
Source/WebKit/efl/ewk/ewk_js.cpp:718: Use 0 instead of NULL. [readability/null] [5]
Source/WebKit/efl/ewk/ewk_js.cpp:723: Use 0 instead of NULL. [readability/null] [5]
Source/WebKit/efl/ewk/ewk_js.cpp:734: Declaration has space between type name and * in Ewk_JS_Object *ewk_js_object_new [whitespace/declaration] [3]
Source/WebKit/efl/ewk/ewk_js.cpp:736: Use 0 instead of NULL. [readability/null] [5]
Source/WebKit/efl/ewk/ewk_private.h:65: One space before end of line comments [whitespace/comments] [5]
Total errors found: 9 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Reopening the bug, as a new patch has been submitted. Created attachment 97605 [details]
Patch
Fixed code style errors.
Regards, Ceolin
It seems to me goal of this bug is for WebOS, right ? If you mention why this patch is needed, it can be helpful to review this bug. (In reply to comment #21) > It seems to me goal of this bug is for WebOS, right ? If you mention why this patch is needed, it can be helpful to review this bug. It is not for WebOS, others ports like qt already has this functionality. The goal of this patch is the ewebkit be able to create rich webwidgets. Regards, Ceolin Comment on attachment 97605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97605&action=review > Source/WebKit/efl/ewk/ewk_js.cpp:21 > + You don't need to add empty line to here. > Source/WebKit/efl/ewk/ewk_js.cpp:93 > + return (Ewk_JS_Object*)np_obj; Please adhere indentation. > Source/WebKit/efl/ewk/ewk_js.cpp:96 > + return 0; ditto. > Source/WebKit/efl/ewk/ewk_js.cpp:714 > +#else Personally, I don't like to make dummy functions for macro. Other ewk functions are using macro its inside for disabling macro. const char *ewk_settings_web_database_path_get(void) { #if ENABLE(DATABASE) return _ewk_default_web_database_path; #else return 0; #endif } > Source/WebKit/efl/ewk/ewk_private.h:1 > + Please remove empty line. Created attachment 98225 [details]
Patch
(In reply to comment #23) > (From update of attachment 97605 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97605&action=review > > > Source/WebKit/efl/ewk/ewk_js.cpp:21 > > + > > You don't need to add empty line to here. Fixed > > > Source/WebKit/efl/ewk/ewk_js.cpp:93 > > + return (Ewk_JS_Object*)np_obj; > Fixed > Please adhere indentation. > > > Source/WebKit/efl/ewk/ewk_js.cpp:96 > > + return 0; > > ditto. > I couldn't see this mistake. The indentation seems to be right. > > Source/WebKit/efl/ewk/ewk_js.cpp:714 > > +#else > > Personally, I don't like to make dummy functions for macro. Other ewk functions are using macro its inside for disabling macro. > > const char *ewk_settings_web_database_path_get(void) > { > #if ENABLE(DATABASE) > return _ewk_default_web_database_path; > #else > return 0; > #endif > } > > > Source/WebKit/efl/ewk/ewk_private.h:1 > > + > > Please remove empty line. Fixed I sent a new patch fixing the mistakes. About the dummy functions, my point is that will be necessary too many #if without them. To my mind the dummy functions is better in this case (although I don't like too). If you still prefer the #if inside the functions I can send a new patch. Thanks for the advices :) Regards, Ceolin Comment on attachment 98225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98225&action=review Informal r-. > Source/WebKit/ChangeLog:8 > +2011-06-16 Jonas M. Gastal <jgastal@profusion.mobi> > + > + Reviewed by NOBODY (OOPS!). > + > + Exporting JavaScript objects for EFL port > + https://bugs.webkit.org/show_bug.cgi?id=43446 > + > + * CMakeLists.txt: Be more clear here: what is being changed in CMakeLists.txt as a whole and what specific change has been performed. > Source/WebKit/efl/ChangeLog:2 > +2011-06-16 Jonas M. Gastal <jgastal@profusion.mobi> > + Reviewed by NOBODY (OOPS!). Usually, there is an empty line after the date/name/mail line. > Source/WebKit/efl/ChangeLog:7 > + Exporting JavaScript objects for EFL port > + https://bugs.webkit.org/show_bug.cgi?id=43446 > + > + * CMakeListsEfl.txt: A paragraph explaining in detail would be nice. > Source/WebKit/efl/ewk/ewk_js.cpp:78 > + /*NPAllocateFunctionPtr*/ 0, > + /*NPDeallocateFunctionPtr*/ 0, > + /*NPInvalidateFunctionPtr*/ 0, > + /*NPHasMethodFunctionPtr*/ _ewk_js_method_has, > + /*NPInvokeFunctionPtr*/ _ewk_js_method_invoke, > + /*NPInvokeDefaultFunctionPtr*/ 0, > + /*NPHasPropertyFunctionPtr*/ _ewk_js_property_has, > + /*NPGetPropertyFunctionPtr*/ _ewk_js_property_get, > + /*NPSetPropertyFunctionPtr*/ _ewk_js_property_set, > + /*NPRemovePropertyFunctionPtr*/ _ewk_js_property_remove, > + /*NPEnumerationFunctionPtr*/ _ewk_js_properties_enumerate, > + /*NPConstructFunctionPtr*/ 0 C++ comments, please. > Source/WebKit/efl/ewk/ewk_js.cpp:92 > + if (EINA_MAGIC_CHECK((Ewk_JS_Object*)np_obj, EWK_JS_OBJECT_MAGIC)) // It already is a Ewk_JS_Object > + return (Ewk_JS_Object*)np_obj; Use C++ casts on .cpp files, please. Also, is that comment really needed? > Source/WebKit/efl/ewk/ewk_js.cpp:97 > + cls = (Ewk_JS_Class*)malloc(sizeof(Ewk_JS_Class)); What happens if malloc() fails? > Source/WebKit/efl/ewk/ewk_js.cpp:120 > + obj = (Ewk_JS_Object*)malloc(sizeof(Ewk_JS_Object)); Ditto. > Source/WebKit/efl/ewk/ewk_js.cpp:130 > + if (!strncmp("Array", jso->imp->className().ascii().data(), strlen("Array"))) > + obj->type = EWK_JS_OBJECT_ARRAY; > + else if (!strncmp("Function", jso->imp->className().ascii().data(), strlen("Function"))) > + obj->type = EWK_JS_OBJECT_FUNCTION; Since you're comparing with string literals, there's no need to use strncmp(), since they're guaranteed to be zero-terminated. > Source/WebKit/efl/ewk/ewk_js.cpp:197 > + STRINGZ_TO_NPVARIANT(strdup(data->value.s), *result); strdup() might return NULL. Will STRINGZ_TO_NPVARIANT be able to handle that? > Source/WebKit/efl/ewk/ewk_js.cpp:203 > + OBJECT_TO_NPVARIANT((NPObject*)data->value.o, *result); C++ casts. > Source/WebKit/efl/ewk/ewk_js.cpp:235 > + data->value.s = (char*)malloc(sizeof(char) * (sz + 1)); malloc() might fail. Please review all malloc() usage. > Source/WebKit/efl/ewk/ewk_js.cpp:264 > + if (!_NPN_IdentifierIsString(name)) { > + ERR("int NPIdentifier is not supported."); > + return fail; Just return false here. > Source/WebKit/efl/ewk/ewk_js.cpp:267 > + if (eina_hash_find(obj->properties, prop_name)) // should search methods too? This comment should include a FIXME: and be proper sentences (begin with uppercase letters, etc). Also, this if can be ditched: char* prop...; bool fail = eina_hash_find(...); free(prop_name); return fail; > Source/WebKit/efl/ewk/ewk_js.cpp:291 > + if (prop && prop->get) { // class has property and property has getter Proper sentences must be used on comments. > Source/WebKit/efl/ewk/ewk_js.cpp:619 > + bool fail = EINA_FALSE; Please do not mix 'bool' and 'Eina_Bool' types. Not harmful, but not pretty either. > Source/WebKit/efl/ewk/ewk_js.cpp:694 > + int i; > + for (i = 0; i < count; i++) { for (int i = 0; ...) > Source/WebKit/efl/ewk/ewk_private.h:59 > + Eina_Hash *properties; // key=name, value=current value of property Is this comment really needed? Created attachment 103098 [details]
Patch
New patch with the suggested fixes.
(In reply to comment #27) > Created an attachment (id=103098) [details] > Patch > > New patch with the suggested fixes. It's a very big patch and hard to review, but I think the major flaws were covered on this last patch. Informal r+. Comment on attachment 103098 [details] Patch Rejecting attachment 103098 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2 Last 500 characters of output: /efl/ewk/ewk_view.cpp Hunk #3 succeeded at 3395 (offset 17 lines). Hunk #4 succeeded at 3556 (offset 19 lines). patching file Source/WebKit/efl/ewk/ewk_view.h Hunk #1 succeeded at 85 (offset 62 lines). Hunk #2 FAILED at 87. Hunk #3 succeeded at 2176 with fuzz 1 (offset 491 lines). 1 out of 3 hunks FAILED -- saving rejects to file Source/WebKit/efl/ewk/ewk_view.h.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Antonio Gomes', u'--fo..." exit_code: 1 Full output: http://queues.webkit.org/results/9462838 Recently there were a lot of patches which move API documentation of WebKit-EFL to the header files. Could you please adjust your patch to these changes and add short description of ewk_js.h (@brief command)? Additionally I think we do not need dots in descriptions of parameters and return value (see EFL's or ewk documetations). Committed r93637: <http://trac.webkit.org/changeset/93637> |