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

Issue 29356615: Issue 4351 - Check History variable exists (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 11 months ago by kzar
Modified:
2 years, 11 months ago
Reviewers:
Sebastian Noack
Visibility:
Public.

Description

Issue 4351 - Check History variable exists

Patch Set 1 #

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

Messages

Total messages: 5
kzar
Patch Set 1 (I've not taken the time to reproduce this and therefore test the ...
2 years, 11 months ago (2016-10-12 15:48:46 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29356615/diff/29356616/safari/include.youtube.js File safari/include.youtube.js (right): https://codereview.adblockplus.org/29356615/diff/29356616/safari/include.youtube.js#newcode100 safari/include.youtube.js:100: if (typeof History != "undefined") It seems that the ...
2 years, 11 months ago (2016-10-12 15:51:58 UTC) #2
kzar
On 2016/10/12 15:51:58, Sebastian Noack wrote: > https://codereview.adblockplus.org/29356615/diff/29356616/safari/include.youtube.js > File safari/include.youtube.js (right): > > https://codereview.adblockplus.org/29356615/diff/29356616/safari/include.youtube.js#newcode100 ...
2 years, 11 months ago (2016-10-12 15:53:31 UTC) #3
Sebastian Noack
LGTM https://codereview.adblockplus.org/29356615/diff/29356616/safari/include.youtube.js File safari/include.youtube.js (right): https://codereview.adblockplus.org/29356615/diff/29356616/safari/include.youtube.js#newcode100 safari/include.youtube.js:100: if (typeof History != "undefined") On 2016/10/12 15:53:31, ...
2 years, 11 months ago (2016-10-12 18:03:35 UTC) #4
kzar
2 years, 11 months ago (2016-10-12 18:22:42 UTC) #5
On 2016/10/12 18:03:35, Sebastian Noack wrote:
> Fair enough, lets give it a try. But please check with Robert, whether this
> actually fixes blocking of YouTube ads, and revert this change if it does not.

Yea, that's fair. I've asked Robert to test it and mentioned that if it doesn't
help for him I'll revert the change.

> While it should avoid that exception, it is still unclear why the History
object
> isn't there in the first place. There might be a larger issue, which causes
more
> problems down the code path. Or the History object might just not be visible
to
> our script for some reason, but to YouTube's script in which case ads will be
> back when clicking on another video.

Right, but to be honest since this is an old version of a barely supported
platform I'd be happy to close as wontfix if the quick fix doesn't help.
Sign in to reply to this message.

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