Add ChromeVox panel and implement caption display functionality.

Add ChromeVox panel and implement caption display functionality.

This patch creates a new HTML background page in the extension that's
formatted with a menu button, a caption area for speech and braille, a
settings button, and a close button.

Currently only the caption part functions - it uses cross-window messages
to send either the current speech from TTS, or the current braille text
and cells if braille captions are enabled.

This patch does not contain the code to show the caption panel on the screen,
that will follow. You can test it in the meantime by manually opening up
the URL.

BUG=420795
Committed: https://crrev.com/2a1101bf6372cc28c09ce359145f3952f00e80f1
Cr-Commit-Position: refs/heads/master@{#358864}

[dmazzoni, 2015-08-07 20:55:35]

dmazzoni@chromium.org changed reviewers:
+ dtseng@chromium.org, plundblad@chromium.org

[David Tseng, 2015-08-07 21:43:03]

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js
(right):

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:18: *
Class to manage the options page.
nit: panel

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:21:
cvox.Panel = function() {
Do we still want cvox namespaces in next code?

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:30: /**
@type {Element} */
nit: @private and !Element

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:33: /**
@type {Element} */
ditto

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:36: /**
@type {Element} */
ditto

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:64:
cvox.Panel.updateFromPrefs = function() {
It would be nice if this class had a prototype so we could subclass for
i-search.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:111: };
TODO?

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:117: };
TODO?

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js
(right):

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:6:
* @fileoverview Commands to pass to the ChromeVox Panel.
From? To?

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:31:
console.log('Posting to panel: ' + JSON.stringify(this));
nit: remove

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/test.html
(right):

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/test.html:13:
</script>
Looks unrelated?

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/host/chrome/tts_background.js
(right):

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/host/chrome/tts_background.js:328:
console.log('Properties: ' + JSON.stringify(utterance.properties));
nit: remove

[dmazzoni, 2015-11-05 23:59:41]

Ready for another look. Addressed feedback, and finished getting this working
with the follow-up changelist that makes the panel appear automatically on
Chrome OS.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js
(right):

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:18: *
Class to manage the options page.
On 2015/08/07 21:43:03, David Tseng wrote:
> nit: panel

Done.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:21:
cvox.Panel = function() {
On 2015/08/07 21:43:03, David Tseng wrote:
> Do we still want cvox namespaces in next code?

Done.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:30: /**
@type {Element} */
On 2015/08/07 21:43:03, David Tseng wrote:
> nit: @private and !Element

Done.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:33: /**
@type {Element} */
On 2015/08/07 21:43:03, David Tseng wrote:
> ditto

Done.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:36: /**
@type {Element} */
On 2015/08/07 21:43:03, David Tseng wrote:
> ditto

Done.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:64:
cvox.Panel.updateFromPrefs = function() {
On 2015/08/07 21:43:03, David Tseng wrote:
> It would be nice if this class had a prototype so we could subclass for
> i-search.

My thinking at this point is that there will be only one panel, but some of the
functionality inside of it, like i-search, will subclass from something like
PanelWidget.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:111: };
On 2015/08/07 21:43:03, David Tseng wrote:
> TODO?

Implemented now.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:117: };
On 2015/08/07 21:43:02, David Tseng wrote:
> TODO?

Implemented now.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js
(right):

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:6:
* @fileoverview Commands to pass to the ChromeVox Panel.
On 2015/08/07 21:43:03, David Tseng wrote:
> From? To?

Done.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:31:
console.log('Posting to panel: ' + JSON.stringify(this));
On 2015/08/07 21:43:03, David Tseng wrote:
> nit: remove

Done.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/test.html
(right):

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/test.html:13:
</script>
On 2015/08/07 21:43:03, David Tseng wrote:
> Looks unrelated?

Done.

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/host/chrome/tts_background.js
(right):

https://codereview.chromium.org/1277183003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/host/chrome/tts_background.js:328:
console.log('Properties: ' + JSON.stringify(utterance.properties));
On 2015/08/07 21:43:03, David Tseng wrote:
> nit: remove

Done.

[Peter Lundblad, 2015-11-06 15:00:58]

Hi,

A bunch of comments and questions below. Were you planning to internationalize
this
later?

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js
(right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:203:
cvox.BrailleCaptionsBackground.setContent(brailleText, buf);
What's the point of this change?

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/chromevox_assets.gypi (right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox_assets.gypi:12:
'chromevox/close-19.png',
Perhaps create an images directory now that we're getting mroe of these
just to keep the top-level unpoluted.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:440:
chrome.tabs.create(optionsPage);
Just since you touch this, is chrome.runtime.openOptionsPage an alternative?
Perhaps just a TODO in this cl.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:723: }
Why is this needed here in addition to background_tts.js?

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html
(right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:7:
<title class="i18n" msgid="panel_title">ChromeVox Panel</title>
Need to add this msgid to chromevox_strings.grd.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:124:
<button id="menu" aria-label="ChromeVox Menu">
i18n of aria label.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:130:
<div id="speech">Speech</div>
i18n

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:133:
<div id="braille-text">Braille text</div>
i18n

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js
(right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:14:
goog.require('cvox.ExtensionBridge');
Do you need to pull in this and the above?

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:30: /**
@type {Element} @private*/
nit: missing space after @private

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:33: /**
@type {Element} @private*/
nit: missing space after @private

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:57: };
Do you need an Msgs.addTranslatedMessagesToDom somewhere here?

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:87:
command.data +
Where is this & and < escaped?

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:97:
this.brailleTextElement_.textContent = command.data;
Will this collapse whitespace? Perhaps this needs a whitespace style property
in the css.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js
(right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:46:
UPDATE_BRAILLE_CELLS: 'update_braille_cells'
Since cells and text should always be updated together, I think it would be
clearer to have it as one command. Just make the command contain a subobject
instead of a string|ArrayBuffer.
This also makes sense if we ever want to support aligning the characters
and cells similar to what we do in the corresponding feature on BrailleBack
on Android. I don't know how much effort that is worth, though.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi (right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:25:
'is_chromeos%': 1,
We already have a variable in gyp called 'chromeos', so it is a bit confusing to
add 'is_chromeos'. I think I understand why you do this, but it needs to be
clarified somehow. Perhaps a variable like chromevox_non_chromeos_build makes
more
sense.

[dmazzoni, 2015-11-06 21:01:01]

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js
(right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:203:
cvox.BrailleCaptionsBackground.setContent(brailleText, buf);
On 2015/11/06 15:00:57, Peter Lundblad wrote:
> What's the point of this change?

Oops, I originally tried hooking into this file to drive the braille captions
but changed my mind. Reverted this change.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/chromevox_assets.gypi (right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox_assets.gypi:12:
'chromevox/close-19.png',
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> Perhaps create an images directory now that we're getting mroe of these
> just to keep the top-level unpoluted.

Done.

I realized the ChromeVox icons weren't in the manifest, so I remedied that. We
don't technically need them for the Chrome OS version, but we should really have
them in the web store version.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
(right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:440:
chrome.tabs.create(optionsPage);
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> Just since you touch this, is chrome.runtime.openOptionsPage an alternative?
> Perhaps just a TODO in this cl.

Thanks. Works great, but only if you call it from the background page.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js
(right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:723: }
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> Why is this needed here in addition to background_tts.js?

I was thinking we could provide more detailed information here. But for now I'll
delete this for simplicity.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html
(right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:7:
<title class="i18n" msgid="panel_title">ChromeVox Panel</title>
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> Need to add this msgid to chromevox_strings.grd.

Done.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:124:
<button id="menu" aria-label="ChromeVox Menu">
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> i18n of aria label.

Done.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:130:
<div id="speech">Speech</div>
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> i18n

This one was just a placeholder. Removed.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:133:
<div id="braille-text">Braille text</div>
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> i18n

Same.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js
(right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:14:
goog.require('cvox.ExtensionBridge');
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> Do you need to pull in this and the above?

ExtensionBridge isn't needed.

cvox.ChromeVox is where we define $(), because Chrome's JS linting complains if
we use document.getElementById.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:30: /**
@type {Element} @private*/
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> nit: missing space after @private

Done.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:33: /**
@type {Element} @private*/
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> nit: missing space after @private

Done.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:57: };
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> Do you need an Msgs.addTranslatedMessagesToDom somewhere here?

Yes. Done and tested.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:87:
command.data +
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> Where is this & and < escaped?

Good catch.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:97:
this.brailleTextElement_.textContent = command.data;
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> Will this collapse whitespace? Perhaps this needs a whitespace style property
> in the css.

Good idea, done.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js
(right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:46:
UPDATE_BRAILLE_CELLS: 'update_braille_cells'
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> Since cells and text should always be updated together, I think it would be
> clearer to have it as one command. Just make the command contain a subobject
> instead of a string|ArrayBuffer.
> This also makes sense if we ever want to support aligning the characters
> and cells similar to what we do in the corresponding feature on BrailleBack
> on Android. I don't know how much effort that is worth, though.

Done.

I already adjusted the CSS so that they line up as well as possible without
doing something more advanced line one character at a time.

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi (right):

https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:25:
'is_chromeos%': 1,
On 2015/11/06 15:00:58, Peter Lundblad wrote:
> We already have a variable in gyp called 'chromeos', so it is a bit confusing
to
> add 'is_chromeos'. I think I understand why you do this, but it needs to be
> clarified somehow. Perhaps a variable like chromevox_non_chromeos_build makes
> more
> sense.

I ended up getting rid of the accessibilityFeatures permission because it turned
out to not reliably close ChromeVox. Apparently only one extension can have
"write" permission for a Chrome setting at a time.

Now clicking the 'close' button triggers the C++ code that hosts the panel to
close it, and that's reliable.

We'll have to figure out what to do for the webstore version. I could just hide
the close button.

[Peter Lundblad, 2015-11-09 15:20:18]

dmazzoni@chromium.org writes:
> 
>
https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js
> (right):
> 
>
https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/background.js:440:
> chrome.tabs.create(optionsPage);
> On 2015/11/06 15:00:58, Peter Lundblad wrote:
> > Just since you touch this, is chrome.runtime.openOptionsPage an
> alternative?
> > Perhaps just a TODO in this cl.
> 
> Thanks. Works great, but only if you call it from the background page.

Could consider sending the message from the panel to the background script
to be consistent (without needing the extension bridge). I read something
about opening a new tab for the options pages being deprecated and
going away, so perhaps it is worth being consistent to begin with.

>
https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js
> (right):
> 
>
https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:14:
> goog.require('cvox.ExtensionBridge');
> On 2015/11/06 15:00:58, Peter Lundblad wrote:
> > Do you need to pull in this and the above?
> 
> ExtensionBridge isn't needed.
> 
> cvox.ChromeVox is where we define $(), because Chrome's JS linting
> complains if we use document.getElementById.

cvox.ChromeVox has a bunch of cruft which I guess is more confusing than
harmful in the context of the panel. Consider just duplicating the $ function.

>
https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
> File
> chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js
> (right):
> 
>
https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:46:
> UPDATE_BRAILLE_CELLS: 'update_braille_cells'
> On 2015/11/06 15:00:58, Peter Lundblad wrote:
> > Since cells and text should always be updated together, I think it
> would be
> > clearer to have it as one command. Just make the command contain a
> subobject
> > instead of a string|ArrayBuffer.
> > This also makes sense if we ever want to support aligning the
> characters
> > and cells similar to what we do in the corresponding feature on
> BrailleBack
> > on Android. I don't know how much effort that is worth, though.
> 
> Done.
> 
> I already adjusted the CSS so that they line up as well as possible
> without doing something more advanced line one character at a time.
> 
>
https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi
> (right):
> 
>
https://codereview.chromium.org/1277183003/diff/20001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:25:
> 'is_chromeos%': 1,
> On 2015/11/06 15:00:58, Peter Lundblad wrote:
> > We already have a variable in gyp called 'chromeos', so it is a bit
> confusing to
> > add 'is_chromeos'. I think I understand why you do this, but it needs
> to be
> > clarified somehow. Perhaps a variable like
> chromevox_non_chromeos_build makes
> > more
> > sense.
> 
> I ended up getting rid of the accessibilityFeatures permission because
> it turned out to not reliably close ChromeVox. Apparently only one
> extension can have "write" permission for a Chrome setting at a time.
> 
> Now clicking the 'close' button triggers the C++ code that hosts the
> panel to close it, and that's reliable.
> 
> We'll have to figure out what to do for the webstore version. I could
> just hide the close button.

Maybe make it trigger the enable/disable code in chromevox itself that David
enabled recently.

Thanks,
//Peter

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-11-09 22:11:38]

On 2015/11/09 15:20:18, Peter Lundblad wrote:
> > Thanks. Works great, but only if you call it from the background page.
> 
> Could consider sending the message from the panel to the background script
> to be consistent (without needing the extension bridge). I read something
> about opening a new tab for the options pages being deprecated and
> going away, so perhaps it is worth being consistent to begin with.

Yep, that's what it does now.

> > cvox.ChromeVox is where we define $(), because Chrome's JS linting
> > complains if we use document.getElementById.
> 
> cvox.ChromeVox has a bunch of cruft which I guess is more confusing than
> harmful in the context of the panel. Consider just duplicating the $ function.

Done.

> > Now clicking the 'close' button triggers the C++ code that hosts the
> > panel to close it, and that's reliable.
> > 
> > We'll have to figure out what to do for the webstore version. I could
> > just hide the close button.
> 
> Maybe make it trigger the enable/disable code in chromevox itself that David
> enabled recently.

Sounds good, but that isn't landed yet.

[Peter Lundblad, 2015-11-10 10:46:36]

lgtm

Thought I sent these comments as well yesterday, but aparently not, so here
we go...

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/BUILD.gn (right):

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/BUILD.gn:290:
compress_js("chromevox2_panel_script") {
nit: Get rid of the 2 in this and other names. We used the 2 when we had two
background pages, but
it should go away now.

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/chromevox_assets.gypi (right):

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox_assets.gypi:7:
'chromevox_assets_images': [
chromevox_assets_chromevox is still referred in BUILD.gn and needs to be
updated.

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html
(right):

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:141:
<button id="options" aria-label="ChromeVox Options"></button>
i18n the label

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:142:
<button id="close" aria-label="Close ChromeVox"></button>
i18n the label.

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js
(right):

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:118:
escapeForHtml(command.data.text);
Shouldn't escape when setting textContent, in this case the text is not parsed.

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:120:
escapeForHtml(command.data.braille);
Same here.

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js
(right):

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:16:
* @param {string|ArrayBuffer|{text: string, braille: string}=} opt_data
Remove the ArrayBuffer. Not needed, and wouldn't be serializable in
JSON anyway.

[dmazzoni, 2015-11-10 17:33:17]

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/BUILD.gn (right):

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/BUILD.gn:290:
compress_js("chromevox2_panel_script") {
On 2015/11/10 10:46:36, Peter Lundblad wrote:
> nit: Get rid of the 2 in this and other names. We used the 2 when we had two
> background pages, but
> it should go away now.

Done.

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/chromevox_assets.gypi (right):

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox_assets.gypi:7:
'chromevox_assets_images': [
On 2015/11/10 10:46:36, Peter Lundblad wrote:
> chromevox_assets_chromevox is still referred in BUILD.gn and needs to be
> updated.

Good catch, thanks.

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html
(right):

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:141:
<button id="options" aria-label="ChromeVox Options"></button>
On 2015/11/10 10:46:36, Peter Lundblad wrote:
> i18n the label

Done.

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:142:
<button id="close" aria-label="Close ChromeVox"></button>
On 2015/11/10 10:46:36, Peter Lundblad wrote:
> i18n the label.

Done.

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js
(right):

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:118:
escapeForHtml(command.data.text);
On 2015/11/10 10:46:36, Peter Lundblad wrote:
> Shouldn't escape when setting textContent, in this case the text is not
parsed.

Done.

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:120:
escapeForHtml(command.data.braille);
On 2015/11/10 10:46:36, Peter Lundblad wrote:
> Same here.

Done.

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js
(right):

https://codereview.chromium.org/1277183003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:16:
* @param {string|ArrayBuffer|{text: string, braille: string}=} opt_data
On 2015/11/10 10:46:36, Peter Lundblad wrote:
> Remove the ArrayBuffer. Not needed, and wouldn't be serializable in
> JSON anyway.

Done.

[dmazzoni, 2015-11-10 17:42:01]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-11-10 17:42:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
plundblad@chromium.org
Link to the patchset: https://codereview.chromium.org/1277183003/#ps100001
(title: "Address last feedback")

[commit-bot: I haz the power, 2015-11-10 17:43:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1277183003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1277183003/100001

[commit-bot: I haz the power, 2015-11-10 17:49:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-10 17:49:59]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2015-11-10 18:20:33]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-11-10 18:20:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
plundblad@chromium.org
Link to the patchset: https://codereview.chromium.org/1277183003/#ps120001
(title: "Fix GN build")

[commit-bot: I haz the power, 2015-11-10 18:21:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1277183003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1277183003/120001

[commit-bot: I haz the power, 2015-11-10 18:55:17]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-10 18:56:24]

Patchset 7 (id:??) landed as
https://crrev.com/2a1101bf6372cc28c09ce359145f3952f00e80f1
Cr-Commit-Position: refs/heads/master@{#358864}