Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(207)

Issue 29349737: Issue 1727 - Fix WebSocket constructor without second argument in Chrome 47 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 1 month ago by Sebastian Noack
Modified:
1 year, 1 month ago
Reviewers:
Wladimir Palant, kzar
Visibility:
Public.

Description

Issue 1727 - Fix WebSocket constructor without second argument in Chrome 47

Patch Set 1 #

Total comments: 6

Patch Set 2 : Rebased #

Patch Set 3 : Don't use Array.push #

Patch Set 4 : Just default to empty array #

Patch Set 5 : Omit the second argument if absent #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -1 line) Patch
M include.preload.js View 1 2 3 4 1 chunk +5 lines, -1 line 4 comments Download

Messages

Total messages: 12
Sebastian Noack
1 year, 1 month ago (2016-08-11 14:12:52 UTC) #1
kzar
https://codereview.adblockplus.org/29349737/diff/29349738/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29349737/diff/29349738/include.preload.js#newcode400 include.preload.js:400: var args = [null]; I think this approach is ...
1 year, 1 month ago (2016-08-11 14:25:05 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29349737/diff/29349738/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29349737/diff/29349738/include.preload.js#newcode400 include.preload.js:400: var args = [null]; On 2016/08/11 14:25:05, kzar wrote: ...
1 year, 1 month ago (2016-08-11 14:38:46 UTC) #3
kzar
https://codereview.adblockplus.org/29349737/diff/29349738/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29349737/diff/29349738/include.preload.js#newcode400 include.preload.js:400: var args = [null]; On 2016/08/11 14:38:46, Sebastian Noack ...
1 year, 1 month ago (2016-08-11 14:54:16 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29349737/diff/29349738/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29349737/diff/29349738/include.preload.js#newcode400 include.preload.js:400: var args = [null]; On 2016/08/11 14:54:16, kzar wrote: ...
1 year, 1 month ago (2016-08-11 15:11:36 UTC) #5
kzar
LGTM
1 year, 1 month ago (2016-08-11 15:27:15 UTC) #6
Sebastian Noack
It's actually not that simple. Not all false-ish values are supposed to be interpreted as ...
1 year, 1 month ago (2016-08-11 15:40:49 UTC) #7
kzar
LGTM
1 year, 1 month ago (2016-08-11 15:44:12 UTC) #8
Wladimir Palant
https://codereview.adblockplus.org/29349737/diff/29349752/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29349737/diff/29349752/include.preload.js#newcode413 include.preload.js:413: websocket = new RealWebSocket(url, protocols); Why make assumptions about ...
1 year, 1 month ago (2016-08-15 12:33:24 UTC) #9
Wladimir Palant
https://codereview.adblockplus.org/29349737/diff/29349752/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29349737/diff/29349752/include.preload.js#newcode413 include.preload.js:413: websocket = new RealWebSocket(url, protocols); On 2016/08/15 12:33:24, Wladimir ...
1 year, 1 month ago (2016-08-15 12:45:36 UTC) #10
Wladimir Palant
https://codereview.adblockplus.org/29349737/diff/29349752/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29349737/diff/29349752/include.preload.js#newcode413 include.preload.js:413: websocket = new RealWebSocket(url, protocols); Never mind, calling RealWebSocket.apply() ...
1 year, 1 month ago (2016-08-15 13:01:24 UTC) #11
Sebastian Noack
1 year, 1 month ago (2016-08-15 13:06:10 UTC) #12
Message was sent while issue was closed.
https://codereview.adblockplus.org/29349737/diff/29349752/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29349737/diff/29349752/include.preload.js#...
include.preload.js:413: websocket = new RealWebSocket(url, protocols);
On 2016/08/15 13:01:24, Wladimir Palant wrote:
> Never mind, calling RealWebSocket.apply() won't work no matter what you pass
in
> - detecting function call vs. new there doesn't work by checking this pointer.
> So avoiding assumptions about parameters here requires spread operator which
> sadly isn't supported before Chrome 46 or Safari 7.1, not worth it.

See patchset #2 (and the related discussin) for a compatible way to pass
variable arguments.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5