Summary: | AX: Chromium needs platform localized strings for media controls. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Tseng <dtseng> | ||||||||
Component: | Accessibility | Assignee: | David Tseng <dtseng> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, aboxhall, dglazkov, dmazzoni, fishd, jamesr, silviapf, tkent+wkapi, tony, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
David Tseng
2013-01-02 13:29:44 PST
Created attachment 181057 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Comment on attachment 181057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181057&action=review Seems fine, just fix the ChangeLog. > Source/Platform/ChangeLog:1 > +2013-01-02 My Name <my@email> Please put your name and email here. I think you can set user.name and user.email in git to have this automatically filled in. > Source/WebKit/chromium/ChangeLog:1 > +2013-01-02 My Name <my@email> Please put your name and email here. > Source/WebKit/chromium/ChangeLog:18 > +2013-01-02 My Name <my@email> > + > + Need a short description (OOPS!). > + Need the bug URL (OOPS!). > + > + Reviewed by NOBODY (OOPS!). Looks like this got duplicated. Created attachment 181067 [details]
Patch
FWIW: LGTM Thanks for taking this on - it's been on my plate for a while, but I never got the chance to look at it. Seems simpler than I thought! Comment on attachment 181067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181067&action=review > Source/Platform/chromium/public/WebLocalizedString.h:55 > + AXMediaDefault, > + AXMediaAudioElement, > + AXMediaVideoElement, > + AXMediaMuteButton, > + AXMediaUnMuteButton, > + AXMediaPlayButton, > + AXMediaPauseButton, Please sort these. > Source/WebKit/chromium/src/LocalizedStrings.cpp:462 > + > + return query(WebLocalizedString::AXMediaDefault); Should we ASSERT_NOT_REACHED() here? That's what WebCore/platform/LocalizedStrings.cpp does. Created attachment 181089 [details]
Patch
(In reply to comment #6) > (From update of attachment 181067 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181067&action=review > > > Source/Platform/chromium/public/WebLocalizedString.h:55 > > + AXMediaDefault, > > + AXMediaAudioElement, > > + AXMediaVideoElement, > > + AXMediaMuteButton, > > + AXMediaUnMuteButton, > > + AXMediaPlayButton, > > + AXMediaPauseButton, > > Please sort these. Done > > > Source/WebKit/chromium/src/LocalizedStrings.cpp:462 > > + > > + return query(WebLocalizedString::AXMediaDefault); > > Should we ASSERT_NOT_REACHED() here? That's what WebCore/platform/LocalizedStrings.cpp does. Done (as well as for localizedHelpText); (I went as far as to include the specific check for the ControlsPanel, but not the exception for the enter full screen button help since it seems odd for webkit to make such a singular exception. Comment on attachment 181089 [details]
Patch
Note that the ews and commit queue bots don't run in debug so if the assert triggers, we probably won't see it until the patch lands.
Comment on attachment 181089 [details] Patch Clearing flags on attachment: 181089 Committed r138680: <http://trac.webkit.org/changeset/138680> All reviewed patches have been landed. Closing bug. |