Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750463006:
ChromePrompt Mojo IPC interface (Closed)
|
Created:
3 years, 9 months ago by ftirelo Modified:
3 years, 9 months ago CC:
Joe Mason Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionChromePrompt Mojo IPC interface.
The interface will be pulled via DEPS in both Chromium (server side) and Software Reporter (client side) repos. Interface implementation will be landed in the Chromium repo.
BUG=690020
R=csharp@chromium.org, grt@chromium.org, joenotcharles@google.com, rockot@chromium.org
Committed: https://chromium.googlesource.com/chromium/pipa/+/ef74310fb4d63c737b8a888c2ea72b4e458218ef
Patch Set 1 #Patch Set 2 : Additional comment #
Total comments: 21
Patch Set 3 : Code review #
Total comments: 24
Patch Set 4 : Code review #Patch Set 5 : Final comments #Patch Set 6 : Add not_shown in prompt enum #Patch Set 7 : Fix typo #
Total comments: 12
Patch Set 8 : Next round of reviews #
Total comments: 2
Patch Set 9 : Observed behaviours as a struct of bools #
Total comments: 1
Patch Set 10 : Standard directory layout #
Messages
Total messages: 29 (6 generated)
Description was changed from ========== ChromePrompt Mojo IPC interface BUG= ========== to ========== ChromePrompt Mojo IPC interface. The interface will be pulled via DEPS in both Chromium (server side) and Software Reporter (client side) repos. Interface implementation will be landed in the Chromium repo. BUG= ==========
ftirelo@google.com changed reviewers: + csharp@chromium.org, ftirelo@google.com, grt@chromium.org, rockot@chromium.org
ftirelo@chromium.org changed reviewers: - ftirelo@google.com
Description was changed from ========== ChromePrompt Mojo IPC interface. The interface will be pulled via DEPS in both Chromium (server side) and Software Reporter (client side) repos. Interface implementation will be landed in the Chromium repo. BUG= ========== to ========== ChromePrompt Mojo IPC interface. The interface will be pulled via DEPS in both Chromium (server side) and Software Reporter (client side) repos. Interface implementation will be landed in the Chromium repo. BUG=690020 ==========
https://codereview.chromium.org/2750463006/diff/20001/GITDEPS File GITDEPS (right): https://codereview.chromium.org/2750463006/diff/20001/GITDEPS#newcode53 GITDEPS:53: "third_party/Webkit", I would probably try to avoid bringing this in if you can since it's such a huge dependency. The ties within //mojo are minimal and should be easy enough to conditionally sever. https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... File pipa/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:43: PromptUser(PromptUserRequest request) => (PromptUserResponse response); nit: If these Request and Response types are only used for PromptUser, and you don't expect the number of fields to grow too large, it's probably cleaner to just inline the fields as message arguments? PromptUser(array<UwS> removable_uws, bool elevation_required) => (bool accepted, bool upload_logs);
Thanks for the suggestions. Below my responses. https://codereview.chromium.org/2750463006/diff/20001/GITDEPS File GITDEPS (right): https://codereview.chromium.org/2750463006/diff/20001/GITDEPS#newcode53 GITDEPS:53: "third_party/Webkit", On 2017/03/14 18:13:06, Ken Rockot wrote: > I would probably try to avoid bringing this in if you can since it's such a huge > dependency. The ties within //mojo are minimal and should be easy enough to > conditionally sever. For the moment, this will be needed, because of this dependency: https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/BUILD.gn?l=173 Once crbug.com/617718 is fixed, we should be able to get rid of it in the Pipa repo. I'm not too concerned overall, since we depend on WebKit in the Reporter anyway. ------------------------------------------- Error message when this is compiled: D:\Workspace\pipa\src>ninja -C out\Default chrome_prompt ninja: Entering directory `out\Default' [1/1] Regenerating ninja files FAILED: build.ninja D:/Workspace/pipa/src/buildtools/win/gn.exe --root=D:/Workspace/pipa/src -q gen . ERROR at //mojo/public/cpp/bindings/BUILD.gn:186:7: Can't load input file. "//third_party/WebKit/Source/wtf", ^-------------------------------- Unable to load: D:/Workspace/pipa/src/third_party/WebKit/Source/wtf/BUILD.gn I also checked in the secondary tree for: D:/Workspace/pipa/src/build/secondary/third_party/WebKit/Source/wtf/BUILD.gn https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... File pipa/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:43: PromptUser(PromptUserRequest request) => (PromptUserResponse response); On 2017/03/14 18:13:06, Ken Rockot wrote: > nit: If these Request and Response types are only used for PromptUser, and you > don't expect the number of fields to grow too large, it's probably cleaner to > just inline the fields as message arguments? > > PromptUser(array<UwS> removable_uws, bool elevation_required) > => (bool accepted, bool upload_logs); The main reason is that we expect this to grow with time, specially in v2. But we are also concerned about possible divergences between the Chrome and the Reporter binaries, when we update the list, since they don't follow the same release cycle. What would happen if, for example, the Reporter sees: PromptUser(removable_uws, elevation_required) but Chrome sees: PromptUser(removable_uws, elevation_required, another_field) ?
On 2017/03/14 at 20:26:58, ftirelo wrote: > Thanks for the suggestions. Below my responses. > > https://codereview.chromium.org/2750463006/diff/20001/GITDEPS > File GITDEPS (right): > > https://codereview.chromium.org/2750463006/diff/20001/GITDEPS#newcode53 > GITDEPS:53: "third_party/Webkit", > On 2017/03/14 18:13:06, Ken Rockot wrote: > > I would probably try to avoid bringing this in if you can since it's such a huge > > dependency. The ties within //mojo are minimal and should be easy enough to > > conditionally sever. > > For the moment, this will be needed, because of this dependency: https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/BUILD.gn?l=173 > > Once crbug.com/617718 is fixed, we should be able to get rid of it in the Pipa repo. I'm not too concerned overall, since we depend on WebKit in the Reporter anyway. > > ------------------------------------------- > > Error message when this is compiled: > > D:\Workspace\pipa\src>ninja -C out\Default chrome_prompt > ninja: Entering directory `out\Default' > [1/1] Regenerating ninja files > FAILED: build.ninja > D:/Workspace/pipa/src/buildtools/win/gn.exe --root=D:/Workspace/pipa/src -q gen . > ERROR at //mojo/public/cpp/bindings/BUILD.gn:186:7: Can't load input file. > "//third_party/WebKit/Source/wtf", > ^-------------------------------- > Unable to load: > D:/Workspace/pipa/src/third_party/WebKit/Source/wtf/BUILD.gn > I also checked in the secondary tree for: > D:/Workspace/pipa/src/build/secondary/third_party/WebKit/Source/wtf/BUILD.gn > > https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... > File pipa/mojom/chrome_prompt.mojom (right): > > https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... > pipa/mojom/chrome_prompt.mojom:43: PromptUser(PromptUserRequest request) => (PromptUserResponse response); > On 2017/03/14 18:13:06, Ken Rockot wrote: > > nit: If these Request and Response types are only used for PromptUser, and you > > don't expect the number of fields to grow too large, it's probably cleaner to > > just inline the fields as message arguments? > > > > PromptUser(array<UwS> removable_uws, bool elevation_required) > > => (bool accepted, bool upload_logs); > > The main reason is that we expect this to grow with time, specially in v2. But we are also concerned about possible divergences between the Chrome and the Reporter binaries, when we update the list, since they don't follow the same release cycle. What would happen if, for example, the Reporter sees: > PromptUser(removable_uws, elevation_required) > but Chrome sees: > PromptUser(removable_uws, elevation_required, another_field) ? Similar to struct fields, message arguments can be versioned (message parameter lists are in fact just anonymous structs, as an implementation detail).
what repo is this in? is there already a top-level OWNERS file? if no, should there be one in the pipa directory? https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... File pipa/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:1: // Copyright 2017 The Chromium Authors. All Rights Reserved. why not the full license header? https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:8: SETTINGS_HIJACKER= 2, nit: missing whitespace before '=' https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:15: string name; nit: document this field https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:16: // List of behaviors to be presented to the user. nit: in c++ style, we have a blank line separating the previous member from the next member's comment. does mojo style deviate from this? https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:18: // List of files that will be deleted by the Chrome Cleanup Tool for this can you be more specific? is it a list of fully-qualified path names of files? https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:31: // The request sent by Chrome to the Software Reporter Tool with the user request -> response? https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:40: // Service invoked by the Software Reporter Tool to request Chrome to prompt nit: document the service from the other side, since it's implemented by Chrome rather than by the SRT. maybe: // A service provided by Chrome to process request to prompt the user to remove unwanted software. This service is used by the Software Reporter Tool so that the removal user experience is provided by Chrome. or something like that
We will use this repo to define the IPC interface and utility libs shared by Chrome and the SRT. This repo will be pulled via deps to both Chrome and SRT repos. Since we only pull Chromium deps in the SRT repo on commits that are in the current stable version, checking this in the Chromium repo would tie updates on the SRT IPC to the Chromium release cycle. In order to avoid this, we decided to use a separate repo instead. https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... File pipa/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:1: // Copyright 2017 The Chromium Authors. All Rights Reserved. On 2017/03/15 10:58:13, grt (UTC plus 1) wrote: > why not the full license header? No reason. Copied the header from the internal reporter repo, which only contains the first line. Thanks for noticing that. https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:8: SETTINGS_HIJACKER= 2, On 2017/03/15 10:58:13, grt (UTC plus 1) wrote: > nit: missing whitespace before '=' I think I'm relying too much on git cl format :) https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:15: string name; On 2017/03/15 10:58:13, grt (UTC plus 1) wrote: > nit: document this field Done. https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:16: // List of behaviors to be presented to the user. On 2017/03/15 10:58:13, grt (UTC plus 1) wrote: > nit: in c++ style, we have a blank line separating the previous member from the > next member's comment. does mojo style deviate from this? A quick search didn't show any style-guide for Mojo. So, I'll try to be as close to C++ style as possible. https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:18: // List of files that will be deleted by the Chrome Cleanup Tool for this On 2017/03/15 10:58:13, grt (UTC plus 1) wrote: > can you be more specific? is it a list of fully-qualified path names of files? Done. https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:31: // The request sent by Chrome to the Software Reporter Tool with the user On 2017/03/15 10:58:13, grt (UTC plus 1) wrote: > request -> response? Done. https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:40: // Service invoked by the Software Reporter Tool to request Chrome to prompt On 2017/03/15 10:58:13, grt (UTC plus 1) wrote: > nit: document the service from the other side, since it's implemented by Chrome > rather than by the SRT. maybe: > > // A service provided by Chrome to process request to prompt the user to remove > unwanted software. This service is used by the Software Reporter Tool so that > the removal user experience is provided by Chrome. > > or something like that Done.
joenotcharles@google.com changed reviewers: + joenotcharles@google.com
https://codereview.chromium.org/2750463006/diff/40001/GITDEPS File GITDEPS (right): https://codereview.chromium.org/2750463006/diff/40001/GITDEPS#newcode53 GITDEPS:53: "third_party/Webkit", Can you also add a comment explaining why this is needed? it's the only one that seems insane as a mojo dependency. https://codereview.chromium.org/2750463006/diff/40001/OWNERS File OWNERS (right): https://codereview.chromium.org/2750463006/diff/40001/OWNERS#newcode1 OWNERS:1: alito@chromium.org Should this be a team name if the idea is for everyone on Protector be in this list? Otherwise it'll go out of date as people move around. https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... File pipa/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:5: module pipa.mojom; I wonder if we should use the name "chrome_cleaner.mojom" for the module. It's more obvious what it means from in Chromium code, and from Foil code it means the mojom file is in the same namespace as our protobufs, which seems reasonable. https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:18: string name; Should we include the ID here too? https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:52: PromptUser(PromptUserRequest request) => (PromptUserResponse response); I think the Request / Response structs obscure what's happening here because they're an extra level of indirection. Under the hood PromptUser is serialized into a message containing the name and a bundle of info that goes with that message, and "=>" tells us we can expect a response with another bundle of info. Having that info tagged "PromptUserRequest" because it's part of a struct doesn't add anything because we already know this is a PromptUser message, and that the left side is the request and the right side is the response. And having the interface "PromptUser(removable_uws_found, elevation_required) => (prompt_accepted, logs_uploading_enabled)" makes it easier to tell what's changed if we look at different versions of the .mojom file: there's no chance we'll look at just the "PromptUser" message and think the parameters haven't changed because we didn't look at the structs, and if we see a diff between two structs we don't need to search for which message uses them.
https://codereview.chromium.org/2750463006/diff/40001/DEPS File DEPS (right): https://codereview.chromium.org/2750463006/diff/40001/DEPS#newcode21 DEPS:21: "chromium_git": "https://chromium.googlesource.com", nit: Please add a blank line after this https://codereview.chromium.org/2750463006/diff/40001/DEPS#newcode24 DEPS:24: # Three lines of non-changing comments so that I don't think we need these 3 lines since I don't think we will have multiple people trying to change different deps at the same time (which is why chromium needs this) https://codereview.chromium.org/2750463006/diff/40001/DEPS#newcode27 DEPS:27: "catapult_revision": "e9f547be045d4e50cb2d4927261d500e425c364c", nit: Move to after buildtools_revision https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... File pipa/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:44: // If true, the user agrees with sending detailed logs information to Google. nit: Worth also mentioning crashes? Maybe the variable name would also need to change
https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... File pipa/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:1: // Copyright 2017 The Chromium Authors. All Rights Reserved. On 2017/03/15 14:27:58, ftirelo wrote: > On 2017/03/15 10:58:13, grt (UTC plus 1) wrote: > > why not the full license header? > > No reason. Copied the header from the internal reporter repo, which only > contains the first line. Thanks for noticing that. Just to confirm: this new repo you're creating should contain open-source code under the same license as Chromium? https://codereview.chromium.org/2750463006/diff/40001/DEPS File DEPS (right): https://codereview.chromium.org/2750463006/diff/40001/DEPS#newcode36 DEPS:36: Var("root") + "/third_party/catapult": nit: please sort these a la csharp's comment above. https://codereview.chromium.org/2750463006/diff/40001/OWNERS File OWNERS (right): https://codereview.chromium.org/2750463006/diff/40001/OWNERS#newcode1 OWNERS:1: alito@chromium.org On 2017/03/15 16:10:00, joenotcharles wrote: > Should this be a team name if the idea is for everyone on Protector be in this > list? Otherwise it'll go out of date as people move around. Are TEAM/COMPONENT relevant here (https://docs.google.com/document/d/1jty6UsFMW9-SYgpQC-ztEc3lltziOQBBArMkROCST... https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... File pipa/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:5: module pipa.mojom; On 2017/03/15 16:10:00, joenotcharles wrote: > I wonder if we should use the name "chrome_cleaner.mojom" for the module. It's > more obvious what it means from in Chromium code, and from Foil code it means > the mojom file is in the same namespace as our protobufs, which seems > reasonable. $0.02: "pipa" means nothing to me, and is counter to the Google C++ style for naming: https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
PTAL https://codereview.chromium.org/2750463006/diff/20001/GITDEPS File GITDEPS (right): https://codereview.chromium.org/2750463006/diff/20001/GITDEPS#newcode53 GITDEPS:53: "third_party/Webkit", On 2017/03/14 20:26:58, ftirelo wrote: > On 2017/03/14 18:13:06, Ken Rockot wrote: > > I would probably try to avoid bringing this in if you can since it's such a > huge > > dependency. The ties within //mojo are minimal and should be easy enough to > > conditionally sever. > > For the moment, this will be needed, because of this dependency: > https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/BUILD.gn?l=173 > > Once crbug.com/617718 is fixed, we should be able to get rid of it in the Pipa > repo. I'm not too concerned overall, since we depend on WebKit in the Reporter > anyway. > > ------------------------------------------- > > Error message when this is compiled: > > D:\Workspace\pipa\src>ninja -C out\Default chrome_prompt > ninja: Entering directory `out\Default' > [1/1] Regenerating ninja files > FAILED: build.ninja > D:/Workspace/pipa/src/buildtools/win/gn.exe --root=D:/Workspace/pipa/src -q gen > . > ERROR at //mojo/public/cpp/bindings/BUILD.gn:186:7: Can't load input file. > "//third_party/WebKit/Source/wtf", > ^-------------------------------- > Unable to load: > D:/Workspace/pipa/src/third_party/WebKit/Source/wtf/BUILD.gn > I also checked in the secondary tree for: > D:/Workspace/pipa/src/build/secondary/third_party/WebKit/Source/wtf/BUILD.gn Done. https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... File pipa/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/20001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:1: // Copyright 2017 The Chromium Authors. All Rights Reserved. On 2017/03/16 09:02:10, grt (UTC plus 1) wrote: > On 2017/03/15 14:27:58, ftirelo wrote: > > On 2017/03/15 10:58:13, grt (UTC plus 1) wrote: > > > why not the full license header? > > > > No reason. Copied the header from the internal reporter repo, which only > > contains the first line. Thanks for noticing that. > > Just to confirm: this new repo you're creating should contain open-source code > under the same license as Chromium? Yes. Same license as Chromium. Our original goal was for this code to reside in the Chromium repo, but rolling out new versions in the reporter repo would not be feasible, which led us to the new repo. https://codereview.chromium.org/2750463006/diff/40001/DEPS File DEPS (right): https://codereview.chromium.org/2750463006/diff/40001/DEPS#newcode21 DEPS:21: "chromium_git": "https://chromium.googlesource.com", On 2017/03/15 17:22:34, csharp wrote: > nit: Please add a blank line after this Done. https://codereview.chromium.org/2750463006/diff/40001/DEPS#newcode24 DEPS:24: # Three lines of non-changing comments so that On 2017/03/15 17:22:34, csharp wrote: > I don't think we need these 3 lines since I don't think we will have multiple > people trying to change different deps at the same time (which is why chromium > needs this) Done. https://codereview.chromium.org/2750463006/diff/40001/DEPS#newcode27 DEPS:27: "catapult_revision": "e9f547be045d4e50cb2d4927261d500e425c364c", On 2017/03/15 17:22:34, csharp wrote: > nit: Move to after buildtools_revision Done. https://codereview.chromium.org/2750463006/diff/40001/DEPS#newcode36 DEPS:36: Var("root") + "/third_party/catapult": On 2017/03/16 09:02:10, grt (UTC plus 1) wrote: > nit: please sort these a la csharp's comment above. Done. https://codereview.chromium.org/2750463006/diff/40001/GITDEPS File GITDEPS (right): https://codereview.chromium.org/2750463006/diff/40001/GITDEPS#newcode53 GITDEPS:53: "third_party/Webkit", On 2017/03/15 16:10:00, joenotcharles wrote: > Can you also add a comment explaining why this is needed? it's the only one that > seems insane as a mojo dependency. Done. https://codereview.chromium.org/2750463006/diff/40001/OWNERS File OWNERS (right): https://codereview.chromium.org/2750463006/diff/40001/OWNERS#newcode1 OWNERS:1: alito@chromium.org On 2017/03/15 16:10:00, joenotcharles wrote: > Should this be a team name if the idea is for everyone on Protector be in this > list? Otherwise it'll go out of date as people move around. We don't have a @chromium.org account from the team. We can do that, but I'd rather discuss it outside the scope of this CL. https://codereview.chromium.org/2750463006/diff/40001/OWNERS#newcode1 OWNERS:1: alito@chromium.org On 2017/03/16 09:02:11, grt (UTC plus 1) wrote: > On 2017/03/15 16:10:00, joenotcharles wrote: > > Should this be a team name if the idea is for everyone on Protector be in this > > list? Otherwise it'll go out of date as people move around. > > Are TEAM/COMPONENT relevant here > (https://docs.google.com/document/d/1jty6UsFMW9-SYgpQC-ztEc3lltziOQBBArMkROCST... We discussed internally and decided to only expose people we expect to be actively contributing to the repo instead of the whole team. https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... File pipa/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:5: module pipa.mojom; On 2017/03/16 09:02:11, grt (UTC plus 1) wrote: > On 2017/03/15 16:10:00, joenotcharles wrote: > > I wonder if we should use the name "chrome_cleaner.mojom" for the module. It's > > more obvious what it means from in Chromium code, and from Foil code it means > > the mojom file is in the same namespace as our protobufs, which seems > > reasonable. > > $0.02: "pipa" means nothing to me, and is counter to the Google C++ style for > naming: https://google.github.io/styleguide/cppguide.html#General_Naming_Rules Moved the code to the chrome_cleaner namespace and defined a top-level dir for chrome_cleaner. Kept pipa/ for repo config code. https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:5: module pipa.mojom; On 2017/03/15 16:10:00, joenotcharles wrote: > I wonder if we should use the name "chrome_cleaner.mojom" for the module. It's > more obvious what it means from in Chromium code, and from Foil code it means > the mojom file is in the same namespace as our protobufs, which seems > reasonable. Done. https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:18: string name; On 2017/03/15 16:10:00, joenotcharles wrote: > Should we include the ID here too? Done. https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:44: // If true, the user agrees with sending detailed logs information to Google. On 2017/03/15 17:22:34, csharp wrote: > nit: Worth also mentioning crashes? Maybe the variable name would also need to > change We will not send crash reports for users in SBER2, only for users with metrics reporting enabled. For crash reporting, we will still rely on the command line switch sent by Chrome. https://codereview.chromium.org/2750463006/diff/40001/pipa/mojom/chrome_promp... pipa/mojom/chrome_prompt.mojom:52: PromptUser(PromptUserRequest request) => (PromptUserResponse response); On 2017/03/15 16:10:00, joenotcharles wrote: > I think the Request / Response structs obscure what's happening here because > they're an extra level of indirection. Under the hood PromptUser is serialized > into a message containing the name and a bundle of info that goes with that > message, and "=>" tells us we can expect a response with another bundle of info. > Having that info tagged "PromptUserRequest" because it's part of a struct > doesn't add anything because we already know this is a PromptUser message, and > that the left side is the request and the right side is the response. > > And having the interface "PromptUser(removable_uws_found, elevation_required) => > (prompt_accepted, logs_uploading_enabled)" makes it easier to tell what's > changed if we look at different versions of the .mojom file: there's no chance > we'll look at just the "PromptUser" message and think the parameters haven't > changed because we didn't look at the structs, and if we see a diff between two > structs we don't need to search for which message uses them. Done.
i'm getting down to the nitty-nits here. looks pretty good. https://codereview.chromium.org/2750463006/diff/120001/DEPS File DEPS (right): https://codereview.chromium.org/2750463006/diff/120001/DEPS#newcode1 DEPS:1: # Copyright 2014 Google Inc. All Rights Reserved. was this file originally committed in 2017? https://codereview.chromium.org/2750463006/diff/120001/GITDEPS File GITDEPS (right): https://codereview.chromium.org/2750463006/diff/120001/GITDEPS#newcode1 GITDEPS:1: # Copyright 2015 Google Inc. All Rights Reserved. was this file originally committed in 2017? https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... File chrome_cleaner/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... chrome_cleaner/mojom/chrome_prompt.mojom:25: array<ObservedBehaviour> observed_behaviours; is it easier for consumers of this api for this to be a bitfield rather than an array? (not sure if this can be accomplished in mojom or not, so pardon if this is a silly question.) https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... chrome_cleaner/mojom/chrome_prompt.mojom:48: // if unwanted software is detected in the system. This service is used by the nit: "...on the system..." seems more natural to me than "in" it. https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... chrome_cleaner/mojom/chrome_prompt.mojom:56: // - logs_uploading_enabled: if the user accepted the prompt, indicates if does it make sense for this to be removed and for ACCEPTED to be split into ACCEPTED and ACCEPTED_WITH_LOGS? as it is, this value is only relevant for one single acceptance code, correct? will this change down the road?
PTAnL The only point I'd like to discuss a bit more is related to bitfields and enum class. Mojo enums are compiled as enum class, and so creating and querying the bitfield will require static casts. If you don't see any issue with that, I'm fine with the uint64 representation in the new patch. https://codereview.chromium.org/2750463006/diff/120001/DEPS File DEPS (right): https://codereview.chromium.org/2750463006/diff/120001/DEPS#newcode1 DEPS:1: # Copyright 2014 Google Inc. All Rights Reserved. On 2017/03/22 13:48:13, grt (UTC plus 1) wrote: > was this file originally committed in 2017? :-) Also changed authorship to Chromium. Will fix other files in this repo in a separate CL. https://codereview.chromium.org/2750463006/diff/120001/GITDEPS File GITDEPS (right): https://codereview.chromium.org/2750463006/diff/120001/GITDEPS#newcode1 GITDEPS:1: # Copyright 2015 Google Inc. All Rights Reserved. On 2017/03/22 13:48:13, grt (UTC plus 1) wrote: > was this file originally committed in 2017? Done. https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... File chrome_cleaner/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... chrome_cleaner/mojom/chrome_prompt.mojom:25: array<ObservedBehaviour> observed_behaviours; On 2017/03/22 13:48:14, grt (UTC plus 1) wrote: > is it easier for consumers of this api for this to be a bitfield rather than an > array? (not sure if this can be accomplished in mojom or not, so pardon if this > is a silly question.) It's not silly at all. In the compiled code, a Mojo enum are represented by an enum class, so converting to and from the enum will require casts. However, I think it's easy enough to wrap the conversions in auxiliary functions, so I changed this to int instead. Another alternative is to define ObservedBehaviour's enumerators as uint64 constants, with the expense of not having the enum's scoping protection. It may not be a big deal though, since we are already in a separate namespace (mojom). WDYT? https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... chrome_cleaner/mojom/chrome_prompt.mojom:48: // if unwanted software is detected in the system. This service is used by the On 2017/03/22 13:48:14, grt (UTC plus 1) wrote: > nit: "...on the system..." seems more natural to me than "in" it. Prepositions... Thanks for the correction :-) https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... chrome_cleaner/mojom/chrome_prompt.mojom:56: // - logs_uploading_enabled: if the user accepted the prompt, indicates if On 2017/03/22 13:48:14, grt (UTC plus 1) wrote: > does it make sense for this to be removed and for ACCEPTED to be split into > ACCEPTED and ACCEPTED_WITH_LOGS? as it is, this value is only relevant for one > single acceptance code, correct? will this change down the road? It doesn't make sense to have logs uploading with other option than ACCEPT, so I think it makes sense to merge both. Given that we want the permissions model to be as simple as possible to the user, I think it's very unlikely that we change logs permissions to allow, for example, different detail levels - only matched files, detailed system report. If that happens, we would need to change this anyway, so I'd rather not over-engineer now.
https://codereview.chromium.org/2750463006/diff/70008/chrome_cleaner/mojom/ch... File chrome_cleaner/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/70008/chrome_cleaner/mojom/ch... chrome_cleaner/mojom/chrome_prompt.mojom:8: enum ObservedBehaviour { Yeah, please don't use enums for bitfields. There's a reason enum classes don't support this and a reason we use enum classes in the generated code. A uint64 field with separate flag value constants is a reasonable option. Another totally reasonable option IMHO is to use an explicit struct of bools: struct ObservedBehaviorFlags { bool ads_injector; bool settings_hijacker; bool extension_injector; bool dropper; }; And in fact you could just inline the bools in the UwS struct, eliminating the (albeit tiny) extra overhead of a secondary struct. bools in a struct are tightly packed into bitfields and therefore space efficient. It's also probably easier to read, and it's naturally extensible.
https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... File chrome_cleaner/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... chrome_cleaner/mojom/chrome_prompt.mojom:25: array<ObservedBehaviour> observed_behaviours; On 2017/03/22 14:25:36, ftirelo wrote: > On 2017/03/22 13:48:14, grt (UTC plus 1) wrote: > > is it easier for consumers of this api for this to be a bitfield rather than > an > > array? (not sure if this can be accomplished in mojom or not, so pardon if > this > > is a silly question.) > > It's not silly at all. > > In the compiled code, a Mojo enum are represented by an enum class, so > converting to and from the enum will require casts. However, I think it's easy > enough to wrap the conversions in auxiliary functions, so I changed this to int > instead. > > Another alternative is to define ObservedBehaviour's enumerators as uint64 > constants, with the expense of not having the enum's scoping protection. It may > not be a big deal though, since we are already in a separate namespace (mojom). > > WDYT? I'd prefer to keep the mojom interface as straightforward as possible, which would be "array<ObservedBehaviour>". (Actually it should be set<ObservedBehaviour>" but mojom doesn't have a set type.) I think that will be easier for the caller too - we're going to be using these values by iterating through all "observed behaviours" and formatting them for display. It's more straightforward to iterate through an array than a bitfield. The only issue that a bitfield avoids is dealing with duplicates (and it's implicitly sorted, but we'll want to sort the output explicitly anyway rather than relying on that.)
On 2017/03/22 at 14:45:04, joenotcharles wrote: > https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... > File chrome_cleaner/mojom/chrome_prompt.mojom (right): > > https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... > chrome_cleaner/mojom/chrome_prompt.mojom:25: array<ObservedBehaviour> observed_behaviours; > On 2017/03/22 14:25:36, ftirelo wrote: > > On 2017/03/22 13:48:14, grt (UTC plus 1) wrote: > > > is it easier for consumers of this api for this to be a bitfield rather than > > an > > > array? (not sure if this can be accomplished in mojom or not, so pardon if > > this > > > is a silly question.) > > > > It's not silly at all. > > > > In the compiled code, a Mojo enum are represented by an enum class, so > > converting to and from the enum will require casts. However, I think it's easy > > enough to wrap the conversions in auxiliary functions, so I changed this to int > > instead. > > > > Another alternative is to define ObservedBehaviour's enumerators as uint64 > > constants, with the expense of not having the enum's scoping protection. It may > > not be a big deal though, since we are already in a separate namespace (mojom). > > > > WDYT? > > I'd prefer to keep the mojom interface as straightforward as possible, which would be "array<ObservedBehaviour>". (Actually it should be set<ObservedBehaviour>" but mojom doesn't have a set type.) I think that will be easier for the caller too - we're going to be using these values by iterating through all "observed behaviours" and formatting them for display. It's more straightforward to iterate through an array than a bitfield. The only issue that a bitfield avoids is dealing with duplicates (and it's implicitly sorted, but we'll want to sort the output explicitly anyway rather than relying on that.) That model makes sense too. Thinking ahead a bit, do you think it's possible that each/any of these behaviors will at some point need to carry additional metadata? If so I would suggest using a union for ObservedBehavior, i.e. something like: struct UnspecifiedBehaviorDetails {}; struct AdInjectorDetails {}; struct FooDetails {}; union ObservedBehavior { UnspecifiedBehaviorDetails unspecified; AdInjectorDetails ad_injector; FooDetails foo; }; and then using an array<ObservedBehavior> as you suggest.
lgtm from me. i'll bow out of the array vs bitfield discussion since i don't really know the implications in the mojo bindings of the two options. if that were out of the picture, i'd be inclined to go with the bitfield since lookups are O(1) and i don't think iterating is necessarily harder than with a vector/list/etc. certainly less overhead as a bitfield.
I ended up going with Ken's suggestion of a struct of bools, since it's easier to extend to new types and more similar to a bitfield. The idea of a union of *Details structs seems a good general solution, but it's not clear if we will ever need to do that, so I'd rather defer to when we actually need it. https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... File chrome_cleaner/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/120001/chrome_cleaner/mojom/c... chrome_cleaner/mojom/chrome_prompt.mojom:25: array<ObservedBehaviour> observed_behaviours; On 2017/03/22 14:45:03, joenotcharles wrote: > On 2017/03/22 14:25:36, ftirelo wrote: > > On 2017/03/22 13:48:14, grt (UTC plus 1) wrote: > > > is it easier for consumers of this api for this to be a bitfield rather than > > an > > > array? (not sure if this can be accomplished in mojom or not, so pardon if > > this > > > is a silly question.) > > > > It's not silly at all. > > > > In the compiled code, a Mojo enum are represented by an enum class, so > > converting to and from the enum will require casts. However, I think it's easy > > enough to wrap the conversions in auxiliary functions, so I changed this to > int > > instead. > > > > Another alternative is to define ObservedBehaviour's enumerators as uint64 > > constants, with the expense of not having the enum's scoping protection. It > may > > not be a big deal though, since we are already in a separate namespace > (mojom). > > > > WDYT? > > I'd prefer to keep the mojom interface as straightforward as possible, which > would be "array<ObservedBehaviour>". (Actually it should be > set<ObservedBehaviour>" but mojom doesn't have a set type.) I think that will be > easier for the caller too - we're going to be using these values by iterating > through all "observed behaviours" and formatting them for display. It's more > straightforward to iterate through an array than a bitfield. The only issue that > a bitfield avoids is dealing with duplicates (and it's implicitly sorted, but > we'll want to sort the output explicitly anyway rather than relying on that.) I'm taking up on Ken's suggestion of using a struct of bools since it's simple and easy to extend. https://codereview.chromium.org/2750463006/diff/70008/chrome_cleaner/mojom/ch... File chrome_cleaner/mojom/chrome_prompt.mojom (right): https://codereview.chromium.org/2750463006/diff/70008/chrome_cleaner/mojom/ch... chrome_cleaner/mojom/chrome_prompt.mojom:8: enum ObservedBehaviour { On 2017/03/22 14:41:44, Ken Rockot wrote: > Yeah, please don't use enums for bitfields. There's a reason enum classes don't > support this and a reason we use enum classes in the generated code. > > A uint64 field with separate flag value constants is a reasonable option. > > Another totally reasonable option IMHO is to use an explicit struct of bools: > > struct ObservedBehaviorFlags { > bool ads_injector; > bool settings_hijacker; > bool extension_injector; > bool dropper; > }; > > And in fact you could just inline the bools in the UwS struct, eliminating the > (albeit tiny) extra overhead of a secondary struct. > > bools in a struct are tightly packed into bitfields and therefore space > efficient. It's also probably easier to read, and it's naturally extensible. I like this idea! Thanks!
lgtm
lgtm
still lgtm
lgtm https://codereview.chromium.org/2750463006/diff/150001/chrome_cleaner/mojom/B... File chrome_cleaner/mojom/BUILD.gn (right): https://codereview.chromium.org/2750463006/diff/150001/chrome_cleaner/mojom/B... chrome_cleaner/mojom/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All Rights Reserved. As one high-level comment, we have tried to standardize mojom interface libraries on a conventional file hierarchy like: chrome_cleaner/public/interfaces/ chrome_cleaner/public/cpp/ where interfaces contains the mojom, and cpp contains any C++ client library helpers you might have for using it (of which you obviously have none yet and may never have any :)) and we typically call the mojom target "interfaces". This means users can depend on "//chrome_cleaner/public/interfaces", and it's really obvious by convention that dependencies on "//foo/public/interfaces" are mojom dependencies effectively devoid of any transitive deps beyond //base and //mojo/public. Of course, this is a separate repo, and we unfortunately violate this convention in a few stale places around chromium anyway (e.g. //url/mojo and //ui/gfx/mojo), so I don't feel too strongly about it. But something to consider!
Thanks for the reviews! On 2017/03/23 17:41:26, Ken Rockot wrote: > lgtm > > https://codereview.chromium.org/2750463006/diff/150001/chrome_cleaner/mojom/B... > File chrome_cleaner/mojom/BUILD.gn (right): > > https://codereview.chromium.org/2750463006/diff/150001/chrome_cleaner/mojom/B... > chrome_cleaner/mojom/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All > Rights Reserved. > As one high-level comment, we have tried to standardize mojom interface > libraries on a conventional file hierarchy like: > > chrome_cleaner/public/interfaces/ > chrome_cleaner/public/cpp/ > > where interfaces contains the mojom, and cpp contains any C++ client library > helpers you might have for using it (of which you obviously have none yet and > may never have any :)) > > and we typically call the mojom target "interfaces". This means users can depend > on "//chrome_cleaner/public/interfaces", and it's really obvious by convention > that dependencies on "//foo/public/interfaces" are mojom dependencies > effectively devoid of any transitive deps beyond //base and //mojo/public. > > Of course, this is a separate repo, and we unfortunately violate this convention > in a few stale places around chromium anyway (e.g. //url/mojo and > //ui/gfx/mojo), so I don't feel too strongly about it. But something to > consider! Done.
Description was changed from ========== ChromePrompt Mojo IPC interface. The interface will be pulled via DEPS in both Chromium (server side) and Software Reporter (client side) repos. Interface implementation will be landed in the Chromium repo. BUG=690020 ========== to ========== ChromePrompt Mojo IPC interface. The interface will be pulled via DEPS in both Chromium (server side) and Software Reporter (client side) repos. Interface implementation will be landed in the Chromium repo. BUG=690020 R=csharp@chromium.org, grt@chromium.org, joenotcharles@google.com, rockot@chromium.org Committed: https://chromium.googlesource.com/chromium/pipa/+/ef74310fb4d63c737b8a888c2ea... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:170001) manually as ef74310fb4d63c737b8a888c2ea72b4e458218ef (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
