Bug 115026 – Improve the way UI element information is passed between AccessibilityMediaControls and the localization code

• Component: New Bugs
• Status: NEW

[Benjamin Poulain]

Improve the way UI element information is passed between AccessibilityMediaControls and the localization code

[Benjamin Poulain]

Created attachment 199205 [details]
Patch

[Early Warning System Bot]

Comment on attachment 199205 [details]
Patch

Attachment 199205 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/59841

[Early Warning System Bot]

Comment on attachment 199205 [details]
Patch

Attachment 199205 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/55830

[Build Bot]

Comment on attachment 199205 [details]
Patch

Attachment 199205 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/200387

New failing tests:
accessibility/media-element.html

[Build Bot]

Created attachment 199217 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 199205 [details]
Patch

Attachment 199205 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/77188

[Benjamin Poulain]

Created attachment 199332 [details]
Patch

[chris fleizach]

Comment on attachment 199332 [details]
Patch

There's already a set of enums for media controls,  (MediaControlElementType). It seems we should leverage that instead of defining another set

[Benjamin Poulain]

(In reply to comment #8)
> (From update of attachment 199332 [details])
> There's already a set of enums for media controls,  (MediaControlElementType). It seems we should leverage that instead of defining another set

I intentionally avoided them.

By defining a enum for the localization, we ensure every new value has a case in the switch-case (on all ports and in both function).
If you look at the two bugs fixed in this patch, they would have been avoided by using this separate enum :)

[Benjamin Poulain]

Although...I am assuming the current design is right.

If every type of MediaControlElementType is supposed to have a string, then it would be better to use that enum.

[chris fleizach]

(In reply to comment #10)
> Although...I am assuming the current design is right.
> 
> If every type of MediaControlElementType is supposed to have a string, then it would be better to use that enum.

My guess is yes. I had a patch ready a few days ago that i forgot about how the VolumeSliders did not have labels. They should have had them

[Ryosuke Niwa]

Comment on attachment 199332 [details]
Patch

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

> Source/WebCore/platform/LocalizedStrings.h:241
> +    ENUM_CLASS(LocalizedMediaControlElement) {

I would call this MediaControlElementStringType or MediaControlElementString.
LocalizedMediaControlElement makes it sound as if this is an element.

> Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:541
> +        return String();

Can we ever reach here?

> Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:587
> +        return String();

Ditto.

> Source/WebCore/platform/qt/LocalizedStringsQt.cpp:515
> +        return String();

Ditto.

> Source/WebCore/platform/qt/LocalizedStringsQt.cpp:561
> +        return String();

Ditto.

[chris fleizach]

(In reply to comment #11)
> (In reply to comment #10)
> > Although...I am assuming the current design is right.
> > 
> > If every type of MediaControlElementType is supposed to have a string, then it would be better to use that enum.
> 
> My guess is yes. I had a patch ready a few days ago that i forgot about how the VolumeSliders did not have labels. They should have had them

I looked over that emum list again. Everything there should get a description of some sort.

[Benjamin Poulain]

> > My guess is yes. I had a patch ready a few days ago that i forgot about how the VolumeSliders did not have labels. They should have had them
> 
> I looked over that emum list again. Everything there should get a description of some sort.

Your call.
Should I land this or do you want to change the class to use the other enum?

[chris fleizach]

(In reply to comment #14)
> > > My guess is yes. I had a patch ready a few days ago that i forgot about how the VolumeSliders did not have labels. They should have had them
> > 
> > I looked over that emum list again. Everything there should get a description of some sort.
> 
> Your call.
> Should I land this or do you want to change the class to use the other enum?

I think we should use the existing enums. The chances of someone adding a new type and forgetting about this enum list is probably non-zero

Thanks