Issue 29417597: Issue 5161 - Use maps and sets where appropriate (Closed)
|
Created:
April 19, 2017, 4:33 p.m. by Manish Jethani Modified:
May 22, 2017, 9:14 a.m. CC:
Wladimir Palant Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionUse maps and sets where appropriate
The following changes have been made:
- A number of instances of Object.create have been changed to use Map or Set objects instead
- Some related optimizations, esp. in ext.PageMap
In some cases the semantics have changed. For example, iteration is now guaranteed to be in insertion order. I have assumed that this won't break anything.
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address comments to Patch Set 1 #
Total comments: 13
Patch Set 3 : Remove blank line #Patch Set 4 : Replace nonEmptyPageMaps with allPageMaps #Patch Set 5 : Revert previous change and remove unnecessary logic #Patch Set 6 : Rebased #
Total comments: 2
Patch Set 7 : Rebased #Patch Set 8 : Minor unrelated changes #
MessagesTotal messages: 24
Patch Set 1
On 2017/04/19 16:46:07, Manish Jethani wrote: > Patch Set 1 Please could you file an issue for this? Take care to add a "Hints for testers" section to the description as well, that way the QA guys will know what to test if this lands before the next release.
On 2017/04/20 06:51:48, kzar wrote: > Please could you file an issue for this? Filed issue #5161.
By the way, the mapIterable function could be implemented as a generator
function like so:
function* mapIterable(iterable, callback, thisArg)
{
for (let item of iterable)
yield callback.call(thisArg, item);
}
I wasn't sure if we had a policy about using generator functions. I couldn't
find any in the source from a quick search. If we can, it would make it a lot
simpler. It looks like both Chrome and Firefox have had basic support for a
while.
On 2017/04/21 12:51:19, Manish Jethani wrote: > I wasn't sure if we had a policy about using generator functions. Using generator functions is generally kosher, and definitely preferable over any more elaborate construct, but I don't see why you would need to define a generator function here. https://codereview.adblockplus.org/29417597/diff/29417598/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29417598/ext/background.js#n... ext/background.js:62: let map = this._map; Why do you import this property into a local variable? https://codereview.adblockplus.org/29417597/diff/29417598/ext/background.js#n... ext/background.js:181: frame = Object.create(null); Why do we use an object with no prototype here again? If it is not used as a map we should just use an object literal. https://codereview.adblockplus.org/29417597/diff/29417598/ext/background.js#n... ext/background.js:518: return frames.get(frameId); How about: return frames && frames.get(frameId); https://codereview.adblockplus.org/29417597/diff/29417598/ext/background.js#n... ext/background.js:643: return frame.parent || null; This seems unrelated. https://codereview.adblockplus.org/29417597/diff/29417598/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29417597/diff/29417598/ext/common.js#newco... ext/common.js:76: ext.mapIterable = function(iterable, callback, thisArg) Why do we need this helper? IMO the calling code should just use a for-of loop. https://codereview.adblockplus.org/29417597/diff/29417598/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29417597/diff/29417598/lib/devtools.js#new... lib/devtools.js:39: return panels.size > 0; Now where this check is less obscure, there isn't much of a point for this function anymore, but the check should rather be inlined. https://codereview.adblockplus.org/29417597/diff/29417598/lib/popupBlocker.js File lib/popupBlocker.js (right): https://codereview.adblockplus.org/29417597/diff/29417598/lib/popupBlocker.js... lib/popupBlocker.js:114: let popup = Object.create(null); As mentioned before in this review, please don't create objects with no prototype. Use a literal, unless a Map object is more appropriate.
Patch Set 2: Address comments to Patch Set 1 https://codereview.adblockplus.org/29417597/diff/29417598/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29417598/ext/background.js#n... ext/background.js:62: let map = this._map; On 2017/04/29 22:46:32, Sebastian Noack wrote: > Why do you import this property into a local variable? I wrongly assumed that this would make it faster. Now I know that's not the case, at least in v8. I've changed it. https://codereview.adblockplus.org/29417597/diff/29417598/ext/background.js#n... ext/background.js:181: frame = Object.create(null); On 2017/04/29 22:46:32, Sebastian Noack wrote: > Why do we use an object with no prototype here again? If it is not used as a map > we should just use an object literal. Again I assumed that an object with no prototype would have faster property lookups (both hits and misses). This is in fact the case in Firefox after a recent update, but in Chrome it actually slows things down. I've changed it back. https://codereview.adblockplus.org/29417597/diff/29417598/ext/background.js#n... ext/background.js:518: return frames.get(frameId); On 2017/04/29 22:46:32, Sebastian Noack wrote: > How about: > > return frames && frames.get(frameId); Done. https://codereview.adblockplus.org/29417597/diff/29417598/ext/background.js#n... ext/background.js:643: return frame.parent || null; On 2017/04/29 22:46:32, Sebastian Noack wrote: > This seems unrelated. frame.parent is either an object or undefined. I thought we'd consistently return null here (see further above in this code). We could also try to make sure it's always either an object or null to begin with, which we were trying earlier, but that's too much work to maintain. It's better IMO not to set the parent property on the frame object if there's no parent object available. We could also just return undefined if we don't care about that. https://codereview.adblockplus.org/29417597/diff/29417598/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29417597/diff/29417598/ext/common.js#newco... ext/common.js:76: ext.mapIterable = function(iterable, callback, thisArg) On 2017/04/29 22:46:32, Sebastian Noack wrote: > Why do we need this helper? IMO the calling code should just use a for-of loop. Fair enough, the function that used this now returns an iterable. Removed. https://codereview.adblockplus.org/29417597/diff/29417598/lib/devtools.js File lib/devtools.js (right): https://codereview.adblockplus.org/29417597/diff/29417598/lib/devtools.js#new... lib/devtools.js:39: return panels.size > 0; On 2017/04/29 22:46:32, Sebastian Noack wrote: > Now where this check is less obscure, there isn't much of a point for this > function anymore, but the check should rather be inlined. Changed here and in popupBlocker. https://codereview.adblockplus.org/29417597/diff/29417598/lib/popupBlocker.js File lib/popupBlocker.js (right): https://codereview.adblockplus.org/29417597/diff/29417598/lib/popupBlocker.js... lib/popupBlocker.js:114: let popup = Object.create(null); On 2017/04/29 22:46:32, Sebastian Noack wrote: > As mentioned before in this review, please don't create objects with no > prototype. Use a literal, unless a Map object is more appropriate. Done.
https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#n... ext/background.js:48: if (prevSize == 0) Why not just calling nonEmptyPageMaps.add() no matter what? The behavior will be the same. https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#n... ext/background.js:630: return frame.parent || null; Any particular reason you force the return value to be null rather than undefined? https://codereview.adblockplus.org/29417597/diff/29429592/options.js File options.js (left): https://codereview.adblockplus.org/29417597/diff/29429592/options.js#oldcode46 options.js:46: Nit: I would get rid of the blank line here.
Patch Set 2: Remove blank line Comments inline. https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#n... ext/background.js:48: if (prevSize == 0) On 2017/05/17 06:46:48, Sebastian Noack wrote: > Why not just calling nonEmptyPageMaps.add() no matter what? The behavior will be > the same. Well, add could be an expensive operation, depending on the way the Set object has been implemented. I actually looked this up. Both Chrome and Firefox use the Close table implementation: https://wiki.mozilla.org/User:Jorend/Deterministic_hash_tables https://github.com/jorendorff/dht/blob/2d8cf136bad0fa92aeb2b7898cc59b46b471ab... https://cs.chromium.org/chromium/src/v8/src/js/collection.js?q=generichash+pa... https://hg.mozilla.org/mozilla-central/file/261a33fad371/js/src/ds/OrderedHas... The add operation is "O(1)", but basically it involves a hash operation and a lookup. The speed of the lookup depends on how many items are in the bucket, that's a linear search at some point, but given the parameters used it's going to be ~2.67 iterations on average and N iterations in the unlikely worst case (N being the number of entries in the set). I don't know how they hash an object reference, but Firefox for example will try to "scramble" the output of the hash function if it's based on a pointer in memory, for security reasons: https://dxr.mozilla.org/mozilla-beta/rev/8bf3d5c1b4b0de2673a74d56c7399a80c543... Anyway, so it's probably not too bad in the grand scheme of things. On the other hand, these things can easily add up to a bad user experience. Does it hurt to avoid calling that method? nonEmptyPageMaps is a bit of a micro-optimization to begin with. I don't know if it adds much value. It's only used in ext._removeFromAllPageMaps, which is only called for top-level frames. https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#n... ext/background.js:630: return frame.parent || null; On 2017/05/17 06:46:48, Sebastian Noack wrote: > Any particular reason you force the return value to be null rather than > undefined? Just for consistency, nothing else. https://codereview.adblockplus.org/29417597/diff/29429592/options.js File options.js (left): https://codereview.adblockplus.org/29417597/diff/29429592/options.js#oldcode46 options.js:46: On 2017/05/17 06:46:48, Sebastian Noack wrote: > Nit: I would get rid of the blank line here. Done.
On 2017/05/17 21:53:24, Manish Jethani wrote: > Patch Set 2: Remove blank line Sorry, messed up again. It's Patch Set 3.
https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#n... ext/background.js:48: if (prevSize == 0) On 2017/05/17 21:53:24, Manish Jethani wrote: > On 2017/05/17 06:46:48, Sebastian Noack wrote: > > Why not just calling nonEmptyPageMaps.add() no matter what? The behavior will > be > > the same. > > Well, add could be an expensive operation, depending on the way the Set object > has been implemented. > > I actually looked this up. Both Chrome and Firefox use the Close table > implementation: > > https://wiki.mozilla.org/User:Jorend/Deterministic_hash_tables > > https://github.com/jorendorff/dht/blob/2d8cf136bad0fa92aeb2b7898cc59b46b471ab... > > https://cs.chromium.org/chromium/src/v8/src/js/collection.js?q=generichash+pa... > > https://hg.mozilla.org/mozilla-central/file/261a33fad371/js/src/ds/OrderedHas... > > The add operation is "O(1)", but basically it involves a hash operation and a > lookup. The speed of the lookup depends on how many items are in the bucket, > that's a linear search at some point, but given the parameters used it's going > to be ~2.67 iterations on average and N iterations in the unlikely worst case (N > being the number of entries in the set). It seems there will be up to 7 PageMap objects at any given time. So I suppose even a linear search over all entries would still be ridicolous fast. > Does it hurt to avoid calling that method? It's additional logic, which makes the code harder to maintain. Also notice that checking the size of the map isn't for free either. So perhaps you want to benchmark this code with and without checking for the map being empty, with a realistic number of entries in nonEmptyPageMaps (i.e. 7). > nonEmptyPageMaps is a bit of a micro-optimization to begin with. I don't know if > it adds much value. It's only used in ext._removeFromAllPageMaps, which is only > called for top-level frames. nonEmptyPageMaps is not a micro-optimization, but was an attempt to mitigate memory leaks. But it seems we could just use a WeakSet() that holds all active instances now.
Comments inline. https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#n... ext/background.js:48: if (prevSize == 0) On 2017/05/18 12:23:00, Sebastian Noack wrote: > On 2017/05/17 21:53:24, Manish Jethani wrote: > > On 2017/05/17 06:46:48, Sebastian Noack wrote: > > Does it hurt to avoid calling that method? > > It's additional logic, which makes the code harder to maintain. Fair enough. > Also notice that checking the size of the map isn't for free either. So perhaps > you want to benchmark this code with and without checking for the map being > empty, with a realistic number of entries in nonEmptyPageMaps (i.e. 7). The size of the map is a value in memory, it's practically free. Also equally relevant to the number of PageMap objects is the number of times the set method is called. > > nonEmptyPageMaps is a bit of a micro-optimization to begin with. I don't know > if > > it adds much value. It's only used in ext._removeFromAllPageMaps, which is > only > > called for top-level frames. > > nonEmptyPageMaps is not a micro-optimization, but was an attempt to mitigate > memory leaks. But it seems we could just use a WeakSet() that holds all active > instances now. What I really meant here was that nonEmptyPageMaps could just have been allPageMaps, then it would only have to be updated in the constructor (a total of about 7 times?). The only reason the program keeps track of the empty status is to avoid calling delete on empty maps, which is exactly the kind of optimization I'm trying to do here, only far less useful. Why not just make it allPageMaps? Then we don't have to keep track of the empty status, hence code that's easier to maintain. Since there are only about 7 PageMap objects, we could just keep all of them in memory, whether they're empty or not. We could also just make allPageMaps a WeakSet object. Now that I look at the code again, we could simply replace ext.PageMap with ext.createPageMap, a function that would look like this: let allPageMaps = new Set(); // or WeakSet function createPageMap() { let pageMap = new Map(); allPageMaps.add(pageMap); return pageMap; } Instead of "new ext.PageMap()" you would do "ext.createPageMap()" and that would be it.
https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#n... ext/background.js:48: if (prevSize == 0) On 2017/05/18 21:16:37, Manish Jethani wrote: > On 2017/05/18 12:23:00, Sebastian Noack wrote: > > On 2017/05/17 21:53:24, Manish Jethani wrote: > > > On 2017/05/17 06:46:48, Sebastian Noack wrote: > > > > Does it hurt to avoid calling that method? > > > > It's additional logic, which makes the code harder to maintain. > > Fair enough. > > > Also notice that checking the size of the map isn't for free either. So > perhaps > > you want to benchmark this code with and without checking for the map being > > empty, with a realistic number of entries in nonEmptyPageMaps (i.e. 7). > > The size of the map is a value in memory, it's practically free. > > Also equally relevant to the number of PageMap objects is the number of times > the set method is called. > > > > nonEmptyPageMaps is a bit of a micro-optimization to begin with. I don't > know > > if > > > it adds much value. It's only used in ext._removeFromAllPageMaps, which is > > only > > > called for top-level frames. > > > > nonEmptyPageMaps is not a micro-optimization, but was an attempt to mitigate > > memory leaks. But it seems we could just use a WeakSet() that holds all active > > instances now. > > What I really meant here was that nonEmptyPageMaps could just have been > allPageMaps, then it would only have to be updated in the constructor (a total > of about 7 times?). The only reason the program keeps track of the empty status > is to avoid calling delete on empty maps, which is exactly the kind of > optimization I'm trying to do here, only far less useful. Why not just make it > allPageMaps? Then we don't have to keep track of the empty status, hence code > that's easier to maintain. > > Since there are only about 7 PageMap objects, we could just keep all of them in > memory, whether they're empty or not. We could also just make allPageMaps a > WeakSet object. > > Now that I look at the code again, we could simply replace ext.PageMap with > ext.createPageMap, a function that would look like this: > > let allPageMaps = new Set(); // or WeakSet > function createPageMap() > { > let pageMap = new Map(); > allPageMaps.add(pageMap); > return pageMap; > } > > Instead of "new ext.PageMap()" you would do "ext.createPageMap()" and that would > be it. That is basically what I meant to suggest. However, I don't quite get why you want to introduce a factory function, rather than having the constructor register the instance? Also please use a WeakSet, as it doesn't add any complexity, but is memory-safe in case we create temporary instances. For reference, the reason I didn't do so when I initially wrote this code, and the reason for keeping track of the non-empty PageMaps (with an array), was that back then WeakSet wasn't there yet, and this way at least empty instances wouldn't be leaked. Not calling delete() for these was a mere side-effect.
Patch Set 4: Replace nonEmptyPageMaps with allPageMaps https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#n... ext/background.js:48: if (prevSize == 0) On 2017/05/18 21:42:12, Sebastian Noack wrote: > On 2017/05/18 21:16:37, Manish Jethani wrote: > > On 2017/05/18 12:23:00, Sebastian Noack wrote: > > > On 2017/05/17 21:53:24, Manish Jethani wrote: > > > > On 2017/05/17 06:46:48, Sebastian Noack wrote: > > > > > > Does it hurt to avoid calling that method? > > > > > > It's additional logic, which makes the code harder to maintain. > > > > Fair enough. > > > > > Also notice that checking the size of the map isn't for free either. So > > perhaps > > > you want to benchmark this code with and without checking for the map being > > > empty, with a realistic number of entries in nonEmptyPageMaps (i.e. 7). > > > > The size of the map is a value in memory, it's practically free. > > > > Also equally relevant to the number of PageMap objects is the number of times > > the set method is called. > > > > > > nonEmptyPageMaps is a bit of a micro-optimization to begin with. I don't > > know > > > if > > > > it adds much value. It's only used in ext._removeFromAllPageMaps, which is > > > only > > > > called for top-level frames. > > > > > > nonEmptyPageMaps is not a micro-optimization, but was an attempt to mitigate > > > memory leaks. But it seems we could just use a WeakSet() that holds all > active > > > instances now. > > > > What I really meant here was that nonEmptyPageMaps could just have been > > allPageMaps, then it would only have to be updated in the constructor (a total > > of about 7 times?). The only reason the program keeps track of the empty > status > > is to avoid calling delete on empty maps, which is exactly the kind of > > optimization I'm trying to do here, only far less useful. Why not just make it > > allPageMaps? Then we don't have to keep track of the empty status, hence code > > that's easier to maintain. > > > > Since there are only about 7 PageMap objects, we could just keep all of them > in > > memory, whether they're empty or not. We could also just make allPageMaps a > > WeakSet object. > > > > Now that I look at the code again, we could simply replace ext.PageMap with > > ext.createPageMap, a function that would look like this: > > > > let allPageMaps = new Set(); // or WeakSet > > function createPageMap() > > { > > let pageMap = new Map(); > > allPageMaps.add(pageMap); > > return pageMap; > > } > > > > Instead of "new ext.PageMap()" you would do "ext.createPageMap()" and that > would > > be it. > > That is basically what I meant to suggest. However, I don't quite get why you > want to introduce a factory function, rather than having the constructor > register the instance? I was hoping to get rid of all the methods entirely since they seemed to be mere passthroughs to the underlying Map object, but it turns out it's not that simple because ext.PageMap deals with page objects where the underlying Map object deals with their IDs. > Also please use a WeakSet, as it doesn't add any > complexity, but is memory-safe in case we create temporary instances. We forgot that WeakSet doesn't allow iteration. We can't use it here. JavaScript still doesn't have weak references, that's sad. We had this in AS3. I think I had a blog post about it back in the day. Anyway, looks like weak references are coming in ES2017 or something.
https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#n... ext/background.js:48: if (prevSize == 0) On 2017/05/18 22:39:09, Manish Jethani wrote: > On 2017/05/18 21:42:12, Sebastian Noack wrote: > > On 2017/05/18 21:16:37, Manish Jethani wrote: > > > On 2017/05/18 12:23:00, Sebastian Noack wrote: > > > > On 2017/05/17 21:53:24, Manish Jethani wrote: > > > > > On 2017/05/17 06:46:48, Sebastian Noack wrote: > > > > > > > > Does it hurt to avoid calling that method? > > > > > > > > It's additional logic, which makes the code harder to maintain. > > > > > > Fair enough. > > > > > > > Also notice that checking the size of the map isn't for free either. So > > > perhaps > > > > you want to benchmark this code with and without checking for the map > being > > > > empty, with a realistic number of entries in nonEmptyPageMaps (i.e. 7). > > > > > > The size of the map is a value in memory, it's practically free. > > > > > > Also equally relevant to the number of PageMap objects is the number of > times > > > the set method is called. > > > > > > > > nonEmptyPageMaps is a bit of a micro-optimization to begin with. I don't > > > know > > > > if > > > > > it adds much value. It's only used in ext._removeFromAllPageMaps, which > is > > > > only > > > > > called for top-level frames. > > > > > > > > nonEmptyPageMaps is not a micro-optimization, but was an attempt to > mitigate > > > > memory leaks. But it seems we could just use a WeakSet() that holds all > > active > > > > instances now. > > > > > > What I really meant here was that nonEmptyPageMaps could just have been > > > allPageMaps, then it would only have to be updated in the constructor (a > total > > > of about 7 times?). The only reason the program keeps track of the empty > > status > > > is to avoid calling delete on empty maps, which is exactly the kind of > > > optimization I'm trying to do here, only far less useful. Why not just make > it > > > allPageMaps? Then we don't have to keep track of the empty status, hence > code > > > that's easier to maintain. > > > > > > Since there are only about 7 PageMap objects, we could just keep all of them > > in > > > memory, whether they're empty or not. We could also just make allPageMaps a > > > WeakSet object. > > > > > > Now that I look at the code again, we could simply replace ext.PageMap with > > > ext.createPageMap, a function that would look like this: > > > > > > let allPageMaps = new Set(); // or WeakSet > > > function createPageMap() > > > { > > > let pageMap = new Map(); > > > allPageMaps.add(pageMap); > > > return pageMap; > > > } > > > > > > Instead of "new ext.PageMap()" you would do "ext.createPageMap()" and that > > would > > > be it. > > > > That is basically what I meant to suggest. However, I don't quite get why you > > want to introduce a factory function, rather than having the constructor > > register the instance? > > I was hoping to get rid of all the methods entirely since they seemed to be mere > passthroughs to the underlying Map object, but it turns out it's not that simple > because ext.PageMap deals with page objects where the underlying Map object > deals with their IDs. > > > Also please use a WeakSet, as it doesn't add any > > complexity, but is memory-safe in case we create temporary instances. > > We forgot that WeakSet doesn't allow iteration. We can't use it here. > > JavaScript still doesn't have weak references, that's sad. We had this in AS3. I > think I had a blog post about it back in the day. Anyway, looks like weak > references are coming in ES2017 or something. Hmm, perhaps we should have stayed with the nonEmptyPageMap approach then. This way we at least make sure that temporary instances get eventually cleaned up when all pages they held are gone. I just carefully checked all places that use a PageMap and it seems all instances are created once and never go out of scope. So while this shouldn't cause a memory leaks with the current code base, I'm mildly concerned we might have a use case for temporary instances in the future, and whoever might introduce these might not be aware that PageMaps are leaked. Eventually, I want to get rid of the abstraction layer, i.e. ext.*, including PageMaps, all together, and have all code use the chrome.* (or browser.*) API and tabIds directly. The only reason we have this abstraction layer was so that we could support Safari, however, we no longer support anything else than Chrome, Firefox and Edge which all have roughly the same API, with this code.
Patch Set 5: Revert previous change and remove unnecessary logic https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#n... ext/background.js:48: if (prevSize == 0) On 2017/05/19 12:01:14, Sebastian Noack wrote: > Hmm, perhaps we should have stayed with the nonEmptyPageMap approach then. Yeah, I wasn't too excited about this change either after I learnt that we couldn't use a WeakSet. I've reverted it now and removed the unnecessary logic as well. > Eventually, I want to get rid of the abstraction layer, i.e. ext.*, including > PageMaps, all together, and have all code use the chrome.* (or browser.*) API > and tabIds directly. The only reason we have this abstraction layer was so that > we could support Safari, however, we no longer support anything else than > Chrome, Firefox and Edge which all have roughly the same API, with this code. I see, thanks. When I feel like I know enough about the code, I'll see if I can come up with something for this.
LGTM https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29417597/diff/29429592/ext/background.js#n... ext/background.js:48: if (prevSize == 0) On 2017/05/19 16:20:34, Manish Jethani wrote: > On 2017/05/19 12:01:14, Sebastian Noack wrote: > > Eventually, I want to get rid of the abstraction layer, i.e. ext.*, including > > PageMaps, all together, and have all code use the chrome.* (or browser.*) API > > and tabIds directly. The only reason we have this abstraction layer was so > that > > we could support Safari, however, we no longer support anything else than > > Chrome, Firefox and Edge which all have roughly the same API, with this code. > > I see, thanks. > > When I feel like I know enough about the code, I'll see if I can come up with > something for this. That'd be awesome. :)
On 2017/05/19 17:33:14, Sebastian Noack wrote: > LGTM (I'm not planning to review this one, don't wait for me if you'd like to push it.)
On 2017/05/19 18:15:44, kzar wrote: > On 2017/05/19 17:33:14, Sebastian Noack wrote: > > LGTM > > (I'm not planning to review this one, don't wait for me if you'd like to push > it.) Submitted the patch, thanks.
Patch Set 6: Rebased
https://codereview.adblockplus.org/29417597/diff/29443555/lib/popupBlocker.js File lib/popupBlocker.js (left): https://codereview.adblockplus.org/29417597/diff/29443555/lib/popupBlocker.js... lib/popupBlocker.js:112: let {tabId} = details; Perhaps get rid of this variable, and in-line details.tabId below? It's debatable unrelated, and not new since the rebase, but IMO it would make the code slightly more consistent and easier to read. I leave it up to you. Either way, with or without this change, still LGTM.
Patch Set 7: Rebased Patch Set 8: Minor unrelated changes https://codereview.adblockplus.org/29417597/diff/29443555/lib/popupBlocker.js File lib/popupBlocker.js (left): https://codereview.adblockplus.org/29417597/diff/29443555/lib/popupBlocker.js... lib/popupBlocker.js:112: let {tabId} = details; On 2017/05/21 20:40:38, Sebastian Noack wrote: > Perhaps get rid of this variable, and in-line details.tabId below? It's > debatable unrelated, and not new since the rebase, but IMO it would make the > code slightly more consistent and easier to read. I leave it up to you. > > Either way, with or without this change, still LGTM. Done.
Still LGTN
On 2017/05/21 22:26:46, Sebastian Noack wrote: > Still LGTN *LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
