Implementation of the "Redirect URLs to Packaged Apps" feature.

Implementation of the "Redirect URLs to Packaged Apps" feature.

Design doc: https://docs.google.com/a/google.com/document/d/1JDRqz7CkHbSKwEOKqera0H9gLrtWI0bQggvfTVG3YZw/edit?usp=sharing

BUG=111422

[asargent_no_longer_on_chrome, 2013-08-14 18:05:12]

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.h (right):

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/app_runtime/app_runtime_api.h:64: // custom
dispatch event in javascript.
This comment seems unrelated to the implementation in the .cc file - copy/paste
error?

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/url_handlers/url_handlers_api.h (right):

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/url_handlers/url_handlers_api.h:32: }  //
namespace extensions
Is this class actually used for anything in the CL yet? I don't see any actual
functionality here. =)

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:2530:
CHECK(app->is_platform_app());
Note that a renderer process could be compromised, so instead of a CHECK here
that crashes the entire browser, it might be more appropriate to kill the
renderer that sent us this IPC message instead. 

(Clearly there are lots more denial-of-service attacks possible from a
compromised renderer, but it's nice not to add another one).

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.h:142: public IPC::Listener {
Since ExtensionService already does a lot of stuff, it's probably better to put
the IPC messaging and associated code in a separate class that just has a
pointer to the owning Profile.

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/pla...
File chrome/browser/extensions/platform_app_launcher.h (right):

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/pla...
chrome/browser/extensions/platform_app_launcher.h:57: const std::string&
handler_id,
It would be helpful to have a comment above about what |handler_id| is, since it
isn't obvious like the rest of the parameters.

https://codereview.chromium.org/22944002/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/22944002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_manifest_features.json:311: "extension_types":
["packaged_app", "platform_app"]
nit: "packaged_app" is the legacy type (not v2 apps) which we are not adding
additional capabilities to. This should just be "platform_app"

https://codereview.chromium.org/22944002/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/app_runtime.idl (right):

https://codereview.chromium.org/22944002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/app_runtime.idl:27: DOMString? referrerUrl;
We might consider wrapping these 2 together into a separate dictionary, e.g.

[inline_doc] dictionary RedirectedUrlData {
  DOMString url;
  DOMString? referrerUrl;  // Null for omnibox navigations, bookmark clicks,
etc.
};

[inline_doc] dictionary LaunchData {
  ...
  RedirectedUrlData? redirect;
  ...
};


That gives a clean separation if we think we might someday want to provide some
additional information, e.g. an enum of "navigation type", etc.

https://codereview.chromium.org/22944002/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension_set.cc (right):

https://codereview.chromium.org/22944002/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension_set.cc:131: for (const auto& ext:
extensions_) {
FYI, we can't use C++11 features, because some platforms we need to compile on
don't support it yet.

[sergeygs, 2013-08-18 11:40:24]

I think I've got it right this time. All kinds of navigations work now (browser-
and WebKit-initiated), and launch the handling app with a correct launchData.

I assigned reviewers based on mixed input from miket@, OWNERS info, prior
conversations, and 'git cl upload' recommendations. Please review your
respective areas and feel free to skip reviewing if assigned inappropriately
(but let me know).

The design doc is here:
https://docs.google.com/a/google.com/document/d/1JDRqz7CkHbSKwEOKqera0H9gLrtW...
Everyone in apps-dev@chromium.org has access.

There are 4 major parts to this concrete implementation:
1) New manifest key 'url_handler', parsed both in the browser and in
extensions::Dispatcher on the renderer side.
2) Mechanism to launch an app with a new type of the onLaunched event,
corresponding to a launch with a URL to be handled (in addition to plain launch
and launch with a file(s) to be handled).
3) Interception and redirection of browser-level navigations in
chrome::Navigate().
4) Interception and redirection of WebKit-initiated in-page navigations in
ChromeContentRenderClient::HandleNavigation().

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.h (right):

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/app_runtime/app_runtime_api.h:64: // custom
dispatch event in javascript.
Sort of. I was part-cloning the similar thing for file_handlers, before I knew
more about what's going on in JavaScript. Now I know that this comment is indeed
unrelated here.
On 2013/08/14 18:05:12, Antony Sargent wrote:
> This comment seems unrelated to the implementation in the .cc file -
copy/paste
> error?

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/url_handlers/url_handlers_api.h (right):

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/url_handlers/url_handlers_api.h:32: }  //
namespace extensions
Yes, it is used. ProfileDependencyManager::AssertFactoriesBuilt() invokes
extensions::UrlHandlersAPI::GetFactoryInstance() (as for all other similar
factories) in order to initialize it and register the 'url_handlers' manifest
field parser.
On 2013/08/14 18:05:12, Antony Sargent wrote:
> Is this class actually used for anything in the CL yet? I don't see any actual
> functionality here. =)

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.cc:2530:
CHECK(app->is_platform_app());
After more careful thinking, this should have been a DCHECK from the start,
since it test a design invariant. Changed in the new patch.
On 2013/08/14 18:05:12, Antony Sargent wrote:
> Note that a renderer process could be compromised, so instead of a CHECK here
> that crashes the entire browser, it might be more appropriate to kill the
> renderer that sent us this IPC message instead. 
> 
> (Clearly there are lots more denial-of-service attacks possible from a
> compromised renderer, but it's nice not to add another one).

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_service.h:142: public IPC::Listener {
It looks to me now that this attempt wasn't viable anyway. I'm working on an
alternative (will be in the next patch). 
On 2013/08/14 18:05:12, Antony Sargent wrote:
> Since ExtensionService already does a lot of stuff, it's probably better to
put
> the IPC messaging and associated code in a separate class that just has a
> pointer to the owning Profile.

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/pla...
File chrome/browser/extensions/platform_app_launcher.h (right):

https://codereview.chromium.org/22944002/diff/1/chrome/browser/extensions/pla...
chrome/browser/extensions/platform_app_launcher.h:57: const std::string&
handler_id,
On 2013/08/14 18:05:12, Antony Sargent wrote:
> It would be helpful to have a comment above about what |handler_id| is, since
it
> isn't obvious like the rest of the parameters.

Done.

https://codereview.chromium.org/22944002/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/22944002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/_manifest_features.json:311: "extension_types":
["packaged_app", "platform_app"]
On 2013/08/14 18:05:12, Antony Sargent wrote:
> nit: "packaged_app" is the legacy type (not v2 apps) which we are not adding
> additional capabilities to. This should just be "platform_app"
> 

Done.

https://codereview.chromium.org/22944002/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/app_runtime.idl (right):

https://codereview.chromium.org/22944002/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/app_runtime.idl:27: DOMString? referrerUrl;
This would be visible on the external API side, in the signature of onLaunched,
and no objection/suggestion has ever come up in the API reviews about the
proposed signature, so I'm not sure I'm at liberty to do that change.
On 2013/08/14 18:05:12, Antony Sargent wrote:
> We might consider wrapping these 2 together into a separate dictionary, e.g.
> 
> [inline_doc] dictionary RedirectedUrlData {
>   DOMString url;
>   DOMString? referrerUrl;  // Null for omnibox navigations, bookmark clicks,
> etc.
> };
> 
> [inline_doc] dictionary LaunchData {
>   ...
>   RedirectedUrlData? redirect;
>   ...
> };
> 
> 
> That gives a clean separation if we think we might someday want to provide
some
> additional information, e.g. an enum of "navigation type", etc.

https://codereview.chromium.org/22944002/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension_set.cc (right):

https://codereview.chromium.org/22944002/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension_set.cc:131: for (const auto& ext:
extensions_) {
Makes sense. I wasn't sure myself, and consulted the coding style guides.
Google-wide one explicitly allows using auto and range-for, and Chromium one
doesn't seem to forbid it anywhere. Style guide deficiency? 

I'll change the code, of course.

On 2013/08/14 18:05:12, Antony Sargent wrote:
> FYI, we can't use C++11 features, because some platforms we need to compile on
> don't support it yet.

[not at google - send to devlin, 2013-08-19 23:32:24]

Brief-ish look at everything under chrome/. Where is the part that restricts it
to apps that can prove ownership?

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:118:
DispatchOnLaunchedEventImpl(extension->id(), args.Pass(), profile);
Cleanup: we generate the event value boilerplate, it's just not being used here.
This method could look something like:

app_runtime::LaunchData launch_data;
launch_data.id.reset(new int(handler_id));
launch_data.url.reset(new std::string(url.spec());
launch_data.referrer_url(new std::string(referrer_url.spec()));
DispatchOnLaunchedEventImpl(extension->id(), launch_data.ToValue(), profile);

It may be nice to update the other events here with the same, or add a TODO.

See:
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/obj/gen/...
for the existing generated code. Yours will add more there obviously.

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/api/url_handlers/url_handlers_api.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/url_handlers/url_handlers_api.cc:4: 
These should be in chrome/common/extensions/manifest_handlers/...

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2515: }
This doesn't need to be in ExtensionService. Pull into a different class.

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/web_accessible_resources_parser.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/web_accessible_resources_parser.cc:3: // found in the
LICENSE file.
please refrain from superflous changes to unrelated files (there have been a few
already), it muddies the change log.

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:462: // TODO(sergeygs): Shouldn't we
return from here if this returns true?
no, it looks like this method modifies the URL to point to an error page.

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:467: }
when you pull the MaybeRedirecturlToApp logic out of ExtensionService, you could
make it an abstraction for platform app redirection. this could be like:

// would be nice to add an AppSystem and use that rather than ExtensionSystem.
extensions::PlatformAppRedirector* platform_app_redirector =
    extensions::ExtensionSystem::Get(params->initiating_profile)->
        platform_app_redirector();

if (platform_app_redirector->HandleUrl(params->url,
                                       params->referrer.url,
                                       &params->url))
  return;

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/app_runtime.idl (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/app_runtime.idl:27: DOMString? referrerUrl;
The docs are going to be generated by this; numbering the "options" and grouping
them with whitespace isn't going to look right.

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:6: 
consider using the same strategy that externally_connectable does for parsing
the manifest - declare a schema for the manifest entry in manifest_types.json,
then use it here.

See:
chrome/common/extensions/api/manifest_types.json
chrome/common/extensions/manifest_handlers/externally_connectable.*

It will remove the base::*Value-based parsing - much nicer - and soon will allow
easily generating documentation for that manifest property (see
https://codereview.chromium.org/22854025/).

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:97: //
TODO(sergeygs): Should be is_platform_app?
yes

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.h (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.h:29: const
Extension* app;
this shouldn't be necessary. it may be convenient but it's unsafe and overall
redundant.

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/extension_set.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/extension_set.cc:130:
ExtensionSet::GetHandlingAppForURL(const GURL& url) const {
the name of this method and the thing it returns isn't consistent.

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/extension_set.cc:132: extIt != extensions_.end();
++extIt) {
s/extIt/it/

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/extension_set.cc:133: if (extIt->second->is_app()) {
I don't really see the point in checking is_app here, since you're validating at
parse time that only packaged apps can have the manifest property in the first
place.

You can DCHECK(extension->is_platform_app()) if you really want (upon finding
the extension that handles it, if any). Makes sense.

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/extension_set.cc:137: for
(std::vector<extensions::UrlHandlerInfo>::const_iterator handlerIt =
add a method to UrlHandlers that does this?

https://codereview.chromium.org/22944002/diff/12001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:914: const ExtensionSet* exts
= extension_dispatcher_->extensions();
inline?

https://codereview.chromium.org/22944002/diff/12001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:923: }
else clause is unnecessary

https://codereview.chromium.org/22944002/diff/12001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:928: default:
leave out the default cases so that the compiler verifies that you've covered
them all.

https://codereview.chromium.org/22944002/diff/12001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
(right):

https://codereview.chromium.org/22944002/diff/12001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:91:
dispatch([launchData]);
Enumerating the various types of launches doesn't really seem necessary, more
confusing. Even as it stands the method is too complicated.

if (!launchData) {
  dispatch([]);
  return;
}

if (launchData.items) {
  logging.CHECK('id' in launchData);
  ...
}

dispatch([launchData]);

[Charlie Reis, 2013-08-26 22:17:51]

Sorry for the late reply.  It might help to more specifically direct reviewers
to which parts you want them to review.  I focused on content/, guessing that
you listed me as an content/ owner.

So far, the main issue is that we won't be able to add extension or app concepts
to the content/ module, since those are specific to chrome/.  Perhaps there's a
way to move those out of content/?

https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
File content/browser/renderer_host/extension_system_message_filter.cc (right):

https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
content/browser/renderer_host/extension_system_message_filter.cc:31:
IPC_MESSAGE_HANDLER(ExtensionSystemMsg_LaunchAppWithUrl,
Same with apps.  Is there a way to move this to chrome/?

https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
File content/browser/renderer_host/extension_system_message_filter.h (right):

https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
content/browser/renderer_host/extension_system_message_filter.h:20: class
ExtensionSystemMessageFilter : public BrowserMessageFilter {
Extensions are a chrome/ concept and don't belong in content/.

https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:547: //
ResourceSchedulerFilter
My sense is that these comments aren't really adding much beyond the code
itself, since they're just repeating what each line says.  I could see a use for
them if there were larger chunks of code being categorized, but almost all of
these are just one line or one block.  I'd suggest leaving them out.

https://codereview.chromium.org/22944002/diff/12001/content/public/browser/ex...
File content/public/browser/extension_system_notifications.h (right):

https://codereview.chromium.org/22944002/diff/12001/content/public/browser/ex...
content/public/browser/extension_system_notifications.h:15: struct
CONTENT_EXPORT LaunchAppWithUrlDetails {
Like the extension stuff, we can't add this in content/.

https://codereview.chromium.org/22944002/diff/12001/content/public/browser/na...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/22944002/diff/12001/content/public/browser/na...
content/public/browser/navigation_controller.h:400: //  virtual void
RegisterURLHandler(Delegate* url_handler) OVERRIDE;
This is commented out, so please remove.

https://codereview.chromium.org/22944002/diff/12001/content/public/browser/no...
File content/public/browser/notification_types.h (right):

https://codereview.chromium.org/22944002/diff/12001/content/public/browser/no...
content/public/browser/notification_types.h:241:
NOTIFICATION_LAUNCH_APP_WITH_URL,
We can't add this to content/.

https://codereview.chromium.org/22944002/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/22944002/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2867: // Webkit is asking whether to
navigate to a new URL.
Is there a functional change here, or is this mainly reformatting?

[benwells, 2013-08-27 01:17:13]

On 2013/08/26 22:17:51, creis wrote:
> Sorry for the late reply.  It might help to more specifically direct reviewers
> to which parts you want them to review.  I focused on content/, guessing that
> you listed me as an content/ owner.
> 
> So far, the main issue is that we won't be able to add extension or app
concepts
> to the content/ module, since those are specific to chrome/.  Perhaps there's
a
> way to move those out of content/?
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
> File content/browser/renderer_host/extension_system_message_filter.cc (right):
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
> content/browser/renderer_host/extension_system_message_filter.cc:31:
> IPC_MESSAGE_HANDLER(ExtensionSystemMsg_LaunchAppWithUrl,
> Same with apps.  Is there a way to move this to chrome/?
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
> File content/browser/renderer_host/extension_system_message_filter.h (right):
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
> content/browser/renderer_host/extension_system_message_filter.h:20: class
> ExtensionSystemMessageFilter : public BrowserMessageFilter {
> Extensions are a chrome/ concept and don't belong in content/.
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
> content/browser/renderer_host/render_process_host_impl.cc:547: //
> ResourceSchedulerFilter
> My sense is that these comments aren't really adding much beyond the code
> itself, since they're just repeating what each line says.  I could see a use
for
> them if there were larger chunks of code being categorized, but almost all of
> these are just one line or one block.  I'd suggest leaving them out.
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/public/browser/ex...
> File content/public/browser/extension_system_notifications.h (right):
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/public/browser/ex...
> content/public/browser/extension_system_notifications.h:15: struct
> CONTENT_EXPORT LaunchAppWithUrlDetails {
> Like the extension stuff, we can't add this in content/.
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/public/browser/na...
> File content/public/browser/navigation_controller.h (right):
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/public/browser/na...
> content/public/browser/navigation_controller.h:400: //  virtual void
> RegisterURLHandler(Delegate* url_handler) OVERRIDE;
> This is commented out, so please remove.
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/public/browser/no...
> File content/public/browser/notification_types.h (right):
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/public/browser/no...
> content/public/browser/notification_types.h:241:
> NOTIFICATION_LAUNCH_APP_WITH_URL,
> We can't add this to content/.
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/renderer/render_v...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/22944002/diff/12001/content/renderer/render_v...
> content/renderer/render_view_impl.cc:2867: // Webkit is asking whether to
> navigate to a new URL.
> Is there a functional change here, or is this mainly reformatting?

sergey: is there anything specific you want me to look at? I think kalman and
asargent cover off most of this stuff from an OWNERS perspective as well as I
do.

[sergeygs, 2013-08-29 07:55:43]

On 2013/08/27 01:17:13, benwells wrote:
> On 2013/08/26 22:17:51, creis wrote:
> > Sorry for the late reply.  It might help to more specifically direct
reviewers
> > to which parts you want them to review.  I focused on content/, guessing
that
> > you listed me as an content/ owner.
> > 
> > So far, the main issue is that we won't be able to add extension or app
> concepts
> > to the content/ module, since those are specific to chrome/.  Perhaps
there's
> a
> > way to move those out of content/?
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
> > File content/browser/renderer_host/extension_system_message_filter.cc
(right):
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
> > content/browser/renderer_host/extension_system_message_filter.cc:31:
> > IPC_MESSAGE_HANDLER(ExtensionSystemMsg_LaunchAppWithUrl,
> > Same with apps.  Is there a way to move this to chrome/?
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
> > File content/browser/renderer_host/extension_system_message_filter.h
(right):
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
> > content/browser/renderer_host/extension_system_message_filter.h:20: class
> > ExtensionSystemMessageFilter : public BrowserMessageFilter {
> > Extensions are a chrome/ concept and don't belong in content/.
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
> > File content/browser/renderer_host/render_process_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
> > content/browser/renderer_host/render_process_host_impl.cc:547: //
> > ResourceSchedulerFilter
> > My sense is that these comments aren't really adding much beyond the code
> > itself, since they're just repeating what each line says.  I could see a use
> for
> > them if there were larger chunks of code being categorized, but almost all
of
> > these are just one line or one block.  I'd suggest leaving them out.
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/public/browser/ex...
> > File content/public/browser/extension_system_notifications.h (right):
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/public/browser/ex...
> > content/public/browser/extension_system_notifications.h:15: struct
> > CONTENT_EXPORT LaunchAppWithUrlDetails {
> > Like the extension stuff, we can't add this in content/.
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/public/browser/na...
> > File content/public/browser/navigation_controller.h (right):
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/public/browser/na...
> > content/public/browser/navigation_controller.h:400: //  virtual void
> > RegisterURLHandler(Delegate* url_handler) OVERRIDE;
> > This is commented out, so please remove.
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/public/browser/no...
> > File content/public/browser/notification_types.h (right):
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/public/browser/no...
> > content/public/browser/notification_types.h:241:
> > NOTIFICATION_LAUNCH_APP_WITH_URL,
> > We can't add this to content/.
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/renderer/render_v...
> > File content/renderer/render_view_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/22944002/diff/12001/content/renderer/render_v...
> > content/renderer/render_view_impl.cc:2867: // Webkit is asking whether to
> > navigate to a new URL.
> > Is there a functional change here, or is this mainly reformatting?
> 
> sergey: is there anything specific you want me to look at? I think kalman and
> asargent cover off most of this stuff from an OWNERS perspective as well as I
> do.

Ben: I added you only because I borrowed quite a few implementation details from
file_handlers implementation, and in some parts the two intersect even. So I
thought your knowing of that code could be useful. The files I'd like you to
look at are:

chrome/browser/extensions/*
chrome/browser/extensions/platform_app_launcher.*
chrome/common/extensions/api/app_runtime.idl
chrome/common/extensions/api/url_handlers/url_handlers_parser.*
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js

Thanks!

[sergeygs, 2013-08-29 08:24:42]

I believe I've addressed all the concerns that were raised. 

I'm also planning to break the CL into 3-4 individual parts, so that it's more
manageable from the code review perspective and can land in smaller chunks. But
for now, since we've already started some discussion here, I wanted to close
those items first. Therefore, I'd like to ask kalman@ and creis@ to go over
their earlier comments (thanks!) and make sure they have been addressed
properly.

Matt: I've added you to review the IPC-related changes (git cl suggested you for
ipc_message_start.h). Could you please take a quick look at:
ipc_message_start.h
url_handling_message.h
url_handling_message_filter.*
chrome_content_renderer_client.cc

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:118:
DispatchOnLaunchedEventImpl(extension->id(), args.Pass(), profile);
Done. Added TODOs for the rest, will fix after this CL.
On 2013/08/19 23:32:24, kalman wrote:
> Cleanup: we generate the event value boilerplate, it's just not being used
here.
> This method could look something like:
> 
> app_runtime::LaunchData launch_data;
> launch_data.id.reset(new int(handler_id));
> launch_data.url.reset(new std::string(url.spec());
> launch_data.referrer_url(new std::string(referrer_url.spec()));
> DispatchOnLaunchedEventImpl(extension->id(), launch_data.ToValue(), profile);
> 
> It may be nice to update the other events here with the same, or add a TODO.
> 
> See:
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/obj/gen/...
> for the existing generated code. Yours will add more there obviously.

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/api/url_handlers/url_handlers_api.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/url_handlers/url_handlers_api.cc:4: 
Did you mean:
a) chrome/common/extensions/api/url_handlers/, or
b) chrome/common/extensions/manifest_url_handler.*

? 

I suspect the latter, but in any case, could you explain why these should be
there? The new manifest key is a part of an API, so
chrome/browser/extensions/api seems like a logical place for its parser. FWIW, I
was mimicking the implementation of the file_handlers API, and the respective
files from there are under in the same places I put mine. Also,
manifest_url_handler.* deals almost exclusively with extension-specific keys:
the only exception is 'update_url', which is still shared between both
extensions and apps. The new url_handlers key will be purely app-specific.

Does that sound convincing enough? If not, I propose to leave this as is for
now, and at a later time refactor both url_handlers and file_handlers as you've
suggested, so they still look similar.

On 2013/08/19 23:32:24, kalman wrote:
> These should be in chrome/common/extensions/manifest_handlers/...

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2515: }
On 2013/08/19 23:32:24, kalman wrote:
> This doesn't need to be in ExtensionService. Pull into a different class.

Done. The class is PlatformAppRedirector in chrome/browser/extensions.

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/web_accessible_resources_parser.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/web_accessible_resources_parser.cc:3: // found in the
LICENSE file.
On 2013/08/19 23:32:24, kalman wrote:
> please refrain from superflous changes to unrelated files (there have been a
few
> already), it muddies the change log.

Done.

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:462: // TODO(sergeygs): Shouldn't we
return from here if this returns true?
On 2013/08/19 23:32:24, kalman wrote:
> no, it looks like this method modifies the URL to point to an error page.

Done.

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:467: }
On 2013/08/19 23:32:24, kalman wrote:
> when you pull the MaybeRedirecturlToApp logic out of ExtensionService, you
could
> make it an abstraction for platform app redirection. this could be like:
> 
> // would be nice to add an AppSystem and use that rather than ExtensionSystem.
> extensions::PlatformAppRedirector* platform_app_redirector =
>     extensions::ExtensionSystem::Get(params->initiating_profile)->
>         platform_app_redirector();
> 
> if (platform_app_redirector->HandleUrl(params->url,
>                                        params->referrer.url,
>                                        &params->url))
>   return;

Done. Would it make sense to also obtain the ExtensionService as
system->extension_service() now, for symmetry and minimizing local variables?

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/app_runtime.idl (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/app_runtime.idl:27: DOMString? referrerUrl;
On 2013/08/19 23:32:24, kalman wrote:
> The docs are going to be generated by this; numbering the "options" and
grouping
> them with whitespace isn't going to look right.

Done.

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:6: 
This is identical to how file_handlers code does that, which I was mimicking.
Since this doesn't affect functionality, and has many equivalents around the
code currently, could we leave it like this for the time being? I'll add a TODO
here to switch to a manifest entry schema ASAP after this lands.

On 2013/08/19 23:32:24, kalman wrote:
> consider using the same strategy that externally_connectable does for parsing
> the manifest - declare a schema for the manifest entry in manifest_types.json,
> then use it here.
> 
> See:
> chrome/common/extensions/api/manifest_types.json
> chrome/common/extensions/manifest_handlers/externally_connectable.*
> 
> It will remove the base::*Value-based parsing - much nicer - and soon will
allow
> easily generating documentation for that manifest property (see
> https://codereview.chromium.org/22854025/).

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:97: //
TODO(sergeygs): Should be is_platform_app?
On 2013/08/19 23:32:24, kalman wrote:
> yes

Done.

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.h (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.h:29: const
Extension* app;
Removing this makes the usage sites a little uglier, as they have to iterate
over extension sets themselves, but I agree that this is a little unsafe, so 
removing.

On 2013/08/19 23:32:24, kalman wrote:
> this shouldn't be necessary. it may be convenient but it's unsafe and overall
> redundant.

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/extension_set.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/extension_set.cc:130:
ExtensionSet::GetHandlingAppForURL(const GURL& url) const {
On 2013/08/19 23:32:24, kalman wrote:
> the name of this method and the thing it returns isn't consistent.

Done. This whole function has disappeared, replaced by a combination of new
static methods UrlHandlers and straightforward iteration over extensions at the
usage sites.

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/extension_set.cc:132: extIt != extensions_.end();
++extIt) {
On 2013/08/19 23:32:24, kalman wrote:
> s/extIt/it/

Done.

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/extension_set.cc:133: if (extIt->second->is_app()) {
On 2013/08/19 23:32:24, kalman wrote:
> I don't really see the point in checking is_app here, since you're validating
at
> parse time that only packaged apps can have the manifest property in the first
> place.
> 
> You can DCHECK(extension->is_platform_app()) if you really want (upon finding
> the extension that handles it, if any). Makes sense.

Done.

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/extension_set.cc:137: for
(std::vector<extensions::UrlHandlerInfo>::const_iterator handlerIt =
On 2013/08/19 23:32:24, kalman wrote:
> add a method to UrlHandlers that does this?

Done.

https://codereview.chromium.org/22944002/diff/12001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:914: const ExtensionSet* exts
= extension_dispatcher_->extensions();
Why not :)

On 2013/08/19 23:32:24, kalman wrote:
> inline?

https://codereview.chromium.org/22944002/diff/12001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:923: }
Done.

On 2013/08/19 23:32:24, kalman wrote:
> else clause is unnecessary

https://codereview.chromium.org/22944002/diff/12001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:928: default:
Done.

On 2013/08/19 23:32:24, kalman wrote:
> leave out the default cases so that the compiler verifies that you've covered
> them all.

https://codereview.chromium.org/22944002/diff/12001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
(right):

https://codereview.chromium.org/22944002/diff/12001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:91:
dispatch([launchData]);
On 2013/08/19 23:32:24, kalman wrote:
> Enumerating the various types of launches doesn't really seem necessary, more
> confusing. Even as it stands the method is too complicated.
> 
> if (!launchData) {
>   dispatch([]);
>   return;
> }
> 
> if (launchData.items) {
>   logging.CHECK('id' in launchData);
>   ...
> }
> 
> dispatch([launchData]);

Done.

https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
File content/browser/renderer_host/extension_system_message_filter.cc (right):

https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
content/browser/renderer_host/extension_system_message_filter.cc:31:
IPC_MESSAGE_HANDLER(ExtensionSystemMsg_LaunchAppWithUrl,
Changed the naming to reflect the actual nature of what's happening here.
On 2013/08/26 22:17:52, creis wrote:
> Same with apps.  Is there a way to move this to chrome/?

https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
File content/browser/renderer_host/extension_system_message_filter.h (right):

https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
content/browser/renderer_host/extension_system_message_filter.h:20: class
ExtensionSystemMessageFilter : public BrowserMessageFilter {
I think it's purely about naming: I've just been overly specific about what's
going on here on the renderer's and renderer host's side. What's actually
happening is:
- The renderer essentially asks ChromeContentRendererClient, which plays a role
of the browser's representative on the renderer's side, whether the browser
could handle the URL. The renderer doesn't need to know what "could handle"
really mean (in reality it means that the browser has a registered app that can
handle the URL).
- If ChromeContentRendererClient thinks so, it tells the renderer back to stop
navigating to the URL, and sends an IPC message to the renderer host which
essentially tells the host to forward the URL to the browser for handling.
- The renderer does so by sending a notification through the
NotificationService.
- The browser receives the notification and only then this gets again
reinterpreted as a command to launch an app.
So on the renderer side it's all about URL forwarding to the browser. I've
changed the naming of classes and files accordingly, so I hope it looks good
now.

On 2013/08/26 22:17:52, creis wrote:
> Extensions are a chrome/ concept and don't belong in content/.

https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/22944002/diff/12001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:547: //
ResourceSchedulerFilter
I'd say that it was rather hard to quickly enumerate filters being added just by
looking at the original code, which wasn't even whitespace-separated to aid
that, but I've removed the comments per your suggestion.

On 2013/08/26 22:17:52, creis wrote:
> My sense is that these comments aren't really adding much beyond the code
> itself, since they're just repeating what each line says.  I could see a use
for
> them if there were larger chunks of code being categorized, but almost all of
> these are just one line or one block.  I'd suggest leaving them out.

https://codereview.chromium.org/22944002/diff/12001/content/public/browser/ex...
File content/public/browser/extension_system_notifications.h (right):

https://codereview.chromium.org/22944002/diff/12001/content/public/browser/ex...
content/public/browser/extension_system_notifications.h:15: struct
CONTENT_EXPORT LaunchAppWithUrlDetails {
Changed the naming to reflect the general nature of what's happening here.
On 2013/08/26 22:17:52, creis wrote:
> Like the extension stuff, we can't add this in content/.

https://codereview.chromium.org/22944002/diff/12001/content/public/browser/na...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/22944002/diff/12001/content/public/browser/na...
content/public/browser/navigation_controller.h:400: //  virtual void
RegisterURLHandler(Delegate* url_handler) OVERRIDE;
Accidentally slipped in. Removed.
On 2013/08/26 22:17:52, creis wrote:
> This is commented out, so please remove.

https://codereview.chromium.org/22944002/diff/12001/content/public/browser/no...
File content/public/browser/notification_types.h (right):

https://codereview.chromium.org/22944002/diff/12001/content/public/browser/no...
content/public/browser/notification_types.h:241:
NOTIFICATION_LAUNCH_APP_WITH_URL,
Changed the naming to reflect the general nature of what's happening here.

On 2013/08/26 22:17:52, creis wrote:
> We can't add this to content/.

https://codereview.chromium.org/22944002/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/22944002/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:2867: // Webkit is asking whether to
navigate to a new URL.
Correct, this is mainly reformatting. I'll revert this in order not to muddle
the CL. Will commit separately.
On 2013/08/26 22:17:52, creis wrote:
> Is there a functional change here, or is this mainly reformatting?

[sergeygs, 2013-08-29 08:34:08]

On 2013/08/19 23:32:24, kalman wrote:
> Brief-ish look at everything under chrome/. Where is the part that restricts
it
> to apps that can prove ownership?
> 

I haven't answered this yet. The restricting part will be first and foremost the
store. They will add domain ownership checking based on the url_handlers in the
manifest, independently of this implementation. There is already a Buganizer bug
tracking that: https://b.corp.google.com/issue?id=10343468.

As an extra precaution, I'm planning to add some sanity checks for that in my
code too (probably as a follow-up to this CL).

[not at google - send to devlin, 2013-08-29 16:55:43]

On 2013/08/29 08:34:08, sergeygs wrote:
> On 2013/08/19 23:32:24, kalman wrote:
> > Brief-ish look at everything under chrome/. Where is the part that restricts
> it
> > to apps that can prove ownership?
> > 
> 
> I haven't answered this yet. The restricting part will be first and foremost
the
> store. They will add domain ownership checking based on the url_handlers in
the
> manifest, independently of this implementation. There is already a Buganizer
bug
> tracking that: https://b.corp.google.com/issue?id=10343468.
> 
> As an extra precaution, I'm planning to add some sanity checks for that in my
> code too (probably as a follow-up to this CL).

Quick response to this: Antony pointed out that we should definitely restrict
this feature to apps downloaded from the webstore in that case.

I also responded on the bug. It'd be nice to do this validation at the Chrome
level as well, just to be extra safe (and it would be interesting to dig through
the hosted app handling to see what that does) but so long as the webstore is
doing things right, that can be in a follow-up.

[sergeygs1, 2013-08-29 17:15:29]

Sounds good.

By "restrict to apps downloaded from the webstore", do you mean "as opposed
to installing local unpackaged apps in developer mode"?
On Aug 29, 2013 9:55 AM, <kalman@chromium.org> wrote:

> On 2013/08/29 08:34:08, sergeygs wrote:
>
>> On 2013/08/19 23:32:24, kalman wrote:
>> > Brief-ish look at everything under chrome/. Where is the part that
>> restricts
>> it
>> > to apps that can prove ownership?
>> >
>>
>
>  I haven't answered this yet. The restricting part will be first and
>> foremost
>>
> the
>
>> store. They will add domain ownership checking based on the url_handlers
>> in
>>
> the
>
>> manifest, independently of this implementation. There is already a
>> Buganizer
>>
> bug
>
>> tracking that:
https://b.corp.google.com/**issue?id=10343468<https://b.corp.google.com/issue...
>> .
>>
>
>  As an extra precaution, I'm planning to add some sanity checks for that
>> in my
>> code too (probably as a follow-up to this CL).
>>
>
> Quick response to this: Antony pointed out that we should definitely
> restrict
> this feature to apps downloaded from the webstore in that case.
>
> I also responded on the bug. It'd be nice to do this validation at the
> Chrome
> level as well, just to be extra safe (and it would be interesting to dig
> through
> the hosted app handling to see what that does) but so long as the webstore
> is
> doing things right, that can be in a follow-up.
>
>
https://codereview.chromium.**org/22944002/<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.

[not at google - send to devlin, 2013-08-29 17:35:56]

You need to add tests for all those classes you're adding.

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/api/url_handlers/url_handlers_api.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/url_handlers/url_handlers_api.cc:4: 
On 2013/08/29 08:24:42, sergeygs wrote:
> Did you mean:
> a) chrome/common/extensions/api/url_handlers/, or
> b) chrome/common/extensions/manifest_url_handler.*
> 
> ? 
> 
> I suspect the latter, but in any case, could you explain why these should be
> there? The new manifest key is a part of an API, so
> chrome/browser/extensions/api seems like a logical place for its parser. FWIW,
I
> was mimicking the implementation of the file_handlers API, and the respective
> files from there are under in the same places I put mine. Also,
> manifest_url_handler.* deals almost exclusively with extension-specific keys:
> the only exception is 'update_url', which is still shared between both
> extensions and apps. The new url_handlers key will be purely app-specific.
> 
> Does that sound convincing enough? If not, I propose to leave this as is for
> now, and at a later time refactor both url_handlers and file_handlers as
you've
> suggested, so they still look similar.
> 
> On 2013/08/19 23:32:24, kalman wrote:
> > These should be in chrome/common/extensions/manifest_handlers/...
> 

URL handles don't really have an API associated with them, do they? Like there
is no chrome.urlHandler.someMethod(). The code in api/ should have been
restricted to such implementations (i.e. things that appear in
_api_features.json) though we haven't stuck with this precedent, as you've
noticed with file handlers.

Up to you though. i would like for both this and file handlers to end up in
chrome/common/extensions/manifest_handlers/{file,url}_handler... and if you want
to move it later, along with file handlers... sure. It's just more work and a
more daunting diff later. IMO it would be easier to move now.

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:467: }
On 2013/08/29 08:24:42, sergeygs wrote:
> On 2013/08/19 23:32:24, kalman wrote:
> > when you pull the MaybeRedirecturlToApp logic out of ExtensionService, you
> could
> > make it an abstraction for platform app redirection. this could be like:
> > 
> > // would be nice to add an AppSystem and use that rather than
ExtensionSystem.
> > extensions::PlatformAppRedirector* platform_app_redirector =
> >     extensions::ExtensionSystem::Get(params->initiating_profile)->
> >         platform_app_redirector();
> > 
> > if (platform_app_redirector->HandleUrl(params->url,
> >                                        params->referrer.url,
> >                                        &params->url))
> >   return;
> 
> Done. Would it make sense to also obtain the ExtensionService as
> system->extension_service() now, for symmetry and minimizing local variables?

yeah that would be nice. You could restructure this part nicely:

ExtenisonSystem* system = ...;
if (system && !system->extension_service()->ShouldBlockUrlInBrowserTab()) {
  // ...
}

writing that snippet made me wonder whether we should only check the platform
app redirector if the extension service didn't rewrite the URL. It doesn't make
any sense for the platform app redirector to be handling error URLs.

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:6: 
On 2013/08/29 08:24:42, sergeygs wrote:
> This is identical to how file_handlers code does that, which I was mimicking.
> Since this doesn't affect functionality, and has many equivalents around the
> code currently, could we leave it like this for the time being? I'll add a
TODO
> here to switch to a manifest entry schema ASAP after this lands.
> 
> On 2013/08/19 23:32:24, kalman wrote:
> > consider using the same strategy that externally_connectable does for
parsing
> > the manifest - declare a schema for the manifest entry in
manifest_types.json,
> > then use it here.
> > 
> > See:
> > chrome/common/extensions/api/manifest_types.json
> > chrome/common/extensions/manifest_handlers/externally_connectable.*
> > 
> > It will remove the base::*Value-based parsing - much nicer - and soon will
> allow
> > easily generating documentation for that manifest property (see
> > https://codereview.chromium.org/22854025/).
> 

Sure, thanks. Make sure to file bugs on all of these things (the file move, the
manifest schema...).

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:113: // TODO: Use
the same way of creating an event (using the generated
TODO(sergeygs)

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:115:
extensions::api::app_runtime::LaunchData launch_data;
this file is already in extensions::, don't need here.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/extension_system.h (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/extension_system.h:192: virtual PlatformAppRedirector*
platform_app_redirector() OVERRIDE;
// shared

I'm pretty sure this doesn't need to be different in incognito mode? Indeed,
that's how you've implemented it.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_launcher.cc:430: profile, extension,
handler_id, url, referrer_url);
bleh, this file should hang of ExtensionSystem rather than being a whole bunch
of static methods. Whatever.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_redirector.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.cc:30:
content::NotificationService::AllBrowserContextsAndSources());
All profiles? really? If you register this only for |profile| then you don't
need that check below.

(but keep incognito mode in mind)

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.cc:54: case
content::NOTIFICATION_HANDLE_URL_IN_BROWSER: {
It would be really nice if you could not use a notification for this but instead
an Observer interface of some sort.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.cc:74: NOTREACHED() <<
"Unexpected notification type.";
just DCHECK(type == content::NOTIFICATION_HANDLE_URL_IN_BROWSER) at the start
and not worry about the switch at all.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_redirector.h (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:32: class
PlatformAppRedirector : public content::NotificationObserver {
I think this class needs a virtual destructor? Though I would expect clang
compiler to complain about that if so.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:34:
PlatformAppRedirector(Profile* profile);
explicit

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:43: virtual void Observe(int
type,
private

https://codereview.chromium.org/22944002/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/api/app_runtime.idl (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/common/extensions/...
chrome/common/extensions/api/app_runtime.idl:19: // 'url_handlers' dictionaries
in the manifest.
for all instance of 'foo' change to <code>foo</code>.

even better: link to where they are documented with <code><a
href="file_handlers.html">file_handlers</a></code> etc. you will need to add
docs for url handlers eventually, so remember to link to those.

https://codereview.chromium.org/22944002/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:63: if
(handlers) {
nit: early return in the exceptional case:

if (!handlers)
  return NULL;

https://codereview.chromium.org/22944002/diff/47001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:65:
handlers->begin(); it != handlers->end(); it++) {
indent--

https://codereview.chromium.org/22944002/diff/47001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:922: new
RendererHostMsg_ForwardUrlToBrowser(url, referrer_url));
I wonder whether there are any issues here with the browser trusting the
renderer.

https://codereview.chromium.org/22944002/diff/47001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
(right):

https://codereview.chromium.org/22944002/diff/47001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:62: // Need
to do some special processin for this case.
processing

but this is kind of vague and so either should be removed or clarified.

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
File content/browser/renderer_host/url_handling_message_filter.cc (right):

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
content/browser/renderer_host/url_handling_message_filter.cc:33:
IPC_MESSAGE_UNHANDLED(handled = false)
without looking into this: would it be possible for PlatformAppRedirector to
listen to these IPCs rather than having them in content?

[sergeygs, 2013-08-30 00:39:43]

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/api/url_handlers/url_handlers_api.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/url_handlers/url_handlers_api.cc:4: 
On 2013/08/29 17:35:56, kalman wrote:
> On 2013/08/29 08:24:42, sergeygs wrote:
> > Did you mean:
> > a) chrome/common/extensions/api/url_handlers/, or
> > b) chrome/common/extensions/manifest_url_handler.*
> > 
> > ? 
> > 
> > I suspect the latter, but in any case, could you explain why these should be
> > there? The new manifest key is a part of an API, so
> > chrome/browser/extensions/api seems like a logical place for its parser.
FWIW,
> I
> > was mimicking the implementation of the file_handlers API, and the
respective
> > files from there are under in the same places I put mine. Also,
> > manifest_url_handler.* deals almost exclusively with extension-specific
keys:
> > the only exception is 'update_url', which is still shared between both
> > extensions and apps. The new url_handlers key will be purely app-specific.
> > 
> > Does that sound convincing enough? If not, I propose to leave this as is for
> > now, and at a later time refactor both url_handlers and file_handlers as
> you've
> > suggested, so they still look similar.
> > 
> > On 2013/08/19 23:32:24, kalman wrote:
> > > These should be in chrome/common/extensions/manifest_handlers/...
> > 
> 
> URL handles don't really have an API associated with them, do they? Like there
> is no chrome.urlHandler.someMethod(). The code in api/ should have been
> restricted to such implementations (i.e. things that appear in
> _api_features.json) though we haven't stuck with this precedent, as you've
> noticed with file handlers.
> 
> Up to you though. i would like for both this and file handlers to end up in
> chrome/common/extensions/manifest_handlers/{file,url}_handler... and if you
want
> to move it later, along with file handlers... sure. It's just more work and a
> more daunting diff later. IMO it would be easier to move now.

Not entirely sure how API is defined for this case, but perhaps they do have an
API: they trigger one flavor of chrome.app.runtime.onLaunched to be sent to the
app. Does that count as an API here? Things in
chrome/common/extensions/manifest_url_handler.* are basically just dumb URLs. 

For now I'd value code uniformity higher than prior conventions that have
already been broken. I've filed b/281754 for this: let's discuss offline.

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/ui/browser...
chrome/browser/ui/browser_navigator.cc:467: }
On 2013/08/29 17:35:56, kalman wrote:
> On 2013/08/29 08:24:42, sergeygs wrote:
> > On 2013/08/19 23:32:24, kalman wrote:
> > > when you pull the MaybeRedirecturlToApp logic out of ExtensionService, you
> > could
> > > make it an abstraction for platform app redirection. this could be like:
> > > 
> > > // would be nice to add an AppSystem and use that rather than
> ExtensionSystem.
> > > extensions::PlatformAppRedirector* platform_app_redirector =
> > >     extensions::ExtensionSystem::Get(params->initiating_profile)->
> > >         platform_app_redirector();
> > > 
> > > if (platform_app_redirector->HandleUrl(params->url,
> > >                                        params->referrer.url,
> > >                                        &params->url))
> > >   return;
> > 
> > Done. Would it make sense to also obtain the ExtensionService as
> > system->extension_service() now, for symmetry and minimizing local
variables?
> 
> yeah that would be nice. You could restructure this part nicely:
> 
> ExtenisonSystem* system = ...;
> if (system && !system->extension_service()->ShouldBlockUrlInBrowserTab()) {
>   // ...
> }
> 
> writing that snippet made me wonder whether we should only check the platform
> app redirector if the extension service didn't rewrite the URL. It doesn't
make
> any sense for the platform app redirector to be handling error URLs.

Done. The second part didn't occur to me, but sounds like a reasonable idea.

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:6: 
On 2013/08/29 17:35:56, kalman wrote:
> On 2013/08/29 08:24:42, sergeygs wrote:
> > This is identical to how file_handlers code does that, which I was
mimicking.
> > Since this doesn't affect functionality, and has many equivalents around the
> > code currently, could we leave it like this for the time being? I'll add a
> TODO
> > here to switch to a manifest entry schema ASAP after this lands.
> > 
> > On 2013/08/19 23:32:24, kalman wrote:
> > > consider using the same strategy that externally_connectable does for
> parsing
> > > the manifest - declare a schema for the manifest entry in
> manifest_types.json,
> > > then use it here.
> > > 
> > > See:
> > > chrome/common/extensions/api/manifest_types.json
> > > chrome/common/extensions/manifest_handlers/externally_connectable.*
> > > 
> > > It will remove the base::*Value-based parsing - much nicer - and soon will
> > allow
> > > easily generating documentation for that manifest property (see
> > > https://codereview.chromium.org/22854025/).
> > 
> 
> Sure, thanks. Make sure to file bugs on all of these things (the file move,
the
> manifest schema...).

Sure, I will file bugs.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:113: // TODO: Use
the same way of creating an event (using the generated
On 2013/08/29 17:35:56, kalman wrote:
> TODO(sergeygs)

Done.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:115:
extensions::api::app_runtime::LaunchData launch_data;
On 2013/08/29 17:35:56, kalman wrote:
> this file is already in extensions::, don't need here.

Done.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/extension_system.h (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/extension_system.h:192: virtual PlatformAppRedirector*
platform_app_redirector() OVERRIDE;
On 2013/08/29 17:35:56, kalman wrote:
> // shared
> 
> I'm pretty sure this doesn't need to be different in incognito mode? Indeed,
> that's how you've implemented it.

Done.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_launcher.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_launcher.cc:430: profile, extension,
handler_id, url, referrer_url);
On 2013/08/29 17:35:56, kalman wrote:
> bleh, this file should hang of ExtensionSystem rather than being a whole bunch
> of static methods. Whatever.

Done. I'm not sure it should hang off ExtensionSystem, though I also didn't like
it being a bunch of static methods. Still, it's very specific to file_handlers
(and now url_handlers) -- these file/command line manipulations hardly belong to
ExtensionSystem. But likewise - whatever.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_redirector.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.cc:30:
content::NotificationService::AllBrowserContextsAndSources());
On 2013/08/29 17:35:56, kalman wrote:
> All profiles? really? If you register this only for |profile| then you don't
> need that check below.
> 
> (but keep incognito mode in mind)

Done. Great catch.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.cc:54: case
content::NOTIFICATION_HANDLE_URL_IN_BROWSER: {
On 2013/08/29 17:35:56, kalman wrote:
> It would be really nice if you could not use a notification for this but
instead
> an Observer interface of some sort.

It seemed really hard to connect the two communicating entities together with an
Observer at the time (now less so, but still). I've added a TODO to follow-up on
this. This currently works fine, and the only difficulty is that it's harder to
find who talks to who, and there is that blind type-casting on the receiving
end.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.cc:74: NOTREACHED() <<
"Unexpected notification type.";
On 2013/08/29 17:35:56, kalman wrote:
> just DCHECK(type == content::NOTIFICATION_HANDLE_URL_IN_BROWSER) at the start
> and not worry about the switch at all.

The way it is now is a common pattern around the code, and it's more easily
extendable (if some other notification is to be handled here in future).

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_redirector.h (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:32: class
PlatformAppRedirector : public content::NotificationObserver {
On 2013/08/29 17:35:56, kalman wrote:
> I think this class needs a virtual destructor? Though I would expect clang
> compiler to complain about that if so.

Done. Clang indeed didn't complain, but a virtual destructor shouldn't hurt here
:)

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:34:
PlatformAppRedirector(Profile* profile);
On 2013/08/29 17:35:56, kalman wrote:
> explicit

Done.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:43: virtual void Observe(int
type,
On 2013/08/29 17:35:56, kalman wrote:
> private

Done.

https://codereview.chromium.org/22944002/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/api/app_runtime.idl (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/common/extensions/...
chrome/common/extensions/api/app_runtime.idl:19: // 'url_handlers' dictionaries
in the manifest.
On 2013/08/29 17:35:56, kalman wrote:
> for all instance of 'foo' change to <code>foo</code>.
> 
> even better: link to where they are documented with <code><a
> href="file_handlers.html">file_handlers</a></code> etc. you will need to add
> docs for url handlers eventually, so remember to link to those.

Done (the first option). I think docs for url_handlers will not have their own
separate html, just like most manifest keys, including file_handlers. Most of
them just have a section in manifest.html. I'll see if I can add references to
there.

https://codereview.chromium.org/22944002/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/api/url_handlers/url_handlers_parser.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:63: if
(handlers) {
On 2013/08/29 17:35:56, kalman wrote:
> nit: early return in the exceptional case:
> 
> if (!handlers)
>   return NULL;

Done.

https://codereview.chromium.org/22944002/diff/47001/chrome/common/extensions/...
chrome/common/extensions/api/url_handlers/url_handlers_parser.cc:65:
handlers->begin(); it != handlers->end(); it++) {
On 2013/08/29 17:35:56, kalman wrote:
> indent--

Done.

https://codereview.chromium.org/22944002/diff/47001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/app_runtime_custom_bindings.js
(right):

https://codereview.chromium.org/22944002/diff/47001/chrome/renderer/resources...
chrome/renderer/resources/extensions/app_runtime_custom_bindings.js:62: // Need
to do some special processin for this case.
On 2013/08/29 17:35:56, kalman wrote:
> processing
> 
> but this is kind of vague and so either should be removed or clarified.

Done. Removed :) Also moved the default case into the 'else' branch and removed
the return from the end of the 'if' branch -- looks better IMO.

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
File content/browser/renderer_host/url_handling_message_filter.cc (right):

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
content/browser/renderer_host/url_handling_message_filter.cc:33:
IPC_MESSAGE_UNHANDLED(handled = false)
On 2013/08/29 17:35:56, kalman wrote:
> without looking into this: would it be possible for PlatformAppRedirector to
> listen to these IPCs rather than having them in content?

Not really. All these IPCs are going through RendererProcessHostImpl first. It
uses the concept of filters to give various entities a chance to intercept
certain IPCs before they reach its own OnMessageReceived. If this filter is not
there, the IPC will unavoidable end up there (in
RendererProcessHostImpl::OnMessageReceived), and will have to be handled there.
At least that's my understanding, and nobody has been able to inform me
otherwise so far.

[Charlie Reis, 2013-08-30 01:31:40]

Thanks for removing the extension concepts from content/.  (It's still probably
worth removing the references to apps from the comments in those files.)

The message filter stuff is getting beyond what I'm comfortable reviewing,
though-- I'm not whether we have existing infrastructure that can meet your
needs without having to introduce the UrlHandlingMessageFilter.  (I don't know
InterceptNavigationResourceThrottle well enough to know if it is or is not
sufficient.)  John, can I defer this to either you or Darin?

[benwells, 2013-08-30 05:41:38]

I haven't looked at others' comments, I hope we don't contradict each other.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:113: // TODO
(sergeygs): Use the same way of creating an event (using the generated
Nit: Move this TODO to the code above where it is relevant. Note it isn't super
trivial as the arguments don't map 1:1 to the IDL (e.g. fileSystemId + baseName
-> FileEntry).

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/api/url_handlers/url_handlers_api.h (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/api/url_handlers/url_handlers_api.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
Nit: copyright for all new files should be 2013 (and without the (c) as that is
so last year).

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/extension_system.h (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/extension_system.h:134: virtual PlatformAppRedirector*
platform_app_redirector() = 0;
Does this have to live here? Can't it be a top level BrowserContextKeyedService
instead of hanging off this one?

From a depencency perspective, this is core Extensions code and should not know
about a platform app concept (which should live in the src/apps component).

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_launcher.h (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_launcher.h:6: #define
CHROME_BROWSER_EXTENSIONS_PLATFORM_APP_LAUNCHER_H_
This file now lives in apps. I think you need to rebase!

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_redirector.cc (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.cc:73: // The issuer of this
notification must that a handling app
Nit: must what?

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_redirector.h (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_PLATFORM_APP_REDIRECTOR_H_
This stuff should go in the src/apps component.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:23: namespace extensions {
This namespace should be apps.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:51: // The normal profile
associated with this ExtensionService.
s/ExtensionService/PlatformAppRedirector/

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:56: const ExtensionSet*
extensions_;
Holding on to this pointer might work now but it means the implementation of
ExtensionService must guarantee this object lives forever. Is there a reason to
hold onto it and not get it every time it is used?

https://codereview.chromium.org/22944002/diff/85001/chrome/common/extensions/...
File chrome/common/extensions/api/app_runtime.idl (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/common/extensions/...
chrome/common/extensions/api/app_runtime.idl:27: // from the
<code>url_handlers</code> manifest key.
Can you foresee a need to send through multiple URLs with one launch event? ...
I don't like the asymmetry of file handlers having an array and url handlers not
but I can see that an array is probably not needed.

[jam, 2013-08-30 16:16:38]

https://codereview.chromium.org/22944002/diff/47001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.h:85: virtual bool
HandleNavigation(WebKit::WebFrame* frame,
this isn't a method in content_renderer_client.h, what are you overriding?

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:573: 
revert all of your whitespace changes to this file

when updating code, please don't change existing style to suit personal
preference. otherwise code will keep changing over and over when different
developers touch it

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
File content/browser/renderer_host/url_handling_message_filter.h (right):

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
content/browser/renderer_host/url_handling_message_filter.h:18: // This class
filters out incoming IPC messages for the extension system on the
extension code, in content? sounds like a layering violation.. see
http://www.chromium.org/developers/content-module

there appears to be no reason to touch content at all. i suggest studying IPCs
in chrome layer to see how IPCs are handled between different layers. of
particular note, IPCs never cross content/chrome divide (i.e. see how there are
no IPCs defined in content/public). also see
http://www.chromium.org/developers/design-documents/multi-process-architectur...

[not at google - send to devlin, 2013-08-30 17:07:57]

pretty much lg from my perspective but I need to have another look after you've
rebased.

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/api/url_handlers/url_handlers_api.cc (right):

https://codereview.chromium.org/22944002/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/api/url_handlers/url_handlers_api.cc:4: 
On 2013/08/30 00:39:44, sergeygs wrote:
> On 2013/08/29 17:35:56, kalman wrote:
> > On 2013/08/29 08:24:42, sergeygs wrote:
> > > Did you mean:
> > > a) chrome/common/extensions/api/url_handlers/, or
> > > b) chrome/common/extensions/manifest_url_handler.*
> > > 
> > > ? 
> > > 
> > > I suspect the latter, but in any case, could you explain why these should
be
> > > there? The new manifest key is a part of an API, so
> > > chrome/browser/extensions/api seems like a logical place for its parser.
> FWIW,
> > I
> > > was mimicking the implementation of the file_handlers API, and the
> respective
> > > files from there are under in the same places I put mine. Also,
> > > manifest_url_handler.* deals almost exclusively with extension-specific
> keys:
> > > the only exception is 'update_url', which is still shared between both
> > > extensions and apps. The new url_handlers key will be purely app-specific.
> > > 
> > > Does that sound convincing enough? If not, I propose to leave this as is
for
> > > now, and at a later time refactor both url_handlers and file_handlers as
> > you've
> > > suggested, so they still look similar.
> > > 
> > > On 2013/08/19 23:32:24, kalman wrote:
> > > > These should be in chrome/common/extensions/manifest_handlers/...
> > > 
> > 
> > URL handles don't really have an API associated with them, do they? Like
there
> > is no chrome.urlHandler.someMethod(). The code in api/ should have been
> > restricted to such implementations (i.e. things that appear in
> > _api_features.json) though we haven't stuck with this precedent, as you've
> > noticed with file handlers.
> > 
> > Up to you though. i would like for both this and file handlers to end up in
> > chrome/common/extensions/manifest_handlers/{file,url}_handler... and if you
> want
> > to move it later, along with file handlers... sure. It's just more work and
a
> > more daunting diff later. IMO it would be easier to move now.
> 
> Not entirely sure how API is defined for this case, but perhaps they do have
an
> API: they trigger one flavor of chrome.app.runtime.onLaunched to be sent to
the
> app. Does that count as an API here? 

Let's just figure it out on that bug.

Things in
> chrome/common/extensions/manifest_url_handler.* are basically just dumb URLs. 
> 
> For now I'd value code uniformity higher than prior conventions that have
> already been broken. I've filed b/281754 for this: let's discuss offline.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:113: // TODO: Use
the same way of creating an event (using the generated
On 2013/08/30 00:39:44, sergeygs wrote:
> On 2013/08/29 17:35:56, kalman wrote:
> > TODO(sergeygs)
> 
> Done.

Sorry to be further nitpicky, but:

# airguitar ~/local_chromium/src (master)
> git grep 'TODO (' | wc -l
      56
# airguitar ~/local_chromium/src (master)
> git grep 'TODO(' | wc -l
   11915

I.e. no space :)

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_redirector.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.cc:54: case
content::NOTIFICATION_HANDLE_URL_IN_BROWSER: {
On 2013/08/30 00:39:44, sergeygs wrote:
> On 2013/08/29 17:35:56, kalman wrote:
> > It would be really nice if you could not use a notification for this but
> instead
> > an Observer interface of some sort.
> 
> It seemed really hard to connect the two communicating entities together with
an
> Observer at the time (now less so, but still). I've added a TODO to follow-up
on
> this. This currently works fine, and the only difficulty is that it's harder
to
> find who talks to who, and there is that blind type-casting on the receiving
> end.

So this class can't implement URL filter or something?

(I have no idea how this stuff is supposed to work)

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
File content/browser/renderer_host/url_handling_message_filter.cc (right):

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
content/browser/renderer_host/url_handling_message_filter.cc:33:
IPC_MESSAGE_UNHANDLED(handled = false)
On 2013/08/30 00:39:44, sergeygs wrote:
> On 2013/08/29 17:35:56, kalman wrote:
> > without looking into this: would it be possible for PlatformAppRedirector to
> > listen to these IPCs rather than having them in content?
> 
> Not really. All these IPCs are going through RendererProcessHostImpl first. It
> uses the concept of filters to give various entities a chance to intercept
> certain IPCs before they reach its own OnMessageReceived. If this filter is
not
> there, the IPC will unavoidable end up there (in
> RendererProcessHostImpl::OnMessageReceived), and will have to be handled
there.
> At least that's my understanding, and nobody has been able to inform me
> otherwise so far.

I responded to this comment in platform_app_redirector.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_redirector.cc (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.cc:35: if
(!profile->IsOffTheRecord()) {
I think you're better off CHECKing here, and making sure that this class never
gets constructed with an incognito profile. ExtensionSystem might already do
this. If you pull it into its own BrowserContextKeyedService then maybe you need
to do something trickier.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_redirector.h (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:56: const ExtensionSet*
extensions_;
On 2013/08/30 05:41:39, benwells wrote:
> Holding on to this pointer might work now but it means the implementation of
> ExtensionService must guarantee this object lives forever. Is there a reason
to
> hold onto it and not get it every time it is used?

yeah, better make it a helper function e.g. extensions() instead, though since
it's only used in one place, even easier to access it there.

https://codereview.chromium.org/22944002/diff/85001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/articles/manifest.html (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/articles/manifest.html:45: "<a
href="#file_handlers">file_handlers</a>": {...},
you have some epic rebasing to do. This file hasn't looked anything like this
for quite some time.

[sergeygs, 2013-09-02 07:36:21]

I'm freezing this CL and opening a new one:
https://codereview.chromium.org/23847004/. Much smaller and better. Let's
continue there.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:113: // TODO: Use
the same way of creating an event (using the generated
On 2013/08/30 17:07:58, kalman wrote:
> On 2013/08/30 00:39:44, sergeygs wrote:
> > On 2013/08/29 17:35:56, kalman wrote:
> > > TODO(sergeygs)
> > 
> > Done.
> 
> Sorry to be further nitpicky, but:
> 
> # airguitar ~/local_chromium/src (master)
> > git grep 'TODO (' | wc -l
>       56
> # airguitar ~/local_chromium/src (master)
> > git grep 'TODO(' | wc -l
>    11915
> 
> I.e. no space :)

Done.

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_redirector.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.cc:54: case
content::NOTIFICATION_HANDLE_URL_IN_BROWSER: {
On 2013/08/30 17:07:58, kalman wrote:
> On 2013/08/30 00:39:44, sergeygs wrote:
> > On 2013/08/29 17:35:56, kalman wrote:
> > > It would be really nice if you could not use a notification for this but
> > instead
> > > an Observer interface of some sort.
> > 
> > It seemed really hard to connect the two communicating entities together
with
> an
> > Observer at the time (now less so, but still). I've added a TODO to
follow-up
> on
> > this. This currently works fine, and the only difficulty is that it's harder
> to
> > find who talks to who, and there is that blind type-casting on the receiving
> > end.
> 
> So this class can't implement URL filter or something?
> 
> (I have no idea how this stuff is supposed to work)
This completely goes away in the 2nd round of smaller CLs (will check in very
soon), and indeed, the entire notifications and BrowserMessageFilter business
disappears, after talking to jam@ and implementing a routed IPC instead of a
control one (which indirectly necessitated all of the above).

https://codereview.chromium.org/22944002/diff/47001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:922: new
RendererHostMsg_ForwardUrlToBrowser(url, referrer_url));
On 2013/08/29 17:35:56, kalman wrote:
> I wonder whether there are any issues here with the browser trusting the
> renderer.

In this first version, apps are severely restricted in what URLs they can handle
(only ones from an app-owned domain). So even with a compromised renderer, I
don't really see what sensible harm it could do other than opening a GDocs
document in the GDocs app.
Still, if you concern is valid, what do you think could be done here to detect
and prevent an attack?

https://codereview.chromium.org/22944002/diff/47001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.h (right):

https://codereview.chromium.org/22944002/diff/47001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.h:85: virtual bool
HandleNavigation(WebKit::WebFrame* frame,
On 2013/08/30 16:16:38, jam wrote:
> this isn't a method in content_renderer_client.h, what are you overriding?

As we've already discussed in person, it is indeed a method in
content_renderer_client.h, line 170 (?).

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:573: 
On 2013/08/30 16:16:38, jam wrote:
> revert all of your whitespace changes to this file
> 
> when updating code, please don't change existing style to suit personal
> preference. otherwise code will keep changing over and over when different
> developers touch it

Already did.

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
File content/browser/renderer_host/url_handling_message_filter.cc (right):

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
content/browser/renderer_host/url_handling_message_filter.cc:33:
IPC_MESSAGE_UNHANDLED(handled = false)
On 2013/08/30 17:07:58, kalman wrote:
> On 2013/08/30 00:39:44, sergeygs wrote:
> > On 2013/08/29 17:35:56, kalman wrote:
> > > without looking into this: would it be possible for PlatformAppRedirector
to
> > > listen to these IPCs rather than having them in content?
> > 
> > Not really. All these IPCs are going through RendererProcessHostImpl first.
It
> > uses the concept of filters to give various entities a chance to intercept
> > certain IPCs before they reach its own OnMessageReceived. If this filter is
> not
> > there, the IPC will unavoidable end up there (in
> > RendererProcessHostImpl::OnMessageReceived), and will have to be handled
> there.
> > At least that's my understanding, and nobody has been able to inform me
> > otherwise so far.
> 
> I responded to this comment in platform_app_redirector.

As already mentioned elsewhere, I've talked to jam@, and he gave me a great
insight on routed-vs-control IPCs, which allowed me to do exactly what you've
suggested. This will be visible in the second round of CLs.

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
File content/browser/renderer_host/url_handling_message_filter.h (right):

https://codereview.chromium.org/22944002/diff/47001/content/browser/renderer_...
content/browser/renderer_host/url_handling_message_filter.h:18: // This class
filters out incoming IPC messages for the extension system on the
On 2013/08/30 16:16:38, jam wrote:
> extension code, in content? sounds like a layering violation.. see
> http://www.chromium.org/developers/content-module
> 
> there appears to be no reason to touch content at all. i suggest studying IPCs
> in chrome layer to see how IPCs are handled between different layers. of
> particular note, IPCs never cross content/chrome divide (i.e. see how there
are
> no IPCs defined in content/public). also see
>
http://www.chromium.org/developers/design-documents/multi-process-architectur...

After discussing this in person, I've used your advice about routed IPCs instead
of control ones, and all of the content/ has become untouched at all. This will
show up in the second round of smaller CLs (will check in very soon).

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/api/app_runtime/app_runtime_api.cc:113: // TODO
(sergeygs): Use the same way of creating an event (using the generated
On 2013/08/30 05:41:39, benwells wrote:
> Nit: Move this TODO to the code above where it is relevant. Note it isn't
super
> trivial as the arguments don't map 1:1 to the IDL (e.g. fileSystemId +
baseName
> -> FileEntry).

Done.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/api/url_handlers/url_handlers_api.h (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/api/url_handlers/url_handlers_api.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2013/08/30 05:41:39, benwells wrote:
> Nit: copyright for all new files should be 2013 (and without the (c) as that
is
> so last year).

This source disappears in the reworked CL.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/extension_system.h (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/extension_system.h:134: virtual PlatformAppRedirector*
platform_app_redirector() = 0;
On 2013/08/30 05:41:39, benwells wrote:
> Does this have to live here? Can't it be a top level
BrowserContextKeyedService
> instead of hanging off this one?
> 
> From a depencency perspective, this is core Extensions code and should not
know
> about a platform app concept (which should live in the src/apps component).
This disappears in the reworked CL.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_launcher.h (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_launcher.h:6: #define
CHROME_BROWSER_EXTENSIONS_PLATFORM_APP_LAUNCHER_H_
On 2013/08/30 05:41:39, benwells wrote:
> This file now lives in apps. I think you need to rebase!

Done in the reworked CL.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_redirector.cc (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.cc:35: if
(!profile->IsOffTheRecord()) {
On 2013/08/30 17:07:58, kalman wrote:
> I think you're better off CHECKing here, and making sure that this class never
> gets constructed with an incognito profile. ExtensionSystem might already do
> this. If you pull it into its own BrowserContextKeyedService then maybe you
need
> to do something trickier.

Done. This is harmless anyway, since apps are not supported in incognito, so
even if a redirection attempt is made, it will never find a suitable app to
handle the URL (since there are no apps).

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.cc:73: // The issuer of this
notification must that a handling app
On 2013/08/30 05:41:39, benwells wrote:
> Nit: must what?

Done.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
File chrome/browser/extensions/platform_app_redirector.h (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_PLATFORM_APP_REDIRECTOR_H_
On 2013/08/30 05:41:39, benwells wrote:
> This stuff should go in the src/apps component.

Done.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:23: namespace extensions {
On 2013/08/30 05:41:39, benwells wrote:
> This namespace should be apps.

Done.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:51: // The normal profile
associated with this ExtensionService.
On 2013/08/30 05:41:39, benwells wrote:
> s/ExtensionService/PlatformAppRedirector/

Done.

https://codereview.chromium.org/22944002/diff/85001/chrome/browser/extensions...
chrome/browser/extensions/platform_app_redirector.h:56: const ExtensionSet*
extensions_;
On 2013/08/30 05:41:39, benwells wrote:
> Holding on to this pointer might work now but it means the implementation of
> ExtensionService must guarantee this object lives forever. Is there a reason
to
> hold onto it and not get it every time it is used?

Done.

https://codereview.chromium.org/22944002/diff/85001/chrome/common/extensions/...
File chrome/common/extensions/api/app_runtime.idl (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/common/extensions/...
chrome/common/extensions/api/app_runtime.idl:27: // from the
<code>url_handlers</code> manifest key.
On 2013/08/30 05:41:39, benwells wrote:
> Can you foresee a need to send through multiple URLs with one launch event?
...
> I don't like the asymmetry of file handlers having an array and url handlers
not
> but I can see that an array is probably not needed.

I really do not foresee such a need, at least not with the currently expected
usage.

https://codereview.chromium.org/22944002/diff/85001/chrome/common/extensions/...
File chrome/common/extensions/docs/templates/articles/manifest.html (right):

https://codereview.chromium.org/22944002/diff/85001/chrome/common/extensions/...
chrome/common/extensions/docs/templates/articles/manifest.html:45: "<a
href="#file_handlers">file_handlers</a>": {...},
On 2013/08/30 17:07:58, kalman wrote:
> you have some epic rebasing to do. This file hasn't looked anything like this
> for quite some time.

Yes, I've been a couple of weeks behind as it turns. Will upload documentation
later in a separate CL.

[darin (slow to review), 2013-09-03 04:10:44]

It would be nice to make the design doc world readable. Ideally, it would be
hosted on chromium.org in some fashion. Is there a section on chromium.org for
Apps V2 design docs?

[darin (slow to review), 2013-09-03 04:10:49]

It would be nice to make the design doc world readable. Ideally, it would be
hosted on chromium.org in some fashion. Is there a section on chromium.org for
Apps V2 design docs?

[sergeygs1, 2013-09-03 16:58:38]

Yes, the copy in my chromium.org drive:

https://docs.google.com/document/d/1r-RoOv2URfZBYrT_B6notQ6MeMqZRd1EP1AITuzJC...

is world readable. I've announced it on apps-dev@chromium.org, according to
the New API Proposals process (
http://www.chromium.org/developers/design-documents/extensions/proposed-chang...).
Also per the same process, I've added the design draft to the Chrome API
Tracking table here:

https://docs.google.com/a/google.com/spreadsheet/ccc?key=0AjdhTmpdv-vvdENKZUI...
.

which is also world readable.

On Mon, Sep 2, 2013 at 9:10 PM, <darin@chromium.org> wrote:

> It would be nice to make the design doc world readable. Ideally, it would
> be
> hosted on chromium.org in some fashion. Is there a section on chromium.orgfor
> Apps V2 design docs?
>
>
https://codereview.chromium.**org/22944002/<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.