Bug 96636 – Add runtime flag that enables lang attribute for form controls in LayoutTests

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Keishi Hattori]

Add feature flag that enables lang attribute for form controls in LayoutTests

[Keishi Hattori]

Created attachment 163845 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Kent Tamura]

Comment on attachment 163845 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        (WebCore::RuntimeEnabledFeatures::langAttributeForFormControlsEnabled):

This name doesn't explain the feature well.
Form controls can have lang attributes even now.
It enabled lang-attribute-aware-form-control-UI, right?

> Source/WebKit/chromium/public/WebRuntimeFeatures.h:165
> +    WEBKIT_EXPORT static void enableLangAttributeForFormControls(bool);
> +    WEBKIT_EXPORT static bool isLangAttributeForFormControlsEnabled();

I prefer enabling the feature in InternalSettings to in public WebKit API because this feature is for testing.

[Keishi Hattori]

Created attachment 163858 [details]
Patch

[Kent Tamura]

Comment on attachment 163858 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:13
> +        * public/WebRuntimeFeatures.h:
> +        (WebRuntimeFeatures):
> +        * src/WebRuntimeFeatures.cpp:
> +        (WebKit::WebRuntimeFeatures::enableLangAttributeForFormControls):
> +        (WebKit):
> +        (WebKit::WebRuntimeFeatures::isLangAttributeAwareFormControlUIEnabled):

These changes are not needed any more.

> Source/WebCore/testing/InternalSettings.cpp:649
> +    UNUSED_PARAM(ec);

So, you don't need to specify rases(DOMException) in the IDL.

> Source/WebCore/testing/InternalSettings.cpp:651
> +}

You need to update InternalSettings::Backup::Backup() and restoreTo().

[Alexey Proskuryakov]

Comment on attachment 163858 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Add feature flag that enables lang attribute aware form controls UI in LayoutTests

Do we call these "feature flags"? The title of the bug made me think of an ENABLE macro.

[Keishi Hattori]

(In reply to comment #6)
> Do we call these "feature flags"? The title of the bug made me think of an ENABLE macro.
I'm sorry. Past change logs seem to call it the runtime flag.

[Keishi Hattori]

Created attachment 163971 [details]
Patch

[Build Bot]

Comment on attachment 163971 [details]
Patch

Attachment 163971 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13841503

[Build Bot]

Comment on attachment 163971 [details]
Patch

Attachment 163971 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13841527

[Keishi Hattori]

Created attachment 164015 [details]
Patch

[Kent Tamura]

Comment on attachment 164015 [details]
Patch

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

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:264
> +    static void setLangAttributeAwareFormControlUIEnabled(bool isEnabled) { isLangAttributeAwareFormControlUIEnabled = isEnabled; }

Please add a warning comment that this should be enabled only for testing.

[Build Bot]

Comment on attachment 164015 [details]
Patch

Attachment 164015 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13852247

[Keishi Hattori]

Created attachment 164040 [details]
Patch

[Keishi Hattori]

Created attachment 164041 [details]
Patch

[Kent Tamura]

Comment on attachment 164041 [details]
Patch

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

> Source/WebCore/platform/text/LocaleWin.h:-97
> -    Vector<String> m_shortMonthLabels;
> -    Vector<String> m_monthLabels;
>  #if ENABLE(CALENDAR_PICKER)
> -    Vector<String> m_weekDayShortLabels;

This part looks unrelated to this bug.

[Keishi Hattori]

Created attachment 164042 [details]
Patch

[Kent Tamura]

Comment on attachment 164042 [details]
Patch

ok

[Kent Tamura]

Comment on attachment 164042 [details]
Patch

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

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.h:264
> +    static void setLangAttributeAwareFormControlUIEnabled(bool isEnabled) { isLangAttributeAwareFormControlUIEnabled = isEnabled; }

Please add a warning here.

[Keishi Hattori]

Created attachment 164068 [details]
Patch

[WebKit Review Bot]

Comment on attachment 164068 [details]
Patch

Rejecting attachment 164068 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13850476

[WebKit Review Bot]

Comment on attachment 164068 [details]
Patch

Clearing flags on attachment: 164068

Committed r128583: <http://trac.webkit.org/changeset/128583>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

Hello,
Currently there are one of two revisions that are causing the Windows test bots to exit early after 20 crashes. This is one possible candidate. 

Can you see if this would have caused a large quantity of accessibility tests on Windows 7 to start failing all of a sudden? The Windows bots have suddenly gotten into a terrible state so if you could verify this quickly I'd be greatly appreciative.

Thanks

[Kent Tamura]

(In reply to comment #24)
> Hello,
> Currently there are one of two revisions that are causing the Windows test bots to exit early after 20 crashes. This is one possible candidate. 
> 
> Can you see if this would have caused a large quantity of accessibility tests on Windows 7 to start failing all of a sudden? The Windows bots have suddenly gotten into a terrible state so if you could verify this quickly I'd be greatly appreciative.
> 
> Thanks

Roger,

It seems the change in this bug was the trigger.
http://build.webkit.org/results/Apple%20Win%20XP%20Debug%20(Tests)/r128619%20(44630)/accessibility/accessibility-node-memory-management-crash-log.txt

I'm not sure why Windows crashes at such code.  I'm investigating.

[Roger Fong]

My test log indicates that the 20 crashes started sometime between
128583 and 128585

128585	#36257	Sep 14 04:23
Build successful	#27689	Sep 14 04:41
Failed exiting early after 20 crashes, 0 web process crashes, and 0 timeouts. 20 tests run. 20 test cases (100%) crashed 1 unit tests failed or timed out webkitpy-test

128584	#36256	Sep 14 04:19
Build successful		

128583	#36255	Sep 14 04:11
Build successful	#27688	Sep 14 04:20
Failed layout-test 1 unit tests failed or timed out 3 python tests failed

128581	#36254	Sep 14 03:58
Build successful	#27687	Sep 14 04:02
Failed 66 test cases (<1%) had incorrect layout 3 test cases (<1%) timed out 1 unit tests failed or timed out 3 python tests failed

[Kent Tamura]

We successfully avoided the Windows issue by http://trac.webkit.org/changeset/128629 though I don't understand the root cause yet.

[Roger Fong]

Hmm, well thanks for fixing things!
Bots are happy(er) again. Much appreciated.

[Roger Fong]

Hi, Apple Windows port is broken again (although only when running DRT locally) and the workaround seems to have been removed...can I add it back in?

[Roger Fong]

(In reply to comment #29)
> Hi, Apple Windows port is broken again (although only when running DRT locally) and the workaround seems to have been removed...can I add it back in?

Work around was removed in a later patch in favor of a patch that theoretically fixed things on Windows, but unfortunately not when run locally. Continuing my investigation in appropriate bug.

[Kent Tamura]

(In reply to comment #30)
> (In reply to comment #29)
> > Hi, Apple Windows port is broken again (although only when running DRT locally) and the workaround seems to have been removed...can I add it back in?
> 
> Work around was removed in a later patch in favor of a patch that theoretically fixed things on Windows, but unfortunately not when run locally. Continuing my investigation in appropriate bug.

Have you never tried local DRT after http://trac.webkit.org/changeset/128850  ?
Anyway, please file another bug.

[Roger Fong]

Apparently not. 
I was working on stuff that didn't require me to use it for a while. 
It's also running fine on the test bots, so I didn't think anything was going wrong until just recently.