Bug 161986 - Add CSS -webkit-appearance property for Apple Pay buttons
Summary: Add CSS -webkit-appearance property for Apple Pay buttons
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anders Carlsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-14 13:52 PDT by Anders Carlsson
Modified: 2016-09-15 12:28 PDT (History)
1 user (show)

See Also:


Attachments
Patch (34.71 KB, patch)
2016-09-14 14:27 PDT, Anders Carlsson
no flags Details | Formatted Diff | Diff
Patch (39.90 KB, patch)
2016-09-14 14:38 PDT, Anders Carlsson
no flags Details | Formatted Diff | Diff
Patch (39.96 KB, patch)
2016-09-14 14:47 PDT, Anders Carlsson
no flags Details | Formatted Diff | Diff
Patch (35.22 KB, patch)
2016-09-14 15:17 PDT, Anders Carlsson
no flags Details | Formatted Diff | Diff
Patch (35.29 KB, patch)
2016-09-14 15:59 PDT, Anders Carlsson
no flags Details | Formatted Diff | Diff
Patch (35.29 KB, patch)
2016-09-14 16:26 PDT, Anders Carlsson
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2016-09-14 13:52:28 PDT
Add CSS -webkit-appearance property for Apple Pay buttons
Comment 1 Anders Carlsson 2016-09-14 14:27:25 PDT
Created attachment 288859 [details]
Patch
Comment 2 Anders Carlsson 2016-09-14 14:38:00 PDT
Created attachment 288861 [details]
Patch
Comment 3 Anders Carlsson 2016-09-14 14:47:39 PDT
Created attachment 288863 [details]
Patch
Comment 4 Anders Carlsson 2016-09-14 15:17:02 PDT
Created attachment 288870 [details]
Patch
Comment 5 Anders Carlsson 2016-09-14 15:59:58 PDT
Created attachment 288879 [details]
Patch
Comment 6 Anders Carlsson 2016-09-14 16:26:41 PDT
Created attachment 288886 [details]
Patch
Comment 7 Dean Jackson 2016-09-14 17:56:40 PDT
Comment on attachment 288886 [details]
Patch

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

If it is possible to put the PDF artwork in Open Source, it would be good to have some ref tests for the combination of 3 styles * 4 types.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:5499
> +    default:
> +        ASSERT_NOT_REACHED();
> +        break;
> +    }

Maybe pick one style between this ^^^^

> Source/WebCore/css/CSSPrimitiveValueMappings.h:5515
> +    default:
> +        break;
> +    }
> +    ASSERT_NOT_REACHED();

... and this ^^^^?

Although maybe this is standard.

Same with the ApplePayButtonType parts.

> Source/WebCore/css/CSSPropertyNames.in:687
> +-apple-pay-button-style [NameForMethods=ApplePayButtonStyle]
> +-apple-pay-button-type [NameForMethods=ApplePayButtonType]

Wouldn't you get these names by default?

> Source/WebCore/rendering/style/RenderStyle.h:1746
> +#if ENABLE(APPLE_PAY)
> +    void setApplePayButtonStyle(ApplePayButtonStyle v) { SET_VAR(rareInheritedData, applePayButtonStyle, static_cast<unsigned>(v)); }
> +    void setApplePayButtonType(ApplePayButtonType v) { SET_VAR(rareInheritedData, applePayButtonType, static_cast<unsigned>(v)); }
> +#endif

I think these should be in rareNonInheritedData, unless you think you'd want to set the style on a parent element and have it inherit down.
Comment 8 Anders Carlsson 2016-09-15 09:58:27 PDT
(In reply to comment #7)
> Comment on attachment 288886 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288886&action=review
> 
> If it is possible to put the PDF artwork in Open Source, it would be good to
> have some ref tests for the combination of 3 styles * 4 types.

I don't think it's possible.

> 
> > Source/WebCore/css/CSSPrimitiveValueMappings.h:5499
> > +    default:
> > +        ASSERT_NOT_REACHED();
> > +        break;
> > +    }
> 
> Maybe pick one style between this ^^^^
> 
> > Source/WebCore/css/CSSPrimitiveValueMappings.h:5515
> > +    default:
> > +        break;
> > +    }
> > +    ASSERT_NOT_REACHED();
> 
> ... and this ^^^^?
> 
> Although maybe this is standard.

Yeah, just matched the existing switch statements.

> 
> Same with the ApplePayButtonType parts.
> 
> > Source/WebCore/css/CSSPropertyNames.in:687
> > +-apple-pay-button-style [NameForMethods=ApplePayButtonStyle]
> > +-apple-pay-button-type [NameForMethods=ApplePayButtonType]
> 
> Wouldn't you get these names by default?

I think so.

> 
> > Source/WebCore/rendering/style/RenderStyle.h:1746
> > +#if ENABLE(APPLE_PAY)
> > +    void setApplePayButtonStyle(ApplePayButtonStyle v) { SET_VAR(rareInheritedData, applePayButtonStyle, static_cast<unsigned>(v)); }
> > +    void setApplePayButtonType(ApplePayButtonType v) { SET_VAR(rareInheritedData, applePayButtonType, static_cast<unsigned>(v)); }
> > +#endif
> 
> I think these should be in rareNonInheritedData, unless you think you'd want
> to set the style on a parent element and have it inherit down.

Yes, you're right.
Comment 9 Anders Carlsson 2016-09-15 10:48:05 PDT
Committed r205980: <http://trac.webkit.org/changeset/205980>
Comment 10 Simon Fraser (smfr) 2016-09-15 12:28:57 PDT
r-, no tests :|