Bug 65972 – [EFL] update ewk_frame_script_execute to return the result for JavaScript

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Jongseok Yang]

A developer using webkit may want to get the result of javascript.
The ewk_frame_script_execute  cannot satisfy the requirement because it returns just TRUE/FALSE.
The function should return the result of javascript.

[Jongseok Yang]

Created attachment 103463 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/efl/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


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

[Jongseok Yang]

Created attachment 103464 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 103464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103464&action=review

Does return value have success or fail message? Could you explain what can application does with the return value ?

> Source/WebKit/efl/ewk/ewk_frame.cpp:426
> +char *ewk_frame_script_execute(Evas_Object* o, const char* script)

Move '*' operator to parameter side.

> Source/WebKit/efl/ewk/ewk_frame.cpp:441
> +    JSC::JSLock lock(JSC::SilenceAssertionsOnly);

You need to use #if USE(JSC) preprocessor. We may use V8 as JavaScriptEngine in future.

[Jongseok Yang]

(In reply to comment #4)
> (From update of attachment 103464 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103464&action=review
> 
> Does return value have success or fail message? Could you explain what can application does with the return value ?

I doest not think that it have success or fail message. Because it returns NULL pointer in specific case like the below code.
=====================================================================================
    if (!sd->frame) // In case the script removed our frame from the page.
        return 0;

    if (!result || (!result.isBoolean() && !result.isString() && !result.isNumber()))
        return 0;
=====================================================================================

To identify return value, it's helpfull to check "toString" function.

<Source/JavaScriptCore/runtime/JSString.h>=========================

inline UString JSValue::toString(ExecState* exec) const
{
    if (isString())
        return static_cast<JSString*>(asCell())->value(exec);
    if (isInt32())
        return exec->globalData().numericStrings.add(asInt32());
    if (isDouble())
        return exec->globalData().numericStrings.add(asDouble());
    if (isTrue())
        return "true";
    if (isFalse())
        return "false";
    if (isNull())
        return "null";
    if (isUndefined())
        return "undefined";
    ASSERT(isCell());
    return asCell()->toString(exec);
}
===================================================================

From "toString" function, you may require to change the code like the below
=====================================================================================
<before>
    if (!result || (!result.isBoolean() && !result.isString() && !result.isNumber()))
        return 0;
<after>
    if (!result)
        return 0;
=====================================================================================

I'll wait for your proposal.

> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:426
> > +char *ewk_frame_script_execute(Evas_Object* o, const char* script)
> 
> Move '*' operator to parameter side.

I'll fix that.
> 
> > Source/WebKit/efl/ewk/ewk_frame.cpp:441
> > +    JSC::JSLock lock(JSC::SilenceAssertionsOnly);
> 
> You need to use #if USE(JSC) preprocessor. We may use V8 as JavaScriptEngine in future.

I'll fix that.

[Jongseok Yang]

Created attachment 103591 [details]
Patch

[Grzegorz Czajkowski]

> Source/WebKit/efl/ewk/ewk_frame.h:325
> + * @return newly allocated string for result or @c NULL if the result cannot be converted to string or failure

Generally we don't use NULL in documentation too. I'd better use @c 0.
Additionally if the method allocates memory, it will be nice to mention that caller should free the string. See other documentation ewk_frame for details.

[Jongseok Yang]

Created attachment 103592 [details]
Patch

[Jongseok Yang]

(In reply to comment #7)
> > Source/WebKit/efl/ewk/ewk_frame.h:325
> > + * @return newly allocated string for result or @c NULL if the result cannot be converted to string or failure
> 
> Generally we don't use NULL in documentation too. I'd better use @c 0.
> Additionally if the method allocates memory, it will be nice to mention that caller should free the string. See other documentation ewk_frame for details.

I accepted your comment.
Please check new patch.

[Raphael Kubo da Costa (:rakuco)]

Looks OK to me.

[Ryuan Choi]

Comment on attachment 103592 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103592&action=review

> Source/WebKit/efl/ewk/ewk_frame.cpp:429
> +    EWK_FRAME_SD_GET_OR_RETURN(o, sd, 0);
> +    EINA_SAFETY_ON_FALSE_RETURN_VAL(sd->frame, 0);

EINA_SAFETY_ON_NULL_RETURN_VAL(sd->frame, 0);

And this patch is out-dated.
please rebased this.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 103592 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103592&action=review

> Source/WebKit/efl/ewk/ewk_frame.cpp:445
> +#elif USE(V8)

BTW, I'd change this to an #else

[Jongseok Yang]

Created attachment 114450 [details]
patch

I updated the patch using re-based webkit source.

[Raphael Kubo da Costa (:rakuco)]

JSC code is not something I'm very familiar with, so from an uninformed point of view the patch LGTM.

[Ryuan Choi]

Looks good to me too.

[WebKit Review Bot]

Comment on attachment 114450 [details]
patch

Clearing flags on attachment: 114450

Committed r100204: <http://trac.webkit.org/changeset/100204>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.