Bug 216910 - [SVG2] Remove color-profile tag
Summary: [SVG2] Remove color-profile tag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks: 191292
  Show dependency treegraph
 
Reported: 2020-09-23 19:03 PDT by Fujii Hironori
Modified: 2020-09-24 20:09 PDT (History)
16 users (show)

See Also:


Attachments
Patch (1.95 KB, patch)
2020-09-23 19:06 PDT, Fujii Hironori
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (3.42 KB, patch)
2020-09-23 22:33 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (2.45 KB, patch)
2020-09-24 13:51 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2020-09-23 19:03:24 PDT
SVG2 removed color_profile element.

Element Index — SVG 2
https://www.w3.org/TR/SVG2/eltindex.html
Comment 1 Fujii Hironori 2020-09-23 19:06:44 PDT
Created attachment 409524 [details]
Patch
Comment 2 Fujii Hironori 2020-09-23 19:45:22 PDT
EWS reported test failures.

imported/w3c/web-platform-tests/custom-elements/CustomElementRegistry.html
imported/w3c/web-platform-tests/custom-elements/custom-element-registry/define.html

WPT has test cases for color-profile element.
Comment 3 Fujii Hironori 2020-09-23 22:10:29 PDT
We should keep color-profile tag.
I'm going to reuse this ticket to add color-profile tag.
Comment 4 Fujii Hironori 2020-09-23 22:33:24 PDT
Created attachment 409532 [details]
Patch
Comment 5 Fujii Hironori 2020-09-24 00:11:31 PDT
OMG, some test failures and crashes on EWS.
Comment 6 Sam Weinig 2020-09-24 07:47:32 PDT
I'm not clear I understand the purpose of this change? What is desired web observable effect you are going for?
Comment 7 Fujii Hironori 2020-09-24 12:59:30 PDT
My motivations are
1. I'd like to stop using C preprocessor to avoid Cygwin fork failure (Bug 206565 comment 1)
2. I'd like to remove "#if 0" hack which looks too tricky

I don't want any web observable effects.
Do you have any better idea? Or, should I give up this?
Comment 8 Sam Weinig 2020-09-24 13:03:13 PDT
(In reply to Fujii Hironori from comment #7)
> My motivations are
> 1. I'd like to stop using C preprocessor to avoid Cygwin fork failure (Bug
> 206565 comment 1)
> 2. I'd like to remove "#if 0" hack which looks too tricky
> 
> I don't want any web observable effects.
> Do you have any better idea? Or, should I give up this?

Just remove the three lines entirely. Longtime #if 0'd code is not useful.
Comment 9 Sam Weinig 2020-09-24 13:06:14 PDT
(In reply to Sam Weinig from comment #8)
> (In reply to Fujii Hironori from comment #7)
> > My motivations are
> > 1. I'd like to stop using C preprocessor to avoid Cygwin fork failure (Bug
> > 206565 comment 1)
> > 2. I'd like to remove "#if 0" hack which looks too tricky
> > 
> > I don't want any web observable effects.
> > Do you have any better idea? Or, should I give up this?
> 
> Just remove the three lines entirely. Longtime #if 0'd code is not useful.

Oh, I see, your first change tried that? Hm. I'd like to understand why the #if isn't working right now.
Comment 10 Sam Weinig 2020-09-24 13:17:35 PDT
(In reply to Sam Weinig from comment #9)
> (In reply to Sam Weinig from comment #8)
> > (In reply to Fujii Hironori from comment #7)
> > > My motivations are
> > > 1. I'd like to stop using C preprocessor to avoid Cygwin fork failure (Bug
> > > 206565 comment 1)
> > > 2. I'd like to remove "#if 0" hack which looks too tricky
> > > 
> > > I don't want any web observable effects.
> > > Do you have any better idea? Or, should I give up this?
> > 
> > Just remove the three lines entirely. Longtime #if 0'd code is not useful.
> 
> Oh, I see, your first change tried that? Hm. I'd like to understand why the
> #if isn't working right now.

Ok, so make_names.pl does a weird thing where it does two passes over the input, one preprocessed, one not. Very fun. 

But now I see what we should do. 

You should go back to your initial change, which is correct, but we need to add "color_profile" to the list of invalid names for custom elements (you can see it in the spec here https://html.spec.whatwg.org/multipage/custom-elements.html#valid-custom-element-name). This will probably mean making a change to JSCustomElementRegistry::define(...). If you need some help with this, let me know and I can try my hand at it.
Comment 11 Sam Weinig 2020-09-24 13:20:01 PDT
(In reply to Sam Weinig from comment #10)
> (In reply to Sam Weinig from comment #9)
> > (In reply to Sam Weinig from comment #8)
> > > (In reply to Fujii Hironori from comment #7)
> > > > My motivations are
> > > > 1. I'd like to stop using C preprocessor to avoid Cygwin fork failure (Bug
> > > > 206565 comment 1)
> > > > 2. I'd like to remove "#if 0" hack which looks too tricky
> > > > 
> > > > I don't want any web observable effects.
> > > > Do you have any better idea? Or, should I give up this?
> > > 
> > > Just remove the three lines entirely. Longtime #if 0'd code is not useful.
> > 
> > Oh, I see, your first change tried that? Hm. I'd like to understand why the
> > #if isn't working right now.
> 
> Ok, so make_names.pl does a weird thing where it does two passes over the
> input, one preprocessed, one not. Very fun. 
> 
> But now I see what we should do. 
> 
> You should go back to your initial change, which is correct, but we need to
> add "color_profile" to the list of invalid names for custom elements (you
> can see it in the spec here
> https://html.spec.whatwg.org/multipage/custom-elements.html#valid-custom-
> element-name). This will probably mean making a change to
> JSCustomElementRegistry::define(...). If you need some help with this, let
> me know and I can try my hand at it.

Actually, your original patch was very close to solving this. Instead of removing the line:

localName == SVGNames::color_profileTag->localName()

just replace it with 

localName == color_profileLocalName

and add:

    static MainThreadNeverDestroyed<const AtomString> color_profileLocalName("color_profile", AtomString::ConstructFromLiteral);

above it.
Comment 12 Fujii Hironori 2020-09-24 13:34:28 PDT
It sounds great. I'm going to try it. Thanks.
Comment 13 Fujii Hironori 2020-09-24 13:51:18 PDT
Created attachment 409619 [details]
Patch
Comment 14 EWS 2020-09-24 15:25:50 PDT
Committed r267550: <https://trac.webkit.org/changeset/267550>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409619 [details].
Comment 15 Radar WebKit Bug Importer 2020-09-24 15:26:18 PDT
<rdar://problem/69529787>