Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(306)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 377753003: Remove GetContexts() from the public interface of extensions::Feature. It was (Closed)

Created:
6 years, 5 months ago by not at google - send to devlin
Modified:
6 years, 5 months ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Project:
chromium
Visibility:
Public.

Description

Remove GetContexts() from the public interface of extensions::Feature. It was being misused in such a way that makes it impossible to add other trusted extension context types. It's also a nice cleanup, though requires rewriting the ExtensionAPI::IsPrivileged function with sightly different semantics. BUG=391944 R=rockot@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282437

Patch Set 1 #

Total comments: 7

Patch Set 2 : fix comment #

Patch Set 3 : test fixes #

Patch Set 4 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+133 lines, -112 lines) Patch
M chrome/common/extensions/api/extension_api_unittest.cc View 1 2 3 chunks +64 lines, -33 lines 0 comments Download
M chrome/test/data/extensions/extension_api_unittest/privileged_api_features.json View 3 chunks +11 lines, -6 lines 0 comments Download
M extensions/browser/event_router.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M extensions/browser/extension_function_dispatcher.cc View 1 chunk +1 line, -1 line 0 comments Download
M extensions/common/extension_api.h View 1 chunk +4 lines, -5 lines 0 comments Download
M extensions/common/extension_api.cc View 2 chunks +12 lines, -7 lines 0 comments Download
M extensions/common/extension_api_stub.cc View 1 chunk +2 lines, -1 line 0 comments Download
M extensions/common/features/api_feature.cc View 1 chunk +1 line, -1 line 0 comments Download
M extensions/common/features/complex_feature.h View 1 chunk +0 lines, -2 lines 0 comments Download
M extensions/common/features/complex_feature.cc View 1 2 chunks +4 lines, -15 lines 0 comments Download
M extensions/common/features/feature.h View 1 chunk +0 lines, -2 lines 0 comments Download
M extensions/common/features/manifest_feature.cc View 1 chunk +1 line, -1 line 0 comments Download
M extensions/common/features/permission_feature.cc View 1 chunk +1 line, -1 line 0 comments Download
M extensions/common/features/simple_feature.h View 2 chunks +1 line, -2 lines 0 comments Download
M extensions/common/features/simple_feature.cc View 1 2 3 chunks +9 lines, -9 lines 0 comments Download
M extensions/common/features/simple_feature_unittest.cc View 9 chunks +19 lines, -24 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
not at google - send to devlin
https://codereview.chromium.org/377753003/diff/1/chrome/test/data/extensions/extension_api_unittest/privileged_api_features.json File chrome/test/data/extensions/extension_api_unittest/privileged_api_features.json (right): https://codereview.chromium.org/377753003/diff/1/chrome/test/data/extensions/extension_api_unittest/privileged_api_features.json#newcode12 chrome/test/data/extensions/extension_api_unittest/privileged_api_features.json:12: "contexts": ["blessed_extension"] the dependencies are broken, and I will ...
6 years, 5 months ago (2014-07-07 22:25:05 UTC) #1
not at google - send to devlin
ping
6 years, 5 months ago (2014-07-08 22:00:06 UTC) #2
not at google - send to devlin
maybe yoz@ is OOO? I haven't seen him. Ken can you take a look?
6 years, 5 months ago (2014-07-09 03:06:24 UTC) #3
Ken Rockot(use gerrit already)
lgtm https://codereview.chromium.org/377753003/diff/1/extensions/common/extension_api.cc File extensions/common/extension_api.cc (right): https://codereview.chromium.org/377753003/diff/1/extensions/common/extension_api.cc#newcode295 extensions/common/extension_api.cc:295: return IsAvailable(name, extension, Feature::CONTENT_SCRIPT_CONTEXT, GURL()) Heh, this is ...
6 years, 5 months ago (2014-07-09 16:24:16 UTC) #4
not at google - send to devlin
https://codereview.chromium.org/377753003/diff/1/extensions/common/extension_api.cc File extensions/common/extension_api.cc (right): https://codereview.chromium.org/377753003/diff/1/extensions/common/extension_api.cc#newcode295 extensions/common/extension_api.cc:295: return IsAvailable(name, extension, Feature::CONTENT_SCRIPT_CONTEXT, GURL()) On 2014/07/09 16:24:15, Ken ...
6 years, 5 months ago (2014-07-09 16:26:26 UTC) #5
not at google - send to devlin
The CQ bit was checked by kalman@chromium.org
6 years, 5 months ago (2014-07-09 16:26:30 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kalman@chromium.org/377753003/1
6 years, 5 months ago (2014-07-09 16:27:16 UTC) #7
Yoyo Zhou
Sorry, apparently I forgot to hit send. LGTM https://codereview.chromium.org/377753003/diff/1/extensions/common/features/complex_feature.cc File extensions/common/features/complex_feature.cc (right): https://codereview.chromium.org/377753003/diff/1/extensions/common/features/complex_feature.cc#newcode15 extensions/common/features/complex_feature.cc:15: // ...
6 years, 5 months ago (2014-07-09 17:06:18 UTC) #8
not at google - send to devlin
no problem https://codereview.chromium.org/377753003/diff/1/extensions/common/features/complex_feature.cc File extensions/common/features/complex_feature.cc (right): https://codereview.chromium.org/377753003/diff/1/extensions/common/features/complex_feature.cc#newcode15 extensions/common/features/complex_feature.cc:15: // Verify IsInternal, & IsBlockedInServiceWorker are consistent ...
6 years, 5 months ago (2014-07-09 17:12:00 UTC) #9
not at google - send to devlin
The CQ bit was checked by kalman@chromium.org
6 years, 5 months ago (2014-07-09 17:12:02 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kalman@chromium.org/377753003/20001
6 years, 5 months ago (2014-07-09 17:13:53 UTC) #11
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium ...
6 years, 5 months ago (2014-07-09 20:58:50 UTC) #12
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 5 months ago (2014-07-09 22:08:13 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/builds/49984)
6 years, 5 months ago (2014-07-09 22:08:14 UTC) #14
not at google - send to devlin
The CQ bit was checked by kalman@chromium.org
6 years, 5 months ago (2014-07-10 16:18:38 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kalman@chromium.org/377753003/40001
6 years, 5 months ago (2014-07-10 16:20:06 UTC) #16
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_gn_rel on tryserver.chromium ...
6 years, 5 months ago (2014-07-10 20:47:20 UTC) #17
commit-bot: I haz the power
6 years, 5 months ago (2014-07-10 21:47:00 UTC) #18
Message was sent while issue was closed. 
Change committed as 282437

Powered by Google App Engine
This is Rietveld 408576698