Bug 84982 – [BlackBerry] run-time websocket availability check was lost

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

[Joe Mason]

The patch to check if websocket is available at runtime was lost when JSDOMWindow::webSocket moved to JSDOMWindowWebSocketCustom.cpp.

[Joe Mason]

Created attachment 139213 [details]
patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/blackberry/http/tests..." exit_code: 1
Source/WebCore/bindings/js/JSDOMWindowWebSocketCustom.cpp:32:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 6 files


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

[Joe Mason]

Comment on attachment 139213 [details]
patch

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

>> Source/WebCore/bindings/js/JSDOMWindowWebSocketCustom.cpp:32
>> +#include "WebSocket.h"
> 
> Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

This is a false positive.  Filed bug 85075.  For now this will have to be committed by hand.

[WebKit Review Bot]

Comment on attachment 139213 [details]
patch

Rejecting attachment 139213 [details] from commit-queue.

New failing tests:
platform/blackberry/http/tests/websocket/tests/enable-disable-setting.html
Full output: http://queues.webkit.org/results/12549321

[WebKit Review Bot]

Created attachment 139263 [details]
Archive of layout-test-results from ec2-cq-02

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 139213 [details]
patch

Rejecting attachment 139213 [details] from commit-queue.

New failing tests:
platform/blackberry/http/tests/websocket/tests/enable-disable-setting.html
Full output: http://queues.webkit.org/results/12555379

[WebKit Review Bot]

Created attachment 139298 [details]
Archive of layout-test-results from ec2-cq-01

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Antonio Gomes]

Is chromium trying to run a layout test added to layouttests/platform/blackberry?

[WebKit Review Bot]

Comment on attachment 139213 [details]
patch

Attachment 139213 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12552470

New failing tests:
platform/blackberry/http/tests/websocket/tests/enable-disable-setting.html

[WebKit Review Bot]

Created attachment 139331 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

Created attachment 168422 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/bindings/js/JSDOMWindowWebSocketCustom.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 7 files


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

[Rob Buis]

Created attachment 168427 [details]
Patch

[Yong Li]

Comment on attachment 168427 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowWebSocketCustom.cpp:57
>  JSValue JSDOMWindow::webSocket(ExecState* exec) const
>  {
>      if (!settingsForWindowWebSocket(this))
>          return jsUndefined();
> +#if PLATFORM(BLACKBERRY)
> +    // FIXME: returning undefined is not the same things as disabling the
> +    // WebSocket object completely. But, this is the best we can do without
> +    // changing the code generation to check RuntimeEnabledFeatures, like V8
> +    // does, which is too big a job at the moment. Remove this check when
> +    // https://bugs.webkit.org/show_bug.cgi?id=52011 is fixed.
> +    if (!WebSocket::isAvailable())
> +        return jsUndefined();
> +#endif
>      return getDOMConstructor<JSWebSocketConstructor>(exec, this);
>  }

I don't like this #if PLATFORM(). Does returning jsUndefined when !WebSocket::isAvailable() also make sense to other ports?

[Rob Buis]

Created attachment 168448 [details]
Patch

[Rob Buis]

Created attachment 168449 [details]
Patch

[Joe Mason]

(In reply to comment #14)
> I don't like this #if PLATFORM(). Does returning jsUndefined when !WebSocket::isAvailable() also make sense to other ports?

Not really - it's kind of a hack. The correct thing to do is to drop "websocket" from the list of properties entirely, which only works under V8. (There's a bug open about that.) If other ports want to take advantage of this they can add themselves to the ifdef, but it would be rude to turn this behaviour on by default.

[Rob Buis]

This landed in r131204, closing.