Bug 164251 – Remove runtime flag for variation fonts

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Remove runtime flag for variation fonts

[Myles C. Maxfield]

Created attachment 293483 [details]
Patch

[Myles C. Maxfield]

*** Bug 164244 has been marked as a duplicate of this bug. ***

[Dean Jackson]

Comment on attachment 293483 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        The mac port has already shipped Safari Technology Preview with the runtime
> +        flag defaulting to on, which means there are no reasons anyone would set the
> +        runtime flag to off when the compile-time flag is on. Because the compile-time
> +        flag suffices, we can remove the runtime flag.

This is not true. We might want to put lots of things behind experimental flags, even though they've been shipping enabled in the STP. Previously we were waiting for nice UI before turning things off by default (because otherwise there was no way to turn them on).

I think you can just leave this whole paragraph out.

> Source/WebCore/ChangeLog:13
> +        No new tests because there is no behavior change.

You can list the tests you changed.

> Source/WebCore/ChangeLog:28
> +        * css/CSSComputedStyleDeclaration.cpp:
> +        (WebCore::ComputedStyleExtractor::propertyValue):
> +        * css/parser/CSSParser.cpp:
> +        (WebCore::CSSParserContext::CSSParserContext):
> +        (WebCore::operator==):
> +        (WebCore::CSSParser::parseValue):
> +        * css/parser/CSSParserMode.h:
> +        (WebCore::CSSParserContext::completeURL):
> +        (WebCore::CSSParserContextHash::hash):
> +        * testing/InternalSettings.cpp:
> +        (WebCore::InternalSettings::Backup::Backup):
> +        (WebCore::InternalSettings::Backup::restoreTo):
> +        (WebCore::InternalSettings::variationFontsEnabled): Deleted.
> +        (WebCore::InternalSettings::setVariationFontsEnabled): Deleted.

This is missing the changes to Settings.in etc.

Also, a WebCore::Setting is not the same as the Experimental Feature exposed from WebKit2. You could have kept the WebCore::Setting, but I assume there is no reason to.

[Myles C. Maxfield]

Reopening.

[Myles C. Maxfield]

Created attachment 297534 [details]
Patch for committing

[Said Abou-Hallawa]

I think the EWS failures are due to https://bugs.webkit.org/show_bug.cgi?id=165220.

[Myles C. Maxfield]

Created attachment 298043 [details]
Patch for committing

[Myles C. Maxfield]

Committed r210315: <http://trac.webkit.org/changeset/210315>

[Darin Adler]

Broke the iOS build:

DerivedSources/WebCore/JSInternalSettingsGenerated.cpp:3846:10: error: no member named 'setVariationFontsEnabled' in 'WebCore::InternalSettingsGenerated'
    impl.setVariationFontsEnabled(WTFMove(variationFontsEnabled));
    ~~~~ ^

[Myles C. Maxfield]

(In reply to comment #9)
> Broke the iOS build:
> 
> DerivedSources/WebCore/JSInternalSettingsGenerated.cpp:3846:10: error: no
> member named 'setVariationFontsEnabled' in
> 'WebCore::InternalSettingsGenerated'
>     impl.setVariationFontsEnabled(WTFMove(variationFontsEnabled));
>     ~~~~ ^

This is a generated file which didn't get re-generated.

In the future, what should I do to avoid this? Trigger a full rebuild on the bots? Touch platform.h?

[Darin Adler]

(In reply to comment #10)
> (In reply to comment #9)
> > Broke the iOS build:
> > 
> > DerivedSources/WebCore/JSInternalSettingsGenerated.cpp:3846:10: error: no
> > member named 'setVariationFontsEnabled' in
> > 'WebCore::InternalSettingsGenerated'
> >     impl.setVariationFontsEnabled(WTFMove(variationFontsEnabled));
> >     ~~~~ ^
> 
> This is a generated file which didn't get re-generated.
> 
> In the future, what should I do to avoid this? Trigger a full rebuild on the
> bots? Touch platform.h?

We should determine why it did not get regenerated, perhaps by reproducing the problem, and fix the problem. I assume it’s some kind of bug in the makefile.

[Myles C. Maxfield]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Broke the iOS build:
> > > 
> > > DerivedSources/WebCore/JSInternalSettingsGenerated.cpp:3846:10: error: no
> > > member named 'setVariationFontsEnabled' in
> > > 'WebCore::InternalSettingsGenerated'
> > >     impl.setVariationFontsEnabled(WTFMove(variationFontsEnabled));
> > >     ~~~~ ^
> > 
> > This is a generated file which didn't get re-generated.
> > 
> > In the future, what should I do to avoid this? Trigger a full rebuild on the
> > bots? Touch platform.h?
> 
> We should determine why it did not get regenerated, perhaps by reproducing
> the problem, and fix the problem. I assume it’s some kind of bug in the
> makefile.

Yes, you're totally right. For some reason I was under the impression that our build regeneration logic was beyond repair, but of course, that isn't true. I'll look into this.

[Darin Adler]

(In reply to comment #12)
> For some reason I was under the impression that
> our build regeneration logic was beyond repair, but of course, that isn't
> true. I'll look into this.

For what it’s worth, the last couple of times I ran into a problem like this, I was able to fix a problem in the makefile and the problems weren’t too complex. Two techniques I used were to first come up with a reproducible technique so I could try things over and over again and to make a local temporary change to the project file so that make is invoked with the -d option, making clear what decisions it is making about what to build.

[Myles C. Maxfield]

(In reply to comment #13)
> (In reply to comment #12)
> > For some reason I was under the impression that
> > our build regeneration logic was beyond repair, but of course, that isn't
> > true. I'll look into this.
> 
> For what it’s worth, the last couple of times I ran into a problem like
> this, I was able to fix a problem in the makefile and the problems weren’t
> too complex. Two techniques I used were to first come up with a reproducible
> technique so I could try things over and over again and to make a local
> temporary change to the project file so that make is invoked with the -d
> option, making clear what decisions it is making about what to build.

This is related to the .INTERMEDIATE rules introduced in r178955, which I don't fully understand. I found the regression point and filed https://bugs.webkit.org/show_bug.cgi?id=166814