Bug 93644 - [BlackBerry] Introduce a JavaScript Variant object
Summary: [BlackBerry] Introduce a JavaScript Variant object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-09 13:12 PDT by Benjamin Meyer
Modified: 2012-08-10 13:40 PDT (History)
5 users (show)

See Also:


Attachments
patch (19.03 KB, patch)
2012-08-09 13:19 PDT, Benjamin Meyer
no flags Details | Formatted Diff | Diff
patch (19.01 KB, patch)
2012-08-09 13:34 PDT, Benjamin Meyer
manyoso: review-
Details | Formatted Diff | Diff
patch (19.13 KB, patch)
2012-08-09 14:32 PDT, Benjamin Meyer
tonikitoo: review+
Details | Formatted Diff | Diff
patch (19.16 KB, patch)
2012-08-10 11:35 PDT, Benjamin Meyer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Meyer 2012-08-09 13:12:22 PDT
The JavaScriptVariant can be used for functions that return results from
JavaScript such as executeJavaScript or functions that take arguments
to pass a variable to JavaScript and they want to make sure the type
is correct.
Comment 1 Benjamin Meyer 2012-08-09 13:19:23 PDT
Created attachment 157524 [details]
patch
Comment 2 WebKit Review Bot 2012-08-09 13:22:08 PDT
Attachment 157524 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/ChangeLog', u'Source/WebKit/..." exit_code: 1
Source/WebKit/blackberry/Api/WebPage.h:24:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Benjamin Meyer 2012-08-09 13:34:47 PDT
Created attachment 157529 [details]
patch

Fixed style error
Comment 4 Adam Treat 2012-08-09 14:04:01 PDT
Comment on attachment 157529 [details]
patch

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

I think this looks good overall.  Please fix the issues and then I'll look one last time.

> Source/WebKit/blackberry/Api/JavaScriptVariant.cpp:87
> +        // Output warning?

ASSERT_NOT_REACHED() I would say.

> Source/WebKit/blackberry/Api/JavaScriptVariant.h:29
> +

Remove the extra new line here.
Comment 5 Benjamin Meyer 2012-08-09 14:32:31 PDT
Created attachment 157551 [details]
patch

Corrections from Adam's review
Comment 6 Antonio Gomes 2012-08-10 10:56:59 PDT
Comment on attachment 157551 [details]
patch

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

> Source/WebKit/blackberry/Api/JavaScriptVariant.h:31
> +class BLACKBERRY_EXPORT JavaScriptVariant {
> +
> +public:

nit: extra blank line.

> Source/WebKit/blackberry/ChangeLog:16
> +        Reviewed by Joe Mason

Should be "Internally reviewed by Joe"
Comment 7 Benjamin Meyer 2012-08-10 11:35:53 PDT
Created attachment 157771 [details]
patch

Changes from Antonio Gomes comments, (remove blank link and change review by link in the changelogs and commit message)
Comment 8 WebKit Review Bot 2012-08-10 13:40:28 PDT
Comment on attachment 157771 [details]
patch

Clearing flags on attachment: 157771

Committed r125324: <http://trac.webkit.org/changeset/125324>
Comment 9 WebKit Review Bot 2012-08-10 13:40:34 PDT
All reviewed patches have been landed.  Closing bug.