Add new braille tables supported by liblouis 2.6.0.

Add new braille tables supported by liblouis 2.6.0.

This adds 9 new braille tables, including the highly requested Unified
English Braille (grade 1 and 2).  Support or two new languages: Hebrew
and Korean is also added.

While touching this code, also added locale-aware sorting of the tables
in the list on the options page to make it easier to browse through it.

BUG=341953
R=dtseng@chromium.org

Committed: https://crrev.com/f29da0d1fd996ab76a674d9abe8c43a2285898e4
Cr-Commit-Position: refs/heads/master@{#296404}

[Peter Lundblad, 2014-09-19 13:39:54]

Alright, time for UEB and some more...

[David Tseng, 2014-09-19 23:13:39]

https://codereview.chromium.org/578303002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/host/interface/abstract_msgs.js
(right):

https://codereview.chromium.org/578303002/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/host/interface/abstract_msgs.js:57:
/**
Kind of odd to put this here. Why not place in ChromeMsgs?

[Peter Lundblad, 2014-09-22 09:18:54]

dtseng@chromium.org writes:
> 
>
https://codereview.chromium.org/578303002/diff/20001/chrome/browser/resources...
> File
> chrome/browser/resources/chromeos/chromevox/host/interface/abstract_msgs.js
> (right):
> 
>
https://codereview.chromium.org/578303002/diff/20001/chrome/browser/resources...
>
chrome/browser/resources/chromeos/chromevox/host/interface/abstract_msgs.js:57:
> /**
> Kind of odd to put this here. Why not place in ChromeMsgs?

Since this is an abstract class and not a pure interface, I think it makes
sense to put functionality here that isn't specific to 'Chrome'. Adding it here
makes it available to cvox.TestMsgs as well.

I could also go ahead and get rid of AbstractMsgs. It doesn't really make sense
anymore.  Thoughts?

> https://codereview.chromium.org/578303002/

-- 

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

[David Tseng, 2014-09-22 17:54:36]

On 2014/09/22 09:18:54, Peter Lundblad wrote:
> mailto:dtseng@chromium.org writes:
> > 
> >
>
https://codereview.chromium.org/578303002/diff/20001/chrome/browser/resources...
> > File
> > chrome/browser/resources/chromeos/chromevox/host/interface/abstract_msgs.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/578303002/diff/20001/chrome/browser/resources...
> >
>
chrome/browser/resources/chromeos/chromevox/host/interface/abstract_msgs.js:57:
> > /**
> > Kind of odd to put this here. Why not place in ChromeMsgs?
> 
> Since this is an abstract class and not a pure interface, I think it makes
> sense to put functionality here that isn't specific to 'Chrome'. Adding it
here
> makes it available to cvox.TestMsgs as well.
> 
> I could also go ahead and get rid of AbstractMsgs. It doesn't really make
sense
> anymore.  Thoughts?

Yes; getting rid of the AbstractMsgs would be cleaner I think. The
host/interface layer seems unnecessary now that we're Chrome-only.

[Peter Lundblad, 2014-09-23 14:42:29]

dtseng@chromium.org writes:
> On 2014/09/22 09:18:54, Peter Lundblad wrote:
> > mailto:dtseng@chromium.org writes:
> > >
> > >
> 
>
https://codereview.chromium.org/578303002/diff/20001/chrome/browser/resources...
> > > File
> > >  
> > chrome/browser/resources/chromeos/chromevox/host/interface/abstract_msgs.js
> > > (right):
> > >
> > >
> 
>
https://codereview.chromium.org/578303002/diff/20001/chrome/browser/resources...
> > >
> 
>
chrome/browser/resources/chromeos/chromevox/host/interface/abstract_msgs.js:57:
> > > /**
> > > Kind of odd to put this here. Why not place in ChromeMsgs?
> 
> > Since this is an abstract class and not a pure interface, I think it makes
> > sense to put functionality here that isn't specific to 'Chrome'. Adding it
> here
> > makes it available to cvox.TestMsgs as well.
> 
> > I could also go ahead and get rid of AbstractMsgs. It doesn't really make
> sense
> > anymore.  Thoughts?
> 
> Yes; getting rid of the AbstractMsgs would be cleaner I think. The
> host/interface layer seems unnecessary now that we're Chrome-only.

Sent out a separate cl to do that and rebased this on that one. PTAL.

Thanks,
//Peter

> https://codereview.chromium.org/578303002/

-- 

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

[David Tseng, 2014-09-23 17:41:50]

lgtm

https://codereview.chromium.org/578303002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/chromevox/messages/msgs.js
(right):

https://codereview.chromium.org/578303002/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox/messages/msgs.js:93: *
NOTE: Only a subset of locale identifiers are supported.  See the
Do we have a test that attempts to retrieve display names for every table we
support?

https://codereview.chromium.org/578303002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/host/chrome/braille_table.js
(right):

https://codereview.chromium.org/578303002/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/host/chrome/braille_table.js:18: *  
variant:(string|undefined),
Besides UEB, is variant used anywhere else?

https://codereview.chromium.org/578303002/diff/40001/chrome/browser/resources...
File
chrome/browser/resources/chromeos/chromevox/host/chrome/braille_table_test.extjs
(right):

https://codereview.chromium.org/578303002/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/host/chrome/braille_table_test.extjs:35:
expectTrue(cvox.BrailleTable.getDisplayName(table).length > 0);
Ok; here's the test.

https://codereview.chromium.org/578303002/diff/40001/third_party/liblouis/tab...
File third_party/liblouis/tables.json (right):

https://codereview.chromium.org/578303002/diff/40001/third_party/liblouis/tab...
third_party/liblouis/tables.json:63: "locale": "en",
nit: if we kept these sorted, would not have to do it at runtime (and would make
this file slightly easier to read).

[chromium-reviews, 2014-09-24 12:31:22]

dtseng@chromium.org writes:
> lgtm
> 
> 
> 
> 
>
https://codereview.chromium.org/578303002/diff/40001/chrome/browser/resources...
> File
> chrome/browser/resources/chromeos/chromevox/chromevox/messages/msgs.js
> (right):
> 
>
https://codereview.chromium.org/578303002/diff/40001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/chromevox/messages/msgs.js:93:
> * NOTE: Only a subset of locale identifiers are supported.  See the
> Do we have a test that attempts to retrieve display names for every
> table we support?

You answered your own question later on;)

>
https://codereview.chromium.org/578303002/diff/40001/chrome/browser/resources...
> File
> chrome/browser/resources/chromeos/chromevox/host/chrome/braille_table.js
> (right):
> 
>
https://codereview.chromium.org/578303002/diff/40001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/host/chrome/braille_table.js:18:
> *   variant:(string|undefined),
> Besides UEB, is variant used anywhere else?

Not for now.

>
https://codereview.chromium.org/578303002/diff/40001/chrome/browser/resources...
> File
>
chrome/browser/resources/chromeos/chromevox/host/chrome/braille_table_test.extjs
> (right):
> 
>
https://codereview.chromium.org/578303002/diff/40001/chrome/browser/resources...
>
chrome/browser/resources/chromeos/chromevox/host/chrome/braille_table_test.extjs:35:
> expectTrue(cvox.BrailleTable.getDisplayName(table).length > 0);
> Ok; here's the test.

Ack.

>
https://codereview.chromium.org/578303002/diff/40001/third_party/liblouis/tab...
> File third_party/liblouis/tables.json (right):
> 
>
https://codereview.chromium.org/578303002/diff/40001/third_party/liblouis/tab...
> third_party/liblouis/tables.json:63: "locale": "en",
> nit: if we kept these sorted, would not have to do it at runtime (and
> would make this file slightly easier to read).

I moved the id field first and sorted on that for ease of reading
the file.

When it comes to sorting, that is locale dependent, both because the names of
languages and countries are translated, but also because collating rules
vary by locale.


Thanks,
//Peter

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

[Peter Lundblad, 2014-09-24 12:33:49]

The CQ bit was checked by plundblad@chromium.org

[commit-bot: I haz the power, 2014-09-24 12:34:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/578303002/60001

[Peter Lundblad, 2014-09-24 12:52:12]

The CQ bit was unchecked by plundblad@chromium.org

[Peter Lundblad, 2014-09-24 13:00:45]

The CQ bit was checked by plundblad@chromium.org

[commit-bot: I haz the power, 2014-09-24 13:01:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/578303002/80001

[commit-bot: I haz the power, 2014-09-24 13:40:39]

Committed patchset #5 (id:80001) as 198d33aac8b58651c1505d57a72f494624ddd971

[commit-bot: I haz the power, 2014-09-24 13:41:11]

Patchset 5 (id:??) landed as
https://crrev.com/f29da0d1fd996ab76a674d9abe8c43a2285898e4
Cr-Commit-Position: refs/heads/master@{#296404}