Bug 62491

Summary: Flash of white on left and right edges of screen when showing fullscreen controller when movie doesn't fill the entire screen
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+

Description Jer Noble 2011-06-10 16:25:00 PDT
Flash of white on left and right edges of screen when showing fullscreen controller when movie doesn't fill the entire screen
Comment 1 Jer Noble 2011-06-10 16:28:38 PDT
Created attachment 96822 [details]
Patch
Comment 2 Jer Noble 2011-06-10 17:02:27 PDT
Comment on attachment 96822 [details]
Patch

Obseleting.  Does not actually fix the flash.
Comment 3 Jer Noble 2011-06-10 19:45:58 PDT
Created attachment 96837 [details]
Patch
Comment 4 Jer Noble 2011-06-10 23:10:37 PDT
Created attachment 96845 [details]
Patch

Forgot to set up the bounds of the newly created content layer.
Comment 5 Darin Adler 2011-06-11 09:06:26 PDT
Comment on attachment 96845 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96845&action=review

Looks great to me, but you said Simon should review it. I am setting review+ but I am not Simon!

> Source/WebCore/rendering/RenderLayerBacking.cpp:314
> +        // Special case the RenderFullScreen case because we know its
> +        // style does not come from CSS and it is therefore safe to set
> +        // the layer's background color to the renderer's style's background
> +        // color.
> +        updateBackgroundColor();

This comment says why it’s safe to use updateBackgroundColor here, but not why it’s important to use it (to avoid flashes you would see if we used a tiled layer).

Also, the statement “its style does not come from CSS” does not clearly express to me why it’s safe to use the background color. It’s tantalizingly close to saying why, but doesn’t quite make it clear.

Also, that orphaned word color bugs me ;-)
Comment 6 Jer Noble 2011-06-11 13:43:09 PDT
(In reply to comment #5)
> (From update of attachment 96845 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96845&action=review
> 
> Looks great to me, but you said Simon should review it. I am setting review+ but I am not Simon!
> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:314
> > +        // Special case the RenderFullScreen case because we know its
> > +        // style does not come from CSS and it is therefore safe to set
> > +        // the layer's background color to the renderer's style's background
> > +        // color.
> > +        updateBackgroundColor();
> 
> This comment says why it’s safe to use updateBackgroundColor here, but not why it’s important to use it (to avoid flashes you would see if we used a tiled layer).
> 
> Also, the statement “its style does not come from CSS” does not clearly express to me why it’s safe to use the background color. It’s tantalizingly close to saying why, but doesn’t quite make it clear.
> 
> Also, that orphaned word color bugs me ;-)

New comment:

        // RenderFullScreen renderers have no content, and only a solid
        // background color.  They also can be large enough to trigger the
        // creation of a tiled-layer, which can cause flashing problems
        // during repainting.  Special case the RenderFullScreen case because
        // we know its style does not come from CSS and it is therefore will
        // not contain paintable content (e.g. background images, gradients,
        // etc.), so it's safe to set the layer's background color to the 
        // renderer's style's background color.

Now "color's" less lonely. ;)
Comment 7 Simon Fraser (smfr) 2011-06-12 16:23:40 PDT
Comment on attachment 96845 [details]
Patch

This looks fine.
Comment 8 Jer Noble 2011-06-13 11:02:25 PDT
Committed r88653: <http://trac.webkit.org/changeset/88653>