Bug 229915 - [css-masking] Add a temporarily prefixed property for mask-mode, aliased to -webkit-mask-source-type
Summary: [css-masking] Add a temporarily prefixed property for mask-mode, aliased to -...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
: 229899 (view as bug list)
Depends on: 229978
Blocks: 229082
  Show dependency treegraph
 
Reported: 2021-09-04 09:49 PDT by Simon Fraser (smfr)
Modified: 2021-09-08 07:59 PDT (History)
19 users (show)

See Also:


Attachments
Patch (32.62 KB, patch)
2021-09-04 09:53 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (53.14 KB, patch)
2021-09-05 11:00 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (75.57 KB, patch)
2021-09-05 14:40 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (77.87 KB, patch)
2021-09-06 20:50 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (77.87 KB, patch)
2021-09-07 08:25 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2021-09-04 09:49:20 PDT
Add a temporarily prefixed property for mask-mode, aliased to -webkit-mask-source-type
Comment 1 Simon Fraser (smfr) 2021-09-04 09:53:28 PDT
Created attachment 437344 [details]
Patch
Comment 2 Simon Fraser (smfr) 2021-09-04 10:01:51 PDT
*** Bug 229899 has been marked as a duplicate of this bug. ***
Comment 3 Antti Koivisto 2021-09-04 22:33:21 PDT
Comment on attachment 437344 [details]
Patch

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

> Source/WebCore/css/makeprop.pl:1307
> -  $setterContent .= $indent . "static void applyValue" . $nameToId{$name} . "(BuilderState& builderState, CSSValue& value)\n";
> +  $setterContent .= $indent . "static void applyValue" . $nameToId{$name} . "(CSSPropertyID property, BuilderState& builderState, CSSValue& value)\n";
>    $setterContent .= $indent . "{\n";
> +  
> +  if (!exists $propertiesWithStyleBuilderOptions{$name}{"fill-layer-property"}) {
> +    $setterContent .= $indent . "    UNUSED_PARAM(property);\n";
> +  }

If I understand correctly this adds the property argument to every one of the 647 generated applyValueFoo functions while it can be statically determined in all cases except one. This doesn't seem ideal.

The code generation could be check for 'synonym' concept and only generate the property parameter for it.

> Source/WebCore/css/makeprop.pl:1385
> +  next if (exists $synonyms{$name});

I suppose an alternative would be to just generate a separate function for the synonym and do the synonym name mapping when generating the function. This would avoid having to pass the propertyid dynamically and might be generally simpler.
Comment 4 Simon Fraser (smfr) 2021-09-05 11:00:54 PDT
Created attachment 437364 [details]
Patch
Comment 5 Simon Fraser (smfr) 2021-09-05 11:02:23 PDT
(In reply to Antti Koivisto from comment #3)

> > Source/WebCore/css/makeprop.pl:1385
> > +  next if (exists $synonyms{$name});
> 
> I suppose an alternative would be to just generate a separate function for
> the synonym and do the synonym name mapping when generating the function.
> This would avoid having to pass the propertyid dynamically and might be
> generally simpler.

I decided for now it was simplest just to pass the CSSPropertyID as an argument to all the fill layer value applier functions, since their implementations already had a hardcoded propertyID that can now just use the argument.
Comment 6 Simon Fraser (smfr) 2021-09-05 14:40:44 PDT
Created attachment 437367 [details]
Patch
Comment 7 Antti Koivisto 2021-09-06 01:34:51 PDT
Comment on attachment 437367 [details]
Patch

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

> Source/WebCore/css/makeprop.pl:306
>                      $nameToAliases{$name} = $codegenProperties->{"aliases"};
> +                } elsif ($codegenOptionName eq "synonym") {
> +                  $synonyms{$name} = 1;
> +                  push @{$namesToSynonyms{$codegenProperties->{"synonym"}}}, $name;
>                  } elsif ($styleBuilderOptions{$codegenOptionName}) {

weird indentation
Comment 8 Simon Fraser (smfr) 2021-09-06 09:33:54 PDT
https://trac.webkit.org/changeset/282058/webkit
Comment 9 Radar WebKit Bug Importer 2021-09-06 09:34:28 PDT
<rdar://problem/82795719>
Comment 10 Aakash Jain 2021-09-06 17:43:49 PDT
(In reply to Simon Fraser (smfr) from comment #8)
> https://trac.webkit.org/changeset/282058/webkit
This seems to have broken two layout tests on iOS:
- imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-csstext.html
- imported/w3c/web-platform-tests/css/cssom/getComputedStyle-detached-subtree.html

History: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcssom%2Fcssstyledeclaration-csstext.html&test=imported%2Fw3c%2Fweb-platform-tests%2Fcss%2Fcssom%2FgetComputedStyle-detached-subtree.html

EWS indicated these failures on previous patch in https://ews-build.webkit.org/#/builders/51/builds/20605

Patch was landed manually and might have been different from the one which passed EWS.
Comment 11 Aakash Jain 2021-09-06 17:44:28 PDT
Separately, please try to use commit-queue. We are trying to discourage manual commits. If you need to land a patch quickly, you can use fast-cq mode (See https://lists.webkit.org/pipermail/webkit-dev/2021-April/031782.html).
Comment 12 WebKit Commit Bot 2021-09-06 17:46:47 PDT
Re-opened since this is blocked by bug 229978
Comment 13 Simon Fraser (smfr) 2021-09-06 20:50:07 PDT
Created attachment 437447 [details]
Patch
Comment 14 Simon Fraser (smfr) 2021-09-07 08:25:32 PDT
Created attachment 437509 [details]
Patch
Comment 15 EWS 2021-09-08 07:59:31 PDT
Committed r282143 (241440@main): <https://commits.webkit.org/241440@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437509 [details].