Extensions: SidePanel: Create manifest key(s), API, and storage [chromium/src : main]
Solomon Kinard (Gerrit)
Attention is currently required from: Devlin Cronin.
Solomon Kinard would like Devlin Cronin to review this change.
Solomon Kinard removed Kelvin Jiang from this change.
Extensions: SidePanel: Create manifest key(s), API, and storage
Design Doc:
https://docs.google.com/document/d/1EjRghIJR1u1-UadYHdBhCCtY3A108LtEznSWzz7FtwE
Bug: 477424
Change-Id: Ic6cae327ac911b48a1ef9cb069f9efbd80bfd80a
---
M chrome/browser/extensions/BUILD.gn
A chrome/browser/extensions/api/side_panel/side_panel_api.cc
A chrome/browser/extensions/api/side_panel/side_panel_api.h
A chrome/browser/extensions/api/side_panel/side_panel_apitest.cc
A chrome/browser/extensions/api/side_panel/side_panel_service.cc
A chrome/browser/extensions/api/side_panel/side_panel_service.h
A chrome/browser/extensions/api/side_panel/side_panel_types.cc
A chrome/browser/extensions/api/side_panel/side_panel_types.h
M chrome/browser/extensions/browser_context_keyed_service_factories.cc
M chrome/common/extensions/api/_api_features.json
M chrome/common/extensions/api/_permission_features.json
M chrome/common/extensions/api/api_sources.gni
A chrome/common/extensions/api/side_panel.idl
A chrome/common/extensions/manifest_tests/extension_manifests_side_panel_unittest.cc
M chrome/common/extensions/permissions/chrome_api_permissions.cc
M chrome/common/extensions/permissions/permission_set_unittest.cc
M chrome/test/BUILD.gn
A chrome/test/data/extensions/api_test/side_panel/extension/default_path.html
A chrome/test/data/extensions/api_test/side_panel/extension/manifest.json
A chrome/test/data/extensions/api_test/side_panel/extension/panel.html
A chrome/test/data/extensions/api_test/side_panel/extension/test.html
A chrome/test/data/extensions/api_test/side_panel/extension/test.js
A chrome/test/data/extensions/api_test/side_panel/permission_missing/manifest.json
A chrome/test/data/extensions/api_test/side_panel/permission_missing/test.html
A chrome/test/data/extensions/api_test/side_panel/permission_missing/test.js
M extensions/browser/extension_function_histogram_value.h
M extensions/common/BUILD.gn
M extensions/common/api/_manifest_features.json
M extensions/common/api/schema.gni
A extensions/common/api/side_panel.idl
M extensions/common/common_manifest_handlers.cc
M extensions/common/extension_features.cc
M extensions/common/extension_features.h
M extensions/common/manifest_constants.cc
M extensions/common/manifest_constants.h
M extensions/common/manifest_handler.cc
A extensions/common/manifest_handlers/side_panel_info.cc
A extensions/common/manifest_handlers/side_panel_info.h
M extensions/common/mojom/api_permission_id.mojom
M tools/metrics/histograms/enums.xml
40 files changed, 878 insertions(+), 2 deletions(-)
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Attention is currently required from: Devlin Cronin.
1 comment:
Patchset:
Code complete. Can haz review? Thank you.
This CL represents everything required for the Side Panel project. Anything else is nice to have.
For context on my approach, the design doc can be commented if there are major disagreements.
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Attention is currently required from: Devlin Cronin.
Solomon Kinard uploaded patch set #7 to this change.
Extensions: SidePanel: Create manifest key(s), API, and storage
Design Doc:
https://docs.google.com/document/d/1EjRghIJR1u1-UadYHdBhCCtY3A108LtEznSWzz7FtwE
Bug: 1328645
M extensions/common/manifest.cc
M extensions/common/manifest_constants.cc
M extensions/common/manifest_constants.h
M extensions/common/manifest_handler.cc
A extensions/common/manifest_handlers/side_panel_info.cc
A extensions/common/manifest_handlers/side_panel_info.h
M extensions/common/mojom/api_permission_id.mojom
M tools/metrics/histograms/enums.xml
41 files changed, 948 insertions(+), 3 deletions(-)
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Solomon Kinard.
28 comments:
Patchset:
Thanks, Solomon!
I made it most of the way through this one (and wanted to get it back to you now so you're unblocked).
File chrome/browser/extensions/api/side_panel/side_panel_api.h:
Patch Set #8, Line 27:
SIDE_PANEL_GETIWYU extension_function_histogram_value
Patch Set #8, Line 27:
sidePanel.getI think we can omit the "panel" from the name here, but I'd like to keep "options" - i.e., "getOptions", "setOptions". This makes it clear it's not returning the panel itself.
Patch Set #8, Line 27:
SIDE_PANEL_GETDefault style is to basically only use '_' for "." in the name, so this should be "SIDEPANEL_GET"
class SidePanelRemoveFunction : public SidePanelApiFunction {
public:
DECLARE_EXTENSION_FUNCTION("sidePanel.remove", SIDE_PANEL_REMOVE)
SidePanelRemoveFunction() = default;
SidePanelRemoveFunction(const SidePanelRemoveFunction&) = delete;
SidePanelRemoveFunction& operator=(const SidePanelRemoveFunction&) = delete;
private:
~SidePanelRemoveFunction() override = default;
ResponseAction RunSidePanelFunction() override;
};rather than remove, extensions can use setOptions with enabled: false. (Separately, adding new methods should go through the API proposal, so let's discuss there if there are pieces missing)
File chrome/browser/extensions/api/side_panel/side_panel_api.cc:
Patch Set #8, Line 22:
class BrowserContext;forward declarations are really only used in header files; this is unnecessary.
SidePanelOptions GetOptionsFromValue(std::unique_ptr<base::Value> value) {
base::Value::Dict dict = std::move(value->GetDict());
const auto id = dict.FindInt("tabId");
const auto* path = dict.FindString("path");
const auto enabled = dict.FindBool("enabled");
return SidePanelOptions(id, *path, enabled);
}we should use the auto-generated types and conversion from the schema. It looks like you're doing this below, but then converting to a value, and then converting to a new type. Why not just use the auto-generated types directly?
Patch Set #8, Line 61:
RespondNow(OneArgument(base::Value(true)));This isn't guaranteed to be the proper return type, which will crash the browser if response validation is enabled. We need to make sure to return the expected type.
std::unique_ptr<api::side_panel::Set::Params> params(
api::side_panel::Set::Params::Create(args()));
EXTENSION_FUNCTION_VALIDATE(params);
GetService()->SetPanel(GetOptionsFromValue(params->options.ToValue()));As mentioned above, I think we can simplify this.
This flow:
- Starts as a base::Value
- Converts to a strongly-typed side panel options struct
- Converts back to a base::Value
- Converts to a custom type.
Let's just use the generated types, and convert once.
File chrome/browser/extensions/api/side_panel/side_panel_apitest.cc:
Patch Set #8, Line 8:
#include "chrome/test/base/ui_test_utils.h"are all these includes needed?
Patch Set #8, Line 31:
{.extension_url = "test.html"}unlike the favicon work (where we need a DOM because we want to embed img tags), these can run directly from the extension's service worker - no need for a separate page.
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
Are all these includes needed?
Patch Set #8, Line 21:
typedefprefer `using` in new code
Patch Set #8, Line 25:
class SidePanelService : public BrowserContextKeyedAPI {add a class comment describing what this is used for
Get the
// default options if already set, otherwise get manifest info.What if there's no manifest info?
prefer `const Extension&` if the extension can't be null (we're bad about this, but should be better)
Patch Set #8, Line 61:
std::map<TabId, SidePanelOptions> panels_;How does this work with multiple extensions?
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
Patch Set #8, Line 75:
static base::LazyInstance<BrowserContextKeyedAPIFactory<SidePanelService>>::prefer base::NoDestructor in new code
File chrome/common/extensions/api/_permission_features.json:
Patch Set #8, Line 824:
"channel": "dev",let's start on canary
File chrome/common/extensions/api/side_panel.idl:
Patch Set #8, Line 7:
// The options used when setting a side panel. Omitted properties are unchanged.nit: wrapping
PanelOptions options,
VoidCallback callbackfunction arguments should either be aligned or all on the same line:
```
static void setOptions(PanelOptions options,
VoidCallback callback);
```
File chrome/common/extensions/manifest_tests/extension_manifests_side_panel_unittest.cc:
Prefer avoiding "All" as test suites. This covers a pretty specific case (the file doesn't exist), so let's give it a more descriptive name.
File chrome/test/data/extensions/api_test/side_panel/extension/test.js:
// The options to change when updating a side panel.
class PanelOptions {
constructor(options) {
// Int?. Side panel to specific tab id. If unset, all unspecified tabs.
this.tabId;
// String?. The local extension path to the side panel HTML file to use.
this.path;
// Bool?. Whether the side panel should be enabled.
this.enabled;
// Update this object from the constructor parameter.
options && Object.keys(options).forEach(key => this[key] = options[key]);
}
};This appears to be unused? (And shouldn't be necessary - just passing a "bag of properties" is standard form in JS for things like this)
Patch Set #8, Line 92:
compareObjectsavoid re-inventing the wheel : ) chrome.test.assertEq() compares objects with > 1 depth and gives detailed error messages.
File extensions/common/api/_manifest_features.json:
Patch Set #8, Line 347:
"channel": "dev",similarly, start with canary
File extensions/common/extension_features.cc:
Patch Set #8, Line 90:
kSidePanelLet's call this something like ExtensionSidePanelIntegration. (While the feature object is prefixed with extensions_features, the string value is shared across all features, so should be reasonably unique and descriptive)
File extensions/common/manifest_handler.cc:
Patch Set #8, Line 17:
#include "extensions/common/stack_frame.h"rm
File extensions/common/manifest_handlers/side_panel_info.h:
Patch Set #8, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.Note: got up to here
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Attention is currently required from: Devlin Cronin.
28 comments:
Patchset:
Thanks.
File chrome/browser/extensions/api/side_panel/side_panel_api.h:
Patch Set #8, Line 27:
SIDE_PANEL_GET
Default style is to basically only use '_' for ". […]
Done
Patch Set #8, Line 27:
sidePanel.get
I think we can omit the "panel" from the name here, but I'd like to keep "options" - i.e. […]
I don't think Options (re: sidePanelOptions) were in the API when I LGTM'd it, based on still present references to "sidePanel.setPanel()" and crbug.com/1328645#c2. That said, I'm not opposed to adding sidePanel.setOptions(). That's better than sidePanel.setPanel(), but not as good as sidePanel.set().
Patch Set #8, Line 27:
SIDE_PANEL_GETIWYU extension_function_histogram_value
Done
class SidePanelRemoveFunction : public SidePanelApiFunction {
public:
DECLARE_EXTENSION_FUNCTION("sidePanel.remove", SIDE_PANEL_REMOVE)
SidePanelRemoveFunction() = default;
SidePanelRemoveFunction(const SidePanelRemoveFunction&) = delete;
SidePanelRemoveFunction& operator=(const SidePanelRemoveFunction&) = delete;
private:
~SidePanelRemoveFunction() override = default;
ResponseAction RunSidePanelFunction() override;
};
rather than remove, extensions can use setOptions with enabled: false. […]
I added a comment to the proposal a few minutes after your comment here. I think I may also have added my reasoning for .remove() in the design doc before uploading. tldr remove is necessary for actual removal, where disabled only obfuscates.
File chrome/browser/extensions/api/side_panel/side_panel_api.cc:
Patch Set #8, Line 22:
class BrowserContext;forward declarations are really only used in header files; this is unnecessary.
Done
SidePanelOptions GetOptionsFromValue(std::unique_ptr<base::Value> value) {
base::Value::Dict dict = std::move(value->GetDict());
const auto id = dict.FindInt("tabId");
const auto* path = dict.FindString("path");
const auto enabled = dict.FindBool("enabled");
return SidePanelOptions(id, *path, enabled);
}
Done
Why not just use the auto-generated types directly?
1) A custom SidePanelOptions type allows for one line calls such as:
`GetService()->SetPanel(GetOptionsFromValue(params->options.ToValue()));`. 2) The manual type had the benefit of portability for the service file to use types agnostic of a generated api schema. 3) auto-generated types rely on deprecated DictionaryValue, and 4) auto-generated types don't have a default constructor for single line initialization.
Patch Set #8, Line 61:
RespondNow(OneArgument(base::Value(true)));
This isn't guaranteed to be the proper return type, which will crash the browser if response validat […]
Done
std::unique_ptr<api::side_panel::Set::Params> params(
api::side_panel::Set::Params::Create(args()));
EXTENSION_FUNCTION_VALIDATE(params);
GetService()->SetPanel(GetOptionsFromValue(params->options.ToValue()));
As mentioned above, I think we can simplify this. […]
After the change, there was no need to convert in this file as options can be passed directly to _service.
File chrome/browser/extensions/api/side_panel/side_panel_apitest.cc:
Patch Set #8, Line 8:
#include "chrome/test/base/ui_test_utils.h"are all these includes needed?
Ack
Patch Set #8, Line 31:
{.extension_url = "test.html"}
unlike the favicon work (where we need a DOM because we want to embed img tags), these can run direc […]
Good call.
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
Are all these includes needed?
Ack
Patch Set #8, Line 21:
typedefprefer `using` in new code
Done
Patch Set #8, Line 25:
class SidePanelService : public BrowserContextKeyedAPI {add a class comment describing what this is used for
Ack
Get the
// default options if already set, otherwise get manifest info.What if there's no manifest info?
Comment updated. Also, browser_test SidePanelApiTest.UndefinedManifestKey.
prefer `const Extension&` if the extension can't be null (we're bad about this, but should be better […]
Done
Patch Set #8, Line 61:
std::map<TabId, SidePanelOptions> panels_;How does this work with multiple extensions?
The new way is extensionIds keyed to tabIds keyed to options. This makes it possible for OnExtensionUninstalled to remove options with ease, less memory usage, and chrome.sidePanel.getAll() functionality.
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
Patch Set #8, Line 75:
static base::LazyInstance<BrowserContextKeyedAPIFactory<SidePanelService>>::prefer base::NoDestructor in new code
Done
File chrome/common/extensions/api/_permission_features.json:
Patch Set #8, Line 824:
"channel": "dev",let's start on canary
Done
File chrome/common/extensions/api/side_panel.idl:
Patch Set #8, Line 7:
// The options used when setting a side panel. Omitted properties are unchanged.nit: wrapping
Done
PanelOptions options,
VoidCallback callback
function arguments should either be aligned or all on the same line: […]
Done
File chrome/common/extensions/manifest_tests/extension_manifests_side_panel_unittest.cc:
Prefer avoiding "All" as test suites. […]
Done
File chrome/test/data/extensions/api_test/side_panel/extension/test.js:
// The options to change when updating a side panel.
class PanelOptions {
constructor(options) {
// Int?. Side panel to specific tab id. If unset, all unspecified tabs.
this.tabId;
// String?. The local extension path to the side panel HTML file to use.
this.path;
// Bool?. Whether the side panel should be enabled.
this.enabled;
// Update this object from the constructor parameter.
options && Object.keys(options).forEach(key => this[key] = options[key]);
}
};
This appears to be unused? (And shouldn't be necessary - just passing a "bag of properties" is stan […]
Ack
Patch Set #8, Line 92:
compareObjects
avoid re-inventing the wheel : ) chrome.test. […]
Ack
File extensions/common/api/_manifest_features.json:
Patch Set #8, Line 347:
"channel": "dev",similarly, start with canary
Done
File extensions/common/extension_features.cc:
Patch Set #8, Line 90:
kSidePanel
Let's call this something like ExtensionSidePanelIntegration. […]
Done
File extensions/common/manifest_handler.cc:
Patch Set #8, Line 17:
#include "extensions/common/stack_frame.h"rm
Done
File extensions/common/manifest_handlers/side_panel_info.h:
Patch Set #8, Line 1:
// Copyright 2022 The Chromium Authors. All rights reserved.Note: got up to here
Ack
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Solomon Kinard.
27 comments:
Patchset:
Thanks, Solomon!
File chrome/browser/extensions/api/side_panel/side_panel_api.h:
Patch Set #8, Line 27:
sidePanel.getI don't think Options (re: sidePanelOptions) were in the API when I LGTM'd it, based on still presen […]
There was a discussion about setPanel vs setPanelOptions on the doc; I think the reasoning there applies the same way for setOptions vs set.
class SidePanelRemoveFunction : public SidePanelApiFunction {
public:
DECLARE_EXTENSION_FUNCTION("sidePanel.remove", SIDE_PANEL_REMOVE)
SidePanelRemoveFunction() = default;
SidePanelRemoveFunction(const SidePanelRemoveFunction&) = delete;
SidePanelRemoveFunction& operator=(const SidePanelRemoveFunction&) = delete;
private:
~SidePanelRemoveFunction() override = default;
ResponseAction RunSidePanelFunction() override;
};
I added a comment to the proposal a few minutes after your comment here. […]
Let's continue the discussion on the doc (and add remove() in a followup if we go that route)
File chrome/browser/extensions/api/side_panel/side_panel_api.cc:
SidePanelOptions GetOptionsFromValue(std::unique_ptr<base::Value> value) {
base::Value::Dict dict = std::move(value->GetDict());
const auto id = dict.FindInt("tabId");
const auto* path = dict.FindString("path");
const auto enabled = dict.FindBool("enabled");
return SidePanelOptions(id, *path, enabled);
}
Done […]
Thanks for the explanation! I think generated types still make a bit more sense here. Responding to those points:
1) This can be SetPanel(std::move(params->options))
2) The service is used for the API, so I don't think having separate types / being agnostic is beneficial (and it introduces the chance they will diverge)
3) True, but we should be able to avoid using Value in these call sites.
4) They do have a default ctor, e.g. https://source.chromium.org/chromium/chromium/src/+/main:out/win-Debug/gen/chrome/common/extensions/api/scripting.h;l=59;drc=441d6f399964a4814a4cc73ebec3c63c85ba9c85
File chrome/browser/extensions/api/side_panel/side_panel_api.cc:
if (!IsSidePanelApiAvailable())
return RunRespondNow();Rather than returning a stub value, I think we should return an error in this case. That also avoids the need for a RunRespondNow() virtual function.
```
if (!IsSidePanelApiAvailable())
return RespondNow(Error("API Unavailable"));
```Patch Set #10, Line 46:
.get()no need for .get() call
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
Patch Set #10, Line 8:
#include <map>needed?
using TabId = int;
using TabPanelOptions = base::flat_map<TabId, api::side_panel::PanelOptions>;
using PanelOptions = base::flat_map<ExtensionId, TabPanelOptions>;Can these live in the SidePanelService class?
Patch Set #10, Line 45:
std::unique_ptr<TabId> tab_idprefer absl::optional over unique_ptr to represent optionality (I know our generated types don't do this, but that's because they predate optional)
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
Patch Set #8, Line 61:
std::map<TabId, SidePanelOptions> panels_;The new way is extensionIds keyed to tabIds keyed to options. […]
Ack
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
base::Value::Dict dict;
if (!path.empty()) {
dict.Set("path", path);
dict.Set("enabled", true);
}
api::side_panel::PanelOptions options;
api::side_panel::PanelOptions::Populate(base::Value(std::move(dict)),
&options);
return options;We shouldn't generate types from base::Value except for in (de)serialization. Instead, we should just use the type directly:
```
api::side_panel::PanelOptions options;
if (!path.empty()) {
options.path = path;
options.enabled = true;
}
return options;
```Patch Set #10, Line 43:
auto* it = &panels_.find(extension.id())->second;We already do this lookup, so let's cache the result earlier.
```
auto iter = panels_.find(extension.id());
if (iter == panels_.end()) {
// Check manifest info
}TabPanelOptions& tab_options = iter->second;
```auto path = SidePanelInfo::GetDefaultPath(&extension);
base::Value::Dict dict;
if (!path.empty()) {
dict.Set("path", path);
dict.Set("enabled", true);
}
api::side_panel::PanelOptions options;
api::side_panel::PanelOptions::Populate(base::Value(std::move(dict)),
&options);
return options;This code is duplicated. Pull it out into a helper function, GetPanelOptionsFromManifest()?
auto dict = std::move((*it)[default_tab_id].ToValue()->GetDict());
dict.Set("tabId", tab_id);
api::side_panel::PanelOptions options;
api::side_panel::PanelOptions::Populate(base::Value(std::move(dict)),
&options);
return options;I'd like to avoid creating a copy of PanelOptions if we can (especially since there isn't a handy Clone() method on it, which led to this dance between PanelOptions -> Value -> PanelOptions). Can we have this method return a const &? This would require us storing the manifest-specified panel options (because we can't return a reference to a temporary), but I think that might be cleaner overall.
if (panels_.find(extension.id()) == panels_.end())
panels_[extension.id()] = TabPanelOptions();
auto* it = &panels_.find(extension.id())->second;This ends up doing 3 different lookups into the map:
- the find()
- the operator[]
- the second find()
We can instead just do:
```
TabPanelOptions& tab_options = panels_[extension.id()];
```This will create a new entry in the map if one doesn't exist, and will re-use the existing one if it does.
File chrome/common/extensions/api/side_panel.idl:
dictionary RemoveOptions {
// If specified, the side panel for the given tab will be removed.
long? tabId;
};no longer used in this CL
[supportsPromises] static void getOptions(GetPanelOptions options,
PanelOptionsCallback callback);As mentioned here [1], arguments either be aligned or all on the same line
File chrome/common/extensions/manifest_tests/extension_manifests_side_panel_unittest.cc:
"found "
"string.";nit: wrapping
File chrome/test/data/extensions/api_test/side_panel/extension/service_worker.js:
Patch Set #10, Line 42:
panellet's call this "new_default.html" or similar to make it clear we're not getting the value from the old setting.
Let's also add a case for retrieving the newly-set default when we try to fetch the options for a tab ID that doesn't have a dedicated entry.
// Compare dictionary with a depth of 1.
function compareObjects(obj1, obj2) {
for (const [key, value] of Object.entries(obj1)) {
if (!(key in obj2 && value == obj2[key]))
return false;
}
return Object.keys(obj1).length == Object.keys(obj2).length;
}Sorry, maybe this comment [1] was unclear. Please use chrome.test.assertEq() instead of this custom method.
File extensions/common/api/_manifest_features.json:
Patch Set #10, Line 348:
"extension_types": ["extension"]add `"min_manifest_version": 3`
File extensions/common/api/side_panel.idl:
// Namespace for the "side_panel" manifest key.
[generate_error_messages] namespace sidePanel {
dictionary SidePanel {
// Developer specified path for side panel display.
DOMString default_path;
};
dictionary ManifestKeys {
SidePanel side_panel;
};
};An API should generally live at one "layer" (i.e., either at //chrome or //extensions). Side Panel should probably be in //chrome, because it depends heavily on browser concepts. We can combine this IDL file with the one at //chrome/common/extensions/api/side_panel.idl. Similarly, the features entries and manifest handlers should also be at the //chrome layer.
File extensions/common/manifest_handlers/side_panel_info.h:
Patch Set #10, Line 11:
#include "base/containers/flat_set.h"needed?
Patch Set #10, Line 20:
// Define out of line constructor/destructor for Clang.nit: I don't think this comment is necessary (since this goes for most ctors/dtors defined in a header file)
File extensions/common/manifest_handlers/side_panel_info.cc:
auto gurl = extension->GetResourceURL(path);
auto relative_path = file_util::ExtensionURLToRelativeFilePath(gurl);This seems to take a relative path, turn it into a GURL, and then turn it into a path. Could we instead just do `auto resource_path = extension->GetResource(path).GetFilePath()`?
if (extension->manifest_version() < 3) {
*error =
ErrorUtils::FormatErrorMessageUTF16(errors::kManifestMinimumError, "3");
return false;
}This should be handled by our features system (with the addition of the min_manifest_version in the features files)
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Attention is currently required from: Devlin Cronin.
23 comments:
File chrome/browser/extensions/api/side_panel/side_panel_api.cc:
SidePanelOptions GetOptionsFromValue(std::unique_ptr<base::Value> value) {
base::Value::Dict dict = std::move(value->GetDict());
const auto id = dict.FindInt("tabId");
const auto* path = dict.FindString("path");
const auto enabled = dict.FindBool("enabled");
return SidePanelOptions(id, *path, enabled);
}
1) This can be SetPanel(std::move(params->options))
Great minds. https://chromium.googlesource.com/chromium/src/+/refs/changes/44/3652544/10/chrome/browser/extensions/api/side_panel/side_panel_api.cc#58 is in this patch that you reviewed here.
4) They do have a default ctor
::Populate(base::Value) is the closest option that I noticed to what I had in mind wrt a default constructor that allows for arguments emplace.
File chrome/browser/extensions/api/side_panel/side_panel_api.cc:
if (!IsSidePanelApiAvailable())
return RunRespondNow();
Rather than returning a stub value, I think we should return an error in this case. […]
Done
Patch Set #10, Line 46:
.get()
no need for . […]
Done
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
Patch Set #10, Line 8:
#include <map>needed?
Done
using TabId = int;
using TabPanelOptions = base::flat_map<TabId, api::side_panel::PanelOptions>;
using PanelOptions = base::flat_map<ExtensionId, TabPanelOptions>;Can these live in the SidePanelService class?
Done
Patch Set #10, Line 45:
std::unique_ptr<TabId> tab_id
prefer absl::optional over unique_ptr to represent optionality (I know our generated types don't do […]
Done
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
base::Value::Dict dict;
if (!path.empty()) {
dict.Set("path", path);
dict.Set("enabled", true);
}
api::side_panel::PanelOptions options;
api::side_panel::PanelOptions::Populate(base::Value(std::move(dict)),
&options);
return options;
We shouldn't generate types from base::Value except for in (de)serialization. […]
Done
Patch Set #10, Line 43:
auto* it = &panels_.find(extension.id())->second;
We already do this lookup, so let's cache the result earlier. […]
Done
auto path = SidePanelInfo::GetDefaultPath(&extension);
base::Value::Dict dict;
if (!path.empty()) {
dict.Set("path", path);
dict.Set("enabled", true);
}
api::side_panel::PanelOptions options;
api::side_panel::PanelOptions::Populate(base::Value(std::move(dict)),
&options);
return options;
This code is duplicated. […]
Done
auto dict = std::move((*it)[default_tab_id].ToValue()->GetDict());
dict.Set("tabId", tab_id);
api::side_panel::PanelOptions options;
api::side_panel::PanelOptions::Populate(base::Value(std::move(dict)),
&options);
return options;
CloneOptions() has been locally created to make it cleaner. A copy has to be made regardless. A member variable was considered in addition to passing an option by reference from the api to the service. Neither was chosen because it adds more code without enough benefits to mitigate the downsides, imho.
This would require us storing the manifest-specified panel options
It seems like member storage could be racy and be harder to reason after while.
if (panels_.find(extension.id()) == panels_.end())
panels_[extension.id()] = TabPanelOptions();
auto* it = &panels_.find(extension.id())->second;
This ends up doing 3 different lookups into the map: […]
Done
File chrome/common/extensions/api/side_panel.idl:
dictionary RemoveOptions {
// If specified, the side panel for the given tab will be removed.
long? tabId;
};no longer used in this CL
Done
[supportsPromises] static void getOptions(GetPanelOptions options,
PanelOptionsCallback callback);
As mentioned here [1], arguments either be aligned or all on the same line […]
I wonder if there's a bug open for git cl format to handle this.
File chrome/common/extensions/manifest_tests/extension_manifests_side_panel_unittest.cc:
"found "
"string.";nit: wrapping
Done
File chrome/test/data/extensions/api_test/side_panel/extension/service_worker.js:
Patch Set #10, Line 42:
panel
let's call this "new_default. […]
Done
Let's also add a case for retrieving the newly-set default when we try to fetch the options for a ta […]
Done
// Compare dictionary with a depth of 1.
function compareObjects(obj1, obj2) {
for (const [key, value] of Object.entries(obj1)) {
if (!(key in obj2 && value == obj2[key]))
return false;
}
return Object.keys(obj1).length == Object.keys(obj2).length;
}
Sorry, maybe this comment [1] was unclear. Please use chrome.test. […]
Done. I moved to a new machine during the time of the last upload, so anything could have happened along the way.
File extensions/common/api/_manifest_features.json:
Patch Set #10, Line 348:
"extension_types": ["extension"]add `"min_manifest_version": 3`
Done
File extensions/common/api/side_panel.idl:
// Namespace for the "side_panel" manifest key.
[generate_error_messages] namespace sidePanel {
dictionary SidePanel {
// Developer specified path for side panel display.
DOMString default_path;
};
dictionary ManifestKeys {
SidePanel side_panel;
};
};
An API should generally live at one "layer" (i.e., either at //chrome or //extensions). […]
Starting this yielded breaking changes such as [generated] side_panel_info.h not found and too few arguments, so it's not included in this patch set. That seems like an LSC (large scale change), so this patch aims to address all other resolved comments while considering which files to move into //c.
File extensions/common/manifest_handlers/side_panel_info.h:
Patch Set #10, Line 11:
#include "base/containers/flat_set.h"needed?
Done
Patch Set #10, Line 20:
// Define out of line constructor/destructor for Clang.
nit: I don't think this comment is necessary (since this goes for most ctors/dtors defined in a head […]
Done
File extensions/common/manifest_handlers/side_panel_info.cc:
auto gurl = extension->GetResourceURL(path);
auto relative_path = file_util::ExtensionURLToRelativeFilePath(gurl);
This seems to take a relative path, turn it into a GURL, and then turn it into a path. […]
Done
if (extension->manifest_version() < 3) {
*error =
ErrorUtils::FormatErrorMessageUTF16(errors::kManifestMinimumError, "3");
return false;
}
This should be handled by our features system (with the addition of the min_manifest_version in the […]
Removed. It seems like your message is worded that way. Also removed associated tests.
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Attention is currently required from: Devlin Cronin.
1 comment:
Patchset:
Thanks.
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Solomon Kinard.
14 comments:
Patchset:
Thanks, Solomon! I wasn't able to take a thorough pass this time, given the size of the CL, but there are a few more comments here.
File chrome/browser/extensions/api/side_panel/side_panel_apitest.cc:
// // Verify chrome.sidePanel behavior without permissions.
// IN_PROC_BROWSER_TEST_F(SidePanelApiTest, PermissionMissing) {
// ASSERT_TRUE(RunExtensionTest("side_panel/permission_missing")) << message_;
// }
// // Verify chrome.sidePanel.get behavior without side_panel manifest key.
// IN_PROC_BROWSER_TEST_F(SidePanelApiTest, UndefinedManifestKey) {
// ASSERT_TRUE(RunExtensionTest("side_panel/undefined_manifest_key"))
// << message_;
// }Do we want to keep these tests? (I think the undefined manifest key is at least valuable, and I'm not against keeping the permission missing - though it should be handled by the feature system.) If so, uncomment them?
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
using TabPanelOptions = base::flat_map<TabId, api::side_panel::PanelOptions>;
// TODO(1328645): Remove options for matching ExtensionId on uninstallation.
using ExtensionPanelOptions = base::flat_map<ExtensionId, TabPanelOptions>;These type aliases, since they're only used in the private data members, should themselves be private.
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
std::move(path)
Patch Set #11, Line 38:
clone ?This should never fail. If you want to check it, we can make it a DCHECK, but I'd also be fine just directly returning it.
api::side_panel::PanelOptions CloneOptions(
const api::side_panel::PanelOptions& options) {
auto clone = api::side_panel::PanelOptions::FromValue(
base::Value(std::move(options.ToValue()->GetDict())));
return clone ? std::move(*clone) : api::side_panel::PanelOptions();
}It's a shame (but understandable) that we need this. Let's file a bug and add a TODO for our generated types to allow conditionally including a Clone() method for types with a given attribute.
Patch Set #11, Line 51:
extensionPanelOptionsc++ code uses snake_case_variable_names
Patch Set #11, Line 67:
if (tab_id != default_tab_id && it->find(default_tab_id) != it->end()) {this logic seems incorrect. It says "if the tab id is not the default tab id (i.e., we're looking for a specific tab) and there is a value for the default tab id, return the default tab id" - but if we're looking for a specific tab, we should check if that's present first. Otherwise, this will return the default options if they're set, even if we have more specific options in the map.
Patch Set #11, Line 68:
(*it)[default_tab_id]This results in a duplicate lookup for default_tab_id, as does the line on 74. Instead, we should re-use the existing iters.
auto it = tab_options.find(tab_id);
if (it == tab_options.end()) {
panels_[extension.id()][tab_id] = std::move(options);these are also duplicate lookups in the panels_ map
File extensions/browser/extension_function_histogram_value.h:
Patch Set #11, Line 1729:
SIDEPANEL_REMOVEOPTIONS = 1666,This no longer exists in this CL
File extensions/common/api/side_panel.idl:
// Namespace for the "side_panel" manifest key.
[generate_error_messages] namespace sidePanel {
dictionary SidePanel {
// Developer specified path for side panel display.
DOMString default_path;
};
dictionary ManifestKeys {
SidePanel side_panel;
};
};
Starting this yielded breaking changes such as [generated] side_panel_info. […]
I'd like us to handle this in this CL, since it should only require modifying the files that are introduced here. Can you provide more detail about the errors you're seeing when you try to move them? (Shot in the dark: you will have to update all includes of the generated side_panel.h file to point to //chrome/common/extensions/api/side_panel/side_panel.h, and the old //extensions copy will still exist in your out dir if you don't remove it, so maybe there's a stale one there?)
File extensions/common/manifest_constants.h:
Patch Set #11, Line 129:
extern const char kSidePanelDefaultPath[];Is this still used?
Patch Set #11, Line 441:
extern const char kManifestMinimumError[];Is this still used?
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Attention is currently required from: Devlin Cronin.
15 comments:
Patchset:
Thanks.
File chrome/browser/extensions/api/side_panel/side_panel_apitest.cc:
// // Verify chrome.sidePanel behavior without permissions.
// IN_PROC_BROWSER_TEST_F(SidePanelApiTest, PermissionMissing) {
// ASSERT_TRUE(RunExtensionTest("side_panel/permission_missing")) << message_;
// }
// // Verify chrome.sidePanel.get behavior without side_panel manifest key.
// IN_PROC_BROWSER_TEST_F(SidePanelApiTest, UndefinedManifestKey) {
// ASSERT_TRUE(RunExtensionTest("side_panel/undefined_manifest_key"))
// << message_;
// }
Do we want to keep these tests? (I think the undefined manifest key is at least valuable, and I'm n […]
Yep, debug upload oversight.
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
using TabPanelOptions = base::flat_map<TabId, api::side_panel::PanelOptions>;
// TODO(1328645): Remove options for matching ExtensionId on uninstallation.
using ExtensionPanelOptions = base::flat_map<ExtensionId, TabPanelOptions>;
These type aliases, since they're only used in the private data members, should themselves be privat […]
Done
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
auto dict = std::move((*it)[default_tab_id].ToValue()->GetDict());
dict.Set("tabId", tab_id);
api::side_panel::PanelOptions options;
api::side_panel::PanelOptions::Populate(base::Value(std::move(dict)),
&options);
return options;
CloneOptions() has been locally created to make it cleaner. A copy has to be made regardless. […]
Ack
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
std::move(path)
Done
Patch Set #11, Line 38:
clone ?
This should never fail. […]
What about this annotation?
```
// Creates a PanelOptions object from a base::Value, or NULL on failure.
static std::unique_ptr<PanelOptions> FromValue(const base::Value& value);
```
api::side_panel::PanelOptions CloneOptions(
const api::side_panel::PanelOptions& options) {
auto clone = api::side_panel::PanelOptions::FromValue(
base::Value(std::move(options.ToValue()->GetDict())));
return clone ? std::move(*clone) : api::side_panel::PanelOptions();
}
It's a shame (but understandable) that we need this. […]
Done
Patch Set #11, Line 51:
extensionPanelOptionsc++ code uses snake_case_variable_names
Done
Patch Set #11, Line 67:
if (tab_id != default_tab_id && it->find(default_tab_id) != it->end()) {
this logic seems incorrect. It says "if the tab id is not the default tab id (i.e. […]
A direct check has been added before this conditional, for clarity. The setAndGetPanel tests were already passing before this addition.
Patch Set #11, Line 68:
(*it)[default_tab_id]
This results in a duplicate lookup for default_tab_id, as does the line on 74. […]
Done
auto it = tab_options.find(tab_id);
if (it == tab_options.end()) {
panels_[extension.id()][tab_id] = std::move(options);these are also duplicate lookups in the panels_ map
Done
File extensions/browser/extension_function_histogram_value.h:
Patch Set #11, Line 1729:
SIDEPANEL_REMOVEOPTIONS = 1666,This no longer exists in this CL
Done
File extensions/common/api/side_panel.idl:
// Namespace for the "side_panel" manifest key.
[generate_error_messages] namespace sidePanel {
dictionary SidePanel {
// Developer specified path for side panel display.
DOMString default_path;
};
dictionary ManifestKeys {
SidePanel side_panel;
};
};
I'd like us to handle this in this CL, since it should only require modifying the files that are int […]
idl, manifest, and features that already had a similarly named landing spot were moved from //e to //c.
File extensions/common/manifest_constants.h:
Patch Set #11, Line 129:
extern const char kSidePanelDefaultPath[];Is this still used?
Done
Patch Set #11, Line 441:
extern const char kManifestMinimumError[];Is this still used?
Done
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Solomon Kinard.
17 comments:
Patchset:
Awesome, Solomon! This is looking really good. A few more comments, but mostly small.
File chrome/browser/extensions/api/side_panel/side_panel_api.cc:
Patch Set #16, Line 56:
GetService()->SetOptions(*extension(), std::move(params->options));Not for this CL (since it's already very, very large), but flagging so we don't forget: we also need to validate the passed `path` is a relative path in the extension.
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
Patch Set #16, Line 44:
1328645nit: prefer `crbug.com/1328645` to disambiguate from b/1328645.
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
Patch Set #11, Line 38:
clone ?What about this annotation? […]
It can fail if the value doesn't have the appropriate fields, which is completely possible when, say, parsing values passed in by extensions. However, in this case, the value we have here is directly created from a PanelOptions, so the value should *always* be a valid serialization of PanelOptions.
Patch Set #11, Line 67:
if (tab_id != default_tab_id && it->find(default_tab_id) != it->end()) {
The setAndGetPanel tests were already passing before this addition.
Yep, the test succeeded because at the time it's called, there isn't a default tab value set, which meant that we didn't enter this condition. The default tab id entry is later set in defaultSetAndGetPanel(), but then we never test again for a specific tab ID.
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
nit: prefer a reference over a pointer here (since it should never be null)
Patch Set #16, Line 64:
auto default_panel_options = it->find(default_tab_id);nit: move this below the `panel_options != it->end()` check, since we otherwise don't need to do this lookup.
Additionally, in the case where tab_id *is* default_tab_id, we've already done this lookup.
Can we restructure the below to be:
```
if (tab_id != default_tab_id) {
auto specific_tab_options = it.find(tab_id);
if (specific_tab_options != it.end()) {
// return specific tab options
}
}// Fall back to the default tab if no tab ID
// was specified or entries for the specific tab weren't found.
auto default_options = it.find(default_tab_id);
if (default_options != it.end()) {
// return default tab options
}// Fall back to the manifest-specified options as a last resort.
// return manifest options
```That way, we do the minimum number of lookups into the map.
Patch Set #16, Line 70:
options.tab_id = std::make_unique<int>(tab_id);we seem to be setting options.tab_id in some cases, but not all. Specifically, we set it to the extension-specified tab_id if we find either options for the specific tab or options for the default tab, but not if we fall back to the manifest. Is that desired? It seems strange that we're setting it for one default case (default tab id) but not the other (manifest specified)
Patch Set #16, Line 72:
} else {nit: no `else` after `return` (though moot with comment above)
File chrome/common/extensions/api/side_panel.idl:
[supportsPromises] static void getOptions(GetPanelOptions options,
PanelOptionsCallback callback);
I wonder if there's a bug open for git cl format to handle this.
Ack
File chrome/common/extensions/api/side_panel.idl:
Patch Set #16, Line 45:
VoidCallbackThis should be "optional VoidCallback callback", since an extension may not care to specify a callback to setOptions(). (This is my bad; I mistyped this in the API proposal. I fixed it there just now.)
File chrome/common/extensions/api/side_panel/side_panel_info.cc:
Patch Set #16, Line 42:
manifest_keys.side_panel.default_pathstd::move()
File chrome/test/data/extensions/api_test/side_panel/extension/service_worker.js:
Patch Set #16, Line 24:
new_defaultnit: instead of `new_default`, let's call this `tab_specific.html` or similar. That way, we can verify that when we set a default value of `new_default` below, we're returning the right value. Otherwise, we can't tell if the service is returning the default or the tab-specific entry.
Patch Set #16, Line 32:
setAndGetPanelnit: differentiate this test case name from the one above.
Patch Set #16, Line 51:
defaultSetAndGetPanelnit: here, too, differentiate test names
let's add one final test that retrieves the old `tabId`'s options to validate that we get the right value for a specified tab ID even when there is a default tab ID value present.
File extensions/common/api/side_panel.idl:
// Namespace for the "side_panel" manifest key.
[generate_error_messages] namespace sidePanel {
dictionary SidePanel {
// Developer specified path for side panel display.
DOMString default_path;
};
dictionary ManifestKeys {
SidePanel side_panel;
};
};
idl, manifest, and features that already had a similarly named landing spot were moved from //e to / […]
Perfect; thanks!
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Patchset:
Thanks.
File chrome/browser/extensions/api/side_panel/side_panel_api.cc:
Patch Set #16, Line 56:
GetService()->SetOptions(*extension(), std::move(params->options));
Not for this CL (since it's already very, very large), but flagging so we don't forget: we also need […]
Done
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
Patch Set #16, Line 44:
1328645nit: prefer `crbug.com/1328645` to disambiguate from b/1328645.
Done
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
Patch Set #11, Line 38:
clone ?
It can fail if the value doesn't have the appropriate fields, which is completely possible when, say […]
Done
Patch Set #11, Line 67:
if (tab_id != default_tab_id && it->find(default_tab_id) != it->end()) {
> The setAndGetPanel tests were already passing before this addition. […]
Ack
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
nit: prefer a reference over a pointer here (since it should never be null)
Done
Patch Set #16, Line 64:
auto default_panel_options = it->find(default_tab_id);
nit: move this below the `panel_options != it->end()` check, since we otherwise don't need to do thi […]
Done
Patch Set #16, Line 70:
options.tab_id = std::make_unique<int>(tab_id);
we seem to be setting options.tab_id in some cases, but not all. […]
`chrome.sidePanel.getOptions({})` is an example case when it doesn't make sense to return a `tabId`.
Patch Set #16, Line 72:
} else {nit: no `else` after `return` (though moot with comment above)
Ack
File chrome/common/extensions/api/side_panel.idl:
Patch Set #16, Line 45:
VoidCallback
This should be "optional VoidCallback callback", since an extension may not care to specify a callba […]
Done
File chrome/common/extensions/api/side_panel/side_panel_info.cc:
Patch Set #16, Line 42:
manifest_keys.side_panel.default_pathstd::move()
Done
File chrome/test/data/extensions/api_test/side_panel/extension/service_worker.js:
Patch Set #16, Line 24:
new_default
nit: instead of `new_default`, let's call this `tab_specific.html` or similar. […]
Done
Patch Set #16, Line 32:
setAndGetPanelnit: differentiate this test case name from the one above.
Done
Patch Set #16, Line 51:
defaultSetAndGetPanelnit: here, too, differentiate test names
Done
let's add one final test that retrieves the old `tabId`'s options to validate that we get the right […]
Done
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Attention is currently required from: Kelvin Jiang.
Solomon Kinard would like Kelvin Jiang to review this change.
Extensions: SidePanel: Create manifest key(s), API, and storage
Design Doc:
https://docs.google.com/document/d/1EjRghIJR1u1-UadYHdBhCCtY3A108LtEznSWzz7FtwE
Bug: 1328645
Change-Id: Ic6cae327ac911b48a1ef9cb069f9efbd80bfd80a
---
M chrome/browser/extensions/BUILD.gn
A chrome/browser/extensions/api/side_panel/side_panel_api.cc
A chrome/browser/extensions/api/side_panel/side_panel_api.h
A chrome/browser/extensions/api/side_panel/side_panel_apitest.cc
A chrome/browser/extensions/api/side_panel/side_panel_service.cc
A chrome/browser/extensions/api/side_panel/side_panel_service.h
M chrome/browser/extensions/browser_context_keyed_service_factories.cc
M chrome/common/BUILD.gn
M chrome/common/extensions/api/_api_features.json
M chrome/common/extensions/api/_manifest_features.json
M chrome/common/extensions/api/_permission_features.json
M chrome/common/extensions/api/api_sources.gni
A chrome/common/extensions/api/side_panel.idl
A chrome/common/extensions/api/side_panel/side_panel_info.cc
A chrome/common/extensions/api/side_panel/side_panel_info.h
M chrome/common/extensions/chrome_manifest_handlers.cc
A chrome/common/extensions/manifest_tests/extension_manifests_side_panel_unittest.cc
M chrome/common/extensions/permissions/chrome_api_permissions.cc
M chrome/common/extensions/permissions/permission_set_unittest.cc
M chrome/test/BUILD.gn
A chrome/test/data/extensions/api_test/side_panel/extension/default_path.html
A chrome/test/data/extensions/api_test/side_panel/extension/manifest.json
A chrome/test/data/extensions/api_test/side_panel/extension/panel.html
A chrome/test/data/extensions/api_test/side_panel/extension/service_worker.js
A chrome/test/data/extensions/api_test/side_panel/permission_missing/manifest.json
A chrome/test/data/extensions/api_test/side_panel/permission_missing/service_worker.js
A chrome/test/data/extensions/api_test/side_panel/undefined_manifest_key/manifest.json
A chrome/test/data/extensions/api_test/side_panel/undefined_manifest_key/panel.html
A chrome/test/data/extensions/api_test/side_panel/undefined_manifest_key/service_worker.js
M extensions/browser/extension_function_histogram_value.h
M extensions/common/extension_features.cc
M extensions/common/extension_features.h
M extensions/common/manifest.cc
M extensions/common/manifest_constants.cc
M extensions/common/manifest_constants.h
M extensions/common/mojom/api_permission_id.mojom
M tools/metrics/histograms/enums.xml
37 files changed, 821 insertions(+), 1 deletion(-)
Solomon Kinard (Gerrit)
Attention is currently required from: Kelvin Jiang.
1 comment:
Patchset:
Adding Kelvin to attention set based on a quick chat. Thanks Kelvin. Removing Devlin from attention set in case Kelvin has time to take a look? Thanks.
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Solomon Kinard abandoned this change.
To view, visit change 3736155. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Solomon Kinard restored this change.
To view, visit change 3736155. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
9 comments:
Patchset:
took a pass
File chrome/browser/extensions/api/side_panel/side_panel_apitest.cc:
Patch Set #21, Line 37:
UndefinedManifestKeytiny nit: `missing_manifest_key` may be may be a slightly more accurate name?
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
using TabPanelOptions = base::flat_map<TabId, api::side_panel::PanelOptions>;
using ExtensionPanelOptions = base::flat_map<ExtensionId, TabPanelOptions>;
Based on what this CL entails, I don't see TabPanelOptions existing outside of an ExtensionPanelOptions.
Was the 2 level mapping here done so we can return `GetPanelOptionsFromManifest`/make checking easier ?
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
Patch Set #21, Line 52:
// Maybe get default path if there are no records for a given extension_id.replace with TODO if this will be done in a follow up?
Patch Set #21, Line 65:
specific_tab_optionsmay need to take another pass but:
Patch Set #21, Line 65:
return CloneOptions(specific_tab_options->second);maybe I'm just confused but: do we really need to Clone() here since we're probably copying the object anyway? or is this because said object has a bunch of unique_ptrs inside it that need to be cloned
File chrome/common/extensions/api/side_panel.idl:
Patch Set #21, Line 13:
SidePanel side_panel;hmm.jpg
thinking of why we need this here vs a constant??
File chrome/test/data/extensions/api_test/side_panel/extension/service_worker.js:
Patch Set #21, Line 60:
defaultSetAndGetPanelAgainnit: this is a duplicate function to what's above, maybe mention in the name that this is for a specific tab?
defaultGetPanelForSpecificTab?
File chrome/test/data/extensions/api_test/side_panel/permission_missing/service_worker.js:
Patch Set #21, Line 7:
chrome.test.assertEq(undefined, chrome.sidePanel);thanks for the coverage, I wonder if cases like these for other APIs are covered by a more specific test class/might this test be redundant?
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Attention is currently required from: Kelvin Jiang.
9 comments:
Patchset:
Thanks Kelvin!
File chrome/browser/extensions/api/side_panel/side_panel_apitest.cc:
Patch Set #21, Line 37:
UndefinedManifestKeytiny nit: `missing_manifest_key` may be may be a slightly more accurate name?
Done
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
using TabPanelOptions = base::flat_map<TabId, api::side_panel::PanelOptions>;
using ExtensionPanelOptions = base::flat_map<ExtensionId, TabPanelOptions>;
Based on what this CL entails, I don't see TabPanelOptions existing outside of an ExtensionPanelOpti […]
You're right that it might be unnecessary. LMK? ExtensionPanelOptions is only used explicitly once in this file and TabPanelOptions is only used twice in .cc. I may have added them thinking idealistically that it'd make the code easier to read with named complex-types.
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
Patch Set #21, Line 52:
// Maybe get default path if there are no records for a given extension_id.replace with TODO if this will be done in a follow up?
Wording updated.
Patch Set #21, Line 65:
return CloneOptions(specific_tab_options->second);
maybe I'm just confused but: do we really need to Clone() here since we're probably copying the obje […]
You might be right. The following comment seems directly related to what you're asking
crrev.com/c/3652544/10/chrome/browser/extensions/api/side_panel/side_panel_service.cc#66. I think the gist of that comment is returning a const& from the service. Any thoughts?
Patch Set #21, Line 65:
specific_tab_optionsmay need to take another pass but:
It looks like your comment was cut off here.
File chrome/common/extensions/api/side_panel.idl:
Patch Set #21, Line 13:
SidePanel side_panel;
hmm.jpg […]
This probably came from the API Proposal. I don't have a preference. Devlin, any preference here?
File chrome/test/data/extensions/api_test/side_panel/extension/service_worker.js:
Patch Set #21, Line 60:
defaultSetAndGetPanelAgain
nit: this is a duplicate function to what's above, maybe mention in the name that this is for a spec […]
Done
File chrome/test/data/extensions/api_test/side_panel/permission_missing/service_worker.js:
Patch Set #21, Line 7:
chrome.test.assertEq(undefined, chrome.sidePanel);
thanks for the coverage, I wonder if cases like these for other APIs are covered by a more specific […]
It would be okay to remove this `permission_missing` extension/test. I didn't know that we already had test coverage for this. LMK if you think we do and it can be deduplicated here?
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Devlin Cronin (Gerrit)
Attention is currently required from: Kelvin Jiang, Solomon Kinard.
Patch set 23:Code-Review +1
6 comments:
Patchset:
This looks great, Solomon! Only real comment left is the one about tabId behavior when returning a default. Otherwise, LGTM!
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
using TabPanelOptions = base::flat_map<TabId, api::side_panel::PanelOptions>;
using ExtensionPanelOptions = base::flat_map<ExtensionId, TabPanelOptions>;
thinking idealistically that it'd make the code easier to read with named complex-types.
I agree with this. I find these aliases make it much cleaner than inlining them all together. I'd keep the aliases.
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
Patch Set #16, Line 70:
options.tab_id = std::make_unique<int>(tab_id);`chrome.sidePanel.getOptions({})` is an example case when it doesn't make sense to return a `tabId`.
I agree with that, but we're being inconsistent in setting `tab_id` for different default cases.
Consider:
Case 1:
- Extension has a manifest entry, but has not explicitly set default options via the API.
- Extension calls `sidePanel.getOptions({tabId: 22});
In this case, we'd return the manifest-specified options and `tabId` would be undefined. The result would be something like:
```
{
enabled: true,
path: 'panel.html',
}
```Case 2:
- Extension has a manifest entry and *has* explicitly set default options via the API (but hasn't set any specific tab options)
- Extension calls `sidePanel.getOptions({tabId: 22});
Here, we'd return an options *with* the tab ID specified. The result would be something like:
```
{
enabled: true,
path: 'panel.html',
tabId: 22,
}
```Even though in both these cases, we return a default options value (and there's never a specific value for tabId 22), the second case has tabId set whereas the first does not. That seems inconsistent to me.
How about we don't set tabId in either case where a default is used? That way, developers could tell whether an option is being applied to that specific tab (in which case tabId would be set) or if a default (from either the API or manifest) is being used.
WDYT?
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
Patch Set #21, Line 65:
return CloneOptions(specific_tab_options->second);You might be right. The following comment seems directly related to what you're asking […]
We can't just return a const & to this here because we return a constructed object in other paths (and you can't return a temporary as a const &), and the generated types have their copy ctors deleted so we need an explicit Clone() method.
One way to get around that (as mentioned in the comment Solomon linked) is to have a static "empty options" that we can return in those cases; the other way is to return by value and copy the object in other cases (which is what this CL does). I don't have a strong preference either way, so I'd kinda default to the one that's written : )
File chrome/common/extensions/api/side_panel.idl:
Patch Set #21, Line 13:
SidePanel side_panel;
This probably came from the API Proposal. I don't have a preference. […]
Sorry, I don't follow - what do you mean "as a constant"?
(This entry allows us to use the generated types in order to do parsing of the manifest key)
File chrome/common/extensions/manifest_tests/extension_manifests_side_panel_unittest.cc:
Patch Set #23, Line 75:
base::DictionaryValue v;I don't think this value is used?
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Attention is currently required from: Kelvin Jiang.
4 comments:
Patchset:
Thanks. Kelvin, Can you take a look?
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
Patch Set #16, Line 70:
options.tab_id = std::make_unique<int>(tab_id);
How about we don't set tabId in either case where a default is used? That way, developers could tell whether an option is being applied to that specific tab (in which case tabId would be set) or if a default (from either the API or manifest) is being used.
Done.
File chrome/common/extensions/manifest_tests/extension_manifests_side_panel_unittest.cc:
Patch Set #23, Line 75:
base::DictionaryValue v;I don't think this value is used?
Done
File extensions/common/manifest_handlers/side_panel_info.cc:
if (extension->manifest_version() < 3) {
*error =
ErrorUtils::FormatErrorMessageUTF16(errors::kManifestMinimumError, "3");
return false;
}
Removed. It seems like your message is worded that way. Also removed associated tests.
Done
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Attention is currently required from: Alex Gough, David Trainor, Kelvin Jiang.
Solomon Kinard would like Alex Gough and David Trainor to review this change.
Extensions: SidePanel: Create manifest key(s), API, and storage
Design Doc:
https://docs.google.com/document/d/1EjRghIJR1u1-UadYHdBhCCtY3A108LtEznSWzz7FtwE
Bug: 1328645
Change-Id: Ic6cae327ac911b48a1ef9cb069f9efbd80bfd80a
---
M chrome/browser/extensions/BUILD.gn
A chrome/browser/extensions/api/side_panel/side_panel_api.cc
A chrome/browser/extensions/api/side_panel/side_panel_api.h
A chrome/browser/extensions/api/side_panel/side_panel_apitest.cc
A chrome/browser/extensions/api/side_panel/side_panel_service.cc
A chrome/browser/extensions/api/side_panel/side_panel_service.h
M chrome/browser/extensions/browser_context_keyed_service_factories.cc
M chrome/browser/profiles/profile_keyed_service_browsertest.cc
M chrome/common/BUILD.gn
M chrome/common/extensions/api/_api_features.json
M chrome/common/extensions/api/_manifest_features.json
M chrome/common/extensions/api/_permission_features.json
M chrome/common/extensions/api/api_sources.gni
A chrome/common/extensions/api/side_panel.idl
A chrome/common/extensions/api/side_panel/side_panel_info.cc
A chrome/common/extensions/api/side_panel/side_panel_info.h
M chrome/common/extensions/chrome_manifest_handlers.cc
A chrome/common/extensions/manifest_tests/extension_manifests_side_panel_unittest.cc
M chrome/common/extensions/permissions/chrome_api_permissions.cc
M chrome/common/extensions/permissions/permission_set_unittest.cc
M chrome/test/BUILD.gn
A chrome/test/data/extensions/api_test/side_panel/extension/default_path.html
A chrome/test/data/extensions/api_test/side_panel/extension/manifest.json
A chrome/test/data/extensions/api_test/side_panel/extension/panel.html
A chrome/test/data/extensions/api_test/side_panel/extension/service_worker.js
A chrome/test/data/extensions/api_test/side_panel/missing_manifest_key/manifest.json
A chrome/test/data/extensions/api_test/side_panel/missing_manifest_key/service_worker.js
A chrome/test/data/extensions/api_test/side_panel/permission_missing/manifest.json
A chrome/test/data/extensions/api_test/side_panel/permission_missing/service_worker.js
M extensions/browser/extension_function_histogram_value.h
M extensions/common/extension_features.cc
M extensions/common/extension_features.h
M extensions/common/manifest.cc
M extensions/common/manifest_constants.cc
M extensions/common/manifest_constants.h
M extensions/common/mojom/api_permission_id.mojom
M tools/metrics/histograms/enums.xml
37 files changed, 809 insertions(+), 1 deletion(-)
Solomon Kinard (Gerrit)
Attention is currently required from: Alex Gough, David Trainor, Kelvin Jiang.
1 comment:
Patchset:
Can specific file owners take a look?
+dtrainor: profile_keyed_service_browsertest.cc
+ajgo: api_permission_id.mojom
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Alex Gough (Gerrit)
Attention is currently required from: David Trainor, Kelvin Jiang, Solomon Kinard.
Patch set 25:Code-Review +1
1 comment:
Patchset:
lgtm mojom - thanks for linking the DD for context.
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
David Trainor (Gerrit)
Attention is currently required from: Kelvin Jiang, Solomon Kinard.
Patch set 26:Code-Review +1
1 comment:
Patchset:
profile_keyed_service_browsertest lgtm!
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Solomon Kinard.
Patch set 26:Code-Review +1
6 comments:
Patchset:
lgtm, thanks Solomon!
File chrome/browser/extensions/api/side_panel/side_panel_service.h:
using TabPanelOptions = base::flat_map<TabId, api::side_panel::PanelOptions>;
using ExtensionPanelOptions = base::flat_map<ExtensionId, TabPanelOptions>;> thinking idealistically that it'd make the code easier to read with named complex-types. […]
I was thinking instead of using something like `base::flatmap<pair<ExtensionId, TabId>, PanelOptions>` but it's probably overkill in this case
File chrome/browser/extensions/api/side_panel/side_panel_service.cc:
Patch Set #21, Line 65:
specific_tab_options
It looks like your comment was cut off here.
oops, this was from another tab and I forgot I had this draft... (resolving)
Patch Set #21, Line 65:
return CloneOptions(specific_tab_options->second);
We can't just return a const & to this here because we return a constructed object in other paths (a […]
I'm fine with just copying the object (cloning)
File chrome/common/extensions/api/side_panel.idl:
Patch Set #21, Line 13:
SidePanel side_panel;
Sorry, I don't follow - what do you mean "as a constant"? […]
^oh I see. was thinking if we could just have a constant in a .h file instead of adding it again here
(resolving comment)
File chrome/test/data/extensions/api_test/side_panel/permission_missing/service_worker.js:
Patch Set #21, Line 7:
chrome.test.assertEq(undefined, chrome.sidePanel);
It would be okay to remove this `permission_missing` extension/test. […]
Not sure... this was just a guess based on the existence if `_api_features.json`, I'm fine with leaving this test in.
@Devlin, do you know which tests would cover what we list in `_api_features.json` for permissions?
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.
Solomon Kinard (Gerrit)
Attention is currently required from: Devlin Cronin, Solomon Kinard.
Patch set 28:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
26 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: tools/metrics/histograms/enums.xml
Insertions: 4, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: extensions/common/extension_features.h
Insertions: 2, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: extensions/common/extension_features.cc
Insertions: 8, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: extensions/browser/extension_function_histogram_value.h
Insertions: 4, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Extensions: SidePanel: Create manifest key(s), API, and storage
Design Doc:
https://docs.google.com/document/d/1EjRghIJR1u1-UadYHdBhCCtY3A108LtEznSWzz7FtwE
Bug: 1328645
Change-Id: Ic6cae327ac911b48a1ef9cb069f9efbd80bfd80a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3652544
Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
Reviewed-by: Alex Gough <aj...@chromium.org>
Reviewed-by: Kelvin Jiang <kelvi...@chromium.org>
Commit-Queue: Solomon Kinard <solomo...@chromium.org>
Reviewed-by: David Trainor <dtra...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1023830}
37 files changed, 816 insertions(+), 1 deletion(-)
Devlin Cronin (Gerrit)
1 comment:
File chrome/test/data/extensions/api_test/side_panel/permission_missing/service_worker.js:
Patch Set #21, Line 7:
chrome.test.assertEq(undefined, chrome.sidePanel);
Not sure... this was just a guess based on the existence if `_api_features. […]
Following up here, this exercises the basic features and bindings system, which, depending on the level you're interested in, is tested in extension_bindings_apitest.cc, native_bindings_apitest.cc, simple/complex_feature_unittest.cc, and a few others. Those test that features are unavailable under certain conditions. What they _don't_ test is that the definition for side panel in _api_features.json is properly entered (i.e., lists the dependency on the permission), and that's exercised by this. We don't usually require that (because the definitions in features.json are usually fairly straightforward and may generate compile / runtime errors if malformed), but it never hurts : )
To view, visit change 3652544. To unsubscribe, or for help writing mail filters, visit settings.