[OBSOLETE] Remove WebURLResponse::initialize()

[OBSOLETE] 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

[Adam Rice, 2016-07-04 10:18:42]

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

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

Sorry for slow review, this code looks generally good but it sticks me that the
current class composition feels a bit convoluted (esp. with
WrappedResourceResponse and WebURLResponsePrivate inheritance hacks). Let me
take another look later today before making more comments.

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

https://codereview.chromium.org/2120233003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WrappedResourceResponse.h:57: void
bind(ResourceResponse& resourceResponse)
If code search's working correctly we no longer seem to have callsites for this
one (other than from the other bind)

[Adam Rice, 2016-07-05 10:02:42]

One possibility I considered is to make WebURLResponse a base class with
undefined ownership semantics and no public constructors.

To instantiate one you'd have to choose between new subclass WebURLResponseImpl
(owns a ResourceResponse) or WrappedResourceResponse (unowned, as now).

The implementation would get simpler, but I'm not sure it would actually help
readability.

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

https://codereview.chromium.org/2120233003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WrappedResourceResponse.h:57: void
bind(ResourceResponse& resourceResponse)
On 2016/07/05 09:29:09, kinuko wrote:
> If code search's working correctly we no longer seem to have callsites for
this
> one (other than from the other bind)

Thanks, removed.

[kinuko, 2016-07-05 14:59:44]

Some more comments.

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

https://codereview.chromium.org/2120233003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:353:
DCHECK(m_private);
This DCHECK seems redundant now (here and below)

https://codereview.chromium.org/2120233003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:528: void
WebURLResponse::assign(WebURLResponsePrivate* p)
At this point I don't feel having this method's very helpful.  In this class's
assignment operator calling dispose() and reallocating WebURLResponsePrivateImpl
isn't probably really necessary either.

I feel WebURLResponsePrivate doesn't need to have m_resourceResponse in its base
class, it's very confusing actually.  It can just expose pure virtual setter and
getter methods and WebURLResponse::operator= and WrappedResourceResponse::bind
can directly call the setter?

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

https://codereview.chromium.org/2120233003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WrappedResourceResponse.h:40: class
WrappedResourceResponse : public WebURLResponse {
How this one's copy assignment works?

https://codereview.chromium.org/2120233003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WrappedResourceResponse.h:40: class
WrappedResourceResponse : public WebURLResponse {
Hmm ok, WebURLResponsePrivate is non-copyable so this one's non-copyable too...
could we explicitly mark this WTF_MAKE_NONCOPYABLE too to avoid confusion?

https://codereview.chromium.org/2120233003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WrappedResourceResponse.h:44:
WrappedResourceResponse() { }
Doesn't it cost us creating a WebURLRespnsePrivateImpl, which is going to be
disposed soon when it gets bound?

: WebURLResponse(&m_handle) ?

[kinuko, 2016-07-05 15:42:11]

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

https://codereview.chromium.org/2120233003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WrappedResourceResponse.h:72: void
dispose() override { m_resourceResponse = 0; }
I don't feel I like this trick in general... I prefer more straightforward,
possibly more stupid solution like having a dumb boolean field in WebURLResponse
and let it delete if it's true.

[kinuko, 2016-07-05 15:53:14]

I've tried revising the patch to see how it'd look if we address comments I
made.  I wouldn't say it's definitely better but at least feels a little more
readable to me.  Wdyt? 
(It's still running on trybots so let's see)

https://codereview.chromium.org/2117313002/

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

I have responded on kinuko's version of the patch. I will resolve the rest of
the comments if we decide to move back to this version.

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

https://codereview.chromium.org/2120233003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/exported/WrappedResourceResponse.h:44:
WrappedResourceResponse() { }
On 2016/07/05 14:59:44, kinuko wrote:
> Doesn't it cost us creating a WebURLRespnsePrivateImpl, which is going to be
> disposed soon when it gets bound?
> 
> : WebURLResponse(&m_handle) ?

I don't think it makes sense to have a default constructor on this class at all,
but unfortunately it is used by WebDataSourceImpl. I considered suboptimal
performance to be acceptable in that one case.

[Adam Rice, 2016-07-08 11:36:13]

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
==========

to

==========
[OBSOLETE] 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
==========