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

Issue 29350357: Issue 4386 - Fixed determining document domain, particularly after being redirected (Closed)

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

Description

Issue 4386 - Fixed determining document domain, particularly after being redirected Repository: hg.adblockplus.org/adblockpluschrome

Patch Set 1 #

Total comments: 11

Patch Set 2 : Fixed variable names #

Patch Set 3 : Fixed another regression from last-minute changes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -9 lines) Patch
M chrome/ext/background.js View 1 2 2 chunks +23 lines, -9 lines 0 comments Download

Messages

Total messages: 10
Wladimir Palant
3 years, 2 months ago (2016-08-31 15:11:59 UTC) #1
kzar
Your code Looks good to Dave apart from a couple of nits. As for if ...
3 years, 2 months ago (2016-08-31 16:06:39 UTC) #2
Wladimir Palant
https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/background.js#newcode121 chrome/ext/background.js:121: frame = frames[details.frameId] = {}; On 2016/08/31 16:06:39, kzar ...
3 years, 2 months ago (2016-08-31 19:13:44 UTC) #3
kzar
https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/background.js#newcode115 chrome/ext/background.js:115: var frames = framesOfTabs[details.tabId]; It should be `tabId` and ...
3 years, 2 months ago (2016-09-01 11:06:49 UTC) #4
kzar
https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/background.js#newcode130 chrome/ext/background.js:130: frame.parent = frames[details.parentFrameId] || null; We don't assign the ...
3 years, 2 months ago (2016-09-01 12:46:15 UTC) #5
kzar
https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/background.js#newcode130 chrome/ext/background.js:130: frame.parent = frames[details.parentFrameId] || null; On 2016/09/01 12:46:15, kzar ...
3 years, 2 months ago (2016-09-01 13:22:57 UTC) #6
kzar
https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/background.js#newcode130 chrome/ext/background.js:130: frame.parent = frames[details.parentFrameId] || null; On 2016/09/01 13:22:57, kzar ...
3 years, 2 months ago (2016-09-01 13:39:14 UTC) #7
Wladimir Palant
I only fixed the one issue below for now. The fix isn't entirely working for ...
3 years, 2 months ago (2016-09-05 15:48:26 UTC) #8
Wladimir Palant
It seems that I tested my changes, then cleaned up the code a bit and ...
3 years, 2 months ago (2016-09-05 16:00:28 UTC) #9
Sebastian Noack
3 years, 2 months ago (2016-09-05 16:33:15 UTC) #10
LGTM
Sign in to reply to this message.

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