scanner: initial setup [chromium/src : main]
Curtis McMullan (Gerrit)
Curtis McMullan added 1 comment![]( "Open in Gerrit")
Related details
- Darren Shen
- Michael Cui
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darren Shen (Gerrit)
Darren Shen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darren Shen
| Code-Review | +1 |
1 comment
nit: I'd also put TODOs or NOT_IMPLEMENTED in all the unimplemented methods
Related details
- Curtis McMullan
- Michael Cui
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Cui (Gerrit)
Michael Cui added 3 comments![]( "Open in Gerrit")
BUG=b:363100868optional: Bug links should appear in the commit message footer (i.e. the last paragraph with Change-Id) and be formatted: like `Bug: b:363100868`. https://chromium.googlesource.com/chromium/src/+/HEAD/docs/contributing.md#cl-footer-reference
class ASH_PUBLIC_EXPORT ScannerClient {optional naming: This is name may be a bit confusing - in another codebase I've worked in, a `Client` class is a long-lived browser-side delegate for a long-lived Ash-side `Controller` class.
This `Client` class, however, is a _short-lived_ (lifetime tied to a session) browser-side delegate for a short-lived Ash-side `Session` class. The long-lived browser-side delegate is named `ClientFactory` instead.
Should we rename `ScannerClientFactory + ScannerClient` to something else, like `ScannerClient + ScannerSessionClient`? Or we can be more explicit and use `ScannerBrowserDelegate + ScannerSessionBrowserDelegate`, but that's quite verbose. WDYT?
ScannerSession();optional: Should we take in a `std::unique_ptr` to the browser-side equivalent (`ScannerClient`) here? We can do this in a future CL if necessary.
Related details
- Curtis McMullan
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Cui (Gerrit)
Michael Cui voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Curtis McMullan
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan added 3 comments![]( "Open in Gerrit")
BUG=b:363100868optional: Bug links should appear in the commit message footer (i.e. the last paragraph with Change-Id) and be formatted: like `Bug: b:363100868`. https://chromium.googlesource.com/chromium/src/+/HEAD/docs/contributing.md#cl-footer-reference
Noted will do this next time!
class ASH_PUBLIC_EXPORT ScannerClient {optional naming: This is name may be a bit confusing - in another codebase I've worked in, a `Client` class is a long-lived browser-side delegate for a long-lived Ash-side `Controller` class.
This `Client` class, however, is a _short-lived_ (lifetime tied to a session) browser-side delegate for a short-lived Ash-side `Session` class. The long-lived browser-side delegate is named `ClientFactory` instead.
Should we rename `ScannerClientFactory + ScannerClient` to something else, like `ScannerClient + ScannerSessionClient`? Or we can be more explicit and use `ScannerBrowserDelegate + ScannerSessionBrowserDelegate`, but that's quite verbose. WDYT?
Is that the convention for the term `Client` in this context, specifically for long lived vs short lived instances? Happy to change to whatever better suits, but my understanding was a `Client` is the convention used in `//ash` for a caller into `//chrome/browser/ash` and there was no specification of long lived vs short lived?
`ScannerBrowserDelegate` feels like the best alternative, but `ScannerSessionBrowserDelegate` is a mouthful and would probably have a corresponding `ScannerSessionBrowserDelegateImpl` in `//chrome/browser/ash/scanner` which is too much!
ScannerSession();optional: Should we take in a `std::unique_ptr` to the browser-side equivalent (`ScannerClient`) here? We can do this in a future CL if necessary.
- Michael Cui
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan added 2 comments![]( "Open in Gerrit")
nit: I'd also put TODOs or NOT_IMPLEMENTED in all the unimplemented methods
Done
Curtis McMullanoptional naming: This is name may be a bit confusing - in another codebase I've worked in, a `Client` class is a long-lived browser-side delegate for a long-lived Ash-side `Controller` class.
This `Client` class, however, is a _short-lived_ (lifetime tied to a session) browser-side delegate for a short-lived Ash-side `Session` class. The long-lived browser-side delegate is named `ClientFactory` instead.
Should we rename `ScannerClientFactory + ScannerClient` to something else, like `ScannerClient + ScannerSessionClient`? Or we can be more explicit and use `ScannerBrowserDelegate + ScannerSessionBrowserDelegate`, but that's quite verbose. WDYT?
Is that the convention for the term `Client` in this context, specifically for long lived vs short lived instances? Happy to change to whatever better suits, but my understanding was a `Client` is the convention used in `//ash` for a caller into `//chrome/browser/ash` and there was no specification of long lived vs short lived?
`ScannerBrowserDelegate` feels like the best alternative, but `ScannerSessionBrowserDelegate` is a mouthful and would probably have a corresponding `ScannerSessionBrowserDelegateImpl` in `//chrome/browser/ash/scanner` which is too much!
I'll resolve this for the moment because the conversation is mainly related to naming, and not blocking the CL. Let's chat offline about some better names for these classes.
Related details
- Michael Cui
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan added 1 comment![]( "Open in Gerrit")
- Ahmed Fakhry
- Michael Cui
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Cui (Gerrit)
Michael Cui voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michael Cui
| Code-Review | +1 |
2 comments
(Sorry - forgot to send out my comments!)
class ASH_PUBLIC_EXPORT ScannerClient {Curtis McMullanoptional naming: This is name may be a bit confusing - in another codebase I've worked in, a `Client` class is a long-lived browser-side delegate for a long-lived Ash-side `Controller` class.
This `Client` class, however, is a _short-lived_ (lifetime tied to a session) browser-side delegate for a short-lived Ash-side `Session` class. The long-lived browser-side delegate is named `ClientFactory` instead.
Should we rename `ScannerClientFactory + ScannerClient` to something else, like `ScannerClient + ScannerSessionClient`? Or we can be more explicit and use `ScannerBrowserDelegate + ScannerSessionBrowserDelegate`, but that's quite verbose. WDYT?
Is that the convention for the term `Client` in this context, specifically for long lived vs short lived instances? Happy to change to whatever better suits, but my understanding was a `Client` is the convention used in `//ash` for a caller into `//chrome/browser/ash` and there was no specification of long lived vs short lived?
`ScannerBrowserDelegate` feels like the best alternative, but `ScannerSessionBrowserDelegate` is a mouthful and would probably have a corresponding `ScannerSessionBrowserDelegateImpl` in `//chrome/browser/ash/scanner` which is too much!
Ack
Related details
- Ahmed Fakhry
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ahmed Fakhry (Gerrit)
Ahmed Fakhry added 16 comments![]( "Open in Gerrit")
// Used to enable scanner update.What does this mean? Can you please make it a little less vague?
sh...@chromium.orgNormally we don't add owners until the feature work is almost stable.
virtual void Execute() = 0;Can you please add some docs to the class and the methods? Ditto every where.
virtual ScannerSystemState GetSystemState() = 0;const
class ASH_PUBLIC_EXPORT ScannerClientFactory {Why do we need this factory? Can you please let `ash::Shell` construct the client directly via its `ShellDelegate` API? Here's an example CL that does a similar thing to what you are doing: https://chromium-review.googlesource.com/c/chromium/src/+/5846149
kMinValue,
kInvalidConsent,
kMaxValue,This makes it that we have **3 unique values**. I think what you want is the following:
```
enum class ASH_PUBLIC_EXPORT ScannerSystemCheck {
kMinValue = 0,
kInvalidConsent = kMinValue,
kMaxValue = kInvalidConsent,
};
```
kBlocked,What does "blocked" mean?
Please make sure to add adequate comments/docs to your CL. 😊 go/cros-cl-checklist
enum ASH_PUBLIC_EXPORT ScannerStatus {enum class. Ditto below.
std::unique_ptr<ScannerSession> StartNewSession();Please help me understand what session we're talking about here?
Sunfish will start capture-mode in a special session that will also include "scanner" capabilities. How is this `ScannerSession` used in relation?
ScannerController();If you follow my earlier suggestion, you can pass the client directly to the ctor, since Shell creates both. Please see this example: https://source.chromium.org/chromium/chromium/src/+/main:ash/shell.cc;l=1331-1332;drc=8e948282d37c0e119e3102236878d6f4d5052c16
ScannerController();Please delete copy ctor and copy assignment.
return system_state_provider_.GetSystemState();For my understanding: Why does this need to be a class object?
- Do you anticipate `ScannerSystemStateProvider` to be a complex class that need to implement other interfaces, and depend on other objects?
- Can the logic be inlined here?
- Will `ScannerSystemStateProvider` be complex enough that it merits being in its own .h/.cc file?
- Will its header need to be included else where, and therefore needs to be exposed?
- Can it live in an anonymous namespace inside this file?
Related details
- Curtis McMullan
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan added 17 comments![]( "Open in Gerrit")
What does this mean? Can you please make it a little less vague?
Updated this with a link providing detail.
Normally we don't add owners until the feature work is almost stable.
I'd like to land this so that others can begin to contribute to the code and not require CL reviews from top level OWNERS. We have done this for other features as well. Let me know if you feel strongly about this.
virtual void Execute() = 0;Can you please add some docs to the class and the methods? Ditto every where.
I've added some comments to this class, but need to add some more throughout the CL so will this open for the moment to remind myself.
virtual ScannerSystemState GetSystemState() = 0;Curtis McMullanconst
Done
Why do we need this factory? Can you please let `ash::Shell` construct the client directly via its `ShellDelegate` API? Here's an example CL that does a similar thing to what you are doing: https://chromium-review.googlesource.com/c/chromium/src/+/5846149
Apologies this name is a little misleading. I've used the term here `ScannerClient` to reference a delegate to the browser that is scoped to a ScannerSession. We will need this in the future as each delegate into the browser should be scoped to a particular profile via a ProfileKeyedService (see this similar setup [1] used for the lobster feature). I'll rename these classes to better fit the convention used here for `Client` classes.
This makes it that we have **3 unique values**. I think what you want is the following:
```
enum class ASH_PUBLIC_EXPORT ScannerSystemCheck {
kMinValue = 0,
kInvalidConsent = kMinValue,
kMaxValue = kInvalidConsent,
};
```
Good catch you are very right, thanks.
What does "blocked" mean?
Please make sure to add adequate comments/docs to your CL. 😊 go/cros-cl-checklist
Added a comment for this.
enum ASH_PUBLIC_EXPORT ScannerStatus {Curtis McMullanenum class. Ditto below.
Done
sh...@chromium.orgCurtis McMullanDitto
Similar to above comment, I'd love to get this landed so others can contribute without reviews going to top level OWNERS.
Please help me understand what session we're talking about here?
Sunfish will start capture-mode in a special session that will also include "scanner" capabilities. How is this `ScannerSession` used in relation?
This is a session for the Scanner feature, the lifetime of `ScannerSession` is linked to the lifetime of a "SunfishSession" however they don't have the exact same lifetimes. For example a "SunfishSession" may end but a `ScannerSession` may continue to exist to complete any processing it needs to do.
So the relation is, a `ScannerSession` will be created alongside a "SunfishSession", but may outlive the "SunfishSession". Hope that helps!
If you follow my earlier suggestion, you can pass the client directly to the ctor, since Shell creates both. Please see this example: https://source.chromium.org/chromium/chromium/src/+/main:ash/shell.cc;l=1331-1332;drc=8e948282d37c0e119e3102236878d6f4d5052c16
Followed the earlier suggestion, and now pass the client directly in the ctor.
Please delete copy ctor and copy assignment.
Done
ScannerSession();Curtis McMullanDitto copy-ctor/copy-assign
Done
sh...@chromium.orgCurtis McMullanDitto
Similar to above comments re OWNERS
For my understanding: Why does this need to be a class object?
- Do you anticipate `ScannerSystemStateProvider` to be a complex class that need to implement other interfaces, and depend on other objects?
- Can the logic be inlined here?
- Will `ScannerSystemStateProvider` be complex enough that it merits being in its own .h/.cc file?
- Will its header need to be included else where, and therefore needs to be exposed?
- Can it live in an anonymous namespace inside this file?
Yes this is anticipated to hold a fair amount of logic in future CLs. It will be responsible for maintaining all required checks to enable / disable the feature, and will depend on other objects (observing their state etc). The current logic could be inlined absolutely, but future logic not so much. For an example of some of the logic required, see `EditorSwitch` [1]. We could add all checks required in this class file too, however it will definitely bloat it.
Related details
- Ahmed Fakhry
- Darren Shen
- Michael Cui
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan added 1 comment![]( "Open in Gerrit")
This will be a `ProfileKeyedService`.
I'll change this to a `ProfileKeyedService` in this CL when I get a chance. With that change the structure of the CL (ie. the need for a `ScannerDelegate` and a `ScannerProfileScopedDelegate`) will make more sense.
Ahmed Fakhry (Gerrit)
Ahmed Fakhry added 14 comments![]( "Open in Gerrit")
Curtis McMullanNormally we don't add owners until the feature work is almost stable.
I'd like to land this so that others can begin to contribute to the code and not require CL reviews from top level OWNERS. We have done this for other features as well. Let me know if you feel strongly about this.
I understand the reason behind this .. the desire to make faster progress, but having the top level owners as reviewers at the initial stages of a project is very important, since they can help you catch a lot of issues with your architecture early on.
I'm ok with keeping ash/scanner/OWNERS, but please remove all others until a later stage of the project.
// Complete the operation contained within this action.Completes
Function comments should use the indicative mood ("Opens the file") rather than the imperative ("Open the file").
virtual void Execute() = 0;Curtis McMullanCan you please add some docs to the class and the methods? Ditto every where.
I've added some comments to this class, but need to add some more throughout the CL so will this open for the moment to remind myself.
Acknowledged
std::unique_ptr<ScannerSession> StartNewSession();Curtis McMullanPlease help me understand what session we're talking about here?
Sunfish will start capture-mode in a special session that will also include "scanner" capabilities. How is this `ScannerSession` used in relation?
This is a session for the Scanner feature, the lifetime of `ScannerSession` is linked to the lifetime of a "SunfishSession" however they don't have the exact same lifetimes. For example a "SunfishSession" may end but a `ScannerSession` may continue to exist to complete any processing it needs to do.
So the relation is, a `ScannerSession` will be created alongside a "SunfishSession", but may outlive the "SunfishSession". Hope that helps!
Can we please document this somewhere?
std::unique_ptr<ScannerProfileScopedDelegate> profile_scoped_delegate =This will be immediately destroyed when this function returns. Was that meant to be passed to `ScannerSession`?
return profile_scoped_delegate != nullptr &&Nit: Just `profile_scoped_delegate && ...`
Curtis McMullanDitto
Similar to above comments re OWNERS
As mentioned earlier. Let's remove this one.
private:
// Not owned by this class
raw_ptr<ash::ScannerController> controller_;Not needed. Please remove for now.
// ash::ScannerDelegate overridesNit: Just
```
// ash::ScannerDelegate:
```
ChromeScannerDelegate();Nit: Delete copy-ctor/copy-assign.
Curtis McMullanThis will be a `ProfileKeyedService`.
I'll change this to a `ProfileKeyedService` in this CL when I get a chance. With that change the structure of the CL (ie. the need for a `ScannerDelegate` and a `ScannerProfileScopedDelegate`) will make more sense.
Acknowledged
// ash::ScannerProfileScopedDelegate overridesDitto:
```
/ ash::ScannerProfileScopedDelegate:
```
ScannerKeyedService();Ditto
std::unique_ptr<ash::ScannerDelegate> CreateScannerDelegate() const override;I think you need to implement this in `TestShellDelegate`. Otherwise ash_unittests will fail to compile.
Related details
- Curtis McMullan
- Darren Shen
- Michael Cui
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ahmed Fakhry (Gerrit)
Ahmed Fakhry added 1 comment![]( "Open in Gerrit")
std::unique_ptr<ScannerSession> StartNewSession();Curtis McMullanPlease help me understand what session we're talking about here?
Sunfish will start capture-mode in a special session that will also include "scanner" capabilities. How is this `ScannerSession` used in relation?
Ahmed FakhryThis is a session for the Scanner feature, the lifetime of `ScannerSession` is linked to the lifetime of a "SunfishSession" however they don't have the exact same lifetimes. For example a "SunfishSession" may end but a `ScannerSession` may continue to exist to complete any processing it needs to do.
So the relation is, a `ScannerSession` will be created alongside a "SunfishSession", but may outlive the "SunfishSession". Hope that helps!
Can we please document this somewhere?
- Curtis McMullan
- Darren Shen
- Elijah Hewer
- Michael Cui
- Sophie Wen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sophie Wen (Gerrit)
Sophie Wen added 1 comment![]( "Open in Gerrit")
std::unique_ptr<ScannerSession> StartNewSession();Curtis McMullanPlease help me understand what session we're talking about here?
Sunfish will start capture-mode in a special session that will also include "scanner" capabilities. How is this `ScannerSession` used in relation?
Ahmed FakhryThis is a session for the Scanner feature, the lifetime of `ScannerSession` is linked to the lifetime of a "SunfishSession" however they don't have the exact same lifetimes. For example a "SunfishSession" may end but a `ScannerSession` may continue to exist to complete any processing it needs to do.
So the relation is, a `ScannerSession` will be created alongside a "SunfishSession", but may outlive the "SunfishSession". Hope that helps!
Ahmed FakhryCan we please document this somewhere?
I'd also like to get more context of when the `ScannerSession` will need to outlive a `SunfishSession`? Since there will be no real-time user interaction after `SunfishSession` ends, could any needed post-processing be done via a deferred task runner? Please correct me if I'm misunderstanding.
Related details
- Ahmed Fakhry
- Curtis McMullan
- Darren Shen
- Elijah Hewer
- Michael Cui
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan added 12 comments![]( "Open in Gerrit")
Curtis McMullanNormally we don't add owners until the feature work is almost stable.
Ahmed FakhryI'd like to land this so that others can begin to contribute to the code and not require CL reviews from top level OWNERS. We have done this for other features as well. Let me know if you feel strongly about this.
I understand the reason behind this .. the desire to make faster progress, but having the top level owners as reviewers at the initial stages of a project is very important, since they can help you catch a lot of issues with your architecture early on.
I'm ok with keeping ash/scanner/OWNERS, but please remove all others until a later stage of the project.
Okay fair enough, I'll leave this OWNERS file and remove the others.
Completes
Function comments should use the indicative mood ("Opens the file") rather than the imperative ("Open the file").
Done
std::unique_ptr<ScannerSession> StartNewSession();Curtis McMullanPlease help me understand what session we're talking about here?
Sunfish will start capture-mode in a special session that will also include "scanner" capabilities. How is this `ScannerSession` used in relation?
Ahmed FakhryThis is a session for the Scanner feature, the lifetime of `ScannerSession` is linked to the lifetime of a "SunfishSession" however they don't have the exact same lifetimes. For example a "SunfishSession" may end but a `ScannerSession` may continue to exist to complete any processing it needs to do.
So the relation is, a `ScannerSession` will be created alongside a "SunfishSession", but may outlive the "SunfishSession". Hope that helps!
Ahmed FakhryCan we please document this somewhere?
Sophie Wen
I'd also like to get more context of when the `ScannerSession` will need to outlive a `SunfishSession`? Since there will be no real-time user interaction after `SunfishSession` ends, could any needed post-processing be done via a deferred task runner? Please correct me if I'm misunderstanding.
For sure! I'll add some of these details in the comments above the `ScannerSession` class definition.
@sophiewen, yes you are right there shouldn't be any real time user interaction after a` SunfishSession` ends. However, we may want to forward the user to a URL after the screen capture overlay has closed or complete some other UX interaction post `SunfishSession`. Or maybe we want to record some metrics or complete some other post session analysis after the overlay has closed.
I really would prefer to not strictly bind the lifetime of this `ScannerSession` to the lifetime of a `SunfishSession` to give us this flexibility with the use of `ScannerSession`.
std::unique_ptr<ScannerProfileScopedDelegate> profile_scoped_delegate =This will be immediately destroyed when this function returns. Was that meant to be passed to `ScannerSession`?
Yeah you are very right, in the following CL this is passed to the `ScannerSession` so the session has access to the browser.
Nit: Just `profile_scoped_delegate && ...`
Done
Curtis McMullanDitto
Ahmed FakhrySimilar to above comments re OWNERS
As mentioned earlier. Let's remove this one.
private:
// Not owned by this class
raw_ptr<ash::ScannerController> controller_;Not needed. Please remove for now.
Done
Nit: Just
```
// ash::ScannerDelegate:
```
Done
ChromeScannerDelegate();Curtis McMullanNit: Delete copy-ctor/copy-assign.
Done
// ash::ScannerProfileScopedDelegate overridesCurtis McMullanDitto:
```
/ ash::ScannerProfileScopedDelegate:
```
Done
ScannerKeyedService();Curtis McMullanDitto
Done
std::unique_ptr<ash::ScannerDelegate> CreateScannerDelegate() const override;I think you need to implement this in `TestShellDelegate`. Otherwise ash_unittests will fail to compile.
- Ahmed Fakhry
- Darren Shen
- Elijah Hewer
- Michael Cui
- Sophie Wen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan added 2 comments![]( "Open in Gerrit")
PTAL sky@ for //chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
Thanks!
Curtis McMullanThis will be a `ProfileKeyedService`.
Ahmed FakhryI'll change this to a `ProfileKeyedService` in this CL when I get a chance. With that change the structure of the CL (ie. the need for a `ScannerDelegate` and a `ScannerProfileScopedDelegate`) will make more sense.
Acknowledged
I've made this a true `ProfileKeyedService` and updated the CL accordingly.
Related details
- Ahmed Fakhry
- Darren Shen
- Elijah Hewer
- Michael Cui
- Scott Violet
- Sophie Wen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Cui (Gerrit)
Michael Cui voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michael Cui
| Code-Review | +1 |
2 comments
return std::make_unique<ChromeScannerDelegate>();optional: If we expect `ScannerDelegate` to not have any more functionality other than getting a pointer to the service, consider making `ScannerKeyedServiceFactory` implement `ash::ScannerDelegate` and return a raw pointer here. See https://crrev.com/c/5846970 for an example.
Related details
- Ahmed Fakhry
- Curtis McMullan
- Darren Shen
- Elijah Hewer
- Scott Violet
- Sophie Wen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Violet (Gerrit)
Scott Violet added 1 comment![]( "Open in Gerrit")
chrome_browser_main_extra_parts_profiles LGTM (I did not look at anything else).
Related details
- Ahmed Fakhry
- Curtis McMullan
- Darren Shen
- Elijah Hewer
- Kyle Horimoto
- Sophie Wen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ahmed Fakhry (Gerrit)
Ahmed Fakhry voted and added 7 comments![]( "Open in Gerrit")
Votes added by Ahmed Fakhry
| Code-Review | +1 |
7 comments
LGTM % below
// due to system level constraints (ie pref disabled, feature not allowed).i.e.
nit: unneeded whitespace
+1
// BrowserContextKeyedServiceFactory// BrowserContextKeyedServiceFactory:
class ScannerSystemStateProvider {Question (doesn't have to be in this CL): Can this class and `ScannerKeyedService` be combined together into one (e.g. just `ScannerKeyedService`)?
Related details
- Curtis McMullan
- Darren Shen
- Elijah Hewer
- Kyle Horimoto
- Michael Cui
- Sophie Wen
Curtis McMullan (Gerrit)
Curtis McMullan added 7 comments![]( "Open in Gerrit")
sh...@chromium.orgCurtis McMullanPlease remove this file.
Done
// due to system level constraints (ie pref disabled, feature not allowed).Curtis McMullani.e.
Done
Ahmed Fakhrynit: unneeded whitespace
+1
Done
// BrowserContextKeyedServiceFactoryCurtis McMullan// BrowserContextKeyedServiceFactory:
Done
Question (doesn't have to be in this CL): Can this class and `ScannerKeyedService` be combined together into one (e.g. just `ScannerKeyedService`)?
As mentioned previously we can inline these, however there will be a solid amount of logic added to this class alone so it will be nice to encapsulate it as its own single unit rather than merging it with its holding class.
optional: If we expect `ScannerDelegate` to not have any more functionality other than getting a pointer to the service, consider making `ScannerKeyedServiceFactory` implement `ash::ScannerDelegate` and return a raw pointer here. See https://crrev.com/c/5846970 for an example.
- Ahmed Fakhry
- Darren Shen
- Elijah Hewer
- Kyle Horimoto
- Michael Cui
- Sophie Wen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan added 1 comment![]( "Open in Gerrit")
chrome_browser_main_extra_parts_profiles LGTM (I did not look at anything else).
Thanks for reviewing Scott, could I get a +1 for `chrome_browser_main_extra_parts_profiles.cc`?
Related details
- Ahmed Fakhry
- Darren Shen
- Elijah Hewer
- Kyle Horimoto
- Michael Cui
- Scott Violet
- Sophie Wen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Violet (Gerrit)
Scott Violet voted Code-Review+1![]( "Open in Gerrit")
- Ahmed Fakhry
- Curtis McMullan
- Darren Shen
- Elijah Hewer
- Kyle Horimoto
- Michael Cui
- Sophie Wen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kyle Horimoto (Gerrit)
Kyle Horimoto added 1 comment![]( "Open in Gerrit")
Looks likes all files are already reviewed; removing myself.
Related details
- Ahmed Fakhry
- Curtis McMullan
- Darren Shen
- Elijah Hewer
- Michael Cui
- Sophie Wen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ahmed Fakhry (Gerrit)
Ahmed Fakhry voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ahmed Fakhry
| Code-Review | +1 |
1 comment
Curtis McMullan (Gerrit)
Curtis McMullan voted Commit-Queue+2![]( "Open in Gerrit")
- Darren Shen
- Elijah Hewer
- Michael Cui
- Sophie Wen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Darren Shen
- Elijah Hewer
- Michael Cui
- Sophie Wen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
- Ahmed Fakhry
- Darren Shen
- Elijah Hewer
- Michael Cui
- Scott Violet
- Sophie Wen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan added 1 comment![]( "Open in Gerrit")
Apologies all, I had to update a DEPS file to pass CQ and lost everyones +1. If you could add it again I would appreciate it, thanks!
Related details
- Ahmed Fakhry
- Darren Shen
- Elijah Hewer
- Michael Cui
- Scott Violet
- Sophie Wen
Code-Coverage
Code-Review
No-Unresolved-Comments
Ahmed Fakhry (Gerrit)
Ahmed Fakhry voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ahmed Fakhry
| Code-Review | +1 |
1 comment
LGTM
- Curtis McMullan
- Darren Shen
- Elijah Hewer
- Michael Cui
- Scott Violet
- Sophie Wen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan added 1 comment![]( "Open in Gerrit")
Apologies all, I had to update a DEPS file to pass CQ and lost everyones +1. If you could add it again I would appreciate it, thanks!
- Darren Shen
- Elijah Hewer
- Michael Cui
- Scott Violet
- Sophie Wen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Michael Cui (Gerrit)
Michael Cui voted Code-Review+1![]( "Open in Gerrit")
- Curtis McMullan
- Darren Shen
- Elijah Hewer
- Scott Violet
- Sophie Wen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Violet (Gerrit)
Scott Violet voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Curtis McMullan
- Darren Shen
- Elijah Hewer
- Kyle Horimoto
- Sophie Wen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Curtis McMullan (Gerrit)
Curtis McMullan voted Commit-Queue+2![]( "Open in Gerrit")
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
scanner: initial setup
This CL sets up the baseline plumbing required for scanner.
BUG=b:363100868
- M ash/BUILD.gn
- M ash/constants/ash_switches.cc
- M ash/constants/ash_switches.h
- M ash/public/cpp/BUILD.gn
- A ash/public/cpp/scanner/scanner_action.h
- A ash/public/cpp/scanner/scanner_delegate.h
- A ash/public/cpp/scanner/scanner_enums.h
- A ash/public/cpp/scanner/scanner_profile_scoped_delegate.h
- A ash/public/cpp/scanner/scanner_system_state.cc
- A ash/public/cpp/scanner/scanner_system_state.h
- A ash/scanner/OWNERS
- A ash/scanner/scanner_controller.cc
- A ash/scanner/scanner_controller.h
- A ash/scanner/scanner_session.cc
- A ash/scanner/scanner_session.h
- M ash/shell.cc
- M ash/shell.h
- M ash/shell_delegate.h
- M ash/test_shell_delegate.cc
- M ash/test_shell_delegate.h
- M chrome/browser/BUILD.gn
- A chrome/browser/ash/scanner/BUILD.gn
- A chrome/browser/ash/scanner/DEPS
- A chrome/browser/ash/scanner/chrome_scanner_delegate.cc
- A chrome/browser/ash/scanner/chrome_scanner_delegate.h
- A chrome/browser/ash/scanner/scanner_keyed_service.cc
- A chrome/browser/ash/scanner/scanner_keyed_service.h
- A chrome/browser/ash/scanner/scanner_keyed_service_factory.cc
- A chrome/browser/ash/scanner/scanner_keyed_service_factory.h
- A chrome/browser/ash/scanner/scanner_system_state_provider.cc
- A chrome/browser/ash/scanner/scanner_system_state_provider.h
- M chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
- M chrome/browser/ui/ash/shell_delegate/BUILD.gn
- M chrome/browser/ui/ash/shell_delegate/DEPS
- M chrome/browser/ui/ash/shell_delegate/chrome_shell_delegate.cc
- M chrome/browser/ui/ash/shell_delegate/chrome_shell_delegate.h
Code-Coverage: +1 by findit...@appspot.gserviceaccount.com
Code-Review: +1 by Scott Violet, +1 by Ahmed Fakhry, +1 by Michael Cui
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |