Display images in multiline Braille

Display images in multiline Braille

When ChromeVox lands on an object that's an image, and when there's a
multi-line braille display, convert's the image to black-and-white and
displays it on the braille display.

Original changelist (never landed): http://crrev.com/2544203004

BUG=670925
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2703663002
Cr-Commit-Position: refs/heads/master@{#453537}
Committed: https://chromium.googlesource.com/chromium/src/+/5d5130c54bb6ba7a3d1e3aa324735ab76e934eb8

[dmazzoni, 2017-02-16 20:45:35]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 20:46:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 22:06:16]

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

[commit-bot: I haz the power, 2017-02-16 22:06:17]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dmazzoni, 2017-02-22 16:50:34]

Description was changed from

==========
Display images in multiline Braille

When ChromeVox lands on an object that's an image, and when there's a
multi-line braille display, convert's the image to black-and-white and
displays it on the braille display.

Original changelist (never landed): http://crrev.com/2544203004

BUG=670925
==========

to

==========
Display images in multiline Braille

When ChromeVox lands on an object that's an image, and when there's a
multi-line braille display, convert's the image to black-and-white and
displays it on the braille display.

Original changelist (never landed): http://crrev.com/2544203004

BUG=670925
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2017-02-22 16:50:59]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 16:51:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-02-22 16:52:13]

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

[dmazzoni, 2017-02-22 16:52:14]

Ready for a look. This is Jeffrey Chen's final change,
rebased after the enum/externs refactoring and with a
couple of tests fixed.

[commit-bot: I haz the power, 2017-02-22 18:08:47]

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

[commit-bot: I haz the power, 2017-02-22 18:08:50]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[David Tseng, 2017-02-22 18:15:24]

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

https://codereview.chromium.org/2703663002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:70:
this.displayingImage_ = false;
js doc

https://codereview.chromium.org/2703663002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:217:
*     textColumnCount: (number|undefined)}} newState Display state reported
This should be cvox.BrailleDisplayState.

https://codereview.chromium.org/2703663002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:268:
if (!this.displayState_.available || this.displayingImage_) {
Seems like you should get this to work without keeping explicit track of whether
an image is displayed or not.

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

https://codereview.chromium.org/2703663002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:422:
var node = evt.target;
I don't think you need most or any of this. If this event comes back as a result
of us requesting image data, then just output the braille directly without all
of the calls to Output.

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

https://codereview.chromium.org/2703663002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:980: if
(this.imageDataUrl_ && brailleDisplayState.textRowCount >= 2) {
I don't think we should hack in the image display like this.

Alternative proposal: add a command to CommandHandler that takes the node from
the current range and gets the image data.

[dmazzoni, 2017-02-24 00:19:42]

OK, take a look. I modified it to display
the image only when you explicitly request
it with a new command bound to Search+Ctrl+G.
When you do it freezes the braille display
until you navigate to another range.

Now there are no changes to output.

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

https://codereview.chromium.org/2703663002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:70:
this.displayingImage_ = false;
On 2017/02/22 18:15:24, David Tseng wrote:
> js doc

Done.

https://codereview.chromium.org/2703663002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:217:
*     textColumnCount: (number|undefined)}} newState Display state reported
On 2017/02/22 18:15:24, David Tseng wrote:
> This should be cvox.BrailleDisplayState.

I did this on purpose. The extension API allows
textRowCount and textColumnCount to be undefined, but
I changed cvox.BrailleDisplayState to require them to be
defined because it helps catch error, like a place in the
code where we were still using textCellCount.

I added this to the jsdoc to be clear.

https://codereview.chromium.org/2703663002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:268:
if (!this.displayState_.available || this.displayingImage_) {
On 2017/02/22 18:15:24, David Tseng wrote:
> Seems like you should get this to work without keeping explicit track of
whether
> an image is displayed or not.

They will have pretty different panning behavior, though, right?

[dmazzoni, 2017-02-24 00:19:46]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 00:20:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 02:17:32]

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

[commit-bot: I haz the power, 2017-02-24 02:17:33]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2017-02-24 05:39:46]

Sorry; many comments enclosed.

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

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:72:
*     we're displaying text.
No longer needed.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:134:
* Takes an image, in the form of a data url, and displays it in braille
Would be helpful to include a note of how this is done e.g. maps a pixel to a
braille dot so we "increase" the image to a multiple of the braille cell's
dimensions. Good to remember, for example, when someone wants to do 2x3 cells.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:139:
this.displayingImage_ = true;
Move this below the check for availability.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:153:
canvas.height = rows * 4;
It would be helpful to name these constants or to add a comment why these values
were picked. i.e. BRAILLE_CELL_WIDTH and BRAILLE_CELL_HEIGHT (and use them
below).

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:161:
// Show if the alpha value is visible (above 20) and the pixel is not
This doesn't seem to match the below check. They're also reversed with the order
they're checked.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:163:
var show = (data[i] +
Name each of these offsets (e.g. i + RED_OFFSET, i + GREEN_OFFSET, etc).

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:165:
data[i + 2] <= 200 * 3) &&
600 please (so it matches the comment above) and better yet, name it.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:171:
var buf = new ArrayBuffer(rows * columns);
Maybe brailleBuff?

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:173:
var coordsToBrailleDot = [0x1, 0x2, 0x4, 0x40, 0x8, 0x10, 0x20, 0x80];
This is just static data; promote to a static class member.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:177:
var index = i * columns + j;
brailleIndex

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:178:
view[index] = 0;
Not need; ArrayBuffer contents are initialized to 0.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:179:
for (var x = 0; x < 2; x++) {
x is cell width so maybe brailleCellRow

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:180:
for (var y = 0; y < 4; y++) {
Use constants (see other comment).

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:181:
if (outputData[((i * columns * 4 + j + y * columns) * 2 + x)])
Can you distribute the index here and add a comment.

Also, be consistent with bracing (use them here since the rest of the file
does).

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:277:
if (!this.displayState_.available || this.displayingImage_) {
Returns also if braille is frozen.

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

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:594:
imageNode.getImageData(0, 0);
I think we should install the event handler directly on this node here so the
code to handle this isn't hidden elsewhere. Also, if we do that, you don't have
to explicitly track the node on ChromeVoxState.

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

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:421:
onImageFrameUpdated: function(evt) {
Please move this event handler to CommandHandler (see other comment).

[dmazzoni, 2017-02-27 06:56:24]

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

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:72:
*     we're displaying text.
On 2017/02/24 05:39:46, David Tseng wrote:
> No longer needed.

Done.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:139:
this.displayingImage_ = true;
On 2017/02/24 05:39:45, David Tseng wrote:
> Move this below the check for availability.

Done.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:153:
canvas.height = rows * 4;
On 2017/02/24 05:39:45, David Tseng wrote:
> It would be helpful to name these constants or to add a comment why these
values
> were picked. i.e. BRAILLE_CELL_WIDTH and BRAILLE_CELL_HEIGHT (and use them
> below). 

Done.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:161:
// Show if the alpha value is visible (above 20) and the pixel is not
On 2017/02/24 05:39:46, David Tseng wrote:
> This doesn't seem to match the below check. They're also reversed with the
order
> they're checked.

Cleaned up, and also made it more easy to justify by
using a standard formula for luminance and thresholding
at 50%.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:163:
var show = (data[i] +
On 2017/02/24 05:39:46, David Tseng wrote:
> Name each of these offsets (e.g. i + RED_OFFSET, i + GREEN_OFFSET, etc).

I did it slightly differently but should be just as
readable.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:165:
data[i + 2] <= 200 * 3) &&
On 2017/02/24 05:39:45, David Tseng wrote:
> 600 please (so it matches the comment above) and better yet, name it.

Should be clear now, let me know.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:171:
var buf = new ArrayBuffer(rows * columns);
On 2017/02/24 05:39:45, David Tseng wrote:
> Maybe brailleBuff?

brailleBuf to match elsewhere in the file

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:173:
var coordsToBrailleDot = [0x1, 0x2, 0x4, 0x40, 0x8, 0x10, 0x20, 0x80];
On 2017/02/24 05:39:46, David Tseng wrote:
> This is just static data; promote to a static class member.

Done.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:177:
var index = i * columns + j;
On 2017/02/24 05:39:46, David Tseng wrote:
> brailleIndex

Done.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:178:
view[index] = 0;
On 2017/02/24 05:39:46, David Tseng wrote:
> Not need; ArrayBuffer contents are initialized to 0.

Done.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:179:
for (var x = 0; x < 2; x++) {
On 2017/02/24 05:39:46, David Tseng wrote:
> x is cell width so maybe brailleCellRow

Done.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:180:
for (var y = 0; y < 4; y++) {
On 2017/02/24 05:39:46, David Tseng wrote:
> Use constants (see other comment).

Done.

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:181:
if (outputData[((i * columns * 4 + j + y * columns) * 2 + x)])
On 2017/02/24 05:39:46, David Tseng wrote:
> Can you distribute the index here and add a comment.
> 
> Also, be consistent with bracing (use them here since the rest of the file
> does).

Cleaned up, how's this?

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:277:
if (!this.displayState_.available || this.displayingImage_) {
On 2017/02/24 05:39:46, David Tseng wrote:
> Returns also if braille is frozen.

Freezing is handled at a higher level than this, though...

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

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:594:
imageNode.getImageData(0, 0);
On 2017/02/24 05:39:46, David Tseng wrote:
> I think we should install the event handler directly on this node here so the
> code to handle this isn't hidden elsewhere. Also, if we do that, you don't
have
> to explicitly track the node on ChromeVoxState.

Done.

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

https://codereview.chromium.org/2703663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js:421:
onImageFrameUpdated: function(evt) {
On 2017/02/24 05:39:46, David Tseng wrote:
> Please move this event handler to CommandHandler (see other comment).

Done.

[dmazzoni, 2017-02-27 07:03:19]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 07:03:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 09:00:19]

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

[commit-bot: I haz the power, 2017-02-27 09:00:20]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2017-02-27 16:15:50]

lgtm once all comments are addressed.

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

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:213:
for (var x = 0; x < cellWidth; x++) {
Rename x to cellColumn

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:214:
for (var y = 0; y < cellHeight; y++) {
y is a cellRow

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/chromevox/background/keymaps/next_keymap.json
(right):

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox/background/keymaps/next_keymap.json:734:
"ctrlKey": [true]
We're using Search+Ctrl+<letter> mostly for list of control type (e.g. links).
Maybe alt rather than ctrl?

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

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:813:
if (target.imageDataUrl) {
If the current range is no longer the image node, remove th elistener

[David Tseng, 2017-02-27 16:27:13]

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

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:795:
if (CommandHandler.imageNode_) {
I don't think you need to track the image node (and this won't catch the case
where someone navigates away from the node).

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:806:
if (!imageNode)
If the current range already has our event listener added, skip the code.

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:820:
CommandHandler.imageNode_ = {node: imageNode, listener: listener};
Set current range to the image node and don't keep the image node as state..

[dmazzoni, 2017-02-28 05:08:23]

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

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:213:
for (var x = 0; x < cellWidth; x++) {
On 2017/02/27 16:15:49, David Tseng wrote:
> Rename x to cellColumn

Done.

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:214:
for (var y = 0; y < cellHeight; y++) {
On 2017/02/27 16:15:49, David Tseng wrote:
> y is a cellRow

Done.

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/chromevox/background/keymaps/next_keymap.json
(right):

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox/background/keymaps/next_keymap.json:734:
"ctrlKey": [true]
On 2017/02/27 16:15:49, David Tseng wrote:
> We're using Search+Ctrl+<letter> mostly for list of control type (e.g. links).
> Maybe alt rather than ctrl?

Done.

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

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:795:
if (CommandHandler.imageNode_) {
On 2017/02/27 16:27:13, David Tseng wrote:
> I don't think you need to track the image node (and this won't catch the case
> where someone navigates away from the node).

Fixed as discussed offline. Tracking the image node but not
the listener. Cleans up the listener if we get a callback
and it's not within the range anymore.

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:813:
if (target.imageDataUrl) {
On 2017/02/27 16:15:50, David Tseng wrote:
> If the current range is no longer the image node, remove th elistener

Done.

https://codereview.chromium.org/2703663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/command_handler.js:820:
CommandHandler.imageNode_ = {node: imageNode, listener: listener};
On 2017/02/27 16:27:13, David Tseng wrote:
> Set current range to the image node and don't keep the image node as state..

As discussed, this would leak too many listeners.

[dmazzoni, 2017-02-28 05:08:29]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2017-02-28 05:08:29]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2703663002/#ps80001
(title: "Final feedback")

[commit-bot: I haz the power, 2017-02-28 05:08:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 07:24:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488258509492600,
"parent_rev": "627ab487f2c43ca1721935cc4c4579ef9eff421d", "commit_rev":
"5d5130c54bb6ba7a3d1e3aa324735ab76e934eb8"}

[commit-bot: I haz the power, 2017-02-28 07:25:10]

Description was changed from

==========
Display images in multiline Braille

When ChromeVox lands on an object that's an image, and when there's a
multi-line braille display, convert's the image to black-and-white and
displays it on the braille display.

Original changelist (never landed): http://crrev.com/2544203004

BUG=670925
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Display images in multiline Braille

When ChromeVox lands on an object that's an image, and when there's a
multi-line braille display, convert's the image to black-and-white and
displays it on the braille display.

Original changelist (never landed): http://crrev.com/2544203004

BUG=670925
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2703663002
Cr-Commit-Position: refs/heads/master@{#453537}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d5130c54bb6ba7a3d1e3aa32473...
==========

[commit-bot: I haz the power, 2017-02-28 07:25:11]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5d5130c54bb6ba7a3d1e3aa32473...