Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(355)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 13355002: Implement chrome.brailleDisplayPrivate API (Closed)

Created:
7 years, 8 months ago by Peter Lundblad
Modified:
7 years, 3 months ago
CC:
chromium-reviews, sadrul, nkostylev+watch_chromium.org, ben+watch_chromium.org, sail+watch_chromium.org, Aaron Boodman, arv+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Implement chrome.brailleDisplayPrivate API for ChromeOS This implementation uses libbrlapi, part of brltty. It is disabled by default and can be enabled by the use_brlapi gyp define when building. BUG=178559 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222492

Patch Set 1 #

Patch Set 2 : Add private braille display API. #

Patch Set 3 : #

Patch Set 4 : wip #

Patch Set 5 : Make the braille api work on chromeos. #

Patch Set 6 : Clean up unneeded dep. #

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Total comments: 1

Patch Set 10 : #

Patch Set 11 : Rename braillePrivate -> brailleDisplayPrivate and other tweaks. #

Patch Set 12 : #

Patch Set 13 : #

Patch Set 14 : fix clang compile error #

Patch Set 15 : #

Patch Set 16 : Temporarily default brlapi to be enabled on chromeos to run through trybots, this is now rebased on… #

Total comments: 14

Patch Set 17 : Style fixes and add support for two versions of libbrlapi when dlopening it. #

Total comments: 7

Patch Set 18 : Refactor and add api tests. #

Patch Set 19 : Build fixes #

Patch Set 20 : #

Total comments: 9

Patch Set 21 : Fix stylistic issues from last review round. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1061 lines, -26 lines) Patch
M build/common.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +3 lines, -0 lines 0 comments Download
M build/linux/system.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +65 lines, -0 lines 0 comments Download
A chrome/browser/extensions/api/braille_display_private/braille_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +64 lines, -0 lines 0 comments Download
A chrome/browser/extensions/api/braille_display_private/braille_controller_brlapi.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +309 lines, -0 lines 0 comments Download
A chrome/browser/extensions/api/braille_display_private/braille_controller_stub.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +70 lines, -0 lines 0 comments Download
M chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +8 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +17 lines, -3 lines 0 comments Download
A chrome/browser/extensions/api/braille_display_private/braille_display_private_apitest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +146 lines, -0 lines 0 comments Download
A chrome/browser/extensions/api/braille_display_private/brlapi_connection.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +68 lines, -0 lines 0 comments Download
A chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +184 lines, -0 lines 0 comments Download
M chrome/browser/resources/chromeos/access_chromevox/manifest.json View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_browser_extensions.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +21 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +6 lines, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/braille_display_private/key_events/manifest.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +6 lines, -11 lines 0 comments Download
A chrome/test/data/extensions/api_test/braille_display_private/key_events/test.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +46 lines, -0 lines 0 comments Download
A + chrome/test/data/extensions/api_test/braille_display_private/write_dots/manifest.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +6 lines, -11 lines 0 comments Download
A chrome/test/data/extensions/api_test/braille_display_private/write_dots/test.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +41 lines, -0 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
Peter Lundblad
@dtseng: early feedback welcome;)
7 years, 3 months ago (2013-09-05 01:10:44 UTC) #1
David Tseng
https://codereview.chromium.org/13355002/diff/29001/chrome/common/extensions/api/braille_private.idl File chrome/common/extensions/api/braille_private.idl (right): https://codereview.chromium.org/13355002/diff/29001/chrome/common/extensions/api/braille_private.idl#newcode1 chrome/common/extensions/api/braille_private.idl:1: // Copyright (c) 2012 The Chromium Authors. All rights ...
7 years, 3 months ago (2013-09-05 19:21:21 UTC) #2
Peter Lundblad
7 years, 3 months ago (2013-09-05 22:53:03 UTC) #3
plundblad
dtseng@chromium.org writes: > > > > > https://codereview.chromium.org/13355002/diff/29001/chrome/common/extensions/api/braille_private.idl > File chrome/common/extensions/api/braille_private.idl (right): > > https://codereview.chromium.org/13355002/diff/29001/chrome/common/extensions/api/braille_private.idl#newcode1 ...
7 years, 3 months ago (2013-09-05 22:53:17 UTC) #4
Peter Lundblad
Jorge, this is still under review, but this is how I plan to hook up ...
7 years, 3 months ago (2013-09-06 16:11:15 UTC) #5
dmazzoni
Please clarify the change description. Aura is now used on Windows, Chrome OS, and Linux ...
7 years, 3 months ago (2013-09-06 16:15:22 UTC) #6
Peter Lundblad
Clafiried cl description.
7 years, 3 months ago (2013-09-06 16:26:08 UTC) #7
dmazzoni
It'd be great to see a test. Even a relatively trivial test of the API ...
7 years, 3 months ago (2013-09-06 16:49:52 UTC) #8
Peter Lundblad
Hi Dominic, Thanks for the comments and pointers. dmazzoni@chromium.org writes: > It'd be great to ...
7 years, 3 months ago (2013-09-10 16:37:20 UTC) #9
Peter Lundblad
PTAL, ready for a next round.
7 years, 3 months ago (2013-09-10 16:39:29 UTC) #10
dmazzoni
lgtm https://codereview.chromium.org/13355002/diff/104001/chrome/browser/extensions/api/braille_display_private/braille_controller.h File chrome/browser/extensions/api/braille_display_private/braille_controller.h (right): https://codereview.chromium.org/13355002/diff/104001/chrome/browser/extensions/api/braille_display_private/braille_controller.h#newcode23 chrome/browser/extensions/api/braille_display_private/braille_controller.h:23: class BrlapiConnection; Why does this need to be ...
7 years, 3 months ago (2013-09-10 19:50:07 UTC) #11
Jorge Lucangeli Obes
Security lgtm. https://codereview.chromium.org/13355002/diff/104001/chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc File chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc (right): https://codereview.chromium.org/13355002/diff/104001/chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc#newcode18 chrome/browser/extensions/api/braille_display_private/brlapi_connection.cc:18: // under CrhomeOS (that is in aura ...
7 years, 3 months ago (2013-09-10 20:37:36 UTC) #12
Peter Lundblad
Adding Zilodrag for the chromevox manifest change. @zilodrag, this is a new private API specifically ...
7 years, 3 months ago (2013-09-10 22:53:46 UTC) #13
plundblad
dmazzoni@chromium.org writes: > lgtm > > > > > https://codereview.chromium.org/13355002/diff/104001/chrome/browser/extensions/api/braille_display_private/braille_controller.h > File > chrome/browser/extensions/api/braille_display_private/braille_controller.h > ...
7 years, 3 months ago (2013-09-10 23:00:52 UTC) #14
dmazzoni
On Tue, Sep 10, 2013 at 4:00 PM, Peter Nilsson Lundblad < plundblad@google.com> wrote: > ...
7 years, 3 months ago (2013-09-10 23:23:32 UTC) #15
Peter Lundblad
@zelidrag (now with correctly spelled name;): could you just have a look at the chromevox ...
7 years, 3 months ago (2013-09-11 01:39:43 UTC) #16
zel
LGTM for chrome/browser/resources/chromeos/ part
7 years, 3 months ago (2013-09-11 01:49:35 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/plundblad@chromium.org/13355002/143001
7 years, 3 months ago (2013-09-11 01:57:04 UTC) #18
commit-bot: I haz the power
Change committed as 222492
7 years, 3 months ago (2013-09-11 11:48:26 UTC) #19
plundblad
7 years, 3 months ago (2013-09-11 17:35:08 UTC) #20 
Hi,



Dominic Mazzoni writes:
> On Tue, Sep 10, 2013 at 4:00 PM, Peter Nilsson Lundblad
<plundblad@google.com>
> wrote:
> 
>     It doesn't srictly have to, but since its user is, I also put this one in
>     one.
> 
> 
> I guess I prefer to avoid ifdefs in header files if at all possible. If it's
> possible to make the header
> file ifdef-free I'd encourage that - but not if that would dramatically
> increase the ugliness elsewhere.

Since I was adding a virtual method, I'd have to add that in the stub, and since
this was only for testing, I choose to leave it in.


>     Not sure how I would inject the braillecontroller subclass since it is a
>     Singleton.  Seems like this is a commo npattern to have some dependency
>     injection method for testing like this.
> 
> 
> OK, that's fine.
> 
> 
>     STREQ is for C strings AFAICT.  These are not C strings because they can
>     contain nulls.
> 
> 
> Oh! Does == actually work for strings that contain nulls here? Might be worth
> commenting somewhere.

C++ strings can have null characters (it's only that c_str() is not that useful
then).  == works for such strings.

>     Tried to clarify.  There are only multiple threads if different
>     connections are used on different threads.  This is like errno in C (not
>     my decision;).
> 
> 
> We're only ever using this from one thread, though, right? Maybe we should
> clarify that. It seems like the detail about multiple threads is not
important
> at this level of abstraction if this API is never intended to be used by
> multiple threads anyway.

There's nothing preventing different instances of this class from being used
from dfferent threads, so then I think it is good to know that the error
is per thread and clbbered by future calls.  Basically this is reflecting the
brlapi implementation.


>     > I don't think you need this, right?
> 
>     It's used to generate the extension id for component extensions and
CHECKs
>     if there's no key.
> 
> 
> Even in a test, though? I didn't think that test extensions needed a key.

I get
[14656:14656:0911/102805:782199899483:FATAL:component_loader.cc(93)] Check
failed: manifest->GetString(manifest_keys::kPublicKey, &raw_key).
if I remove that line.

Incidentally, I stole the key from another private api test;)

Thaks,
//Peter


-- 

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

Powered by Google App Engine
This is Rietveld 408576698