Remove WebURLResponse::initialize() [revised]

Remove WebURLResponse::initialize()

The implementation of WebURLResponse::initialize() was trivial and could
easily be moved to the constructor. Remove it.

Also remove the public assign() method and the reset() method.

BUG=625529
patch from issue 2120233003 at patchset 40001 (http://crrev.com/2120233003#ps40001)

Committed: https://crrev.com/1a1f044d3edb9634238a267b9ec7186dd7933581
Cr-Commit-Position: refs/heads/master@{#404279}

[kinuko, 2016-07-05 15:38:29]

Description was changed from

==========
Remove WebURLResponse::initialize()

The implementation of WebURLResponse::initialize() was trivial and could
easily be moved to the constructor. Remove it.

Also remove the public assign() method and the reset() method.

BUG=625529

patch from issue 2120233003 at patchset 40001
(http://crrev.com/2120233003#ps40001)
==========

to

==========
Remove WebURLResponse::initialize()

The implementation of WebURLResponse::initialize() was trivial and could
easily be moved to the constructor. Remove it.

Also remove the public assign() method and the reset() method.

BUG=625529

patch from issue 2120233003 at patchset 40001
(http://crrev.com/2120233003#ps40001)
==========

[kinuko, 2016-07-05 15:38:41]

Patchset #2 (id:20001) has been deleted

[kinuko, 2016-07-05 15:38:49]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-05 15:39:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-05 17:00:51]

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

[commit-bot: I haz the power, 2016-07-05 17:00:52]

Dry run: This issue passed the CQ dry run.

[Adam Rice, 2016-07-06 01:38:59]

ricea@chromium.org changed reviewers:
+ ricea@chromium.org

[Adam Rice, 2016-07-06 01:39:00]

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right):

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:125:
m_private->setResourceResponse(*r.m_private->resourceResponse());
This changes behaviour if m_private is a WrappedResourceResponse::Handle. The
WebURLResponse will refer to the same underlying ResourceResponse, whereas
previously assignment always created a new ResourceResponse.

The tests seem to indicate that nothing actually cares about the change in
semantics, but I am not completely confident it is okay.

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebURLResponsePrivate.h
(right):

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebURLResponsePrivate.h:48: virtual
ResourceResponse* resourceResponse() = 0;
I have been assuming that whoever originally wrote this was trying to avoid
virtual method dispatch on the regular setters and getters. Otherwise why make
it so complicated?

They might have been wrong about the need for this optimisation, but they might
have been right.

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WrappedResourceResponse.h
(right):

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WrappedResourceResponse.h:72: return
m_resourceResponse;
We could optionally track whether the ResourceResponse was originally const and
DCHECK() here. In other words, moving the const checking we lost at compile time
to runtime.

[kinuko, 2016-07-06 09:45:57]

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right):

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:125:
m_private->setResourceResponse(*r.m_private->resourceResponse());
On 2016/07/06 01:39:00, Adam Rice wrote:
> This changes behaviour if m_private is a WrappedResourceResponse::Handle. The
> WebURLResponse will refer to the same underlying ResourceResponse, whereas
> previously assignment always created a new ResourceResponse.
> 
> The tests seem to indicate that nothing actually cares about the change in
> semantics, but I am not completely confident it is okay.

This assignment operator doesn't work / wouldn't be called for
WrappedResourceResponse as it's non-copyable, and if the other WebURLResponse
(r) was WrappedResourceResponse we just copy the internal ResourceResponse by
value, am I right?

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebURLResponsePrivate.h
(right):

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebURLResponsePrivate.h:48: virtual
ResourceResponse* resourceResponse() = 0;
On 2016/07/06 01:39:00, Adam Rice wrote:
> I have been assuming that whoever originally wrote this was trying to avoid
> virtual method dispatch on the regular setters and getters. Otherwise why make
> it so complicated?
> 
> They might have been wrong about the need for this optimisation, but they
might
> have been right.

That might be the case (given that WebKit code base generally tried to avoid
doing that), but it's making the code harder to read.  I could revert this part
though and try to land it separately (so that we can easily revert this specific
change if we see any regression)

[Adam Rice, 2016-07-06 10:06:50]

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right):

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:125:
m_private->setResourceResponse(*r.m_private->resourceResponse());
On 2016/07/06 09:45:57, kinuko wrote:
> On 2016/07/06 01:39:00, Adam Rice wrote:
> > This changes behaviour if m_private is a WrappedResourceResponse::Handle.
The
> > WebURLResponse will refer to the same underlying ResourceResponse, whereas
> > previously assignment always created a new ResourceResponse.
> > 
> > The tests seem to indicate that nothing actually cares about the change in
> > semantics, but I am not completely confident it is okay.
> 
> This assignment operator doesn't work / wouldn't be called for
> WrappedResourceResponse as it's non-copyable, and if the other WebURLResponse
> (r) was WrappedResourceResponse we just copy the internal ResourceResponse by
> value, am I right?

I think WrappedResourceResponse is still copyable. It doesn't have
WTF_MAKE_NONCOPYABLE at the moment. Also, it could still be assigned if passed
by non-const reference to the base class.

I agree that there are no issues when (r) is WrappedResourceResponse.

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebURLResponsePrivate.h
(right):

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebURLResponsePrivate.h:48: virtual
ResourceResponse* resourceResponse() = 0;
On 2016/07/06 09:45:57, kinuko wrote:
> On 2016/07/06 01:39:00, Adam Rice wrote:
> > I have been assuming that whoever originally wrote this was trying to avoid
> > virtual method dispatch on the regular setters and getters. Otherwise why
make
> > it so complicated?
> > 
> > They might have been wrong about the need for this optimisation, but they
> might
> > have been right.
> 
> That might be the case (given that WebKit code base generally tried to avoid
> doing that), but it's making the code harder to read.  I could revert this
part
> though and try to land it separately (so that we can easily revert this
specific
> change if we see any regression)

I think makes sense to split out the removal of initialize() from the
refactoring so we have less to revert if we see a regression.

[kinuko, 2016-07-06 11:06:33]

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right):

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:125:
m_private->setResourceResponse(*r.m_private->resourceResponse());
On 2016/07/06 10:06:50, Adam Rice wrote:
> On 2016/07/06 09:45:57, kinuko wrote:
> > On 2016/07/06 01:39:00, Adam Rice wrote:
> > > This changes behaviour if m_private is a WrappedResourceResponse::Handle.
> The
> > > WebURLResponse will refer to the same underlying ResourceResponse, whereas
> > > previously assignment always created a new ResourceResponse.
> > > 
> > > The tests seem to indicate that nothing actually cares about the change in
> > > semantics, but I am not completely confident it is okay.
> > 
> > This assignment operator doesn't work / wouldn't be called for
> > WrappedResourceResponse as it's non-copyable, and if the other
WebURLResponse
> > (r) was WrappedResourceResponse we just copy the internal ResourceResponse
by
> > value, am I right?
> 
> I think WrappedResourceResponse is still copyable. It doesn't have
> WTF_MAKE_NONCOPYABLE at the moment. Also, it could still be assigned if passed
> by non-const reference to the base class.

No it is not because Handle is not copyable.  It's not very obvious, and that's
why I commented on your patch to explicitly mark it with WTF_MAKE_NONCOPYABLE
(which I haven't done that in my revised version yet-- I'll do so in next
upload).

https://codereview.chromium.org/2120233003/diff/60001/third_party/WebKit/Sour...

> I agree that there are no issues when (r) is WrappedResourceResponse.

[kinuko, 2016-07-06 17:04:50]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-06 17:05:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinuko, 2016-07-06 17:07:03]

Patchset #3 (id:60001) has been deleted

[kinuko, 2016-07-06 17:07:12]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-06 17:07:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinuko, 2016-07-06 17:13:12]

Patchset #3 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-07-06 17:18:45]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[kinuko, 2016-07-06 18:17:17]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-06 18:18:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-06 21:08:09]

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

[commit-bot: I haz the power, 2016-07-06 21:08:10]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-07-06 23:19:12]

Uploaded a new patch.  Reg: copying wrapped ones via base refs/ptrs I started to
feel it's purely problematic no matter how it works (and it doesn't look it's
used), I just made it fail/crash in thew new patch.  ptal

[Adam Rice, 2016-07-07 01:32:48]

lgtm

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right):

https://codereview.chromium.org/2117313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:125:
m_private->setResourceResponse(*r.m_private->resourceResponse());
On 2016/07/06 11:06:33, kinuko wrote:
> On 2016/07/06 10:06:50, Adam Rice wrote:
> > On 2016/07/06 09:45:57, kinuko wrote:
> > > On 2016/07/06 01:39:00, Adam Rice wrote:
> > > > This changes behaviour if m_private is a
WrappedResourceResponse::Handle.
> > The
> > > > WebURLResponse will refer to the same underlying ResourceResponse,
whereas
> > > > previously assignment always created a new ResourceResponse.
> > > > 
> > > > The tests seem to indicate that nothing actually cares about the change
in
> > > > semantics, but I am not completely confident it is okay.
> > > 
> > > This assignment operator doesn't work / wouldn't be called for
> > > WrappedResourceResponse as it's non-copyable, and if the other
> WebURLResponse
> > > (r) was WrappedResourceResponse we just copy the internal ResourceResponse
> by
> > > value, am I right?
> > 
> > I think WrappedResourceResponse is still copyable. It doesn't have
> > WTF_MAKE_NONCOPYABLE at the moment. Also, it could still be assigned if
passed
> > by non-const reference to the base class.
> 
> No it is not because Handle is not copyable.  It's not very obvious, and
that's
> why I commented on your patch to explicitly mark it with WTF_MAKE_NONCOPYABLE
> (which I haven't done that in my revised version yet-- I'll do so in next
> upload).
> 
>
https://codereview.chromium.org/2120233003/diff/60001/third_party/WebKit/Sour...
> 
> > I agree that there are no issues when (r) is WrappedResourceResponse. 
> 

Sorry, I missed that point.

[kinuko, 2016-07-07 03:11:02]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 03:11:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinuko, 2016-07-07 03:22:30]

kinuko@chromium.org changed reviewers:
+ dcheng@chromium.org

[kinuko, 2016-07-07 03:23:26]

Thanks Adam!

dcheng@- can you probably review changes in Source/web (and maybe also in
public/)?

[commit-bot: I haz the power, 2016-07-07 04:33:10]

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

[commit-bot: I haz the power, 2016-07-07 04:33:12]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-07-07 05:02:50]

https://codereview.chromium.org/2117313002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right):

https://codereview.chromium.org/2117313002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:127: return
!m_private->m_resourceResponse || m_private->m_resourceResponse->isNull();
It looks like this can only happen when using the default constructor of
WrappedResourceRequest. If codesearch is accurate, the only case where this
happens in WebDataSourceImpl. But WebDataSourceImpl::response() always binds
DocumentLoader::response() before returning.

Does it makes sense to:
- Always bind the response in WebDataSourceImpl ctor
- Remove the default ctor for WrappedResourceResponse
- Remove the !m_private->m_resourceResponse check, since I /think/ it will be
non-null in all other cases.

[kinuko, 2016-07-07 06:20:36]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 06:20:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 07:30:21]

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

[commit-bot: I haz the power, 2016-07-07 07:30:23]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-07-07 09:21:03]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 09:21:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinuko, 2016-07-07 09:23:04]

https://codereview.chromium.org/2117313002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right):

https://codereview.chromium.org/2117313002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:127: return
!m_private->m_resourceResponse || m_private->m_resourceResponse->isNull();
On 2016/07/07 05:02:50, dcheng wrote:
> It looks like this can only happen when using the default constructor of
> WrappedResourceRequest. If codesearch is accurate, the only case where this
> happens in WebDataSourceImpl. But WebDataSourceImpl::response() always binds
> DocumentLoader::response() before returning.
> 
> Does it makes sense to:
> - Always bind the response in WebDataSourceImpl ctor
> - Remove the default ctor for WrappedResourceResponse
> - Remove the !m_private->m_resourceResponse check, since I /think/ it will be
> non-null in all other cases.

Looks like the change works, and I like simplifying code.  Done.

[kinuko, 2016-07-07 09:25:45]

On 2016/07/07 09:23:04, kinuko wrote:
>
https://codereview.chromium.org/2117313002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right):
> 
>
https://codereview.chromium.org/2117313002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:127: return
> !m_private->m_resourceResponse || m_private->m_resourceResponse->isNull();
> On 2016/07/07 05:02:50, dcheng wrote:
> > It looks like this can only happen when using the default constructor of
> > WrappedResourceRequest. If codesearch is accurate, the only case where this
> > happens in WebDataSourceImpl. But WebDataSourceImpl::response() always binds
> > DocumentLoader::response() before returning.
> > 
> > Does it makes sense to:
> > - Always bind the response in WebDataSourceImpl ctor
> > - Remove the default ctor for WrappedResourceResponse
> > - Remove the !m_private->m_resourceResponse check, since I /think/ it will
be
> > non-null in all other cases.
> 
> Looks like the change works, and I like simplifying code.  Done.

Hold on... I guess we can then also remove lazy binding code.

[kinuko, 2016-07-07 09:26:32]

Patchset #6 (id:160001) has been deleted

[kinuko, 2016-07-07 09:26:40]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 09:26:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinuko, 2016-07-07 09:29:56]

ok, PTAL

[dcheng, 2016-07-07 09:54:19]

LGTM

[commit-bot: I haz the power, 2016-07-07 10:40:18]

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

[commit-bot: I haz the power, 2016-07-07 10:40:20]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-07-07 12:45:18]

kinuko@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[kinuko, 2016-07-07 12:47:17]

Dale: can you review media/ changes?  (It's refactoring-related one-liner
change, might just TBR)

[DaleCurtis, 2016-07-07 20:35:43]

lgtm

[kinuko, 2016-07-08 00:33:27]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2016-07-08 00:33:28]

The patchset sent to the CQ was uploaded after l-g-t-m from ricea@chromium.org
Link to the patchset: https://codereview.chromium.org/2117313002/#ps180001
(title: "cleanups")

[commit-bot: I haz the power, 2016-07-08 00:34:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 00:46:15]

Description was changed from

==========
Remove WebURLResponse::initialize()

The implementation of WebURLResponse::initialize() was trivial and could
easily be moved to the constructor. Remove it.

Also remove the public assign() method and the reset() method.

BUG=625529

patch from issue 2120233003 at patchset 40001
(http://crrev.com/2120233003#ps40001)
==========

to

==========
Remove WebURLResponse::initialize()

The implementation of WebURLResponse::initialize() was trivial and could
easily be moved to the constructor. Remove it.

Also remove the public assign() method and the reset() method.

BUG=625529

patch from issue 2120233003 at patchset 40001
(http://crrev.com/2120233003#ps40001)
==========

[commit-bot: I haz the power, 2016-07-08 00:46:17]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-07-08 00:46:20]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 00:47:43]

Description was changed from

==========
Remove WebURLResponse::initialize()

The implementation of WebURLResponse::initialize() was trivial and could
easily be moved to the constructor. Remove it.

Also remove the public assign() method and the reset() method.

BUG=625529

patch from issue 2120233003 at patchset 40001
(http://crrev.com/2120233003#ps40001)
==========

to

==========
Remove WebURLResponse::initialize()

The implementation of WebURLResponse::initialize() was trivial and could
easily be moved to the constructor. Remove it.

Also remove the public assign() method and the reset() method.

BUG=625529

patch from issue 2120233003 at patchset 40001
(http://crrev.com/2120233003#ps40001)

Committed: https://crrev.com/1a1f044d3edb9634238a267b9ec7186dd7933581
Cr-Commit-Position: refs/heads/master@{#404279}
==========

[commit-bot: I haz the power, 2016-07-08 00:47:45]

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