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

Issue 29331940: Issue 3225 - Make Diagnostics compatible with the upcoming Adblock Plus 2.7 release (Closed)

Created:
Dec. 4, 2015, 3:28 p.m. by Wladimir Palant
Modified:
Dec. 7, 2015, 5:24 p.m.
Reviewers:
Thomas Greiner
CC:
Erik
Visibility:
Public.

Description

There is a number of changes here: 1) No longer hooking PolicyImplementation.shouldLoad, this functionality moved to a different process and isn`t easily accessible. 2) Because of that, remove the context, document and additional info columns, we no longer have that kind of unprocessed data. 3) Because of that, remove the early returns filter - we can no longer recognize those. 4) Hook Policy.shouldAllow instead of Policy.processNode. The consequences are mostly that we don`t need any additional processing of the parameters, but the return value changes as well. 5) Change the Origin column from a single line to multiple lines, the tooltip should show the entire frame hierarchy. Changed setMultilineContent function to support this. 6) Given that Diagnostics will only work with ABP 2.7 now and not with the older releases, changed the compatibility info to match.

Patch Set 1 #

Total comments: 1

Patch Set 2 : Track filter hits properly #

Total comments: 4

Patch Set 3 : Make currentData a local variable and removed unnecessary try block #

Unified diffs Side-by-side diffs Delta from patch set Stats (+53 lines, -220 lines) Patch
M chrome/content/watcher.js View 1 2 8 chunks +46 lines, -196 lines 0 comments Download
M chrome/content/watcher.xul View 1 chunk +0 lines, -8 lines 0 comments Download
M chrome/locale/en-US/global.properties View 1 chunk +0 lines, -10 lines 0 comments Download
M chrome/locale/en-US/watcher.dtd View 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/skin/watcher.css View 1 chunk +4 lines, -0 lines 0 comments Download
M metadata.gecko View 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 5
Wladimir Palant
Dec. 4, 2015, 3:28 p.m. (2015-12-04 15:28:58 UTC) #1
Wladimir Palant
https://codereview.adblockplus.org/29331940/diff/29331941/chrome/content/watcher.js File chrome/content/watcher.js (right): https://codereview.adblockplus.org/29331940/diff/29331941/chrome/content/watcher.js#newcode51 chrome/content/watcher.js:51: notifier = new RequestNotifier(null, handleFilterHit); I realized that I ...
Dec. 7, 2015, 12:17 p.m. (2015-12-07 12:17:55 UTC) #2
Thomas Greiner
https://codereview.adblockplus.org/29331940/diff/29332026/chrome/content/watcher.js File chrome/content/watcher.js (left): https://codereview.adblockplus.org/29331940/diff/29332026/chrome/content/watcher.js#oldcode195 chrome/content/watcher.js:195: entry.cols.type = String(Policy.localizedDescr[entry.type]); I know that it is no ...
Dec. 7, 2015, 3:24 p.m. (2015-12-07 15:24:20 UTC) #3
Wladimir Palant
https://codereview.adblockplus.org/29331940/diff/29332026/chrome/content/watcher.js File chrome/content/watcher.js (left): https://codereview.adblockplus.org/29331940/diff/29332026/chrome/content/watcher.js#oldcode195 chrome/content/watcher.js:195: entry.cols.type = String(Policy.localizedDescr[entry.type]); On 2015/12/07 15:24:20, Thomas Greiner wrote: ...
Dec. 7, 2015, 3:37 p.m. (2015-12-07 15:37:55 UTC) #4
Thomas Greiner
Dec. 7, 2015, 3:41 p.m. (2015-12-07 15:41:55 UTC) #5
LGTM

Powered by Google App Engine
This is Rietveld