Decouple MIDIAccess initialization from MIDIAccess class.

Decouple MIDIAccess initialization from MIDIAccess class.

Introduce a new class MIDIAccessInitializer which initializes MIDIAccess.
Previously the initialization code were on MIDIAccess itself, but this CL
decouples it.

This CL changes the various initialization-related resource ownerships.
In this CL MIDIAccessInitializer owns all initialization related resources
and when the initialization is done (no matter whether successfull or not)
it releases such resources.

Unfortunately MIDIAccess owns MIDIAccessInitializer in this CL because
there is no other natural owner of MIDIAccessInitializer.
Hopefully MIDIAccess will stop owning MIDIAccessInitializer when [1] lands.

1. https://codereview.chromium.org/311733004/

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

[yhirano, 2014-06-04 02:06:05]

This CL doesn't change the behavior.
This CL is a preparation CL for introducing AsyncInitializerResolver (see also
https://codereview.chromium.org/311733004/).

[Takashi Toyoshima, 2014-06-04 20:52:35]

looking in terms of Web MIDI.
Also add kouhei@ for pointers since my understanding on smart pointers could not
really catch up with Oilpan.
We will need one more review for public/ and maybe tkent@ can review.

[Takashi Toyoshima, 2014-06-04 22:03:53]

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.h (right):

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.h:80: virtual void didAddOutputPort(const
String& id, const String& manufacturer, const String& name, const String&
version) OVERRIDE;
We may want to have a comment here.
Why didStartSession() is not implemented here, e.g. IIUC,
MIDIAccessInitializer's implementation is called?

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.cpp:73: if (enable) {
It is not changed from the original, but parentheses for one liners are not
consistent with others. Can you change this to be consistent?

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.cpp:95:
controller->cancelSysexPermissionRequest(this);
Related to a comment on WebMIDIPermissionRequest.h.
I guess this ensures that MIDIAccessInitializer lives longer than
WebMIDIPermissionRequest, right?

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccessor.h (right):

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessor.h:50: void sendMIDIData(unsigned portIndex,
const unsigned char* data, size_t length, double timeStamp);
Can you add some useful comments here?
It will be useful to say why setClient() is needed, and when MIDIAccessorClient
is replaced.

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIClient.h (left):

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIClient.h:43: virtual void
requestSysexPermission(PassRefPtrWillBeRawPtr<MIDIAccess>) = 0;
Note: I just didn't understand why we can change it from PassRefPtrWillBeRwPtr<>
to raw pointer.

https://codereview.chromium.org/311773003/diff/40001/public/web/WebMIDIPermis...
File public/web/WebMIDIPermissionRequest.h (right):

https://codereview.chromium.org/311773003/diff/40001/public/web/WebMIDIPermis...
public/web/WebMIDIPermissionRequest.h:65: WebCore::MIDIAccessInitializer*
m_initializer;
Can we ensure that m_initializer continues to be valid while
WebMIDIPermissionRequest is alive?
It will be related to my question on PassRefPtrWillBeRawPtr<>.

[kouhei (in TOK), 2014-06-05 01:19:44]

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.h (right):

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.h:80: virtual void didAddOutputPort(const
String& id, const String& manufacturer, const String& name, const String&
version) OVERRIDE;
On 2014/06/04 22:03:53, Takashi Toyoshima (chromium) wrote:
> We may want to have a comment here.
> Why didStartSession() is not implemented here, e.g. IIUC,
> MIDIAccessInitializer's implementation is called?

Can we have this as { ASSERT_NOT_REACHED(); }

[yhirano, 2014-06-05 11:35:25]

Updated issue description.

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.h (right):

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.h:80: virtual void didAddOutputPort(const
String& id, const String& manufacturer, const String& name, const String&
version) OVERRIDE;
On 2014/06/05 01:19:44, kouhei wrote:
> On 2014/06/04 22:03:53, Takashi Toyoshima (chromium) wrote:
> > We may want to have a comment here.
> > Why didStartSession() is not implemented here, e.g. IIUC,
> > MIDIAccessInitializer's implementation is called?
> 
> Can we have this as { ASSERT_NOT_REACHED(); }

Done.

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.h:80: virtual void didAddOutputPort(const
String& id, const String& manufacturer, const String& name, const String&
version) OVERRIDE;
On 2014/06/04 22:03:53, Takashi Toyoshima (chromium) wrote:
> We may want to have a comment here.
> Why didStartSession() is not implemented here, e.g. IIUC,
> MIDIAccessInitializer's implementation is called?

Done.

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.cpp:73: if (enable) {
On 2014/06/04 22:03:53, Takashi Toyoshima (chromium) wrote:
> It is not changed from the original, but parentheses for one liners are not
> consistent with others. Can you change this to be consistent?

Done.

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.cpp:95:
controller->cancelSysexPermissionRequest(this);
On 2014/06/04 22:03:53, Takashi Toyoshima (chromium) wrote:
> Related to a comment on WebMIDIPermissionRequest.h.
> I guess this ensures that MIDIAccessInitializer lives longer than
> WebMIDIPermissionRequest, right?

Yes, request must not outlive the initializer and calling cancel ensures it.

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccessor.h (right):

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessor.h:50: void sendMIDIData(unsigned portIndex,
const unsigned char* data, size_t length, double timeStamp);
On 2014/06/04 22:03:53, Takashi Toyoshima (chromium) wrote:
> Can you add some useful comments here?
> It will be useful to say why setClient() is needed, and when
MIDIAccessorClient
> is replaced.

Done.

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIClient.h (left):

https://codereview.chromium.org/311773003/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIClient.h:43: virtual void
requestSysexPermission(PassRefPtrWillBeRawPtr<MIDIAccess>) = 0;
On 2014/06/04 22:03:53, Takashi Toyoshima (chromium) wrote:
> Note: I just didn't understand why we can change it from
PassRefPtrWillBeRwPtr<>
> to raw pointer.

I would like MIDIInitializer to own all initialization-related resouces.
It means that the request will be canceled when MIDIAccessInitializer becomes
invalid and so I think it is OK to pass a raw pointer.

https://codereview.chromium.org/311773003/diff/40001/public/web/WebMIDIPermis...
File public/web/WebMIDIPermissionRequest.h (right):

https://codereview.chromium.org/311773003/diff/40001/public/web/WebMIDIPermis...
public/web/WebMIDIPermissionRequest.h:65: WebCore::MIDIAccessInitializer*
m_initializer;
On 2014/06/04 22:03:53, Takashi Toyoshima (chromium) wrote:
> Can we ensure that m_initializer continues to be valid while
> WebMIDIPermissionRequest is alive?
> It will be related to my question on PassRefPtrWillBeRawPtr<>.

I updated the class comment: is it clear now?

[Takashi Toyoshima, 2014-06-09 05:31:57]

lgtm.

[yhirano, 2014-06-10 02:29:27]

kouhei, do you have any other comments?

[kouhei (in TOK), 2014-06-10 03:43:32]

On 2014/06/10 02:29:27, yhirano wrote:
> kouhei, do you have any other comments?

lgtm

[yhirano, 2014-06-10 03:52:00]

+tkent for Source/web and public/web

[tkent, 2014-06-10 04:28:32]

https://codereview.chromium.org/311773003/diff/40001/public/web/WebMIDIPermis...
File public/web/WebMIDIPermissionRequest.h (right):

https://codereview.chromium.org/311773003/diff/40001/public/web/WebMIDIPermis...
public/web/WebMIDIPermissionRequest.h:65: WebCore::MIDIAccessInitializer*
m_initializer;
On 2014/06/05 11:35:25, yhirano wrote:
> On 2014/06/04 22:03:53, Takashi Toyoshima (chromium) wrote:
> > Can we ensure that m_initializer continues to be valid while
> > WebMIDIPermissionRequest is alive?
> > It will be related to my question on PassRefPtrWillBeRawPtr<>.
> 
> I updated the class comment: is it clear now?

I don't understand how it is ensured.
This looks very easy to cause use-after-free. No one clears m_initializer and
WebMIDIPermissionRequest is copyable.

[yhirano, 2014-06-10 07:20:27]

On 2014/06/10 04:28:32, tkent wrote:
>
https://codereview.chromium.org/311773003/diff/40001/public/web/WebMIDIPermis...
> File public/web/WebMIDIPermissionRequest.h (right):
> 
>
https://codereview.chromium.org/311773003/diff/40001/public/web/WebMIDIPermis...
> public/web/WebMIDIPermissionRequest.h:65: WebCore::MIDIAccessInitializer*
> m_initializer;
> On 2014/06/05 11:35:25, yhirano wrote:
> > On 2014/06/04 22:03:53, Takashi Toyoshima (chromium) wrote:
> > > Can we ensure that m_initializer continues to be valid while
> > > WebMIDIPermissionRequest is alive?
> > > It will be related to my question on PassRefPtrWillBeRawPtr<>.
> > 
> > I updated the class comment: is it clear now?
> 
> I don't understand how it is ensured.
> This looks very easy to cause use-after-free. No one clears m_initializer and
> WebMIDIPermissionRequest is copyable.

A MIDIAccessInitializer is responsible for cancelling the pending request when
it stops the initialization.
A WebMIDIClient is responsible for not accessing cancelled requests.

The WebMIDIPermissionRequest copy constructor is only used in MIDIDispatcher
[1].
Should I rename it as clone() and make WebMIDIPermissionRequest noncopyable?

1:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

[tkent, 2014-06-10 07:40:33]

On 2014/06/10 07:20:27, yhirano wrote:
> A WebMIDIClient is responsible for not accessing cancelled requests.

As a public API, this rule must be documented in a code comment.  The class
comment of WebMIDIPermissionRequest looks an implementation note. 
WebMIDIClient::cancelSysexPermissionRequest() needs it?

If I were you, I would also make MIDIAccessInitializer RefCounted<> and
WebMIDIPermissionRequest::m_private WebPrivatePtr<MIDIAccessInitializer>.  It
makes the code solid.  It's up to you.

[yhirano, 2014-06-10 08:17:07]

> As a public API, this rule must be documented in a code comment.  The class
> comment of WebMIDIPermissionRequest looks an implementation note. 
> WebMIDIClient::cancelSysexPermissionRequest() needs it?
Added comments to WebMIDIClient.h.

> If I were you, I would also make MIDIAccessInitializer RefCounted<> and
> WebMIDIPermissionRequest::m_private WebPrivatePtr<MIDIAccessInitializer>.  It
> makes the code solid.  It's up to you.
I would like to create OwnPtr<MIDIAccessInitializer> in the next CL, so I
would leave it be.

[tkent, 2014-06-10 08:33:32]

lgtm

[yhirano, 2014-06-11 08:44:09]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-06-11 08:44:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yhirano@chromium.org/311773003/100001

[commit-bot: I haz the power, 2014-06-11 08:44:45]

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

[commit-bot: I haz the power, 2014-06-11 08:44:46]

Failed to apply patch for Source/modules/webmidi/MIDIController.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/webmidi/MIDIController.h
  Hunk #1 FAILED at 36.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/modules/webmidi/MIDIController.h.rej

Patch:       Source/modules/webmidi/MIDIController.h
Index: Source/modules/webmidi/MIDIController.h
diff --git a/Source/modules/webmidi/MIDIController.h
b/Source/modules/webmidi/MIDIController.h
index
1535534bad57c285ba7dfaa7e10e6b88c0fa6e17..261f488e9a157a2efa4efe9e4fedcb520b3a9192
100644
--- a/Source/modules/webmidi/MIDIController.h
+++ b/Source/modules/webmidi/MIDIController.h
@@ -36,15 +36,15 @@
 
 namespace WebCore {
 
-class MIDIAccess;
+class MIDIAccessInitializer;
 class MIDIClient;
 
 class MIDIController FINAL : public Supplement<LocalFrame> {
 public:
     virtual ~MIDIController();
 
-    void requestSysexPermission(PassRefPtrWillBeRawPtr<MIDIAccess>);
-    void cancelSysexPermissionRequest(MIDIAccess*);
+    void requestSysexPermission(MIDIAccessInitializer*);
+    void cancelSysexPermissionRequest(MIDIAccessInitializer*);
 
     static PassOwnPtr<MIDIController> create(PassOwnPtr<MIDIClient>);
     static const char* supplementName();

[yhirano, 2014-06-11 08:54:29]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-06-11 08:54:42]

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

[commit-bot: I haz the power, 2014-06-11 10:02:13]

Change committed as 175952