Scaffolding for an experimental discovery API letting users inject links in the recommended pane of…

Scaffolding for an experimental discovery API letting users inject links in the recommended pane of the New Tab Page.

Documentation changes can be seen at:
http://www.corp.google.com/~beaudoin/no_crawl/docs/experimental.discovery.html

The following files don't have to be reviewed as they are generated by build.py:

chrome/common/extensions/docs/experimental.discovery.html
chrome/common/extensions/docs/experimental.html
chrome/common/extensions/docs/samples.json	

BUG=none
TEST=none

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

[beaudoin, 2012-05-10 14:51:28]

Hi Aaron,

Following the email and the proposal for a discovery API. Comments welcome on
the patch, or I'm happy to wait until the review next Wednesday. However, I'll
be building on the stuff in this CL until then so if you notice I'm out in the
left field please let me know. :)

[Aaron Boodman, 2012-05-11 21:06:46]

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/api...
File chrome/browser/extensions/api/discovery/discovery_api.cc (right):

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/api...
chrome/browser/extensions/api/discovery/discovery_api.cc:28:
EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details));
Use JSONSchemaCompiler to parse arguments. See alarms API for an example.

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/api...
chrome/browser/extensions/api/discovery/discovery_api.cc:45:
manager->Add(extension_id(), new ExtensionSuggestedLink(link_url, link_text,
Wrapping looks wrong. I think 'new ' should start on a new line.

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/api...
File chrome/browser/extensions/api/discovery/discovery_api.h (right):

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/api...
chrome/browser/extensions/api/discovery/discovery_api.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
You should test this API code. See alarms API for an example of how extension
API testing is done nowadays.

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_suggested_link.h (right):

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_suggested_link.h:15: // wants to inject in
the NTP's recommended pane.
This and related classes should all be in the c/b/e/api/discovery directory.

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_suggested_link.h:17: 
Nit: extraneous blank line.

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_suggested_links_manager.h (right):

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_suggested_links_manager.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
You should unit test this middle-level code.

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_suggested_links_manager.h:15: // This class
keeps track of links suggested by an extension using the discovery
Are the suggestions not saved anywhere? I think they need to be. It's
inefficient to start the extension on every browser startup, just so that it can
suggest these links. It should only do it when it has new links to suggest.
Also, the way you have it means that the extension startup needs to block NTP,
which we don't want to do.

I think that the links should be saved in prefs. There should be a limit to how
many links can be suggested per-extension, like 3 or 5 to avoid bloating the
preferences.

If you make that change, you can get rid of this manager object with
profile-lifetime. Instead you can just have a little helper class that can read
and write from preferences and which is created when needed.

http://codereview.chromium.org/10391034/diff/48/chrome/common/extensions/api/...
File chrome/common/extensions/api/experimental.discovery.json (right):

http://codereview.chromium.org/10391034/diff/48/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental.discovery.json:29: "description": "A
score indicating how interesting that suggestion is. A score of 2 is twice as
likely to be displayed than a score of 1. Defaults to 1."
What is the range of this number? Are non-integral values allowed? You can
tighten the specification quite a bit.

http://codereview.chromium.org/10391034/diff/48/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental.discovery.json:36: "name":
"clearSuggestion",
Should there be a way to get the current suggestions?

http://codereview.chromium.org/10391034/diff/48/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental.discovery.json:36: "name":
"clearSuggestion",
Suggest "removeSuggestion". "clear" usually means to remove all.

[beaudoin, 2012-05-12 03:16:29]

Some answers to the comments, for discussion.

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/api...
File chrome/browser/extensions/api/discovery/discovery_api.cc (right):

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/api...
chrome/browser/extensions/api/discovery/discovery_api.cc:45:
manager->Add(extension_id(), new ExtensionSuggestedLink(link_url, link_text,
On 2012/05/11 21:06:46, Aaron Boodman wrote:
> Wrapping looks wrong. I think 'new ' should start on a new line.

Done.

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_suggested_link.h (right):

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_suggested_link.h:15: // wants to inject in
the NTP's recommended pane.
On 2012/05/11 21:06:46, Aaron Boodman wrote:
> This and related classes should all be in the c/b/e/api/discovery directory.

I was strongly inspired by c/b/e/app_notification* for this. Has the recommended
directory layout changed since?

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_suggested_link.h:17: 
On 2012/05/11 21:06:46, Aaron Boodman wrote:
> Nit: extraneous blank line.

Done.

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_suggested_links_manager.h (right):

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_suggested_links_manager.h:15: // This class
keeps track of links suggested by an extension using the discovery
On 2012/05/11 21:06:46, Aaron Boodman wrote:
> Are the suggestions not saved anywhere? I think they need to be. It's
> inefficient to start the extension on every browser startup, just so that it
can
> suggest these links. It should only do it when it has new links to suggest.
> Also, the way you have it means that the extension startup needs to block NTP,
> which we don't want to do.
> 
> I think that the links should be saved in prefs. There should be a limit to
how
> many links can be suggested per-extension, like 3 or 5 to avoid bloating the
> preferences.
> 
> If you make that change, you can get rid of this manager object with
> profile-lifetime. Instead you can just have a little helper class that can
read
> and write from preferences and which is created when needed.

The expected usage is for the links to be dynamically generated with freshness
in mind, so for typical use cases links stored in preferences would be stale and
uninteresting. In fact, they may even be detrimental since, currently, the
NTP-recommended pane fills its spots with top links from history when no
extension can provide discovery links.

Regarding blocking the NTP on extension startup, I believe the solution should
be in the NTP logic, where it should not wait on extension startup. In fact, the
model I'm working on (for the next CL) already does this: using only
recommendations from currently started-up and running extensions.

[Aaron Boodman, 2012-05-12 05:00:09]

On 2012/05/12 03:16:29, beaudoin wrote:
> Some answers to the comments, for discussion.
> 
>
http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/api...
> File chrome/browser/extensions/api/discovery/discovery_api.cc (right):
> 
>
http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/api...
> chrome/browser/extensions/api/discovery/discovery_api.cc:45:
> manager->Add(extension_id(), new ExtensionSuggestedLink(link_url, link_text,
> On 2012/05/11 21:06:46, Aaron Boodman wrote:
> > Wrapping looks wrong. I think 'new ' should start on a new line.
> 
> Done.
> 
>
http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
> File chrome/browser/extensions/extension_suggested_link.h (right):
> 
>
http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
> chrome/browser/extensions/extension_suggested_link.h:15: // wants to inject in
> the NTP's recommended pane.
> On 2012/05/11 21:06:46, Aaron Boodman wrote:
> > This and related classes should all be in the c/b/e/api/discovery directory.
> 
> I was strongly inspired by c/b/e/app_notification* for this. Has the
recommended
> directory layout changed since?

Yes, sorry for the out of date examples. In fact, maybe you could help solve
this problem for others... consider adding a README file to c/b/e/api/ with the
steps to add an API.

>
http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
> chrome/browser/extensions/extension_suggested_link.h:17: 
> On 2012/05/11 21:06:46, Aaron Boodman wrote:
> > Nit: extraneous blank line.
> 
> Done.
> 
>
http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
> File chrome/browser/extensions/extension_suggested_links_manager.h (right):
> 
>
http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
> chrome/browser/extensions/extension_suggested_links_manager.h:15: // This
class
> keeps track of links suggested by an extension using the discovery
> On 2012/05/11 21:06:46, Aaron Boodman wrote:
> > Are the suggestions not saved anywhere? I think they need to be. It's
> > inefficient to start the extension on every browser startup, just so that it
> can
> > suggest these links. It should only do it when it has new links to suggest.
> > Also, the way you have it means that the extension startup needs to block
NTP,
> > which we don't want to do.
> > 
> > I think that the links should be saved in prefs. There should be a limit to
> how
> > many links can be suggested per-extension, like 3 or 5 to avoid bloating the
> > preferences.
> > 
> > If you make that change, you can get rid of this manager object with
> > profile-lifetime. Instead you can just have a little helper class that can
> read
> > and write from preferences and which is created when needed.
> 
> The expected usage is for the links to be dynamically generated with freshness
> in mind

Generated when? I was imaging something using the alarms API:

http://code.google.com/chrome/extensions/trunk/experimental.alarms.html

Extension wakes up every few hours, gets the latest suggestions, and pushes them
into storage.

> Regarding blocking the NTP on extension startup, I believe the solution should
> be in the NTP logic, where it should not wait on extension startup.

I was just meant to point out that if you do that, any NTP at browser startup
won't show any links at all (because extensions haven't started yet).

Maybe it would be better to turn this inside out and pull the links from the
extension rather than push them from the extension? We could make that part of
the NTP load asynchronously.

[beaudoin, 2012-05-14 23:28:02]

Thanks for the initial review Aaron. I made most of the changes, the only thing
still missing are the tests. If you want to make sure I did not mess up moving
from JSON to IDL that would be really appreciated.

DONE:
- Wrote README. We should probably update:
http://www.chromium.org/developers/design-documents/extensions/proposed-chang...
- Switched from JSON to IDL.
- Moved the support classes to c/b/e/api/discovery
- Made extension_suggested_links_manager a ProfileKeyedService
- Regenerated documentation

STILL TODO:
- Write the suggested tests.

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/api...
File chrome/browser/extensions/api/discovery/discovery_api.cc (right):

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/api...
chrome/browser/extensions/api/discovery/discovery_api.cc:28:
EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &details));
On 2012/05/11 21:06:46, Aaron Boodman wrote:
> Use JSONSchemaCompiler to parse arguments. See alarms API for an example.

Done.

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_suggested_link.h (right):

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_suggested_link.h:15: // wants to inject in
the NTP's recommended pane.
On 2012/05/12 03:16:29, beaudoin wrote:
> On 2012/05/11 21:06:46, Aaron Boodman wrote:
> > This and related classes should all be in the c/b/e/api/discovery directory.
> 
> I was strongly inspired by c/b/e/app_notification* for this. Has the
recommended
> directory layout changed since?

Done.

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_suggested_links_manager.h (right):

http://codereview.chromium.org/10391034/diff/48/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_suggested_links_manager.h:15: // This class
keeps track of links suggested by an extension using the discovery
On 2012/05/12 03:16:29, beaudoin wrote:
> On 2012/05/11 21:06:46, Aaron Boodman wrote:
> > Are the suggestions not saved anywhere? I think they need to be. It's
> > inefficient to start the extension on every browser startup, just so that it
> can
> > suggest these links. It should only do it when it has new links to suggest.
> > Also, the way you have it means that the extension startup needs to block
NTP,
> > which we don't want to do.
> > 
> > I think that the links should be saved in prefs. There should be a limit to
> how
> > many links can be suggested per-extension, like 3 or 5 to avoid bloating the
> > preferences.
> > 
> > If you make that change, you can get rid of this manager object with
> > profile-lifetime. Instead you can just have a little helper class that can
> read
> > and write from preferences and which is created when needed.
> 
> The expected usage is for the links to be dynamically generated with freshness
> in mind, so for typical use cases links stored in preferences would be stale
and
> uninteresting. In fact, they may even be detrimental since, currently, the
> NTP-recommended pane fills its spots with top links from history when no
> extension can provide discovery links.
> 
> Regarding blocking the NTP on extension startup, I believe the solution should
> be in the NTP logic, where it should not wait on extension startup. In fact,
the
> model I'm working on (for the next CL) already does this: using only
> recommendations from currently started-up and running extensions.
> 

Discussed in side thread.

http://codereview.chromium.org/10391034/diff/48/chrome/common/extensions/api/...
File chrome/common/extensions/api/experimental.discovery.json (right):

http://codereview.chromium.org/10391034/diff/48/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental.discovery.json:29: "description": "A
score indicating how interesting that suggestion is. A score of 2 is twice as
likely to be displayed than a score of 1. Defaults to 1."
On 2012/05/11 21:06:46, Aaron Boodman wrote:
> What is the range of this number? Are non-integral values allowed? You can
> tighten the specification quite a bit.

Forced to be within the 0-1 range. Added a check (see discovery_api.cc)

http://codereview.chromium.org/10391034/diff/48/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental.discovery.json:36: "name":
"clearSuggestion",
On 2012/05/11 21:06:46, Aaron Boodman wrote:
> Suggest "removeSuggestion". "clear" usually means to remove all.

Done.

[Aaron Boodman, 2012-05-15 00:07:43]

+yoz for PKS goop

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/api/README.txt (right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/README.txt:13: 1) Write your API specification.
We shouldn't recommend that people use IDL, because support for that is not yet
complete. There are some features of the JSON Schema system that are not exposed
yet.

(In your case, it's fine to leave as IDL though)

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/README.txt:22: Create foo_api.cc and foo_api.h
under "chrome/browser/extensions/api/foo".
Can you add a statement recommending use of JSON Schema Compiler and an example
of where to look?

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/README.txt:27: directly to
chrome/browser/extensions. Don't do that.
Insert a step about testing here?

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/api/discovery/discovery_api.cc (right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/discovery_api.cc:19: }  // anonymous
namespace
Typically this comment just says 'namespace' not 'anonymous namespace'

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/api/discovery/extension_suggested_link.h (right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_link.h:16: class
ExtensionSuggestedLink {
This class should be extensions::SuggestedLink, not ExtensionSuggestedLink. You
can also rename the file to just suggested_link.h.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_link.h:19: const
std::string& link_text_, double score);
Indent doesn't seem right. I think it is supposed to line up with the first
param, and each param should be on its own line in declarations.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_link.h:22:
std::string link_url() const { return link_url_; }
Return type should be const ref.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_link.h:35: typedef
std::vector<linked_ptr<ExtensionSuggestedLink> >
It usually ends up making more sense to define these typedefs where they are
used.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.cc
(right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.cc:12:
// implicit copy constructor to be used with std::find.
Blech, std::find. Not worth the trouble. Just loop manually.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.cc:75:
// |insert| returns the element if it's already in the map.
Cool trick, never seen that one.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h
(right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h:17:
class ExtensionSuggestedLinksManager
extensions::SuggestedLinkManager. Also rename file to suggested_link_manager.h.

Also, I have kind of an aversion to classes called 'Manager', because they are
so general.

What about SuggestedLinkRegistry?

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h:23:
void Add(const std::string& extension_id, ExtensionSuggestedLink* item);
You can declare item as scoped_ptr<ExtensionSuggestedLink> item to enforce the
ownership transfer.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h:25:
const ExtensionSuggestedLinkList* GetAll(
Comment.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h:28:
void Clear(const std::string& extension_id, const std::string& linkUrl);
Comment.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h:28:
void Clear(const std::string& extension_id, const std::string& linkUrl);
link_url

http://codereview.chromium.org/10391034/diff/14001/chrome/common/extensions/d...
File chrome/common/extensions/docs/js/api_page_generator.js (right):

http://codereview.chromium.org/10391034/diff/14001/chrome/common/extensions/d...
chrome/common/extensions/docs/js/api_page_generator.js:39:
'../api/experimental_discovery.json',  // autogenerated
Add this step to your README.

[Yoyo Zhou, 2012-05-15 00:38:04]

PKS LGTM, except that you should add this factory to the list in
profile_dependency_manager.cc in AssertFactoriesBuilt.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.cc
(right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.cc:11:
// A functor that mathes an ExtensionSuggestedLink having a given URL. Needs an
typo: matches

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/app_notification_manager.h (right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/app_notification_manager.h:129: // Gets notificiations
for a given extension id.
nit: Still not spelled right. =)

[beaudoin, 2012-05-16 16:56:16]

Answered all the comments save for the ones about test. I'll get working on
tests as soon as this is approved. Not sure I'm correctly using scoped_ptr<> for
ownership transfer, comments appreciated. :)

Thanks again for the review!

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/api/README.txt (right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/README.txt:13: 1) Write your API specification.
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> We shouldn't recommend that people use IDL, because support for that is not
yet
> complete. There are some features of the JSON Schema system that are not
exposed
> yet.
> 
> (In your case, it's fine to leave as IDL though)

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/README.txt:22: Create foo_api.cc and foo_api.h
under "chrome/browser/extensions/api/foo".
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> Can you add a statement recommending use of JSON Schema Compiler and an
example
> of where to look?

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/api/discovery/discovery_api.cc (right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/discovery_api.cc:19: }  // anonymous
namespace
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> Typically this comment just says 'namespace' not 'anonymous namespace'

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/api/discovery/extension_suggested_link.h (right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_link.h:16: class
ExtensionSuggestedLink {
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> This class should be extensions::SuggestedLink, not ExtensionSuggestedLink.
You
> can also rename the file to just suggested_link.h.

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_link.h:19: const
std::string& link_text_, double score);
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> Indent doesn't seem right. I think it is supposed to line up with the first
> param, and each param should be on its own line in declarations.

I've been doing too much JavaScript. :)

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_link.h:22:
std::string link_url() const { return link_url_; }
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> Return type should be const ref.

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_link.h:35: typedef
std::vector<linked_ptr<ExtensionSuggestedLink> >
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> It usually ends up making more sense to define these typedefs where they are
> used.

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.cc
(right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.cc:11:
// A functor that mathes an ExtensionSuggestedLink having a given URL. Needs an
On 2012/05/15 00:38:04, Yoyo Zhou wrote:
> typo: matches

Ack. (Gone.)

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.cc:12:
// implicit copy constructor to be used with std::find.
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> Blech, std::find. Not worth the trouble. Just loop manually.

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.cc:75:
// |insert| returns the element if it's already in the map.
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> Cool trick, never seen that one.

Ack.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h
(right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h:17:
class ExtensionSuggestedLinksManager
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> extensions::SuggestedLinkManager. Also rename file to
suggested_link_manager.h.
> 
> Also, I have kind of an aversion to classes called 'Manager', because they are
> so general.
> 
> What about SuggestedLinkRegistry?

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h:23:
void Add(const std::string& extension_id, ExtensionSuggestedLink* item);
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> You can declare item as scoped_ptr<ExtensionSuggestedLink> item to enforce the
> ownership transfer.

Not sure I did that correctly, in particular I'm not 100% clear on the
difference between .Pass() and .release(). More info welcome! :)

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h:25:
const ExtensionSuggestedLinkList* GetAll(
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> Comment.

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h:28:
void Clear(const std::string& extension_id, const std::string& linkUrl);
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> Comment.

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h:28:
void Clear(const std::string& extension_id, const std::string& linkUrl);
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> Comment.

Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/app_notification_manager.h (right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/app_notification_manager.h:129: // Gets notificiations
for a given extension id.
On 2012/05/15 00:38:04, Yoyo Zhou wrote:
> nit: Still not spelled right. =)

LOL! :) 
Done.

http://codereview.chromium.org/10391034/diff/14001/chrome/common/extensions/d...
File chrome/common/extensions/docs/js/api_page_generator.js (right):

http://codereview.chromium.org/10391034/diff/14001/chrome/common/extensions/d...
chrome/common/extensions/docs/js/api_page_generator.js:39:
'../api/experimental_discovery.json',  // autogenerated
On 2012/05/15 00:07:43, Aaron Boodman wrote:
> Add this step to your README.

Done.

[beaudoin, 2012-05-16 17:01:48]

(Spotted a problem in PatchSet 4, re-sending.)

[Aaron Boodman, 2012-05-17 00:05:29]

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
File chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h
(right):

http://codereview.chromium.org/10391034/diff/14001/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/extension_suggested_links_manager.h:23:
void Add(const std::string& extension_id, ExtensionSuggestedLink* item);
On 2012/05/16 16:56:16, beaudoin wrote:
> On 2012/05/15 00:07:43, Aaron Boodman wrote:
> > You can declare item as scoped_ptr<ExtensionSuggestedLink> item to enforce
the
> > ownership transfer.
> 
> Not sure I did that correctly, in particular I'm not 100% clear on the
> difference between .Pass() and .release(). More info welcome! :)
> 
> Done.

release() is appropriate here. Pass() is for when you want to transfer ownership
between two scoped pointers. If you tried to use Pass() here, the compiler would
tell you you were wrong anyway, because the result of Pass() is scoped_ptr<T>.

http://codereview.chromium.org/10391034/diff/10003/chrome/browser/extensions/...
File chrome/browser/extensions/api/README.txt (right):

http://codereview.chromium.org/10391034/diff/10003/chrome/browser/extensions/...
chrome/browser/extensions/api/README.txt:9: Discuss with a member of the
extensions team (aa@chromium.org) before you decide
80 cols

http://codereview.chromium.org/10391034/diff/10003/chrome/browser/extensions/...
chrome/browser/extensions/api/README.txt:28: at the "app" API for the parsing
mechanism.
Should use JSON schema compiler this way too.

http://codereview.chromium.org/10391034/diff/10003/chrome/browser/extensions/...
chrome/browser/extensions/api/README.txt:70: documentation to be properly
generated. Do this now.
Really? What happens if you don't?

http://codereview.chromium.org/10391034/diff/10003/chrome/browser/extensions/...
File chrome/browser/extensions/api/discovery/suggested_link.h (right):

http://codereview.chromium.org/10391034/diff/10003/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/suggested_link.h:31: double score_;
You should add a comment for this field since its meaning is non-obvious.

[Aaron Boodman, 2012-05-17 00:33:25]

LGTM (you don't need to wait for another LG from me before committing, but
please fix the nits I picked in the previous comment)

[commit-bot: I haz the power, 2012-05-17 19:00:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/10391034/10004

[commit-bot: I haz the power, 2012-05-17 19:01:01]

Presubmit check for 10391034-10004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/profiles

Presubmit checks took 2.5s to calculate.

[Miranda Callahan, 2012-05-17 19:10:45]

On 2012/05/17 19:01:01, I haz the power (commit-bot) wrote:
> Presubmit check for 10391034-10004 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome/browser/profiles
> 
> Presubmit checks took 2.5s to calculate.

profiles/* LGTM.

[beaudoin, 2012-05-17 19:11:16]

http://codereview.chromium.org/10391034/diff/10003/chrome/browser/extensions/...
File chrome/browser/extensions/api/README.txt (right):

http://codereview.chromium.org/10391034/diff/10003/chrome/browser/extensions/...
chrome/browser/extensions/api/README.txt:9: Discuss with a member of the
extensions team (aa@chromium.org) before you decide
On 2012/05/17 00:05:29, Aaron Boodman wrote:
> 80 cols

Done.

http://codereview.chromium.org/10391034/diff/10003/chrome/browser/extensions/...
chrome/browser/extensions/api/README.txt:28: at the "app" API for the parsing
mechanism.
On 2012/05/17 00:05:29, Aaron Boodman wrote:
> Should use JSON schema compiler this way too.

Ah! This is what you wanted me to do. :) Looking at alarms_api.cc it wasn't
clear to me how to do this without an IDL since the parameter parsing code
relies on generated code with 3 levels of nested namespaces. Are we expected to
write that kind of code by hand? If so, is there an example of an JSON-based API
that uses the JSON schema compiler?

http://codereview.chromium.org/10391034/diff/10003/chrome/browser/extensions/...
chrome/browser/extensions/api/README.txt:70: documentation to be properly
generated. Do this now.
On 2012/05/17 00:05:29, Aaron Boodman wrote:
> Really? What happens if you don't?

The documentation is generated from the JSON file which is generated when you
build the project. If you run build.py without building the project you end up
with an empty documentation. At least that's what my investigation showed -- I
didn't really dig in the doc generation process to confirm this.

http://codereview.chromium.org/10391034/diff/10003/chrome/browser/extensions/...
File chrome/browser/extensions/api/discovery/suggested_link.h (right):

http://codereview.chromium.org/10391034/diff/10003/chrome/browser/extensions/...
chrome/browser/extensions/api/discovery/suggested_link.h:31: double score_;
On 2012/05/17 00:05:29, Aaron Boodman wrote:
> You should add a comment for this field since its meaning is non-obvious.

Done.

[commit-bot: I haz the power, 2012-05-17 19:11:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/10391034/10004

[commit-bot: I haz the power, 2012-05-17 21:13:01]

Change committed as 137735