Add experimental extension APIs to notify about wakeup and screen unlock

Add experimental extensions APIs to notify about wakeup and screen unlock

BUG=chromium-os:20009
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111076

[hashimoto, 2011-11-18 12:18:07]

http://codereview.chromium.org/8558014/diff/6002/chrome/browser/accessibility...
File chrome/browser/accessibility_events.h (right):

http://codereview.chromium.org/8558014/diff/6002/chrome/browser/accessibility...
chrome/browser/accessibility_events.h:75: virtual const char* type() const
OVERRIDE;
It looks like a good chance to add OVERRIDE

[dmazzoni, 2011-11-21 06:49:57]

LGTM.

I'm happy aside from one thing, below.

Suggesting aa@ for owners approval of chrome/browser/extensions/

http://codereview.chromium.org/8558014/diff/6002/chrome/browser/accessibility...
File chrome/browser/accessibility_events.h (right):

http://codereview.chromium.org/8558014/diff/6002/chrome/browser/accessibility...
chrome/browser/accessibility_events.h:290: class AccessibilityEmptyEventInfo :
public AccessibilityEventInfo {
Hmmm, I'm not sure I like having an "empty" subclass. I'd prefer to either (1)
make AccessibilityEventInfo not abstract, change the comments to indicate that
it can be used, or else (2) create separate subclasses for
ScreenUnlockedEventInfo and WakeUpEventInfo, in case we want to add information
to them later.

[Aaron Boodman, 2011-11-21 08:13:26]

I forget: is the accessibility API ever intended to go out of experimental, or
is it more of a private API indefinitely for Chrome OS?

I'm OK rubber-stamping this for now, but I'd want to think more carefully before
making it public. Because I still think that this is getting into the area of
general system status, and if we're going to release API for that, we should
consider doing it in a complete way.

[Zachary Kuznia, 2011-11-21 08:23:35]

We definitely plan to revisit the design before we would move out of
experimental.  Placing this in the accessibility API is mostly motivated by the
desire to test out the API to verify use cases without adding arbitrary top
level APIs in the mean time.

On 2011/11/21 08:13:26, Aaron Boodman wrote:
> I forget: is the accessibility API ever intended to go out of experimental, or
> is it more of a private API indefinitely for Chrome OS?
> 
> I'm OK rubber-stamping this for now, but I'd want to think more carefully
before
> making it public. Because I still think that this is getting into the area of
> general system status, and if we're going to release API for that, we should
> consider doing it in a complete way.

[Aaron Boodman, 2011-11-21 08:37:13]

On Mon, Nov 21, 2011 at 12:23 AM,  <zork@chromium.org> wrote:
> We definitely plan to revisit the design before we would move out of
> experimental.  Placing this in the accessibility API is mostly motivated by
> the
> desire to test out the API to verify use cases without adding arbitrary top
> level APIs in the mean time.

LGTM.

We typically we require that teams go back through extension API
review before moving out of experimental to fine-tune the API design.
We can revisit this then.

Can you mail me offline with your rough goal for when to move out, if
there is one?

Thanks,

- a

[dmazzoni, 2011-11-21 08:54:46]

>
> I forget: is the accessibility API ever intended to go out of
> experimental, or
> is it more of a private API indefinitely for Chrome OS?


I think the major reason to make this whole API public someday is because
ideally we'd like third-party developers to be able to innovate
accessibility solutions. However, that's a long-term goal, and I don't see
any reason to overdesign now while we're still figuring out what belongs in
the API.


> I'm OK rubber-stamping this for now, but I'd want to think more carefully
> before
> making it public. Because I still think that this is getting into the area
> of
> general system status, and if we're going to release API for that, we
> should
> consider doing it in a complete way.


If anyone comes up with another good use-case for any of these
notifications, I think we'd be happy to help pull them into a separate API
that could go public sooner. Do you think any of these overlap with any of
the top API goals for Extensions 2.0?

Can you mail me offline with your rough goal for when to move out, if
> there is one?
>

I'm thinking this wouldn't be for a while, like a year - again, if there
are other good uses for any of the APIs, let's launch those sooner, but for
anything that's just for accessibility, let's keep them private until our
own accessibility solution is feeling more complete and stable.

Idea: if your goal is to avoid having extension team members review
extension API changes that don't affect the public API, what would you
think about moving experimental extension APIs to a different .json file,
with more lax OWNERS? That would let people iterate rapidly on experimental
APIs and not worry about bothering the extensions team until they have
something they think is ready.

- Dominic

[hashimoto, 2011-11-21 12:36:27]

New patch set.

http://codereview.chromium.org/8558014/diff/6002/chrome/browser/accessibility...
File chrome/browser/accessibility_events.h (right):

http://codereview.chromium.org/8558014/diff/6002/chrome/browser/accessibility...
chrome/browser/accessibility_events.h:290: class AccessibilityEmptyEventInfo :
public AccessibilityEventInfo {
Created ScreenUnlockedEventInfo and WokeUpEventInfo instead.

On 2011/11/21 06:49:57, Dominic Mazzoni wrote:
> Hmmm, I'm not sure I like having an "empty" subclass. I'd prefer to either (1)
> make AccessibilityEventInfo not abstract, change the comments to indicate that
> it can be used, or else (2) create separate subclasses for
> ScreenUnlockedEventInfo and WakeUpEventInfo, in case we want to add
information
> to them later.

[dmazzoni, 2011-11-21 15:15:34]

lgtm

[commit-bot: I haz the power, 2011-11-21 16:22:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/8558014/14001

[commit-bot: I haz the power, 2011-11-21 17:25:44]

Can't apply patch for file chrome/browser/accessibility_events.h.
While running patch -p1 --forward --force;
patching file chrome/browser/accessibility_events.h
Hunk #1 FAILED at 72.
Hunk #2 FAILED at 81.
Hunk #3 FAILED at 90.
Hunk #4 FAILED at 103.
Hunk #5 FAILED at 128.
Hunk #6 FAILED at 149.
Hunk #7 FAILED at 177.
Hunk #8 FAILED at 207.
Hunk #9 FAILED at 240.
Hunk #10 FAILED at 268.
Hunk #11 FAILED at 278.
Hunk #12 FAILED at 295.
12 out of 12 hunks FAILED -- saving rejects to file
chrome/browser/accessibility_events.h.rej

[commit-bot: I haz the power, 2011-11-22 00:57:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/8558014/20001

[commit-bot: I haz the power, 2011-11-22 02:06:57]

Change committed as 111076

[Nikita (slow), 2011-11-22 08:35:58]

UserNotPassed
UserPassed

started failing with this CL as seen on the trybot. 

Expectation need to be changed as new AddObserver/RemoveObserver calls has been
added.

EXPECT_CALL(*mock_screen_lock_library_, AddObserver(_))
  .WillOnce(Return());

I'll take a look today if I'll have time.

[Nikita (slow), 2011-11-22 08:36:19]

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromeOS/bui...

[Aaron Boodman, 2011-11-22 08:58:09]

On Mon, Nov 21, 2011 at 12:54 AM, Dominic Mazzoni <dmazzoni@chromium.org> wrote:
>> I forget: is the accessibility API ever intended to go out of
>> experimental, or
>> is it more of a private API indefinitely for Chrome OS?
>
> I think the major reason to make this whole API public someday is because
> ideally we'd like third-party developers to be able to innovate
> accessibility solutions. However, that's a long-term goal, and I don't see
> any reason to overdesign now while we're still figuring out what belongs in
> the API.

Ok.

>> I'm OK rubber-stamping this for now, but I'd want to think more carefully
>> before
>> making it public. Because I still think that this is getting into the area
>> of
>> general system status, and if we're going to release API for that, we
>> should
>> consider doing it in a complete way.
>
> If anyone comes up with another good use-case for any of these
> notifications, I think we'd be happy to help pull them into a separate API
> that could go public sooner. Do you think any of these overlap with any of
> the top API goals for Extensions 2.0?

No.

>> Can you mail me offline with your rough goal for when to move out, if
>> there is one?
>
> I'm thinking this wouldn't be for a while, like a year - again, if there are
> other good uses for any of the APIs, let's launch those sooner, but for
> anything that's just for accessibility, let's keep them private until our
> own accessibility solution is feeling more complete and stable.

Ok.

> Idea: if your goal is to avoid having extension team members review
> extension API changes that don't affect the public API, what would you think
> about moving experimental extension APIs to a different .json file, with
> more lax OWNERS? That would let people iterate rapidly on experimental APIs
> and not worry about bothering the extensions team until they have something
> they think is ready.

Separating the JSON files is a high priority of mine. There are two
guys in the Sydney office that have signed up to work on it and I'm
really excited to see what they come up with.

The reason I'm asking all these questions is because the API is
labeled experimental. Some 'experimental' APIs are intended to be
released to users someday, and I thought it would be better to bring
up potential problems sooner rather than later.

It sounds like this API falls more into the 'private' category. There
are several other of these related to Chrome OS. I basically ignore
those because I look at them as an implementation detail of that
team's feature, not a potentially public part of the extension system.

If there are no concrete plans to release this in the near future and
you'd rather iterate more quickly, could it be made a private API?
Such APIs are typically only available to component extensions.

- a

[dmazzoni, 2011-11-23 06:26:04]

On Tue, Nov 22, 2011 at 12:57 AM, Aaron Boodman <aa@chromium.org> wrote:

> If there are no concrete plans to release this in the near future and
> you'd rather iterate more quickly, could it be made a private API?
> Such APIs are typically only available to component extensions.
>

That sounds great. It'd be much faster to iterate quickly on a private API
and then later pull out a subset of that into an experimental API that has
the potential to be public someday.

Could you point us to an example of a private API that would be a good one
to follow in terms of code location / style?

- Dominic