Closed
Bug 1343747
Opened 8 years ago
Closed 7 years ago
Label runnables in WebSocket
Categories
(Core :: Networking: WebSockets, defect, P2)
Core
Networking: WebSockets
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: kershaw, Assigned: kershaw)
References
Details
(Whiteboard: [necko-active][QDL][TDC-MVP][NECKO])
Attachments
(2 files, 4 obsolete files)
9.51 KB,
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
18.06 KB,
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
This should include files below. - netwerk/protocol/websocket/WebSocketEventService.cpp - netwerk/protocol/websocket/WebSocketChannelChild.cpp
Updated•8 years ago
|
Whiteboard: [necko-next] → [necko-next][QDL][TDC-MVP][NECKO]
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → kechang
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
Summary: - Make WebSocketChannelChild inherit from NeckoTargetHolder - Get a labeled event target from nsContentUtils::GetEventTargetByLoadInfo and use it to dispatch runnables. Thanks.
Attachment #8876645 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 2•7 years ago
|
||
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.
Attachment #8876648 -
Flags: review?(honzab.moz)
![]() |
||
Comment 3•7 years ago
|
||
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!
Attachment #8876645 -
Flags: review?(honzab.moz) → review+
![]() |
||
Updated•7 years ago
|
Attachment #8876648 -
Flags: review?(honzab.moz) → review+
Updated•7 years ago
|
Whiteboard: [necko-next][QDL][TDC-MVP][NECKO] → [necko-active][QDL][TDC-MVP][NECKO]
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Assignee | ||
Comment 5•7 years ago
|
||
Rebase and carry r+.
Attachment #8876645 -
Attachment is obsolete: true
Attachment #8876648 -
Attachment is obsolete: true
Attachment #8876974 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 7•7 years ago
|
||
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
Flags: needinfo?(kechang)
Keywords: checkin-needed
Assignee | ||
Comment 8•7 years ago
|
||
Rebase again.
Attachment #8876974 -
Attachment is obsolete: true
Attachment #8876975 -
Attachment is obsolete: true
Flags: needinfo?(kechang)
Attachment #8877393 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 10•7 years ago
|
||
Pushed by ryanvm@gmail.com: 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
Keywords: checkin-needed
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7b96aecbac7b https://hg.mozilla.org/mozilla-central/rev/5f4a90d6dd5f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•