Summary: | Controls panel should have system blurry background | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||||||||||||||||||
Component: | Media | Assignee: | Dean Jackson <dino> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, rniwa, simon.fraser, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=240756 | ||||||||||||||||||||||
Attachments: |
|
Description
Dean Jackson
2015-03-01 16:00:50 PST
Created attachment 247642 [details]
Patch
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 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 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 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 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. Created attachment 247725 [details]
Patch
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 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 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. Test failures don't happen locally. Object fit shouldn't be affected. 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? Created attachment 247784 [details]
Patch
Created attachment 247788 [details]
Patch
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.
Created attachment 247790 [details]
Patch
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 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
Committed r180965: <http://trac.webkit.org/changeset/180965> |