Bug 216910 – [SVG2] Remove color-profile tag

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Fujii Hironori]

SVG2 removed color_profile element.

Element Index — SVG 2
https://www.w3.org/TR/SVG2/eltindex.html

[Fujii Hironori]

Created attachment 409524 [details]
Patch

[Fujii Hironori]

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.

[Fujii Hironori]

We should keep color-profile tag.
I'm going to reuse this ticket to add color-profile tag.

[Fujii Hironori]

Created attachment 409532 [details]
Patch

[Fujii Hironori]

OMG, some test failures and crashes on EWS.

[Sam Weinig]

I'm not clear I understand the purpose of this change? What is desired web observable effect you are going for?

[Fujii Hironori]

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?

[Sam Weinig]

(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.

[Sam Weinig]

(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.

[Sam Weinig]

(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.

[Sam Weinig]

(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.

[Fujii Hironori]

It sounds great. I'm going to try it. Thanks.

[Fujii Hironori]

Created attachment 409619 [details]
Patch

[EWS]

Committed r267550: <https://trac.webkit.org/changeset/267550>

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

[Radar WebKit Bug Importer]

<rdar://problem/69529787>