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

Issue 29863604: Issue 6865 - Update ABP dependency to version 3.2

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 4 days ago by diegocarloslima
Modified:
3 weeks, 1 day ago
Reviewers:
jens, Thomas Greiner, anton
Visibility:
Public.

Description

Issue 6865 - Update ABP dependency to version 3.2

Patch Set 1 #

Patch Set 2 : Removing patch file #

Total comments: 22

Patch Set 3 : Adjustments based on comments #

Patch Set 4 : Removing allowed contexts #

Unified diffs Side-by-side diffs Delta from patch set Stats (+390 lines, -462 lines) Patch
M adblockplus/build.py View 1 2 1 chunk +21 lines, -16 lines 0 comments Download
M adblockplus/extensionBridge.js View 1 2 1 chunk +177 lines, -311 lines 0 comments Download
M adblockplus/issue-6070.patch View 2 chunks +6 lines, -6 lines 0 comments Download
R adblockplus/issue-6108.patch View 1 1 chunk +0 lines, -42 lines 0 comments Download
M adblockplus/metadata.gecko View 1 chunk +3 lines, -3 lines 0 comments Download
M dependencies View 1 chunk +2 lines, -2 lines 0 comments Download
M ensure_dependencies.py View 7 chunks +75 lines, -8 lines 0 comments Download
M mobile/android/base/java/org/adblockplus/browser/AbpCheckBoxPreference.java View 1 chunk +2 lines, -2 lines 0 comments Download
M mobile/android/base/java/org/adblockplus/browser/BrowserAppUtils.java View 2 chunks +2 lines, -2 lines 0 comments Download
M mobile/android/base/java/org/adblockplus/browser/ExtensionBridge.java View 5 chunks +18 lines, -18 lines 0 comments Download
M mobile/android/base/java/org/adblockplus/browser/MoreSubscriptionsPreferenceGroup.java View 5 chunks +9 lines, -9 lines 0 comments Download
M mobile/android/base/java/org/adblockplus/browser/SubscriptionContainer.java View 5 chunks +21 lines, -28 lines 0 comments Download
M mobile/android/base/java/org/adblockplus/browser/SubscriptionPreferenceCategory.java View 3 chunks +4 lines, -5 lines 0 comments Download
M mobile/android/base/java/org/adblockplus/browser/WhitelistedWebsitesPreferenceGroup.java View 3 chunks +3 lines, -3 lines 0 comments Download
M mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java View 1 chunk +2 lines, -2 lines 0 comments Download
M mobile/android/base/moz.build View 1 chunk +1 line, -1 line 0 comments Download
M mobile/android/chrome/content/browser.js View 1 chunk +0 lines, -4 lines 0 comments Download
M toolkit/components/extensions/ext-c-runtime.js View 1 2 1 chunk +22 lines, -0 lines 0 comments Download
M toolkit/components/extensions/schemas/runtime.json View 1 2 3 1 chunk +22 lines, -0 lines 0 comments Download

Messages

Total messages: 5
diegocarloslima
3 weeks, 4 days ago (2018-08-24 20:44:09 UTC) #1
anton
https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensionBridge.js File adblockplus/extensionBridge.js (right): https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensionBridge.js#newcode53 adblockplus/extensionBridge.js:53: do we need this empty line? https://codereview.adblockplus.org/29863604/diff/29863624/mobile/android/base/java/org/adblockplus/browser/SubscriptionContainer.java File mobile/android/base/java/org/adblockplus/browser/SubscriptionContainer.java ...
3 weeks, 2 days ago (2018-08-27 05:57:10 UTC) #2
Thomas Greiner
https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/build.py File adblockplus/build.py (right): https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/build.py#newcode33 adblockplus/build.py:33: "subscriptions.xml") The canonical version of subscriptions.xml is located in ...
3 weeks, 2 days ago (2018-08-27 15:24:42 UTC) #3
diegocarloslima
https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/build.py File adblockplus/build.py (right): https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/build.py#newcode33 adblockplus/build.py:33: "subscriptions.xml") On 2018/08/27 15:24:41, Thomas Greiner wrote: > The ...
3 weeks, 1 day ago (2018-08-27 21:19:17 UTC) #4
Thomas Greiner
3 weeks, 1 day ago (2018-08-28 10:36:07 UTC) #5
https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/...
File toolkit/components/extensions/schemas/runtime.json (right):

https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/...
toolkit/components/extensions/schemas/runtime.json:164: "allowedContexts":
["content", "devtools"],
On 2018/08/27 21:19:17, diegocarloslima wrote:
> If I dont add at least the 'content' context, I cant actually call those
> functions from extensionBridge.js .

That's odd because "content" seems to refer to content scripts (i.e. the
extension code that is running in the context of a web page) and not to the
background page which we want to communicate with.

Unfortunately, I couldn't find a proper documentation for it other than what's
mentioned at
https://wiki.mozilla.org/WebExtensions/Implementing_APIs_out-of-process#shoul....
But there appear to be other API functions which don't specify the
"allowedContexts" property (e.g.
https://github.com/mozilla/gecko-dev/blob/master/toolkit/components/extension...).

> I understand that it could be a potential
> security breach, but since we're in control of the extensions that are
installed
> on ABB (for instance, we only install ABP), this shouldnt be a big concern.
For
> the 'devtools' context, it might not be needed, just added because all the
other
> functions had it, so I didnt want to have some unexpected edge case behavior

Note that it's not about whether a different extension could use this API but
about us exposing an API to a part of our extension that is directly interacting
with untrusted content (i.e. the web page). So it'd be great if we could somehow
limit our APIs to only the background page to sandbox it further.

https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/...
toolkit/components/extensions/schemas/runtime.json:165: "description": "Function
required to listen to Android requests. See
https://issues.adblockplus.org/ticket/6865",
On 2018/08/27 21:19:17, diegocarloslima wrote:
> I will rename 'abbSendRequest' to 'sendAbbMessage', but for the listener, I
> prefer to keep it as a function, if I add it as another property that has a
> function, it would require more changes in the upstream code than the actual
> solution, which is not desired. I will keep it as a function, but will rename
it
> to 'registerAbbMessageListener'

Sounds good. :)
Sign in to reply to this message.

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