Issue 29349737: Issue 1727 - Fix WebSocket constructor without second argument in Chrome 47 (Closed)
|
Created:
Aug. 11, 2016, 2:12 p.m. by Sebastian Noack Modified:
Aug. 15, 2016, 1:06 p.m. Visibility:
Public. |
DescriptionIssue 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
MessagesTotal messages: 12
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#... include.preload.js:400: var args = [null]; I think this approach is extremely hard to follow, when something way simpler would do: ``` if (!(this instanceof WrappedWebSocket)) return RealWebSocket(); if (arguments.length == 0) return new RealWebSocket(); var websocket; if (arguments.length == 1) websocket = new RealWebSocket(url); else if (arguments.length == 2) websocket = new RealWebSocket(url, protocols); ``` Sure it has a little duplication and isn't as clever, but is that such a bad thing? I mean bindWebSocket for example is getting nearly impenetrable to follow, good luck trying to figure out what `Function.prototype.bind.apply.bind(Function.prototype.bind, RealWebSocket);` does! The logic below with the creator is hardly easy to follow either. https://codereview.adblockplus.org/29349737/diff/29349738/include.preload.js#... include.preload.js:402: args.push(arguments[i]); This would allow for circumvention since we didn't keep track of Array.prototype.push, but like I said above I think we should rather keep this simple.
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#... include.preload.js:400: var args = [null]; On 2016/08/11 14:25:05, kzar wrote: > I think this approach is extremely hard to follow, when something way simpler > would do: > > ``` > if (!(this instanceof WrappedWebSocket)) return RealWebSocket(); > if (arguments.length == 0) return new RealWebSocket(); > > var websocket; > if (arguments.length == 1) websocket = new RealWebSocket(url); > else if (arguments.length == 2) websocket = new RealWebSocket(url, protocols); > ``` > > Sure it has a little duplication and isn't as clever, but is that such a bad > thing? I mean bindWebSocket for example is getting nearly impenetrable to > follow, good luck trying to figure out what > `Function.prototype.bind.apply.bind(Function.prototype.bind, RealWebSocket);` > does! The logic below with the creator is hardly easy to follow either. Well, if you'd write your logic compliant to our coding styles (add newlines where appropriate) it wouldn't be any shorter, however it has quite a few assumptions, and it's virtually impossible to make sure those hold true for all current and future supported browser versions. The pure fact that we'd have to extend that logic already, is a clear indication that this was a bad idea in the first place. My code on the other hand makes sure that the original WebSocket function is called in the exact same way our wrapper is called, delegating the whole behavior to the browser. https://codereview.adblockplus.org/29349737/diff/29349738/include.preload.js#... include.preload.js:402: args.push(arguments[i]); On 2016/08/11 14:25:05, kzar wrote: > This would allow for circumvention since we didn't keep track of > Array.prototype.push, but like I said above I think we should rather keep this > simple. Well spotted, fixed.
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#... include.preload.js:400: var args = [null]; On 2016/08/11 14:38:46, Sebastian Noack wrote: > On 2016/08/11 14:25:05, kzar wrote: > > I think this approach is extremely hard to follow, when something way simpler > > would do: > > > > ``` > > if (!(this instanceof WrappedWebSocket)) return RealWebSocket(); > > if (arguments.length == 0) return new RealWebSocket(); > > > > var websocket; > > if (arguments.length == 1) websocket = new RealWebSocket(url); > > else if (arguments.length == 2) websocket = new RealWebSocket(url, protocols); > > ``` > > > > Sure it has a little duplication and isn't as clever, but is that such a bad > > thing? I mean bindWebSocket for example is getting nearly impenetrable to > > follow, good luck trying to figure out what > > `Function.prototype.bind.apply.bind(Function.prototype.bind, RealWebSocket);` > > does! The logic below with the creator is hardly easy to follow either. > > Well, if you'd write your logic compliant to our coding styles (add newlines > where appropriate) it wouldn't be any shorter, however it has quite a few > assumptions, and it's virtually impossible to make sure those hold true for all > current and future supported browser versions. The pure fact that we'd have to > extend that logic already, is a clear indication that this was a bad idea in the > first place. > > My code on the other hand makes sure that the original WebSocket function is > called in the exact same way our wrapper is called, delegating the whole > behavior to the browser. I didn't say anything about the code being shorter, but simpler. As far as I see it we can fix the problem you spotted by simply adding a check to see if the arguments' length is 1, I just don't agree about instead doing something so complicated and hard to follow. The WebSocket constructor takes two arguments url and protocol, we know this. Maybe one day in the future that will change, but maybe one day in the future Chrome will support interception of WebSocket connections anyway. Since when do we do things in case it's one day useful?
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#... include.preload.js:400: var args = [null]; On 2016/08/11 14:54:16, kzar wrote: > On 2016/08/11 14:38:46, Sebastian Noack wrote: > > On 2016/08/11 14:25:05, kzar wrote: > > > I think this approach is extremely hard to follow, when something way > simpler > > > would do: > > > > > > ``` > > > if (!(this instanceof WrappedWebSocket)) return RealWebSocket(); > > > if (arguments.length == 0) return new RealWebSocket(); > > > > > > var websocket; > > > if (arguments.length == 1) websocket = new RealWebSocket(url); > > > else if (arguments.length == 2) websocket = new RealWebSocket(url, > protocols); > > > ``` > > > > > > Sure it has a little duplication and isn't as clever, but is that such a bad > > > thing? I mean bindWebSocket for example is getting nearly impenetrable to > > > follow, good luck trying to figure out what > > > `Function.prototype.bind.apply.bind(Function.prototype.bind, > RealWebSocket);` > > > does! The logic below with the creator is hardly easy to follow either. > > > > Well, if you'd write your logic compliant to our coding styles (add newlines > > where appropriate) it wouldn't be any shorter, however it has quite a few > > assumptions, and it's virtually impossible to make sure those hold true for > all > > current and future supported browser versions. The pure fact that we'd have to > > extend that logic already, is a clear indication that this was a bad idea in > the > > first place. > > > > My code on the other hand makes sure that the original WebSocket function is > > called in the exact same way our wrapper is called, delegating the whole > > behavior to the browser. > > I didn't say anything about the code being shorter, but simpler. As far as I see > it we can fix the problem you spotted by simply adding a check to see if the > arguments' length is 1, I just don't agree about instead doing something so > complicated and hard to follow. > > The WebSocket constructor takes two arguments url and protocol, we know this. > Maybe one day in the future that will change, but maybe one day in the future > Chrome will support interception of WebSocket connections anyway. Since when do > we do things in case it's one day useful? I don't think that I agree. But there you go.
LGTM
It's actually not that simple. Not all false-ish values are supposed to be interpreted as empty array. So we have to explicitly call the constructor with either one or two arguments.
LGTM
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); Why make assumptions about the arguments in the first place? What if a third parameter is added to the spec? var websocket = Object.create(RealWebSocket.prototype); RealWebSocket.apply(websocket, arguments);
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 12:33:24, Wladimir Palant wrote: > Why make assumptions about the arguments in the first place? What if a third > parameter is added to the spec? > > var websocket = Object.create(RealWebSocket.prototype); > RealWebSocket.apply(websocket, arguments); This will also allow dropping protocols parameter from the function signature - then WebSocket.length will match the real thing.
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); 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.
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. 
|
