[Cast Receiver] Boostrap Protos Integration [chromium/src : main]
Richard Nichols (Gerrit)
Richard Nichols added 26 comments![]( "Open in Gerrit")
[Cast Receiver] Boostrap Protos IntegrationPlease fix this WARNING reported by Spellchecker: "Boostrap" is a possible misspelling of "Bootstrap".
To bypass Spellchecker, ad...
"Boostrap" is a possible misspelling of "Bootstrap".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
bool GetExtendedInputSupported(
const cast::common::ApplicationConfig& core_config) {
const auto& extra_features = core_config.extra_features().entries();
auto it =
std::ranges::find(extra_features, feature::kCastCoreRendererFeatures,
&cast::common::Dictionary::Entry::key);
if (it == extra_features.end()) {
return false;
}
if (!it->value().has_dictionary()) {
return false;
}
const auto& entries = it->value().dictionary().entries();
auto feature_it = std::ranges::find(entries, feature::kExtendedInputSupported,
&cast::common::Dictionary::Entry::key);
return feature_it != entries.end();
}I don't get why this cl has overlap with https://chromium-review.git.corp.google.com/c/chromium/src/+/7962739
did you mean to chain commits together and upload individual commits? That'd help with the lint/warning about a large cl.
class MockEmbedderApplication : public EmbedderApplication {
public:
MOCK_METHOD0(NotifyApplicationStarted, void());
MOCK_METHOD2(NotifyApplicationStopped, void(ApplicationStopReason, int32_t));
MOCK_METHOD1(NotifyMediaPlaybackChanged, void(bool));
MOCK_METHOD1(GetAllBindings, void(GetAllBindingsCallback));
MOCK_METHOD0(GetMessagePortService, MessagePortService*());
MOCK_METHOD0(GetWebContents, content::WebContents*());
MOCK_METHOD0(GetContentWindowControls, ContentWindowControls*());
MOCK_METHOD0(GetStreamingConfigManager, StreamingConfigManager*());
};
class MockMessagePortService : public MessagePortService {
public:
MOCK_METHOD2(ConnectToPortAsync,
void(std::string_view,
std::unique_ptr<cast_api_bindings::MessagePort>));
MOCK_METHOD1(RegisterOutgoingPort,
uint32_t(std::unique_ptr<cast_api_bindings::MessagePort>));
MOCK_METHOD2(RegisterIncomingPort,
void(uint32_t, std::unique_ptr<cast_api_bindings::MessagePort>));
MOCK_METHOD1(Remove, void(uint32_t));
};
class MockContentWindowControls : public ContentWindowControls {
public:
MOCK_METHOD0(ShowWindow, void());
MOCK_METHOD0(HideWindow, void());
MOCK_METHOD0(EnableTouchInput, void());
MOCK_METHOD0(DisableTouchInput, void());
};are these useful in other tests? would it be good to have them in a header instead?
// The user id representing the user that the application should be launched
// as. On Android, this is the ID returned by UserHandle.getIdentifier().all things that wind up in chromium will be public. I'm not sure if the exo portion is also part of aosp or not. If it isn't, you probably don't want this type of comment here.
basically, be careful about any comments/commit messages/etc that wind up going public at one point or another.
// StatusBarNotification.getKey().hashCode().this may change in the future I imagine. If this is removed the comment name is pretty low value as it just repeats the name of the field.
string data = 3;if this is a data payload, it is proto? json? "unknown" - should it be bytes instead of string?
// The hashcode of the StatusBarNotification for detecting if the notificationwhy is a hash being stored in a string? how many bits of entropy do you need to determine uniqueness?
// The user id representing the user that the application should be launched
// as. On Android, this is the ID returned by UserHandle.getIdentifier().whatever happens with the prior android comment do the same here.
// The package name of the application to be launched.
string package_name = 2;is this a reverse launch as in the receiver launching something on the sender device? I vaguely recall one of the features for Cast being that.
Basically, I don't get why we have what amounts to an android package name here when we're already in chromium and can only handle web urls and not package names.
If this is the first assumption this file could use some clarifying, if it's the second you probably need to trim off the protos values or include a link/reference to where it is copied from and what it needs to stay in sync with.
int32 height_px = 2;in rendering we usually care about the offsets too. Is there any chance that there needs to be a buffer space on the window (as in, we're not starting the render from 0,0 and instead some other biased offset).
may be overkill, but I'm always a fan of trying to stick named types in that're as close to a normal gpu rep for graphics code as we can. (e.g. vec2, vec3, vec4)
// game is launched or a video is played in full screen in Youtube), noI don't think we should be referencing a single product type here. Also this sounds like Youtube has a commitment to this feature already and/or that it's already in. Which they probably don't have.
// Error information if bootstrapping failed.this and the above comment are self explanatory based on the field name.
UID uid = 1;remove the need for a comment, just label the field `device_uid`. ideally _what_ device.
repeated ResolutionInfo resolutions = 2;similarly, `supported_resolutions` makes reading the code clearer, rather than having to look at comments which may be a royal pain to even look up depending on how the code is being investigated. e.g. if we're stuck using vscode to look at src finding where a definition is can be extremely tedious.
// Supported input simulation capabilities (Touch, Keyboard, Mouse).to..? This may just be me not knowing exo. is this the receiver/sender input capabilities? allowed input capabilities? what's it doing. whatever the answer is, rename the field to have the intent.
int32 version = 3;`bootstrap_protocol_version`/`bootstrap_version`?
NO_CDM_ASSOCIATION_ERROR = 1;I hate it, but protos generally don't handle enum class like we'd want in C++. Typically we have them prefixed with what enum type name too.
// Deprectated field ActionInvocation is unused as we launch notificationsPlease fix this WARNING reported by Spellchecker: "Deprectated" is a possible misspelling of "Deprecated".
To bypass Spellchecker...
"Deprectated" is a possible misspelling of "Deprecated".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
ActionInvocation action_invocation = 2 [deprecated = true];if it's deprecated why are we including it? where is this copied from?
bool any_accessibility_features_enabled = 6;do we not need to care about what accessibility feature is enabled?
// New field containing client details.low value comment.
BootstrappingSystem system = 1;`system_type` + rename enum to match (and fix enum not including `BOOTSTRAPPING_`
string user_agent = 3;is this _actually_ needed, or is this for metrics? I know there's always a pile of PII stuff. does this need some PII consideration or are we fine?
message UID {what's wrong with using a uuid instead?
SIMPLE_RANDOM = 2;is there any functional reason why we shouldn't stick to 1 random type? what happens if they're mixed? are there some statistical guarantees we need to be worried about here?
Related details
- Dewita Sonya
- Luke Halliwell
- Shawn Quereshi
- Simeon Anfinrud
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |