Bug 79951 - [chromium] Mark root layer scrollbars as always opaque to disable blending
Summary: [chromium] Mark root layer scrollbars as always opaque to disable blending
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on: 78872
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-29 15:30 PST by Adrienne Walker
Modified: 2012-03-13 15:09 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.94 KB, patch)
2012-02-29 15:36 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (5.48 KB, patch)
2012-03-09 15:42 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Whoops, remove printf (4.38 KB, patch)
2012-03-09 15:43 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch for landing (4.88 KB, patch)
2012-03-09 16:20 PST, Adrienne Walker
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-02-29 15:30:39 PST
[chromium] Mark root layer scrollbars as always opaque to disable blending
Comment 1 Adrienne Walker 2012-02-29 15:36:19 PST
Created attachment 129537 [details]
Patch
Comment 2 James Robinson 2012-02-29 15:37:12 PST
Comment on attachment 129537 [details]
Patch

I do not think overlay root layer scrollbars are always opaque (custom or non-custom). This needs some more checks, methinks.
Comment 3 James Robinson 2012-02-29 15:39:12 PST
Comment on attachment 129537 [details]
Patch

I think checking for isOverlayScrollbar() might be sufficient - all non-overlay root layer scrollbars are definitely opaque.  We need to handle overlay->non overlay transitions and vice versa, but we should get a setViewport() call when that happens.
Comment 4 Adrienne Walker 2012-02-29 15:44:40 PST
(In reply to comment #3)
> (From update of attachment 129537 [details])
> I think checking for isOverlayScrollbar() might be sufficient - all non-overlay root layer scrollbars are definitely opaque.  We need to handle overlay->non overlay transitions and vice versa, but we should get a setViewport() call when that happens.

Oh, quite right.  Scrollbar::isOverlayScrollbar is pretty far away from LayerChromium at that point, and it seems like a layering violation to pipe it in.

I suppose the other option is to wait and do this at the ScrollbarLayer level after https://bugs.webkit.org/show_bug.cgi?id=78872 lands.
Comment 5 James Robinson 2012-02-29 15:52:40 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 129537 [details] [details])
> > I think checking for isOverlayScrollbar() might be sufficient - all non-overlay root layer scrollbars are definitely opaque.  We need to handle overlay->non overlay transitions and vice versa, but we should get a setViewport() call when that happens.
> 
> Oh, quite right.  Scrollbar::isOverlayScrollbar is pretty far away from LayerChromium at that point, and it seems like a layering violation to pipe it in.
> 

Ah yeah, that is farther than I thought (I was thinking we could just reach in to the ScrollableArea but we've helpfully abstracted NonCompositedContentHost away from being able to do things like that).

> I suppose the other option is to wait and do this at the ScrollbarLayer level after https://bugs.webkit.org/show_bug.cgi?id=78872 lands.

Coincidentally I've been looking at that patch. That would suck hardcore to have to merge if we need to do that.

Other gross option: pass isOverlay bits in the setViewport call from WebViewImpl.
Comment 6 Adrienne Walker 2012-03-09 15:42:03 PST
Created attachment 131126 [details]
Patch
Comment 7 Adrienne Walker 2012-03-09 15:43:19 PST
Created attachment 131127 [details]
Whoops, remove printf
Comment 8 James Robinson 2012-03-09 15:55:32 PST
Comment on attachment 131127 [details]
Whoops, remove printf

If we pass FrameView* to the scrollbarLayerDidChange function then we can also move the scrollLayerForFrameView() code into that helper too, instead of duplicating it across both of the frameView*ScrollbarLayerDidChange() functions
Comment 9 Adrienne Walker 2012-03-09 16:20:45 PST
Created attachment 131130 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-03-09 21:13:01 PST
Comment on attachment 131130 [details]
Patch for landing

Rejecting attachment 131130 [details] from commit-queue.

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-text-alignment.html
Full output: http://queues.webkit.org/results/11892959
Comment 11 Adrienne Walker 2012-03-12 13:03:20 PDT
Comment on attachment 131130 [details]
Patch for landing

This passes locally for me.  Let me try this again.
Comment 12 WebKit Review Bot 2012-03-12 14:22:10 PDT
Comment on attachment 131130 [details]
Patch for landing

Rejecting attachment 131130 [details] from commit-queue.

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/canvas-text-alignment.html
Full output: http://queues.webkit.org/results/11949021
Comment 13 Adrienne Walker 2012-03-13 15:06:47 PDT
Committed r110620: <http://trac.webkit.org/changeset/110620>
Comment 14 Adrienne Walker 2012-03-13 15:09:59 PDT
(In reply to comment #13)
> Committed r110620: <http://trac.webkit.org/changeset/110620>

Failing test was due to https://bugs.webkit.org/show_bug.cgi?id=78529 (and test being disabled on Linux debug).  This patch should make debug and release equivalent and I will rebaseline after this goes through the bots.