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

Issue 5359707120205824: Issue 654 - Attach pages to their tabs to improve performance on Safari (Closed)

Created:
June 7, 2014, 2:46 p.m. by Sebastian Noack
Modified:
June 11, 2014, 11:07 a.m.
Reviewers:
Wladimir Palant
Visibility:
Public.

Description

Issue 654 - Attach pages to their tabs to improve performance on Safari

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -50 lines) Patch
M safari/ext/background.js View 9 chunks +29 lines, -50 lines 1 comment Download

Messages

Total messages: 2
Sebastian Noack
June 7, 2014, 2:47 p.m. (2014-06-07 14:47:08 UTC) #1
Wladimir Palant
June 11, 2014, 8:17 a.m. (2014-06-11 08:17:10 UTC) #2
Not really convinced that the performance gain here would justify a larger
change like this one. However, the logic improvements here are well worth it -
LGTM with the comment below addressed.

http://codereview.adblockplus.org/5359707120205824/diff/5629499534213120/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5359707120205824/diff/5629499534213120/safa...
safari/ext/background.js:597: pages[pageId] = (tab._pages || (tab._pages =
{__proto__: null}))[pageId] = page;
Seriously, don't be too clever - go for readable code instead.

if (!tab._pages)
  tab._pages = {__proto__: null};
pages[pageId] = tab._pages[pageId] = page;

Powered by Google App Engine
This is Rietveld