Sets the default background color of inline signin and user manager to grey

Sets the default background color of inline signin and user manager to grey

The purpose is to avoid the white flash before the web content is loaded.

This CL only handles the view implementation, mac implementation will be handled in the next cl.

BUG=368225
Committed: https://crrev.com/fe517ffc1a9550ff4557b5718b01b103bd134616
Cr-Commit-Position: refs/heads/master@{#299594}

Committed: https://crrev.com/0d3bef753bf4541c906b3d5b90f691a8e2dbddd7
Cr-Commit-Position: refs/heads/master@{#300961}

[guohui, 2014-10-07 21:00:25]

guohui@chromium.org changed reviewers:
+ noms@chromium.org, rogerta@chromium.org, sky@chromium.org

[guohui, 2014-10-07 21:00:26]

Hey,

could you please review the CL?

noms@ for chrome/browser/profiles/...
sky@ for chrome/browser/ui/views/... and content/...
rogerta@ for the entire CL

Thanks,

Hui

[sky, 2014-10-07 22:03:59]

https://codereview.chromium.org/637083002/diff/40001/content/public/browser/r...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/637083002/diff/40001/content/public/browser/r...
content/public/browser/render_widget_host_view.h:114: virtual void
SetBackgroundColor(SkColor color) = 0;
Can SetBackgroundOpaque be removed if you add this?

[noms (inactive), 2014-10-08 14:35:04]

profiles lgtm

[guohui, 2014-10-08 18:11:51]

https://codereview.chromium.org/637083002/diff/40001/content/public/browser/r...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/637083002/diff/40001/content/public/browser/r...
content/public/browser/render_widget_host_view.h:114: virtual void
SetBackgroundColor(SkColor color) = 0;
On 2014/10/07 22:03:59, sky wrote:
> Can SetBackgroundOpaque be removed if you add this?

Theoretically yes, you could replace SetBackgroundOpaque(false) with
SetBackgroundColor(0x00). However i prefer to keep both since all existing
callers of SetBakcgroundOpaque do not care about the color, also maybe in the
future we will support colored transparent background.

[guohui, 2014-10-08 18:22:33]

https://codereview.chromium.org/637083002/diff/40001/content/public/browser/r...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/637083002/diff/40001/content/public/browser/r...
content/public/browser/render_widget_host_view.h:114: virtual void
SetBackgroundColor(SkColor color) = 0;
On 2014/10/08 18:11:51, guohui wrote:
> On 2014/10/07 22:03:59, sky wrote:
> > Can SetBackgroundOpaque be removed if you add this?
> 
> Theoretically yes, you could replace SetBackgroundOpaque(false) with
> SetBackgroundColor(0x00). However i prefer to keep both since all existing
> callers of SetBakcgroundOpaque do not care about the color, also maybe in the
> future we will support colored transparent background.

eh i forgot SkColor has an alpha component, so it could represent a transparent
color, then i do agree that it is cleaner to have only one method to set the
transparency. I ll fix it in a new patch.

Thanks!

[guohui, 2014-10-08 19:13:37]

Patchset #1 (id:1) has been deleted

[guohui, 2014-10-08 19:13:44]

Patchset #1 (id:20001) has been deleted

[guohui, 2014-10-08 20:18:14]

Patchset #2 (id:60001) has been deleted

[guohui, 2014-10-08 20:27:14]

Patchset #2 (id:80001) has been deleted

[guohui, 2014-10-08 20:32:50]

In patch 2, I kept both SetBackgroundOpaque and SetBackgroundColor but removed
the member variable background_opaque_ and delegated SetBackgroundOpaque to
SetBackgroundColor, so that not only we have a consistent way of setting the
transparency, but also the existing callers who care nothing about the color
dont need to pass the default color everywhere.

I also added mac implementation, which is required now due to interface changes.

Please take another look, thanks!

[guohui, 2014-10-08 21:32:55]

guohui@chromium.org changed reviewers:
+ asvitkine@chromium.org

[guohui, 2014-10-08 21:32:55]

+asvitkine@ for cocoa implementation,

chrome/browser/ui/cocoa/profiles/...
content/browser/renderer_host/render_widget_host_view_mac.mm

[sky, 2014-10-08 22:23:19]

As early mentioned I'm not a fan of both background color and opaque.
Why are you keeping both?

On Wed, Oct 8, 2014 at 1:32 PM,  <guohui@chromium.org> wrote:
> In patch 2, I kept both SetBackgroundOpaque and SetBackgroundColor but
> removed
> the member variable background_opaque_ and delegated SetBackgroundOpaque to
> SetBackgroundColor, so that not only we have a consistent way of setting the
> transparency, but also the existing callers who care nothing about the color
> dont need to pass the default color everywhere.
>
> I also added mac implementation, which is required now due to interface
> changes.
>
> Please take another look, thanks!
>
>
> https://codereview.chromium.org/637083002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[guohui, 2014-10-09 14:49:55]

On 2014/10/08 22:23:19, sky wrote:
> As early mentioned I'm not a fan of both background color and opaque.
> Why are you keeping both?
> 
> On Wed, Oct 8, 2014 at 1:32 PM,  <mailto:guohui@chromium.org> wrote:
> > In patch 2, I kept both SetBackgroundOpaque and SetBackgroundColor but
> > removed
> > the member variable background_opaque_ and delegated SetBackgroundOpaque to
> > SetBackgroundColor, so that not only we have a consistent way of setting the
> > transparency, but also the existing callers who care nothing about the color
> > dont need to pass the default color everywhere.
> >
> > I also added mac implementation, which is required now due to interface
> > changes.
> >
> > Please take another look, thanks!
> >
> >
> > https://codereview.chromium.org/637083002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

As discussed offline, the major concern i had with having callers to pass the
default color is that the current
approach of hardcoding the default color code is not accessibility friendly,
e.g. it looks bad on high contrast
screens. By moving the hardcoded color code from callees to callers, we will
create more hardcoded values in
out code and thus make it even harder to fix the issue in the future.

However I do agree that the current name of SetBackgroundOpaque is confusing
since it may suggest the
opaqueness could be modified independently of the color. Thus I removed the
existing method
SetBackgroundOpaque, and updated callers to either call
SetBackgroundColor(Sk_ColorTRANSPARENT) for
transparent background or a new method FillBackgroundWithDefaultColor() for
default opaque background.

Please take another look, thanks!

[guohui, 2014-10-09 15:51:46]

Patchset #3 (id:120001) has been deleted

[Alexei Svitkine (slow), 2014-10-09 15:51:47]

asvitkine@chromium.org changed reviewers:
+ ccameron@google.com

[Alexei Svitkine (slow), 2014-10-09 15:51:48]

+ccameron, who's a better reviewer for render_widget_host_view_mac.mm changes.

https://codereview.chromium.org/637083002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/637083002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1534: if
(render_widget_host_) {
Nit: No {}'s.

https://codereview.chromium.org/637083002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:1537: if
(background_layer_.get()) {
Nit: Is the .get() needed?

[guohui, 2014-10-09 16:01:12]

On 2014/10/09 15:51:48, Alexei Svitkine wrote:
> +ccameron, who's a better reviewer for render_widget_host_view_mac.mm changes.
> 
>
https://codereview.chromium.org/637083002/diff/120001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
>
https://codereview.chromium.org/637083002/diff/120001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_mac.mm:1534: if
> (render_widget_host_) {
> Nit: No {}'s.
Done.

> 
>
https://codereview.chromium.org/637083002/diff/120001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_mac.mm:1537: if
> (background_layer_.get()) {
> Nit: Is the .get() needed?
Done.

[guohui, 2014-10-09 16:05:39]

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

[guohui, 2014-10-09 16:05:40]

+sievers for render_widget_host_view_android and content_view_core_impl.cc.

[ccameron, 2014-10-09 16:54:32]

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

[ccameron, 2014-10-09 16:54:32]

lgtm Mac bits, with an extra request: can we make it so that web contents
appears with a background color appropriate for the theme, too? I have a grey
test theme, and it looks strange to be flashing white.

[guohui, 2014-10-09 17:24:16]

guohui@chromium.org changed reviewers:
+ benwells@chromium.org

[guohui, 2014-10-09 17:24:17]

+benwells@ for extensions/...

@ccameron, good point, though i guess it should be addressed in a separate CL,
would you mind filing a bug for it?

Thanks!

[Alexei Svitkine (slow), 2014-10-09 18:01:00]

lgtm

[benwells, 2014-10-09 22:22:57]

native_app_window_views lgtm

Please get a more specific OWNER for the web_view change, like fsamuel@ or
lazyboy@

[guohui, 2014-10-09 22:28:16]

guohui@chromium.org changed reviewers:
+ fsamuel@chromium.org, nasko@chromium.org

[guohui, 2014-10-09 22:28:17]

+fsamule@ for owner review of
extensions/browser/guest_view/web_view/web_view_guest.cc and

+nasko@ for owner review of
render_widget_host_view_guest.cc

[Fady Samuel, 2014-10-10 01:32:45]

web_view_guest lgtm

[sky, 2014-10-10 02:17:33]

LGTM

https://codereview.chromium.org/637083002/diff/180001/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/637083002/diff/180001/content/public/browser/...
content/public/browser/render_widget_host_view.h:113: virtual void
FillBackgroundWithDefaultColor() = 0;
FillBackground sounds immediate. I would go with SetBackgroundColorToDefault.

[guohui, 2014-10-10 17:20:17]

https://codereview.chromium.org/637083002/diff/180001/content/public/browser/...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/637083002/diff/180001/content/public/browser/...
content/public/browser/render_widget_host_view.h:113: virtual void
FillBackgroundWithDefaultColor() = 0;
On 2014/10/10 02:17:33, sky wrote:
> FillBackground sounds immediate. I would go with SetBackgroundColorToDefault.

Done.

[nasko, 2014-10-13 17:16:22]

Rubberstamp LGTM, since fady@ has reviewed render_widget_host_view_guest*.

[guohui, 2014-10-14 11:47:10]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-10-14 11:47:57]

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

[commit-bot: I haz the power, 2014-10-14 11:51:10]

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

[commit-bot: I haz the power, 2014-10-14 11:51:12]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/68310)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[guohui, 2014-10-14 20:04:44]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-10-14 20:07:13]

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

[commit-bot: I haz the power, 2014-10-15 00:19:40]

Committed patchset #8 (id:450001)

[commit-bot: I haz the power, 2014-10-15 00:20:26]

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

[Ken Russell (switch to Gerrit), 2014-10-15 01:50:42]

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

[Ken Russell (switch to Gerrit), 2014-10-15 01:50:43]

I'm reverting the patch above due to apparent test breakage on the Blink
waterfall which I think is likely to block Blink rolls. See Issue
https://code.google.com/p/chromium/issues/detail?id=368225#c5 .

(I'm not sure why this wasn't caught by the commit queue. Perhaps a change in
Blink caused this code path to start being exercised, and that hasn't rolled
into Chromium yet?)

[Ken Russell (switch to Gerrit), 2014-10-15 01:51:19]

A revert of this CL (patchset #8 id:450001) has been created in
https://codereview.chromium.org/654123004/ by kbr@chromium.org.

The reason for reverting is: Broke content_unittests on Blink waterfall. See
https://code.google.com/p/chromium/issues/detail?id=368225#c5 .
.

[guohui, 2014-10-23 20:17:23]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-10-23 20:18:30]

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

[commit-bot: I haz the power, 2014-10-23 22:13:05]

Committed patchset #9 (id:470001)

[commit-bot: I haz the power, 2014-10-23 22:13:48]

Patchset 9 (id:??) landed as
https://crrev.com/0d3bef753bf4541c906b3d5b90f691a8e2dbddd7
Cr-Commit-Position: refs/heads/master@{#300961}