Move ImageWindowDelegate to ui/aura_extra

Move ImageWindowDelegate to ui/aura_extra

In order to be able to use it in places other than content (e.g. handles
for unified touch selection controller), ImageWindowDelegate is moved to
a new ui module called aura_extra.

Also, to make it more useful, ImageWindowDelegate can now be customized
by setting background color and image offset.

COLLABORATOR=mfomitchev
BUG=399721
Committed: https://crrev.com/166a918168791ad0f62ee28862ef520c5d7fbf35
Cr-Commit-Position: refs/heads/master@{#315344}

[mohsen, 2015-01-26 23:51:59]

Patchset #2 (id:20001) has been deleted

[mohsen, 2015-01-27 03:08:08]

Patchset #2 (id:40001) has been deleted

[mohsen, 2015-01-27 05:51:15]

Patchset #3 (id:80001) has been deleted

[mohsen, 2015-01-27 15:51:32]

mohsen@chromium.org changed reviewers:
+ sadrul@chromium.org

[mohsen, 2015-01-27 15:51:33]

sadrul@: Can you please take a look...

[sadrul, 2015-01-28 18:26:57]

https://codereview.chromium.org/880703002/diff/100001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/880703002/diff/100001/content/DEPS#newcode83
content/DEPS:83: "+ui/aura_extra",
Move this to content/browser/

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

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/BUILD.gn#...
ui/aura_extra/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
2015

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
File ui/aura_extra/image_window_delegate.cc (right):

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.cc:42: self_destroy_ = self_destroy;
Is this really necessary? It looks like this is used by TouchHandleDrawableAura,
but it could simply override OnWindowDestroyed instead to not 'delete this;'

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.cc:88: }
Can this be:

  if (image_.IsEmpty() || size_mismatch_)
    DrawColor

  if (!image_.IsEmpty())
    DrawImage

(whether we should paint an image or a color shouldn't depend on what the
background color is, or what the image offset is).

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
File ui/aura_extra/image_window_delegate.h (right):

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.h:25: void SetBackgroundColor(SkColor
color);
set_background_color

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.h:27: void SetImageOffset(gfx::Vector2d
offset);
set_image_offset

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.h:30: void SetSelfDestroy(bool
self_destroy);
set_self_destroy()

[mfomitchev, 2015-01-28 19:15:42]

mfomitchev@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[mfomitchev, 2015-01-28 19:15:43]

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
File ui/aura_extra/image_window_delegate.cc (right):

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.cc:88: }
If offset is non-zero we should paint the background color even if the sizes
match..

If the background color is SK_ColorTRANSPARENT, then there's no reason to paint
it (although there's no harm in doing so other than potential waste of cpu).

[sadrul, 2015-01-28 19:28:47]

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
File ui/aura_extra/image_window_delegate.cc (right):

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.cc:88: }
On 2015/01/28 19:15:42, mfomitchev wrote:
> If offset is non-zero we should paint the background color even if the sizes
> match..

Why?

> 
> If the background color is SK_ColorTRANSPARENT, then there's no reason to
paint
> it (although there's no harm in doing so other than potential waste of cpu).

[sadrul, 2015-01-28 19:30:04]

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
File ui/aura_extra/image_window_delegate.cc (right):

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.cc:88: }
On 2015/01/28 19:28:47, sadrul wrote:
> On 2015/01/28 19:15:42, mfomitchev wrote:
> > If offset is non-zero we should paint the background color even if the sizes
> > match..
> 
> Why?

Oh, I see, because there will be parts of the Window that will remain unpainted
otherwise. OK, that makes sense.

[mohsen, 2015-01-28 21:10:29]

https://codereview.chromium.org/880703002/diff/100001/content/DEPS
File content/DEPS (right):

https://codereview.chromium.org/880703002/diff/100001/content/DEPS#newcode83
content/DEPS:83: "+ui/aura_extra",
On 2015/01/28 18:26:56, sadrul wrote:
> Move this to content/browser/

Done.

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

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/BUILD.gn#...
ui/aura_extra/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
On 2015/01/28 18:26:56, sadrul wrote:
> 2015

Done.

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
File ui/aura_extra/image_window_delegate.cc (right):

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.cc:42: self_destroy_ = self_destroy;
On 2015/01/28 18:26:56, sadrul wrote:
> Is this really necessary? It looks like this is used by
TouchHandleDrawableAura,
> but it could simply override OnWindowDestroyed instead to not 'delete this;'

Actually, that was Mikhail's original approach, but, I found it a bit confusing
as an ImageWindowDelegate is expected to self-destroy, while
TouchHandleDrawableAura, which is an ImageWindowDelegate, does not self-destroy.

Do you think I should switch back to that approach?

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
File ui/aura_extra/image_window_delegate.h (right):

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.h:25: void SetBackgroundColor(SkColor
color);
On 2015/01/28 18:26:56, sadrul wrote:
> set_background_color

Done.

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.h:27: void SetImageOffset(gfx::Vector2d
offset);
On 2015/01/28 18:26:56, sadrul wrote:
> set_image_offset

Done.

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.h:30: void SetSelfDestroy(bool
self_destroy);
On 2015/01/28 18:26:56, sadrul wrote:
> set_self_destroy()

Done.

[sadrul, 2015-01-28 21:13:39]

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
File ui/aura_extra/image_window_delegate.cc (right):

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.cc:42: self_destroy_ = self_destroy;
On 2015/01/28 21:10:29, mohsen wrote:
> On 2015/01/28 18:26:56, sadrul wrote:
> > Is this really necessary? It looks like this is used by
> TouchHandleDrawableAura,
> > but it could simply override OnWindowDestroyed instead to not 'delete this;'
> 
> Actually, that was Mikhail's original approach, but, I found it a bit
confusing
> as an ImageWindowDelegate is expected to self-destroy, while
> TouchHandleDrawableAura, which is an ImageWindowDelegate, does not
self-destroy.
> 
> Do you think I should switch back to that approach?

Yeah, I think that'd be cleaner. (some other parts of the code base does this,
e.g. RenderViewObserver, BrowserMessageFilter etc.) Make sure to explain this
clearly in ImageWindowDelegate doc

[mohsen, 2015-01-29 03:23:15]

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
File ui/aura_extra/image_window_delegate.cc (right):

https://codereview.chromium.org/880703002/diff/100001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.cc:42: self_destroy_ = self_destroy;
On 2015/01/28 21:13:39, sadrul wrote:
> On 2015/01/28 21:10:29, mohsen wrote:
> > On 2015/01/28 18:26:56, sadrul wrote:
> > > Is this really necessary? It looks like this is used by
> > TouchHandleDrawableAura,
> > > but it could simply override OnWindowDestroyed instead to not 'delete
this;'
> > 
> > Actually, that was Mikhail's original approach, but, I found it a bit
> confusing
> > as an ImageWindowDelegate is expected to self-destroy, while
> > TouchHandleDrawableAura, which is an ImageWindowDelegate, does not
> self-destroy.
> > 
> > Do you think I should switch back to that approach?
> 
> Yeah, I think that'd be cleaner. (some other parts of the code base does this,
> e.g. RenderViewObserver, BrowserMessageFilter etc.) Make sure to explain this
> clearly in ImageWindowDelegate doc

Done.

[sadrul, 2015-01-30 00:14:43]

lgtm

https://codereview.chromium.org/880703002/diff/140001/ui/aura_extra/image_win...
File ui/aura_extra/image_window_delegate.h (right):

https://codereview.chromium.org/880703002/diff/140001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.h:67: // white background for the missing
regions.
*... then the delegate fills the missing region with |background_color_|
(default is white)

[mohsen, 2015-01-30 05:53:08]

https://codereview.chromium.org/880703002/diff/140001/ui/aura_extra/image_win...
File ui/aura_extra/image_window_delegate.h (right):

https://codereview.chromium.org/880703002/diff/140001/ui/aura_extra/image_win...
ui/aura_extra/image_window_delegate.h:67: // white background for the missing
regions.
On 2015/01/30 00:14:43, sadrul wrote:
> *... then the delegate fills the missing region with |background_color_|
> (default is white)

Done.

[mohsen, 2015-01-30 15:24:05]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2015-01-30 15:24:31]

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

[commit-bot: I haz the power, 2015-01-30 15:29:44]

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

[commit-bot: I haz the power, 2015-01-30 15:29:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mohsen, 2015-01-30 15:50:44]

mohsen@chromium.org changed reviewers:
+ jochen@chromium.org

[mohsen, 2015-01-30 15:50:45]

jochen@: for OWNERS in build/all.gyp and content/browser/. Can you please take a
look?

[jochen (gone - plz use gerrit), 2015-02-04 11:55:59]

lgtm

[mohsen, 2015-02-04 17:15:18]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2015-02-04 17:16:33]

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

[commit-bot: I haz the power, 2015-02-04 17:22:03]

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

[commit-bot: I haz the power, 2015-02-04 17:22:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mohsen, 2015-02-04 18:21:52]

mohsen@chromium.org changed reviewers:
+ reed@google.com

[mohsen, 2015-02-04 18:21:53]

reed@google.com: can you please review addition of third_party/skia to
ui/aura_extra/DEPS file?

[reed1, 2015-02-09 18:14:40]

aura DEPS adding skia lgtm

[mohsen, 2015-02-09 18:15:23]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2015-02-09 18:15:43]

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

[commit-bot: I haz the power, 2015-02-09 18:19:23]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-02-09 18:20:13]

Patchset 6 (id:??) landed as
https://crrev.com/166a918168791ad0f62ee28862ef520c5d7fbf35
Cr-Commit-Position: refs/heads/master@{#315344}