Add stubs for brailleDisplayPrivate extension API.

Add stubs for brailleDisplayPrivate extension API.

BUG=178559
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221262

[not at google - send to devlin, 2013-09-03 14:35:34]

just one comment really, with the threading implementation of this function. it
depends on what the interface to the braille display system is really.

Incidentally depending on complexity I actually slightly prefer dropping a
functional API all at once so that you can see such threading issues.
Complexity.. maybe up to a couple hundred lines.

Lastly: add an OWNERS file for the new API implementation directory.

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
File
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h
(right):

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:13:

Comment; but can be "implementation of the chrome.brailleDisplayPrivate API".

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:38:
class BrailleDisplayPrivateGetDisplayStateFunction : public AsyncApiFunction {
the "AsyncApiFunction" class is for extension functions implemented cross-thread
(typically IO vs UI). is that the situation that the braille devices need?

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:51:
public:
ideally, 1 less indent space here, and a line break between the public,
protected, and private blocks.

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:60:
};
and a line break here

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:62:
}  // namespace extensions
and after here

https://codereview.chromium.org/23717020/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/23717020/diff/16001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:88: },
you want to whitelist this for chromevox, or at least for component extensions,
right?

[Peter Lundblad, 2013-09-03 23:03:34]

PTAL

Thanks for the review!

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
File
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h
(right):

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:13:

On 2013/09/03 14:35:35, kalman wrote:
> Comment; but can be "implementation of the chrome.brailleDisplayPrivate API".

Done.

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:38:
class BrailleDisplayPrivateGetDisplayStateFunction : public AsyncApiFunction {
On 2013/09/03 14:35:35, kalman wrote:
> the "AsyncApiFunction" class is for extension functions implemented
cross-thread
> (typically IO vs UI). is that the situation that the braille devices need?

The API uses a library that communicates with a deamon over a local socket. I
thought the IO thread would be good for that (the communication is nonblocking).
Since getting the display state (size in particular) potentially calls the
daemon, this should happen on the IO thread I think.

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:51:
public:
On 2013/09/03 14:35:35, kalman wrote:
> ideally, 1 less indent space here, and a line break between the public,
> protected, and private blocks.

Done.

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:60:
};
On 2013/09/03 14:35:35, kalman wrote:
> and a line break here

Done.

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:62:
}  // namespace extensions
On 2013/09/03 14:35:35, kalman wrote:
> and after here

Done.

https://codereview.chromium.org/23717020/diff/16001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/23717020/diff/16001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:88: },
On 2013/09/03 14:35:35, kalman wrote:
> you want to whitelist this for chromevox, or at least for component
extensions,
> right?

Whitelisted for component apps.

[not at google - send to devlin, 2013-09-03 23:40:18]

lgtm

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
File
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h
(right):

https://codereview.chromium.org/23717020/diff/16001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:38:
class BrailleDisplayPrivateGetDisplayStateFunction : public AsyncApiFunction {
On 2013/09/03 23:03:34, Peter Lundblad wrote:
> On 2013/09/03 14:35:35, kalman wrote:
> > the "AsyncApiFunction" class is for extension functions implemented
> cross-thread
> > (typically IO vs UI). is that the situation that the braille devices need?
> 
> The API uses a library that communicates with a deamon over a local socket. I
> thought the IO thread would be good for that (the communication is
nonblocking).
> Since getting the display state (size in particular) potentially calls the
> daemon, this should happen on the IO thread I think.

ok sounds good.

[David Tseng, 2013-09-03 23:57:16]

https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions...
File
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc
(right):

https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc:11:
namespace WriteDots = extensions::api::braille_display_private::WriteDots;
nit: sort.

https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions...
File
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h
(right):

https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:35:
static const bool kServiceIsNULLWhileTesting = true;
Add a small comment to explain this.

https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions...
chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h:38:
namespace api {
nit: I think you want a blank line here.

https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions...
chrome/browser/extensions/extension_function_histogram_value.h:625:
BRAILLEDISPLAYPRIVATE_WRITETEXT,
Obsolete?

[David Tseng, 2013-09-04 00:22:35]

lgtm once you remove the obsolete enum.




On Tue, Sep 3, 2013 at 4:57 PM, <dtseng@chromium.org> wrote:

>
> https://codereview.chromium.**org/23717020/diff/36001/**
> chrome/browser/extensions/api/**braille_display_private/**
>
braille_display_private_api.cc<https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc>
> File
> chrome/browser/extensions/api/**braille_display_private/**
> braille_display_private_api.cc
> (right):
>
> https://codereview.chromium.**org/23717020/diff/36001/**
> chrome/browser/extensions/api/**braille_display_private/**
>
braille_display_private_api.**cc#newcode11<https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions/api/braille_display_private/braille_display_private_api.cc#newcode11>
> chrome/browser/extensions/api/**braille_display_private/**
> braille_display_private_api.**cc:11:
> namespace WriteDots =
> extensions::api::braille_**display_private::WriteDots;
> nit: sort.
>
> https://codereview.chromium.**org/23717020/diff/36001/**
> chrome/browser/extensions/api/**braille_display_private/**
>
braille_display_private_api.h<https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h>
> File
> chrome/browser/extensions/api/**braille_display_private/**
> braille_display_private_api.h
> (right):
>
> https://codereview.chromium.**org/23717020/diff/36001/**
> chrome/browser/extensions/api/**braille_display_private/**
>
braille_display_private_api.h#**newcode35<https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h#newcode35>
> chrome/browser/extensions/api/**braille_display_private/**
> braille_display_private_api.h:**35:
> static const bool kServiceIsNULLWhileTesting = true;
> Add a small comment to explain this.
>
> https://codereview.chromium.**org/23717020/diff/36001/**
> chrome/browser/extensions/api/**braille_display_private/**
>
braille_display_private_api.h#**newcode38<https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions/api/braille_display_private/braille_display_private_api.h#newcode38>
> chrome/browser/extensions/api/**braille_display_private/**
> braille_display_private_api.h:**38:
> namespace api {
> nit: I think you want a blank line here.
>
> https://codereview.chromium.**org/23717020/diff/36001/**
>
chrome/browser/extensions/**extension_function_histogram_**value.h<https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions/extension_function_histogram_value.h>
> File chrome/browser/extensions/**extension_function_histogram_**value.h
> (right):
>
> https://codereview.chromium.**org/23717020/diff/36001/**
> chrome/browser/extensions/**extension_function_histogram_**
>
value.h#newcode625<https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions/extension_function_histogram_value.h#newcode625>
> chrome/browser/extensions/**extension_function_histogram_**value.h:625:
> BRAILLEDISPLAYPRIVATE_**WRITETEXT,
> Obsolete?
>
>
https://codereview.chromium.**org/23717020/<https://codereview.chromium.org/2...
>

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

[commit-bot: I haz the power, 2013-09-04 01:29:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/plundblad@chromium.org/23717020/67001

[commit-bot: I haz the power, 2013-09-04 01:29:54]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 621.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
97ca08df7ea877364791facdc71f3104ddcd32a2..baa852d780962ea6bf7c4df226d0b84e011a5273
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -621,6 +621,8 @@ enum HistogramValue {
   FILEBROWSERPRIVATE_INSTALLWEBSTOREITEM,
   FILEBROWSERPRIVATE_STARTCOPY,
   FILEBROWSERPRIVATE_CANCELCOPY,
+  BRAILLEDISPLAYPRIVATE_GETDISPLAYSTATE,
+  BRAILLEDISPLAYPRIVATE_WRITEDOTS,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[Peter Lundblad, 2013-09-04 01:35:33]

https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/23717020/diff/36001/chrome/browser/extensions...
chrome/browser/extensions/extension_function_histogram_value.h:625:
BRAILLEDISPLAYPRIVATE_WRITETEXT,
On 2013/09/03 23:57:17, David Tseng wrote:
> Obsolete?

Removed, thanks for catching!

[commit-bot: I haz the power, 2013-09-04 16:38:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/plundblad@chromium.org/23717020/77001

[commit-bot: I haz the power, 2013-09-04 17:08:26]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Peter Lundblad, 2013-09-04 17:48:43]

erg, rlp, , I need OWNERS approval in  chrome/browser/profiles.

[Elliot Glaysher, 2013-09-04 18:03:15]

profiles lgtm

[commit-bot: I haz the power, 2013-09-04 18:07:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/plundblad@chromium.org/23717020/77001

[commit-bot: I haz the power, 2013-09-04 20:44:30]

Change committed as 221262