Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2544203004:
Display images in multiline Braille
|
Created:
4 years ago by ultimatedbz Modified:
3 years, 10 months ago CC:
chromium-reviews, alemate+watch_chromium.org, oshima+watch_chromium.org, aboxhall+watch_chromium.org, jlklein+watch-closure_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, arv+watch_chromium.org, dtseng+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-closure_chromium.org, dmazzoni+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisplay images in multiline Braille
This change allows the code to take in an image, scale it to the dimensions of the braille display, turn it to black and white, and then convert that data into braille, and then
output it onto the virtual braille captions display.
The algorithm that turns the image to black and white is very basic. If the pixel is white or transparent, I don't put a
dot, otherwise I do.
BUG=670925
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Patch Set 1 #Patch Set 2 : set upstream to correct branch #
Total comments: 32
Patch Set 3 : Addressed Comments #
Total comments: 4
Patch Set 4 : Addressed comments #Patch Set 5 : Rebased back onto Master #
Total comments: 6
Patch Set 6 : Minor Nits, added TODO comments #
Total comments: 1
Messages
Total messages: 34 (15 generated)
Description was changed from ========== Display images in multiline Braille This is an in progress code review. The code is very rough, but is able to take in an image, turn it to black and white, scale it to be the dimensions of the current displaystate, and then convert that data into braille, and then output it onto the virtual braille captions display. The algorithm that turns the image to black and white will probably need fine tuning, the kmeans algorithm seems like it might work. Right now, I am going through pixel by pixel. If the pixel is white or transparent, I don't put a dot, otherwise I do. I also had some trouble with the presubmit message. It said node.imageDataUrl was not defined, which led me to the generated code, which didn't have the imageDataUrl. After hardcoding imageDataUrl and getImageData(), the presubmit passed, but this is something we shoud look at. Thanks! BUG=670925 ========== to ========== Display images in multiline Braille This is an in progress code review. The code is very rough, but is able to take in an image, turn it to black and white, scale it to be the dimensions of the current displaystate, and then convert that data into braille, and then output it onto the virtual braille captions display. The algorithm that turns the image to black and white will probably need fine tuning, the kmeans algorithm seems like it might work. Right now, I am going through pixel by pixel. If the pixel is white or transparent, I don't put a dot, otherwise I do. I also had some trouble with the presubmit message. It said node.imageDataUrl was not defined, which led me to the generated code, which didn't have the imageDataUrl. After hardcoding imageDataUrl and getImageData(), the presubmit passed, but this is something we shoud look at. Thanks! BUG=670925 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Display images in multiline Braille This is an in progress code review. The code is very rough, but is able to take in an image, turn it to black and white, scale it to be the dimensions of the current displaystate, and then convert that data into braille, and then output it onto the virtual braille captions display. The algorithm that turns the image to black and white will probably need fine tuning, the kmeans algorithm seems like it might work. Right now, I am going through pixel by pixel. If the pixel is white or transparent, I don't put a dot, otherwise I do. I also had some trouble with the presubmit message. It said node.imageDataUrl was not defined, which led me to the generated code, which didn't have the imageDataUrl. After hardcoding imageDataUrl and getImageData(), the presubmit passed, but this is something we shoud look at. Thanks! BUG=670925 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Display images in multiline Braille This is an in progress code review. The code is very rough, but is able to take in an image, turn it to black and white, scale it to be the dimensions of the current displaystate, and then convert that data into braille, and then output it onto the virtual braille captions display. The algorithm that turns the image to black and white will probably need fine tuning, the kmeans algorithm seems like it might work. Right now, I am going through pixel by pixel. If the pixel is white or transparent, I don't put a dot, otherwise I do. I also had some trouble with the presubmit message. It said node.imageDataUrl was not defined, which led me to the generated code, which didn't have the imageDataUrl. After hardcoding imageDataUrl and getImageData(), the presubmit passed, but this is something we shoud look at. Thanks! This branch branches off the multiline panning branch, and it seems like the code from that ticket is included in this CL... How do we remove that code from the code review? BUG=670925 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
ultimatedbz@google.com changed reviewers: + dmazzoni@chromium.org, dtseng@google.com
Description was changed from ========== Display images in multiline Braille This is an in progress code review. The code is very rough, but is able to take in an image, turn it to black and white, scale it to be the dimensions of the current displaystate, and then convert that data into braille, and then output it onto the virtual braille captions display. The algorithm that turns the image to black and white will probably need fine tuning, the kmeans algorithm seems like it might work. Right now, I am going through pixel by pixel. If the pixel is white or transparent, I don't put a dot, otherwise I do. I also had some trouble with the presubmit message. It said node.imageDataUrl was not defined, which led me to the generated code, which didn't have the imageDataUrl. After hardcoding imageDataUrl and getImageData(), the presubmit passed, but this is something we shoud look at. Thanks! This branch branches off the multiline panning branch, and it seems like the code from that ticket is included in this CL... How do we remove that code from the code review? BUG=670925 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Display images in multiline Braille This is an in progress code review. The code is very rough, but is able to take in an image, turn it to black and white, scale it to be the dimensions of the current displaystate, and then convert that data into braille, and then output it onto the virtual braille captions display. The algorithm that turns the image to black and white will probably need fine tuning, the kmeans algorithm seems like it might work. Right now, I am going through pixel by pixel. If the pixel is white or transparent, I don't put a dot, otherwise I do. I also had some trouble with the presubmit message. It said node.imageDataUrl was not defined, which led me to the generated code, which didn't have the imageDataUrl. After hardcoding imageDataUrl and getImageData(), the presubmit passed, but this is something we shoud look at. Thanks! BUG=670925 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Display images in multiline Braille This is an in progress code review. The code is very rough, but is able to take in an image, turn it to black and white, scale it to be the dimensions of the current displaystate, and then convert that data into braille, and then output it onto the virtual braille captions display. The algorithm that turns the image to black and white will probably need fine tuning, the kmeans algorithm seems like it might work. Right now, I am going through pixel by pixel. If the pixel is white or transparent, I don't put a dot, otherwise I do. I also had some trouble with the presubmit message. It said node.imageDataUrl was not defined, which led me to the generated code, which didn't have the imageDataUrl. After hardcoding imageDataUrl and getImageData(), the presubmit passed, but this is something we shoud look at. Thanks! BUG=670925 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Display images in multiline Braille This is an in progress code review. The code is very rough, but is able to take in an image, turn it to black and white, scale it to be the dimensions of the current displaystate, and then convert that data into braille, and then output it onto the virtual braille captions display. The algorithm that turns the image to black and white will probably need fine tuning, the kmeans algorithm seems like it might work. Right now, I am going through pixel by pixel. If the pixel is white or transparent, I don't put a dot, otherwise I do. I also had some trouble with the presubmit message. It said node.imageDataUrl was not defined, which led me to the generated code, which didn't have the imageDataUrl. After hardcoding imageDataUrl and getImageData(), the presubmit passed, but this is something we shoud look at. Thanks! Within the code, I also switched from using local storage to chrome.storage.local, to store VirtualBrailleRows and VirtualBrailleColumns. BUG=670925 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Try editing chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js and add an event handler for imageFrameUpdated that calls onEventDefault. That might be sufficient to get the image to show up immediately rather than needing to focus it twice https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:78: if (area == 'local' && ('virtualBrailleRows' in changes || This should probably be part of the previous change, or a separate change https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:126: * Takes a imageDataUrl and displays it in braille onto the physical braille Takes an image, in the form of a data url, https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:128: * @param {!string} imageUrl The imageDataUrl of the image to display. Same, "The image, in the form of a data url" or something like that https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:136: var c1 = document.createElement('canvas'); var canvas https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:145: var c2 = document.createElement('canvas'); You probably don't need two canvases for this, right? I think you can do it in-place https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:165: // Scaled down to braille dimensions. I think it'd look better to rescale first, then convert to black and white. You can rescale up above where you call context.drawImage() - basically you can just call scale() on the canvas context if you want the image to be drawn smaller or larger. So I think you can skip this step entirely. https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:190: // Covert to Braille. Convert https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:208: if (data3[(i * columns * 4 + j) * 2 * 4] == 255) I'd do something like this: coordsToBrailleDot[0][0] = 0x1; ... for (int x = 0; x < 2; x++) { for (int y = 0; y < 4; y++) { if (data3[((i * columns * 4 + j + y * columns) * 2 + x) * 4] == 255) view[index] += coordsToBrailleDot[x][y]; } } https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:228: if (!this.displayState_.available) { Put this check at the top https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:109: var groups = [['Unused', brailleChars]]; Maybe it should say "Image" https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:110: if (cvox.ChromeVox.isChromeOS) { You don't need to worry about this being false, the other code path is historical https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:55: chrome.storage.local.get({'virtualBrailleRows' : 1}, function(items) { Same - put this in a separate change https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1015: node.getImageData(0, 0); This should be inside the if node != null. Also, only call this if node.role == Role.canvas or node.role == Role.image. https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:548: var brailleText = groups[i][1]; Are these changes for images or something else? https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/host/interface/braille_interface.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/host/interface/braille_interface.js:32: * Takes in a dataUrl in the form of a PNG and outputs it into a Braille I'd say, takes an image (PNG is irrelevant) in the form of a data url https://codereview.chromium.org/2544203004/diff/20001/third_party/closure_com... File third_party/closure_compiler/externs/automation.js (right): https://codereview.chromium.org/2544203004/diff/20001/third_party/closure_com... third_party/closure_compiler/externs/automation.js:435: * @type {string} How come this wasn't auto generated? :) The change to this file is fine, we don't have a script to automatically regenerate it yet. No comment needed
https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:126: * Takes a imageDataUrl and displays it in braille onto the physical braille On 2016/12/05 17:29:40, dmazzoni wrote: > Takes an image, in the form of a data url, Done. https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:128: * @param {!string} imageUrl The imageDataUrl of the image to display. On 2016/12/05 17:29:40, dmazzoni wrote: > Same, "The image, in the form of a data url" or something like that Done. https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:136: var c1 = document.createElement('canvas'); On 2016/12/05 17:29:40, dmazzoni wrote: > var canvas Done. https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:145: var c2 = document.createElement('canvas'); On 2016/12/05 17:29:40, dmazzoni wrote: > You probably don't need two canvases for this, right? I > think you can do it in-place Done. https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:165: // Scaled down to braille dimensions. On 2016/12/05 17:29:40, dmazzoni wrote: > I think it'd look better to rescale first, then > convert to black and white. You can rescale up above > where you call context.drawImage() - basically you > can just call scale() on the canvas context if you > want the image to be drawn smaller or larger. So I > think you can skip this step entirely. > That's awesome! You can actually specify how big you want to draw the image in context.drawImage(), so ' canvas1.width = columns * 2; canvas1.height = rows * 4; context.drawImage(imgElement, 0, 0, canvas1.width, canvas1.height); ' Did the trick! https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:190: // Covert to Braille. On 2016/12/05 17:29:40, dmazzoni wrote: > Convert Done. https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:208: if (data3[(i * columns * 4 + j) * 2 * 4] == 255) On 2016/12/05 17:29:40, dmazzoni wrote: > I'd do something like this: > > coordsToBrailleDot[0][0] = 0x1; > ... > > for (int x = 0; x < 2; x++) { > for (int y = 0; y < 4; y++) { > if (data3[((i * columns * 4 + j + y * columns) * 2 + x) * 4] == 255) > view[index] += coordsToBrailleDot[x][y]; > } > } This was really clever. On top of this, I made some other optimizations too! https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:228: if (!this.displayState_.available) { On 2016/12/05 17:29:40, dmazzoni wrote: > Put this check at the top Done. https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:109: var groups = [['Unused', brailleChars]]; On 2016/12/05 17:29:41, dmazzoni wrote: > Maybe it should say "Image" Done. https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:110: if (cvox.ChromeVox.isChromeOS) { On 2016/12/05 17:29:40, dmazzoni wrote: > You don't need to worry about this being false, > the other code path is historical Is chromeVox supported on other OS's besides ChromeOS? https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1015: node.getImageData(0, 0); On 2016/12/05 17:29:41, dmazzoni wrote: > This should be inside the if node != null. > > Also, only call this if node.role == Role.canvas or node.role == Role.image. Done. https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:548: var brailleText = groups[i][1]; On 2016/12/05 17:29:41, dmazzoni wrote: > Are these changes for images or something else? These are for images. Before, if a group of braille characters didn't fit a line, I'd move the rest of it to the next line, assuming incorrectly that the next line would be enough to fit the rest of the braille characters. This bug became apparent when the braille image displayed would only be two lines, the first line being correct, and the rest of the image being put on the second line. https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/host/interface/braille_interface.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/host/interface/braille_interface.js:32: * Takes in a dataUrl in the form of a PNG and outputs it into a Braille On 2016/12/05 17:29:41, dmazzoni wrote: > I'd say, takes an image (PNG is irrelevant) in the form of a data url Done. https://codereview.chromium.org/2544203004/diff/20001/third_party/closure_com... File third_party/closure_compiler/externs/automation.js (right): https://codereview.chromium.org/2544203004/diff/20001/third_party/closure_com... third_party/closure_compiler/externs/automation.js:435: * @type {string} How come this wasn't auto generated? On 2016/12/05 17:29:41, dmazzoni wrote: > :) > > The change to this file is fine, we don't have a script > to automatically regenerate it yet. > > No comment needed Done.
https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:78: if (area == 'local' && ('virtualBrailleRows' in changes || On 2016/12/05 17:29:40, dmazzoni wrote: > This should probably be part of the previous change, or > a separate change Done. https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js (right): https://codereview.chromium.org/2544203004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:55: chrome.storage.local.get({'virtualBrailleRows' : 1}, function(items) { On 2016/12/05 17:29:41, dmazzoni wrote: > Same - put this in a separate change Done.
lgtm I had one last suggestion, maybe you missed it before. Try editing chrome/browser/resources/chromeos/chromevox/cvox2/background/desktop_automation_handler.js and add an event handler for imageFrameUpdated that calls onEventDefault. That might be sufficient to get the image to show up immediately rather than needing to focus it twice. Just one other comment, otherwise this code looks good! https://codereview.chromium.org/2544203004/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (right): https://codereview.chromium.org/2544203004/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1017: node.getImageData(0, 0); Oh, I just realized this is going to keep fetching it over and over again. Maybe that's good for animation, but probably better would be: if (node.imageDataUrl) this.imageDataUrl_ = node.imageDataUrl; else node.getImageData(0, 0);
https://codereview.chromium.org/2544203004/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2544203004/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:136: var canvas1 = document.createElement('canvas'); Nit: maybe just |canvas| since you don't have more than one of the anymore
https://codereview.chromium.org/2544203004/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2544203004/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:136: var canvas1 = document.createElement('canvas'); On 2016/12/06 07:39:42, dmazzoni wrote: > Nit: maybe just |canvas| since you don't have more than one of the anymore Done. https://codereview.chromium.org/2544203004/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (right): https://codereview.chromium.org/2544203004/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1017: node.getImageData(0, 0); On 2016/12/06 07:39:06, dmazzoni wrote: > Oh, I just realized this is going to keep fetching it over and over again. > Maybe that's good for animation, but probably better would be: > > if (node.imageDataUrl) > this.imageDataUrl_ = node.imageDataUrl; > else > node.getImageData(0, 0); Done.
Update the change description and run try jobs? If clean this looks ready to go in.
Description was changed from ========== Display images in multiline Braille This is an in progress code review. The code is very rough, but is able to take in an image, turn it to black and white, scale it to be the dimensions of the current displaystate, and then convert that data into braille, and then output it onto the virtual braille captions display. The algorithm that turns the image to black and white will probably need fine tuning, the kmeans algorithm seems like it might work. Right now, I am going through pixel by pixel. If the pixel is white or transparent, I don't put a dot, otherwise I do. I also had some trouble with the presubmit message. It said node.imageDataUrl was not defined, which led me to the generated code, which didn't have the imageDataUrl. After hardcoding imageDataUrl and getImageData(), the presubmit passed, but this is something we shoud look at. Thanks! Within the code, I also switched from using local storage to chrome.storage.local, to store VirtualBrailleRows and VirtualBrailleColumns. BUG=670925 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Display images in multiline Braille This change allows the code to take in an image, scale it to the dimensions of the braille display, turn it to black and white, and then convert that data into braille, and then output it onto the virtual braille captions display. The algorithm that turns the image to black and white is very basic. If the pixel is white or transparent, I don't put a dot, otherwise I do. BUG=670925 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by ultimatedbz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2544203004/#ps80001 (title: "Rebased back onto Master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dtseng@chromium.org changed reviewers: + dtseng@chromium.org
https://codereview.chromium.org/2544203004/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2544203004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:147: * @param {!string} imageUrl The image, in the form of a data url. primitive type doesn't require nullable i.e. @param {string} is the same as @param {!string} https://codereview.chromium.org/2544203004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:186: //Index in braille array nit: period at end. https://codereview.chromium.org/2544203004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:259: if (!this.displayState_.available || this.displayingImage_) { Panning support in v2? TODO(dmazzoni) perhaps? You could just override by using fixed panning and make this work for an actual braille display.
dmazzoni@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam for third_party/closure_compiler/externs/automation.js
https://codereview.chromium.org/2544203004/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (right): https://codereview.chromium.org/2544203004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1020: node.getImageData(0, 0); This is going to be pretty strange in terms of feedback. The user will see whatever is the default output for an image (e.g. name + role), then get a non-panning image pattern. I would hook this up as a command (command_handler.js) that gets triggered via a keyboard shortcut (e.g. showTactleImage). Add the key binding in chromevox/background/keymaps/next_keymap.json.
On 2016/12/08 17:09:21, David Tseng wrote: > This is going to be pretty strange in terms of feedback. The user will see > whatever is the default output for an image (e.g. name + role), then get a > non-panning image pattern. > > I would hook this up as a command (command_handler.js) that gets triggered via a > keyboard shortcut (e.g. showTactleImage). Add the key binding in > chromevox/background/keymaps/next_keymap.json. Agreed, I wouldn't want to ship what we have here, but maybe as a first step let's have this code trigger only if you have a multi-line display, then we can add a command and once we have panning support it might be worth exploring on a single-line display too.
https://codereview.chromium.org/2544203004/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2544203004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:147: * @param {!string} imageUrl The image, in the form of a data url. On 2016/12/08 16:44:22, David Tseng wrote: > primitive type doesn't require nullable i.e. @param {string} is the same as > @param {!string} Done. https://codereview.chromium.org/2544203004/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:186: //Index in braille array On 2016/12/08 16:44:22, David Tseng wrote: > nit: period at end. Done.
where was the corresponding .idl change?
The IDL change was here: https://codereview.chromium.org/2522543004 This is the first time we're calling the new API I added from ChromeVox so that's why we need it in the externs now. On Thu, Dec 8, 2016 at 11:35 AM <dbeam@chromium.org> wrote: > where was the corresponding .idl change? > > https://codereview.chromium.org/2544203004/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by ultimatedbz@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
lgtm
not lgtm https://codereview.chromium.org/2544203004/diff/100001/third_party/closure_co... File third_party/closure_compiler/externs/automation.js (right): https://codereview.chromium.org/2544203004/diff/100001/third_party/closure_co... third_party/closure_compiler/externs/automation.js:561: function(width, height) {}; if this were run by the script on the top of this file, these variable names would be maxWidth and maxHeight. please either figure out a way to run the script (preferred), file bugs as to why it doesn't work, or remove the note about generation from the top of this file (and potentially move the file somewhere else). |
