Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2401393003:
Add multiline braille support. (Closed)
|
Created:
4 years, 2 months ago by ultimatedbz Modified:
4 years, 1 month ago CC:
chromium-reviews, extensions-reviews_chromium.org, alemate+watch_chromium.org, oshima+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, chromium-apps-reviews_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd multiline braille support.
Added code to take in 1D array and also row and column count.
Code compiles up to auto generated code. Cannot figure out how to generate a vector of vectors of chars. Also don't know where is appropriate to add in col and row information. Also do no know if input is assumed to be 1D or 2D
BUG=651570
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/ffd46520fa89b84acb6a829f1b3d3b183477f1ad
Cr-Commit-Position: refs/heads/master@{#431444}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Got braille cells to show up on braille display #
Total comments: 2
Patch Set 3 : Addressed review comments #
Total comments: 8
Patch Set 4 : Fixed tests and addressed comments #
Total comments: 15
Patch Set 5 : Addressed comments #
Total comments: 6
Patch Set 6 : minor nit #Patch Set 7 : Validated params #Patch Set 8 : Fixed Test by typecasting size. #Patch Set 9 : updated braille_display_private_apitest #Patch Set 10 : updated some tests #Patch Set 11 : Update more tests #Patch Set 12 : more tests #Messages
Total messages: 67 (29 generated)
Description was changed from ========== Add multiline braille support. Added code to take in 1D array and also row and column count. Code compiles up to auto generated code. Cannot figure out how to generate a vector of vectors of chars. Also don't know where is appropriate to add in col and row information. Also do no know if input is assumed to be 1D or 2D BUG=651570 ========== to ========== Add multiline braille support. Added code to take in 1D array and also row and column count. Code compiles up to auto generated code. Cannot figure out how to generate a vector of vectors of chars. Also don't know where is appropriate to add in col and row information. Also do no know if input is assumed to be 1D or 2D BUG=651570 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Add multiline braille support. Added code to take in 1D array and also row and column count. Code compiles up to auto generated code. Cannot figure out how to generate a vector of vectors of chars. Also don't know where is appropriate to add in col and row information. Also do no know if input is assumed to be 1D or 2D BUG=651570 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add multiline braille support. Added code to take in 1D array and also row and column count. Code compiles up to auto generated code. Cannot figure out how to generate a vector of vectors of chars. Also don't know where is appropriate to add in col and row information. Also do no know if input is assumed to be 1D or 2D BUG=651570 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
ultimatedbz@google.com changed reviewers: + dmazzoni@chromium.org
https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc (right): https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:103: void BrailleControllerImpl::WriteDots(const std::vector<std::vector<char>>& I'd probably indent it like this: void BrailleControllerImpl::WriteDots( const std::vector<std::vector<char>>& cells) { If you run "git cl format" it should indent it automatically. If it suggested this indentation then that's fine. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:111: std::vector<std::vector<unsigned char>> sizedCells(rows, sizedCells -> sized_cells https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:113: for (unsigned int i = 0; i < rows; i++) { I think you need to handle the case where cells.size() < rows, just like you're handling the case where a single row has fewer than |columns| columns. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:115: (unsigned long)columns)); Use a C++-style cast, and I think size_t is the correct type to use: static_cast<size_t>(columns) https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:116: if (columns > cells[i].size()) For readability, you need braces {} if there's more than one line inside the "if" block. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc (right): https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc:179: const std::vector<std::vector<unsigned char>> cells) { This should be indented 4 spaces https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc:183: if (libbrlapi_loader_->brlapi__writeDots(handle_.get(), &cells[0][0]) < 0) { I could be wrong but I think it just wants a single 1-D array with |rows * columns| bytes https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/braille_display_private/brlapi_connection.h (right): https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/brlapi_connection.h:54: // Gets the total size of the display, which may be 0 if no display is Update this comment https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/brlapi_connection.h:59: // Sends the specified cells to the display. The array size must Update the comment https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/brlapi_connection.h:61: // virtual bool WriteDots(const std::vector<unsigned char> cells) = 0; Delete the old commented-out line https://codereview.chromium.org/2401393003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2401393003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:160: var oldSize = this.displayState_.textCellCount || 0; You replaced textCellCount with textRowCount and textColumnCount, so you'll need to update this and every other place where textCellCount was used in JavaScript. See comment in externs.js https://codereview.chromium.org/2401393003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:196: chrome.brailleDisplayPrivate.writeDots(buf, 1, buf.byteLength); Ideally you can change all of the code inside ChromeVox to keep track of rows and columns too https://codereview.chromium.org/2401393003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/host/chrome/externs.js (right): https://codereview.chromium.org/2401393003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/host/chrome/externs.js:11: * @param {function(!{available: boolean, textCellCount: (number|undefined)})} If you update this to textRowCount and textColumnCount, then the JavaScript compiler should automatically catch any place in the code where you access textCellCount instead of those.
https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc (right): https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:103: void BrailleControllerImpl::WriteDots(const std::vector<std::vector<char>>& On 2016/10/11 17:15:39, dmazzoni wrote: > I'd probably indent it like this: > > void BrailleControllerImpl::WriteDots( > const std::vector<std::vector<char>>& cells) { > > If you run "git cl format" it should indent it automatically. If it suggested > this indentation then that's fine. > Cool! didn't know about git cl format! https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:103: void BrailleControllerImpl::WriteDots(const std::vector<std::vector<char>>& On 2016/10/11 17:15:39, dmazzoni wrote: > I'd probably indent it like this: > > void BrailleControllerImpl::WriteDots( > const std::vector<std::vector<char>>& cells) { > > If you run "git cl format" it should indent it automatically. If it suggested > this indentation then that's fine. > Done. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:111: std::vector<std::vector<unsigned char>> sizedCells(rows, On 2016/10/11 17:15:39, dmazzoni wrote: > sizedCells -> sized_cells Done. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:113: for (unsigned int i = 0; i < rows; i++) { On 2016/10/11 17:15:39, dmazzoni wrote: > I think you need to handle the case where cells.size() < rows, just like you're > handling the case where a single row has fewer than |columns| columns. Done. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:115: (unsigned long)columns)); On 2016/10/11 17:15:39, dmazzoni wrote: > Use a C++-style cast, and I think size_t is the correct type to use: > > static_cast<size_t>(columns) Done. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:116: if (columns > cells[i].size()) On 2016/10/11 17:15:39, dmazzoni wrote: > For readability, you need braces {} if there's more than one line inside the > "if" block. Done. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc (right): https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc:179: const std::vector<std::vector<unsigned char>> cells) { On 2016/10/11 17:15:39, dmazzoni wrote: > This should be indented 4 spaces Done. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc:183: if (libbrlapi_loader_->brlapi__writeDots(handle_.get(), &cells[0][0]) < 0) { On 2016/10/11 17:15:39, dmazzoni wrote: > I could be wrong but I think it just wants a single 1-D array with |rows * > columns| bytes Done. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/braille_display_private/brlapi_connection.h (right): https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/brlapi_connection.h:54: // Gets the total size of the display, which may be 0 if no display is On 2016/10/11 17:15:39, dmazzoni wrote: > Update this comment Done. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/brlapi_connection.h:59: // Sends the specified cells to the display. The array size must On 2016/10/11 17:15:39, dmazzoni wrote: > Update the comment Done. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/braille_display_private/brlapi_connection.h:61: // virtual bool WriteDots(const std::vector<unsigned char> cells) = 0; On 2016/10/11 17:15:39, dmazzoni wrote: > Delete the old commented-out line Done. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2401393003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:160: var oldSize = this.displayState_.textCellCount || 0; On 2016/10/11 17:15:39, dmazzoni wrote: > You replaced textCellCount with textRowCount and textColumnCount, so you'll need > to update > this and every other place where textCellCount was used in JavaScript. > > See comment in externs.js Done. https://codereview.chromium.org/2401393003/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/host/chrome/externs.js (right): https://codereview.chromium.org/2401393003/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/host/chrome/externs.js:11: * @param {function(!{available: boolean, textCellCount: (number|undefined)})} On 2016/10/11 17:15:39, dmazzoni wrote: > If you update this to textRowCount and textColumnCount, then the JavaScript > compiler should automatically catch any place in the code where you access > textCellCount instead of those. Done.
lgtm Looks great, just one formatting suggestion then ready to commit. https://codereview.chromium.org/2401393003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc (right): https://codereview.chromium.org/2401393003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:104: unsigned int oldCols, "old" is a little confusing to me, I know what you mean but it's not really "old", it's just the rows and columns of the passed-in array. How about cells_cols, cells_rows for the input arguments?
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/2401393003/#ps40001 (title: "Addressed review comments")
https://codereview.chromium.org/2401393003/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc (right): https://codereview.chromium.org/2401393003/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:104: unsigned int oldCols, On 2016/10/17 05:13:05, dmazzoni wrote: > "old" is a little confusing to me, I know what you mean but it's not really > "old", it's just the rows and columns of the passed-in array. How about > cells_cols, cells_rows for the input arguments? Done.
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...)
dmazzoni@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+Devlin for OWNERS review of chrome/browser/extensions/api
just a couple small things, but looks like this also doesn't make it through the CQ with green bots yet? https://codereview.chromium.org/2401393003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc (right): https://codereview.chromium.org/2401393003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:119: : cells[i * cells_cols + j]; nit: why not init the vector to all zeros (std::vector<unsigned char> sized_cells(rows * columns, 0);) and then just iterate from 0 -> cells_[rows|cols]? https://codereview.chromium.org/2401393003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc (right): https://codereview.chromium.org/2401393003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc:104: unsigned int rows, columns; chromium style - only one var per line, initialize all pod vars. https://codereview.chromium.org/2401393003/diff/40001/chrome/common/extension... File chrome/common/extensions/api/braille_display_private.idl (right): https://codereview.chromium.org/2401393003/diff/40001/chrome/common/extension... chrome/common/extensions/api/braille_display_private.idl:72: static void writeDots(ArrayBuffer cells, long columns, long rows); document new params columns and rows?
Looks like mock_braille_controller.cc needs to be updated
https://codereview.chromium.org/2401393003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc (right): https://codereview.chromium.org/2401393003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:119: : cells[i * cells_cols + j]; On 2016/10/18 23:01:28, Devlin wrote: > nit: why not init the vector to all zeros (std::vector<unsigned char> > sized_cells(rows * columns, 0);) and then just iterate from 0 -> > cells_[rows|cols]? That's a great suggestion! As I was thinking about it though, I thought of the case where cells_[row|cols] is bigger than [rows|columns]. With your suggestion, wouldn't I still need to do some bounds checking and iterate from 0 -> min(cells_rows, rows)? https://codereview.chromium.org/2401393003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc (right): https://codereview.chromium.org/2401393003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc:104: unsigned int rows, columns; On 2016/10/18 23:01:28, Devlin wrote: > chromium style - only one var per line, initialize all pod vars. Done. https://codereview.chromium.org/2401393003/diff/40001/chrome/common/extension... File chrome/common/extensions/api/braille_display_private.idl (right): https://codereview.chromium.org/2401393003/diff/40001/chrome/common/extension... chrome/common/extensions/api/braille_display_private.idl:72: static void writeDots(ArrayBuffer cells, long columns, long rows); On 2016/10/18 23:01:29, Devlin wrote: > document new params columns and rows? Done.
https://codereview.chromium.org/2401393003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc (right): https://codereview.chromium.org/2401393003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:119: : cells[i * cells_cols + j]; On 2016/10/20 18:27:45, ultimatedbz wrote: > On 2016/10/18 23:01:28, Devlin wrote: > > nit: why not init the vector to all zeros (std::vector<unsigned char> > > sized_cells(rows * columns, 0);) and then just iterate from 0 -> > > cells_[rows|cols]? > > That's a great suggestion! As I was thinking about it though, I thought of the > case where cells_[row|cols] is bigger than [rows|columns]. With your suggestion, > wouldn't I still need to do some bounds checking and iterate from 0 -> > min(cells_rows, rows)? Ah, didn't realize that cells_rows could be bigger than rows. Slightly-tweaked suggestion: std::vector<unsigned char> sized_cells(rows * columns, 0); unsigned int row_limit = std::min(rows, cells_rows); unisgned int col_limit = std::min(cols, cells_cols); for (unsigned int row = 0; row < row_limit; ++row) { for (unsinged int col = 0; col < col_limit; ++col) { sized_cells[row * columns + col] = cells[row * cells_cols + col]; }
https://codereview.chromium.org/2401393003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc (right): https://codereview.chromium.org/2401393003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:119: : cells[i * cells_cols + j]; Looks good. Thanks!
sorry, a few last nits. https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc (right): https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:90: unsigned int columns, rows; nit: chromium style says one variable per line, initialize to a sane default value (e.g. 0). so unsigned int columns = 0; unsigned int rows = 0; https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:109: unsigned int rows, columns; ditto https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc (right): https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc:169: if (libbrlapi_loader_->brlapi__writeDots(handle_.get(), &cells[0]) < 0) { Do we need to check if cells is empty? Also, prefer cells.data() to access the pointer to the beginning of the array, rather than &cells[0]. https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/brlapi_connection.h (right): https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/brlapi_connection.h:62: virtual bool WriteDots(const std::vector<unsigned char> cells) = 0; const &? https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... File chrome/common/extensions/api/braille_display_private.idl (right): https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... chrome/common/extensions/api/braille_display_private.idl:75: remove extra newline
dtseng@chromium.org changed reviewers: + dtseng@chromium.org
https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... File chrome/common/extensions/api/braille_display_private.idl (right): https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... chrome/common/extensions/api/braille_display_private.idl:76: static void writeDots(ArrayBuffer cells, long columns, long rows); Driving by. It seems like you don't really need the rows parameter throughout all of the bindings. The caller needs to fill the array in order to construct the flattened cells. So, rows = ceiling(cells.size() / columns), right?
https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc (right): https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:90: unsigned int columns, rows; On 2016/10/25 22:36:53, Devlin wrote: > nit: chromium style says one variable per line, initialize to a sane default > value (e.g. 0). > so > unsigned int columns = 0; > unsigned int rows = 0; Done. https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc:109: unsigned int rows, columns; On 2016/10/25 22:36:53, Devlin wrote: > ditto Done. https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc (right): https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc:169: if (libbrlapi_loader_->brlapi__writeDots(handle_.get(), &cells[0]) < 0) { On 2016/10/25 22:36:54, Devlin wrote: > Do we need to check if cells is empty? > > Also, prefer cells.data() to access the pointer to the beginning of the array, > rather than &cells[0]. Cool! I did not know vectors had a data() function! And I can't say for sure what would happen if cells was empty, but I think nothing would be drawn on the braille display, or brlapi__writeDots(...) would return a negative number. https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/brlapi_connection.h (right): https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/brlapi_connection.h:62: virtual bool WriteDots(const std::vector<unsigned char> cells) = 0; On 2016/10/25 22:36:54, Devlin wrote: > const &? Done. https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... File chrome/common/extensions/api/braille_display_private.idl (right): https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... chrome/common/extensions/api/braille_display_private.idl:75: On 2016/10/25 22:36:54, Devlin wrote: > remove extra newline Done. https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... chrome/common/extensions/api/braille_display_private.idl:76: static void writeDots(ArrayBuffer cells, long columns, long rows); On 2016/10/26 04:49:24, David Tseng wrote: > Driving by. It seems like you don't really need the rows parameter throughout > all of the bindings. The caller needs to fill the array in order to construct > the flattened cells. So, rows = ceiling(cells.size() / columns), right? Hey David! Yes, what you said definitely makes sense and we could just derive rows from the dividing the cell's size by |columns|. However, if we also pass in |rows|, would you say that it makes the code more readable and clear?
I see a bunch of "done"s, but no new patch set - could you upload the latest version? :) https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc (right): https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc:169: if (libbrlapi_loader_->brlapi__writeDots(handle_.get(), &cells[0]) < 0) { On 2016/10/27 03:16:58, ultimatedbz wrote: > On 2016/10/25 22:36:54, Devlin wrote: > > Do we need to check if cells is empty? > > > > Also, prefer cells.data() to access the pointer to the beginning of the array, > > rather than &cells[0]. > > Cool! I did not know vectors had a data() function! And I can't say for sure > what would happen if cells was empty, but I think nothing would be drawn on the > braille display, or brlapi__writeDots(...) would return a negative number. Reason I asked about checking emptiness before was because it would otherwise crash on cells[0]. Using data() should be safe as long as writeDots doesn't try to write anything with the passed data. https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... File chrome/common/extensions/api/braille_display_private.idl (right): https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... chrome/common/extensions/api/braille_display_private.idl:76: static void writeDots(ArrayBuffer cells, long columns, long rows); On 2016/10/27 03:16:58, ultimatedbz wrote: > On 2016/10/26 04:49:24, David Tseng wrote: > > Driving by. It seems like you don't really need the rows parameter throughout > > all of the bindings. The caller needs to fill the array in order to construct > > the flattened cells. So, rows = ceiling(cells.size() / columns), right? > > Hey David! Yes, what you said definitely makes sense and we could just derive > rows from the dividing the cell's size by |columns|. However, if we also pass in > |rows|, would you say that it makes the code more readable and clear? FWIW, I have a slight preference towards including both columns and rows as well for readability. But since its a private API, I'm fine either way.
https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... File chrome/common/extensions/api/braille_display_private.idl (right): https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... chrome/common/extensions/api/braille_display_private.idl:76: static void writeDots(ArrayBuffer cells, long columns, long rows); On 2016/10/27 14:59:28, Devlin wrote: > On 2016/10/27 03:16:58, ultimatedbz wrote: > > On 2016/10/26 04:49:24, David Tseng wrote: > > > Driving by. It seems like you don't really need the rows parameter > throughout > > > all of the bindings. The caller needs to fill the array in order to > construct > > > the flattened cells. So, rows = ceiling(cells.size() / columns), right? > > > > Hey David! Yes, what you said definitely makes sense and we could just derive > > rows from the dividing the cell's size by |columns|. However, if we also pass > in > > |rows|, would you say that it makes the code more readable and clear? > > FWIW, I have a slight preference towards including both columns and rows as well > for readability. But since its a private API, I'm fine either way. As is, it is unclear what happens if you pass in less rows than is contained in the buffer. If you mean to allow the caller to clip output, then I guess this is fine. Otherwise, it seems unnecessary and imo unclear.
On 2016/10/27 15:39:36, David Tseng wrote: > https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... > File chrome/common/extensions/api/braille_display_private.idl (right): > > https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... > chrome/common/extensions/api/braille_display_private.idl:76: static void > writeDots(ArrayBuffer cells, long columns, long rows); > On 2016/10/27 14:59:28, Devlin wrote: > > On 2016/10/27 03:16:58, ultimatedbz wrote: > > > On 2016/10/26 04:49:24, David Tseng wrote: > > > > Driving by. It seems like you don't really need the rows parameter > > throughout > > > > all of the bindings. The caller needs to fill the array in order to > > construct > > > > the flattened cells. So, rows = ceiling(cells.size() / columns), right? > > > > > > Hey David! Yes, what you said definitely makes sense and we could just > derive > > > rows from the dividing the cell's size by |columns|. However, if we also > pass > > in > > > |rows|, would you say that it makes the code more readable and clear? > > > > FWIW, I have a slight preference towards including both columns and rows as > well > > for readability. But since its a private API, I'm fine either way. > > As is, it is unclear what happens if you pass in less rows than is contained in > the buffer. If you mean to allow the caller to clip output, then I guess this is > fine. Otherwise, it seems unnecessary and imo unclear. I think there are two scenarios: 1. If rows * columns does not equal the size of the buffer, that seems like a programmer error, and I'd be happy to just DCHECK for this at the top 2. If rows * columns does not equal the size of the current display, that's normal because it's a race condition - the display may have updated but ChromeVox may not know about it yet. That case should be handled gracefully by clipping and padding.
On 2016/10/27 15:58:58, dmazzoni wrote: > On 2016/10/27 15:39:36, David Tseng wrote: > > > https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... > > File chrome/common/extensions/api/braille_display_private.idl (right): > > > > > https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... > > chrome/common/extensions/api/braille_display_private.idl:76: static void > > writeDots(ArrayBuffer cells, long columns, long rows); > > On 2016/10/27 14:59:28, Devlin wrote: > > > On 2016/10/27 03:16:58, ultimatedbz wrote: > > > > On 2016/10/26 04:49:24, David Tseng wrote: > > > > > Driving by. It seems like you don't really need the rows parameter > > > throughout > > > > > all of the bindings. The caller needs to fill the array in order to > > > construct > > > > > the flattened cells. So, rows = ceiling(cells.size() / columns), right? > > > > > > > > Hey David! Yes, what you said definitely makes sense and we could just > > derive > > > > rows from the dividing the cell's size by |columns|. However, if we also > > pass > > > in > > > > |rows|, would you say that it makes the code more readable and clear? > > > > > > FWIW, I have a slight preference towards including both columns and rows as > > well > > > for readability. But since its a private API, I'm fine either way. > > > > As is, it is unclear what happens if you pass in less rows than is contained > in > > the buffer. If you mean to allow the caller to clip output, then I guess this > is > > fine. Otherwise, it seems unnecessary and imo unclear. > > I think there are two scenarios: > > 1. If rows * columns does not equal the size of the buffer, that seems like a > programmer error, and I'd be happy to just DCHECK for this at the top > Seems reasonable if that's what Jeffrey wants (see below). > 2. If rows * columns does not equal the size of the current display, that's > normal because it's a race condition - the display may have updated but > ChromeVox may not know about it yet. That case should be handled gracefully > by clipping and padding. This is the first time I've heard that a display can change its dimensions at runtime. I interpreted the rows/columns params as the ability for the caller to create a subrect into the buffer and that the buffer might be less than the total dimensions of the display (so you could use 1/4 of the display, for example). I don't want to hold this cl up so this is all optional and as was the previous suggestion. If I were trying to design this api, I'd think about how I would call it in the various use cases I want to achieve. In your current design, would you ever want to call it with rows less than the rows contained in the buffer? If I took a stab at designing this api, it might go something like the following. To make my life easier, I'd prefer encoding most things in the cells buffer. Not sure what was discussed before, but a 2d array (e.g. an array of array buffers) would have been my first choice. You could then have an optional rect that restricts the view of the matrix. This could allow you to pan around the buffer via translations on the rect; as a caller, you would also be able to re-write the buffer in place and not have to regenerate the entire buffer every time you wanted to write its contents.
On Thu, Oct 27, 2016 at 9:18 AM <dtseng@chromium.org> wrote: > This is the first time I've heard that a display can change its dimensions > at > runtime. > It's not that the display can change its dimensions, it's that the user can unplug one display and plug in another. I think this can also happen when the user has had braille mode on with a virtual display and then they plug in a display. > I interpreted the rows/columns params as the ability for the caller to > create a subrect into the buffer and that the buffer might be less than the > total dimensions of the display (so you could use 1/4 of the display, for > example). > Yes, that also seems reasonable. The point is that this API should be tolerant of calling it with a different size, but there's no reason that the buffer passed in shouldn't match |rows| and |columns|. To make my life easier, I'd prefer encoding most things in the cells > buffer. Not > sure what was discussed before, but a 2d array (e.g. an array of array > buffers) > would have been my first choice. > Actually he originally tried a 2d array. The reasons we moved away from that here is (1) the extension function system doesn't support 2d arrays, and (2) brlapi doesn't support 2d arrays. Since this particular function is a low-level interface it makes sense to use 1d here. For the higher-level APIs that ChromeVox will be calling the plan is to use 2d arrays. -- 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.
On 2016/10/27 15:39:36, David Tseng wrote: > https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... > File chrome/common/extensions/api/braille_display_private.idl (right): > > https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... > chrome/common/extensions/api/braille_display_private.idl:76: static void > writeDots(ArrayBuffer cells, long columns, long rows); > On 2016/10/27 14:59:28, Devlin wrote: > > On 2016/10/27 03:16:58, ultimatedbz wrote: > > > On 2016/10/26 04:49:24, David Tseng wrote: > > > > Driving by. It seems like you don't really need the rows parameter > > throughout > > > > all of the bindings. The caller needs to fill the array in order to > > construct > > > > the flattened cells. So, rows = ceiling(cells.size() / columns), right? > > > > > > Hey David! Yes, what you said definitely makes sense and we could just > derive > > > rows from the dividing the cell's size by |columns|. However, if we also > pass > > in > > > |rows|, would you say that it makes the code more readable and clear? > > > > FWIW, I have a slight preference towards including both columns and rows as > well > > for readability. But since its a private API, I'm fine either way. > > As is, it is unclear what happens if you pass in less rows than is contained in > the buffer. If you mean to allow the caller to clip output, then I guess this is > fine. Otherwise, it seems unnecessary and imo unclear. Yes, the implementation of this function will pad the cells if there is too much space, and will clip cells by both rows and columns if there isn't enough space in the buffer. Thanks!
On 2016/10/27 14:59:28, Devlin wrote: > I see a bunch of "done"s, but no new patch set - could you upload the latest > version? :) > > https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... > File chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc > (right): > > https://codereview.chromium.org/2401393003/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc:169: > if (libbrlapi_loader_->brlapi__writeDots(handle_.get(), &cells[0]) < 0) { > On 2016/10/27 03:16:58, ultimatedbz wrote: > > On 2016/10/25 22:36:54, Devlin wrote: > > > Do we need to check if cells is empty? > > > > > > Also, prefer cells.data() to access the pointer to the beginning of the > array, > > > rather than &cells[0]. > > > > Cool! I did not know vectors had a data() function! And I can't say for sure > > what would happen if cells was empty, but I think nothing would be drawn on > the > > braille display, or brlapi__writeDots(...) would return a negative number. > > Reason I asked about checking emptiness before was because it would otherwise > crash on cells[0]. Using data() should be safe as long as writeDots doesn't try > to write anything with the passed data. > > https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... > File chrome/common/extensions/api/braille_display_private.idl (right): > > https://codereview.chromium.org/2401393003/diff/60001/chrome/common/extension... > chrome/common/extensions/api/braille_display_private.idl:76: static void > writeDots(ArrayBuffer cells, long columns, long rows); > On 2016/10/27 03:16:58, ultimatedbz wrote: > > On 2016/10/26 04:49:24, David Tseng wrote: > > > Driving by. It seems like you don't really need the rows parameter > throughout > > > all of the bindings. The caller needs to fill the array in order to > construct > > > the flattened cells. So, rows = ceiling(cells.size() / columns), right? > > > > Hey David! Yes, what you said definitely makes sense and we could just derive > > rows from the dividing the cell's size by |columns|. However, if we also pass > in > > |rows|, would you say that it makes the code more readable and clear? > > FWIW, I have a slight preference towards including both columns and rows as well > for readability. But since its a private API, I'm fine either way. Hey Devlin! Sorry I completely forgot to upload it. Uploaded!
Devlin: friendly ping, this is ready for another look.
sorry for the delay! https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc (right): https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc:171: EXTENSION_FUNCTION_VALIDATE(params_); Can we either EXTENSION_FUNCTION_VALIDATE or throw an error on failure that `params_->cells.size() >= params_->columns * params_->rows`? Otherwise if we're passed bad input we'll read past the end of the vector and have bad times. https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/mock_braille_controller.cc (right): https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/mock_braille_controller.cc:17: if (available_){ nit: need space after end paren here
https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc (right): https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc:171: EXTENSION_FUNCTION_VALIDATE(params_); On 2016/11/02 15:10:30, Devlin (slow) wrote: > Can we either EXTENSION_FUNCTION_VALIDATE or throw an error on failure that > `params_->cells.size() >= params_->columns * params_->rows`? > > Otherwise if we're passed bad input we'll read past the end of the vector and > have bad times. Didn't really account for this in braille_controller_brlapi.cc? + unsigned int row_limit = std::min(rows, cells_rows); + unsigned int col_limit = std::min(columns, cells_cols); If params_->cells.size() is too big, it just tapers off. If I were to EXTENSION_FUNCTION_VALIDATE, where would I write this code? Thanks! https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/mock_braille_controller.cc (right): https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/mock_braille_controller.cc:17: if (available_){ On 2016/11/02 15:10:30, Devlin (slow) wrote: > nit: need space after end paren here Thanks!
https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc (right): https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc:171: EXTENSION_FUNCTION_VALIDATE(params_); On 2016/11/03 19:28:54, ultimatedbz wrote: > On 2016/11/02 15:10:30, Devlin (slow) wrote: > > Can we either EXTENSION_FUNCTION_VALIDATE or throw an error on failure that > > `params_->cells.size() >= params_->columns * params_->rows`? > > > > Otherwise if we're passed bad input we'll read past the end of the vector and > > have bad times. > > Didn't really account for this in braille_controller_brlapi.cc? > + unsigned int row_limit = std::min(rows, cells_rows); > + unsigned int col_limit = std::min(columns, cells_cols); > > If params_->cells.size() is too big, it just tapers off. If I were to > EXTENSION_FUNCTION_VALIDATE, where would I write this code? Thanks! I think he's saying that if (params_->cells.size() >= params_->columns * params_->rows) is not true, it shouldn't validate. In other words, it should fail if you try to pass a vector that's not as large as you say its dimensions are Just call EXTENSION_FUNCTION_VALIDATE again: EXTENSION_FUNCTION_VALIDATE( params_->cells.size() >= params_->columns * params_->rows);
On 2016/11/04 18:10:36, dmazzoni wrote: > https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... > File > chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc > (right): > > https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... > chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc:171: > EXTENSION_FUNCTION_VALIDATE(params_); > On 2016/11/03 19:28:54, ultimatedbz wrote: > > On 2016/11/02 15:10:30, Devlin (slow) wrote: > > > Can we either EXTENSION_FUNCTION_VALIDATE or throw an error on failure that > > > `params_->cells.size() >= params_->columns * params_->rows`? > > > > > > Otherwise if we're passed bad input we'll read past the end of the vector > and > > > have bad times. > > > > Didn't really account for this in braille_controller_brlapi.cc? > > + unsigned int row_limit = std::min(rows, cells_rows); > > + unsigned int col_limit = std::min(columns, cells_cols); > > > > If params_->cells.size() is too big, it just tapers off. If I were to > > EXTENSION_FUNCTION_VALIDATE, where would I write this code? Thanks! > > I think he's saying that if (params_->cells.size() >= params_->columns * > params_->rows) is not true, it shouldn't validate. > > In other words, it should fail if you try to pass a vector that's not as large > as you say its dimensions are > > Just call EXTENSION_FUNCTION_VALIDATE again: > > EXTENSION_FUNCTION_VALIDATE( > params_->cells.size() >= params_->columns * params_->rows); Right, otherwise we'll read past the end of the vector. Using EXTENSION_FUNCTION_VALIDATE() will kill the renderer if it fails, so you should use that if you're sure no extension should ever, ever, ever call this with incorrect parameters. We can also throw an error here (i.e., return RespondNow(Error("Invalid dimensions"));), which would set chrome.runtime.lastError for the extension.
https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc (right): https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc:171: EXTENSION_FUNCTION_VALIDATE(params_); On 2016/11/04 18:10:35, dmazzoni wrote: > On 2016/11/03 19:28:54, ultimatedbz wrote: > > On 2016/11/02 15:10:30, Devlin (slow) wrote: > > > Can we either EXTENSION_FUNCTION_VALIDATE or throw an error on failure that > > > `params_->cells.size() >= params_->columns * params_->rows`? > > > > > > Otherwise if we're passed bad input we'll read past the end of the vector > and > > > have bad times. > > > > Didn't really account for this in braille_controller_brlapi.cc? > > + unsigned int row_limit = std::min(rows, cells_rows); > > + unsigned int col_limit = std::min(columns, cells_cols); > > > > If params_->cells.size() is too big, it just tapers off. If I were to > > EXTENSION_FUNCTION_VALIDATE, where would I write this code? Thanks! > > I think he's saying that if (params_->cells.size() >= params_->columns * > params_->rows) is not true, it shouldn't validate. > > In other words, it should fail if you try to pass a vector that's not as large > as you say its dimensions are > > Just call EXTENSION_FUNCTION_VALIDATE again: > > EXTENSION_FUNCTION_VALIDATE( > params_->cells.size() >= params_->columns * params_->rows); Done.
https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc (right): https://codereview.chromium.org/2401393003/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc:171: EXTENSION_FUNCTION_VALIDATE(params_); On 2016/11/04 18:10:35, dmazzoni wrote: > On 2016/11/03 19:28:54, ultimatedbz wrote: > > On 2016/11/02 15:10:30, Devlin (slow) wrote: > > > Can we either EXTENSION_FUNCTION_VALIDATE or throw an error on failure that > > > `params_->cells.size() >= params_->columns * params_->rows`? > > > > > > Otherwise if we're passed bad input we'll read past the end of the vector > and > > > have bad times. > > > > Didn't really account for this in braille_controller_brlapi.cc? > > + unsigned int row_limit = std::min(rows, cells_rows); > > + unsigned int col_limit = std::min(columns, cells_cols); > > > > If params_->cells.size() is too big, it just tapers off. If I were to > > EXTENSION_FUNCTION_VALIDATE, where would I write this code? Thanks! > > I think he's saying that if (params_->cells.size() >= params_->columns * > params_->rows) is not true, it shouldn't validate. > > In other words, it should fail if you try to pass a vector that's not as large > as you say its dimensions are > > Just call EXTENSION_FUNCTION_VALIDATE again: > > EXTENSION_FUNCTION_VALIDATE( > params_->cells.size() >= params_->columns * params_->rows); Done.
lgtm
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/2401393003/#ps120001 (title: "Validated params")
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by ultimatedbz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2401393003/#ps140001 (title: "Fixed Test by typecasting size.")
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 ultimatedbz@google.com
The CQ bit was checked by ultimatedbz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2401393003/#ps220001 (title: "more tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add multiline braille support. Added code to take in 1D array and also row and column count. Code compiles up to auto generated code. Cannot figure out how to generate a vector of vectors of chars. Also don't know where is appropriate to add in col and row information. Also do no know if input is assumed to be 1D or 2D BUG=651570 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add multiline braille support. Added code to take in 1D array and also row and column count. Code compiles up to auto generated code. Cannot figure out how to generate a vector of vectors of chars. Also don't know where is appropriate to add in col and row information. Also do no know if input is assumed to be 1D or 2D BUG=651570 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add multiline braille support. Added code to take in 1D array and also row and column count. Code compiles up to auto generated code. Cannot figure out how to generate a vector of vectors of chars. Also don't know where is appropriate to add in col and row information. Also do no know if input is assumed to be 1D or 2D BUG=651570 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add multiline braille support. Added code to take in 1D array and also row and column count. Code compiles up to auto generated code. Cannot figure out how to generate a vector of vectors of chars. Also don't know where is appropriate to add in col and row information. Also do no know if input is assumed to be 1D or 2D BUG=651570 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/ffd46520fa89b84acb6a829f1b3d3b183477f1ad Cr-Commit-Position: refs/heads/master@{#431444} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/ffd46520fa89b84acb6a829f1b3d3b183477f1ad Cr-Commit-Position: refs/heads/master@{#431444} |
