Unified Diff: lib/crawler.js
Issue 29338442:
Issue 3815 - Fix TabAllocator. Now it returns tab with initialized outerWindowID (Closed)
Patch Set:
Created March 16, 2016, 2:18 p.m.
Use n/p to move between diff chunks;
N/P to move between comments.
« no previous file with comments
|
« no previous file
|
no next file »
|
no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')
| Index: lib/crawler.js |
| diff --git a/lib/crawler.js b/lib/crawler.js |
| index 7bc28dbbe75187a1a752583c81d5c638a24c98ca..21a4cf970d14f3569bb0a05783547b251695ce08 100644 |
| --- a/lib/crawler.js |
| +++ b/lib/crawler.js |
| @@ -25,7 +25,34 @@ let {FilterNotifier} = abprequire("filterNotifier"); |
| let {FilterStorage} = abprequire("filterStorage"); |
| /** |
| - * Creates a pool of tabs and allocates them to tasks on request. |
| + * Allocates a new tab "about:blank" in the `browser`. |
|
Wladimir Palant
2016/03/16 14:52:52
Please don't use backticks, these have no special
sergei
2016/03/16 15:47:00
Done.
|
| + * The method returns a {Promise} promise which is resolved with the `tab` |
|
Wladimir Palant
2016/03/16 14:52:52
"{Promise} promise" => "promise" please, the type
sergei
2016/03/16 15:47:00
Done.
|
| + * when `outerWindowID` is already initialized. |
|
Wladimir Palant
2016/03/16 14:52:52
Too many implementation details here. "which is re
sergei
2016/03/16 15:47:00
Done.
|
| + * |
| + * See: https://bugzilla.mozilla.org/show_bug.cgi?id=1256602#c1 |
|
Wladimir Palant
2016/03/16 14:52:52
This belongs into a comment above the actual imple
sergei
2016/03/16 15:47:00
Done.
|
| + * |
| + * @param {tabbrowser} browser |
| + * The tabbed browser where tabs should be created |
|
Wladimir Palant
2016/03/16 14:52:52
"tabs" => "the tab"
sergei
2016/03/16 15:47:01
Done.
|
| + * @return {Promise} promise which will be resolved with the tab. |
|
Wladimir Palant
2016/03/16 14:52:52
Please use the more specific Promise.<tab> as type
sergei
2016/03/16 15:47:00
Done.
|
| + */ |
| +function createTab(browser) |
| +{ |
| + let tab = browser.addTab("about:blank"); |
| + if (tab.linkedBrowser.outerWindowID) |
| + return Promise.resolve(tab); |
| + return new Promise((resolve, reject)=> |
|
Wladimir Palant
2016/03/16 14:52:52
Nit: Space before => please.
sergei
2016/03/16 15:47:00
Done.
|
| + { |
| + let onBrowserInit = (msg) => |
| + { |
| + tab.linkedBrowser.messageManager.removeMessageListener("Browser:Init", onBrowserInit); |
| + resolve(tab); |
| + }; |
| + tab.linkedBrowser.messageManager.addMessageListener("Browser:Init", onBrowserInit); |
| + }); |
| +} |
| + |
| +/** |
| + * Allocates tabs on request but not more than `maxtabs` at the same time. |
| * |
| * @param {tabbrowser} browser |
| * The tabbed browser where tabs should be created |
| @@ -35,33 +62,37 @@ let {FilterStorage} = abprequire("filterStorage"); |
| */ |
| function TabAllocator(browser, maxtabs) |
| { |
| + this._browser = browser; |
| + this._tabs = 0; |
| + this._maxtabs = maxtabs; |
| + // the array of `resolve` functions of {Promise} promises returned by `getTab`. |
|
Wladimir Palant
2016/03/16 14:52:52
This is a code comment, it's intended to be read b
sergei
2016/03/16 15:47:01
Sounds good!
|
| + this._resolvers = []; |
| + // Keep at least one tab alive to prevent browser from closing of it self. |
|
Wladimir Palant
2016/03/16 14:52:52
"browser from closing of it self" => "the browser
sergei
2016/03/16 15:47:00
Done.
|
| + // That tab will be removed when the first tab is requested. |
| browser.removeAllTabsBut(browser.tabs[0]) |
| - |
| - this._tabs = []; |
| - for (let i = 0; i < maxtabs; i++) |
| - this._tabs.push(browser.addTab("about:blank")); |
| - |
| - browser.removeTab(browser.tabs[0]); |
| - |
| - this._deferred = []; |
| } |
| TabAllocator.prototype = { |
| /** |
| - * Returns a promise that will resolve into a tab once a tab can be allocated. |
| + * Returns a promise that will resolve into a tab once a tab is allocated. |
| * The tab cannot be used by other tasks until releaseTab() is called. |
| * |
| * @result {Promise} |
|
Wladimir Palant
2016/03/16 14:52:52
Nit: Promise.<tab> here as well.
|
| */ |
| getTab: function() |
| { |
| - if (this._tabs.length) |
| - return this._tabs.shift(); |
| - else |
| + if (this._tabs < this._maxtabs) |
| { |
| - let deferred = Promise.defer(); |
| - this._deferred.push(deferred); |
| - return deferred.promise; |
| + let tab = createTab(this._browser); |
| + // close initial tab, we don't need it anymore. |
|
Wladimir Palant
2016/03/16 14:52:52
Nit: Please start sentences with a capital letter.
|
| + if (this._tabs == 0) |
| + this._browser.removeTab(this._browser.tabs[0]); |
| + this._tabs++; |
| + return tab; |
| } |
| + return new Promise((resolve, reject) => |
| + { |
| + this._resolvers.push(resolve); |
| + }); |
| }, |
| /** |
| @@ -73,12 +104,11 @@ TabAllocator.prototype = { |
| { |
| let browser = tab.parentNode.tabbrowser; |
| browser.removeTab(tab); |
| - tab = browser.addTab("about:blank"); |
| - if (this._deferred.length) |
| - this._deferred.shift().resolve(tab); |
| + if (this._resolvers.length) |
| + this._resolvers.shift()(createTab(this._browser)); |
| else |
| - this._tabs.push(tab); |
| + this._tabs--; |
|
Wladimir Palant
2016/03/16 14:52:52
Now you have the problem that in the theoretical s
sergei
2016/03/16 15:47:00
Yes. However I think it should be addressed at ano
sergei
2016/04/11 15:20:22
I have addressed that issue.
BTW, having an array
|
| } |
| }; |
« no previous file with comments
|
« no previous file
|
no next file »
|
no next file with comments »
