Bug 217802 - Unify CSS property enablement (-apple-color-filter should not exist on CSSStyleDeclaration by default)
Summary: Unify CSS property enablement (-apple-color-filter should not exist on CSSSty...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Sneddon [:gsnedders]
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-15 20:01 PDT by Sam Sneddon [:gsnedders]
Modified: 2022-07-01 23:57 PDT (History)
21 users (show)

See Also:


Attachments
Patch (20.61 KB, patch)
2021-04-29 17:35 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (39.56 KB, patch)
2021-04-30 11:40 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
List of calls to isCSSPropertyEnabledBySettings/isInternalCSSProperty etc. (3.84 KB, text/plain)
2021-05-04 08:37 PDT, Sam Sneddon [:gsnedders]
no flags Details
Patch (48.86 KB, patch)
2021-05-05 04:50 PDT, Sam Sneddon [:gsnedders]
gsnedders: review-
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (78.96 KB, patch)
2021-05-05 14:19 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (93.39 KB, patch)
2021-12-21 21:43 PST, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (202.04 KB, patch)
2022-03-30 08:50 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (218.32 KB, patch)
2022-04-05 10:40 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (236.65 KB, patch)
2022-04-07 13:40 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (236.59 KB, patch)
2022-04-07 15:40 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (239.68 KB, patch)
2022-05-13 16:53 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (239.67 KB, patch)
2022-05-13 18:30 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (267.67 KB, patch)
2022-05-18 11:54 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (173.01 KB, patch)
2022-05-20 12:33 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (170.71 KB, patch)
2022-05-24 11:18 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (170.71 KB, patch)
2022-05-25 09:42 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (170.46 KB, patch)
2022-05-26 05:55 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (171.65 KB, patch)
2022-05-27 10:16 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (179.87 KB, patch)
2022-06-29 16:34 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (170.56 KB, patch)
2022-06-30 09:22 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (181.44 KB, patch)
2022-07-01 08:13 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
I should debug why git-webkit isn't working for me again, but in the meantime I think this addresses all the above feedback. So, uh, r?. (190.83 KB, patch)
2022-07-01 13:01 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (374.79 KB, patch)
2022-07-01 13:24 PDT, Sam Sneddon [:gsnedders]
koivisto: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Sneddon [:gsnedders] 2020-10-15 20:01:05 PDT
per smfr in https://bugs.webkit.org/show_bug.cgi?id=217626#c1
Comment 1 Radar WebKit Bug Importer 2020-10-22 20:02:14 PDT
<rdar://problem/70600812>
Comment 2 Sam Sneddon [:gsnedders] 2021-01-19 11:29:38 PST
Further investigation shows that this is only the attribute on CSSStyleDeclaration which is exposed; the CSS parser special-cases it.

Looking at https://wpt.fyi/results/css/css-conditional/js/CSS-supports-CSSStyleDeclaration.html?label=master&label=experimental&aligned&q=safari%3Afail shows there's a few more: overscroll-behavior{,-x,-y} and scroll-behavior. (The other failures are other issues)
Comment 3 Sam Sneddon [:gsnedders] 2021-04-29 12:09:10 PDT
Bug 217623 fixed most of these, but there's a few still failing.
Comment 4 Sam Sneddon [:gsnedders] 2021-04-29 13:02:59 PDT
So properties that look sus now:

-apple-color-filter
-webkit-text-size-adjust

There's also the failures due to Bug 224930.
Comment 5 Sam Sneddon [:gsnedders] 2021-04-29 17:35:15 PDT
Created attachment 427391 [details]
Patch

By no means done, but maybe getting somewhere useful?
Comment 6 Sam Sneddon [:gsnedders] 2021-04-30 11:40:21 PDT
Created attachment 427435 [details]
Patch

Still not quite there, but would be interested in some initial review at least. (Things can be better, for sure!)
Comment 7 Darin Adler 2021-04-30 12:00:26 PDT
Comment on attachment 427435 [details]
Patch

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

This looks like a great approach; brief look at the patch doesn’t make it clear to me how many different bits there are in CSSPropertySettings.

> Source/WebCore/css/makeprop.pl:556
> +foreach my $name (uniq(values %settingsFlags)) {

Any sorting needed for predictable order?

> Source/WebCore/css/makeprop.pl:715
> +    inline CSSPropertySettings() {};

The word "inline" here isn’t needed. The semicolon isn’t needed. But I suggest this alternative form:

    CSSPropertySetting() = default;

> Source/WebCore/css/makeprop.pl:716
> +    CSSPropertySettings(const Settings& settings);

WebKit coding style says leave out the argument name here. I also suggest "explicit" for this.

> Source/WebCore/css/makeprop.pl:717
> +    bool isPropertyEnabled(const CSSPropertyID id) const;

No need for the "const" here or the "id". Just:

    bool isPropertyEnabled(CSSPropertyID) const;

> Source/WebCore/css/makeprop.pl:725
> +void add(Hasher&, const CSSPropertySettings&);
> +
> +
>  } // namespace WebCore

Just one blank line.
Comment 8 Sam Sneddon [:gsnedders] 2021-05-04 08:37:05 PDT
Created attachment 427671 [details]
List of calls to isCSSPropertyEnabledBySettings/isInternalCSSProperty etc.
Comment 9 Sam Sneddon [:gsnedders] 2021-05-04 08:46:31 PDT
Looking through this new attachment makes it very clear that the vast majority of the time we're calling things like:

> if (!isCSSPropertyEnabledBySettings(propertyID, &m_element->document().settings()) || !isEnabledCSSProperty(propertyID) || isInternalCSSProperty(propertyID))

This is… rather silly. It almost makes me wonder how many of the other cases are actually wrong.

I wonder if we should just have a single function in CSSPropertyNames.h called something like isCSSPropertyExposed(CSSPropertyID, Settings&)?
Comment 10 Darin Adler 2021-05-04 19:52:42 PDT
(In reply to Sam Sneddon [:gsnedders] from comment #9)
> I wonder if we should just have a single function in CSSPropertyNames.h
> called something like isCSSPropertyExposed(CSSPropertyID, Settings&)?

We should add this, then see how many call sites need something more specific. We can remove the other functions once we confirm they aren’t needed.
Comment 11 Sam Sneddon [:gsnedders] 2021-05-05 04:50:56 PDT
Created attachment 427753 [details]
Patch
Comment 12 Sam Sneddon [:gsnedders] 2021-05-05 05:03:45 PDT
Comment on attachment 427753 [details]
Patch

It turns out uploading the wrong set of commits isn't helpful.
Comment 13 Sam Sneddon [:gsnedders] 2021-05-05 14:19:35 PDT
Created attachment 427807 [details]
Patch
Comment 14 Darin Adler 2021-05-05 16:39:45 PDT
Comment on attachment 427807 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1472
> +        CSSPropertyID propertyID = computedPropertyIDs[i];

I prefer auto here.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1475
> +        auto name = getPropertyName(propertyID);

Not sure we need to stick this into a local; OK, I guess, but could also just call it on the append line like we did before.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1478
> +            // FIXME: ideally we'd assert in this case, but currently it's hit too often

WebKit coding style says write this like a sentence with a capitalized first word and a full stop at the end (which I call a period because educated in U.S.).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3997
> +        /* Internal properties should be handled by isCSSPropertyExposed above */

WebKit coding style encourages use of C++ style // comments, even if code nearby was done in a different style (not sure why it was, maybe imported from Chromium).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4223
> +        auto propertyID = set[i];

While this probably how I would write it, there are others who discourage me from caching simply computed values like this in local variables.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4225
> +            list.append(CSSProperty(propertyID, WTFMove(value), false));

After doing reserveInitialCapacity, can and should use uncheckedAppend instead of append for a little tighter code. Not sure why the old code was not doing that.

> Source/WebCore/css/CSSStyleDeclaration.cpp:203
> -    if (!isEnabledCSSProperty(id) || !isCSSPropertyEnabledBySettings(id, settings))
> +    if (!isCSSPropertyExposed(id, settings))

So presumably this is a behavior change, and for the better, by handling more cases. We should spend a moment and reflect on how we could detect that with tests.

> Source/WebCore/css/DOMCSSNamespace.cpp:64
> -    if (parserContext.isPropertyRuntimeDisabled(propertyID))
> +    if (!isCSSPropertyExposed(propertyID, &document.settings()))

Ditto.

> Source/WebCore/css/StyleProperties.cpp:863
> +    if (!isCSSPropertyExposed(propertyID, &parserContext.propertySettings) && !isInternalCSSProperty(propertyID)) {

I sort of expected a single helper function call here, not two. What makes this different enough from the other call sites that it needs an explicit isInternalCSSProperty check and they do not?

> Source/WebCore/css/makeprop.pl:337
> +static bool isEnabledCSSProperty(const CSSPropertyID);
> +static bool isCSSPropertyEnabledBySettings(const CSSPropertyID, const Settings*);
> +static bool isCSSPropertyEnabledByCSSPropertySettings(const CSSPropertyID, const CSSPropertySettings*);

These should all say "CSSPropertyID", not "const CSSPropertyID".

But also, why do we need to declare them at the top of the file? Why not just define them?

> Source/WebCore/css/makeprop.pl:404
> +static bool isEnabledCSSProperty(const CSSPropertyID id)

Ditto.

> Source/WebCore/css/makeprop.pl:419
> +static bool isEnabledCSSProperty(const CSSPropertyID)

Ditto.

> Source/WebCore/css/makeprop.pl:428
> +static bool isCSSPropertyEnabledBySettings(const CSSPropertyID id, const Settings* settings)

Ditto.

> Source/WebCore/css/makeprop.pl:448
> +static bool isCSSPropertyEnabledByCSSPropertySettings(const CSSPropertyID id, const CSSPropertySettings* settings)

Ditto.

> Source/WebCore/css/makeprop.pl:465
> +    return true;

This isn’t needed, because we have the default above.

> Source/WebCore/css/makeprop.pl:468
> +bool isCSSPropertyExposed(const CSSPropertyID id, const Settings* settings)

Ditto, no const CSSPropertyID.

> Source/WebCore/css/makeprop.pl:473
> +bool isCSSPropertyExposed(const CSSPropertyID id, const CSSPropertySettings* settings)

Ditto.

> Source/WebCore/css/makeprop.pl:732
>  bool isInternalCSSProperty(const CSSPropertyID);
> -bool isEnabledCSSProperty(const CSSPropertyID);
> -bool isCSSPropertyEnabledBySettings(const CSSPropertyID, const Settings* = nullptr);
> +bool isCSSPropertyExposed(const CSSPropertyID, const Settings*);
> +bool isCSSPropertyExposed(const CSSPropertyID, const CSSPropertySettings*);

Same here, just CSSPropertyID, no const.

> Source/WebCore/css/makeprop.pl:735
>  const WTF::AtomString& getPropertyNameAtomString(CSSPropertyID id);
>  WTF::String getPropertyNameString(CSSPropertyID id);

Should take these "id" out of here.

> Source/WebCore/css/parser/CSSParserContext.cpp:87
> +    , propertySettings { CSSPropertySettings(document.settings()) }

Could use braces instead of parentheses here if you like.

    , propertySettings { CSSPropertySettings { document.settings() } }

> Source/WebCore/css/parser/CSSParserImpl.cpp:813
> -    if (m_context.isPropertyRuntimeDisabled(propertyID))
> +    if (!isCSSPropertyExposed(propertyID, &m_context.propertySettings))

So presumably this is a behavior change, and for the better, by handling more cases. We should spend a moment and reflect on how we could detect that with tests.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:114
>          auto propertyID = static_cast<CSSPropertyID>(hashTableEntry->id);
> -        // FIXME: Should take account for flags in settings().
> -        if (isEnabledCSSProperty(propertyID))
> -            return propertyID;
> +        return propertyID;

I do not think we need a local variable here or braces.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:204
> -    if (!isEnabledCSSProperty(property))
> +    if (!isCSSPropertyExposed(property, &m_context.propertySettings) && !isInternalCSSProperty(property)) {

So presumably this is a behavior change, and for the better, by handling more cases. We should spend a moment and reflect on how we could detect that with tests.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:278
> +    if (!isCSSPropertyExposed(property, &context.propertySettings)) {
> +        ASSERT_NOT_REACHED();
> +        return nullptr;
> +    }

Is it important to add the check here? All these new places we are checking ... are they detectable? Is the benefit substantial enough to justify additional runtime overhead.

Maybe just an assertion instead of a always-compiled check to back the assertion. I’m concerned about adding all these additional checks just to be assertions.
Comment 15 Sam Sneddon [:gsnedders] 2021-12-21 21:43:38 PST
Created attachment 447781 [details]
Patch
Comment 16 Sam Sneddon [:gsnedders] 2021-12-21 21:47:29 PST
Not putting this up for review quite yet; could still do with slightly better testing (though this is gonna run into CSSOM being underdefined…).
Comment 17 Devin Rousso 2022-02-11 11:49:43 PST
Comment on attachment 447781 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorCSSAgent.h:178
> +    const RefPtr<Settings> m_settings;

Rather than hold the `Settings`, could we just have a `Page& m_inspectedPage` instead?  This is what many of the other `Inspector*Agent` do so it would be nice to match that pattern :)

> LayoutTests/inspector/css/css-property.html:49
> +                    case "background-repeat-y":

Is this an actual legit property now?  If not, can you explain why this was changed?
Comment 18 Sam Sneddon [:gsnedders] 2022-03-30 08:50:59 PDT
Created attachment 456127 [details]
Patch
Comment 19 EWS Watchlist 2022-03-30 08:52:28 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 20 Tim Nguyen (:ntim) 2022-03-30 10:34:10 PDT
Comment on attachment 456127 [details]
Patch

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

> Source/WebCore/ChangeLog:87
> +2021-04-29  Sam Sneddon  <gsnedders@apple.com>
> +
> +        -apple-color-filter should not exist on CSSStyleDeclaration by default
> +        https://bugs.webkit.org/show_bug.cgi?id=217802
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Double changelog
Comment 21 Darin Adler 2022-03-30 14:45:55 PDT
Comment on attachment 456127 [details]
Patch

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

Looks like a really good approach; tests not passing yet though

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4333
>          auto results = copyToVector(inheritedCustomProperties.keys());
> -        return results.at(i - numComputedPropertyIDs);
> +        return results.at(i - m_activeComputedPropertyIDs.size());

Not new to this patch: This seems so inefficient; there must be some way to get the nth without building a vector. Also, is it really OK to have the order depend on hash table ordering? Seems like we’d want things to be in a predictable order, not pseudo-random.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4338
>      auto results = copyToVector(nonInheritedCustomProperties.keys());
> -    return results.at(i - inheritedCustomProperties.size() - numComputedPropertyIDs);
> +    return results.at(i - inheritedCustomProperties.size() - m_activeComputedPropertyIDs.size());

Ditto.
Comment 22 Darin Adler 2022-03-30 14:47:03 PDT
Wow I did not remember the last time I reviewed this, 10 months ago.

Maybe we should break this down into a set of smaller changes so we can make some progress on it. Would still like it to end up just like this.
Comment 23 Sam Sneddon [:gsnedders] 2022-04-04 07:12:03 PDT
(In reply to Darin Adler from comment #22)
> Wow I did not remember the last time I reviewed this, 10 months ago.
> 
> Maybe we should break this down into a set of smaller changes so we can make
> some progress on it. Would still like it to end up just like this.

I basically just need to make sure the per-platform expectations are right, I think otherwise we're pretty much good to go, modulo any further review comments, which I'd been hoping to get done before I was out at the end of last week. This week, hopefully!
Comment 24 Sam Sneddon [:gsnedders] 2022-04-04 10:43:00 PDT
Comment on attachment 447781 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorCSSAgent.h:178
>> +    const RefPtr<Settings> m_settings;
> 
> Rather than hold the `Settings`, could we just have a `Page& m_inspectedPage` instead?  This is what many of the other `Inspector*Agent` do so it would be nice to match that pattern :)

👍

>> LayoutTests/inspector/css/css-property.html:49
>> +                    case "background-repeat-y":
> 
> Is this an actual legit property now?  If not, can you explain why this was changed?

It's not, it's the opposite: it's no longer accidentally allowed in some places, as it's an internal property. The old results are clearly nonsense: it should always be the same as the equally invalid background-repeat-x and background-repeat-invalid.
Comment 25 Sam Sneddon [:gsnedders] 2022-04-05 10:40:28 PDT
Created attachment 456722 [details]
Patch

I'm hoping I now have all the per-platform expectations accurate and this can land, but EWS alone will say.
Comment 26 Sam Sneddon [:gsnedders] 2022-04-05 14:26:19 PDT
Unfortunately, still regressed:

imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/computed/computed.tentative.html
imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/computed/iterable.tentative.html
Comment 27 Sam Sneddon [:gsnedders] 2022-04-07 13:40:50 PDT
Created attachment 456964 [details]
Patch
Comment 28 Darin Adler 2022-04-07 14:11:36 PDT
Comment on attachment 456964 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1693
> +    for (auto & propertyID : computedPropertyIDs) {

Formatting mistake. This should be "auto&" with no space in WebKit coding style.

Is computedPropertyIDs ordered? Seems this ordering may be web-visible?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1694
> +        if (isCSSPropertyExposed(propertyID, &m_element->document().settings()))

Could hoist the m_element->document().settings() out of the loop to avoid chasing the same pointer repeatedly.

Is there some faster way to do this across the properties? I imagine there are some repeated operations we do when calling isCSSPropertyExposed over and over again.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1699
> +    m_activeComputedPropertyIDs = WTFMove(active);

Given this is never changed after being initially created, we could save some memory by using something like a FixedVector instead of a Vector. Could easily build entirely on the stack and then move to the FixedVector, so it would not mean allocating memory twice. But I suppose the real question is how adding this affects speed and memory use.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:352
> +    return ::WebCore::isCSSPropertyExposed(propertyID, &parserContext.propertySettings)

We’d normally write "WebCore::" not "::WebCore::" even though you could do either.

> Source/WebCore/css/parser/CSSParserContext.cpp:170
> +        | (unsigned long long)context.mode                  << 21; // This is multiple bits, so keep it last.

The typecast should be uint64_t since that’s the type we use above for the whole thing.

By the way, do we really need 64 bits? Is 12 bits not enough for context.mode? Because, if it is, we can use uint32_t instead of uint64_t for "bits" and that’s going to be more efficient.

> Source/WebCore/css/parser/CSSParserContext.h:39
> +class Settings;

I don’t understand this change. I don’t see any new uses of Settings below in changes to this header file, although I see a new use of CSSPropertySettings.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:123
>      if (auto hashTableEntry = findProperty(buffer, length)) {
> -        auto propertyID = static_cast<CSSPropertyID>(hashTableEntry->id);
> -        // FIXME: Should take account for flags in settings().
> -        if (isEnabledCSSProperty(propertyID))
> -            return propertyID;
> +        return static_cast<CSSPropertyID>(hashTableEntry->id);
>      }

WebKit coding style says no braces for a one line if body like this one.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4945
> +    ASSERT(context.propertySettings.cssCounterStyleAtRulesEnabled && isCSSPropertyExposed(propId, &context.propertySettings));

I typically suggest two separate ASSERT rather than an ASSERT with && in it, in part because it can make it clearer which of the two failed.

> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:57
> +    Vector<CSSPropertyID> active;
> +    active.reserveInitialCapacity(numComputedPropertyIDs);
> +    for (auto & propertyID : computedPropertyIDs) {
> +        if (isCSSPropertyExposed(propertyID, &element.document().settings()))
> +            active.uncheckedAppend(propertyID);
> +    }
> +    active.shrinkToFit();
> +
> +    m_activeComputedPropertyIDs = WTFMove(active);

Should share this vector-building code with CSSComputedStyleDeclaration::CSSComputedStyleDeclaration rather than writing it out twice.

> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:127
> +    for (auto & propertyID : m_activeComputedPropertyIDs) {

Given that property IDs are simple scalars, I suggest just "auto" here instead of "auto&". (Also WebKit coding style says "auto&" without the space.)

> Source/WebCore/inspector/agents/InspectorCSSAgent.h:66
> +    InspectorCSSAgent(PageAgentContext&);

This line of code really needs "explicit" (not new).
Comment 29 Sam Sneddon [:gsnedders] 2022-04-07 15:40:19 PDT
Created attachment 456977 [details]
Patch
Comment 30 Sam Sneddon [:gsnedders] 2022-04-07 15:47:30 PDT
(This new patch only fixes the merge conflict, doesn't actually address anything else, such as from Darin's review above. It should find any new platform-specific failures on EWS at least…)
Comment 31 Darin Adler 2022-04-07 15:55:37 PDT
Comment on attachment 456964 [details]
Patch

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

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1693
>> +    for (auto & propertyID : computedPropertyIDs) {
> 
> Formatting mistake. This should be "auto&" with no space in WebKit coding style.
> 
> Is computedPropertyIDs ordered? Seems this ordering may be web-visible?

Oh, actually just auto, realized this is a scalar.
Comment 32 Sam Sneddon [:gsnedders] 2022-04-14 09:43:11 PDT
Comment on attachment 456964 [details]
Patch

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

>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1693
>>> +    for (auto & propertyID : computedPropertyIDs) {
>> 
>> Formatting mistake. This should be "auto&" with no space in WebKit coding style.
>> 
>> Is computedPropertyIDs ordered? Seems this ordering may be web-visible?
> 
> Oh, actually just auto, realized this is a scalar.

It's ordered (alphabetically, with prefixed properties after unprefixed ones). Is there any concern about the ordering here? It should be preserved throughout, I think?

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1694
>> +        if (isCSSPropertyExposed(propertyID, &m_element->document().settings()))
> 
> Could hoist the m_element->document().settings() out of the loop to avoid chasing the same pointer repeatedly.
> 
> Is there some faster way to do this across the properties? I imagine there are some repeated operations we do when calling isCSSPropertyExposed over and over again.

After a small amount of inlining, it's just a switch statement with things like:

    case CSSPropertyID::CSSPropertyRotate:
        return settings->cssIndividualTransformPropertiesEnabled;

It's not really clear there's anything we can do better than calling it repeatedly.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1699
>> +    m_activeComputedPropertyIDs = WTFMove(active);
> 
> Given this is never changed after being initially created, we could save some memory by using something like a FixedVector instead of a Vector. Could easily build entirely on the stack and then move to the FixedVector, so it would not mean allocating memory twice. But I suppose the real question is how adding this affects speed and memory use.

Perhaps a silly question: what's the easiest way to stack-allocate something to build it before moving it into a FixedVector?

>> Source/WebCore/css/parser/CSSParserContext.h:39
>> +class Settings;
> 
> I don’t understand this change. I don’t see any new uses of Settings below in changes to this header file, although I see a new use of CSSPropertySettings.

This'll be me failing to remove it from earlier iterations of the patch!

>> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:57
>> +    m_activeComputedPropertyIDs = WTFMove(active);
> 
> Should share this vector-building code with CSSComputedStyleDeclaration::CSSComputedStyleDeclaration rather than writing it out twice.

I couldn't figure out where I ought to put this; any suggestions? I duplicated it mostly to reach a point where I could get tests passing!
Comment 33 Darin Adler 2022-04-14 10:01:53 PDT
Comment on attachment 456964 [details]
Patch

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

>>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1693
>>>> +    for (auto & propertyID : computedPropertyIDs) {
>>> 
>>> Formatting mistake. This should be "auto&" with no space in WebKit coding style.
>>> 
>>> Is computedPropertyIDs ordered? Seems this ordering may be web-visible?
>> 
>> Oh, actually just auto, realized this is a scalar.
> 
> It's ordered (alphabetically, with prefixed properties after unprefixed ones). Is there any concern about the ordering here? It should be preserved throughout, I think?

That’s great.

Might possibly rename computedPropertyIDs to express it’s ordering some day. Sounds more like a set than the "order properties should appear in web API".

>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1699
>>> +    m_activeComputedPropertyIDs = WTFMove(active);
>> 
>> Given this is never changed after being initially created, we could save some memory by using something like a FixedVector instead of a Vector. Could easily build entirely on the stack and then move to the FixedVector, so it would not mean allocating memory twice. But I suppose the real question is how adding this affects speed and memory use.
> 
> Perhaps a silly question: what's the easiest way to stack-allocate something to build it before moving it into a FixedVector?

The idiom I would have used would be an array, since these are scalars: either std::array or just a plain old C array depending on whether std::array helped make any of the code more elegant. That does require keeping count of the size in a separate variable. If we find that idiom too ugly we could create a tiny class template to make it more elegant, local to this file, or eventually moved to WTF if it’s a recurring pattern.

Could also use a Vector with inline capacity: that would still use only stack memory since we’d never get past the inline capacity, but would compile to a bit more code than we might want, with some extra indirection and branches deciding not to do heap allocation. So I’d use the array.

Given its name, it’s kind of non-obvious that FixedVector always implicitly uses the heap to optimize moving; might want to ask Yusuke about that at some point.
Comment 34 Darin Adler 2022-04-14 10:25:50 PDT
Comment on attachment 456964 [details]
Patch

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

>>> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:57
>>> +    m_activeComputedPropertyIDs = WTFMove(active);
>> 
>> Should share this vector-building code with CSSComputedStyleDeclaration::CSSComputedStyleDeclaration rather than writing it out twice.
> 
> I couldn't figure out where I ought to put this; any suggestions? I duplicated it mostly to reach a point where I could get tests passing!

I typically choose mechanically rather than thinking too deeply. One possibility is to put it in the same header as the isCSSPropertyExposed function. Or it can go into ComputedStylePropertyMapReadOnly.h or CSSComputedStyleDeclaration.h. I think the function we want is:

    Vector<CSSPropertyID> activeComputedCSSPropertyIDs(Element&);
Comment 35 Sam Sneddon [:gsnedders] 2022-05-13 16:53:11 PDT
Created attachment 459330 [details]
Patch
Comment 36 Sam Sneddon [:gsnedders] 2022-05-13 18:30:06 PDT
Created attachment 459336 [details]
Patch
Comment 37 Sam Sneddon [:gsnedders] 2022-05-18 11:54:02 PDT
Created attachment 459550 [details]
Patch
Comment 38 Sam Sneddon [:gsnedders] 2022-05-20 12:33:30 PDT
Created attachment 459624 [details]
Patch
Comment 39 Sam Sneddon [:gsnedders] 2022-05-20 12:47:03 PDT
reviewers: plz re-review; might need to fix up one or two of the platform-specific expectation files, but don’t foresee any other changes now
Comment 40 Tim Nguyen (:ntim) 2022-05-21 11:09:43 PDT
Comment on attachment 459624 [details]
Patch

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

> Source/WebCore/css/parser/CSSSelectorParser.cpp:652
> +            if (!RuntimeEnabledFeatures::sharedFeatures().attachmentElementEnabled() && selector->pseudoClassType() == CSSSelector::PseudoClassHasAttachment)

I'm pretty sure we don't want people to use this singleton anymore.

> Source/WebCore/css/typedom/ComputedStylePropertyMapReadOnly.cpp:50
> +    auto settings = &element.document().settings();
> +    m_activeComputedPropertyIDs = getExposedCSSProperties(settings);

nit: could be one line.

> LayoutTests/imported/w3c/web-platform-tests/css/css-conditional/js/CSS-supports-CSSStyleDeclaration-expected.txt:895
> +FAIL font-display: _camel_cased_attribute v. CSS.supports assert_equals: expected true but got false
> +FAIL font-display: _dashed_attribute v. CSS.supports assert_equals: expected true but got false

Sorry if I missed this from earlier comments, but why is this now a failure?

> LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-cssfontrule.tentative.html:1
> +<!doctype html>

I don't particularly mind, but landing these new WPT in a different patch might make this easier to review/understand for others, and they also wouldn't get reverted with the full patch in case of regression.

> LayoutTests/platform/ios-wk2/editing/pasteboard/emacs-ctrl-k-y-001-expected.txt:16
> +          RenderBR {BR} at (65,1) size 1x28

Why is this new test result happening?
Comment 41 Sam Sneddon [:gsnedders] 2022-05-24 08:01:28 PDT
Comment on attachment 459624 [details]
Patch

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

>> Source/WebCore/css/parser/CSSSelectorParser.cpp:652
>> +            if (!RuntimeEnabledFeatures::sharedFeatures().attachmentElementEnabled() && selector->pseudoClassType() == CSSSelector::PseudoClassHasAttachment)
> 
> I'm pretty sure we don't want people to use this singleton anymore.

We don't want people to use runtime features in general; that doesn't mean migrating everything away from it instantaneously. In this case, all that's happened is it no longer gets read into CSSParserContext for this single usage.

>> LayoutTests/imported/w3c/web-platform-tests/css/css-conditional/js/CSS-supports-CSSStyleDeclaration-expected.txt:895
>> +FAIL font-display: _dashed_attribute v. CSS.supports assert_equals: expected true but got false
> 
> Sorry if I missed this from earlier comments, but why is this now a failure?

font-display is only a descriptor, and we still expose it on CSSStyleDeclaration, because fixing this up per CSS WG resolution (i.e., having a Firefox-like split of the actual properties onto another interface, what Firefox/DOM Style L2 calls CSS2Properties, but also adding a CSSFontRuleDeclaration with font-display) is a much larger change; this patch however stops it from showing up via CSS.supports (mostly because it was the only reasonable way to stop asserts from being easily hit from there!).

>> LayoutTests/platform/ios-wk2/editing/pasteboard/emacs-ctrl-k-y-001-expected.txt:16
>> +          RenderBR {BR} at (65,1) size 1x28
> 
> Why is this new test result happening?

Oh, this is unneeded because it's marked [ Failure ].
Comment 42 Sam Sneddon [:gsnedders] 2022-05-24 11:18:17 PDT
Created attachment 459729 [details]
Patch
Comment 43 Tim Nguyen (:ntim) 2022-05-24 15:05:40 PDT
I think it would be helpful if you started some perf runs to see if there are any regressions. I don't expect any, but it's not unlikely for a change like this.
Comment 44 Sam Sneddon [:gsnedders] 2022-05-25 09:42:44 PDT
Created attachment 459759 [details]
Patch
Comment 45 Sam Sneddon [:gsnedders] 2022-05-25 09:56:08 PDT
(In reply to Tim Nguyen (:ntim) from comment #43)
> I think it would be helpful if you started some perf runs to see if there
> are any regressions. I don't expect any, but it's not unlikely for a change
> like this.

I've done so internally, but on the whole I'd rather land this without waiting for that, and avoid having to spend yet more time having to rebase results (especially for platforms I don't have locally!).
Comment 46 Sam Sneddon [:gsnedders] 2022-05-26 05:55:39 PDT
Created attachment 459784 [details]
Patch
Comment 47 Darin Adler 2022-05-26 09:54:09 PDT
(In reply to Sam Sneddon [:gsnedders] from comment #45)
> (In reply to Tim Nguyen (:ntim) from comment #43)
> > I think it would be helpful if you started some perf runs to see if there
> > are any regressions. I don't expect any, but it's not unlikely for a change
> > like this.
> 
> I've done so internally, but on the whole I'd rather land this without
> waiting for that, and avoid having to spend yet more time having to rebase
> results (especially for platforms I don't have locally!).

I empathize, but I think this is backwards. We should get those test results *before* this is merged even if we risk the frustration of slowing this down further. Should take <24 hours with the A/B bots that are available to Apple engineers once you have a patch that builds on iOS and macOS; get help from someone who knows how if you haven’t done it before. I recently did it with a patch of mine for the first time (but sadly didn’t finish getting it to build on all platforms before I left on vacation, so likely won’t be able to land it until I get back to my computer at home).
Comment 48 Sam Sneddon [:gsnedders] 2022-05-27 10:16:45 PDT
Created attachment 459818 [details]
Patch
Comment 49 Sam Sneddon [:gsnedders] 2022-05-27 10:18:42 PDT
For the record: the perf runs showed no regression anywhere; the patch just uploaded will still fail fast/scrolling/ios/overflow-scrolling-touch-disabled-stacking.html on iOS (needs investigating, etc.), but everything else should be good now, and this at least uploads what I currently have before I go on vacation.
Comment 50 Tim Nguyen (:ntim) 2022-05-27 16:34:51 PDT
(In reply to Sam Sneddon [:gsnedders] from comment #49)
> For the record: the perf runs showed no regression anywhere;
👍

> the patch just uploaded will still fail
> fast/scrolling/ios/overflow-scrolling-touch-disabled-stacking.html on iOS
> (needs investigating, etc.), but everything else should be good now, and
> this at least uploads what I currently have before I go on vacation.

I think it's the -webkit-overflow-scrolling CSS property which is causing this. Seems like it's enabled/exposed by default on iOS only. macOS does not have it enabled nor exposed (the build flag is off))

It seems gated behind shouldEnableLegacyOverflowScrollingTouch + the legacyOverflowScrollingTouchEnabled setting. Maybe it can be made internal only? or at least gated behind the setting?

https://developer.mozilla.org/en-US/docs/Web/CSS/-webkit-overflow-scrolling#browser_compatibility says it's pretty much useless at this point.
Comment 51 Simon Fraser (smfr) 2022-05-27 19:01:09 PDT
-webkit-overflow-scrolling:touch still has the side-effect of making a scroller be a stacking context.
Comment 52 Tim Nguyen (:ntim) 2022-05-29 01:09:30 PDT
Sam, sorry for the churn, but can you rebase on top of https://github.com/WebKit/WebKit/pull/993 ?
Comment 53 Antti Koivisto 2022-06-03 07:24:36 PDT
Comment on attachment 459818 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.h:149
> +    FixedVector<CSSPropertyID> m_activeComputedPropertyIDs;

This is a vector containing every active CSSPropertyID so ~500 entries and 2 bytes per entry so ~1KB. This is being constructed from scratch for every style declaration created (which there can unbounded number). This seems way excessive in terms of both memory and processing time. The content of the vector is dependent on settings only so this structure should be per-document, not per declaration.
Comment 54 Sam Sneddon [:gsnedders] 2022-06-29 16:34:14 PDT
Created attachment 460557 [details]
Patch
Comment 55 Sam Sneddon [:gsnedders] 2022-06-30 09:22:34 PDT
Created attachment 460586 [details]
Patch
Comment 56 Sam Sneddon [:gsnedders] 2022-07-01 08:13:55 PDT
Created attachment 460612 [details]
Patch
Comment 57 Sam Sneddon [:gsnedders] 2022-07-01 13:01:45 PDT
Created attachment 460618 [details]
I should debug why git-webkit isn't working for me again, but in the meantime I think this addresses all the above feedback. So, uh, r?.
Comment 58 Sam Sneddon [:gsnedders] 2022-07-01 13:24:48 PDT
Created attachment 460619 [details]
Patch
Comment 59 Darin Adler 2022-07-01 22:18:45 PDT
Looked at the iOS-WK2 EWS failure and it’s this line, missing from imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-actual.txt:

    PASS -webkit-tap-highlight-color

If this line being missing is an improvement and intentional, then I think we need to update LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/all-prop-initial-xml-expected.txt in a new version of the patch.
Comment 60 Antti Koivisto 2022-07-01 22:55:08 PDT
Comment on attachment 460619 [details]
Patch

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

nice!

> Source/WebCore/css/CSSComputedStyleDeclaration.h:144
> +    const FixedVector<CSSPropertyID>& getActiveComputedPropertyIDs() const;

WebKit coding style does not use get* for getters (examples to the contrary are DOM API implementations or old leftovers). "Exposed" could  be a better name for this concept than "active computed", you use it elsewhere and in the prose too. So exposedCSSPropertyIDs() perhaps?

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:355
> +bool PropertySetCSSStyleDeclaration::isCSSPropertyExposed(CSSPropertyID propertyID) const

or isExposedCSSProperty, similar to existing isInternalCSSProperty/isEnabledCSSProperty etc.

> Source/WebCore/dom/Document.cpp:648
> +    {
> +        std::array<CSSPropertyID, numComputedPropertyIDs> exposed;
> +        auto last = std::copy_if(std::begin(computedPropertyIDs), std::end(computedPropertyIDs),
> +            exposed.begin(), [&](CSSPropertyID x) {
> +                return isCSSPropertyExposed(x, m_settings.ptr());
> +            });
> +
> +        FixedVector<CSSPropertyID> active(std::distance(exposed.begin(), last));
> +        std::copy(exposed.begin(), last, active.begin());
> +        m_activeComputedPropertyIDs = active;
> +    }

This work is not needed unless the page actually uses computed style APIs. Initialization could by done lazily in getActiveComputedPropertyIDs (though it is not that expensive either).

> Source/WebCore/dom/Document.h:1604
> +    const FixedVector<CSSPropertyID>& getActiveComputedPropertyIDs() const { return m_activeComputedPropertyIDs; }

Similarly exposedCSSPropertyIDs (including "CSS" since the document scope doesn't imply it).
Comment 61 Darin Adler 2022-07-01 23:57:21 PDT
More efficient to write WTFMove(active) to move the newly created vector in rather than copying it.