Update scrollbar layer opacity when scrollbar theme changes

Update scrollbar layer opacity when scrollbar theme changes

If the scrollbar theme changes from being opaque to non-opaque (as happen
when unplugging an external mouse on Mac), a situation results where the
scrollbar layers think they were opaque (because they were when they were
created), but the content that they draw assumes that it is not opaque, and
black scrollbars result.

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

[jamesr, 2014-07-10 01:28:25]

Can you get rid of the ScrollingCoordinator indirection for opacity completely
and push the correct bit inside ScrollView's positionScrollbarLayer()?  The
ScrollingCoordinator has the ScrollbarMap, but we don't need to talk to the
WebScrollbarLayer here just the WebLayer which we can get to from the
GraphicsLayer, IIRC.

[ccameron, 2014-07-10 02:57:20]

On 2014/07/10 01:28:25, jamesr wrote:
> Can you get rid of the ScrollingCoordinator indirection for opacity completely
> and push the correct bit inside ScrollView's positionScrollbarLayer()?  The
> ScrollingCoordinator has the ScrollbarMap, but we don't need to talk to the
> WebScrollbarLayer here just the WebLayer which we can get to from the
> GraphicsLayer, IIRC.

I can get *a* WebLayer from the GraphicsLayer, but it appears to be a different
WebLayer than the one that we get from getWebScrollbarLayer(...)->layer(). In
particular, the one I'm looking at is
scrollableArea->layerForVerticalScrollbar()->layer(), and it is different from
the one that I'm setting inside ScrollingCoordinator. Perhaps it's a parent or a
child (though we don't seem to have accessors for that).

[jamesr, 2014-07-10 03:13:01]

Ideally the ownership of the WebScrollbarLayer itself would be closer to
ScrollView instead of being out in a map-on-the-side in ScrollingCoordinator. 
The point of ScrollingCoordinator was  to insulate the /core/ code from the
layers used for scrollbars (mostly the type) back when there was a CoreAnimation
and chromium backend.  That need has long gone since this code only ever talks
to chromium's compositor, so we have this extra layer in the middle that just
gets in the way.  Sadness.

You want the GraphicsLayer's m_contentsLayer.  It's not exposed (currently) but
it'll be the first child of the GraphicsLayer's m_layer, but that's fragile to
rely on.

[jamesr, 2014-07-10 04:14:11]

That said, doesn't setContentsOpaque() on the graphicslayer do what you want? 
Why is there another call?  Maybe it'd be useful to dump out the layer tree
structure under the GraphicsLayer for the scrollbar so we can see what all these
things are.

[ccameron, 2014-07-10 04:57:55]

On 2014/07/10 04:14:11, jamesr wrote:
> That said, doesn't setContentsOpaque() on the graphicslayer do what you want? 

Nah, not quite -- the WebLayer that we get through the ScrollingCoordinator
isn't the contents layer of the graphics layer.

> Why is there another call?

It looks like only the getWebScrollbarLayer()->layer()->setOpaque() is needed
(at least on Mac, empirically, in the cases that I tested)

> Maybe it'd be useful to dump out the layer tree
> structure under the GraphicsLayer for the scrollbar so we can see what all
these
> things are.

Is there a link that shows how to print out the
WebLayer/GraphicsLayer/RenderLayer trees? I'm particularly interested in dumping
them at the moment that I change the theme.

[jamesr, 2014-07-10 04:59:27]

You'd probably have to write some code to dump things, but it'd probably be
worth it.

[jamesr, 2014-07-10 06:45:05]

On 2014/07/10 04:57:55, ccameron1 wrote:
> Is there a link that shows how to print out the
> WebLayer/GraphicsLayer/RenderLayer trees? I'm particularly interested in
dumping
> them at the moment that I change the theme.

Actually there is for RenderLayer trees:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[ccameron, 2014-07-10 07:06:40]

On 2014/07/10 04:57:55, ccameron1 wrote:
> On 2014/07/10 04:14:11, jamesr wrote:
> > That said, doesn't setContentsOpaque() on the graphicslayer do what you
want? 
> 
> Nah, not quite -- the WebLayer that we get through the ScrollingCoordinator
> isn't the contents layer of the graphics layer.

It was an especially cruel person who gave GraphicsLayer both a contentLayer and
a contentsLayer. Of note is that calling GraphicsLayer::SetContentsOpaque
doesn't actually change the opacity of the contentsLayer -- it changes the
opacity of the content (without an s) layer. To improve readability, I suggest
that we replace "content" or "contents" (either one) with "wombat".

Indeed, for a the layer that we need to set the opacity on is indeed the
contentsLayer (with an s) of layerForVerticalScrollbar (oops). I took a dump of
the graphics tree below:

*SV* ScrollView::adjustScrollbarOpacity's
layerForVerticalScrollbar()->platformLayer()=0x7b147a40
*SC* ScrollingCoordinator::updateScrollbarOpacity's
getWebScrollbarLayer()->layer()=0x7b147b90
  GraphicsLayer=0x531549d0, platformLayer()=0x7b1473e0, contentsLayer()=0x0
    GraphicsLayer=0x53154b70, platformLayer()=0x7b147e50, contentsLayer()=0x0
      GraphicsLayer=0x531544f0, platformLayer()=0x7b147530, contentsLayer()=0x0
        GraphicsLayer=0x53154690, platformLayer()=0x7b147680,
contentsLayer()=0x0
          GraphicsLayer=0x531541b0, platformLayer()=0x7b1463c0,
contentsLayer()=0x0
            GraphicsLayer=0x53154d10, platformLayer()=0x7a6343a0,
contentsLayer()=0x0
          GraphicsLayer=0x53154350, platformLayer()=0x7b147840,
contentsLayer()=0x7b147930
      GraphicsLayer=0x53154830, platformLayer()=0x7b147a40 *SV*,
contentsLayer()=0x7b147b90 *SC*
      GraphicsLayer=0x53154eb0, platformLayer()=0x7b624b90, contentsLayer()=0x0

So ... solutions include
1. Calling layerForVerticalScrollbar()->contentsLayer()->setOpaque()
(contentsLayer has a "protected, testing only" next to it which would need to be
ignored).
2. Making GraphicsLayer::SetContentsOpaque change the contentsLayer in addition
to the contentLayer
3. Adding a GraphicsLayer::SetContentsLayerOpaque function to do the reaching
into contentsLayer

> > Why is there another call?
> 
> It looks like only the getWebScrollbarLayer()->layer()->setOpaque() is needed
> (at least on Mac, empirically, in the cases that I tested)

If we make setContentsOpaque to set the contentsLayer to be opaque (option 2),
then this becomes one call.  I've updated the patch with option 2 (seeing if it
happens to not break anything).

[jamesr, 2014-07-10 22:05:51]

On 2014/07/10 07:06:40, ccameron1 wrote:
> On 2014/07/10 04:57:55, ccameron1 wrote:
> > On 2014/07/10 04:14:11, jamesr wrote:
> > > That said, doesn't setContentsOpaque() on the graphicslayer do what you
> want? 
> > 
> > Nah, not quite -- the WebLayer that we get through the ScrollingCoordinator
> > isn't the contents layer of the graphics layer.
> 
> It was an especially cruel person who gave GraphicsLayer both a contentLayer
and
> a contentsLayer. Of note is that calling GraphicsLayer::SetContentsOpaque
> doesn't actually change the opacity of the contentsLayer -- it changes the
> opacity of the content (without an s) layer. To improve readability, I suggest
> that we replace "content" or "contents" (either one) with "wombat".

I say we go with 'wombatLayer' and 'wombatsLayer' :)

> 
> Indeed, for a the layer that we need to set the opacity on is indeed the
> contentsLayer (with an s) of layerForVerticalScrollbar (oops). I took a dump
of
> the graphics tree below:
> 
> *SV* ScrollView::adjustScrollbarOpacity's
> layerForVerticalScrollbar()->platformLayer()=0x7b147a40
> *SC* ScrollingCoordinator::updateScrollbarOpacity's
> getWebScrollbarLayer()->layer()=0x7b147b90
>   GraphicsLayer=0x531549d0, platformLayer()=0x7b1473e0, contentsLayer()=0x0
>     GraphicsLayer=0x53154b70, platformLayer()=0x7b147e50, contentsLayer()=0x0
>       GraphicsLayer=0x531544f0, platformLayer()=0x7b147530,
contentsLayer()=0x0
>         GraphicsLayer=0x53154690, platformLayer()=0x7b147680,
> contentsLayer()=0x0
>           GraphicsLayer=0x531541b0, platformLayer()=0x7b1463c0,
> contentsLayer()=0x0
>             GraphicsLayer=0x53154d10, platformLayer()=0x7a6343a0,
> contentsLayer()=0x0
>           GraphicsLayer=0x53154350, platformLayer()=0x7b147840,
> contentsLayer()=0x7b147930
>       GraphicsLayer=0x53154830, platformLayer()=0x7b147a40 *SV*,
> contentsLayer()=0x7b147b90 *SC*
>       GraphicsLayer=0x53154eb0, platformLayer()=0x7b624b90,
contentsLayer()=0x0
> 
> So ... solutions include
> 1. Calling layerForVerticalScrollbar()->contentsLayer()->setOpaque()
> (contentsLayer has a "protected, testing only" next to it which would need to
be
> ignored).
> 2. Making GraphicsLayer::SetContentsOpaque change the contentsLayer in
addition
> to the contentLayer
> 3. Adding a GraphicsLayer::SetContentsLayerOpaque function to do the reaching
> into contentsLayer
> 
> > > Why is there another call?
> > 
> > It looks like only the getWebScrollbarLayer()->layer()->setOpaque() is
needed
> > (at least on Mac, empirically, in the cases that I tested)
> 
> If we make setContentsOpaque to set the contentsLayer to be opaque (option 2),
> then this becomes one call.  I've updated the patch with option 2 (seeing if
it
> happens to not break anything).

Hmm, it might work.  Are there any layout test failures from 2?  If so, we
should probably go with 3.

[ccameron, 2014-07-10 22:18:34]

On 2014/07/10 22:05:51, jamesr wrote:
> 
> Hmm, it might work.  Are there any layout test failures from 2?  If so, we
> should probably go with 3.

I see these guys failing on a few bots ... Not sure it's legit, taking a look
locally ...

editing/spelling/inline-spelling-markers-hidpi-composited.html
virtual/softwarecompositing/culling/tile-occlusion-boundaries.html
virtual/softwarecompositing/lots-of-img-layers.html

[ccameron, 2014-07-14 19:08:09]

On 2014/07/10 22:18:34, ccameron1 wrote:
> On 2014/07/10 22:05:51, jamesr wrote:
> > 
> > Hmm, it might work.  Are there any layout test failures from 2?  If so, we
> > should probably go with 3.
> 
> I see these guys failing on a few bots ... Not sure it's legit, taking a look
> locally ...
> 
> editing/spelling/inline-spelling-markers-hidpi-composited.html
> virtual/softwarecompositing/culling/tile-occlusion-boundaries.html
> virtual/softwarecompositing/lots-of-img-layers.html

The virtual/softwarecompositing ones are imperceptible pixel differences (which
I think are unrelated to this change). The other guy passes locally. Does this
lgty then?

[jamesr, 2014-07-17 19:43:24]

lgtm

the setContentsOpaque() call is being used for orthogonal things now. 
CompositedLayerMapping uses it to control the GraphicsLayer::m_layer, but that
code never runs for scrollbar layers.  This code only wants to really control
the GraphicsLayer::m_contentsLayer, since the m_layer is always empty for
scrollbar layers.  So it's pretty confusing but I think it should all work out.

[ccameron, 2014-07-17 22:09:04]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-07-17 22:09:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ccameron@chromium.org/383603002/60001

[commit-bot: I haz the power, 2014-07-17 23:17:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/16017)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17582)

[commit-bot: I haz the power, 2014-07-17 23:47:46]

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

[commit-bot: I haz the power, 2014-07-17 23:47:47]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[ccameron, 2014-07-18 03:58:42]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-07-18 03:58:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ccameron@chromium.org/383603002/80001

[commit-bot: I haz the power, 2014-07-18 04:15:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)

[commit-bot: I haz the power, 2014-07-18 04:28:12]

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

[commit-bot: I haz the power, 2014-07-18 04:28:13]

Try jobs failed on following builders:
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)

[ccameron, 2014-07-18 08:33:47]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-07-18 08:34:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ccameron@chromium.org/383603002/80001

[commit-bot: I haz the power, 2014-07-18 08:41:47]

Change committed as 178421