[webkit-reviews] review granted: [Bug 121990] AX: Regression: media controls are no longer accessible : [Attachment 215736] patch for review/commit

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted James Craig <jcraig at apple.com>'s
request for review:
Bug 121990: AX: Regression: media controls are no longer accessible
https://bugs.webkit.org/show_bug.cgi?id=121990

Attachment 215736: patch for review/commit
https://bugs.webkit.org/attachment.cgi?id=215736&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215736&action=review


Looks great.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:80
> +    /* Localized strings */

We normally use // comments everywhere, not /* */ comments.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:81
> +    loc: function(s){

Is this a standard name we use for this function elsewhere? Could we use a word
rather than a little abbreviation? I understand that brevity is nice, but I at
least find words easier to read.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:85
> +	       return s; // TODO: log something if string not localized.

We normally use FIXME for all things like this, never TODO.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:91
> +	   'Display Fullscreen': 'Display Fullscreen',

We use two words for this in Apple user interface and marketing materials,
“Full Screen”, not one word “Fullscreen”.