Add a mimeHandler extension API.

Add a mimeHandler extension API.

This adds the mimeHandler extension API, which is a simplified version
of the streamsPrivateApi. Unlike the streamsPrivateApi, mimeHandler only
allows access to streams from RenderFrames within MimeHandlerViewGuests,
and in particular only allows access to the stream being handled by that
MimeHandlerViewGuest instance. This removes the need for an event to be
dispatched to the background page and a user-exposed view_id.

BUG=439867
Committed: https://crrev.com/c5eb526d0e15b28dbbbcaae87d82e022e413b20d
Cr-Commit-Position: refs/heads/master@{#312728}

[Sam McNally, 2015-01-09 03:00:05]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-01-09 04:55:56]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2015-01-09 05:39:45]

Patchset #2 (id:60001) has been deleted

[Sam McNally, 2015-01-09 05:39:53]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2015-01-09 05:54:45]

sammc@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2015-01-12 05:26:05]

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(left):

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:126:
streams_[extension_id][url] = make_linked_ptr(stream->handle.release());
I think we might need this.

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(right):

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:91:
render_process_id, render_frame_id);
Can we split this up into a few statements?? :)

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/resources...
File chrome/browser/resources/pdf/main.js (right):

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/main.js:59: initViewer(streamDetails);
See my comment below about putting the synchronous case first with the early
return. WDYT?

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/main.js:80: generateStreamDetailsAndInitViewer();
I think an if/else here might be cleaer

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/main.js:84: generateStreamDetailsAndInitViewer();
Could you put the synchronous case first? This might be clearer, e.g. 

if (!chrome.mimHandler) {
  generateStreamDetailsAndInitViewer();
  return;
}

...

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/api/m...
File extensions/browser/api/mime_handler/mime_handler.cc (right):

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/api/m...
extensions/browser/api/mime_handler/mime_handler.cc:58: auto info =
stream_->stream_info();
Please add a type converter as discussed :)

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/api/m...
File extensions/browser/api/mime_handler/mime_handler.h (right):

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/api/m...
extensions/browser/api/mime_handler/mime_handler.h:26: void GetStreamInfo(const
mojo::Callback<void(mime_handler::StreamInfoPtr)>&
// mojo::InterfaceImpl<mime_handler::MimeHandlerService> overrides

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/api/m...
extensions/browser/api/mime_handler/mime_handler.h:30:
base::WeakPtr<StreamContainer> stream_;
Please add a comment.

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/guest...
File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h
(right):

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/guest...
extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h:27:
int64_t expected_content_size,
I'm tempted to remove content size because I don't think it's used by anyone.
WDYT? We could add it again later.

https://codereview.chromium.org/797183005/diff/80001/extensions/common/api/mi...
File extensions/common/api/mime_handler.mojom (right):

https://codereview.chromium.org/797183005/diff/80001/extensions/common/api/mi...
extensions/common/api/mime_handler.mojom:28: // times, the header values are
merged in the dictionary and separated by a ",
nit: maybe push " onto the next line

https://codereview.chromium.org/797183005/diff/80001/extensions/common/api/mi...
extensions/common/api/mime_handler.mojom:43: AbortStream() => ();
Maybe we should have a callback for this so we can test better :)

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
File extensions/renderer/resources/mime_handler_custom_bindings.js (right):

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/mime_handler_custom_bindings.js:13: var
STREAM_ABORTED_ERROR = 'Stream has been aborted';
nit: should we add full stops to these messages?

[Sam McNally, 2015-01-12 07:12:35]

Patchset #2 (id:100001) has been deleted

[Sam McNally, 2015-01-12 07:13:35]

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(left):

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:126:
streams_[extension_id][url] = make_linked_ptr(stream->handle.release());
On 2015/01/12 05:26:04, raymes wrote:
> I think we might need this.

Done.

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(right):

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:91:
render_process_id, render_frame_id);
On 2015/01/12 05:26:04, raymes wrote:
> Can we split this up into a few statements?? :)

Done.

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/resources...
File chrome/browser/resources/pdf/main.js (right):

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/main.js:59: initViewer(streamDetails);
On 2015/01/12 05:26:04, raymes wrote:
> See my comment below about putting the synchronous case first with the early
> return. WDYT?

Done.

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/main.js:80: generateStreamDetailsAndInitViewer();
On 2015/01/12 05:26:04, raymes wrote:
> I think an if/else here might be cleaer

Done.

https://codereview.chromium.org/797183005/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/main.js:84: generateStreamDetailsAndInitViewer();
On 2015/01/12 05:26:04, raymes wrote:
> Could you put the synchronous case first? This might be clearer, e.g. 
> 
> if (!chrome.mimHandler) {
>   generateStreamDetailsAndInitViewer();
>   return;
> }
> 
> ...

Done.

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/api/m...
File extensions/browser/api/mime_handler/mime_handler.cc (right):

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/api/m...
extensions/browser/api/mime_handler/mime_handler.cc:58: auto info =
stream_->stream_info();
On 2015/01/12 05:26:04, raymes wrote:
> Please add a type converter as discussed :)

Done.

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/api/m...
File extensions/browser/api/mime_handler/mime_handler.h (right):

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/api/m...
extensions/browser/api/mime_handler/mime_handler.h:26: void GetStreamInfo(const
mojo::Callback<void(mime_handler::StreamInfoPtr)>&
On 2015/01/12 05:26:04, raymes wrote:
> // mojo::InterfaceImpl<mime_handler::MimeHandlerService> overrides

Done.

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/api/m...
extensions/browser/api/mime_handler/mime_handler.h:30:
base::WeakPtr<StreamContainer> stream_;
On 2015/01/12 05:26:04, raymes wrote:
> Please add a comment.

Done.

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/guest...
File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h
(right):

https://codereview.chromium.org/797183005/diff/80001/extensions/browser/guest...
extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h:27:
int64_t expected_content_size,
On 2015/01/12 05:26:04, raymes wrote:
> I'm tempted to remove content size because I don't think it's used by anyone.
> WDYT? We could add it again later.

Done.

https://codereview.chromium.org/797183005/diff/80001/extensions/common/api/mi...
File extensions/common/api/mime_handler.mojom (right):

https://codereview.chromium.org/797183005/diff/80001/extensions/common/api/mi...
extensions/common/api/mime_handler.mojom:28: // times, the header values are
merged in the dictionary and separated by a ",
On 2015/01/12 05:26:04, raymes wrote:
> nit: maybe push " onto the next line

Done.

https://codereview.chromium.org/797183005/diff/80001/extensions/common/api/mi...
extensions/common/api/mime_handler.mojom:43: AbortStream() => ();
On 2015/01/12 05:26:04, raymes wrote:
> Maybe we should have a callback for this so we can test better :)

It does have a callback. It just didn't bother waiting for the stream to
actually close.

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
File extensions/renderer/resources/mime_handler_custom_bindings.js (right):

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/mime_handler_custom_bindings.js:13: var
STREAM_ABORTED_ERROR = 'Stream has been aborted';
On 2015/01/12 05:26:04, raymes wrote:
> nit: should we add full stops to these messages?

Done.

[raymes, 2015-01-13 00:05:11]

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
File extensions/renderer/resources/mime_handler_custom_bindings.js (right):

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/mime_handler_custom_bindings.js:27: var
streamInfoPromise;
Do we really need to store this promise? Would it hurt to reconstruct it for
each call, assuming there are a small number of calls?

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/mime_handler_custom_bindings.js:29: function
populateStreamInfo() {
Can this be called createStreamInfoPromise?

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/mime_handler_custom_bindings.js:38: });
Can we pull this apart a bit? Like

serviecPromise.then(getStreamInfo)
                       .then(returnStreamInfo, noStreamError);

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/mime_handler_custom_bindings.js:45:
populateStreamInfo();
If you returned the promise in the function you could change this line to
streamInfoPromise = createStreamInfoPromise();

https://codereview.chromium.org/797183005/diff/120001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/mime_handler_view/embedded.js (right):

https://codereview.chromium.org/797183005/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/mime_handler_view/embedded.js:82: var test
= urlComponents[urlComponents.length - 1].split('.')[0];
nit: maybe just add a comment about which tests are run

https://codereview.chromium.org/797183005/diff/120001/extensions/renderer/res...
File extensions/renderer/resources/mime_handler_custom_bindings.js (right):

https://codereview.chromium.org/797183005/diff/120001/extensions/renderer/res...
extensions/renderer/resources/mime_handler_custom_bindings.js:46: return
streamInfoPromise.then(function(streamInfo) {
can we pull this out into a separate function and just have 

streamInfoPromise.then(constructStreamInfoDict)

https://codereview.chromium.org/797183005/diff/120001/extensions/renderer/res...
extensions/renderer/resources/mime_handler_custom_bindings.js:64: return
service.abortStream().then(function() {}, function() {
Maybe move the then out a level here as discussed.

[Sam McNally, 2015-01-13 05:28:05]

Patchset #3 (id:140001) has been deleted

[Sam McNally, 2015-01-13 05:30:09]

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
File extensions/renderer/resources/mime_handler_custom_bindings.js (right):

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/mime_handler_custom_bindings.js:27: var
streamInfoPromise;
On 2015/01/13 00:05:11, raymes wrote:
> Do we really need to store this promise? Would it hurt to reconstruct it for
> each call, assuming there are a small number of calls?

Added a comment on what this caches and why.

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/mime_handler_custom_bindings.js:29: function
populateStreamInfo() {
On 2015/01/13 00:05:10, raymes wrote:
> Can this be called createStreamInfoPromise?

Done.

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/mime_handler_custom_bindings.js:38: });
On 2015/01/13 00:05:11, raymes wrote:
> Can we pull this apart a bit? Like
> 
> serviecPromise.then(getStreamInfo)
>                        .then(returnStreamInfo, noStreamError);
> 

Done for the error case. I think the normal case is fairly reasonable.

https://codereview.chromium.org/797183005/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/mime_handler_custom_bindings.js:45:
populateStreamInfo();
On 2015/01/13 00:05:10, raymes wrote:
> If you returned the promise in the function you could change this line to
> streamInfoPromise = createStreamInfoPromise();

Done.

https://codereview.chromium.org/797183005/diff/120001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/mime_handler_view/embedded.js (right):

https://codereview.chromium.org/797183005/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/mime_handler_view/embedded.js:82: var test
= urlComponents[urlComponents.length - 1].split('.')[0];
On 2015/01/13 00:05:11, raymes wrote:
> nit: maybe just add a comment about which tests are run

Done.

https://codereview.chromium.org/797183005/diff/120001/extensions/renderer/res...
File extensions/renderer/resources/mime_handler_custom_bindings.js (right):

https://codereview.chromium.org/797183005/diff/120001/extensions/renderer/res...
extensions/renderer/resources/mime_handler_custom_bindings.js:46: return
streamInfoPromise.then(function(streamInfo) {
On 2015/01/13 00:05:11, raymes wrote:
> can we pull this out into a separate function and just have 
> 
> streamInfoPromise.then(constructStreamInfoDict)

Done.

https://codereview.chromium.org/797183005/diff/120001/extensions/renderer/res...
extensions/renderer/resources/mime_handler_custom_bindings.js:64: return
service.abortStream().then(function() {}, function() {
On 2015/01/13 00:05:11, raymes wrote:
> Maybe move the then out a level here as discussed.

Done.

[raymes, 2015-01-13 06:28:50]

lgtm

[Sam McNally, 2015-01-13 06:41:09]

sammc@chromium.org changed reviewers:
+ benwells@chromium.org, thestig@chromium.org, zork@chromium.org

[Sam McNally, 2015-01-13 06:41:10]

+zork for chrome/browser/extensions/api/streams_private/

+benwells for extensions/

+thestig for chrome/browser/resources/component_extension_resources.grd

[Lei Zhang, 2015-01-13 19:55:28]

On 2015/01/13 06:41:10, Sam McNally wrote:
> +thestig for chrome/browser/resources/component_extension_resources.grd

.grd lgtm

[Zachary Kuznia, 2015-01-14 00:40:08]

https://codereview.chromium.org/797183005/diff/160001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/797183005/diff/160001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:521: "dependencies":
["permission:streamsPrivate"],
Is there a reason to reuse this permission?

https://codereview.chromium.org/797183005/diff/160001/extensions/common/api/m...
File extensions/common/api/mime_handler.idl (right):

https://codereview.chromium.org/797183005/diff/160001/extensions/common/api/m...
extensions/common/api/mime_handler.idl:19: long tabId;
Do you need this ID?  It should be deprecated in streamsPrivate

https://codereview.chromium.org/797183005/diff/160001/extensions/common/api/m...
File extensions/common/api/mime_handler.mojom (right):

https://codereview.chromium.org/797183005/diff/160001/extensions/common/api/m...
extensions/common/api/mime_handler.mojom:20: int32 tab_id;
ditto.

[Sam McNally, 2015-01-14 01:48:49]

https://codereview.chromium.org/797183005/diff/160001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/797183005/diff/160001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:521: "dependencies":
["permission:streamsPrivate"],
On 2015/01/14 00:40:08, Zachary Kuznia wrote:
> Is there a reason to reuse this permission?

Switched to the mime_types_handler manifest feature.

https://codereview.chromium.org/797183005/diff/160001/extensions/common/api/m...
File extensions/common/api/mime_handler.idl (right):

https://codereview.chromium.org/797183005/diff/160001/extensions/common/api/m...
extensions/common/api/mime_handler.idl:19: long tabId;
On 2015/01/14 00:40:08, Zachary Kuznia wrote:
> Do you need this ID?  It should be deprecated in streamsPrivate

Yes. PDF uses it for zoom.

[benwells, 2015-01-14 04:21:07]

https://codereview.chromium.org/797183005/diff/180001/extensions/common/api/m...
File extensions/common/api/mime_handler.idl (right):

https://codereview.chromium.org/797183005/diff/180001/extensions/common/api/m...
extensions/common/api/mime_handler.idl:6: [nodoc] namespace mimeHandler {
Is this meant to be a private API or not? If it is a private API it should be
called out in the name; otherwise we should do some kind of API approval.

[Sam McNally, 2015-01-14 05:53:15]

https://codereview.chromium.org/797183005/diff/180001/extensions/common/api/m...
File extensions/common/api/mime_handler.idl (right):

https://codereview.chromium.org/797183005/diff/180001/extensions/common/api/m...
extensions/common/api/mime_handler.idl:6: [nodoc] namespace mimeHandler {
On 2015/01/14 04:21:07, benwells wrote:
> Is this meant to be a private API or not? If it is a private API it should be
> called out in the name; otherwise we should do some kind of API approval.

It's private now, but it could become public later.

[benwells, 2015-01-14 06:31:28]

On 2015/01/14 05:53:15, Sam McNally wrote:
>
https://codereview.chromium.org/797183005/diff/180001/extensions/common/api/m...
> File extensions/common/api/mime_handler.idl (right):
> 
>
https://codereview.chromium.org/797183005/diff/180001/extensions/common/api/m...
> extensions/common/api/mime_handler.idl:6: [nodoc] namespace mimeHandler {
> On 2015/01/14 04:21:07, benwells wrote:
> > Is this meant to be a private API or not? If it is a private API it should
be
> > called out in the name; otherwise we should do some kind of API approval.
> 
> It's private now, but it could become public later.

In these cases we would normally go through an API review now to find / fix any
API issues early.

zork: how does this new API fit into your plans for streamsPrivate?

[Zachary Kuznia, 2015-01-15 00:14:59]

On 2015/01/14 06:31:28, benwells wrote:
> On 2015/01/14 05:53:15, Sam McNally wrote:
> >
>
https://codereview.chromium.org/797183005/diff/180001/extensions/common/api/m...
> > File extensions/common/api/mime_handler.idl (right):
> > 
> >
>
https://codereview.chromium.org/797183005/diff/180001/extensions/common/api/m...
> > extensions/common/api/mime_handler.idl:6: [nodoc] namespace mimeHandler {
> > On 2015/01/14 04:21:07, benwells wrote:
> > > Is this meant to be a private API or not? If it is a private API it should
> be
> > > called out in the name; otherwise we should do some kind of API approval.
> > 
> > It's private now, but it could become public later.
> 
> In these cases we would normally go through an API review now to find / fix
any
> API issues early.
> 
> zork: how does this new API fit into your plans for streamsPrivate?

I support this.  The name better fits what the API is being used for, and Raymes
and I have already discussed the design.  The QO team might need some help
moving to this API, however.

[Zachary Kuznia, 2015-01-15 00:17:33]

lgtm

[Sam McNally, 2015-01-15 05:37:37]

Patchset #5 (id:200001) has been deleted

[Sam McNally, 2015-01-15 05:40:16]

https://codereview.chromium.org/797183005/diff/180001/extensions/common/api/m...
File extensions/common/api/mime_handler.idl (right):

https://codereview.chromium.org/797183005/diff/180001/extensions/common/api/m...
extensions/common/api/mime_handler.idl:6: [nodoc] namespace mimeHandler {
On 2015/01/14 05:53:15, Sam McNally wrote:
> On 2015/01/14 04:21:07, benwells wrote:
> > Is this meant to be a private API or not? If it is a private API it should
be
> > called out in the name; otherwise we should do some kind of API approval.
> 
> It's private now, but it could become public later.

Renamed to mimeHandlerPrivate.

[benwells, 2015-01-19 01:05:19]

This CL is kind of special. It is creating and enabling the first mojo based
extension API.

+kalman, +rockot for opinions about this.

https://codereview.chromium.org/797183005/diff/360001/extensions/common/api/m...
File extensions/common/api/mime_handler_private.idl (right):

https://codereview.chromium.org/797183005/diff/360001/extensions/common/api/m...
extensions/common/api/mime_handler_private.idl:19: long tabId;
Are tab ids unique (I don't think they are). Do they need to be?

https://codereview.chromium.org/797183005/diff/360001/extensions/renderer/res...
File extensions/renderer/resources/binding.js (right):

https://codereview.chromium.org/797183005/diff/360001/extensions/renderer/res...
extensions/renderer/resources/binding.js:80: if (callback) {
Why is this necessary now? Do null callbacks normally get generated
automatically, or something?

https://codereview.chromium.org/797183005/diff/360001/extensions/renderer/res...
File extensions/renderer/resources/mime_handler_private_custom_bindings.js
(right):

https://codereview.chromium.org/797183005/diff/360001/extensions/renderer/res...
extensions/renderer/resources/mime_handler_private_custom_bindings.js:31:
function throwError(error) {
Sorry this code makes me feel dumb. Why does throwError not throw an error but
return a function that will throw an error?

[benwells, 2015-01-19 01:08:37]

For kalman / rockot's benefit: this API is just for the PDF viewer currently,
and is required to fix some bugs with making the PDF viewer out of process.

It will be applicable to QuickOffice as well one day. We'd like to delay a
proper API review until QO needs it. That will let us get the API more final
first. We know of more stuff that needs to come in to this API, and as the out
of process PDF viewer stabilises it might need more things added.

https://codereview.chromium.org/797183005/diff/380001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/797183005/diff/380001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:530: "mimeHandlerPrivate": {
How come this is at the chrome level, but the API is implemented at the
extensions level?

[Sam McNally, 2015-01-19 04:29:57]

https://codereview.chromium.org/797183005/diff/360001/extensions/common/api/m...
File extensions/common/api/mime_handler_private.idl (right):

https://codereview.chromium.org/797183005/diff/360001/extensions/common/api/m...
extensions/common/api/mime_handler_private.idl:19: long tabId;
On 2015/01/19 01:05:19, benwells wrote:
> Are tab ids unique (I don't think they are). Do they need to be?

If they aren't the tabs API is probably broken.

https://codereview.chromium.org/797183005/diff/360001/extensions/renderer/res...
File extensions/renderer/resources/binding.js (right):

https://codereview.chromium.org/797183005/diff/360001/extensions/renderer/res...
extensions/renderer/resources/binding.js:80: if (callback) {
On 2015/01/19 01:05:19, benwells wrote:
> Why is this necessary now? Do null callbacks normally get generated
> automatically, or something?

This is only used by APIs implemented on mojo and abortStream() is the first
with an optional callback. Optional args that are omitted turn into null (or
maybe undefined).

https://codereview.chromium.org/797183005/diff/360001/extensions/renderer/res...
File extensions/renderer/resources/mime_handler_private_custom_bindings.js
(right):

https://codereview.chromium.org/797183005/diff/360001/extensions/renderer/res...
extensions/renderer/resources/mime_handler_private_custom_bindings.js:31:
function throwError(error) {
On 2015/01/19 01:05:19, benwells wrote:
> Sorry this code makes me feel dumb. Why does throwError not throw an error but
> return a function that will throw an error?

Changed to not do this.

https://codereview.chromium.org/797183005/diff/380001/chrome/common/extension...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/797183005/diff/380001/chrome/common/extension...
chrome/common/extensions/api/_api_features.json:530: "mimeHandlerPrivate": {
On 2015/01/19 01:08:37, benwells wrote:
> How come this is at the chrome level, but the API is implemented at the
> extensions level?

Moved this along with mime_types_handler into extensions/

[benwells, 2015-01-19 04:56:53]

benwells@chromium.org changed reviewers:
+ kalman@chromium.org, rockot@chromium.org

[benwells, 2015-01-19 04:56:54]

lgtm

BUT +kalman and +rockot for their opinions on shipping our first mojo based API.

Really adding this time.

For context, this API is a private PDF API which fixes some problems with the
stream handling stuff and the browser plugin.

As it is only usable by out of process PDF at the moment we haven't done an API
review. We expect it to change before the out of process stuff ships.

At some point QuickOffice will need to use this as well, at which point we
should go through the API review process.

https://codereview.chromium.org/797183005/diff/360001/extensions/common/api/m...
File extensions/common/api/mime_handler_private.idl (right):

https://codereview.chromium.org/797183005/diff/360001/extensions/common/api/m...
extensions/common/api/mime_handler_private.idl:19: long tabId;
On 2015/01/19 04:29:57, Sam McNally wrote:
> On 2015/01/19 01:05:19, benwells wrote:
> > Are tab ids unique (I don't think they are). Do they need to be?
> 
> If they aren't the tabs API is probably broken.

Yep cool. I just checked, I was thinking of page IDs.

https://codereview.chromium.org/797183005/diff/360001/extensions/renderer/res...
File extensions/renderer/resources/binding.js (right):

https://codereview.chromium.org/797183005/diff/360001/extensions/renderer/res...
extensions/renderer/resources/binding.js:80: if (callback) {
On 2015/01/19 04:29:57, Sam McNally wrote:
> On 2015/01/19 01:05:19, benwells wrote:
> > Why is this necessary now? Do null callbacks normally get generated
> > automatically, or something?
> 
> This is only used by APIs implemented on mojo and abortStream() is the first
> with an optional callback. Optional args that are omitted turn into null (or
> maybe undefined).

OK. What I am wondering is if we should make safeCallbackApply and lastError.run
check the callback instead of doing it here, but I don't think it really
matters.

[Ken Rockot(use gerrit already), 2015-01-20 00:16:12]

heck yeah, lgtm

https://codereview.chromium.org/797183005/diff/460001/extensions/common/api/m...
File extensions/common/api/mime_handler_private.idl (right):

https://codereview.chromium.org/797183005/diff/460001/extensions/common/api/m...
extensions/common/api/mime_handler_private.idl:19: long tabId;
I'm pretty wary of referencing the concept of tabs in new src/extensions code,
especially APIs. I wish we had a better, content-embedder-agnostic identifier. I
don't think this is something that needs to be addressed right now unless anyone
has a suggestion that would be an immediately suitable replacement.

[not at google - send to devlin, 2015-01-20 18:34:09]

If the question is "what do you think of a mojo based API" then my answer is
"cool", is there something in the code to look at specifically?

[benwells, 2015-01-21 03:20:53]

On 2015/01/20 18:34:09, kalman wrote:
> If the question is "what do you think of a mojo based API" then my answer is
> "cool", is there something in the code to look at specifically?

It was mainly "are you ok with us releasing a mojo based api?" with a pinch of
"are you happy with the bindings implementation that mojo apis are using?".

[not at google - send to devlin, 2015-01-21 19:13:27]

Ok cool. Yes mojo APIs sound good in theory.

My one question: how much of mime_handler_private_custom_bindings.js could be
automatically generated (similar to how binding.js autogenerates from IDL/JSON)?
Seems like a lot of code. It's not sustainable to need to write something like
that for every mojo API.

Assuming it's not the case that you actually do need that amount of custom
bindings code in this case (regardless of mojo or IDL).

(ideally, assuming it's all new code, the generation could happen in C++ not JS
since we have so many issues with the JS side of things and it'd be great not to
repeat that mistake)

[raymes, 2015-01-22 05:01:50]

Hey Ben!

We're definitely aware of the custom bindings issue (that came up as a
result of implementing the serial API on mojo). We totally agree, ideally
this whole file would be auto generated. rockot@ was going to look at this
before he got roped into mojo integration with Chrome. I'm not sure if he
is still interested or has time?

We should be able to pull out the custom bindings and replace them with an
automatically generated ones later. Given that it's a known work item do
you feel like it's ok to land this CL now?

On Thu Jan 22 2015 at 6:13:29 AM <kalman@chromium.org> wrote:

> Ok cool. Yes mojo APIs sound good in theory.
>
> My one question: how much of mime_handler_private_custom_bindings.js could
> be
> automatically generated (similar to how binding.js autogenerates from
> IDL/JSON)?
> Seems like a lot of code. It's not sustainable to need to write something
> like
> that for every mojo API.
>
> Assuming it's not the case that you actually do need that amount of custom
> bindings code in this case (regardless of mojo or IDL).
>
> (ideally, assuming it's all new code, the generation could happen in C++
> not JS
> since we have so many issues with the JS side of things and it'd be great
> not to
> repeat that mistake)
>
> https://codereview.chromium.org/797183005/
>

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

[Ken Rockot(use gerrit already), 2015-01-22 05:31:10]

Oh, yeah. I started writing a response and it got lost in the land of lost
draft e-mails. TL;DR is yes I'm still looking at how do solve this!

I've become convinced, as IIRC you guys seem to have done earlier, that
generated code is probably not the right approach after all. I don't like
the idea of having 2+ layers of (effectively) IDL definitions, given that
the upper layer for mojo service <=> extension API mapping is going to
require a gnarly assortment of hooks and annotations and one-off features
in order to support all edge cases in a strictly declarative way. As I
started to flesh out what such an annotation language might look like, it
started to seem really obvious that I shouldn't be inventing another
language.

I'm looking to approach this by providing some built-in library code for
renderer JS to consume (work in progress), that reduces most renderer-side
API implementation to minimal declarative JS, with a very low barrier for
amending bits of custom, non-declarative support code as needed.

While this is still more renderer work than is usually required with
today's system (which of course typically requires no renderer work at
all), it should be much less code than we see in this CL, and the real win
as I see it comes from our eventual ability to eliminate ExtensionFunction
and all related extensions IPC, and not need any extensions-specific code
living in the browser process for the vast majority of our APIs.

In any case I think there will be some necessary ramp-up work where we have
a few more of these more complex, one-off renderer implementations. It's
really informative work and although I agree we should not be encouraging
other developers to go down the mojo-extensions-API path yet lest they
yearn to scale mountains of JavaScript, we shouldn't hesitate to move
forward with CLs like this.


On Wed, Jan 21, 2015 at 9:01 PM, Raymes Khoury <raymes@chromium.org> wrote:

> Hey Ben!
>
> We're definitely aware of the custom bindings issue (that came up as a
> result of implementing the serial API on mojo). We totally agree, ideally
> this whole file would be auto generated. rockot@ was going to look at
> this before he got roped into mojo integration with Chrome. I'm not sure if
> he is still interested or has time?
>
> We should be able to pull out the custom bindings and replace them with an
> automatically generated ones later. Given that it's a known work item do
> you feel like it's ok to land this CL now?
>
> On Thu Jan 22 2015 at 6:13:29 AM <kalman@chromium.org> wrote:
>
>> Ok cool. Yes mojo APIs sound good in theory.
>>
>> My one question: how much of mime_handler_private_custom_bindings.js
>> could
>> be
>> automatically generated (similar to how binding.js autogenerates from
>> IDL/JSON)?
>> Seems like a lot of code. It's not sustainable to need to write something
>> like
>> that for every mojo API.
>>
>> Assuming it's not the case that you actually do need that amount of custom
>> bindings code in this case (regardless of mojo or IDL).
>>
>> (ideally, assuming it's all new code, the generation could happen in C++
>> not JS
>> since we have so many issues with the JS side of things and it'd be great
>> not to
>> repeat that mistake)
>>
>> https://codereview.chromium.org/797183005/
>>
>

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

[benwells, 2015-01-22 05:36:18]

On 2015/01/22 05:31:10, Ken Rockot wrote:
> Oh, yeah. I started writing a response and it got lost in the land of lost
> draft e-mails. TL;DR is yes I'm still looking at how do solve this!
> 
> I've become convinced, as IIRC you guys seem to have done earlier, that
> generated code is probably not the right approach after all. I don't like
> the idea of having 2+ layers of (effectively) IDL definitions, given that
> the upper layer for mojo service <=> extension API mapping is going to
> require a gnarly assortment of hooks and annotations and one-off features
> in order to support all edge cases in a strictly declarative way. As I
> started to flesh out what such an annotation language might look like, it
> started to seem really obvious that I shouldn't be inventing another
> language.
> 
> I'm looking to approach this by providing some built-in library code for
> renderer JS to consume (work in progress), that reduces most renderer-side
> API implementation to minimal declarative JS, with a very low barrier for
> amending bits of custom, non-declarative support code as needed.
> 
> While this is still more renderer work than is usually required with
> today's system (which of course typically requires no renderer work at
> all), it should be much less code than we see in this CL, and the real win
> as I see it comes from our eventual ability to eliminate ExtensionFunction
> and all related extensions IPC, and not need any extensions-specific code
> living in the browser process for the vast majority of our APIs.
> 
> In any case I think there will be some necessary ramp-up work where we have
> a few more of these more complex, one-off renderer implementations. It's
> really informative work and although I agree we should not be encouraging
> other developers to go down the mojo-extensions-API path yet lest they
> yearn to scale mountains of JavaScript, we shouldn't hesitate to move
> forward with CLs like this.

Thanks for the great explanation of your thinking. I agree completely with it :)

> 
> 
> On Wed, Jan 21, 2015 at 9:01 PM, Raymes Khoury <mailto:raymes@chromium.org>
wrote:
> 
> > Hey Ben!
> >
> > We're definitely aware of the custom bindings issue (that came up as a
> > result of implementing the serial API on mojo). We totally agree, ideally
> > this whole file would be auto generated. rockot@ was going to look at
> > this before he got roped into mojo integration with Chrome. I'm not sure if
> > he is still interested or has time?
> >
> > We should be able to pull out the custom bindings and replace them with an
> > automatically generated ones later. Given that it's a known work item do
> > you feel like it's ok to land this CL now?
> >
> > On Thu Jan 22 2015 at 6:13:29 AM <mailto:kalman@chromium.org> wrote:
> >
> >> Ok cool. Yes mojo APIs sound good in theory.
> >>
> >> My one question: how much of mime_handler_private_custom_bindings.js
> >> could
> >> be
> >> automatically generated (similar to how binding.js autogenerates from
> >> IDL/JSON)?
> >> Seems like a lot of code. It's not sustainable to need to write something
> >> like
> >> that for every mojo API.
> >>
> >> Assuming it's not the case that you actually do need that amount of custom
> >> bindings code in this case (regardless of mojo or IDL).
> >>
> >> (ideally, assuming it's all new code, the generation could happen in C++
> >> not JS
> >> since we have so many issues with the JS side of things and it'd be great
> >> not to
> >> repeat that mistake)
> >>
> >> https://codereview.chromium.org/797183005/
> >>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[not at google - send to devlin, 2015-01-22 15:21:58]

Oh, I certainly don't want to block this CL on figuring out an automated (or
other) way of doing this. It's a good data point. The question was whether
implementing extension APIs using mojo is good.

We can discuss this outside this CL, but Ken do strongly favour providing as
much of this library in C++ that you can (i.e. such that extensions or whatever
can't monkey patch and cause time consuming crashes).

[Sam McNally, 2015-01-22 22:20:47]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2015-01-22 22:25:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/797183005/480001

[commit-bot: I haz the power, 2015-01-23 00:28:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-23 00:28:20]

Exceeded time limit waiting for builds to trigger.

[raymes, 2015-01-23 00:30:16]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2015-01-23 00:32:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/797183005/480001

[commit-bot: I haz the power, 2015-01-23 01:22:26]

Committed patchset #18 (id:480001)

[commit-bot: I haz the power, 2015-01-23 01:23:54]

Patchset 18 (id:??) landed as
https://crrev.com/c5eb526d0e15b28dbbbcaae87d82e022e413b20d
Cr-Commit-Position: refs/heads/master@{#312728}