1164417 - Cleanup |PrepareAccept| in socket I/O code

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

With the changes of bug 1161020, the current code for accepting a socket connection can be simplified. The socket connector of the listen socket shall create a connector for the connection-oriented socket that handles the connection. No need to override |StreamSocket::PrepareAccept|.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7d47d24e441

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1164417: Add |UnixSocketConnector::Duplicate|

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1164417: Store PDU consumer in |BluetoothDaemonConnection|

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1164417: Add |ConnectionOrientedSocket::PrepareAccept| for accepting socket connections

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

This patch set simplifies accepting a socket. In one of the next patch sets, it might become possible to remove the subclasses of |StreamSocket| altogether.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8607562 [details] [diff] [review]
[03] Bug 1164417: Add |ConnectionOrientedSocket::PrepareAccept| for accepting socket connections

Review of attachment 8607562 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits addressed/questions answered.

::: ipc/bluetooth/BluetoothDaemonConnection.cpp
@@ +451,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(!mIO);
>  
> +  nsAutoPtr<UnixSocketConnector> connector(aConnector); // We don't need this

Why is this still here if we don't need it?

@@ +463,2 @@
>  
> +  return NS_OK;;

nit: only one semicolon

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> >  
> > +  nsAutoPtr<UnixSocketConnector> connector(aConnector); // We don't need this
> 
> Why is this still here if we don't need it?

Right, that comment is misleading. It means that the connector is part of the interface, but we currently don't actually need or use it. Since |BluetoothDaemonConnection| owns the pointer, it stores it in an |nsAutoPtr| so it gets deleted.

I'll update the comment to be precise.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1164417: Add |ConnectionOrientedSocket::PrepareAccept| for accepting socket connections (v2)

Changes since v1:

  - clarified comment about connector ownership
  - removed unnecessary semicolon

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/0b9b45499f77
https://hg.mozilla.org/integration/b2g-inbound/rev/13411cc00f09
https://hg.mozilla.org/integration/b2g-inbound/rev/d68065786c99

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks Kyle!

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=d68065786c99

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0b9b45499f77
https://hg.mozilla.org/mozilla-central/rev/13411cc00f09
https://hg.mozilla.org/mozilla-central/rev/d68065786c99