Expose a way to set a view's base background color.

Expose a way to set a view's base background color.

BUG=263263
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154891

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154942

[joth, 2013-07-22 04:56:52]

I need a hand with how to push through the invalidation...

[jamesr, 2013-07-22 17:56:32]

What is a view's base background color?  Why is the BUG= line empty?

[aelias_OOO_until_Jul13, 2013-07-22 18:30:33]

On 2013/07/22 17:56:32, jamesr wrote:
> What is a view's base background color?  Why is the BUG= line empty?

Context at http://b/6593265, we don't have a crbug for it yet.

[joth, 2013-07-23 01:20:18]

On 2013/07/22 17:56:32, jamesr wrote:
> What is a view's base background color?  

I attempted to document it in the bug and in WebView.h, but basically, it's 
FrameView::baseBackgroundColor()  (exists prior to this patch)
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...


> Why is the BUG= line empty?

Fixed. https://code.google.com/p/chromium/issues/detail?id=263263

[aelias_OOO_until_Jul13, 2013-07-23 19:02:16]

https://codereview.chromium.org/19883002/diff/14001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/19883002/diff/14001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3577: m_isTransparent =
!m_baseBackgroundColor.alpha();
This is testing for complete transparency, but isn't partial transparency
enough?  I think this should probably be:

m_isTransparent = m_baseBackgroundColor.hasAlpha();

(which tests for alpha() < 255).

[aelias_OOO_until_Jul13, 2013-07-23 19:05:01]

https://codereview.chromium.org/19883002/diff/14001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/19883002/diff/14001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3574: void
WebViewImpl::setBaseBackgroundColor(WebColor color)
Please add an early return if the color hasn't changed.

[joth, 2013-07-23 21:13:16]

https://codereview.chromium.org/19883002/diff/14001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/19883002/diff/14001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3574: void
WebViewImpl::setBaseBackgroundColor(WebColor color)
On 2013/07/23 19:05:01, aelias wrote:
> Please add an early return if the color hasn't changed.

Done

https://codereview.chromium.org/19883002/diff/14001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3577: m_isTransparent =
!m_baseBackgroundColor.alpha();
On 2013/07/23 19:02:16, aelias wrote:
> This is testing for complete transparency, but isn't partial transparency
> enough?  I think this should probably be:
> 
> m_isTransparent = m_baseBackgroundColor.hasAlpha();
> 
> (which tests for alpha() < 255).

My reading of WebViewImpl::backgroundColor() is that m_isTransparent == true
means that the background is 100% transparent: it short-circuits calculating any
background color and just returns Color::transparent.

Ah... so that means WebView::setTransparent() *overrides* and CSS
background-color, c.f. setBaseBackgroundColor which does not.  So, I've removed
this line from here.

Done

[aelias_OOO_until_Jul13, 2013-07-23 23:51:27]

Could you add a unit test?

https://codereview.chromium.org/19883002/diff/25001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/19883002/diff/25001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3582:
m_page->mainFrame()->view()->updateBackgroundRecursively(c, m_isTransparent);
Here you don't check that the FrameView is non-NULL.  But it looks like the
whole point of keeping a separate value of m_baseBackgroundColor in WebViewImpl
is to initialize the FrameView to the right color.

[joth, 2013-07-24 05:38:50]

On 2013/07/23 23:51:27, aelias wrote:
> Could you add a unit test?
> 

Done. Hopefully it's in the blink style (I'm on unfamiliar ground here...)


> https://codereview.chromium.org/19883002/diff/25001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/19883002/diff/25001/Source/web/WebViewImpl.cp...
> Source/web/WebViewImpl.cpp:3582:
> m_page->mainFrame()->view()->updateBackgroundRecursively(c, m_isTransparent);
> Here you don't check that the FrameView is non-NULL. 

Right. This seems the prevailing pattern, and AFAICT the main frame view will
always be valid except perhaps prior to first WebView::initializeMainFrame()
call and when the thing is being destroyed, and it seems reasonable to disallow
calling setBaseBackgroundColor at those times ?
Certainly in writing tests I couldn't get it to hit a NULL here.

> But it looks like the
> whole point of keeping a separate value of m_baseBackgroundColor in
WebViewImpl
> is to initialize the FrameView to the right color.

Not quite. It's main job (besides allowing the early out in the setter) is to
make baseBackground() return the correct (non-white) value in the various
early-outs in has. No page valid? During shutdown? Not really sure how to force
them -- mostly wanted to get rid of spurious Color::white return paths.

Thanks

[aelias_OOO_until_Jul13, 2013-07-24 06:56:02]

OK, lg2m.  I double-checked FrameView lifetime and it seems fine.  James, PTAL?

https://codereview.chromium.org/19883002/diff/33001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/19883002/diff/33001/Source/web/WebViewImpl.h#...
Source/web/WebViewImpl.h:329: WebCore::Color baseBackgroundColor() const {
return m_baseBackgroundColor; }
nit: move this out from between the two "overlay" methods.  (No natural place to
put it, just move it somewhere else random.)

[joth, 2013-07-24 17:15:57]

On 2013/07/24 06:56:02, aelias wrote:
> OK, lg2m.  I double-checked FrameView lifetime and it seems fine.  James,
PTAL?
> 
> https://codereview.chromium.org/19883002/diff/33001/Source/web/WebViewImpl.h
> File Source/web/WebViewImpl.h (right):
> 
>
https://codereview.chromium.org/19883002/diff/33001/Source/web/WebViewImpl.h#...
> Source/web/WebViewImpl.h:329: WebCore::Color baseBackgroundColor() const {
> return m_baseBackgroundColor; }
> nit: move this out from between the two "overlay" methods.  (No natural place
to
> put it, just move it somewhere else random.)

Done. 
Also fixed WebViewImpl::setBaseBackgroundColor to not be interleaved in the
m_isTransparent setter and getter.

[joth, 2013-07-24 23:11:00]

+eae@chromium.org FYI as my FrameView.cpp baseBackgroundColor() edit very
slightly overlaps with your just-landed SytleColor change.
https://chromiumcodereview.appspot.com/20061003

[jamesr, 2013-07-24 23:16:27]

Hmm, can't setting a base background color change the opaque-ness of the root
layer? I don't see any code notifying the layer that this may have occurred -
see the setIsTransparent() code.

https://codereview.chromium.org/19883002/diff/42001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/19883002/diff/42001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3583: Color c(static_cast<RGBA32>(color));
avoid one-letter variable names

WebColor and RGBA32 are both just typedefs for unsigned, do you really need a
cast between them?

[joth, 2013-07-24 23:57:51]

Thanks James. New patchset uploaded https://codereview.chromium.org/19883002


On 24 July 2013 16:16, <jamesr@chromium.org> wrote:

> Hmm, can't setting a base background color change the opaque-ness of the
> root
> layer? I don't see any code notifying the layer that this may have
> occurred -
> see the setIsTransparent() code.
>
>
Yes.
So in WebViewImpl setBaseBackgroundColor I call
FrameView::updateBackgroundRecursively() which
calls FrameView::setBaseBackgroundColor which
calls backing->updateContentsOpaque
That should nail it? (My testing here, in downstream chromium, and
down-downstream android, all suggest it's working... but I may have
systematically screwed something up)




>
> https://codereview.chromium.**org/19883002/diff/42001/**
>
Source/web/WebViewImpl.cpp<https://codereview.chromium.org/19883002/diff/42001/Source/web/WebViewImpl.cpp>
> File Source/web/WebViewImpl.cpp (right):
>
> https://codereview.chromium.**org/19883002/diff/42001/**
>
Source/web/WebViewImpl.cpp#**newcode3583<https://codereview.chromium.org/19883002/diff/42001/Source/web/WebViewImpl.cpp#newcode3583>
> Source/web/WebViewImpl.cpp:**3583: Color c(static_cast<RGBA32>(color));
> avoid one-letter variable names
>
> Done.


> WebColor and RGBA32 are both just typedefs for unsigned, do you really
> need a cast between them?
>
>
oppps yeah sorry about that -- a remnant of ill-informed copy and paste
before I worked out what I was doing (bad example in
Source/web/ColorChooserUIController.cpp)






>
https://codereview.chromium.**org/19883002/<https://codereview.chromium.org/1...
>

[jamesr, 2013-07-25 00:02:27]

https://codereview.chromium.org/19883002/diff/54001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/19883002/diff/54001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3589:
m_page->mainFrame()->view()->updateBackgroundRecursively(color,
m_isTransparent);
hmm, why recursive? won't this only change the background color on the root
frameview?

[jamesr, 2013-07-25 00:16:06]

https://codereview.chromium.org/19883002/diff/54001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/19883002/diff/54001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3589:
m_page->mainFrame()->view()->updateBackgroundRecursively(color,
m_isTransparent);
On 2013/07/25 00:02:28, jamesr wrote:
> hmm, why recursive? won't this only change the background color on the root
> frameview?

I'm pretty sure calling FrameView::setBaseBackgroundColor() is the right thing
to do. it already has logic to call
renderView()->layer()->backing()->updateContentsOpaque(), so you don't have to
add that other call.

[joth, 2013-07-25 00:39:43]

You're right, of course! The recursive call is not needed, at least to the limit
my testing can show.


(I was somewhat blindly copying the pattern from WebViewImpl::setIsTransparent
which also "recurses" all frame views in the tree, just the same way that
FrameView::updateBackgroundRecursively does)

https://codereview.chromium.org/19883002/diff/54001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/19883002/diff/54001/Source/web/WebViewImpl.cp...
Source/web/WebViewImpl.cpp:3589:
m_page->mainFrame()->view()->updateBackgroundRecursively(color,
m_isTransparent);
On 2013/07/25 00:16:06, jamesr wrote:
> On 2013/07/25 00:02:28, jamesr wrote:
> > hmm, why recursive? won't this only change the background color on the root
> > frameview?
> 
> I'm pretty sure calling FrameView::setBaseBackgroundColor() is the right thing
> to do. it already has logic to call
> renderView()->layer()->backing()->updateContentsOpaque(), so you don't have to
> add that other call.

Done.

(just to clarify: I am relying on the existing updateContentsOpaque() either way
: I haven't added any new call to it just rearranged it to use the already
null-checked 'backing' local)

[jamesr, 2013-07-25 00:41:37]

lgtm

[commit-bot: I haz the power, 2013-07-25 00:55:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/19883002/65001

[commit-bot: I haz the power, 2013-07-25 03:20:55]

Change committed as 154891

[joth, 2013-07-25 18:23:22]

Previous patch was reverted - 
Very minor diff in the test in PS15 to reland:-
https://codereview.chromium.org/19883002/diff2/65001:76001/Source/web/tests/W...

(data: URLs don't work in unit tests on android it turns out, and CQ doesn't run
these tests on android)

[commit-bot: I haz the power, 2013-07-25 19:02:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/19883002/76001

[commit-bot: I haz the power, 2013-07-25 22:41:36]

Change committed as 154942