Disable screenshot uploading if there has been user input.

Disable screenshot uploading if there has been user input.

Updated StatusUploader to expose an API to determine if session data uploading
is allowed, and updated screenshot remote command to call this function.

BUG=487261
Committed: https://crrev.com/43bfb070ff0c80bba819aca72e53800d6f597b31
Cr-Commit-Position: refs/heads/master@{#330071}

[Andrew T Wilson (Slow), 2015-05-13 15:01:28]

atwilson@chromium.org changed reviewers:
+ cschuet@chromium.org

[Andrew T Wilson (Slow), 2015-05-13 15:01:28]

PTAL

[cschuet (SLOW), 2015-05-13 15:56:09]

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.h
(right):

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.h:82:
StatusUploader* uploader);
This is quite a heavy dependency just to call a bool(void) function.  Have you
considered passing in a simple Callback<void(bool* allowed)>
is_allowed_callback. This could even be a member function weakly bound to an
instance if you have to worry about lifetime

bool allowed = false;
is_allowed_callback.Run(&allowed);

OR

You could put that method into the ScreenshotDelegate. This would probably mean
that the ScreenshotDelegate aquires a dependency on DeviceStatusCollector. Hmm.
Also I can see the argument that you may want this IsSessionDataUploadAllowed()
to be more generically available although currently the method is only called
for screenshots.

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_commands_factory_chromeos.cc
(right):

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_commands_factory_chromeos.cc:41:
->browser_policy_connector_chromeos();
DCHECK(connector)?

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_commands_factory_chromeos.cc:45:
}
Thanks for cleaning that up. That was a little too nested to be easily readable.

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/status_uploader.h (right):

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader.h:55: StatusUploader();
I see the necessity to introduce this empty constructor in order to write
MockStatusUploader  without having to introduce a StatusUploader interface (I
understand that you don't want to have to pass in all those parameters into the
real constructor). But doesn't this also suggest that either StatusUploader is
not the right place for IsSessionDataUploadAllowed() or at least the screenshot
command shouldn't have a dependency on StatusUploader?

[Andrew T Wilson (Slow), 2015-05-14 09:20:28]

OK, agreed that it probably makes sense to expose a new API to
ScreenshotDelegate - IsScreenshotAllowed()

There are basically two places that can have a dependency on StatusUploader (and
BrowserProcess, and DeviceCloudPolicyManagerChromeOS, which are used to get
access to StatusUploader).

One approach is to have the DeviceCommandsFactory have this dependency - it
could inject a callback into the ScreenshotDelegate it creates.

The other approach is to just have all the logic in
ScreenshotDelegate::IsScreenshotAllowed(), and our MockScreenshotDelegate can
override this for tests (and no need to mock out StatusUploader).

I'm thinking of going with the second approach - it seems cleaner
(DeviceCommandsFactory shouldn't really care about the user input logic). WDYT?

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.cc
(right):

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.cc:202:
if (root_windows.size() == 0) {
BTW, should this check for attached screens be done before we create the
upload_job_ above?

[cschuet (SLOW), 2015-05-14 09:46:01]

> I'm thinking of going with the second approach - it seems cleaner
> (DeviceCommandsFactory shouldn't really care about the user input logic). 
> WDYT?

I agree - this seems cleaner.

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.cc
(right):

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.cc:202:
if (root_windows.size() == 0) {
On 2015/05/14 09:20:28, Andrew T Wilson wrote:
> BTW, should this check for attached screens be done before we create the
> upload_job_ above?

Yes, that makes total sense. Would you move it up?

[Andrew T Wilson (Slow), 2015-05-15 09:43:51]

PTAL

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.cc
(right):

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.cc:202:
if (root_windows.size() == 0) {
On 2015/05/14 09:46:01, cschuet wrote:
> On 2015/05/14 09:20:28, Andrew T Wilson wrote:
> > BTW, should this check for attached screens be done before we create the
> > upload_job_ above?
> 
> Yes, that makes total sense. Would you move it up?

Done.

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.h
(right):

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.h:82:
StatusUploader* uploader);
On 2015/05/13 15:56:09, cschuet wrote:
> This is quite a heavy dependency just to call a bool(void) function.  Have you
> considered passing in a simple Callback<void(bool* allowed)>
> is_allowed_callback. This could even be a member function weakly bound to an
> instance if you have to worry about lifetime
> 
> bool allowed = false;
> is_allowed_callback.Run(&allowed);
> 
> OR
> 
> You could put that method into the ScreenshotDelegate. This would probably
mean
> that the ScreenshotDelegate aquires a dependency on DeviceStatusCollector.
Hmm.
> Also I can see the argument that you may want this
IsSessionDataUploadAllowed()
> to be more generically available although currently the method is only called
> for screenshots.

Moved logic into ScreenshotDelegate - good call.

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/remote_commands/device_commands_factory_chromeos.cc
(right):

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_commands_factory_chromeos.cc:41:
->browser_policy_connector_chromeos();
On 2015/05/13 15:56:09, cschuet wrote:
> DCHECK(connector)?

No need, we'll just crash if it's null, which is even better than a DCHECK.
Plus, it can't really be null.

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/remote_commands/device_commands_factory_chromeos.cc:45:
}
On 2015/05/13 15:56:09, cschuet wrote:
> Thanks for cleaning that up. That was a little too nested to be easily
readable.

Hah, and I reverted back to the original since now I don't need to touch this
file :)

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/status_uploader.h (right):

https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/status_uploader.h:55: StatusUploader();
On 2015/05/13 15:56:09, cschuet wrote:
> I see the necessity to introduce this empty constructor in order to write
> MockStatusUploader  without having to introduce a StatusUploader interface (I
> understand that you don't want to have to pass in all those parameters into
the
> real constructor). But doesn't this also suggest that either StatusUploader is
> not the right place for IsSessionDataUploadAllowed() or at least the
screenshot
> command shouldn't have a dependency on StatusUploader?

Removed. Thanks for the feedback - much cleaner now.

[cschuet (SLOW), 2015-05-15 09:55:51]

On 2015/05/15 09:43:51, Andrew T Wilson wrote:
> PTAL
> 
>
https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
> File
>
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.cc
> (right):
> 
>
https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
>
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.cc:202:
> if (root_windows.size() == 0) {
> On 2015/05/14 09:46:01, cschuet wrote:
> > On 2015/05/14 09:20:28, Andrew T Wilson wrote:
> > > BTW, should this check for attached screens be done before we create the
> > > upload_job_ above?
> > 
> > Yes, that makes total sense. Would you move it up?
> 
> Done.
> 
>
https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
> File
> chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.h
> (right):
> 
>
https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
>
chrome/browser/chromeos/policy/remote_commands/device_command_screenshot_job.h:82:
> StatusUploader* uploader);
> On 2015/05/13 15:56:09, cschuet wrote:
> > This is quite a heavy dependency just to call a bool(void) function.  Have
you
> > considered passing in a simple Callback<void(bool* allowed)>
> > is_allowed_callback. This could even be a member function weakly bound to an
> > instance if you have to worry about lifetime
> > 
> > bool allowed = false;
> > is_allowed_callback.Run(&allowed);
> > 
> > OR
> > 
> > You could put that method into the ScreenshotDelegate. This would probably
> mean
> > that the ScreenshotDelegate aquires a dependency on DeviceStatusCollector.
> Hmm.
> > Also I can see the argument that you may want this
> IsSessionDataUploadAllowed()
> > to be more generically available although currently the method is only
called
> > for screenshots.
> 
> Moved logic into ScreenshotDelegate - good call.
> 
>
https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
> File
>
chrome/browser/chromeos/policy/remote_commands/device_commands_factory_chromeos.cc
> (right):
> 
>
https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
>
chrome/browser/chromeos/policy/remote_commands/device_commands_factory_chromeos.cc:41:
> ->browser_policy_connector_chromeos();
> On 2015/05/13 15:56:09, cschuet wrote:
> > DCHECK(connector)?
> 
> No need, we'll just crash if it's null, which is even better than a DCHECK.
> Plus, it can't really be null.
> 
>
https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
>
chrome/browser/chromeos/policy/remote_commands/device_commands_factory_chromeos.cc:45:
> }
> On 2015/05/13 15:56:09, cschuet wrote:
> > Thanks for cleaning that up. That was a little too nested to be easily
> readable.
> 
> Hah, and I reverted back to the original since now I don't need to touch this
> file :)
> 
>
https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
> File chrome/browser/chromeos/policy/status_uploader.h (right):
> 
>
https://codereview.chromium.org/1135413002/diff/20001/chrome/browser/chromeos...
> chrome/browser/chromeos/policy/status_uploader.h:55: StatusUploader();
> On 2015/05/13 15:56:09, cschuet wrote:
> > I see the necessity to introduce this empty constructor in order to write
> > MockStatusUploader  without having to introduce a StatusUploader interface
(I
> > understand that you don't want to have to pass in all those parameters into
> the
> > real constructor). But doesn't this also suggest that either StatusUploader
is
> > not the right place for IsSessionDataUploadAllowed() or at least the
> screenshot
> > command shouldn't have a dependency on StatusUploader?
> 
> Removed. Thanks for the feedback - much cleaner now.

LGTM

[Andrew T Wilson (Slow), 2015-05-15 10:30:11]

The CQ bit was checked by atwilson@chromium.org

[commit-bot: I haz the power, 2015-05-15 10:30:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1135413002/60001

[commit-bot: I haz the power, 2015-05-15 10:33:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-15 10:34:42]

Patchset 4 (id:??) landed as
https://crrev.com/43bfb070ff0c80bba819aca72e53800d6f597b31
Cr-Commit-Position: refs/heads/master@{#330071}