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

Issue 29329404: Issue 443 - Fix blocking pop-ups after redirect (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 11 months ago by Wladimir Palant
Modified:
2 years, 11 months ago
Visibility:
Public.

Description

The current code will merely prevent the redirect, not close the pop-up.

Patch Set 1 #

Total comments: 1

Patch Set 2 : Made sure to look at the original channel as well #

Total comments: 2

Patch Set 3 : Improved comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -12 lines) Patch
M lib/contentPolicy.js View 1 2 1 chunk +12 lines, -12 lines 0 comments Download

Messages

Total messages: 6
Wladimir Palant
2 years, 11 months ago (2015-10-26 20:28:24 UTC) #1
Wladimir Palant
https://codereview.adblockplus.org/29329404/diff/29329405/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/29329404/diff/29329405/lib/contentPolicy.js#newcode554 lib/contentPolicy.js:554: if (contentType == Ci.nsIContentPolicy.TYPE_DOCUMENT) This relies on the changes ...
2 years, 11 months ago (2015-10-26 20:50:26 UTC) #2
Wladimir Palant
With the new patchset all the pop-up blocking unit tests are passing now (at least ...
2 years, 11 months ago (2015-10-27 13:30:49 UTC) #3
tschuster
LGTM https://codereview.adblockplus.org/29329404/diff/29329417/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/29329404/diff/29329417/lib/contentPolicy.js#newcode512 lib/contentPolicy.js:512: } I think your comment about this patch ...
2 years, 11 months ago (2015-11-05 15:27:19 UTC) #4
Wladimir Palant
Note that interdiff will show further changes beyond the comment - these are due to ...
2 years, 11 months ago (2015-11-05 15:51:59 UTC) #5
tschuster
2 years, 11 months ago (2015-11-06 20:00:23 UTC) #6
On 2015/11/05 15:51:59, Wladimir Palant wrote:
> Note that interdiff will show further changes beyond the comment - these are
due
> to rebasing.
> 
> https://codereview.adblockplus.org/29329404/diff/29329417/lib/contentPolicy.js
> File lib/contentPolicy.js (right):
> 
>
https://codereview.adblockplus.org/29329404/diff/29329417/lib/contentPolicy.j...
> lib/contentPolicy.js:512: }
> On 2015/11/05 15:27:18, tschuster wrote:
> > I think your comment about this patch set would help here.
> > // The normal code below will merely prevent the redirect, not close the
> pop-up.
> 
> Done.

LGTM
Sign in to reply to this message.

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