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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 6 months ago by Wladimir Palant
Modified:
4 years, 6 months ago
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
4 years, 6 months ago (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 ...
4 years, 6 months ago (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 ...
4 years, 6 months ago (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: ...
4 years, 6 months ago (2015-12-07 15:37:55 UTC) #4
Thomas Greiner
4 years, 6 months ago (2015-12-07 15:41:55 UTC) #5
LGTM
Sign in to reply to this message.

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