Implement chrome.streamsPrivate.abort() extensions function

Implement chrome.streamsPrivate.abort() extensions function

This implements an extensions API to abort a stream that has been started. Aborting the stream will cause the original URL request to be cancelled, which may be desired if the client of the API wants to cancel the request before it is completely downloaded.

BUG=348464, 303491
R=jam@chromium.org, kalman@chromium.org, mpearson@chromium.org, zork@chromium.org

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

[raymes, 2014-05-12 06:15:50]

Previously the underlying URL request wouldn't be cancelled if you abort() a
request for a blob URL corresponding to a stream. This meant that the original
URL could never be re-requested until it had been downloaded completely.

BUG=348464

[Zachary Kuznia, 2014-05-12 09:20:22]

This will break the use case where a stream is passed from one reader to
another.  Instead of hooking this functionality into stream_url_request_job.cc,
there should be a chrome.streamsPrivate.abortStream() function added.

The rest of the patch should be good, though.

tyoshino@: Do you expect this to affect W3C Streams?  This API should eventually
be replaced with ReadableStream.readAbort()

jliebrand@:  FYI

[tyoshino (SeeGerritForStatus), 2014-05-12 11:46:07]

On 2014/05/12 09:20:22, Zachary Kuznia wrote:
> This will break the use case where a stream is passed from one reader to
> another.  Instead of hooking this functionality into
stream_url_request_job.cc,
> there should be a chrome.streamsPrivate.abortStream() function added.
> 
> The rest of the patch should be good, though.
> 
> tyoshino@: Do you expect this to affect W3C Streams?  This API should
eventually
> be replaced with ReadableStream.readAbort()

FYI, there was some hard discussion between WHATWG and W3C last year and the
spec is now drafted at github.com/whatwg/streams.

A big change is expected to happen and maybe I'll need to do drastic redesign on
the streams infrastructure.

For now, please feel free to make change on the existing streams implementation.

> 
> jliebrand@:  FYI

[raymes, 2014-05-15 03:48:05]

Thanks Zach. PTAL at the new CL. I added a chrome.streamsPrivate.abort()
function which takes a stream URL.

[Zachary Kuznia, 2014-05-15 13:43:26]

https://codereview.chromium.org/281513003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/streams_private/streams_private_api.h
(right):

https://codereview.chromium.org/281513003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/streams_private/streams_private_api.h:73: class
StreamsPrivateAbortFunction : public AsyncApiFunction {
Is there a reason you don't derive from SyncExtensionFunction?  It looks like
you know immediately if the call succeeded or failed.

[raymes, 2014-05-16 01:09:18]

https://codereview.chromium.org/281513003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/streams_private/streams_private_api.h
(right):

https://codereview.chromium.org/281513003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/streams_private/streams_private_api.h:73: class
StreamsPrivateAbortFunction : public AsyncApiFunction {
On 2014/05/15 13:43:27, Zachary Kuznia wrote:
> Is there a reason you don't derive from SyncExtensionFunction?  It looks like
> you know immediately if the call succeeded or failed.

Done.

[Zachary Kuznia, 2014-05-17 05:40:02]

lgtm

[raymes, 2014-05-19 00:06:09]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-05-19 00:06:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/281513003/60001

[raymes, 2014-05-19 00:26:07]

+mpearson,kalman for OWNERS
mpearson:
  extensions/browser/extension_function_histogram_value.h
kalman:
 chrome/browser/extensions/api/streams_private/streams_private_api.cc
 chrome/browser/extensions/api/streams_private/streams_private_api.h
  chrome/common/extensions/api/streams_private.idl

[not at google - send to devlin, 2014-05-19 16:28:16]

CL description looks like it's been cut off?

Could you add a test?

[Mark P, 2014-05-19 22:23:53]

extensions/browser/extension_function_histogram_value.h lgtm

[raymes, 2014-05-20 03:24:08]

On 2014/05/19 22:23:53, Mark P wrote:
> extensions/browser/extension_function_histogram_value.h lgtm

Thanks for pushing on the test. I thought it would be hard to test but it turned
out not to be too bad. PTAL

[not at google - send to devlin, 2014-05-20 19:47:45]

https://codereview.chromium.org/281513003/diff/100001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/streams_private/handle_mime_type/background.js
(right):

https://codereview.chromium.org/281513003/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/streams_private/handle_mime_type/background.js:64:
if (xhr.status == 200) {
how about chrome.test.assertEq(200, xhr.status, 'Expected....')

maybe include the status in the message?

you'd lose testing both files (assertEq throws an exception) but that doesn't
seem too bad.

https://codereview.chromium.org/281513003/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/streams_private/handle_mime_type/background.js:69:
hasfailed = true;
hasFailed?

[raymes, 2014-05-21 00:37:56]

https://codereview.chromium.org/281513003/diff/100001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/streams_private/handle_mime_type/background.js
(right):

https://codereview.chromium.org/281513003/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/streams_private/handle_mime_type/background.js:64:
if (xhr.status == 200) {
hmm but then I wouldn't be able to set hasFailed to false? I'm not sure what
hasFailed is actually useful for...I was just following the pattern of the
previous tests.

https://codereview.chromium.org/281513003/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/streams_private/handle_mime_type/background.js:69:
hasfailed = true;
On 2014/05/20 19:47:45, kalman wrote:
> hasFailed?

Done.

[not at google - send to devlin, 2014-05-21 01:37:53]

lgtm

https://codereview.chromium.org/281513003/diff/120001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/streams_private/handle_mime_type/background.js
(right):

https://codereview.chromium.org/281513003/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/streams_private/handle_mime_type/background.js:81:
chrome.test.notifyFail(
yeah i'm not sure if repeatedly calling notifyFail(..) is a good idea here. but
as you said, that's the idiom of the test.

same goes for notifyPass().

however... from reading the code, the only place where hasFailed is read appears
to be when it can only ever be true, because every time hasFailed is set to
false the test returns early.

eh, this test is supicious. oh well.

[raymes, 2014-05-21 01:46:54]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-05-21 01:51:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/281513003/120001

[commit-bot: I haz the power, 2014-05-21 07:55:39]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-21 09:31:13]

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

[commit-bot: I haz the power, 2014-05-21 09:31:13]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[raymes, 2014-05-22 23:49:40]

kalman@: I had to make a change to the test to prevent flakiness because the the
deletion of the stream happens on a separate thread to the API call in the
browser. Issuing a separate request ensures that the message loops are pumped
sufficiently.

Alternatively I could add a callback to the abort() API but it seems unnecessary
because in practice a client never has to know when the stream is aborted, only
that it will be.

[not at google - send to devlin, 2014-05-23 00:10:37]

notifying the extension when the stream is aborted sounds fine, if you actually
know when that is. it's pretty common practice (for avoiding race conditions...)

[raymes, 2014-05-23 04:25:23]

Done. I had to add a function to the content API to allow registering a callback
for abort. zach@ could you PTAL too?

[raymes, 2014-05-23 04:25:47]

On 2014/05/23 04:25:23, raymes wrote:
> Done. I had to add a function to the content API to allow registering a
callback
> for abort. zach@ could you PTAL too?

Sorry I meant zork@!

[Zachary Kuznia, 2014-05-23 17:01:13]

lgtm

[not at google - send to devlin, 2014-05-23 17:03:48]

lgtm

https://codereview.chromium.org/281513003/diff/260001/chrome/common/extension...
File chrome/common/extensions/api/streams_private.idl (right):

https://codereview.chromium.org/281513003/diff/260001/chrome/common/extension...
chrome/common/extensions/api/streams_private.idl:40: AbortCallback callback);
consider making this optional.

[raymes, 2014-05-26 01:22:29]

+jam for content API

[jam, 2014-05-27 01:35:44]

On 2014/05/26 01:22:29, raymes wrote:
> +jam for content API

lgtm

[jam, 2014-05-27 01:35:45]

On 2014/05/26 01:22:29, raymes wrote:
> +jam for content API

lgtm

[raymes, 2014-05-27 01:45:26]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-05-27 01:45:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/281513003/280001

[commit-bot: I haz the power, 2014-05-27 02:41:21]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/9217)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12307)

[commit-bot: I haz the power, 2014-05-27 02:43:55]

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

[commit-bot: I haz the power, 2014-05-27 02:43:56]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[raymes, 2014-05-27 06:57:20]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-05-27 06:57:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/281513003/300001

[commit-bot: I haz the power, 2014-05-27 08:42:50]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-27 09:09:03]

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

[commit-bot: I haz the power, 2014-05-27 09:09:03]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[raymes, 2014-05-28 00:27:03]

Committed patchset #12 manually as r273087 (presubmit successful).

[wjywbs, 2014-05-28 00:53:58]

https://codereview.chromium.org/281513003/diff/320001/extensions/browser/exte...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/281513003/diff/320001/extensions/browser/exte...
extensions/browser/extension_function_histogram_value.h:844:
STREAMSPRIVATE_ABORT,
I think you also need to change
tools/metrics/histograms/histograms/histograms.xml as described below. For
example, https://codereview.chromium.org/299443009 After your fix, I'll rebase
the histograms of my CL again... Thanks a lot.

[raymes, 2014-05-28 01:32:48]

Thanks for letting me know, sorry about that! I posted:
https://codereview.chromium.org/307453009/


On Wed, May 28, 2014 at 10:53 AM, <wjywbs@gmail.com> wrote:

>
> https://codereview.chromium.org/281513003/diff/320001/
> extensions/browser/extension_function_histogram_value.h
> File extensions/browser/extension_function_histogram_value.h (right):
>
> https://codereview.chromium.org/281513003/diff/320001/
> extensions/browser/extension_function_histogram_value.h#newcode844
> extensions/browser/extension_function_histogram_value.h:844:
> STREAMSPRIVATE_ABORT,
> I think you also need to change
> tools/metrics/histograms/histograms/histograms.xml as described below.
> For example, https://codereview.chromium.org/299443009 After your fix,
> I'll rebase the histograms of my CL again... Thanks a lot.
>
> https://codereview.chromium.org/281513003/
>

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