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

Issue 29532767: Issue 5593 - Use messaging in popup for prefs, whitelisting, and stats (Closed)

Created:
Aug. 31, 2017, 5:07 p.m. by Manish Jethani
Modified:
Sept. 27, 2017, 10:13 p.m.
Reviewers:
Sebastian Noack
CC:
Thomas Greiner, Wladimir Palant, kzar
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Patch Set 1 : Use messaging for prefs I/O #

Total comments: 4

Patch Set 2 : Use messaging for prefs in stats.js #

Total comments: 2

Patch Set 3 : Use messaging to communicate with filterComposer #

Total comments: 1

Patch Set 4 : Use chrome APIs instead of ext.pages #

Total comments: 7

Patch Set 5 : Move functions into popup.js #

Total comments: 3

Patch Set 6 : Check for page ready again after adding listener #

Total comments: 2

Patch Set 7 : Use promises for page ready check #

Patch Set 8 : Remove callback parameter #

Total comments: 5

Patch Set 9 : Remove odd use of promise rejection #

Total comments: 1

Patch Set 10 : Check for page ready state only once #

Total comments: 1

Patch Set 11 : Remove listener on promise fulfillment #

Total comments: 3

Patch Set 12 : Refactor whenPageReady #

Total comments: 16

Patch Set 13 : Simplify whenPageReady #

Total comments: 4

Patch Set 14 : Add comment explaining createPageObject hack #

Total comments: 1

Patch Set 15 : Change whitelisting calls to use messaging #

Total comments: 15

Patch Set 16 : Rebase on next #

Patch Set 17 : Fix lint error and simplify code #

Total comments: 1

Patch Set 18 : Revert to using ext.pages.open #

Total comments: 1

Patch Set 19 : Move require to notification.js #

Patch Set 20 : Revert one more instance of ext.pages.open #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+182 lines, -87 lines) Patch
M dependencies View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -1 line 0 comments Download
M lib/filterComposer.js View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M lib/stats.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +5 lines, -0 lines 0 comments Download
M lib/whitelisting.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +37 lines, -1 line 0 comments Download
M notification.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +2 lines, -0 lines 0 comments Download
M popup.html View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -4 lines 0 comments Download
M popup.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +89 lines, -58 lines 0 comments Download
M skin/popup.css View 1 chunk +1 line, -1 line 0 comments Download
M stats.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +37 lines, -22 lines 1 comment Download

Messages

Total messages: 43
Manish Jethani
Aug. 31, 2017, 5:07 p.m. (2017-08-31 17:07:42 UTC) #1
Manish Jethani
Patch Set 1 Since the Prefs object is not going to be available in the ...
Aug. 31, 2017, 5:10 p.m. (2017-08-31 17:10:26 UTC) #2
Manish Jethani
Comments inline. https://codereview.adblockplus.org/29532767/diff/29532768/popup.html File popup.html (right): https://codereview.adblockplus.org/29532767/diff/29532768/popup.html#newcode74 popup.html:74: <li id="stats-container" class="collapsed"> Collapsed by default. https://codereview.adblockplus.org/29532767/diff/29532768/popup.html#newcode81 ...
Aug. 31, 2017, 5:12 p.m. (2017-08-31 17:12:21 UTC) #3
Sebastian Noack
What is about the other APIs imported from the background page? I suppose we have ...
Aug. 31, 2017, 6:19 p.m. (2017-08-31 18:19:19 UTC) #4
Wladimir Palant
https://codereview.adblockplus.org/29532767/diff/29532768/popup.html File popup.html (right): https://codereview.adblockplus.org/29532767/diff/29532768/popup.html#newcode25 popup.html:25: <script src="ext/content.js"></script> Do we need this? The messaging uses ...
Aug. 31, 2017, 6:19 p.m. (2017-08-31 18:19:33 UTC) #5
Wladimir Palant
On 2017/08/31 18:19:19, Sebastian Noack wrote: > It might be easier to review all changes ...
Aug. 31, 2017, 6:20 p.m. (2017-08-31 18:20:30 UTC) #6
Sebastian Noack
On 2017/08/31 18:20:30, Wladimir Palant wrote: > On 2017/08/31 18:19:19, Sebastian Noack wrote: > > ...
Aug. 31, 2017, 6:28 p.m. (2017-08-31 18:28:57 UTC) #7
Manish Jethani
On 2017/08/31 18:20:30, Wladimir Palant wrote: > On 2017/08/31 18:19:19, Sebastian Noack wrote: > > ...
Aug. 31, 2017, 6:30 p.m. (2017-08-31 18:30:14 UTC) #8
Manish Jethani
On 2017/08/31 18:28:57, Sebastian Noack wrote: > On 2017/08/31 18:20:30, Wladimir Palant wrote: > > ...
Aug. 31, 2017, 6:37 p.m. (2017-08-31 18:37:22 UTC) #9
Sebastian Noack
On 2017/08/31 18:37:22, Manish Jethani wrote: > On 2017/08/31 18:28:57, Sebastian Noack wrote: > > ...
Aug. 31, 2017, 7:22 p.m. (2017-08-31 19:22:05 UTC) #10
Wladimir Palant
On 2017/08/31 19:22:05, Sebastian Noack wrote: > How about multiple patch sets in the same ...
Aug. 31, 2017, 7:23 p.m. (2017-08-31 19:23:42 UTC) #11
Manish Jethani
On 2017/08/31 19:23:42, Wladimir Palant wrote: > On 2017/08/31 19:22:05, Sebastian Noack wrote: > > ...
Sept. 1, 2017, 3:50 p.m. (2017-09-01 15:50:58 UTC) #12
Manish Jethani
Patch Set 2: Use messaging for prefs in stats.js With this update the popup code ...
Sept. 1, 2017, 3:56 p.m. (2017-09-01 15:56:20 UTC) #13
Manish Jethani
Patch Set 3: Use messaging to communicate with filterComposer https://codereview.adblockplus.org/29532767/diff/29533656/popup.html File popup.html (right): https://codereview.adblockplus.org/29532767/diff/29533656/popup.html#newcode25 popup.html:25: ...
Sept. 1, 2017, 5:17 p.m. (2017-09-01 17:17:57 UTC) #14
Manish Jethani
Patch Set 4: Use chrome APIs instead of ext.pages
Sept. 1, 2017, 6:43 p.m. (2017-09-01 18:43:19 UTC) #15
Sebastian Noack
https://codereview.adblockplus.org/29532767/diff/29533671/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29532767/diff/29533671/ext/popup.js#newcode29 ext/popup.js:29: }; While moving this function around anyway, how about ...
Sept. 13, 2017, 2:49 a.m. (2017-09-13 02:49:39 UTC) #16
Manish Jethani
Patch Set 5: Move functions into popup.js https://codereview.adblockplus.org/29532767/diff/29533671/ext/popup.js File ext/popup.js (right): https://codereview.adblockplus.org/29532767/diff/29533671/ext/popup.js#newcode29 ext/popup.js:29: }; On ...
Sept. 18, 2017, 2:25 a.m. (2017-09-18 02:25:51 UTC) #17
kzar
(Please at least Cc me for platform reviews, I've added myself now.) The way that ...
Sept. 18, 2017, 11:08 a.m. (2017-09-18 11:08:09 UTC) #18
Wladimir Palant
I still think that this change needs to be finished and first, removing other requires ...
Sept. 18, 2017, 12:01 p.m. (2017-09-18 12:01:28 UTC) #19
kzar
On 2017/09/18 12:01:28, Wladimir Palant wrote: > I still think that this change needs to ...
Sept. 18, 2017, 1:23 p.m. (2017-09-18 13:23:09 UTC) #20
Manish Jethani
Patch Set 6: Check for page ready again after adding listener https://codereview.adblockplus.org/29532767/diff/29547558/popup.js File popup.js (right): ...
Sept. 19, 2017, 8:59 a.m. (2017-09-19 08:59:13 UTC) #21
Manish Jethani
Patch Set 6: Use promises for page ready check Same, but use promises instead.
Sept. 19, 2017, 9:24 a.m. (2017-09-19 09:24:44 UTC) #22
Manish Jethani
On 2017/09/19 09:24:44, Manish Jethani wrote: > Patch Set 6: Use promises for page ready ...
Sept. 19, 2017, 9:25 a.m. (2017-09-19 09:25:38 UTC) #23
Manish Jethani
Patch Set 8: Remove callback parameter
Sept. 19, 2017, 9:28 a.m. (2017-09-19 09:28:01 UTC) #24
Wladimir Palant
https://codereview.adblockplus.org/29532767/diff/29549595/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549595/popup.js#newcode91 popup.js:91: new Promise(resolve => assertPageReady(pageId).then(resolve)) You cannot just ignore the ...
Sept. 19, 2017, 10:17 a.m. (2017-09-19 10:17:14 UTC) #25
Manish Jethani
Patch Set 9: Remove odd use of promise rejection https://codereview.adblockplus.org/29532767/diff/29549595/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549595/popup.js#newcode91 popup.js:91: ...
Sept. 19, 2017, 10:22 a.m. (2017-09-19 10:22:50 UTC) #26
Manish Jethani
Patch Set 10: Check for page ready state only once https://codereview.adblockplus.org/29532767/diff/29549595/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549595/popup.js#newcode112 ...
Sept. 19, 2017, 10:35 a.m. (2017-09-19 10:35:33 UTC) #27
Manish Jethani
Patch Set 11: Remove listener on promise fulfillment https://codereview.adblockplus.org/29532767/diff/29549777/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549777/popup.js#newcode60 popup.js:60: ext.onMessage.addListener(listener ...
Sept. 19, 2017, 2:25 p.m. (2017-09-19 14:25:56 UTC) #28
Manish Jethani
Patch Set 12: Refactor whenPageReady https://codereview.adblockplus.org/29532767/diff/29549882/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/popup.js#newcode98 popup.js:98: promise.then(cleanUp, error => We ...
Sept. 19, 2017, 6:07 p.m. (2017-09-19 18:07:24 UTC) #29
Sebastian Noack
https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js File ext/popup.js (left): https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js#oldcode13 ext/popup.js:13: }; It seems you need to rebase. There is ...
Sept. 20, 2017, 6:57 p.m. (2017-09-20 18:57:03 UTC) #30
Manish Jethani
Patch Set 13: Simplify whenPageReady I'll address the other comments in the next update. https://codereview.adblockplus.org/29532767/diff/29549882/popup.js ...
Sept. 21, 2017, 4:54 a.m. (2017-09-21 04:54:00 UTC) #31
Manish Jethani
Patch Set 14: Add comment explaining createPageObject hack https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js File ext/popup.js (left): https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js#oldcode13 ext/popup.js:13: }; ...
Sept. 21, 2017, 6:11 a.m. (2017-09-21 06:11:17 UTC) #32
Manish Jethani
Patch Set 15: Change whitelisting calls to use messaging Most uses of require are gone, ...
Sept. 21, 2017, 8:02 a.m. (2017-09-21 08:02:39 UTC) #33
Manish Jethani
https://codereview.adblockplus.org/29532767/diff/29551564/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29551564/popup.js#newcode24 popup.js:24: function getPref(key, callback) By the way, since we moved ...
Sept. 21, 2017, 8:06 a.m. (2017-09-21 08:06:25 UTC) #34
Sebastian Noack
https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js File ext/popup.js (left): https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js#oldcode13 ext/popup.js:13: }; On 2017/09/21 06:11:16, Manish Jethani wrote: > On ...
Sept. 21, 2017, 10:57 p.m. (2017-09-21 22:57:18 UTC) #35
Manish Jethani
Patch Set 16: Rebase on next https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js File ext/popup.js (left): https://codereview.adblockplus.org/29532767/diff/29549882/ext/popup.js#oldcode13 ext/popup.js:13: }; On 2017/09/21 ...
Sept. 24, 2017, 9:25 p.m. (2017-09-24 21:25:33 UTC) #36
Manish Jethani
Patch Set 17: Fix lint error and simplify code https://codereview.adblockplus.org/29532767/diff/29549882/notification.js File notification.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/notification.js#newcode85 notification.js:85: ...
Sept. 24, 2017, 10:37 p.m. (2017-09-24 22:37:28 UTC) #37
Sebastian Noack
https://codereview.adblockplus.org/29532767/diff/29549882/notification.js File notification.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/notification.js#newcode85 notification.js:85: chrome.tabs.create({url: link.href}); On 2017/09/24 22:37:23, Manish Jethani wrote: > ...
Sept. 25, 2017, 5:50 p.m. (2017-09-25 17:50:53 UTC) #38
Manish Jethani
Patch Set 18: Revert to using ext.pages.open https://codereview.adblockplus.org/29532767/diff/29549882/notification.js File notification.js (right): https://codereview.adblockplus.org/29532767/diff/29549882/notification.js#newcode85 notification.js:85: chrome.tabs.create({url: link.href}); ...
Sept. 26, 2017, 11:13 p.m. (2017-09-26 23:13:27 UTC) #39
Manish Jethani
Patch Set 19: Move require to notification.js https://codereview.adblockplus.org/29532767/diff/29556789/popup.js File popup.js (right): https://codereview.adblockplus.org/29532767/diff/29556789/popup.js#newcode20 popup.js:20: const {require} ...
Sept. 26, 2017, 11:22 p.m. (2017-09-26 23:22:09 UTC) #40
Sebastian Noack
LGTM
Sept. 27, 2017, 12:35 a.m. (2017-09-27 00:35:49 UTC) #41
Manish Jethani
Patch Set 20: Revert one more instance of ext.pages.open https://codereview.adblockplus.org/29532767/diff/29557569/stats.js File stats.js (right): https://codereview.adblockplus.org/29532767/diff/29557569/stats.js#newcode136 stats.js:136: ...
Sept. 27, 2017, 9:42 a.m. (2017-09-27 09:42:32 UTC) #42
Sebastian Noack
Sept. 27, 2017, 9:06 p.m. (2017-09-27 21:06:22 UTC) #43
LGTM

Powered by Google App Engine
This is Rietveld