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

Issue 29711592: Issue 6424 - Use internal symbol in ext namespace (Closed)

Created:
Feb. 28, 2018, 3:08 p.m. by Manish Jethani
Modified:
Sept. 10, 2018, 2:28 p.m.
Reviewers:
CC:
kzar, Sebastian Noack
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Following up on our discussion on #6424, this is a list of changes to the code in the ext namespace to make the underscore properties "internal" using a symbol. The symbol is defined in ext/common.js Every object that wants to have a separate "internal" namespace referenced via this symbol must call the global defineNamespace function. Underscore properties on class instances are now in their own internal namespace.

Patch Set 1 #

Total comments: 8

Patch Set 2 : Add base.js #

Patch Set 3 : Support internal methods #

Patch Set 4 : Switch to internal methods everywhere #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+181 lines, -143 lines) Patch
M composer.html View 1 chunk +1 line, -0 lines 0 comments Download
M desktop-options.html View 1 chunk +1 line, -0 lines 0 comments Download
M ext/background.js View 1 2 3 8 chunks +139 lines, -116 lines 1 comment Download
M ext/common.js View 1 2 3 1 chunk +19 lines, -14 lines 0 comments Download
M ext/content.js View 1 2 3 1 chunk +5 lines, -1 line 0 comments Download
M lib/requestBlocker.js View 3 chunks +6 lines, -9 lines 0 comments Download
M metadata.chrome View 2 chunks +9 lines, -3 lines 0 comments Download
M popup.html View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 5
Manish Jethani
Feb. 28, 2018, 3:08 p.m. (2018-02-28 15:08:28 UTC) #1
Manish Jethani
Patch Set 1 https://codereview.adblockplus.org/29711592/diff/29711593/composer.html File composer.html (right): https://codereview.adblockplus.org/29711592/diff/29711593/composer.html#newcode27 composer.html:27: <script type="text/javascript" src="base.js"></script> base.js is included ...
Feb. 28, 2018, 3:23 p.m. (2018-02-28 15:23:27 UTC) #2
Manish Jethani
Patch Set 2: Add base.js This new file defines a window.defineNamespace function that creates a ...
Feb. 28, 2018, 3:27 p.m. (2018-02-28 15:27:41 UTC) #3
Manish Jethani
Patch Set 3: Support internal methods This change allows us to define an internal namespace ...
Feb. 28, 2018, 4:35 p.m. (2018-02-28 16:35:48 UTC) #4
Manish Jethani
March 1, 2018, 12:04 p.m. (2018-03-01 12:04:14 UTC) #5
Patch Set 4: Switch to internal methods everywhere

https://codereview.adblockplus.org/29711592/diff/29712590/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29711592/diff/29712590/ext/background.js#n...
ext/background.js:328: {
This is mostly just an indentation change. this._applyChanges,
this._queueChanges, and this._addChange are now this[internal].applyChanges,
this[internal].queueChanges, and this[internal].addChange and that's all.

Powered by Google App Engine
This is Rietveld