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

Issue 5133931611422720: Simplified and removed memory leaks from code dealing with frames on Chrome (Closed)

Created:
Feb. 25, 2014, 5:52 p.m. by Sebastian Noack
Modified:
April 12, 2014, 8:32 a.m.
Visibility:
Public.

Description

This fixes a few issues with the code dealing with frames on Chrome, while it also reduces that code by half: 1. We used an array instead of an object, accessed by frame ID, resulting in an array with a lot of empty elements. 2. We didn't flushed the previously seen frames, when the main frame was reloading or loading another page. That resulted in constantly increasing memory usage, while navigating within the same tab. 3. We are used to fallback to the top level frame when encountering an unknown frame. But this fall back failed sometimes after reloading the extension, because we didn't retrieved the URLs of tabs, already there on startup. 4. I was able to eliminate the Frame constructor and prototype (and a lot of unnecessary lookups) by just storing the frame data in the format expected by the high level code, in the first place.

Patch Set 1 #

Total comments: 10

Patch Set 2 : Addressed comments #

Total comments: 4

Patch Set 3 : Addressed comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+78 lines, -105 lines) Patch
M chrome/ext/background.js View 1 2 2 chunks +74 lines, -101 lines 0 comments Download
M popupBlocker.js View 1 chunk +2 lines, -2 lines 0 comments Download
M webrequest.js View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 8
Sebastian Noack
Feb. 25, 2014, 6:23 p.m. (2014-02-25 18:23:44 UTC) #1
Wladimir Palant
http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chrome/ext/background.js File chrome/ext/background.js (right): http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chrome/ext/background.js#newcode200 chrome/ext/background.js:200: return framesOfTabs[tabId][frameId]; What about tabs we don't know? http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chrome/ext/background.js#newcode210 ...
March 6, 2014, 3:32 p.m. (2014-03-06 15:32:20 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chrome/ext/background.js File chrome/ext/background.js (right): http://codereview.adblockplus.org/5133931611422720/diff/5629499534213120/chrome/ext/background.js#newcode200 chrome/ext/background.js:200: return framesOfTabs[tabId][frameId]; On 2014/03/06 15:32:20, Wladimir Palant wrote: > ...
March 6, 2014, 7:54 p.m. (2014-03-06 19:54:49 UTC) #3
Wladimir Palant
http://codereview.adblockplus.org/5133931611422720/diff/5757334940811264/chrome/ext/background.js File chrome/ext/background.js (right): http://codereview.adblockplus.org/5133931611422720/diff/5757334940811264/chrome/ext/background.js#newcode224 chrome/ext/background.js:224: parent: parentFrameId != -1 ? frames[parentFrameId] : null I ...
March 6, 2014, 8:58 p.m. (2014-03-06 20:58:18 UTC) #4
Sebastian Noack
http://codereview.adblockplus.org/5133931611422720/diff/5757334940811264/chrome/ext/background.js File chrome/ext/background.js (right): http://codereview.adblockplus.org/5133931611422720/diff/5757334940811264/chrome/ext/background.js#newcode224 chrome/ext/background.js:224: parent: parentFrameId != -1 ? frames[parentFrameId] : null On ...
March 6, 2014, 9:11 p.m. (2014-03-06 21:11:55 UTC) #5
Wladimir Palant
http://codereview.adblockplus.org/5133931611422720/diff/5757334940811264/chrome/ext/background.js File chrome/ext/background.js (right): http://codereview.adblockplus.org/5133931611422720/diff/5757334940811264/chrome/ext/background.js#newcode224 chrome/ext/background.js:224: parent: parentFrameId != -1 ? frames[parentFrameId] : null I ...
March 21, 2014, 2:17 p.m. (2014-03-21 14:17:35 UTC) #6
Sebastian Noack
http://codereview.adblockplus.org/5133931611422720/diff/5757334940811264/chrome/ext/background.js File chrome/ext/background.js (right): http://codereview.adblockplus.org/5133931611422720/diff/5757334940811264/chrome/ext/background.js#newcode224 chrome/ext/background.js:224: parent: parentFrameId != -1 ? frames[parentFrameId] : null On ...
March 31, 2014, 3:43 p.m. (2014-03-31 15:43:44 UTC) #7
Wladimir Palant
April 1, 2014, 11:16 a.m. (2014-04-01 11:16:33 UTC) #8
LGTM

Powered by Google App Engine
This is Rietveld