Bug 142154 – Controls panel should have system blurry background

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Dean Jackson]

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.

[Radar WebKit Bug Importer]

<rdar://problem/20000964>

[Dean Jackson]

Created attachment 247642 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Dean Jackson]

Created attachment 247725 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Tim Horton]

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.

[Dean Jackson]

Test failures don't happen locally. Object fit shouldn't be affected.

[Simon Fraser (smfr)]

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?

[Dean Jackson]

Created attachment 247784 [details]
Patch

[Dean Jackson]

Created attachment 247788 [details]
Patch

[WebKit Commit Bot]

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.

[Dean Jackson]

Created attachment 247790 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Dean Jackson]

Committed r180965: <http://trac.webkit.org/changeset/180965>