Apply OwnPtr|PassOwnPtr to handle member variables and arguments in MIDIClient

Apply OwnPtr|PassOwnPtr to handle member variables and arguments in MIDIClient

As r170438 and r170576, WebViewImpl has used m_midiClientProxy unnecessarily in order to pass it to provideMIDITo().
To remove it, this cl changes the parameter type to OwnPtr|PassOwnPtr in MIDIClient and MIDIControler
classes.

BUG=N/A

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170746

[gyuyoung-inactive, 2014-04-01 23:02:06]

Add haraken and jochen to reviewer

[haraken, 2014-04-02 01:08:53]

https://codereview.chromium.org/220333003/diff/1/Source/web/MIDIClientProxy.cpp
File Source/web/MIDIClientProxy.cpp (right):

https://codereview.chromium.org/220333003/diff/1/Source/web/MIDIClientProxy.c...
Source/web/MIDIClientProxy.cpp:54: if (m_client)

Is it possible that m_client is 0?

https://codereview.chromium.org/220333003/diff/1/Source/web/MIDIClientProxy.c...
Source/web/MIDIClientProxy.cpp:62: if (m_client)

Ditto.

[gyuyoung-inactive, 2014-04-02 01:17:35]

https://codereview.chromium.org/220333003/diff/1/Source/web/MIDIClientProxy.cpp
File Source/web/MIDIClientProxy.cpp (right):

https://codereview.chromium.org/220333003/diff/1/Source/web/MIDIClientProxy.c...
Source/web/MIDIClientProxy.cpp:54: if (m_client)
On 2014/04/02 01:08:54, haraken wrote:
> 
> Is it possible that m_client is 0?

Yes, client parameter of WebViewImpl's ctor can be 0. So, if the client is null,
WebViewImpl has passed 0 to MIDIClientProxy. That's why MIDIClientProxy's
m_client can be 0.

- m_midiClientProxy(adoptPtr(new MIDIClientProxy(client ?
client->webMIDIClient() : 0)))
+ provideMIDITo(*m_page, MIDIClientProxy::create(client ?
client->webMIDIClient() : 0));

[haraken, 2014-04-02 01:20:07]

LGTM.

> > Is it possible that m_client is 0?
> 
> Yes, client parameter of WebViewImpl's ctor can be 0. So, if the client is
null,
> WebViewImpl has passed 0 to MIDIClientProxy. That's why MIDIClientProxy's
> m_client can be 0.
> 
> - m_midiClientProxy(adoptPtr(new MIDIClientProxy(client ?
> client->webMIDIClient() : 0)))
> + provideMIDITo(*m_page, MIDIClientProxy::create(client ?
> client->webMIDIClient() : 0));

ah, yes. Sorry I was mis-reading the code :)

[gyuyoung-inactive, 2014-04-02 01:37:51]

On 2014/04/02 01:20:07, haraken wrote:
> LGTM.
> 
> > > Is it possible that m_client is 0?
> > 
> > Yes, client parameter of WebViewImpl's ctor can be 0. So, if the client is
> null,
> > WebViewImpl has passed 0 to MIDIClientProxy. That's why MIDIClientProxy's
> > m_client can be 0.
> > 
> > - m_midiClientProxy(adoptPtr(new MIDIClientProxy(client ?
> > client->webMIDIClient() : 0)))
> > + provideMIDITo(*m_page, MIDIClientProxy::create(client ?
> > client->webMIDIClient() : 0));
> 
> ah, yes. Sorry I was mis-reading the code :)

But, I wonder when WebViewImpl's ctor has null in
WebViewImpl::WebViewImpl(WebViewClient* client). If it is impossible, it looks
we can remove the null check conditions for client.
Let me investigate it further.

[haraken, 2014-04-02 01:49:44]

On 2014/04/02 01:37:51, gyuyoung wrote:
> On 2014/04/02 01:20:07, haraken wrote:
> > LGTM.
> > 
> > > > Is it possible that m_client is 0?
> > > 
> > > Yes, client parameter of WebViewImpl's ctor can be 0. So, if the client is
> > null,
> > > WebViewImpl has passed 0 to MIDIClientProxy. That's why MIDIClientProxy's
> > > m_client can be 0.
> > > 
> > > - m_midiClientProxy(adoptPtr(new MIDIClientProxy(client ?
> > > client->webMIDIClient() : 0)))
> > > + provideMIDITo(*m_page, MIDIClientProxy::create(client ?
> > > client->webMIDIClient() : 0));
> > 
> > ah, yes. Sorry I was mis-reading the code :)
> 
> But, I wonder when WebViewImpl's ctor has null in
> WebViewImpl::WebViewImpl(WebViewClient* client). If it is impossible, it looks
> we can remove the null check conditions for client.
> Let me investigate it further.

However, as far as I see other constructors in
WebViewImpl::WebViewImpl(WebViewClient* client), they are assuming that client
can be 0.

[gyuyoung-inactive, 2014-04-02 01:53:05]

On 2014/04/02 01:49:44, haraken wrote:
> On 2014/04/02 01:37:51, gyuyoung wrote:
> > On 2014/04/02 01:20:07, haraken wrote:
> > > LGTM.
> > > 
> > > > > Is it possible that m_client is 0?
> > > > 
> > > > Yes, client parameter of WebViewImpl's ctor can be 0. So, if the client
is
> > > null,
> > > > WebViewImpl has passed 0 to MIDIClientProxy. That's why
MIDIClientProxy's
> > > > m_client can be 0.
> > > > 
> > > > - m_midiClientProxy(adoptPtr(new MIDIClientProxy(client ?
> > > > client->webMIDIClient() : 0)))
> > > > + provideMIDITo(*m_page, MIDIClientProxy::create(client ?
> > > > client->webMIDIClient() : 0));
> > > 
> > > ah, yes. Sorry I was mis-reading the code :)
> > 
> > But, I wonder when WebViewImpl's ctor has null in
> > WebViewImpl::WebViewImpl(WebViewClient* client). If it is impossible, it
looks
> > we can remove the null check conditions for client.
> > Let me investigate it further.
> 
> However, as far as I see other constructors in
> WebViewImpl::WebViewImpl(WebViewClient* client), they are assuming that client
> can be 0.

Yes, I wanna check if the client is really able to be 0 when I have time. :)

[jochen (gone - plz use gerrit), 2014-04-02 09:28:59]

lgtm with nit

https://codereview.chromium.org/220333003/diff/1/Source/web/MIDIClientProxy.cpp
File Source/web/MIDIClientProxy.cpp (right):

https://codereview.chromium.org/220333003/diff/1/Source/web/MIDIClientProxy.c...
Source/web/MIDIClientProxy.cpp:49: return adoptPtr(new MIDIClientProxy(client));
nit should go into the header

[gyuyoung-inactive, 2014-04-02 09:53:55]

The CQ bit was checked by gyuyoung.kim@samsung.com

[gyuyoung-inactive, 2014-04-02 09:53:59]

The CQ bit was unchecked by gyuyoung.kim@samsung.com

[gyuyoung-inactive, 2014-04-02 10:10:48]

The CQ bit was checked by gyuyoung.kim@samsung.com

[commit-bot: I haz the power, 2014-04-02 10:10:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/220333003/20001

[gyuyoung-inactive, 2014-04-02 10:11:21]

On 2014/04/02 09:28:59, jochen wrote:
> lgtm with nit
> 
>
https://codereview.chromium.org/220333003/diff/1/Source/web/MIDIClientProxy.cpp
> File Source/web/MIDIClientProxy.cpp (right):
> 
>
https://codereview.chromium.org/220333003/diff/1/Source/web/MIDIClientProxy.c...
> Source/web/MIDIClientProxy.cpp:49: return adoptPtr(new
MIDIClientProxy(client));
> nit should go into the header

I move it to the MIDIClientProxy.h. Thanks.

[commit-bot: I haz the power, 2014-04-02 10:26:31]

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

[commit-bot: I haz the power, 2014-04-02 10:26:32]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[gyuyoung-inactive, 2014-04-03 02:25:36]

The CQ bit was checked by gyuyoung.kim@samsung.com

[commit-bot: I haz the power, 2014-04-03 02:25:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/220333003/20001

[commit-bot: I haz the power, 2014-04-03 02:38:54]

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

[commit-bot: I haz the power, 2014-04-03 02:38:55]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[gyuyoung-inactive, 2014-04-03 03:44:21]

The CQ bit was checked by gyuyoung.kim@samsung.com

[commit-bot: I haz the power, 2014-04-03 03:44:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/220333003/40001

[commit-bot: I haz the power, 2014-04-03 03:55:50]

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

[commit-bot: I haz the power, 2014-04-03 03:55:52]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[gyuyoung-inactive, 2014-04-03 06:30:11]

https://codereview.chromium.org/220333003/diff/60001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIController.cpp (right):

https://codereview.chromium.org/220333003/diff/60001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIController.cpp:47: ASSERT(m_client);
I missed that client becomes a null after passing ownership to m_client. So, I
change to check assertion with m_client.

[gyuyoung-inactive, 2014-04-03 06:30:27]

The CQ bit was checked by gyuyoung.kim@samsung.com

[commit-bot: I haz the power, 2014-04-03 06:30:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gyuyoung.kim@samsung.com/220333003/60001

[commit-bot: I haz the power, 2014-04-03 07:37:20]

Change committed as 170746