Re: Comments on Networked Service Discovery and Messaging draft (general comments)

Hi Cathy,

Thank you for taking the time to review and for providing excellent
feedback.

Comments inline below.

- Rich

Cathy.Chan@nokia.com wrote:
  > First of all, thank you Rich and Clarke for putting together the draft. I
  > think it cleaned up a lot of ambiguities in the previous drafts. I also
  > applaud the change to now allow UAs to choose to implement SSDP or
  > zeroconf
  > or both as opposed to mandating both. Below are some comments and
  > questions
  > on the design of the API in general. I'll be sending a separate
  > message with
  > comments specific to UPnP discovery as I expect those to draw a different
  > set of audience.

I hope to review your comments on the UPnP section in another email soon.

  >
  > 4.1
  > - In Step 1, add "and return" at the end (to be consistent with e.g. Step
  > 5).

I've actually removed Step 1 from the algorithm since the check that
successCallback is present and of type
NavigatorNetworkServiceSuccessCallback (i.e. Function callback) is
handled implicitly in the WebIDL processing.

  > - The PERMISSION_DENIED_ERR error code is used in Steps 8 and 10, but the
  > error conditions in Step 8 ("services found is an empty array") and
  > Step 10
  > ("based on a previously-established user preference", "for security
  > reasons", "or due to platform limitations") do not really match the
  > definition of the error "The user denied the page permission to access
  > any
  > services." I would suggest either making the error code (and description)
  > more generic, or creating a separate error code to handle these other
  > conditions and leaving the PERMISSION_DENIED_ERR error code for use
  > solely
  > in the situation where the user *actively* denies permission.

I think this is a privacy preserving feature. We don't want web pages to
develop an attitude toward users if it knows that the user was the one
to reject feature permission.

If permission is denied then the page doesn't need to know exactly who
denied that permission or why it was denied, just that it was denied for
any of the reasons you mention above.

Having said that, I've updated the definition of PERMISSION_DENIED to
"The user or user agent denied the page permission to access any services".

  > - In Step 13-2, instead of using "service was originally created from
  > a UPnP
  > discovery process", using "the type attribute of service begins with
  > "upnp:"
  > is a more precise condition. (Probably similar changes are needed at
  > other
  > places.)

Done.

  >
  > 5.1
  > - [typo] s/in one or the requested valid service types/in one of the
  > requested valid service types/ (two instances)

Fixed.

  > - On servicesAvailable: "By default, servicesAvailable must be set to 1."

Because successCallback cannot fire unless at least one service is
provided to the page by the user (step 7 in getNetworkServices
algorithm). Therefore the default case is for a page to be provided with
1 service and servicesAvailable defaults to 1.

If no services are provided then errorCallback, if provided, is invoked.

  > When do we expect a default value to be in use instead of an actual
  > value?
  > Why is the default value not 0?

I think you're right that we don't need a default value here since
servicesAvailable is always set in step 13 of the getNetworkServices
algorithm before a success callback is ever fired.

  > - The onserviceavailable event is fired when a previously unknown
  > instance
  > of a matching service becomes available. How about the case when a
  > previously known instance re-connects to the network (after having been
  > disconnected from the network for any period of time)? In other words, is
  > the onserviceavailable event tied solely to newly available services, or
  > does it reflect an increase in the value of servicesAvailable?

onserviceavailable is intended to fire each time a service connects or
re-connects, having been dropped from the 'list of available service
records' table. If a service is dropped then onserviceunavailable is
fired and servicesAvailable is decremented by 1.

  >
  > 5.2
  > - In previous drafts, the list of "current authorized services" in
  > NetworkServices is explicitly stated as immutable. Similar language is no
  > longer found in this draft, and the Note also mentions the UA dynamically
  > adding or removing network services. Does it mean that the list can
  > dynamically change now? If so, how, and where is it described?

The authorized services list provided in NetworkServices remain
immutable. I've (re-)added some text to this effect in this section.

  >
  > 6.1
  > - "The id attribute is a unique identifier for the service. Two services
  > provided at different times or on different objects must have the same id
  > value." Shouldn't they have "different" values?
  >

I don't think so. I call getNetworkServices and obtain e.g. 2 objects. I
then bind those objects to e.g. in-page UI elements. I then re-call
getNetworkServices and this time I obtain e.g. 4 objects. I can then use
the .id attribute to re-apply the bindings used from the initial call to
getNetworkServices with their corresponding UI elements (assuming that
the NetworkService objects provided in the second call are not provided
in the same order as they were in the first call).

  > 6.3
  > - There is no description on when these events will be fired (albeit
  > quite
  > obvious).

A 'message' event is fired only in the algorithm to 'setup a UPnP Events
Subscription'. I wouldn't mind changing the name of this event to e.g.
'notify' since it is only applicable in the UPnP service case and
'message' conflicts with other well-defined event names. I haven't made
this change yet pending further feedback.

A 'readystatechange' event is fired when a service leaves or rejoins the
user's current local network. The latter case was a little underdefined
in the spec so I've added an additional step in both discovery
algorithms for this (Step 8.3 for UPnP. Step 9.3 for Zeroconf). I think
these sections are not really fully defined at this point though so I'm
going to have to revisit their definition at a later time.

  >
  > 7.
  > - In the first sentence, the reference to Zeroconf is to RFC 3927,
  > which is
  > misleading, as RFC 3927 is only one part of what is generally known as
  > the
  > Zeroconf discovery mechanism and does not have much to do with discovery.
  > Implementing that reference alone is not sufficient to provide the
  > feature
  > detailed in this specification. The actual relevant reference would be
  > mDNS
  > and DNS-SD.

Agreed and fixed.

  >
  > 7.3
  > - In the case that the user has dropped from the network, there needs
  > to be
  > a Step 2-4 to set servicesAvailable to 0, and possibly to dispatch an
  > onserviceunavailable event.

Roger. I've added text to this effect in the spec.

  > - In the case that the user has connected to a new network, what
  > parameters
  > are being used in re-issuing the discovery search (e.g. what search
  > target
  > is used in SSDP)? There is not mentioned in the section on Service
  > Discovery either.

I haven't found a reasonable way to include this in the draft yet. It
will probably require a rewrite or the UPnP and Zeroconf sections in
Section 7 so I'll have to re-visit this at a later date.

  > Also, s/Section 6: Service Discovery/Section 7: Service
  > Discovery/.

Typo fixed.

  >
  > References
  > - RFC 3927 has been published and is no longer a draft. The reference
  > should
  > be to http://www.ietf.org/rfc/rfc3927.txt, unless there is a reason to
  > refer
  > to the specific draft.

It turns out that changing the reference at the beginning of Section 7
made this reference superfluous. Therefore it has been removed from the
spec. I believe this reference was erroneous in the first place.

Again, thank you for your review and if you have any further feedback
please let us know. As I mentioned above, I hope to address your
feedback on UPnP in a separate thread soon.

Best regards,

Rich

Received on Wednesday, 22 August 2012 14:30:53 UTC