Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2462483002:
Support multi-line braille in the virtual braille display. (Closed)
|
Created:
4 years, 1 month ago by ultimatedbz Modified:
4 years, 1 month ago Reviewers:
dmazzoni CC:
chromium-reviews, alemate+watch_chromium.org, oshima+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, arv+watch_chromium.org, dtseng+watch_chromium.org, dmazzoni+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport multi-line braille in the virtual braille display.
The virtual braille display now can display multiple lines of braille and text. One can now change the number of rows and columns of the display in the options page. They can also change the display style, whether it is interleaved or side by side. Interleaved is when there are alternating lines of braille cells and regular text characters. Side by side is when all lines of text are on the left and all lines of braille are on the right. When one hovers their mouse over a [braille/text] cell, the corresponding [text/braille] cell would highlight.
BUG=660214
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/b88383e838aa4ca1435b5b96d164642b2db53746
Cr-Commit-Position: refs/heads/master@{#428905}
Patch Set 1 #
Total comments: 36
Patch Set 2 : Addressed Review comments. #
Total comments: 14
Patch Set 3 : Addressed more Review comments. #
Total comments: 4
Patch Set 4 : Addressed small nit #Messages
Total messages: 27 (9 generated)
Description was changed from ========== Implemented internationalization. Makes sure rows and columns are not negative or empty. Rebased ontop of master. Added row limits. Finished connecting localStorage. Need to figure out how to update captions after changing virtual braille captions values in preferences. Now able to store brailleSideBySide correctly. Now able to store brailleSideBySide correctly. Multiline is supported on braille captions display. Interleaving and side by side. highlighting works. In side by side mode, both sides are scrollable. Captions size is now bigger. Also got it to interleave and be side by side. BUG= 660214 ========== to ========== Implemented internationalization. Makes sure rows and columns are not negative or empty. Rebased ontop of master. Added row limits. Finished connecting localStorage. Need to figure out how to update captions after changing virtual braille captions values in preferences. Now able to store brailleSideBySide correctly. Now able to store brailleSideBySide correctly. Multiline is supported on braille captions display. Interleaving and side by side. highlighting works. In side by side mode, both sides are scrollable. Captions size is now bigger. Also got it to interleave and be side by side. BUG= 660214 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Implemented internationalization. Makes sure rows and columns are not negative or empty. Rebased ontop of master. Added row limits. Finished connecting localStorage. Need to figure out how to update captions after changing virtual braille captions values in preferences. Now able to store brailleSideBySide correctly. Now able to store brailleSideBySide correctly. Multiline is supported on braille captions display. Interleaving and side by side. highlighting works. In side by side mode, both sides are scrollable. Captions size is now bigger. Also got it to interleave and be side by side. BUG= 660214 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Implemented internationalization. Makes sure rows and columns are not negative or empty. Rebased ontop of master. Added row limits. Finished connecting localStorage. Need to figure out how to update captions after changing virtual braille captions values in preferences. Now able to store brailleSideBySide correctly. Now able to store brailleSideBySide correctly. Multiline is supported on braille captions display. Interleaving and side by side. highlighting works. In side by side mode, both sides are scrollable. Captions size is now bigger. Also got it to interleave and be side by side. BUG= 660214 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
ultimatedbz@google.com changed reviewers: + dmazzoni@chromium.org
Please update the change description. Remember that the first line of the change description should match the title. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:248: // Copy the translated content to a separate buffer and add the cursor Thanks for fixing this! https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html (right): https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:92: <form> No <form> needed https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:93: <p class="i18n description" msgid="options_virtual_braille_display_rows" id="virtual_braille_display_rows_description"></p> Use a <label> element to associate this text with the <input>. That way ChromeVox knows that this text goes with that text field and it will describe it as such. See some examples above, the basic structure is: <label> <input> <span class="i18n"></span> </label> You can optionally put the whole thing in a <p> to or whatever, but the main idea is to have a <label> element. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js (right): https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:138: }, true); indentation https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:142: if ($('virtual_braille_display_rows_input').value === '') Put braces around all of the blocks because at least one of them below needs multiple lines https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:155: $('virtual_braille_display_rows_input').value = Same here, braces needed https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:160: addEventListener('input', function(evt) { put addEventListener( on the previous line and wrap before 'input' or before function https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:161: if ($('virtual_braille_display_columns_input').value === '') Do you need to handle the empty string separately? What does parseInt return? https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:171: $('virtual_braille_display_columns_input'). Can you cut down on code duplication by writing a helper function handleNumericalInputPref() or something like that, taking these parameters: * The id of the input element * The name of the localStorage pref I believe the rows and columns are basically the same code except for those two variables, right? https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/chromevox/background/prefs.js (right): https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/prefs.js:83: 'useNext': true, Thanks for rearranging these, but if you're sorting them alphabetically this should probably go before useVerboseMode https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js (right): https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:189: var cols = parseInt(localStorage['virtualBrailleColumns'], 10); Since this is more than a few lines of code I think it'd be more clear to move it out of the switch/case. Have case PanelCommandType.UPDATE_BRAILLE: call Panel.onUpdateBraille() or something like that, and put all of the logic in that function. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:196: cell.style.border = '1px solid white'; Use cell.addClass or cell.removeClass, and then have a CSS style that sets the border https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:214: this.brailleTableElement_.addEventListener('mouseover', addBorders); Not a big deal, but is there an element that's a parent of both tables that you could put the event listeners on rather than needing two of them? It'd probably work if you put the listener on document.body https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:216: Remove extra blank line https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:253: topCell.style.fontSize = '85%'; Use panel.css for this https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd (right): https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:442: Virtal Braille Display typo https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:444: <message desc="The description of the Rows spinbox. Displayed in the Options page." name="IDS_CHROMEVOX_OPTIONS_VIRTUAL_BRAILLE_DISPLAY_ROWS"> Remember that the purpose of this string is to help a translator understand the meaning of these words in context. It might be helpful to describe that. Imagine someone asked you to translate the word "Rows", but you didn't know the context. I'd maybe say: "The label for the numeric input field where the user can choose the number of rows in a grid" I think the key is to clarify that it's expressing "the number of rows" because that might affect how to translate it. The fact that it's in an options page or about braille is actually not as important. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:450: <message desc="A description that tells the user that the current display style of the virtual display interleaves braille and regular text, one on top of the other." name="IDS_CHROMEVOX_OPTIONS_CURRENT_DISPLAY_STYLE_INTERLEAVE"> These next four descriptions are much better. I think the most important thing is clarifying what the word interleaved means.
Description was changed from ========== Implemented internationalization. Makes sure rows and columns are not negative or empty. Rebased ontop of master. Added row limits. Finished connecting localStorage. Need to figure out how to update captions after changing virtual braille captions values in preferences. Now able to store brailleSideBySide correctly. Now able to store brailleSideBySide correctly. Multiline is supported on braille captions display. Interleaving and side by side. highlighting works. In side by side mode, both sides are scrollable. Captions size is now bigger. Also got it to interleave and be side by side. BUG= 660214 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Support multi-line braille in the virtual braille display. Implemented internationalization. Makes sure rows and columns are not negative or empty. Rebased ontop of master. Added row limits. Finished connecting localStorage. Need to figure out how to update captions after changing virtual braille captions values in preferences. Now able to store brailleSideBySide correctly. Now able to store brailleSideBySide correctly. Multiline is supported on braille captions display. Interleaving and side by side. highlighting works. In side by side mode, both sides are scrollable. Captions size is now bigger. Also got it to interleave and be side by side. BUG= 660214 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js (right): https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager.js:248: // Copy the translated content to a separate buffer and add the cursor On 2016/10/28 06:19:29, dmazzoni wrote: > Thanks for fixing this! Woo! https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html (right): https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:92: <form> On 2016/10/28 06:19:29, dmazzoni wrote: > No <form> needed Done. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:93: <p class="i18n description" msgid="options_virtual_braille_display_rows" id="virtual_braille_display_rows_description"></p> On 2016/10/28 06:19:29, dmazzoni wrote: > Use a <label> element to associate this text with the <input>. That way > ChromeVox knows that this text goes with that text field and it will describe it > as such. > > See some examples above, the basic structure is: > > <label> > <input> > <span class="i18n"></span> > </label> > > You can optionally put the whole thing in a <p> to or whatever, but the main > idea is to have a <label> element. Done. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js (right): https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:138: }, true); On 2016/10/28 06:19:29, dmazzoni wrote: > indentation Done. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:142: if ($('virtual_braille_display_rows_input').value === '') On 2016/10/28 06:19:29, dmazzoni wrote: > Put braces around all of the blocks because at least one of them below needs > multiple lines The code after it was moved to the helper function no longer needed multiple lines so I didn't add braces. Please check if it's okay. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:155: $('virtual_braille_display_rows_input').value = On 2016/10/28 06:19:29, dmazzoni wrote: > Same here, braces needed Done. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:160: addEventListener('input', function(evt) { On 2016/10/28 06:19:29, dmazzoni wrote: > put addEventListener( on the previous line and wrap before 'input' or before > function Done. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:161: if ($('virtual_braille_display_columns_input').value === '') On 2016/10/28 06:19:29, dmazzoni wrote: > Do you need to handle the empty string separately? What does parseInt return? I believe so. I just checked in the console and parseInt('') would return NaN. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:171: $('virtual_braille_display_columns_input'). On 2016/10/28 06:19:29, dmazzoni wrote: > Can you cut down on code duplication by writing a helper function > handleNumericalInputPref() or something like that, taking these parameters: > > * The id of the input element > * The name of the localStorage pref > > I believe the rows and columns are basically the same code except for those > two variables, right? > Wow, I got rid of so much duplicated code! https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/chromevox/background/prefs.js (right): https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/chromevox/background/prefs.js:83: 'useNext': true, On 2016/10/28 06:19:29, dmazzoni wrote: > Thanks for rearranging these, but if you're sorting them alphabetically this > should probably go before useVerboseMode Done. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js (right): https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:189: var cols = parseInt(localStorage['virtualBrailleColumns'], 10); On 2016/10/28 06:19:29, dmazzoni wrote: > Since this is more than a few lines of code I think it'd be more clear to move > it out of the switch/case. Have case PanelCommandType.UPDATE_BRAILLE: call > Panel.onUpdateBraille() or something like that, and put all of the logic in that > function. Done. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:196: cell.style.border = '1px solid white'; On 2016/10/28 06:19:29, dmazzoni wrote: > Use cell.addClass or cell.removeClass, and then have a CSS style that sets the > border I had to use cell.className = '[className]'; Thanks! https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:214: this.brailleTableElement_.addEventListener('mouseover', addBorders); On 2016/10/28 06:19:30, dmazzoni wrote: > Not a big deal, but is there an element that's a parent of both tables that you > could put the event listeners on rather than > needing two of them? It'd probably work if you put the listener on document.body > Yes, I added the eventListener to brailleContainer. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:216: On 2016/10/28 06:19:29, dmazzoni wrote: > Remove extra blank line Done. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:253: topCell.style.fontSize = '85%'; On 2016/10/28 06:19:30, dmazzoni wrote: > Use panel.css for this Do we still want to show regular letters at size '85%'? The original motivation for doing so was so that when interleaved, the braille cells and text characters would both completely show within the bounds of the virtual braille display. Since we're already going to be expanding the height of the virtual braille display, would we still want to do this? https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd (right): https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:442: Virtal Braille Display On 2016/10/28 06:19:30, dmazzoni wrote: > typo Done. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:444: <message desc="The description of the Rows spinbox. Displayed in the Options page." name="IDS_CHROMEVOX_OPTIONS_VIRTUAL_BRAILLE_DISPLAY_ROWS"> On 2016/10/28 06:19:30, dmazzoni wrote: > Remember that the purpose of this string is to help a translator understand the > meaning of these words in context. It might be helpful to describe that. Imagine > someone asked you to translate the word "Rows", but you didn't know the context. > > I'd maybe say: "The label for the numeric input field where the user can choose > the number of rows in a grid" > > I think the key is to clarify that it's expressing "the number of rows" because > that might affect how to translate it. The fact that it's in an options page or > about braille is actually not as important. Done. https://codereview.chromium.org/2462483002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:450: <message desc="A description that tells the user that the current display style of the virtual display interleaves braille and regular text, one on top of the other." name="IDS_CHROMEVOX_OPTIONS_CURRENT_DISPLAY_STYLE_INTERLEAVE"> On 2016/10/28 06:19:30, dmazzoni wrote: > These next four descriptions are much better. I think the most important thing > is clarifying what the word interleaved means. Acknowledged. Do you think "interleaved" needs to be clarified more?
Please update the change description too and let me know when it's ready You can just rid of the 85% font size for now and we can fix it in a follow-up by resizing the panel as needed
On 2016/10/28 18:50:47, dmazzoni wrote: > Please update the change description too and let > me know when it's ready > > You can just rid of the 85% font size for now and > we can fix it in a follow-up by resizing the panel > as needed I just addressed presubmit issues and uploaded the code. In the description, the top line matches the title. Should I also get rid of the lines between the top line and the bug number?
On 2016/10/28 19:05:35, ultimatedbz wrote: > On 2016/10/28 18:50:47, dmazzoni wrote: > > Please update the change description too and let > > me know when it's ready > > > > You can just rid of the 85% font size for now and > > we can fix it in a follow-up by resizing the panel > > as needed > > I just addressed presubmit issues and uploaded the code. In the description, the > top line matches the title. Should I also get rid of the lines between the top > line and the bug number? Yes, rewrite them to be a concise description of the change as a whole, rather than a list of log messages from your original commits.
On 2016/10/28 19:08:23, dmazzoni wrote: > On 2016/10/28 19:05:35, ultimatedbz wrote: > > On 2016/10/28 18:50:47, dmazzoni wrote: > > > Please update the change description too and let > > > me know when it's ready > > > > > > You can just rid of the 85% font size for now and > > > we can fix it in a follow-up by resizing the panel > > > as needed > > > > I just addressed presubmit issues and uploaded the code. In the description, > the > > top line matches the title. Should I also get rid of the lines between the top > > line and the bug number? > > Yes, rewrite them to be a concise description of the change as > a whole, rather than a list of log messages from your original > commits. Type "git log" and read through a few dozen commit messages to get an idea of what a good description looks like. Note that about half are autogenerated, reverts, or trivial changes, so focus on ones that are actual code changes to be reviewed with a good description.
Description was changed from ========== Support multi-line braille in the virtual braille display. Implemented internationalization. Makes sure rows and columns are not negative or empty. Rebased ontop of master. Added row limits. Finished connecting localStorage. Need to figure out how to update captions after changing virtual braille captions values in preferences. Now able to store brailleSideBySide correctly. Now able to store brailleSideBySide correctly. Multiline is supported on braille captions display. Interleaving and side by side. highlighting works. In side by side mode, both sides are scrollable. Captions size is now bigger. Also got it to interleave and be side by side. BUG= 660214 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Support multi-line braille in the virtual braille display. The virtual braille display now can display multiple lines of braille and text. One can now change the number of rows and columns of the display in the options page. They can also change the display style, whether it is interleaved or side by side. Interleaved is when there are alternating lines of braille cells and regular text characters. Side by side is when all lines of text are on the left and all lines of braille are on the right. When one hovers their mouse over a [braille/text] cell, the corresponding [text/braille] cell would highlight. BUG= 660214 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/10/28 19:09:55, dmazzoni wrote: > On 2016/10/28 19:08:23, dmazzoni wrote: > > On 2016/10/28 19:05:35, ultimatedbz wrote: > > > On 2016/10/28 18:50:47, dmazzoni wrote: > > > > Please update the change description too and let > > > > me know when it's ready > > > > > > > > You can just rid of the 85% font size for now and > > > > we can fix it in a follow-up by resizing the panel > > > > as needed > > > > > > I just addressed presubmit issues and uploaded the code. In the description, > > the > > > top line matches the title. Should I also get rid of the lines between the > top > > > line and the bug number? > > > > Yes, rewrite them to be a concise description of the change as > > a whole, rather than a list of log messages from your original > > commits. > > Type "git log" and read through a few dozen commit messages to > get an idea of what a good description looks like. Note that about half > are autogenerated, reverts, or trivial changes, so focus on ones > that are actual code changes to be reviewed with a good description. It's okay to write commit messages as I code right? But when I upload a cl, I'd have to change my description. And then when I click commit, it would push the branch with the description as the commit message. Is that right?
On 2016/10/28 19:09:55, dmazzoni wrote: > On 2016/10/28 19:08:23, dmazzoni wrote: > > On 2016/10/28 19:05:35, ultimatedbz wrote: > > > On 2016/10/28 18:50:47, dmazzoni wrote: > > > > Please update the change description too and let > > > > me know when it's ready > > > > > > > > You can just rid of the 85% font size for now and > > > > we can fix it in a follow-up by resizing the panel > > > > as needed > > > > > > I just addressed presubmit issues and uploaded the code. In the description, > > the > > > top line matches the title. Should I also get rid of the lines between the > top > > > line and the bug number? > > > > Yes, rewrite them to be a concise description of the change as > > a whole, rather than a list of log messages from your original > > commits. > > Type "git log" and read through a few dozen commit messages to > get an idea of what a good description looks like. Note that about half > are autogenerated, reverts, or trivial changes, so focus on ones > that are actual code changes to be reviewed with a good description. It's okay to write commit messages as I code right? But when I upload a cl, I'd have to change my description. And then when I click commit, it would push the branch with the description as the commit message. Is that right?
On 2016/10/28 19:15:34, ultimatedbz wrote: > On 2016/10/28 19:09:55, dmazzoni wrote: > > On 2016/10/28 19:08:23, dmazzoni wrote: > > > On 2016/10/28 19:05:35, ultimatedbz wrote: > > > > On 2016/10/28 18:50:47, dmazzoni wrote: > > > > > Please update the change description too and let > > > > > me know when it's ready > > > > > > > > > > You can just rid of the 85% font size for now and > > > > > we can fix it in a follow-up by resizing the panel > > > > > as needed > > > > > > > > I just addressed presubmit issues and uploaded the code. In the > description, > > > the > > > > top line matches the title. Should I also get rid of the lines between the > > top > > > > line and the bug number? > > > > > > Yes, rewrite them to be a concise description of the change as > > > a whole, rather than a list of log messages from your original > > > commits. > > > > Type "git log" and read through a few dozen commit messages to > > get an idea of what a good description looks like. Note that about half > > are autogenerated, reverts, or trivial changes, so focus on ones > > that are actual code changes to be reviewed with a good description. > > It's okay to write commit messages as I code right? But when I upload a cl, I'd > have to change my description. And then when I click commit, it would push the > branch with the description as the commit message. Is that right? When you run "git cl upload" it should prompt you to edit the description. It's initially populated with the commit messages, but once you edit it, it will stick. If you add more commit messages it won't affect the description. So yeah, the commit messages are basically for your own benefit. You can use them as a draft of the change description but they don't have to be. Chrome is a bit unusual that way. In a few months we may be migrating to Gerrit and then the description will be more tied to the commit message, but I don't know how that will work exactly yet.
https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html (right): https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:92: <label > nit: no space before the closing > https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:98: <input type="number" min="1" id="virtual_braille_display_rows_input"> Maybe we should have a max of 99 so that it doesn't crash if you put in a number too large to handle Or handle it in the JS code https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:126: <p class="i18n description" msgid="options_shortcuts_description" I think this is a merge error. David recently deleted this section, and when you rebased you added it back. https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js (right): https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:141: var handleNumbericalInputPref = function(id, pref) { Numberical -> Numerical Move this to a complete documented private method of cvox.OptionsPage, like: /** * Docs go here * @param {string} id Docs * @param {string} pref Docs * @private */ cvox.OptionsPage.handleNumericalInputPref_ = function(id, pref) { }; The docs can be very concise, but explain what it does https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js (right): https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:410: Panel.onUpdateBraille = function(data) { Note that you'll also need to edit cvox.BrailleCaptionsBackground.getVirtualDisplayState and have it use this virtual braille size, but that can come in the next change. https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:411: var groups = data.groups; Fix indentation here https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:423: No blank line
https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html (right): https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:92: <label > On 2016/10/28 20:19:18, dmazzoni wrote: > nit: no space before the closing > Done. https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:98: <input type="number" min="1" id="virtual_braille_display_rows_input"> On 2016/10/28 20:19:18, dmazzoni wrote: > Maybe we should have a max of 99 so that it doesn't crash if you put in a number > too large to handle > > Or handle it in the JS code > Done. https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:126: <p class="i18n description" msgid="options_shortcuts_description" On 2016/10/28 20:19:18, dmazzoni wrote: > I think this is a merge error. David recently deleted > this section, and when you rebased you added it back. Oh! My bad. I will be more careful the next time I rebase and merge. https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js (right): https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:141: var handleNumbericalInputPref = function(id, pref) { On 2016/10/28 20:19:18, dmazzoni wrote: > Numberical -> Numerical > > Move this to a complete documented private method > of cvox.OptionsPage, like: > > /** > * Docs go here > * @param {string} id Docs > * @param {string} pref Docs > * @private > */ > cvox.OptionsPage.handleNumericalInputPref_ = function(id, pref) { > }; > > The docs can be very concise, but explain what it does Done. https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js (right): https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:410: Panel.onUpdateBraille = function(data) { On 2016/10/28 20:19:18, dmazzoni wrote: > Note that you'll also need to edit > cvox.BrailleCaptionsBackground.getVirtualDisplayState and have it use this > virtual braille size, but that can come in the next change. Acknowledged. https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:411: var groups = data.groups; On 2016/10/28 20:19:18, dmazzoni wrote: > Fix indentation here Done. https://codereview.chromium.org/2462483002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.js:423: On 2016/10/28 20:19:18, dmazzoni wrote: > No blank line Done.
lgtm https://codereview.chromium.org/2462483002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js (right): https://codereview.chromium.org/2462483002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:171: return; nit: Need braces throughout this if/else
https://codereview.chromium.org/2462483002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js (right): https://codereview.chromium.org/2462483002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:171: return; On 2016/10/31 20:03:44, dmazzoni wrote: > nit: Need braces throughout this if/else What if I bring line 173 up to line 172 so the whole else/if is only one line, assuming that it fits?
https://codereview.chromium.org/2462483002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js (right): https://codereview.chromium.org/2462483002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:171: return; On 2016/10/31 20:05:57, ultimatedbz wrote: > On 2016/10/31 20:03:44, dmazzoni wrote: > > nit: Need braces throughout this if/else > > What if I bring line 173 up to line 172 so the whole else/if is only one line, > assuming that it fits? If it fits, that's fine. If anything wraps then uses braces for everything.
https://codereview.chromium.org/2462483002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js (right): https://codereview.chromium.org/2462483002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/options.js:171: return; On 2016/10/31 20:55:41, dmazzoni wrote: > On 2016/10/31 20:05:57, ultimatedbz wrote: > > On 2016/10/31 20:03:44, dmazzoni wrote: > > > nit: Need braces throughout this if/else > > > > What if I bring line 173 up to line 172 so the whole else/if is only one line, > > assuming that it fits? > > If it fits, that's fine. If anything wraps then uses braces for everything. Gotcha!
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/2462483002/#ps60001 (title: "Addressed small nit")
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 ========== Support multi-line braille in the virtual braille display. The virtual braille display now can display multiple lines of braille and text. One can now change the number of rows and columns of the display in the options page. They can also change the display style, whether it is interleaved or side by side. Interleaved is when there are alternating lines of braille cells and regular text characters. Side by side is when all lines of text are on the left and all lines of braille are on the right. When one hovers their mouse over a [braille/text] cell, the corresponding [text/braille] cell would highlight. BUG= 660214 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Support multi-line braille in the virtual braille display. The virtual braille display now can display multiple lines of braille and text. One can now change the number of rows and columns of the display in the options page. They can also change the display style, whether it is interleaved or side by side. Interleaved is when there are alternating lines of braille cells and regular text characters. Side by side is when all lines of text are on the left and all lines of braille are on the right. When one hovers their mouse over a [braille/text] cell, the corresponding [text/braille] cell would highlight. BUG= 660214 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Support multi-line braille in the virtual braille display. The virtual braille display now can display multiple lines of braille and text. One can now change the number of rows and columns of the display in the options page. They can also change the display style, whether it is interleaved or side by side. Interleaved is when there are alternating lines of braille cells and regular text characters. Side by side is when all lines of text are on the left and all lines of braille are on the right. When one hovers their mouse over a [braille/text] cell, the corresponding [text/braille] cell would highlight. BUG= 660214 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Support multi-line braille in the virtual braille display. The virtual braille display now can display multiple lines of braille and text. One can now change the number of rows and columns of the display in the options page. They can also change the display style, whether it is interleaved or side by side. Interleaved is when there are alternating lines of braille cells and regular text characters. Side by side is when all lines of text are on the left and all lines of braille are on the right. When one hovers their mouse over a [braille/text] cell, the corresponding [text/braille] cell would highlight. BUG= 660214 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b88383e838aa4ca1435b5b96d164642b2db53746 Cr-Commit-Position: refs/heads/master@{#428905} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b88383e838aa4ca1435b5b96d164642b2db53746 Cr-Commit-Position: refs/heads/master@{#428905} |
