Bug 142154 - Controls panel should have system blurry background
Summary: Controls panel should have system blurry background
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-03-01 16:00 PST by Dean Jackson
Modified: 2015-03-03 16:40 PST (History)
5 users (show)

See Also:


Attachments
Patch (34.98 KB, patch)
2015-03-01 16:36 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-mavericks (537.44 KB, application/zip)
2015-03-01 17:39 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (761.95 KB, application/zip)
2015-03-01 17:44 PST, Build Bot
no flags Details
Patch (37.43 KB, patch)
2015-03-02 17:41 PST, Dean Jackson
simon.fraser: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (716.79 KB, application/zip)
2015-03-02 19:01 PST, Build Bot
no flags Details
Patch (38.14 KB, patch)
2015-03-03 13:12 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (38.15 KB, patch)
2015-03-03 13:38 PST, Dean Jackson
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (38.31 KB, patch)
2015-03-03 13:50 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (848.26 KB, application/zip)
2015-03-03 15:05 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2015-03-01 16:00:50 PST
The media controls on iOS should have a blurry background (like the system default). This means we need to expose the UIBackdropView appearance to a CSS property.
Comment 1 Radar WebKit Bug Importer 2015-03-01 16:01:54 PST
<rdar://problem/20000964>
Comment 2 Dean Jackson 2015-03-01 16:36:01 PST
Created attachment 247642 [details]
Patch
Comment 3 Build Bot 2015-03-01 17:39:32 PST
Comment on attachment 247642 [details]
Patch

Attachment 247642 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5327221405253632

New failing tests:
compositing/tiling/tiled-layer-resize.html
compositing/tiling/empty-to-tiled.html
Comment 4 Build Bot 2015-03-01 17:39:35 PST
Created attachment 247646 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 5 Build Bot 2015-03-01 17:44:34 PST
Comment on attachment 247642 [details]
Patch

Attachment 247642 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5387368932573184

New failing tests:
compositing/masks/become-tiled-mask.html
compositing/tiling/tiled-layer-resize.html
compositing/tiling/empty-to-tiled.html
Comment 6 Build Bot 2015-03-01 17:44:37 PST
Created attachment 247647 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 7 Darin Adler 2015-03-01 21:01:06 PST
Comment on attachment 247642 [details]
Patch

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

> Source/WebCore/css/CSSValueKeywords.in:722
>  media-time-remaining-display
> +media-controls-dark-bar-background
> +media-controls-light-bar-background

Please keep these in alphabetical order.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3153
> +    bool needsTileBackingUsageChanged = true;
> +    if (((layerType == PlatformCALayer::LayerTypeDarkSystemBackdropLayer || layerType == PlatformCALayer::LayerTypeLightSystemBackdropLayer) && existingLayerType != PlatformCALayer::LayerTypeTiledBackingLayer)
> +        || ((existingLayerType == PlatformCALayer::LayerTypeDarkSystemBackdropLayer && existingLayerType == PlatformCALayer::LayerTypeLightSystemBackdropLayer) && layerType != PlatformCALayer::LayerTypeTiledBackingLayer))
> +        needsTileBackingUsageChanged = false;

I think this should be written using a helper function that maps a type into a usage rather than these long boolean expressions. I don’t know exactly how the code would come out, but I think it would be more readable.

> Source/WebKit2/UIProcess/ios/RemoteLayerTreeHostIOS.mm:164
> +@interface WKBackdropView : _UIBackdropView
> +
> +@end

Not sure you need the blank line here.

> Source/WebKit2/UIProcess/ios/RemoteLayerTreeHostIOS.mm:181
> +@end
>  namespace WebKit {

Need a blank line here.
Comment 8 Dean Jackson 2015-03-02 17:41:48 PST
Created attachment 247725 [details]
Patch
Comment 9 Build Bot 2015-03-02 19:01:18 PST
Comment on attachment 247725 [details]
Patch

Attachment 247725 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5376368447586304

New failing tests:
fast/css/object-fit/object-fit-canvas.html
Comment 10 Build Bot 2015-03-02 19:01:26 PST
Created attachment 247734 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 11 Tim Horton 2015-03-03 10:40:20 PST
Comment on attachment 247725 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsLayer.h:435
> +    enum CustomAppearance { NoCustomAppearance, ScrollingOverhang, ScrollingShadow, CustomLightBackdropAppearance, CustomDarkBackdropAppearance };

I don't think these should have "Custom" in their names.
Comment 12 Dean Jackson 2015-03-03 11:13:26 PST
Test failures don't happen locally. Object fit shouldn't be affected.
Comment 13 Simon Fraser (smfr) 2015-03-03 11:43:29 PST
Comment on attachment 247725 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1328
> +    if (!needBackdropLayerType && m_usingBackdropLayerType) {
> +        changeLayerTypeTo(needTiledLayer ? PlatformCALayer::LayerTypeTiledBackingLayer : PlatformCALayer::LayerTypeWebLayer);
> +        m_usingBackdropLayerType = false;
> +    } else if (needBackdropLayerType && !m_usingBackdropLayerType) {
> +        changeLayerTypeTo(layerTypeForCustomBackdropAppearance(customAppearance()));
> +        m_usingBackdropLayerType = true;
> +    } else if (needTiledLayer != m_usingTiledBacking)
> +        changeLayerTypeTo(needTiledLayer ? PlatformCALayer::LayerTypeTiledBackingLayer : PlatformCALayer::LayerTypeWebLayer);

I would much prefer this first computed the new layer type, then just called changeLayerTypeTo() once. I think that's better coding style.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3148
> +static bool changeRequiresTiledBackingUsageUpdate(PlatformCALayer::LayerType oldLayerType, PlatformCALayer::LayerType newLayerType)
> +{
> +    return (oldLayerType == PlatformCALayer::LayerTypeTiledBackingLayer || newLayerType == PlatformCALayer::LayerTypeTiledBackingLayer);
> +}

Not sure this is worth its own function.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:487
> +    unsigned m_usingBackdropLayerType : 1;

Can be a bool : 1

> Source/WebCore/rendering/RenderBox.h:57
> +            || hasTransformRelatedProperty() || hasHiddenBackface() || hasReflection() || hasAppearanceRequiringLayer()
> +            || style().specifiesColumns() || !style().hasAutoZIndex();

I think you could unwrap this.

> Source/WebCore/rendering/RenderBoxModelObject.h:126
> -    virtual bool requiresLayer() const override { return isRoot() || isPositioned() || createsGroup() || hasClipPath() || hasTransformRelatedProperty() || hasHiddenBackface() || hasReflection(); }
> +    virtual bool requiresLayer() const override { return isRoot() || isPositioned() || createsGroup() || hasClipPath() || hasTransformRelatedProperty() || hasHiddenBackface() || hasReflection() || hasAppearanceRequiringLayer(); }

It's a bit sad that you're slowing everyone down with this change.

What about the other requiresLayer() impls?
Comment 14 Dean Jackson 2015-03-03 13:12:38 PST
Created attachment 247784 [details]
Patch
Comment 15 Dean Jackson 2015-03-03 13:38:51 PST
Created attachment 247788 [details]
Patch
Comment 16 WebKit Commit Bot 2015-03-03 13:41:20 PST
Attachment 247788 [details] did not pass style-queue:


ERROR: Source/WebKit2/Platform/spi/ios/UIKitSPI.h:659:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebKit2/Platform/spi/ios/UIKitSPI.h:662:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Dean Jackson 2015-03-03 13:50:17 PST
Created attachment 247790 [details]
Patch
Comment 18 Build Bot 2015-03-03 15:05:22 PST
Comment on attachment 247788 [details]
Patch

Attachment 247788 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5712639690801152

New failing tests:
fast/css/object-fit/object-fit-canvas.html
Comment 19 Build Bot 2015-03-03 15:05:25 PST
Created attachment 247801 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 20 Dean Jackson 2015-03-03 16:40:35 PST
Committed r180965: <http://trac.webkit.org/changeset/180965>