Move non-browser specific ScreenshotTaker code to ui/snapshot.

Componentize ScreenshotTaker.

The filename determination is still done in chrome/browser/ui/ash as it depends on profile settings (download dir and preferred time format) and the save to drive support is exposed as part of the client api for now.

BUG=428745
TEST=ScreenshotGrabberTest.TakeScreenshot
TEST=Screenshot features continue to work as before on Chromebooks with Ctrl+F5 and Ctrl+Shift+F5

Committed: https://crrev.com/149080069ead026c48e07c32430ec98bed17f75c
Cr-Commit-Position: refs/heads/master@{#306600}

[flackr, 2014-11-11 23:31:43]

flackr@chromium.org changed reviewers:
+ oshima@chromium.org

[flackr, 2014-11-12 00:42:05]

Please take a look. This moves the core screenshot taker code to
components/screenshot_taker. I'm not sure if there's more we can do to
share/access prefs to move the filename generation, but at the very least we
should be able to remove the client if/when drive is componentized.

[oshima, 2014-11-12 11:32:13]

oshima@chromium.org changed reviewers:
+ mukai@chromium.org

[oshima, 2014-11-12 11:32:14]

-> mukai

https://codereview.chromium.org/706013004/diff/40001/ash/screenshot_delegate.h
File ash/screenshot_delegate.h (right):

https://codereview.chromium.org/706013004/diff/40001/ash/screenshot_delegate....
ash/screenshot_delegate.h:36: }  // namespace
namespace ash

https://codereview.chromium.org/706013004/diff/40001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_screenshot_taker.cc (right):

https://codereview.chromium.org/706013004/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_screenshot_taker.cc:150: Profile* profile_;
Is it hard to change this to BrowserContext? (It's often the case that callee
just use it as browser context)

https://codereview.chromium.org/706013004/diff/40001/components/screenshot_ta...
File components/screenshot_taker/screenshot_taker.h (right):

https://codereview.chromium.org/706013004/diff/40001/components/screenshot_ta...
components/screenshot_taker/screenshot_taker.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: 2014

https://codereview.chromium.org/706013004/diff/40001/components/screenshot_ta...
components/screenshot_taker/screenshot_taker.h:19: class Profile;
you don't need this right?

https://codereview.chromium.org/706013004/diff/40001/components/screenshot_ta...
components/screenshot_taker/screenshot_taker.h:75: };
virtual dtor

[oshima, 2014-11-12 16:14:04]

can you also look into failing try jobs?

[Jun Mukai, 2014-11-12 19:36:21]

https://codereview.chromium.org/706013004/diff/60001/components/screenshot_ta...
File components/screenshot_taker/screenshot_taker.cc (right):

https://codereview.chromium.org/706013004/diff/60001/components/screenshot_ta...
components/screenshot_taker/screenshot_taker.cc:34:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
Can we use SequencedWorkerPool as a constructor parameter?  Then it doesn't have
to depend on the browser thread and content/

And then -- I think we can put the files into ui/snapshot rather than a new
directory in components.

[flackr, 2014-11-18 16:46:30]

https://codereview.chromium.org/706013004/diff/40001/ash/screenshot_delegate.h
File ash/screenshot_delegate.h (right):

https://codereview.chromium.org/706013004/diff/40001/ash/screenshot_delegate....
ash/screenshot_delegate.h:36: }  // namespace
On 2014/11/12 11:32:14, oshima wrote:
> namespace ash

Done.

https://codereview.chromium.org/706013004/diff/40001/chrome/browser/ui/ash/ch...
File chrome/browser/ui/ash/chrome_screenshot_taker.cc (right):

https://codereview.chromium.org/706013004/diff/40001/chrome/browser/ui/ash/ch...
chrome/browser/ui/ash/chrome_screenshot_taker.cc:150: Profile* profile_;
On 2014/11/12 11:32:14, oshima wrote:
> Is it hard to change this to BrowserContext? (It's often the case that callee
> just use it as browser context)

It looks like it's only used as a browser context, but would require changing
many other methods in file_manager, drive, and a few other places. If you'd like
I can go ahead and do this here, or even as a follow-on change.

https://codereview.chromium.org/706013004/diff/40001/components/screenshot_ta...
File components/screenshot_taker/screenshot_taker.h (right):

https://codereview.chromium.org/706013004/diff/40001/components/screenshot_ta...
components/screenshot_taker/screenshot_taker.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2014/11/12 11:32:14, oshima wrote:
> nit: 2014

Done.

https://codereview.chromium.org/706013004/diff/40001/components/screenshot_ta...
components/screenshot_taker/screenshot_taker.h:19: class Profile;
On 2014/11/12 11:32:14, oshima wrote:
> you don't need this right?

Done.

https://codereview.chromium.org/706013004/diff/40001/components/screenshot_ta...
components/screenshot_taker/screenshot_taker.h:75: };
On 2014/11/12 11:32:14, oshima wrote:
> virtual dtor

Done.

https://codereview.chromium.org/706013004/diff/60001/components/screenshot_ta...
File components/screenshot_taker/screenshot_taker.cc (right):

https://codereview.chromium.org/706013004/diff/60001/components/screenshot_ta...
components/screenshot_taker/screenshot_taker.cc:34:
DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
On 2014/11/12 19:36:21, Jun Mukai wrote:
> Can we use SequencedWorkerPool as a constructor parameter?  Then it doesn't
have
> to depend on the browser thread and content/
> 
> And then -- I think we can put the files into ui/snapshot rather than a new
> directory in components.

Done.

[oshima, 2014-11-18 21:06:04]

fyi: removing myself from reviewer.

[oshima, 2014-11-18 21:06:13]

oshima@chromium.org changed reviewers:
- oshima@chromium.org

[Jun Mukai, 2014-11-18 21:56:15]

lgtm with a nit

https://codereview.chromium.org/706013004/diff/100001/ui/snapshot/BUILD.gn
File ui/snapshot/BUILD.gn (right):

https://codereview.chromium.org/706013004/diff/100001/ui/snapshot/BUILD.gn#ne...
ui/snapshot/BUILD.gn:10: "screenshot_taker.h"
They are currently only for ash right?

[flackr, 2014-11-20 00:06:05]

flackr@chromium.org changed reviewers:
+ sky@chromium.org

[flackr, 2014-11-20 00:06:07]

+sky for OWNERS review. Please take a look, thanks!

https://codereview.chromium.org/706013004/diff/100001/ui/snapshot/BUILD.gn
File ui/snapshot/BUILD.gn (right):

https://codereview.chromium.org/706013004/diff/100001/ui/snapshot/BUILD.gn#ne...
ui/snapshot/BUILD.gn:10: "screenshot_taker.h"
On 2014/11/18 21:56:15, Jun Mukai wrote:
> They are currently only for ash right?

Currently only used by ash, but should technically work on other platforms since
it's using snapshot of a NativeWindow. I can exclude compilation on other
platforms though.

[sky, 2014-11-20 15:03:59]

https://codereview.chromium.org/706013004/diff/120001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/706013004/diff/120001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:677:
chrome_screenshot_taker->screenshot_taker()->AddObserver(&observer_);
RemoveObserver?

https://codereview.chromium.org/706013004/diff/120001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_screenshot_taker.h (right):

https://codereview.chromium.org/706013004/diff/120001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_screenshot_taker.h:20: class ChromeScreenshotTaker
: public ash::ScreenshotDelegate,
Same comment on naming.

https://codereview.chromium.org/706013004/diff/120001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_screenshot_taker.h:46: virtual Profile*
GetProfile();
I'm not a fan of virtuals just for tests. I much prefer injecting the profile.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
File ui/snapshot/screenshot_taker.cc (right):

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:31: typedef
base::Callback<void(ScreenshotTakerObserver::Result screenshot_result,
using.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:65: static_cast<int>(png_data->size()))) !=
png_data->size()) {
DCHECK size isn't too big (because of cast)?

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:92:
callback.Run(ScreenshotTakerClient::WRITABLE_FILE_SUCCESS, path);
Why is the callback run directly here but posted to the UI thread above?

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:130: is_partial = rect == aura_window->bounds();
I would have thought you compare sizes here? But this gets back to my question
in the .h as to what coordinates rect is in.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:131: window_identifier =
aura_window->GetBoundsInScreen().ToString();
What is the window identifier used for? Seems bizarre to use the bounds in
screen as the identifier.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:172: if (!png_data.get()) {
DCHECK for thread?

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:190: base::Bind(&SaveScreenshot,
base::MessageLoop::current()->task_runner(),
Nested bindings... Holy unreadable code batman!

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
File ui/snapshot/screenshot_taker.h (right):

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:26: class SNAPSHOT_EXPORT ScreenshotTakerObserver
{
move to its own file.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:52: class SNAPSHOT_EXPORT ScreenshotTakerClient {
'taker' should sounds like bad english. Did you consider grabber?

Also, I think this is more of a delegate than client.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:54: enum WritableFileResult {
'Writable' is rather extraneous and confusing here. I think you can nuke it.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:63: typedef
base::Callback<void(WritableFileResult result,
using.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:72: virtual void
PrepareWritableFileAndRunOnBlockingPool(
PrepareFileAndRun...

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:75: WritableFileCallback
callback_on_blocking_pool);
const&?

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:85: const gfx::Rect& rect,
It's not clear what coordinates rect is in. Please document it.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:86: const base::FilePath& screenshot_paths);
screenshot_path

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:108: base::Time last_screenshot_timestamp_;
timeticks

[flackr, 2014-11-26 18:05:09]

https://codereview.chromium.org/706013004/diff/120001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/706013004/diff/120001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:677:
chrome_screenshot_taker->screenshot_taker()->AddObserver(&observer_);
On 2014/11/20 15:03:58, sky wrote:
> RemoveObserver?

Done.

https://codereview.chromium.org/706013004/diff/120001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_screenshot_taker.h (right):

https://codereview.chromium.org/706013004/diff/120001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_screenshot_taker.h:20: class ChromeScreenshotTaker
: public ash::ScreenshotDelegate,
On 2014/11/20 15:03:58, sky wrote:
> Same comment on naming.

Done.

https://codereview.chromium.org/706013004/diff/120001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_screenshot_taker.h:46: virtual Profile*
GetProfile();
On 2014/11/20 15:03:58, sky wrote:
> I'm not a fan of virtuals just for tests. I much prefer injecting the profile.

Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
File ui/snapshot/screenshot_taker.cc (right):

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:31: typedef
base::Callback<void(ScreenshotTakerObserver::Result screenshot_result,
On 2014/11/20 15:03:59, sky wrote:
> using.

Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:65: static_cast<int>(png_data->size()))) !=
png_data->size()) {
On 2014/11/20 15:03:59, sky wrote:
> DCHECK size isn't too big (because of cast)?

Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:92:
callback.Run(ScreenshotTakerClient::WRITABLE_FILE_SUCCESS, path);
On 2014/11/20 15:03:59, sky wrote:
> Why is the callback run directly here but posted to the UI thread above?

Oops, we actually should always run the result on the blocking pool (which will
post to ui thread on error on line 71). Removed post to ui thread above and
added DCHECK that we're not running callback (SaveScreenshot) on ui thread.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:130: is_partial = rect == aura_window->bounds();
On 2014/11/20 15:03:59, sky wrote:
> I would have thought you compare sizes here? But this gets back to my question
> in the .h as to what coordinates rect is in.

Oops, the inequality was wrong, but yes this can just be a size check. Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:131: window_identifier =
aura_window->GetBoundsInScreen().ToString();
On 2014/11/20 15:03:59, sky wrote:
> What is the window identifier used for? Seems bizarre to use the bounds in
> screen as the identifier.

It's only used for the logged error message (line 180) to indicate which screen
the screenshot failed on.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:172: if (!png_data.get()) {
On 2014/11/20 15:03:59, sky wrote:
> DCHECK for thread?

Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.cc:190: base::Bind(&SaveScreenshot,
base::MessageLoop::current()->task_runner(),
On 2014/11/20 15:03:59, sky wrote:
> Nested bindings... Holy unreadable code batman!

Does it help to locally declare the inner callback like so?

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
File ui/snapshot/screenshot_taker.h (right):

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:26: class SNAPSHOT_EXPORT ScreenshotTakerObserver
{
On 2014/11/20 15:03:59, sky wrote:
> move to its own file.

Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:52: class SNAPSHOT_EXPORT ScreenshotTakerClient {
On 2014/11/20 15:03:59, sky wrote:
> 'taker' should sounds like bad english. Did you consider grabber?
> 
> Also, I think this is more of a delegate than client.

Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:54: enum WritableFileResult {
On 2014/11/20 15:03:59, sky wrote:
> 'Writable' is rather extraneous and confusing here. I think you can nuke it.

Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:63: typedef
base::Callback<void(WritableFileResult result,
On 2014/11/20 15:03:59, sky wrote:
> using.

Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:72: virtual void
PrepareWritableFileAndRunOnBlockingPool(
On 2014/11/20 15:03:59, sky wrote:
> PrepareFileAndRun...

Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:75: WritableFileCallback
callback_on_blocking_pool);
On 2014/11/20 15:03:59, sky wrote:
> const&?

Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:85: const gfx::Rect& rect,
On 2014/11/20 15:03:59, sky wrote:
> It's not clear what coordinates rect is in. Please document it.

Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:86: const base::FilePath& screenshot_paths);
On 2014/11/20 15:03:59, sky wrote:
> screenshot_path

Done.

https://codereview.chromium.org/706013004/diff/120001/ui/snapshot/screenshot_...
ui/snapshot/screenshot_taker.h:108: base::Time last_screenshot_timestamp_;
On 2014/11/20 15:03:59, sky wrote:
> timeticks

Done.

[sky, 2014-12-01 17:01:18]

Yes, LGTM

[flackr, 2014-12-01 18:25:06]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-12-01 18:26:12]

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

[commit-bot: I haz the power, 2014-12-01 18:31:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-01 18:31:03]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[flackr, 2014-12-03 13:42:15]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2014-12-03 13:42:35]

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

[commit-bot: I haz the power, 2014-12-03 14:35:45]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2014-12-03 14:36:34]

Patchset 11 (id:??) landed as
https://crrev.com/149080069ead026c48e07c32430ec98bed17f75c
Cr-Commit-Position: refs/heads/master@{#306600}