Remove support for a URLRequest having a NULL Delegate.

Remove support for a URLRequest having a NULL Delegate.

This could be used to prevent it from invoking any callbacks, but
this codepath was untested, and is only used by one consumer in
production codes and a couple random tests, which can all easily
be modified not to depend on it.

BUG=569055
TBR=marq@chromium.org

Committed: https://crrev.com/8e329d672df2c725b3add82a186f30a1241eee01
Cr-Commit-Position: refs/heads/master@{#365909}

[mmenke, 2015-12-11 18:35:06]

Description was changed from

==========
Remove support for setting a URLRequest's Delegate to nullptr.

This could be used to prevent it from invoking any callbacks, but
this codepath is largely untested, and only has one consumer, which
can easily be modified not to depend on this behavior.

BUG=569055
==========

to

==========
Remove support for a URLRequest having a NULL Delegate.

This could be used to prevent it from invoking any callbacks, but
this codepath is largely untested, and only has one consumer, which
can easily be modified not to depend on this behavior.

BUG=569055
==========

[mmenke, 2015-12-11 18:36:05]

Description was changed from

==========
Remove support for a URLRequest having a NULL Delegate.

This could be used to prevent it from invoking any callbacks, but
this codepath is largely untested, and only has one consumer, which
can easily be modified not to depend on this behavior.

BUG=569055
==========

to

==========
Remove support for a URLRequest having a NULL Delegate.

This could be used to prevent it from invoking any callbacks, but
this codepath was untested, and is only used by one consumer in
production codes and a couple random tests, which can all easily
be modified not to depend on it.

BUG=569055
==========

[mmenke, 2015-12-11 22:22:15]

Patchset #5 (id:80001) has been deleted

[mmenke, 2015-12-11 22:22:24]

Patchset #4 (id:60001) has been deleted

[mmenke, 2015-12-14 16:58:22]

mmenke@chromium.org changed reviewers:
+ michaeln@chromium.org, stuartmorgan@chromium.org, tzik@chromium.org,
xunjieli@chromium.org

[mmenke, 2015-12-14 16:58:23]

[xunjieli]:  Please review everything.
[michaeln]:  Please review appcache.
[stuartmorgan]:  Please review iOS.
[tzik]:  Please review storage/browser/fileapi

[mmenke, 2015-12-14 17:02:30]

https://codereview.chromium.org/1523433002/diff/100001/content/browser/appcac...
File content/browser/appcache/appcache_storage_impl_unittest.cc (right):

https://codereview.chromium.org/1523433002/diff/100001/content/browser/appcac...
content/browser/appcache/appcache_storage_impl_unittest.cc:394:
AppCacheStorageImplTest() { request_delegate_.set_quit_on_complete(false); }
Note that I used a different approach here and request_tracker_impl_unittest.mm,
largely because I consider AppCache more integrated into the network stack, so
more reasonable to depend on funky net classes, like TestDelegate.  They could
just both use the same solution.

[xunjieli, 2015-12-14 20:12:08]

code is a lot easier to understand with the CL!

https://codereview.chromium.org/1523433002/diff/100001/net/url_request/url_re...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1523433002/diff/100001/net/url_request/url_re...
net/url_request/url_request.h:528: // must have a delegate set before this
method is called.
nit: maybe s/delegate/Delegate to be consistent with the first instance of this
word in this comment.

https://codereview.chromium.org/1523433002/diff/100001/net/url_request/url_re...
net/url_request/url_request.h:729: // |delegate_| is not NULL. See
URLRequest::Delegate for the meaning
I think we probably can get rid of "and may only be used if |delegate_| is not
NULL", since these functions are called after Start().

[mmenke, 2015-12-14 20:24:11]

Thanks!

https://codereview.chromium.org/1523433002/diff/100001/net/url_request/url_re...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1523433002/diff/100001/net/url_request/url_re...
net/url_request/url_request.h:528: // must have a delegate set before this
method is called.
On 2015/12/14 20:12:08, xunjieli wrote:
> nit: maybe s/delegate/Delegate to be consistent with the first instance of
this
> word in this comment.

I decided to go with "delegate" in both places instead.  I think Delegate is
better, when we mean URLRequest::Delegate only and not NetworkDelegate, but this
file uses "delegate" almost everywhere.  May fix it in another CL, but too many
places to fix in this CL, I think.

https://codereview.chromium.org/1523433002/diff/100001/net/url_request/url_re...
net/url_request/url_request.h:729: // |delegate_| is not NULL. See
URLRequest::Delegate for the meaning
On 2015/12/14 20:12:08, xunjieli wrote:
> I think we probably can get rid of "and may only be used if |delegate_| is not
> NULL", since these functions are called after Start().

Nice catch, done!

[michaeln, 2015-12-14 20:27:27]

https://codereview.chromium.org/1523433002/diff/100001/storage/browser/fileap...
File storage/browser/fileapi/file_writer_delegate.cc (right):

https://codereview.chromium.org/1523433002/diff/100001/storage/browser/fileap...
storage/browser/fileapi/file_writer_delegate.cc:160: Read();
Not sure? I think you may need a null check in Read() now.

[mmenke, 2015-12-14 20:38:23]

https://codereview.chromium.org/1523433002/diff/100001/storage/browser/fileap...
File storage/browser/fileapi/file_writer_delegate.cc (right):

https://codereview.chromium.org/1523433002/diff/100001/storage/browser/fileap...
storage/browser/fileapi/file_writer_delegate.cc:160: Read();
On 2015/12/14 20:27:26, michaeln wrote:
> Not sure? I think you may need a null check in Read() now.

I don't think so.  From FileStreamWriter::Cancel:

// In either case, the callback function passed to the in-flight async
// operation is dismissed immediately when Cancel() is called, and thus
// will never be called.

If that documentation is accurate, the call to FileStreamWriter::Cancel in
FileWriterDelegate::Cancel should ensure this isn't called after cancellation,
right?

[michaeln, 2015-12-14 20:58:30]

https://codereview.chromium.org/1523433002/diff/100001/storage/browser/fileap...
File storage/browser/fileapi/file_writer_delegate.cc (right):

https://codereview.chromium.org/1523433002/diff/100001/storage/browser/fileap...
storage/browser/fileapi/file_writer_delegate.cc:160: Read();
On 2015/12/14 20:38:23, mmenke wrote:
> On 2015/12/14 20:27:26, michaeln wrote:
> > Not sure? I think you may need a null check in Read() now.
> 
> I don't think so.  From FileStreamWriter::Cancel:
> 
> // In either case, the callback function passed to the in-flight async
> // operation is dismissed immediately when Cancel() is called, and thus
> // will never be called.
> 
> If that documentation is accurate, the call to FileStreamWriter::Cancel in
> FileWriterDelegate::Cancel should ensure this isn't called after cancellation,
> right?

What about the other .reset() callsite in OnError? It's hard to be convinced
this isn't reentrant in unexpected ways?

[mmenke, 2015-12-14 21:15:56]

On 2015/12/14 20:58:30, michaeln wrote:
>
https://codereview.chromium.org/1523433002/diff/100001/storage/browser/fileap...
> File storage/browser/fileapi/file_writer_delegate.cc (right):
> 
>
https://codereview.chromium.org/1523433002/diff/100001/storage/browser/fileap...
> storage/browser/fileapi/file_writer_delegate.cc:160: Read();
> On 2015/12/14 20:38:23, mmenke wrote:
> > On 2015/12/14 20:27:26, michaeln wrote:
> > > Not sure? I think you may need a null check in Read() now.
> > 
> > I don't think so.  From FileStreamWriter::Cancel:
> > 
> > // In either case, the callback function passed to the in-flight async
> > // operation is dismissed immediately when Cancel() is called, and thus
> > // will never be called.
> > 
> > If that documentation is accurate, the call to FileStreamWriter::Cancel in
> > FileWriterDelegate::Cancel should ensure this isn't called after
cancellation,
> > right?
> 
> What about the other .reset() callsite in OnError? It's hard to be convinced
> this isn't reentrant in unexpected ways?

It looks like OnFlushed doesn't call into request_, and there's only a network
read or file write in progress, so if OnError is triggered by the URLRequest,
there's no pending write operation.  If it's due to a write failure, there was a
pending write operation, but it just completed.  Or am I missing something?

[michaeln, 2015-12-14 22:57:43]

lgtm, sorry for the distracting questions

> It looks like OnFlushed doesn't call into request_, and there's only a network
> read or file write in progress, so if OnError is triggered by the URLRequest,
> there's no pending write operation.  If it's due to a write failure, there was
a
> pending write operation, but it just completed.  Or am I missing something?

[mmenke, 2015-12-14 22:59:52]

On 2015/12/14 22:57:43, michaeln wrote:
> lgtm, sorry for the distracting questions

Nothing to apologize for - I'd much rather you be thinking about these things
and risk being wrong than just stamp the CL without thinking about them.

[tzik, 2015-12-15 05:58:03]

fileapi lgtm

[xunjieli, 2015-12-15 14:42:21]

lgtm! was unable to find other things to comment on :)

[mmenke, 2015-12-16 20:39:41]

mmenke@chromium.org changed reviewers:
+ marq@chromium.org
- stuartmorgan@chromium.org

[mmenke, 2015-12-16 20:39:42]

[+marq]:  Please review iOS.

[mmenke, 2015-12-17 20:36:30]

On 2015/12/16 20:39:42, mmenke wrote:
> [+marq]:  Please review iOS.

Just going to TBR it.  Want to get it in before I leave, and this gives me time
tomorrow in case anything breaks.

[mmenke, 2015-12-17 20:36:46]

Description was changed from

==========
Remove support for a URLRequest having a NULL Delegate.

This could be used to prevent it from invoking any callbacks, but
this codepath was untested, and is only used by one consumer in
production codes and a couple random tests, which can all easily
be modified not to depend on it.

BUG=569055
==========

to

==========
Remove support for a URLRequest having a NULL Delegate.

This could be used to prevent it from invoking any callbacks, but
this codepath was untested, and is only used by one consumer in
production codes and a couple random tests, which can all easily
be modified not to depend on it.

BUG=569055
TBR=marq@chromium.org
==========

[mmenke, 2015-12-17 20:37:12]

On 2015/12/17 20:36:30, mmenke wrote:
> On 2015/12/16 20:39:42, mmenke wrote:
> > [+marq]:  Please review iOS.
> 
> Just going to TBR it.  Want to get it in before I leave, and this gives me
time
> tomorrow in case anything breaks.

(And worth noting this is just updating the test for a minor API change)

[mmenke, 2015-12-17 20:37:20]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2015-12-17 20:39:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1523433002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1523433002/140001

[commit-bot: I haz the power, 2015-12-17 22:16:56]

Description was changed from

==========
Remove support for a URLRequest having a NULL Delegate.

This could be used to prevent it from invoking any callbacks, but
this codepath was untested, and is only used by one consumer in
production codes and a couple random tests, which can all easily
be modified not to depend on it.

BUG=569055
TBR=marq@chromium.org
==========

to

==========
Remove support for a URLRequest having a NULL Delegate.

This could be used to prevent it from invoking any callbacks, but
this codepath was untested, and is only used by one consumer in
production codes and a couple random tests, which can all easily
be modified not to depend on it.

BUG=569055
TBR=marq@chromium.org
==========

[commit-bot: I haz the power, 2015-12-17 22:16:58]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-12-17 22:18:29]

Description was changed from

==========
Remove support for a URLRequest having a NULL Delegate.

This could be used to prevent it from invoking any callbacks, but
this codepath was untested, and is only used by one consumer in
production codes and a couple random tests, which can all easily
be modified not to depend on it.

BUG=569055
TBR=marq@chromium.org
==========

to

==========
Remove support for a URLRequest having a NULL Delegate.

This could be used to prevent it from invoking any callbacks, but
this codepath was untested, and is only used by one consumer in
production codes and a couple random tests, which can all easily
be modified not to depend on it.

BUG=569055
TBR=marq@chromium.org

Committed: https://crrev.com/8e329d672df2c725b3add82a186f30a1241eee01
Cr-Commit-Position: refs/heads/master@{#365909}
==========

[commit-bot: I haz the power, 2015-12-17 22:18:30]

Patchset 6 (id:??) landed as
https://crrev.com/8e329d672df2c725b3add82a186f30a1241eee01
Cr-Commit-Position: refs/heads/master@{#365909}

[marq (ping after 24h), 2015-12-18 08:18:19]

LGTM.