Bug 146123 - Provide a way for web developers to draw a Theme-specific Wireless Playback icon
Summary: Provide a way for web developers to draw a Theme-specific Wireless Playback icon
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: Dean Jackson
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2015-06-18 13:58 PDT by Dean Jackson
Modified: 2015-06-18 20:32 PDT (History)
5 users (show)

See Also:


Attachments
Patch (109.51 KB, patch)
2015-06-18 14:17 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (48.76 KB, patch)
2015-06-18 15:29 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (48.76 KB, patch)
2015-06-18 15:45 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (55.33 KB, patch)
2015-06-18 16:53 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (55.78 KB, patch)
2015-06-18 17:20 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (116.88 KB, patch)
2015-06-18 18:00 PDT, Dean Jackson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2015-06-18 13:58:40 PDT
Provide a way for web developers to draw a Theme-specific Wireless Playback icon
Comment 1 Dean Jackson 2015-06-18 13:59:01 PDT
<rdar://problem/21119287>
Comment 2 Dean Jackson 2015-06-18 14:01:18 PDT
We're going to expose a new <image> type to CSS via the function "named-icon(name)". This will render a system icon, and can be used to get access to platform-specific artwork like Airplay on Cocoa.

e.g. background-image: named-icon(wireless-playback);

Since it is an image generator, a site can adjust it to their theme using normal CSS techniques such as masking.
Comment 3 Dean Jackson 2015-06-18 14:17:43 PDT
Created attachment 255133 [details]
Patch
Comment 4 Dean Jackson 2015-06-18 15:29:08 PDT
Created attachment 255141 [details]
Patch
Comment 5 Dean Jackson 2015-06-18 15:45:13 PDT
Created attachment 255145 [details]
Patch
Comment 6 Dean Jackson 2015-06-18 16:53:37 PDT
Created attachment 255153 [details]
Patch
Comment 7 Dean Jackson 2015-06-18 17:20:13 PDT
Created attachment 255156 [details]
Patch
Comment 8 Simon Fraser (smfr) 2015-06-18 17:24:38 PDT
Comment on attachment 255153 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:8414
> +		314BE3A01B30F6B700141982 /* CSSNamedImageValue.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; lineEnding = 0; path = CSSNamedImageValue.h; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.objcpp; };
> +		314BE3A21B30F6D100141982 /* CSSNamedImageValue.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; lineEnding = 0; path = CSSNamedImageValue.cpp; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.cpp; };
> +		314BE3A41B3103FB00141982 /* NamedImageGeneratedImage.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = NamedImageGeneratedImage.cpp; sourceTree = "<group>"; };
> +		314BE3A51B3103FB00141982 /* NamedImageGeneratedImage.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = NamedImageGeneratedImage.h; sourceTree = "<group>"; };

Please edit out the xcLanguageSpecificationIdentifier.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:12724
> +		BC23E76B0DAE88A9009FDC91 /* CSSImageGeneratorValue.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; lineEnding = 0; path = CSSImageGeneratorValue.cpp; sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.cpp; };

I don't think the xcLanguageSpecificationIdentifier should be there.

> Source/WebCore/css/CSSNamedImageValue.h:39
> +    static Ref<CSSNamedImageValue> create(const String& name) { return adoptRef(*new CSSNamedImageValue(name)); }

I think you should unwrap this.

> Source/WebCore/css/CSSNamedImageValue.h:43
> +    bool isFixedSize() const { return false; }

Do named images have implicit size? We should probably say they do.

> Source/WebCore/platform/Theme.cpp:71
> +    context->scale(FloatSize(rect.size().width() / 100, rect.size().height() / 100));

The beziers below are designed to fit in 100x100?

> Source/WebCore/platform/Theme.cpp:111
> +    Path outline;
> +    outline.moveTo(FloatPoint(59, 58.7));
> +    outline.addBezierCurveTo(FloatPoint(58.1, 58.7), FloatPoint(57.2, 58.4), FloatPoint(56.4, 57.7));
> +    outline.addLineTo(FloatPoint(42, 45.5));
> +    outline.addLineTo(FloatPoint(27.6, 57.8));
> +    outline.addBezierCurveTo(FloatPoint(25.9, 59.2), FloatPoint(23.4, 59), FloatPoint(22, 57.3));
> +    outline.addBezierCurveTo(FloatPoint(20.6, 55.6), FloatPoint(20.8, 53.1), FloatPoint(22.5, 51.7));
> +    outline.addLineTo(FloatPoint(39.5, 37.3));
> +    outline.addBezierCurveTo(FloatPoint(41, 36), FloatPoint(43.2, 36), FloatPoint(44.7, 37.3));
> +    outline.addLineTo(FloatPoint(61.7, 51.7));
> +    outline.addBezierCurveTo(FloatPoint(63.4, 53.1), FloatPoint(63.6, 55.7), FloatPoint(62.2, 57.3));
> +    outline.addBezierCurveTo(FloatPoint(61.3, 58.2), FloatPoint(60.1, 58.7), FloatPoint(59, 58.7));
> +    outline.addLineTo(FloatPoint(59, 58.7));
> +    outline.closeSubpath();
> +
> +    outline.moveTo(FloatPoint(42, 98));
> +    outline.addBezierCurveTo(FloatPoint(39.8, 98), FloatPoint(38, 96.3), FloatPoint(38, 94.2));
> +    outline.addLineTo(FloatPoint(38, 43.6));
> +    outline.addBezierCurveTo(FloatPoint(38, 41.5), FloatPoint(39.8, 39.8), FloatPoint(42, 39.8));
> +    outline.addBezierCurveTo(FloatPoint(44.2, 39.8), FloatPoint(46, 41.5), FloatPoint(46, 43.6));
> +    outline.addLineTo(FloatPoint(46, 94.2));
> +    outline.addBezierCurveTo(FloatPoint(46, 96.3), FloatPoint(44.2, 98), FloatPoint(42, 98));
> +    outline.addLineTo(FloatPoint(42, 98));
> +    outline.closeSubpath();
> +
> +    outline.moveTo(FloatPoint(83.6, 41.6));
> +    outline.addBezierCurveTo(FloatPoint(83.6, 18.6), FloatPoint(65, 0), FloatPoint(42, 0));
> +    outline.addBezierCurveTo(FloatPoint(19, 0), FloatPoint(0.4, 18.6), FloatPoint(0.4, 41.6));
> +    outline.addBezierCurveTo(FloatPoint(0.4, 62.2), FloatPoint(15, 79.2), FloatPoint(35, 82.6));
> +    outline.addLineTo(FloatPoint(35, 74.5));
> +    outline.addBezierCurveTo(FloatPoint(20, 71.2), FloatPoint(8.4, 57.7), FloatPoint(8.4, 41.6));
> +    outline.addBezierCurveTo(FloatPoint(8.4, 23.1), FloatPoint(23.5, 8), FloatPoint(42, 8));
> +    outline.addBezierCurveTo(FloatPoint(60.5, 8), FloatPoint(75.5, 23.1), FloatPoint(75.5, 41.6));
> +    outline.addBezierCurveTo(FloatPoint(75.6, 57.7), FloatPoint(64, 71.2), FloatPoint(49, 74.5));
> +    outline.addLineTo(FloatPoint(49, 82.6));
> +    outline.addBezierCurveTo(FloatPoint(69, 79.3), FloatPoint(83.6, 62.2), FloatPoint(83.6, 41.6));
> +    outline.addLineTo(FloatPoint(83.6, 41.6));
> +    outline.closeSubpath();

Icky. Would be nicer to put a .svg in the bundle.

> Source/WebCore/platform/cocoa/ThemeCocoa.cpp:63
> +    FloatSize wirelessPlaybackSrcSize(32, 24.016);

24.016, really?

> Source/WebCore/platform/graphics/NamedImageGeneratedImage.cpp:57
> +    std::unique_ptr<ImageBuffer> imageBuffer = ImageBuffer::create(size(), 1, ColorSpaceDeviceRGB, context->isAcceleratedContext() ? Accelerated : Unaccelerated);

Doesn't createCompatibleBuffer do the right thing?
Comment 9 Dean Jackson 2015-06-18 17:28:37 PDT
Comment on attachment 255153 [details]
Patch

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

>> Source/WebCore/css/CSSNamedImageValue.h:43
>> +    bool isFixedSize() const { return false; }
> 
> Do named images have implicit size? We should probably say they do.

No. They draw into whatever region they have been given by CSS. At least for now :)

>> Source/WebCore/platform/Theme.cpp:71
>> +    context->scale(FloatSize(rect.size().width() / 100, rect.size().height() / 100));
> 
> The beziers below are designed to fit in 100x100?

Yes.

>> Source/WebCore/platform/Theme.cpp:111
>> +    outline.closeSubpath();
> 
> Icky. Would be nicer to put a .svg in the bundle.

Then we'd have to load from the bundle, parse the SVG, wait for the Image to be ready, get a RenderImage and paint it. This is probably what we'll end up doing, but for now I think it is ok to hardcode this single outline.

>> Source/WebCore/platform/cocoa/ThemeCocoa.cpp:63
>> +    FloatSize wirelessPlaybackSrcSize(32, 24.016);
> 
> 24.016, really?

That's the artwork we were given. See below for the extent of the lines.

>> Source/WebCore/platform/graphics/NamedImageGeneratedImage.cpp:57
>> +    std::unique_ptr<ImageBuffer> imageBuffer = ImageBuffer::create(size(), 1, ColorSpaceDeviceRGB, context->isAcceleratedContext() ? Accelerated : Unaccelerated);
> 
> Doesn't createCompatibleBuffer do the right thing?

Maybe. I'll try. I copied this from crossfades.
Comment 10 Dean Jackson 2015-06-18 18:00:16 PDT
Created attachment 255158 [details]
Patch
Comment 11 Dean Jackson 2015-06-18 18:14:52 PDT
Committed r185731: <http://trac.webkit.org/changeset/185731>
Comment 12 Joseph Pecoraro 2015-06-18 20:32:30 PDT
Comment on attachment 255158 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:9157
> +        || equalIgnoringCase(value.function->name, "-webkit-named-image(");

Should we add this to Web Inspector's autocompletion?