Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Description • 8 years ago

https://wiki.mozilla.org/WebAPI/PresentationAPI:Protocol_Draft

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 1 • 8 years ago

change mdns service name to "_presentation-ctrl._tcp" and add "version" field in service record.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 2 • 8 years ago

move the protocol handling out of control channel.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 6 • 8 years ago

rebased to m-c

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 7 • 8 years ago

rebased to m-c

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 8 • 8 years ago

rebase to m-c

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 9 • 8 years ago

Will further split the patches into review-able parts, or even bugs tomorrow.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 10 • 8 years ago

introduce new MDNS service type and version number concept

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 11 • 8 years ago

implement connect and launch command

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 12 • 8 years ago

implement terminate command

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 13 • 8 years ago

add legacy device provider for TV 2.5 backward compatibility

	Junior [inactive]
	Comment 14 • 8 years ago

Comment on attachment 8765863 [details] [diff] [review]
part 1, broadcast-protcol-version.patch

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

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +349,5 @@
> +
> +  uint32_t localVersion;
> +  Unused << NS_WARN_IF(NS_FAILED(mPresentationService->GetVersion(&localVersion)));
> +
> +  return localVersion == remoteVersion;

Is the service compatible only if the versions are the same? Or maybe backward compatible?

	Junior [inactive]
	Comment 15 • 8 years ago

Comment on attachment 8765864 [details] [diff] [review]
part 2, start-session-procedure.patch

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

::: dom/presentation/provider/ControllerStateMachine.jsm
@@ +15,5 @@
> +/* globals State, CommandType */
> +Cu.import("resource://gre/modules/presentation/StateMachineHelper.jsm");
> +
> +const DEBUG = true;
> +function debug (str) {

nit: debug(str) {

::: dom/presentation/provider/PresentationControlService.js
@@ +119,2 @@
>      if (!this.id) {
> +      DEBUG && log("PresentationControlService - Id has not initialized; requestSession fails"); //jshint ignore:line

Lots of noise for jshint here and there 
It's fine with the new-add code, but it distracts the review process for code with only adding ignore message.

If you'd like to land those jshint symbol, maybe we need to sort out these code to another patch.

@@ +353,5 @@
>                       .openOutputStream(Ci.nsITransport.OPEN_UNBUFFERED, 0, 0);
>  
> +  this._stateMachine =
> +    (direction === "sender") ? new ControllerStateMachine(this, presentationService.id)
> +                             : new ReceiverStateMachine(this);

Is there a circular reference?

::: dom/presentation/provider/ReceiverStateMachine.jsm
@@ +116,5 @@
> +    handlers[this.state](this, command);
> +  },
> +
> +  onChannelReady: function _onChannelReady() {
> +    this.state = State.CONNECTING;

Any precondition?

@@ +142,5 @@
> +            reason = this._closeReason;
> +            delete this._closeReason;
> +          }
> +          this._notifyClosed(reason);
> +        }

How about the else case?

	Junior [inactive]
	Comment 16 • 8 years ago

Comment on attachment 8765866 [details] [diff] [review]
part 4, support-legacy-ctrl-protocol.patch

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

Originally I hope we can move logic to stateMachine.
However it seems not easy per offline discussion.

But it's fine. These should be removed eventually.

I'm not familiar with MDNS code, maybe we need another eye for feedback.

::: dom/presentation/provider/LegacyPresentationControlService.js
@@ +467,5 @@
> +      this._connected = false;
> +    }
> +  },
> +
> +  classID: Components.ID("{fefb8286-0bdc-488b-98bf-0c11b485c955}"),

I guess the id should be changed

	Junior [inactive]
	Comment 17 • 8 years ago

Comment on attachment 8765865 [details] [diff] [review]
part 3, support-terminate-command.patch

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

Sorry not able to finish feedback this week. Will continue on this.

::: dom/presentation/provider/PresentationControlService.js
@@ +396,5 @@
>  
> +  terminate: function(aPresentationId) {
> +    this._stateMachine.terminate(aPresentationId);
> +  },
> +

Where is the caller? I guess it should be control by SessionInfo.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 18 • 8 years ago

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

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 19 • 8 years ago

Comment on attachment 8765863 [details] [diff] [review]
part 1, broadcast-protcol-version.patch

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

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +349,5 @@
> +
> +  uint32_t localVersion;
> +  Unused << NS_WARN_IF(NS_FAILED(mPresentationService->GetVersion(&localVersion)));
> +
> +  return localVersion == remoteVersion;

How about move this compatibility check into PresentationControlService? There might be more complex check every time we update the protocol.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 20 • 8 years ago

Comment on attachment 8765864 [details] [diff] [review]
part 2, start-session-procedure.patch

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

::: dom/presentation/provider/ControllerStateMachine.jsm
@@ +15,5 @@
> +/* globals State, CommandType */
> +Cu.import("resource://gre/modules/presentation/StateMachineHelper.jsm");
> +
> +const DEBUG = true;
> +function debug (str) {

done.

::: dom/presentation/provider/PresentationControlService.js
@@ +119,2 @@
>      if (!this.id) {
> +      DEBUG && log("PresentationControlService - Id has not initialized; requestSession fails"); //jshint ignore:line

sure I can separate jshint code in part-0 patch.

@@ +353,5 @@
>                       .openOutputStream(Ci.nsITransport.OPEN_UNBUFFERED, 0, 0);
>  
> +  this._stateMachine =
> +    (direction === "sender") ? new ControllerStateMachine(this, presentationService.id)
> +                             : new ReceiverStateMachine(this);

circular reference in between pure JS objects is fine IIRC.

::: dom/presentation/provider/ReceiverStateMachine.jsm
@@ +116,5 @@
> +    handlers[this.state](this, command);
> +  },
> +
> +  onChannelReady: function _onChannelReady() {
> +    this.state = State.CONNECTING;

Add |if (this.state === State.INIT)| as precondition.

@@ +142,5 @@
> +            reason = this._closeReason;
> +            delete this._closeReason;
> +          }
> +          this._notifyClosed(reason);
> +        }

In closing state, it means we have triggered close from local peer. Local channel close will do nothing.
Or do you have second thoughts?

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 21 • 8 years ago

Comment on attachment 8765865 [details] [diff] [review]
part 3, support-terminate-command.patch

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

::: dom/presentation/provider/PresentationControlService.js
@@ +396,5 @@
>  
> +  terminate: function(aPresentationId) {
> +    this._stateMachine.terminate(aPresentationId);
> +  },
> +

yes by session info object, see patch on bug 1276378.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 22 • 8 years ago

Comment on attachment 8765866 [details] [diff] [review]
part 4, support-legacy-ctrl-protocol.patch

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

::: dom/presentation/provider/LegacyPresentationControlService.js
@@ +467,5 @@
> +      this._connected = false;
> +    }
> +  },
> +
> +  classID: Components.ID("{fefb8286-0bdc-488b-98bf-0c11b485c955}"),

done.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 23 • 8 years ago

@junior you might want to check my response between comment #19 ~ #22.

	Junior [inactive]
	Comment 24 • 8 years ago

> ::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
> @@ +349,5 @@
> > +
> > +  uint32_t localVersion;
> > +  Unused << NS_WARN_IF(NS_FAILED(mPresentationService->GetVersion(&localVersion)));
> > +
> > +  return localVersion == remoteVersion;
> 
> How about move this compatibility check into PresentationControlService?
> There might be more complex check every time we update the protocol.

That's up to you. My point is to consider the precise compatible situation.

> @@ +142,5 @@
> > +            reason = this._closeReason;
> > +            delete this._closeReason;
> > +          }
> > +          this._notifyClosed(reason);
> > +        }
> 
> In closing state, it means we have triggered close from local peer. Local
> channel close will do nothing.
> Or do you have second thoughts?

Just confirm you have checked this condition.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 25 • 8 years ago

(In reply to Junior [:junior] from comment #24)
> > ::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
> > @@ +349,5 @@
> > > +
> > > +  uint32_t localVersion;
> > > +  Unused << NS_WARN_IF(NS_FAILED(mPresentationService->GetVersion(&localVersion)));
> > > +
> > > +  return localVersion == remoteVersion;
> > 
> > How about move this compatibility check into PresentationControlService?
> > There might be more complex check every time we update the protocol.
> 
> That's up to you. My point is to consider the precise compatible situation.
Every time when updating protocol implementation, we'll need to rethink the compatibility check algorithm.
Since this is the first version of this protocol, exactly match of version number seems reasonable.

	Junior [inactive]
	Comment 26 • 8 years ago

Comment on attachment 8765865 [details] [diff] [review]
part 3, support-terminate-command.patch

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

::: dom/presentation/interfaces/nsIPresentationTerminateRequest.idl
@@ +24,5 @@
> +  // The Id for representing this session.
> +  readonly attribute DOMString presentationId;
> +
> +  // The control channel for this session.
> +  readonly attribute nsIPresentationControlChannel controlChannel;

I guess the control channel is supposed to limited use.
Could we use all the APIs of controlChannel? Annotate it.

::: dom/presentation/provider/ControllerStateMachine.jsm
@@ +50,5 @@
>        case CommandType.LAUNCH_ACK:
>          stateMachine._notifyLaunch(command.presentationId);
>          break;
> +      case CommandType.TERMINATE_ACK:
> +        stateMachine._notifyTerminate(command.presentationId);

This call ends with |ControlService.onSessionTerminate| but the annotation there says "Callback while the remote host is requesting to terminate a presentation session."

Kinda semantically weird

::: dom/presentation/tests/xpcshell/test_tcp_control_channel.js
@@ +185,5 @@
> +  let yayFuncs = makeJointSuccess(['controllerControlChannelClose',
> +                                   'presenterControlChannelClose']);
> +  let controllerControlChannel;
> +
> +  tps.listener = {

tps should be renamed to pcs or something else.
Modify here or file a good first bug.

@@ +217,5 @@
> +
> +  presenterControlChannel.listener = {
> +    notifyOpened: function() {
> +      presenterControlChannel.terminate('testPresentationId', 'http://example.com');
> +      presenterControlChannel.close(CLOSE_CONTROL_CHANNEL_REASON);

The setup is kinda complicated and I can not easily tell the difference between |terminate| and |close|. Maybe we should mention something in |PresentationControlChannel.idl|:
1. the control channel is still alive after terminate
2. the relationship between |resume| and |terminate|
3. what API should be trigger after |terminate|? It seems |ControlService.onTerminateRequest|.

Should we always call a |terminate| before |close|?
Annotate clearly since it tends to confuse us when we have |terminate| and |close|.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 27 • 8 years ago

Comment on attachment 8765865 [details] [diff] [review]
part 3, support-terminate-command.patch

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

::: dom/presentation/interfaces/nsIPresentationTerminateRequest.idl
@@ +24,5 @@
> +  // The Id for representing this session.
> +  readonly attribute DOMString presentationId;
> +
> +  // The control channel for this session.
> +  readonly attribute nsIPresentationControlChannel controlChannel;

done.

::: dom/presentation/provider/ControllerStateMachine.jsm
@@ +50,5 @@
>        case CommandType.LAUNCH_ACK:
>          stateMachine._notifyLaunch(command.presentationId);
>          break;
> +      case CommandType.TERMINATE_ACK:
> +        stateMachine._notifyTerminate(command.presentationId);

termination procedure doesn't really need "ack", however I tried to make the protocol looks more symmetric. I'll rethink about this part.

::: dom/presentation/tests/xpcshell/test_tcp_control_channel.js
@@ +185,5 @@
> +  let yayFuncs = makeJointSuccess(['controllerControlChannelClose',
> +                                   'presenterControlChannelClose']);
> +  let controllerControlChannel;
> +
> +  tps.listener = {

done.

@@ +217,5 @@
> +
> +  presenterControlChannel.listener = {
> +    notifyOpened: function() {
> +      presenterControlChannel.terminate('testPresentationId', 'http://example.com');
> +      presenterControlChannel.close(CLOSE_CONTROL_CHANNEL_REASON);

"terminate" means trigger/continue session termination procedure on this control channel and "close" means close control channel.

So, we'll always do "close" after "terminate".

semantically, "launch" and "terminate" will be a pair for controlling start/stop a presentation session.

"resume" and close transport channel will be a pair for controlling the lifecycle of a transport channel within a session.

do you think rename "close" to "disconnect" will make more sense? since I'm going to rename nsIPresentationControlService.requestSession to nsIPresentationControlService.connect.

	Junior [inactive]
	Comment 28 • 8 years ago

> do you think rename "close" to "disconnect" will make more sense? since I'm
> going to rename nsIPresentationControlService.requestSession to
> nsIPresentationControlService.connect.

That will be lovely.

	Gary Chen [:xeonchen]
	Comment 29 • 8 years ago

Comment on attachment 8765866 [details] [diff] [review]
part 4, support-legacy-ctrl-protocol.patch

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

Overall looks good :)

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 30 • 8 years ago

I decided to move patch for terminate command to bug 1276378 so that we can have a clear view on how to implement a new session control API.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 32 • 8 years ago

move compatibility check to PresentationControlService.js.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 33 • 8 years ago

update according to review comment, rebase to latest m-c.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 34 • 8 years ago

update according to review comment, rebase to latest m-c.

	Junior [inactive]
	Comment 36 • 8 years ago

Comment on attachment 8768637 [details] [diff] [review]
part 0, jshint.patch

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

::: dom/presentation/provider/PresentationControlService.js
@@ +498,4 @@
>    },
>  
>    // nsIStreamListener (Triggered by nsIInputStreamPump.asyncRead)
> +  onDataAvailable: function(aRequest, aContext, aInputStream) {

Why need this change?

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 37 • 8 years ago

Comment on attachment 8768637 [details] [diff] [review]
part 0, jshint.patch

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

::: dom/presentation/provider/PresentationControlService.js
@@ +498,4 @@
>    },
>  
>    // nsIStreamListener (Triggered by nsIInputStreamPump.asyncRead)
> +  onDataAvailable: function(aRequest, aContext, aInputStream) {

unused parameter (aOffset / aCount)

	Junior [inactive]
	Comment 38 • 8 years ago

Comment on attachment 8768638 [details] [diff] [review]
part 1, broadcast-protcol-version.patch

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

::: dom/presentation/provider/MulticastDNSDeviceProvider.cpp
@@ +256,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv = serviceInfo->SetPort(servicePort)))) {
>      return rv;
>    }
>  
> +  nsCOMPtr<nsIWritablePropertyBag2> propbag =

It's up to you, but I prefer |propBag| which is easy to recognize.

@@ +333,5 @@
> +MulticastDNSDeviceProvider::IsCompatibleServer(nsIDNSServiceInfo* aServiceInfo)
> +{
> +  MOZ_ASSERT(aServiceInfo);
> +
> +  nsCOMPtr<nsIPropertyBag2> propbag;

same here

::: dom/presentation/tests/xpcshell/test_multicast_dns_device_provider.js
@@ +23,5 @@
> +const latestVersion = 1;
> +const serviceType = "_presentation-ctrl._tcp";
> +const versionAttr = Cc["@mozilla.org/hash-property-bag;1"]
> +                      .createInstance(Ci.nsIWritablePropertyBag2);
> +versionAttr.setPropertyAsUint32("version", latestVersion);

nit: naming should be aligned like LATEST_VERSION

	Junior [inactive]
	Comment 39 • 8 years ago

Comment on attachment 8768640 [details] [diff] [review]
part 2, start-session-procedure.patch

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

::: dom/presentation/interfaces/nsIPresentationControlChannel.idl
@@ +113,5 @@
> +  /*
> +   * Disconnect the control channel.
> +   * @param reason The reason of disconnecting channel; NS_OK represents normal.
> +   */
> +  void disconnect(in nsresult reason);

I'm thinking about changing |notifyClosed| to |notifyDisconnect|. Also |notifyOpened| to |notifyConnect|.

How do you think?

::: dom/presentation/provider/ReceiverStateMachine.jsm
@@ +7,5 @@
> +/* globals Components, dump */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["ReceiverStateMachine"]; //jshint ignore:line

nit: space between "//" and comment. Also in patch 0 and elsewhere
Sorry for picking up this so late.

@@ +15,5 @@
> +/* globals State, CommandType */
> +Cu.import("resource://gre/modules/presentation/StateMachineHelper.jsm");
> +
> +const DEBUG = true;
> +function debug (str) {

nit: no space between debug and (str)
and any plan to let DEBUG = false?

@@ +39,5 @@
> +        stateMachine._notifyClosed(command.reason);
> +        break;
> +      default:
> +        debug("unexpected command: " + JSON.stringify(command));
> +        // ignore

nit: the annotation is incomplete

@@ +63,5 @@
> +        stateMachine._notifyChannelDescriptor(command);
> +        break;
> +      default:
> +        debug("unexpected command: " + JSON.stringify(command));
> +        // ignore

same here and also those in controlling side

@@ +149,5 @@
> +          // do nothing and wait for remote channel closed.
> +        }
> +        break;
> +      default:
> +        DEBUG && debug("unexpected channel close: " + reason + ", " + isByRemote); //jshint ignore:line

I found that you're not always using "DEBUG && debug(...)" pattern.
Any policy?

::: dom/presentation/tests/mochitest/PresentationSessionChromeScript.js
@@ +407,5 @@
>  addMessageListener('trigger-control-channel-open', function(reason) {
>    mockedControlChannel.simulateNotifyOpened();
>  });
>  
>  addMessageListener('trigger-control-channel-close', function(reason) {

This event name should be changed.
We have lots open/close/connect/disconnect.
Any policy for the naming?

	Junior [inactive]
	Comment 40 • 8 years ago

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #34)
> Created attachment 8768641 [details] [diff] [review]
> part 3, support-legacy-ctrl-protocol.patch
> 
> update according to review comment, rebase to latest m-c.

Is the |terminate| syntax out of this bug?

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 41 • 8 years ago

(In reply to Junior [:junior] from comment #40)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #34)
> > Created attachment 8768641 [details] [diff] [review]
> > part 3, support-legacy-ctrl-protocol.patch
> > 
> > update according to review comment, rebase to latest m-c.
> 
> Is the |terminate| syntax out of this bug?
Yes, all the terminate-related changes are moved to another bug.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 42 • 8 years ago

Comment on attachment 8768640 [details] [diff] [review]
part 2, start-session-procedure.patch

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

::: dom/presentation/provider/ReceiverStateMachine.jsm
@@ +149,5 @@
> +          // do nothing and wait for remote channel closed.
> +        }
> +        break;
> +      default:
> +        DEBUG && debug("unexpected channel close: " + reason + ", " + isByRemote); //jshint ignore:line

DEBUG will not be applied for error message that I think shouldn't happen.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 43 • 8 years ago

Comment on attachment 8768640 [details] [diff] [review]
part 2, start-session-procedure.patch

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

::: dom/presentation/provider/ReceiverStateMachine.jsm
@@ +39,5 @@
> +        stateMachine._notifyClosed(command.reason);
> +        break;
> +      default:
> +        debug("unexpected command: " + JSON.stringify(command));
> +        // ignore

this is not jshint annotation. it is a comment for ignoring unexpected protocol message.

@@ +63,5 @@
> +        stateMachine._notifyChannelDescriptor(command);
> +        break;
> +      default:
> +        debug("unexpected command: " + JSON.stringify(command));
> +        // ignore

not jshint annotation, either.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 44 • 8 years ago

(In reply to Junior [:junior] from comment #39)
> ::: dom/presentation/tests/mochitest/PresentationSessionChromeScript.js
> @@ +407,5 @@
> >  addMessageListener('trigger-control-channel-open', function(reason) {
> >    mockedControlChannel.simulateNotifyOpened();
> >  });
> >  
> >  addMessageListener('trigger-control-channel-close', function(reason) {
> 
> This event name should be changed.
> We have lots open/close/connect/disconnect.
> Any policy for the naming?

Tedious for fixing all the naming in test cases so I'm not going to do it on this bug.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 45 • 8 years ago

Comment on attachment 8768640 [details] [diff] [review]
part 2, start-session-procedure.patch

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

::: dom/presentation/interfaces/nsIPresentationControlChannel.idl
@@ +113,5 @@
> +  /*
> +   * Disconnect the control channel.
> +   * @param reason The reason of disconnecting channel; NS_OK represents normal.
> +   */
> +  void disconnect(in nsresult reason);

would be included in part-5 patch.

	Junior [inactive]
	Comment 46 • 8 years ago

> ::: dom/presentation/provider/ReceiverStateMachine.jsm
> @@ +39,5 @@
> > +        stateMachine._notifyClosed(command.reason);
> > +        break;
> > +      default:
> > +        debug("unexpected command: " + JSON.stringify(command));
> > +        // ignore
> 
> this is not jshint annotation. it is a comment for ignoring unexpected
> protocol message.
> 
> @@ +63,5 @@
> > +        stateMachine._notifyChannelDescriptor(command);
> > +        break;
> > +      default:
> > +        debug("unexpected command: " + JSON.stringify(command));
> > +        // ignore
> 
> not jshint annotation, either.

I guess annotation like "ignore unexpected protocol message" would be better than only one word annotation. When I see this comment, "ignore what" comes to my mind and I need a few seconds to figure out.

> > ::: dom/presentation/tests/mochitest/PresentationSessionChromeScript.js
> > @@ +407,5 @@
> > >  addMessageListener('trigger-control-channel-open', function(reason) {
> > >    mockedControlChannel.simulateNotifyOpened();
> > >  });
> > >  
> > >  addMessageListener('trigger-control-channel-close', function(reason) {
> > 
> > This event name should be changed.
> > We have lots open/close/connect/disconnect.
> > Any policy for the naming?
> 
> Tedious for fixing all the naming in test cases so I'm not going to do it on
> this bug.

It's up to you but we need to keep in mind we have this issue which makes others have difficulty to decide the name.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 47 • 8 years ago

(In reply to Junior [:junior] from comment #46)
> > ::: dom/presentation/provider/ReceiverStateMachine.jsm
> > @@ +39,5 @@
> > > +        stateMachine._notifyClosed(command.reason);
> > > +        break;
> > > +      default:
> > > +        debug("unexpected command: " + JSON.stringify(command));
> > > +        // ignore
> > 
> > this is not jshint annotation. it is a comment for ignoring unexpected
> > protocol message.
> > 
> > @@ +63,5 @@
> > > +        stateMachine._notifyChannelDescriptor(command);
> > > +        break;
> > > +      default:
> > > +        debug("unexpected command: " + JSON.stringify(command));
> > > +        // ignore
> > 
> > not jshint annotation, either.
> 
> I guess annotation like "ignore unexpected protocol message" would be better
> than only one word annotation. When I see this comment, "ignore what" comes
> to my mind and I need a few seconds to figure out.
yes I'll make a complete sentence in my next version.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 48 • 8 years ago

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

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 49 • 8 years ago

update according to review comments and rebase to latest m-c, carry r+.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 50 • 8 years ago

update according to review comment and rebase to latest m-c, carry r+.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 51 • 8 years ago

update according to review comment and rebase to latest m-c, carry r+.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 52 • 8 years ago

update according to review comment and rebase to latest m-c, carry r+.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 53 • 8 years ago

update according to review comment and rebase to latest m-c, carry r+.

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 54 • 8 years ago

rename notifyOpened/notifyClosed to notifyConnect/notifyDisconnected

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 55 • 8 years ago

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): presentation api
User impact if declined: n/a
Testing completed: manual test
Risk to taking this patch (and alternatives if risky): n/a
String or UUID changes made by this patch: n/a

	Pulsebot
	Comment 56 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd6b414ffb8b
Part 0 - introduce jshint in PresentationControlService. r=junior
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae8dbcd27f34
Part 1 - broadcast new service type and protocol version on mDNS. r=junior
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff7f8ad4bdd
Part 2, implement start presentation procedure. r=junior
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c64cab758d8
Part 3, backward compatibility for connecting TV 2.5 devices. r=junior
https://hg.mozilla.org/integration/mozilla-inbound/rev/c460c012034e
Part 4, fix naming in test cases. r=junior
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b73e428562
Part 5, rename callback functions in nsIPresentationControlChannel.idl. r=junior

	Carsten Book [:Tomcat]
	Comment 57 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fd6b414ffb8b
https://hg.mozilla.org/mozilla-central/rev/ae8dbcd27f34
https://hg.mozilla.org/mozilla-central/rev/2ff7f8ad4bdd
https://hg.mozilla.org/mozilla-central/rev/6c64cab758d8
https://hg.mozilla.org/mozilla-central/rev/c460c012034e
https://hg.mozilla.org/mozilla-central/rev/b1b73e428562

	Josh Cheng [:josh]
	Comment 58 • 8 years ago

Comment on attachment 8770004 [details]
pull request for tv 2.6

Approve for TV 2.6

	Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) Assignee
	Comment 59 • 8 years ago

Comment on attachment 8770004 [details]
pull request for tv 2.6

ni? @xeonchen for uplift.

	Gary Chen [:xeonchen]
	Comment 60 • 8 years ago

https://github.com/mozilla-b2g/gecko-b2g/commit/a1cc00d50a8c4feaefa62fc9fedc9d015357aa61