[Extensions] Implement scripting.registercontentscripts [chromium/src : main]
Kelvin Jiang (Gerrit)
Attention is currently required from: Devlin.
Kelvin Jiang would like Devlin to review this change.
[Extensions] Implement scripting.registercontentscripts
This CL implements scripting.registerContentScripts, which is restricted
to MV3. Currently, registered content scripts are session scoped only.
Unlike manifest content scripts, the extension must specify a distinct ID
for each dynamic script.
Since the RegisteredContentScript type in scripting.idl shares a lot of
common logic wrt parsing/validating files as manifest content scripts,
common methods have been extracted into content_script_utils.
Bug: 1168627
Change-Id: I0e842230917ce27052968719f3af71311d418ca2
---
M chrome/browser/extensions/api/scripting/scripting_api.cc
M chrome/browser/extensions/api/scripting/scripting_api.h
M chrome/browser/extensions/api/scripting/scripting_apitest.cc
M chrome/browser/extensions/content_script_tracker_browsertest.cc
M chrome/common/extensions/api/_api_features.json
M chrome/common/extensions/api/scripting.idl
A chrome/test/data/extensions/api_test/scripting/dynamic_scripts/manifest.json
A chrome/test/data/extensions/api_test/scripting/dynamic_scripts/script.js
A chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js
M extensions/browser/content_script_tracker.cc
M extensions/browser/extension_function_histogram_value.h
M extensions/browser/user_script_manager.cc
M extensions/browser/user_script_manager.h
M extensions/common/BUILD.gn
M extensions/common/manifest_handlers/content_scripts_handler.cc
M extensions/common/user_script.cc
M extensions/common/user_script.h
A extensions/common/utils/content_script_utils.cc
A extensions/common/utils/content_script_utils.h
M extensions/renderer/user_script_set.cc
M tools/metrics/histograms/enums.xml
21 files changed, 1,036 insertions(+), 198 deletions(-)
To view, visit change 2892136. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Devlin.
Patch set 11:Commit-Queue +1
Devlin (Gerrit)
Attention is currently required from: Kelvin Jiang.
16 comments:
Commit Message:
Patch Set #11, Line 12:
Unlike manifest content scripts, the extension must specify a distinct IDnit: please wrap at 72 chars
Patchset:
Thanks, Kelvin! This looks really, really good!
I took a high-level pass here to pick out some of the larger pieces that might require a bit of code shift (and also anything I was worried I'd forget later ; )). I'll take another pass next week.
File chrome/browser/extensions/api/scripting/scripting_api.cc:
What if ID is empty?
Patch Set #11, Line 689:
weak_factory_.GetWeakPtr()nit: since we AddRef() here, no need in having the WeakPtrFactory. We can just pass `this` directly.
File chrome/common/extensions/api/_api_features.json:
Patch Set #11, Line 741:
"min_manifest_version": 3let's also restrict to channel: canary since the API will still be WIP after this CL (e.g., for adding extra testing, other methods, etc)
That will also mean we need to add a ScopedCurrentChannel in the API test.
File chrome/common/extensions/api/scripting.idl:
Patch Set #11, Line 19:
enum RunAt {can we instead use extensionTypes.RunAt, or does that cause issues?
Patch Set #11, Line 113:
dictionary RegisteredContentScript {while these are in development, we should add [nodoc] to these types and methods
Patch Set #11, Line 124:
exclude_matchesnit: Since these are now JS objects, they should use jsNamingStyle
// Whether the script should inject into an about:blank frame where the
// parent or opener frame matches one of the patterns declared in matches.
// Defaults to false.
boolean? match_about_blank;Let's remove this one, since it's replaced by the one above
// Applied after matches to include only those URLs that also match this
// glob. Intended to emulate the
// <a href="http://wiki.greasespot.net/Metadata_Block#.40include">@include
// </a> Greasemonkey keyword.
DOMString[]? include_globs;
// Applied after matches to exclude URLs that match this glob. Intended to
// emulate the
// <a href="https://wiki.greasespot.net/Metadata_Block#.40exclude">@exclude
// </a> Greasemonkey keyword.
DOMString[]? exclude_globs;I know we chatted about whether to include these in this first version of the API. Did we decide they were necessary?
Patch Set #11, Line 203:
RegisteredContentScript[] scripts,Probably should have caught this in the API doc, but is it worth wrapping this in a dictionary so that we can add additional properties later (such as scope for session vs persistent)?
File extensions/browser/content_script_tracker.cc:
const UserScriptList& dynamic_scripts =
manager == nullptr
? g_empty_script_list.Get().user_script_list
: manager->GetLoadedDynamicExtensionScripts(extension.id());nit1: When do we expect manager to be null? (Add a comment if it can happen)
nit2: Rather than the global empty list, I feel like this would be cleaner by just guarding with an if:if (any_of(manifest_scripts, match))
return true;
if (manager) {
dynamic_scripts = manager->GetScripts();
if (any_of(dynamic_scripts, match))
return true;
}
return false;
File extensions/browser/user_script_manager.h:
Maybe s/New/Pending?
// The IDs of scripts registered by the extension's API calls but have not
// beeen loaded yet. When an registerContentScripts call is made, script IDs
// are put into this set. They are removed from the set when the API call
// encounters an error or if the scripts from that call have been loaded.
std::set<std::string> pending_dynamic_script_ids;
// The metadata of dynamic scripts from the extension that have been loaded.
UserScriptList loaded_dynamic_scripts;
std::unique_ptr<ExtensionUserScriptLoader> loader;Rather than storing all this extension-specific metadata in a separate struct in the USM, would it make sense to expose on the ExtensionUserScriptLoader? That's already per-extension and contains (through USL) a bunch of the "stored script"-style info. It also seems like that would keep this manager a bit cleaner as the "manager of each loader" rather than being a pseudo-loader itself.
WDYT?
File extensions/common/utils/content_script_utils.h:
Patch Set #11, Line 22:
const std::unique_ptr<bool>&rather than `const std::unique_ptr<T>&`, typically prefer `const T*` (if possibly null) or `const T&` (if definitely not null)
Patch Set #11, Line 22:
void ParseMatchFlags(const std::unique_ptr<bool>& match_origin_as_fallback,add comments on the rest of these functions, too
To view, visit change 2892136. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Kelvin Jiang.
Kelvin Jiang uploaded patch set #12 to this change.
[Extensions] Implement scripting.registercontentscripts
This CL implements scripting.registerContentScripts, which is restricted
to MV3. Currently, registered content scripts are session scoped only.
Unlike manifest content scripts, the extension must specify a distinct
ID for each dynamic script.
To view, visit change 2892136. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Devlin.
Patch set 14:Commit-Queue +1
16 comments:
Commit Message:
Patch Set #11, Line 12:
Unlike manifest content scripts, the extension must specify a distinct IDnit: please wrap at 72 chars
Done
File chrome/browser/extensions/api/scripting/scripting_api.cc:
What if ID is empty?
then it shouldn't be a valid script. Going to add a test for this too
Patch Set #11, Line 689:
weak_factory_.GetWeakPtr()
nit: since we AddRef() here, no need in having the WeakPtrFactory. […]
Done
Patch Set #11, Line 689:
weak_factory_.GetWeakPtr()
nit: since we AddRef() here, no need in having the WeakPtrFactory. […]
Done
File chrome/common/extensions/api/_api_features.json:
Patch Set #11, Line 741:
"min_manifest_version": 3
let's also restrict to channel: canary since the API will still be WIP after this CL (e.g. […]
oops I forgot to restrict this to channel: trunk
The API test already restricts it to channel::UNKNOWN (which I think is trunk)
File chrome/common/extensions/api/scripting.idl:
Patch Set #11, Line 19:
enum RunAt {
can we instead use extensionTypes. […]
we can (tested by making this change, then adding this property to the test)
Patch Set #11, Line 113:
dictionary RegisteredContentScript {while these are in development, we should add [nodoc] to these types and methods
Done
Patch Set #11, Line 124:
exclude_matchesnit: Since these are now JS objects, they should use jsNamingStyle
Done
// Whether the script should inject into an about:blank frame where the
// parent or opener frame matches one of the patterns declared in matches.
// Defaults to false.
boolean? match_about_blank;Let's remove this one, since it's replaced by the one above
Done
// Applied after matches to include only those URLs that also match this
// glob. Intended to emulate the
// <a href="http://wiki.greasespot.net/Metadata_Block#.40include">@include
// </a> Greasemonkey keyword.
DOMString[]? include_globs;
// Applied after matches to exclude URLs that match this glob. Intended to
// emulate the
// <a href="https://wiki.greasespot.net/Metadata_Block#.40exclude">@exclude
// </a> Greasemonkey keyword.
DOMString[]? exclude_globs;
I know we chatted about whether to include these in this first version of the API. […]
We decided to not include them until running extplorer shows that they're used often enough to warrant including it in a follow up
Patch Set #11, Line 203:
RegisteredContentScript[] scripts,
Probably should have caught this in the API doc, but is it worth wrapping this in a dictionary so th […]
I am leaning towards including additional properties (iirc there wouldn't be many) inside RegisteredContentScript because it would make the API look cleaner)
File extensions/browser/content_script_tracker.cc:
const UserScriptList& dynamic_scripts =
manager == nullptr
? g_empty_script_list.Get().user_script_list
: manager->GetLoadedDynamicExtensionScripts(extension.id());
nit1: When do we expect manager to be null? (Add a comment if it can happen) […]
Done
File extensions/browser/user_script_manager.h:
Maybe s/New/Pending?
Done
// The IDs of scripts registered by the extension's API calls but have not
// beeen loaded yet. When an registerContentScripts call is made, script IDs
// are put into this set. They are removed from the set when the API call
// encounters an error or if the scripts from that call have been loaded.
std::set<std::string> pending_dynamic_script_ids;
// The metadata of dynamic scripts from the extension that have been loaded.
UserScriptList loaded_dynamic_scripts;
std::unique_ptr<ExtensionUserScriptLoader> loader;
Rather than storing all this extension-specific metadata in a separate struct in the USM, would it m […]
I moved all the methods/state into the EUSL.
Actually resulted in less LOC too/made USM easier to reason about, thanks
File extensions/common/utils/content_script_utils.h:
Patch Set #11, Line 22:
const std::unique_ptr<bool>&
rather than `const std::unique_ptr<T>&`, typically prefer `const T*` (if possibly null) or `const T& […]
Done
Patch Set #11, Line 22:
void ParseMatchFlags(const std::unique_ptr<bool>& match_origin_as_fallback,add comments on the rest of these functions, too
Done
To view, visit change 2892136. To unsubscribe, or for help writing mail filters, visit settings.
Devlin (Gerrit)
Attention is currently required from: Kelvin Jiang.
33 comments:
Patchset:
Thanks, Kelvin! This is shaping up nicely.
Lots of nits, but I think most should be very straightforward.
File chrome/browser/extensions/api/scripting/scripting_api.h:
Patch Set #14, Line 13:
#include "extensions/browser/extension_system.h"nit: needed?
Patch Set #14, Line 100:
using ValidateContentScriptsResult =nit: Can we put this in the private section of the API function?
File chrome/browser/extensions/api/scripting/scripting_api.cc:
Patch Set #14, Line 37:
#include "extensions/common/api/extension_types.h"nit: out of place
Patch Set #14, Line 231:
mojom::RunLocation ConvertRunLocation(api::extension_types::RunAt run_at) {We do this same thing in ExecuteCodeFunction. Let's pull it into a common area where we can share the code.
nit: pass by const& if it can't be null
Patch Set #14, Line 255:
// idnit: I know that the user script parsing code does this, but it's not really necessary here (where the variables are very clear). We can probably remove these comments
Patch Set #14, Line 636:
->GetUserScriptLoaderForExtension(extension()->id())We use this again on line 684. Let's cache it in a local.
const bool can_execute_script_everywhere =
PermissionsData::CanExecuteScriptEverywhere(extension()->id(),
extension()->location());
const int valid_schemes =
UserScript::ValidUserScriptSchemes(can_execute_script_everywhere);
const bool all_urls_includes_chrome_urls =
PermissionsData::AllUrlsIncludesChromeUrls(extension()->id());This is a bit of an "ugly" aspect of the permissions API, and is really only used for the legacy version of Chromevox. How about we leave this out for now (and assume that can_execute_everywhere is always false), and add a comment that this _could_ be added in later if needed?
std::set<std::string> new_script_ids;
for (size_t i = 0; i < scripts.size(); ++i) {
// Parse/Create user script.
std::unique_ptr<UserScript> user_script = ParseUserScript(
extension(), scripts[i], i, can_execute_script_everywhere,
valid_schemes, all_urls_includes_chrome_urls, &parse_error);
if (!user_script)
return RespondNow(Error(base::UTF16ToASCII(parse_error)));
new_script_ids.insert(user_script->id());
parsed_scripts->push_back(std::move(user_script));
}Optional: We could push this into the loop above. It's slower in the case that there is an error in IDs, but we don't usually optimize for error cases. But, it's not any slower this way, so I don't have a strong preference.
In either case, I'd slightly prefer we build new_script_ids up in the previous (and perhaps only) loop, and just check both for dupes:
if (base::Contains(previous_script_ids, script.id) ||
base::Contains(new_script_ids, script_id)) {
return Error(...);
}new_script_ids.insert(script.id);
Patch Set #14, Line 695:
AddRef();Add a
// Balanced in...
comment (ditto for the Release()s)
File chrome/common/extensions/api/scripting.idl:
Patch Set #11, Line 203:
RegisteredContentScript[] scripts,I am leaning towards including additional properties (iirc there wouldn't be many) inside Registered […]
Ack
File chrome/common/extensions/api/scripting.idl:
Patch Set #14, Line 91:
// Describes a content script to be injected into a web page registered throughnit: wrap at 80 (also the rest of this file)
File chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js:
Patch Set #14, Line 18:
if (message === 'SCRIPT_INJECTED') {Do we expect any other message? If not, let's
chrome.test.assertEquals('SCRIPT_INJECTED', message);
Patch Set #14, Line 24:
const results = await Promise.all([optional nit: this is fun (and is actually mildly more efficient), but I think it's a hair less readable than:
await chrome.scripting.registerContentScripts(scripts);
const config = await chrome.test.getConfig();Patch Set #14, Line 32:
awaitnit: no need for an await here. And it could _potentially_ lead to slightly weird behavior, since we might still be waiting on it after the message comes in (depending on injection timing)
Patch Set #14, Line 37:
duplicateScripts1nit: test1, test2, test3 naming style isn't as descriptive, and forces the reader to comb into the test to figure out the differences. Let's instead add some more details here, e.g.
duplicateScriptId_DuplicatesInSameCall
duplicateScriptId_DuplicateInPendingRegistration
duplicateScriptId_DuplicatePreviouslyRegisteredPatch Set #14, Line 38:
const scriptId = 'script1';nit: let's maybe just add a quick comment:
// This was registered in the previous test.
Patch Set #14, Line 41:
scriptIdnit: this would technically fail anyway, since the scriptId was already registered. Let's make scriptId something different in this test.
Patch Set #14, Line 63:
chrome.test.assertEq(undefined, chrome.runtime.lastError);nit: let's still use a promise here, just without the await. (And then we can Promise.all() with the below, rather than "first and second callback finished")
Patch Set #14, Line 88:
chrome.test.assertEq(undefined, result);nit: probably don't need this.
Patch Set #14, Line 139:
// content scripts will only be injected once.Also permissions-related tests
File extensions/browser/content_script_tracker.cc:
Patch Set #14, Line 226:
std::any_ofnit: while we're here, let's use base::ranges::any_of (also below)
File extensions/browser/extension_user_script_loader.h:
Patch Set #14, Line 51:
the scripts to which the ids belong tonitty nit: extra "to" in here
either
"the scripts to which the ids belong"
or
"the scripts the ids belong to"
(Technically, ending in a preposition, but I ascribe to Churchill's supposed comment on that...)
// Removes `script_ids` from `pending_dynamic_script_ids_`. Should only be
// called when an API call is about to return with an error before attempting
// to load its scripts.
void RemovePendingDynamicScriptIDs(std::set<std::string> script_ids);nit: Since this is only for the error case, how about calling it "LoadFailedForPendingScripts()" or similar?
When an registerContentScripts call is made, script IDs
// are put into this set.
nitty nit: UserScriptLoader shouldn't care about registerContentScripts()
File extensions/browser/user_script_manager.h:
Patch Set #14, Line 38:
using DynamicScriptsModifiedCallback =nit: no longer needed
File extensions/browser/user_script_manager.cc:
Patch Set #14, Line 7:
#include <tuple>no longer needed
File extensions/common/user_script.h:
Patch Set #14, Line 35:
static const char kGeneratedIDPrefix;Add a comment
File extensions/common/user_script.cc:
Patch Set #14, Line 274:
return user_script_id_[0] == kGeneratedIDPrefix;Let's also CHECK(!user_script_id_.empty())
File extensions/common/utils/content_script_utils.h:
Patch Set #14, Line 1:
// Copyright 2021 The Chromium Authors. All rights reserved.Wild idea:
What if we took the data from the params in RegisterContentScriptsFunction and constructed a content_scripts_api::ContentScript out of it? RegisterContentScriptsFunction is a strict subset, and they both convert to a UserScript at the end of the day. Then, we could just have a CreateUserScript() function like the one in content_scripts_handler.cc and use it for both.
WDYT?
Patch Set #14, Line 15:
#include "extensions/common/mojom/run_location.mojom-shared.h"Are all of these necessary?
Patch Set #14, Line 21:
namespace extensions {Let's put these in an additional namespace, e.g. extensions::script_parsing
To view, visit change 2892136. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Devlin.
Patch set 18:Commit-Queue +1
33 comments:
Patchset:
ready (since last night)
File chrome/browser/extensions/api/scripting/scripting_api.h:
Patch Set #14, Line 13:
#include "extensions/browser/extension_system.h"nit: needed?
nope, removed
Patch Set #14, Line 100:
using ValidateContentScriptsResult =nit: Can we put this in the private section of the API function?
No because a helper function in the anonymous namespace returns this. (do you prefer this as-is vs moving helper function/this "using statement" to private)
File chrome/browser/extensions/api/scripting/scripting_api.cc:
Patch Set #14, Line 37:
#include "extensions/common/api/extension_types.h"nit: out of place
oops
Patch Set #14, Line 231:
mojom::RunLocation ConvertRunLocation(api::extension_types::RunAt run_at) {
We do this same thing in ExecuteCodeFunction. […]
Done
nit: pass by const& if it can't be null
Done
Patch Set #14, Line 255:
// id
nit: I know that the user script parsing code does this, but it's not really necessary here (where t […]
Done
Patch Set #14, Line 636:
->GetUserScriptLoaderForExtension(extension()->id())We use this again on line 684. Let's cache it in a local.
Done
const bool can_execute_script_everywhere =
PermissionsData::CanExecuteScriptEverywhere(extension()->id(),
extension()->location());
const int valid_schemes =
UserScript::ValidUserScriptSchemes(can_execute_script_everywhere);
const bool all_urls_includes_chrome_urls =
PermissionsData::AllUrlsIncludesChromeUrls(extension()->id());
This is a bit of an "ugly" aspect of the permissions API, and is really only used for the legacy ver […]
Done
std::set<std::string> new_script_ids;
for (size_t i = 0; i < scripts.size(); ++i) {
// Parse/Create user script.
std::unique_ptr<UserScript> user_script = ParseUserScript(
extension(), scripts[i], i, can_execute_script_everywhere,
valid_schemes, all_urls_includes_chrome_urls, &parse_error);
if (!user_script)
return RespondNow(Error(base::UTF16ToASCII(parse_error)));
new_script_ids.insert(user_script->id());
parsed_scripts->push_back(std::move(user_script));
}
Optional: We could push this into the loop above. […]
done (building new script ids in first loop)
Patch Set #14, Line 695:
AddRef();
Add a […]
Done
File chrome/common/extensions/api/scripting.idl:
Patch Set #14, Line 91:
// Describes a content script to be injected into a web page registered throughnit: wrap at 80 (also the rest of this file)
Done
File chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js:
Patch Set #14, Line 18:
if (message === 'SCRIPT_INJECTED') {
Do we expect any other message? If not, let's […]
Done
Patch Set #14, Line 24:
const results = await Promise.all([
optional nit: this is fun (and is actually mildly more efficient), but I think it's a hair less read […]
Done
Patch Set #14, Line 32:
await
nit: no need for an await here. […]
Done
Patch Set #14, Line 37:
duplicateScripts1
nit: test1, test2, test3 naming style isn't as descriptive, and forces the reader to comb into the t […]
Done
Patch Set #14, Line 38:
const scriptId = 'script1';
nit: let's maybe just add a quick comment: […]
noted, but since scriptId is different now for this test I don't think I need to mention this
Patch Set #14, Line 41:
scriptId
nit: this would technically fail anyway, since the scriptId was already registered. […]
Done
Patch Set #14, Line 63:
chrome.test.assertEq(undefined, chrome.runtime.lastError);
nit: let's still use a promise here, just without the await. (And then we can Promise. […]
Done
Patch Set #14, Line 63:
chrome.test.assertEq(undefined, chrome.runtime.lastError);
nit: let's still use a promise here, just without the await. (And then we can Promise. […]
Done
Patch Set #14, Line 88:
chrome.test.assertEq(undefined, result);nit: probably don't need this.
Done
Patch Set #14, Line 139:
// content scripts will only be injected once.Also permissions-related tests
Done
File extensions/browser/content_script_tracker.cc:
Patch Set #14, Line 226:
std::any_ofnit: while we're here, let's use base::ranges::any_of (also below)
Done
File extensions/browser/extension_user_script_loader.h:
Patch Set #14, Line 51:
the scripts to which the ids belong to
nitty nit: extra "to" in here […]
done (second)
// Removes `script_ids` from `pending_dynamic_script_ids_`. Should only be
// called when an API call is about to return with an error before attempting
// to load its scripts.
void RemovePendingDynamicScriptIDs(std::set<std::string> script_ids);
nit: Since this is only for the error case, how about calling it "LoadFailedForPendingScripts()" or […]
Pending scripts in this context can pertain to scripts that will be loaded, and scripts that are being loaded (but not complete).
I think I'll use AbortLoadForPendingScripts() since it should describe both of those cases
When an registerContentScripts call is made, script IDs
// are put into this set.nitty nit: UserScriptLoader shouldn't care about registerContentScripts()
Done
File extensions/browser/user_script_manager.h:
Patch Set #14, Line 38:
using DynamicScriptsModifiedCallback =nit: no longer needed
Done
File extensions/browser/user_script_manager.cc:
Patch Set #14, Line 7:
#include <tuple>no longer needed
Done
File extensions/common/user_script.h:
Patch Set #14, Line 35:
static const char kGeneratedIDPrefix;Add a comment
Done
File extensions/common/user_script.cc:
Patch Set #14, Line 274:
return user_script_id_[0] == kGeneratedIDPrefix;
Let's also CHECK(!user_script_id_. […]
Done
File extensions/common/utils/content_script_utils.h:
Patch Set #14, Line 1:
// Copyright 2021 The Chromium Authors. All rights reserved.
Wild idea: […]
This sounds nifty but at some point, the content script in RegisterContentScripts may diverge from content_script_api::ContentScript (especially with the persist_across_sessions flag), and the required id.
I would prefer keeping them as two similar but different types and converting both directly to a UserScript.
Patch Set #14, Line 15:
#include "extensions/common/mojom/run_location.mojom-shared.h"Are all of these necessary?
removed some
Patch Set #14, Line 21:
namespace extensions {
Let's put these in an additional namespace, e.g. […]
Done
To view, visit change 2892136. To unsubscribe, or for help writing mail filters, visit settings.
Devlin (Gerrit)
Attention is currently required from: Kelvin Jiang.
15 comments:
Patchset:
A few more nits, but overall, LGTM! Great job, Kelvin!
File chrome/browser/extensions/api/scripting/scripting_api.cc:
Patch Set #18, Line 248:
script_parsing::ParseMatchFlags(content_script.match_origin_as_fallback.get(),Let's actually just leave this out for now. I'll add it in as part of finishing up that work.
Patch Set #18, Line 653:
/*can_execute_script_everywhere=*/falseLet's pull this up into a constant in the anonymous namespace (along with your comment in the other function) and use it in both places:
// ...
constexpr bool kScriptsCanExecuteEverywhere = false;Patch Set #18, Line 654:
const bool all_urls_includes_chrome_urls =I think we can do the same for this one (since it also only applies to the legacy chromevox extension)
Patch Set #18, Line 681:
OnContentScriptFilesValidatednit: `OnContentScriptFilesValidated()` (which makes codesearch parse them)
File chrome/common/extensions/api/scripting.idl:
Patch Set #18, Line 132:
ModifyContentScriptsCallbacknit: RegisterContentScriptsCallback?
File chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js:
const results = await Promise.allSettled([
chrome.scripting.registerContentScripts(scripts),
chrome.scripting.registerContentScripts(scripts)
]);
chrome.test.assertEq('fulfilled', results[0].status);
chrome.test.assertEq('rejected', results[1].status);
chrome.test.assertEq(
`Duplicate script ID '${scriptId}'`, results[1].reason.message);This came out nicely : )
File extensions/browser/extension_user_script_loader.h:
Patch Set #18, Line 58:
AbortLoadForPendingScriptsChatted offline. I'm not keen on AbortLoad(), because this doesn't actually abort the load from the USL's point of view at all. Let's actually just go back to RemovePending(). Sorry for the churn!
File extensions/browser/extension_user_script_loader.cc:
Patch Set #18, Line 271:
std::set<std::string> script_ids) {nit: since we don't modify this object, let's make it const &
and const& here
File extensions/common/manifest_handlers/content_scripts_handler.cc:
script_parsing::ParseMatchFlags(content_script.match_origin_as_fallback.get(),
content_script.match_about_blank.get(),
result.get());and then let's just keep the logic for match_* here.
File extensions/common/utils/content_script_utils.h:
using OnScriptFilesValidatedCallback =
base::OnceCallback<void(std::unique_ptr<UserScriptList> scripts,
const absl::optional<std::string>& error)>;Used?
File extensions/common/utils/content_script_utils.cc:
Patch Set #18, Line 38:
namespace {\n
\n
Patch Set #18, Line 98:
// matchesnit: probably not needed (ditto for others below)
To view, visit change 2892136. To unsubscribe, or for help writing mail filters, visit settings.
Devlin (Gerrit)
Attention is currently required from: Kelvin Jiang.
Patch set 18:Code-Review +1
Kelvin Jiang (Gerrit)
Patch set 19:Commit-Queue +2
13 comments:
Patchset:
its_happening.gif
File chrome/browser/extensions/api/scripting/scripting_api.cc:
Patch Set #18, Line 248:
script_parsing::ParseMatchFlags(content_script.match_origin_as_fallback.get(),Let's actually just leave this out for now. I'll add it in as part of finishing up that work.
Done
Patch Set #18, Line 653:
/*can_execute_script_everywhere=*/false
Let's pull this up into a constant in the anonymous namespace (along with your comment in the other […]
Done
Patch Set #18, Line 654:
const bool all_urls_includes_chrome_urls =
I think we can do the same for this one (since it also only applies to the legacy chromevox extensio […]
Done
Patch Set #18, Line 681:
OnContentScriptFilesValidatednit: `OnContentScriptFilesValidated()` (which makes codesearch parse them)
Done
File chrome/common/extensions/api/scripting.idl:
Patch Set #18, Line 132:
ModifyContentScriptsCallbacknit: RegisterContentScriptsCallback?
Done
File extensions/browser/extension_user_script_loader.cc:
Patch Set #18, Line 271:
std::set<std::string> script_ids) {nit: since we don't modify this object, let's make it const &
Done
and const& here
Done
File extensions/common/manifest_handlers/content_scripts_handler.cc:
script_parsing::ParseMatchFlags(content_script.match_origin_as_fallback.get(),
content_script.match_about_blank.get(),
result.get());and then let's just keep the logic for match_* here.
Done
File extensions/common/utils/content_script_utils.h:
using OnScriptFilesValidatedCallback =
base::OnceCallback<void(std::unique_ptr<UserScriptList> scripts,
const absl::optional<std::string>& error)>;Used?
no
File extensions/common/utils/content_script_utils.cc:
Patch Set #18, Line 38:
namespace {\n
Done
Done
Patch Set #18, Line 98:
// matchesnit: probably not needed (ditto for others below)
Done
To view, visit change 2892136. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
[Extensions] Implement scripting.registercontentscripts
This CL implements scripting.registerContentScripts, which is restricted
to MV3. Currently, registered content scripts are session scoped only.
Unlike manifest content scripts, the extension must specify a distinct
ID for each dynamic script.
Since the RegisteredContentScript type in scripting.idl shares a lot of
common logic wrt parsing/validating files as manifest content scripts,
common methods have been extracted into content_script_utils.
Bug: 1168627
Change-Id: I0e842230917ce27052968719f3af71311d418ca2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2892136
Commit-Queue: Kelvin Jiang <kelvi...@chromium.org>
Reviewed-by: Devlin <rdevlin...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#895367}
---
M chrome/browser/extensions/api/scripting/scripting_api.cc
M chrome/browser/extensions/api/scripting/scripting_api.h
M chrome/browser/extensions/api/scripting/scripting_apitest.cc
M chrome/browser/extensions/content_script_tracker_browsertest.cc
M chrome/common/extensions/api/_api_features.json
M chrome/common/extensions/api/scripting.idl
A chrome/test/data/extensions/api_test/scripting/dynamic_scripts/manifest.json
A chrome/test/data/extensions/api_test/scripting/dynamic_scripts/script.js
A chrome/test/data/extensions/api_test/scripting/dynamic_scripts/worker.js
M extensions/browser/BUILD.gn
M extensions/browser/api/execute_code_function.cc
A extensions/browser/api/extension_types_utils.cc
A extensions/browser/api/extension_types_utils.h
M extensions/browser/content_script_tracker.cc
M extensions/browser/extension_function_histogram_value.h
M extensions/browser/extension_user_script_loader.cc
M extensions/browser/extension_user_script_loader.h
M extensions/browser/user_script_manager.cc
M extensions/common/BUILD.gn
M extensions/common/manifest_handlers/content_scripts_handler.cc
M extensions/common/user_script.cc
M extensions/common/user_script.h
A extensions/common/utils/content_script_utils.cc
A extensions/common/utils/content_script_utils.h
M extensions/renderer/user_script_set.cc
M tools/metrics/histograms/enums.xml
26 files changed, 945 insertions(+), 178 deletions(-)
To view, visit change 2892136. To unsubscribe, or for help writing mail filters, visit settings.