Issue 5607308285444096: Issue 1258 - Block ads in YouTube's HTML5 player on Safari (Closed)
|
Created:
Aug. 22, 2014, 2 p.m. by Sebastian Noack Modified:
Aug. 30, 2014, 7:12 p.m. Reviewers:
Wladimir Palant CC:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 1258 - Block ads in YouTube's HTML5 player on Safari
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addressed comments #
Total comments: 3
Patch Set 3 : Use Object.create() #MessagesTotal messages: 9
I don't really like messing with website functionality like that, but I assume that you looked into this properly and there is no other way... http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... File safari/include.youtube.js (right): http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:96: // the HMLT5 player is configured via ytplayer.config.args. We have Nit: "The HTML5 player" (properly capitalized and spelled correctly). http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:98: var ytplayer = undefined; What if ytplayer variable was set already? I'd recommend saving window.ytplayer first and assigning the original value after creating the property. http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:107: if (typeof rawYtplayer != "object") console.log(typeof null); This should be: if (!rawYtplayer || typeof rawYtplayer != "object") http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:113: var config = undefined; Here as well: what if the config property is already set? http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:121: if (typeof rawConfig != "object") Here as well: please consider the possibility that rawConfig is null. http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:148: }; There is a recurring pattern here: you wrap objects to change the value of a particular property when it is being set. Maybe put this into a helper function instead of repeating the same code three times? http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:156: ytplayer[prop] = rawYtplayer[prop]; I think that inheriting from rawYtplayer would be a better idea than copying the properties: ytplayer = Object.create(rawYtplayer); Same for the config object.
On 2014/08/28 16:43:38, Wladimir Palant wrote: > I don't really like messing with website functionality like that, but I assume > that you looked into this properly and there is no other way... I don't like it either. But it is the best approach I came up with, and it seems to work very reliable. http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... File safari/include.youtube.js (right): http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:96: // the HMLT5 player is configured via ytplayer.config.args. We have On 2014/08/28 16:43:38, Wladimir Palant wrote: > Nit: "The HTML5 player" (properly capitalized and spelled correctly). Done. http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:98: var ytplayer = undefined; On 2014/08/28 16:43:38, Wladimir Palant wrote: > What if ytplayer variable was set already? I'd recommend saving window.ytplayer > first and assigning the original value after creating the property. This hack only works anyway, if our JavaScript runs before YouTube's JavaScript, which is made sure by http://codereview.adblockplus.org/4757413714460672/. And in that case there is no way ytplayer is already set. http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:107: if (typeof rawYtplayer != "object") On 2014/08/28 16:43:38, Wladimir Palant wrote: > console.log(typeof null); > > This should be: > > if (!rawYtplayer || typeof rawYtplayer != "object") Thanks for reminding me that null is an object. I'm used to know that. :( http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:113: var config = undefined; On 2014/08/28 16:43:38, Wladimir Palant wrote: > Here as well: what if the config property is already set? This was handled by the loop below: for (var prop in rawYtplayer) ytplayer[prop] = rawYtplayer[prop]; However, now where I use inheritance instead, I have to explicitly handle that case. http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:121: if (typeof rawConfig != "object") On 2014/08/28 16:43:38, Wladimir Palant wrote: > Here as well: please consider the possibility that rawConfig is null. Done. http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:148: }; On 2014/08/28 16:43:38, Wladimir Palant wrote: > There is a recurring pattern here: you wrap objects to change the value of a > particular property when it is being set. Maybe put this into a helper function > instead of repeating the same code three times? Of course, I already considered that. However, the pattern differs. window.ytplayer needs to be defined with Object.defineProperty instead wrapping the original object. And ytplayer.config.args must be processed differently, than the other properties. So abstracting those cases in a reusable function would just make the code even more complex. http://codereview.adblockplus.org/5607308285444096/diff/5629499534213120/safa... safari/include.youtube.js:156: ytplayer[prop] = rawYtplayer[prop]; On 2014/08/28 16:43:38, Wladimir Palant wrote: > I think that inheriting from rawYtplayer would be a better idea than copying the > properties: > > ytplayer = Object.create(rawYtplayer); > > Same for the config object. Done. However, I'm not convinced that it is a better or worse idea. The resulting amount of code is the same.
LGTM if you decide that you don't want to address the comment below. http://codereview.adblockplus.org/5607308285444096/diff/5685265389584384/safa... File safari/include.youtube.js (right): http://codereview.adblockplus.org/5607308285444096/diff/5685265389584384/safa... safari/include.youtube.js:116: get config() Given that __proto__ is deprecated, I wonder whether this should be: ytplayer = Object.create(rawYtplayer, { config: { enumerable: true, get: function() {...}, set: function() {...} } }); You will of course point out that this adds two lines of course so it's up to you whether you want to do it :)
http://codereview.adblockplus.org/5607308285444096/diff/5685265389584384/safa... File safari/include.youtube.js (right): http://codereview.adblockplus.org/5607308285444096/diff/5685265389584384/safa... safari/include.youtube.js:116: get config() On 2014/08/29 20:37:07, Wladimir Palant wrote: > Given that __proto__ is deprecated, I wonder whether this should be: > > ytplayer = Object.create(rawYtplayer, { > config: { > enumerable: true, > get: function() {...}, > set: function() {...} > } > }); > > You will of course point out that this adds two lines of course so it's up to > you whether you want to do it :) We don't use Object.create() so far in this codebase. So I prefer to keep consistent for now. However, we should replace {__proto__: ...} with Object.create() all over the place at some point. Is there already an issue for that?
On 2014/08/29 20:45:05, Sebastian Noack wrote:
> We don't use Object.create() so far in this codebase. So I prefer to keep
> consistent for now. However, we should replace {__proto__: ...} with
> Object.create() all over the place at some point. Is there already an issue
for
> that?
No. The problem with Object.create() is that the property definition is very
verbose. So far we only agreed on using it in the obvious scenarios like
Object.create(null). However, the case where you extend the prototype with a
single getter/setter combination is a fairly obvious one as well.
http://codereview.adblockplus.org/5607308285444096/diff/5685265389584384/safa... File safari/include.youtube.js (right): http://codereview.adblockplus.org/5607308285444096/diff/5685265389584384/safa... safari/include.youtube.js:116: get config() On 2014/08/29 20:45:05, Sebastian Noack wrote: > On 2014/08/29 20:37:07, Wladimir Palant wrote: > > Given that __proto__ is deprecated, I wonder whether this should be: > > > > ytplayer = Object.create(rawYtplayer, { > > config: { > > enumerable: true, > > get: function() {...}, > > set: function() {...} > > } > > }); > > > > You will of course point out that this adds two lines of course so it's up to > > you whether you want to do it :) > > We don't use Object.create() so far in this codebase. So I prefer to keep > consistent for now. However, we should replace {__proto__: ...} with > Object.create() all over the place at some point. Is there already an issue for > that? Done.
LGTM 
|
