Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29338442: Issue 3815 - Fix TabAllocator. Now it returns tab with initialized outerWindowID (Closed)

Created:
March 16, 2016, 2:18 p.m. by sergei
Modified:
Sept. 23, 2016, 10:51 a.m.
Reviewers:
Wladimir Palant
CC:
tschuster
Visibility:
Public.

Description

#abpcrawler project

Patch Set 1 #

Total comments: 25

Patch Set 2 : address comments #

Patch Set 3 : add missed semicolon #

Patch Set 4 : eliminate race conditions #

Total comments: 4

Patch Set 5 : address comment about comment #

Patch Set 6 : Address comment about tab keeping window alive #

Total comments: 6

Patch Set 7 : remove additional empty line #

Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -30 lines) Patch
M lib/crawler.js View 1 2 3 4 5 6 4 chunks +68 lines, -30 lines 0 comments Download
M run.py View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 9
sergei
March 16, 2016, 2:21 p.m. (2016-03-16 14:21:46 UTC) #1
Wladimir Palant
https://codereview.adblockplus.org/29338442/diff/29338443/lib/crawler.js File lib/crawler.js (right): https://codereview.adblockplus.org/29338442/diff/29338443/lib/crawler.js#newcode13 lib/crawler.js:13: Cu.import("resource://gre/modules/Promise.jsm"); Please remove this if you don't use Promise.defer() ...
March 16, 2016, 2:52 p.m. (2016-03-16 14:52:53 UTC) #2
sergei
https://codereview.adblockplus.org/29338442/diff/29338443/lib/crawler.js File lib/crawler.js (right): https://codereview.adblockplus.org/29338442/diff/29338443/lib/crawler.js#newcode13 lib/crawler.js:13: Cu.import("resource://gre/modules/Promise.jsm"); On 2016/03/16 14:52:52, Wladimir Palant wrote: > Please ...
March 16, 2016, 3:47 p.m. (2016-03-16 15:47:01 UTC) #3
sergei
https://codereview.adblockplus.org/29338442/diff/29338443/lib/crawler.js File lib/crawler.js (right): https://codereview.adblockplus.org/29338442/diff/29338443/lib/crawler.js#newcode111 lib/crawler.js:111: this._tabs--; On 2016/03/16 15:47:00, sergei wrote: > On 2016/03/16 ...
April 11, 2016, 3:20 p.m. (2016-04-11 15:20:23 UTC) #4
Wladimir Palant
Sorry about taking so long, this review simply got forgotten. If this happens again - ...
Sept. 14, 2016, 3 p.m. (2016-09-14 15:00:14 UTC) #5
sergei
https://codereview.adblockplus.org/29338442/diff/29339639/lib/crawler.js File lib/crawler.js (right): https://codereview.adblockplus.org/29338442/diff/29339639/lib/crawler.js#newcode55 lib/crawler.js:55: }); On 2016/09/14 15:00:13, Wladimir Palant wrote: > There ...
Sept. 15, 2016, 3:33 p.m. (2016-09-15 15:33:28 UTC) #6
Wladimir Palant
https://codereview.adblockplus.org/29338442/diff/29353170/lib/crawler.js File lib/crawler.js (right): https://codereview.adblockplus.org/29338442/diff/29353170/lib/crawler.js#newcode68 lib/crawler.js:68: return Promise.resolve(tab); I think that rather than introducing a ...
Sept. 16, 2016, 7:10 a.m. (2016-09-16 07:10:38 UTC) #7
sergei
https://codereview.adblockplus.org/29338442/diff/29353170/lib/crawler.js File lib/crawler.js (right): https://codereview.adblockplus.org/29338442/diff/29353170/lib/crawler.js#newcode68 lib/crawler.js:68: return Promise.resolve(tab); On 2016/09/16 07:10:37, Wladimir Palant wrote: > ...
Sept. 16, 2016, 12:34 p.m. (2016-09-16 12:34:13 UTC) #8
Wladimir Palant
Sept. 19, 2016, 12:40 p.m. (2016-09-19 12:40:51 UTC) #9
LGTM

Powered by Google App Engine
This is Rietveld