1343747 - Label runnables in WebSocket

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect, P2)

Description

[Kershaw Chang [:kershaw]]

This should include files below. 
 - netwerk/protocol/websocket/WebSocketEventService.cpp
 - netwerk/protocol/websocket/WebSocketChannelChild.cpp

Comment 1

[Kershaw Chang [:kershaw]]

Attachment: Part1: Label runnables in WebSocketChannelChild

Summary:
 - Make WebSocketChannelChild inherit from NeckoTargetHolder
 - Get a labeled event target from nsContentUtils::GetEventTargetByLoadInfo and use it to dispatch runnables.

Thanks.

Comment 2

[Kershaw Chang [:kershaw]]

Attachment: Part2: Label runnables in WebSocketEventListenerChild

Summary:
 - Make WebSocketEventListenerChild inherit NeckoTargetHolder
 - Initialize mNeckoTarget in WebSocketEventListenerChild's constructor
 - Use window id to get a labeled event target
 - Use labeled event target to dispatch runnables in WebSocketEventService.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8876645 [details] [diff] [review]
Part1: Label runnables in WebSocketChannelChild

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

::: netwerk/protocol/websocket/WebSocketChannelChild.cpp
@@ +505,5 @@
>    }
>  }
>  
> +void
> +WebSocketChannelChild::EnsureNeckoTarget()

according the implementation I'm not sure of the name.  usually "EnsureSomething" methods make sure that the object in demand exists after they exit.  This one more just assigns the target when it happens to exists on load info.  Maybe call it AssignNeckoTarget() or SetupNeckoTarget() ?

@@ +573,5 @@
>  
>      transportProvider = ipcChild;
>    }
>  
> +  // This must have happened before sending constructor message.

// This must be called before... ?

::: netwerk/protocol/websocket/WebSocketChannelChild.h
@@ +71,5 @@
>    bool IsOnTargetThread();
>  
>    void MaybeReleaseIPCObject();
>  
> +  void EnsureNeckoTarget();

add a comment what this is doing!

Comment 4

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> Comment on attachment 8876645 [details] [diff] [review]
> Part1: Label runnables in WebSocketChannelChild
> 
> Review of attachment 8876645 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/websocket/WebSocketChannelChild.cpp
> @@ +505,5 @@
> >    }
> >  }
> >  
> > +void
> > +WebSocketChannelChild::EnsureNeckoTarget()
> 
> according the implementation I'm not sure of the name.  usually
> "EnsureSomething" methods make sure that the object in demand exists after
> they exit.  This one more just assigns the target when it happens to exists
> on load info.  Maybe call it AssignNeckoTarget() or SetupNeckoTarget() ?
> 

I like "SetupNeckoTarget". Thanks!

> @@ +573,5 @@
> >  
> >      transportProvider = ipcChild;
> >    }
> >  
> > +  // This must have happened before sending constructor message.
> 
> // This must be called before... ?
> 

Got it.

> ::: netwerk/protocol/websocket/WebSocketChannelChild.h
> @@ +71,5 @@
> >    bool IsOnTargetThread();
> >  
> >    void MaybeReleaseIPCObject();
> >  
> > +  void EnsureNeckoTarget();
> 
> add a comment what this is doing!

Will do.

Thanks again for the prompt review.

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: Part1: Label runnables in WebSocketChannelChild, r=mayhemer

Rebase and carry r+.

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Part2: Label runnables in WebSocketEventListenerChild, r=mayhemer

Carry r+.

Comment 7

[Carsten Book [:Tomcat]]

has problems to apply:

adding 1343747 to series file
renamed 1343747 -> 0001-Bug-1343747-Part1-Label-runnables-in-WebSocketChanne.patch
applying 0001-Bug-1343747-Part1-Label-runnables-in-WebSocketChanne.patch
patching file netwerk/protocol/websocket/WebSocketChannelChild.cpp
Hunk #6 FAILED at 624
Hunk #9 FAILED at 756
2 out of 9 hunks FAILED -- saving rejects to file netwerk/protocol/websocket/WebSocketChannelChild.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0001-Bug-1343747-Part1-Label-runnables-in-WebSocketChanne.patch

Comment 8

[Kershaw Chang [:kershaw]]

Attachment: Part1: Label runnables in WebSocketChannelChild, r=mayhemer

Rebase again.

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Part2: Label runnables in WebSocketEventListenerChild, r=mayhemer

Rebase

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b96aecbac7b
Part 1: Label runnables in WebSocketChannelChild. r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/5f4a90d6dd5f
Part 2: Label runnables in WebSocketEventListenerChild. r=mayhemer

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b96aecbac7b
https://hg.mozilla.org/mozilla-central/rev/5f4a90d6dd5f

Comment 12

[Gerry Chang [:gchang]]

Mark 54 won't fix as 54 was released.