Start caching the theme color in web_contents

This implements APIs plumbing theme color changes to java
 side and cached it in web_contents_impl. Fixes the reset
and publish mechanism where we now wait for the first
visually non empty paint to publish the color, making sure
we can reset it on page load begin and have time enough to
get the color update. Once this is in, we can remove the
workarounds on the java side for the reset.

BUG=435768
Committed: https://crrev.com/d41c5f99c2937d00bb234a75387c7329998b8a7f
Cr-Commit-Position: refs/heads/master@{#319377}

[Yusuf, 2015-03-04 21:44:50]

yusufo@chromium.org changed reviewers:
+ sievers@chromium.org

[Yusuf, 2015-03-04 21:49:50]

yusufo@chromium.org changed reviewers:
+ tedchoc@chromium.org

[no sievers, 2015-03-04 23:00:46]

https://codereview.chromium.org/975293004/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/975293004/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2774:
rwhv->SetThemeColor(theme_color);
Why don't you just cache it in here?
Esp. since the getter API is also in WebContents.

[Yusuf, 2015-03-04 23:26:30]

https://codereview.chromium.org/975293004/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/975293004/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2774:
rwhv->SetThemeColor(theme_color);
On 2015/03/04 23:00:46, sievers wrote:
> Why don't you just cache it in here?
> Esp. since the getter API is also in WebContents.

Then wouldn't we have to refresh it explicitly when we navigate to a new URL? We
get a new rwhv in that case, but web contents wont change, right?

[no sievers, 2015-03-04 23:37:43]

https://codereview.chromium.org/975293004/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/975293004/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2774:
rwhv->SetThemeColor(theme_color);
On 2015/03/04 23:26:29, Yusuf wrote:
> On 2015/03/04 23:00:46, sievers wrote:
> > Why don't you just cache it in here?
> > Esp. since the getter API is also in WebContents.
> 
> Then wouldn't we have to refresh it explicitly when we navigate to a new URL?
We
> get a new rwhv in that case, but web contents wont change, right?

Wait, isn't the bug then that we don't get a notification here although the
theme color changed?

[Yusuf, 2015-03-04 23:38:56]

https://codereview.chromium.org/975293004/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/975293004/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2774:
rwhv->SetThemeColor(theme_color);
On 2015/03/04 23:37:42, sievers wrote:
> On 2015/03/04 23:26:29, Yusuf wrote:
> > On 2015/03/04 23:00:46, sievers wrote:
> > > Why don't you just cache it in here?
> > > Esp. since the getter API is also in WebContents.
> > 
> > Then wouldn't we have to refresh it explicitly when we navigate to a new
URL?
> We
> > get a new rwhv in that case, but web contents wont change, right?
> 
> Wait, isn't the bug then that we don't get a notification here although the
> theme color changed?

We only get notified if there is a color, but don't get anything if there isn't.

[no sievers, 2015-03-05 01:23:39]

It would be nice if we could have WebContents trigger
NotifyNavigationStateChanged(INVALIDATE_TYPE_THEME_COLOR) when we know the new
document does or does not have a theme color.
We can then remove WebContentsObserver::DidChangeThemeColor() (because now we'd
be using WebContentsDelegateAndroid::navigationStateChanged()).
And maybe WebContents::GetThemeColor() could return a bool whether the color is
valid.

[Yusuf, 2015-03-05 20:04:24]

Changed to caching in web_contents. We always reset on load and then publish
once there is a non empty paint since at that point the theme color should have
updated.

[no sievers, 2015-03-05 20:18:41]

https://codereview.chromium.org/975293004/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/975293004/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:131: const SkColor
kDefaultThemeColor = 0xFFF2F2F2;
It would be nice if this could be set by the application.

What's actually easier is to not worry about the default color here at all, but
rather make it 'SkColor* WebContents::GetThemeColor()' which returns NULL for
none/default (for example by having scoped_ptr<SkColor>
WebContentsImpl::theme_color_).

Then you could either plumb this up to Java by doing the same
(android.graphics.Color WebContents.getThemeColor() which could return null), or
you split it into two APIs in web_contents_android.cc
(isThemeColorValid()/getThemeColor()).

https://codereview.chromium.org/975293004/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3281:
DidChangeThemeColor(theme_color_));
Should we cache the last sent color and only notify if it really changed?

[Yusuf, 2015-03-05 22:05:15]

https://codereview.chromium.org/975293004/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/975293004/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:131: const SkColor
kDefaultThemeColor = 0xFFF2F2F2;
On 2015/03/05 20:18:41, sievers wrote:
> It would be nice if this could be set by the application.
> 
> What's actually easier is to not worry about the default color here at all,
but
> rather make it 'SkColor* WebContents::GetThemeColor()' which returns NULL for
> none/default (for example by having scoped_ptr<SkColor>
> WebContentsImpl::theme_color_).
> 
> Then you could either plumb this up to Java by doing the same
> (android.graphics.Color WebContents.getThemeColor() which could return null),
or
> you split it into two APIs in web_contents_android.cc
> (isThemeColorValid()/getThemeColor()).

Moved to setting it to transparent and handling the rest on the java side. We
can not set the omnibox to transparent anyway, so I think using
Sk_ColorTRANSPARENT is a good solution.

https://codereview.chromium.org/975293004/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:3281:
DidChangeThemeColor(theme_color_));
On 2015/03/05 20:18:41, sievers wrote:
> Should we cache the last sent color and only notify if it really changed?

Done.

[no sievers, 2015-03-05 22:38:49]

lgtm

[Yusuf, 2015-03-05 22:39:04]

The CQ bit was checked by yusufo@chromium.org

[commit-bot: I haz the power, 2015-03-05 22:40:13]

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

[commit-bot: I haz the power, 2015-03-06 00:14:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-06 00:15:57]

Patchset 3 (id:??) landed as
https://crrev.com/d41c5f99c2937d00bb234a75387c7329998b8a7f
Cr-Commit-Position: refs/heads/master@{#319377}