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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 8 months ago by Sebastian Noack
Modified:
5 years, 7 months ago
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
5 years, 8 months ago (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 ...
5 years, 8 months ago (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: > ...
5 years, 8 months ago (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 ...
5 years, 8 months ago (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 ...
5 years, 8 months ago (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 ...
5 years, 8 months ago (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 ...
5 years, 7 months ago (2014-03-31 15:43:44 UTC) #7
Wladimir Palant
5 years, 7 months ago (2014-04-01 11:16:33 UTC) #8
LGTM
Sign in to reply to this message.

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