Issue 2715243004: [blink] Support (semi-)transparent background colors in WebView/Frame.

Eric Seckler

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-02-27 15:56:03 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715243004/1

3 years, 9 months ago (2017-02-27 15:56:25 UTC) #2

commit-bot: I haz the power

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

3 years, 9 months ago (2017-02-27 16:59:47 UTC) #3

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/396257)

3 years, 9 months ago (2017-02-27 16:59:48 UTC) #4

Eric Seckler

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-01 12:17:39 UTC) #5

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715243004/20001

3 years, 9 months ago (2017-03-01 12:17:54 UTC) #6

Eric Seckler

Description was changed from ========== [blink] Support (semi-)transparent background colors in WebView/Frame. We allow controlling ...

3 years, 9 months ago (2017-03-01 12:23:37 UTC) #7

Eric Seckler

eseckler@chromium.org changed reviewers: + dgozman@chromium.org, jam@chromium.org

3 years, 9 months ago (2017-03-01 13:00:32 UTC) #8

Eric Seckler

John, would you be a good person to take a look at this? I'm making ...

3 years, 9 months ago (2017-03-01 13:00:32 UTC) #9

commit-bot: I haz the power

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

3 years, 9 months ago (2017-03-01 14:02:11 UTC) #10

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-01 14:02:15 UTC) #11

jam

On 2017/03/01 13:00:32, Eric Seckler wrote: > John, would you be a good person to ...

3 years, 9 months ago (2017-03-01 15:47:47 UTC) #12

chrishtr

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

3 years, 9 months ago (2017-03-01 16:16:53 UTC) #13

chrishtr

https://codereview.chromium.org/2715243004/diff/20001/third_party/WebKit/Source/web/FullscreenController.cpp File third_party/WebKit/Source/web/FullscreenController.cpp (right): https://codereview.chromium.org/2715243004/diff/20001/third_party/WebKit/Source/web/FullscreenController.cpp#newcode199 third_party/WebKit/Source/web/FullscreenController.cpp:199: m_initialTransparency = m_webViewImpl->isTransparent(); For m_initialPageScaleFactor, it is "reset" in ...

3 years, 9 months ago (2017-03-01 16:16:54 UTC) #14

dgozman

https://codereview.chromium.org/2715243004/diff/20001/third_party/WebKit/Source/web/WebViewImpl.cpp File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2715243004/diff/20001/third_party/WebKit/Source/web/WebViewImpl.cpp#newcode4019 third_party/WebKit/Source/web/WebViewImpl.cpp:4019: m_layerTreeView->setHasTransparentBackground(alphaChannel(color) < Similarly to Chris' question, do we actually ...

3 years, 9 months ago (2017-03-01 19:11:52 UTC) #15

Eric Seckler

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-07 17:47:51 UTC) #16

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715243004/40001

3 years, 9 months ago (2017-03-07 17:49:15 UTC) #17

Eric Seckler

+lfg@ -jam@ Lucas: I'm attempting to remove setIsTransparent from WebViewImpl and use setBackgroundColorOverride in RenderViewImpl::OnSetBackgroundOpaque ...

3 years, 9 months ago (2017-03-07 18:00:47 UTC) #18

+lfg@ -jam@

Lucas: I'm attempting to remove setIsTransparent from WebViewImpl and use
setBackgroundColorOverride in RenderViewImpl::OnSetBackgroundOpaque instead. I'm
still having trouble with FrameView::SetTransparent(), however. Right now, I'm
trying to call SetTransparent according to whether the WebViewImpl's effective
background color is non-opaque. However, FrameView::IsTransparent() seems to
affect whether the the baseBackgroundColor is drawn at all (and because of that,
some tests fail):

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Vie...

I have the feeling that that's a different setting, and we shouldn't be setting
it depending on whether WebViewImpl::backgroundColor() is opaque or not?

Thank you! :)

https://codereview.chromium.org/2715243004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2715243004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FullscreenController.cpp:199:
m_initialTransparency = m_webViewImpl->isTransparent();
On 2017/03/01 16:16:53, chrishtr wrote:
> For m_initialPageScaleFactor, it is "reset" in
> FullscreenController::didUpdateLayout. Why is that done there and
> your new value reset in fullscreenElementChanged?

This stuff changed a bit now, but I still restore within
fullscreenElementChanged. I added a few comments explaining why.

https://codereview.chromium.org/2715243004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2715243004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:285: m_isTransparent =
isTransparent;
On 2017/03/01 16:16:53, chrishtr wrote:
> It seems it is not guaranteed that m_isTransparent and
> alphaChannel(color) < SK_AlphaOPAQUE are the same.
FWIW, I think this would have been sort-of guaranteed because m_isTransparent
overrode the backgroundColor to transparent.

> Should there even be a setIsTransparent method if we always
> know the actual background color including transparency?
Since lfg@ suggested the same, I'm attempting to get rid of it! :)

https://codereview.chromium.org/2715243004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2715243004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebViewImpl.cpp:4019:
m_layerTreeView->setHasTransparentBackground(alphaChannel(color) <
On 2017/03/01 19:11:52, dgozman wrote:
> Similarly to Chris' question, do we actually need two methods here if we
always
> pass the background color and layer tree view can reason on it's transparency?

Implemented as suggested.

Eric Seckler

eseckler@chromium.org changed reviewers: + lfg@chromium.org - jam@chromium.org

3 years, 9 months ago (2017-03-07 18:01:31 UTC) #19

Eric Seckler

+lfg -jam for real :P Lucas: I'm attempting to remove setIsTransparent from WebViewImpl and use ...

3 years, 9 months ago (2017-03-07 18:01:32 UTC) #20

commit-bot: I haz the power

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

3 years, 9 months ago (2017-03-07 19:01:31 UTC) #21

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/404011)

3 years, 9 months ago (2017-03-07 19:01:32 UTC) #22

chrishtr

Hi - I kind of assumed you are waiting on other feedback and not mine, ...

3 years, 9 months ago (2017-03-08 22:47:04 UTC) #23

Eric Seckler

On 2017/03/08 22:47:04, chrishtr wrote: > Hi - I kind of assumed you are waiting ...

3 years, 9 months ago (2017-03-08 22:51:25 UTC) #24

lfg

Apologies for the delay, as I was OOO last week. On 2017/03/07 18:01:32, Eric Seckler ...

3 years, 9 months ago (2017-03-20 21:35:07 UTC) #25

Eric Seckler

Description was changed from ========== [blink] Support (semi-)transparent background colors in WebView/Frame. We allow controlling ...

3 years, 9 months ago (2017-03-22 12:54:03 UTC) #26

Eric Seckler

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-22 12:54:31 UTC) #27

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715243004/60001

3 years, 9 months ago (2017-03-22 12:54:48 UTC) #28

commit-bot: I haz the power

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

3 years, 9 months ago (2017-03-22 13:14:19 UTC) #29

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/413988) linux_layout_tests_slimming_paint_v2 on ...

3 years, 9 months ago (2017-03-22 13:14:21 UTC) #30

Eric Seckler

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-22 13:54:52 UTC) #32

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715243004/80001

3 years, 9 months ago (2017-03-22 13:55:03 UTC) #33

commit-bot: I haz the power

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

3 years, 9 months ago (2017-03-22 14:29:04 UTC) #34

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_tests_slimming_paint_v2/builds/3405)

3 years, 9 months ago (2017-03-22 14:29:05 UTC) #35

Eric Seckler

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-22 14:44:41 UTC) #36

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715243004/100001

3 years, 9 months ago (2017-03-22 14:44:54 UTC) #37

Eric Seckler

On 2017/03/20 21:35:07, lfg wrote: > Apologies for the delay, as I was OOO last ...

3 years, 9 months ago (2017-03-22 16:16:43 UTC) #39

commit-bot: I haz the power

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

3 years, 9 months ago (2017-03-22 16:37:27 UTC) #40

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-22 16:37:28 UTC) #41

lfg

Thanks for doing this. lgtm with nits. https://codereview.chromium.org/2715243004/diff/100001/third_party/WebKit/Source/web/FullscreenController.h File third_party/WebKit/Source/web/FullscreenController.h (right): https://codereview.chromium.org/2715243004/diff/100001/third_party/WebKit/Source/web/FullscreenController.h#newcode35 third_party/WebKit/Source/web/FullscreenController.h:35: #include "platform/geometry/FloatPoint.h" ...

3 years, 9 months ago (2017-03-23 16:05:21 UTC) #42

Eric Seckler

Thanks, Lucas! Chris, I think this is ready for you to have another look now ...

3 years, 9 months ago (2017-03-23 16:34:49 UTC) #43

Eric Seckler

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-23 16:37:27 UTC) #44

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715243004/120001

3 years, 9 months ago (2017-03-23 16:37:56 UTC) #45

commit-bot: I haz the power

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

3 years, 9 months ago (2017-03-23 17:01:27 UTC) #46

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/415169)

3 years, 9 months ago (2017-03-23 17:01:28 UTC) #47

Eric Seckler

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-23 17:29:38 UTC) #48

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715243004/120001

3 years, 9 months ago (2017-03-23 17:30:10 UTC) #49

commit-bot: I haz the power

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

3 years, 9 months ago (2017-03-23 18:56:38 UTC) #50

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-23 18:56:39 UTC) #51

chrishtr

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Source/core/paint/ViewPainter.cpp File third_party/WebKit/Source/core/paint/ViewPainter.cpp (right): https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Source/core/paint/ViewPainter.cpp#newcode90 third_party/WebKit/Source/core/paint/ViewPainter.cpp:90: isMainFrame && (frameView.baseBackgroundColor().alpha() > 0); This doesn't look right. ...

3 years, 8 months ago (2017-03-28 19:45:56 UTC) #53

Eric Seckler

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-03-29 09:56:58 UTC) #54

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715243004/140001

3 years, 8 months ago (2017-03-29 09:57:15 UTC) #55

Eric Seckler

lfg@, PTAL at WebLocalFrameImpl comment :) https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Source/core/paint/ViewPainter.cpp File third_party/WebKit/Source/core/paint/ViewPainter.cpp (right): https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Source/core/paint/ViewPainter.cpp#newcode90 third_party/WebKit/Source/core/paint/ViewPainter.cpp:90: isMainFrame && (frameView.baseBackgroundColor().alpha() ...

3 years, 8 months ago (2017-03-29 10:50:18 UTC) #56

lfg@, PTAL at WebLocalFrameImpl comment :)

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/ViewPainter.cpp (right):

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/ViewPainter.cpp:90: isMainFrame &&
(frameView.baseBackgroundColor().alpha() > 0);
On 2017/03/28 19:45:55, chrishtr wrote:
> This doesn't look right. hasAlpha() instead? Is there a layout or unittest
> exercising this code?

hasAlpha() == alpha() < 255, i.e. is true iff not-opaque.

We actually want to paint the base background in any case except full
transparency, see an earlier reply from lfg@.

There are quite a few layout tests that exercise this, plus a unit test for
semi-transparency [1]. To check, I replaced this condition and ran the try bots:
a) using hasAlpha(), about 5000 layout tests fail (primarily SVG related) [2],
because full transparency doesn't work correctly.
b) using !hasAlpha(), about the unit test fails [3], because semi-transparency
doesn't work correctly.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebV...
[2] https://codereview.chromium.org/2784783002/#ps1
[3] https://codereview.chromium.org/2784783002/#ps60001

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/FullscreenController.cpp:190:
restoreBackgroundColorOverride();
On 2017/03/28 19:45:56, chrishtr wrote:
> Are the code changes in this file tested by a layout test or unittest?

Yeah, there's a unit test [1], which fails if we don't set transparency
correctly here [2].

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebF...
[2] https://codereview.chromium.org/2784783002/#ps20001

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1721: baseBackgroundColor =
Color::transparent;
On 2017/03/28 19:45:56, chrishtr wrote:
> I think the old code just used a conditional here to detect alpha and
> pass it along, but still respected the non-alpha parts of
> webView->baseBackgroundColor(). Also, tested?

My understanding is that the old code was setting FrameView::isTransparent to
true, which resulted in !paintsBackgroundColor in ViewPainter, thus no
background color would have been painted for the FrameView. Setting a completely
transparent baseBackgroundColor should have the same effect now.

Based on the fact that I forgot to actually use the local variable in createView
below, I think it's a safe bet that this isn't tested :P

lfg@, do you have a suggestion of how to test this, since you added this
condition in https://codereview.chromium.org/1565893004 ? :)

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3571:
view->updateBaseBackgroundColorRecursively(color);
On 2017/03/28 19:45:56, chrishtr wrote:
> Why this new line?

setIsTransparent was previously setting the transparency bit on all existing
FrameViews. It needed to do that, because it was called when some FrameViews may
already exist.

Now that we are using the baseBackgroundColor(Override) to enable transparency,
setBaseBackgroundColor may be called when some FrameViews already exist. (I
don't think the baseBackgroundColor was previously intended to change after
initial WebViewImpl creation.) In any case, we now need to pass on the
baseBackgroundColor update to all existing FrameViews, so that we preserve a
behavior similar to what setIsTransparent did previously.

commit-bot: I haz the power

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

3 years, 8 months ago (2017-03-29 11:48:37 UTC) #57

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 8 months ago (2017-03-29 11:48:38 UTC) #58

lfg

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp#newcode1721 third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1721: baseBackgroundColor = Color::transparent; On 2017/03/29 10:50:18, Eric Seckler wrote: ...

3 years, 8 months ago (2017-03-29 15:19:43 UTC) #59

chrishtr

lgtm LGTM modulo lfg@'s comments. https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Source/core/paint/ViewPainter.cpp File third_party/WebKit/Source/core/paint/ViewPainter.cpp (right): https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Source/core/paint/ViewPainter.cpp#newcode90 third_party/WebKit/Source/core/paint/ViewPainter.cpp:90: isMainFrame && (frameView.baseBackgroundColor().alpha() > ...

3 years, 8 months ago (2017-03-29 15:36:41 UTC) #60

lgtm

LGTM modulo lfg@'s comments.

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/ViewPainter.cpp (right):

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/ViewPainter.cpp:90: isMainFrame &&
(frameView.baseBackgroundColor().alpha() > 0);
On 2017/03/29 at 10:50:18, Eric Seckler wrote:
> On 2017/03/28 19:45:55, chrishtr wrote:
> > This doesn't look right. hasAlpha() instead? Is there a layout or unittest
> > exercising this code?
> 
> hasAlpha() == alpha() < 255, i.e. is true iff not-opaque.
> 
> We actually want to paint the base background in any case except full
transparency, see an earlier reply from lfg@.
> 
> There are quite a few layout tests that exercise this, plus a unit test for
semi-transparency [1]. To check, I replaced this condition and ran the try bots:
> a) using hasAlpha(), about 5000 layout tests fail (primarily SVG related) [2],
because full transparency doesn't work correctly.
> b) using !hasAlpha(), about the unit test fails [3], because semi-transparency
doesn't work correctly.
> 
> [1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebV...
> [2] https://codereview.chromium.org/2784783002/#ps1
> [3] https://codereview.chromium.org/2784783002/#ps60001

Ah I see now. The old code was calling isTransparent(), which meant alpha() ==
0. Got it, thanks for the thorough investigation.

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.cpp:3571:
view->updateBaseBackgroundColorRecursively(color);
On 2017/03/29 at 10:50:18, Eric Seckler wrote:
> On 2017/03/28 19:45:56, chrishtr wrote:
> > Why this new line?
> 
> setIsTransparent was previously setting the transparency bit on all existing
FrameViews. It needed to do that, because it was called when some FrameViews may
already exist.
> 
> Now that we are using the baseBackgroundColor(Override) to enable
transparency, setBaseBackgroundColor may be called when some FrameViews already
exist. (I don't think the baseBackgroundColor was previously intended to change
after initial WebViewImpl creation.) In any case, we now need to pass on the
baseBackgroundColor update to all existing FrameViews, so that we preserve a
behavior similar to what setIsTransparent did previously.

Ok.

Eric Seckler

eseckler@chromium.org changed reviewers: + dcheng@chromium.org, jbroman@chromium.org, piman@chromium.org - dgozman@chromium.org

3 years, 8 months ago (2017-03-30 10:46:37 UTC) #61

Eric Seckler

+dcheng@ for WebKit/web +jbroman@ for Webkit/platform +piman@ for content/ Thanks! https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp#newcode1721 ...

3 years, 8 months ago (2017-03-30 10:46:38 UTC) #62

+dcheng@ for WebKit/web

+jbroman@ for Webkit/platform

+piman@ for content/

Thanks!

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2715243004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1721: baseBackgroundColor =
Color::transparent;
On 2017/03/29 15:19:43, lfg wrote:
> On 2017/03/29 10:50:18, Eric Seckler wrote:
> > On 2017/03/28 19:45:56, chrishtr wrote:
> > > I think the old code just used a conditional here to detect alpha and
> > > pass it along, but still respected the non-alpha parts of
> > > webView->baseBackgroundColor(). Also, tested?
> > 
> > My understanding is that the old code was setting FrameView::isTransparent
to
> > true, which resulted in !paintsBackgroundColor in ViewPainter, thus no
> > background color would have been painted for the FrameView. Setting a
> completely
> > transparent baseBackgroundColor should have the same effect now.
> 
> Yes, that's my understanding as well.
> 
> > Based on the fact that I forgot to actually use the local variable in
> createView
> > below, I think it's a safe bet that this isn't tested :P
> > 
> > lfg@, do you have a suggestion of how to test this, since you added this
> > condition in https://codereview.chromium.org/1565893004 ? :)
> 
> Yes, ideally, this should be a pixel test where we can check that the
background
> is being drawn properly. At the time I added that, there was no way to do
pixel
> tests on out-of-process iframes (which is what we are checking here -- remote
> parent), I know that lukasza@ (OOO) did a lot of work around this area (see
bug
> 587175), but I'm not sure if it's fully functional yet.
> 
> Maybe alternatively, we could write a unit test in WebFrameTest that just
> creates a LocalFrame with a RemoteFrame parent, and asserts that the
background
> color is transparent. It's not as good as a layouttest, but at least gives us
> some test coverage (you can file a bug and assign it to me, no need to do it
in
> this CL).

Added a unit test and filed https://crbug.com/706782 for the layout test.

Eric Seckler

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

3 years, 8 months ago (2017-03-30 10:46:58 UTC) #63

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715243004/160001

3 years, 8 months ago (2017-03-30 10:47:19 UTC) #64

commit-bot: I haz the power

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

3 years, 8 months ago (2017-03-30 12:13:54 UTC) #65

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 8 months ago (2017-03-30 12:13:55 UTC) #66

jbroman

Chris' approval covers Source/platform/, so removing myself as a reviewer.

3 years, 8 months ago (2017-03-30 14:24:21 UTC) #67

Eric Seckler

On 2017/03/30 14:24:21, jbroman wrote: > Chris' approval covers Source/platform/, so removing myself as a ...

3 years, 8 months ago (2017-03-30 14:29:43 UTC) #68

jbroman

On 2017/03/30 at 14:29:43, eseckler wrote: > On 2017/03/30 14:24:21, jbroman wrote: > > Chris' ...

3 years, 8 months ago (2017-03-30 14:32:01 UTC) #69

jbroman

jbroman@chromium.org changed reviewers: - jbroman@chromium.org

3 years, 8 months ago (2017-03-30 14:32:08 UTC) #70

Eric Seckler

eseckler@chromium.org changed reviewers: - dcheng@chromium.org

3 years, 8 months ago (2017-03-30 14:32:57 UTC) #71

Eric Seckler

On 2017/03/30 14:29:43, Eric Seckler wrote: > On 2017/03/30 14:24:21, jbroman wrote: > > Chris' ...

3 years, 8 months ago (2017-03-30 14:32:58 UTC) #72

dcheng

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

3 years, 8 months ago (2017-03-31 01:21:14 UTC) #74

dcheng

https://codereview.chromium.org/2715243004/diff/160001/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2715243004/diff/160001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode2357 third_party/WebKit/Source/core/frame/FrameView.cpp:2357: forAllNonThrottledFrameViews([baseBackgroundColor](FrameView& frameView) { Btw, I'm wondering something: we only ...

3 years, 8 months ago (2017-03-31 01:21:15 UTC) #75

Eric Seckler

https://codereview.chromium.org/2715243004/diff/160001/third_party/WebKit/Source/core/frame/FrameView.cpp File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2715243004/diff/160001/third_party/WebKit/Source/core/frame/FrameView.cpp#newcode2357 third_party/WebKit/Source/core/frame/FrameView.cpp:2357: forAllNonThrottledFrameViews([baseBackgroundColor](FrameView& frameView) { On 2017/03/31 01:21:15, dcheng wrote: > ...

3 years, 8 months ago (2017-03-31 08:32:28 UTC) #76

Eric Seckler

The patchset sent to the CQ was uploaded after l-g-t-m from lfg@chromium.org, chrishtr@chromium.org Link to ...

3 years, 8 months ago (2017-03-31 09:06:12 UTC) #78

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715243004/160001

3 years, 8 months ago (2017-03-31 09:06:27 UTC) #79

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490951172117850, "parent_rev": "826f594306b79439714faec487b06a9fd860c4a2", "commit_rev": "f2bd55f7d81eae9cd0736aaf68879ceb0309c92e"}

3 years, 8 months ago (2017-03-31 09:12:02 UTC) #80

commit-bot: I haz the power

Description was changed from ========== [blink] Support (semi-)transparent background colors in WebView/Frame. We allow controlling ...

3 years, 8 months ago (2017-03-31 09:12:48 UTC) #81

commit-bot: I haz the power

3 years, 8 months ago (2017-03-31 09:12:50 UTC) #82

Message was sent while issue was closed.

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/f2bd55f7d81eae9cd0736aaf6887...

Issue 2715243004: [blink] Support (semi-)transparent background colors in WebView/Frame. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Set WebViewImpl transparency for fullscreen. #

Patch Set 3 : Use background color override for setIsTransparent. #

Patch Set 4 : remove set/isTransparent from WebViewImpl and FrameView, use setBaseBackgroundColor instead #

Patch Set 5 : address comments #

Patch Set 6 : fix WebLocalFrameImpl::createFrameView() #

Patch Set 7 : add unit test for remote frame transparency #

Messages