Tom Tung [:tt, :ttung] Assignee
	Description • 9 years ago

Add a mapping mechanism to convert between service/characteristic/descriptor names and UUIDs.

	Tom Tung [:tt, :ttung] Assignee
	Comment 1 • 9 years ago

The attachment is the test app used for testing functionalities of BluetoohUUID. You can press the "Test UUID" button and the result will be shown in console. The test case is at the end of |main.js|.

	Tom Tung [:tt, :ttung] Assignee
	Comment 2 • 9 years ago

The attachment is the patch for BluetoothUUID. In this patch, I mainly impelment |BluetoothServiceUUID|, |BluetoothCharacteristicUUID|, |BluetoothDescriptorUUID| and |canonicalUUID| functions in webapi/webidl for bluetooth which can be used for mapping between "name"s and "UUID"s.

	Tom Tung [:tt, :ttung] Assignee
	Comment 3 • 9 years ago

The requirement of bluetoothUUID from W3C spec can be found in [1].

[1] https://webbluetoothcg.github.io/web-bluetooth/#dom-bluetoothuuid-getservice

	Jocelyn Liu [:jocelyn] [:joliu]
	Comment 4 • 9 years ago

Comment on attachment 8709357 [details] [diff] [review]
bluetoothUUID.patch

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

::: dom/bluetooth/common/BluetoothGattUUIDName.h
@@ +12,5 @@
> +  nsString name;
> +  uint16_t uuid;
> +};
> +
> +BluetoothGattUUIDName ServiceTable[] = {

Suggest to add a link of BT SIG website for all the tables.

::: dom/bluetooth/common/webapi/BluetoothUUID.cpp
@@ +5,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "BluetoothCommon.h"
> +#include "BluetoothGattUUIDName.h"
> +#include "BluetoothUUID.h"

mozilla/dom/bluetooth/GattUUIDName.h and mozilla/dom/bluetooth/BluetoothUUID.h.

@@ +11,5 @@
> +#include "mozilla/dom/BluetoothUUIDBinding.h"
> +#include "mozilla/dom/bluetooth/BluetoothCommon.h"
> +#include "mozilla/dom/bluetooth/BluetoothGattCharacteristic.h"
> +#include "mozilla/dom/bluetooth/BluetoothTypes.h"
> +#include "mozilla/dom/DataStoreBinding.h" /* StringOrUnsignedLong */

UnionTypes.h instead of DataStroreBinding.h?

@@ +46,5 @@
> +{
> +  return BluetoothUUIDBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +/* static */ void

nit: Revise as follow to be consistent with current BT code base.
// static
void

@@ +49,5 @@
> +
> +/* static */ void
> +BluetoothUUID::InitServiceTable()
> +{
> +  size_t end = sizeof(ServiceTable)/sizeof(ServiceTable[0]);

How about using sizeof(BluetoothGattUUIDName)?
And suggest to use 'length' rather than 'end'.

sizeof(ServiceTable) / sizeof(ServiceTable[0])

@@ +50,5 @@
> +/* static */ void
> +BluetoothUUID::InitServiceTable()
> +{
> +  size_t end = sizeof(ServiceTable)/sizeof(ServiceTable[0]);
> +  for(size_t i = 0;i < end;++i)

for (size_t i = 0; i < end; ++i)

@@ +51,5 @@
> +BluetoothUUID::InitServiceTable()
> +{
> +  size_t end = sizeof(ServiceTable)/sizeof(ServiceTable[0]);
> +  for(size_t i = 0;i < end;++i)
> +    mUUIDServiceTable.Put(ServiceTable[i].name,ServiceTable[i].uuid);

space after ','

And I think we should always have braces for control blocks.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +55,5 @@
> +    mUUIDServiceTable.Put(ServiceTable[i].name,ServiceTable[i].uuid);
> +}
> +
> +/* static */ void
> +BluetoothUUID::InitCharacteristicTable()

DITTO.

@@ +64,5 @@
> +                                 CharacteristicTable[i].uuid);
> +}
> +
> +/* static */ void
> +BluetoothUUID::InitDescriptorTable()

DITTO.

@@ +87,5 @@
> +  const char16_t char_0 = char('0');
> +  const char16_t char_9 = char('9');
> +  const char16_t char_a = char('a');
> +  const char16_t char_f = char('f');
> +  const char16_t char_hyphen = char('-');

How about we use the character directly instead of defining them all?

@@ +89,5 @@
> +  const char16_t char_a = char('a');
> +  const char16_t char_f = char('f');
> +  const char16_t char_hyphen = char('-');
> +
> +  for (size_t i = 0; i < len; ++i, ++uuid) {

Suggest to use uuid[i] directly instead of adding uuid.

@@ +130,5 @@
> +    } else {
> +      // Exception, Syntax error, assign aReturn the error message
> +      aReturn.AssignLiteral("Invalid name: It can be a 32-bit UUID alias,\
> + 128-bit valid UUID (lower-case hex characters) or known Service/Characteristic\
> +/Descriptor name");

It's a bit weird that we put error strings directly in the return string.

I would prefer https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaStreamError.cpp#26, this kind of style when the string is over 80 chars.

@@ +157,5 @@
> +                     NS_LITERAL_STRING("org.bluetooth.service"),
> +                     aReturn) == NS_ERROR_DOM_SYNTAX_ERR) {
> +    aRv.ThrowDOMException(NS_ERROR_DOM_SYNTAX_ERR,
> +                          NS_ConvertUTF16toUTF8(aReturn));
> +    aReturn = nullptr;

I think what we should do here is reset to empty string?

@@ +207,5 @@
> +{
> +  char uuidStr[37];
> +  uint8_t aUuid[4];
> +
> +  // rearrange the alia for |ntohl|

alias.

::: dom/bluetooth/common/webapi/BluetoothUUID.h
@@ +7,5 @@
> +#ifndef mozilla_dom_bluetooth_BluetoothUUID_h
> +#define mozilla_dom_bluetooth_BluetoothUUID_h
> +
> +#include "mozilla/Attributes.h"
> +#include "mozilla/DOMEventTargetHelper.h"

Why is it needed?

@@ +24,5 @@
> +} // mozilla
> +
> +BEGIN_BLUETOOTH_NAMESPACE
> +
> +class BluetoothSignal;

Why is it needed?

@@ +39,5 @@
> +    CHARACTERISTIC,
> +    DESCRIPTOR
> +  };
> +
> +  BluetoothUUID(nsPIDOMWindow* aOwner);

I haven't see use cases in your patch accessing this constructor, could you share that why is this need to be public?

@@ +47,5 @@
> +  }
> +
> +  virtual JSObject* WrapObject(JSContext* aCx,
> +                               JS::Handle<JSObject*> aGivenProto) override;
> +  static void InitServiceTable();

Add a newline before this line.

@@ +61,5 @@
> +  static void GetDescriptor(const GlobalObject& aGlobal,
> +                            const StringOrUnsignedLong& aName,
> +                            nsString& aReturn, ErrorResult& aRv);
> +
> +  static void CanonicalUUID(const GlobalObject& aGlobal, uint32_t alias,

aAlias

@@ +66,5 @@
> +                            nsString& aReturn);
> +private:
> +  ~BluetoothUUID();
> +
> +  static NS_IMETHODIMP ResolveUUIDName(const GlobalObject& aGlobal,

nsresult?

::: dom/webidl/BluetoothUUID.webidl
@@ +1,5 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

This is for c++, please use the WebIDL one and add the spec link.
ex: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Cache.webidl#1-9

	Tom Tung [:tt, :ttung] Assignee
	Comment 5 • 9 years ago

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #4)
Hi Joycelyn, 

Thanks for your review! Sorry for my late reply and bad coding style. I will resolve them as soon as possible.

> ::: dom/bluetooth/common/BluetoothGattUUIDName.h
> Suggest to add a link of BT SIG website for all the tables.
OK, I will put each link of BT SIG website in comment at the top of table.
 
> ::: dom/bluetooth/common/webapi/BluetoothUUID.cpp
> @@ +5,5 @@
> mozilla/dom/bluetooth/GattUUIDName.h and
> mozilla/dom/bluetooth/BluetoothUUID.h.
Sure.

> @@ +11,5 @@ 
> UnionTypes.h instead of DataStroreBinding.h?
Sure, I should include |UnionTypes.h| rather than others binding header file.

> @@ +49,5 @@
> > +  size_t end = sizeof(ServiceTable)/sizeof(ServiceTable[0]); 
> How about using sizeof(BluetoothGattUUIDName)?
> And suggest to use 'length' rather than 'end'.
OK, it makes the code much easier to read.

> @@ +87,5 @@
> > +  const char16_t char_0 = char('0');
> > +  const char16_t char_9 = char('9');
> > +  const char16_t char_a = char('a');
> > +  const char16_t char_f = char('f');
> > +  const char16_t char_hyphen = char('-');
> How about we use the character directly instead of defining them all?
Actually, the reason for defining them before the 'for loop' is to avoid type casting in each round of the 'for loop'. If you think that it is unnecessary, I will remove all of them.

> @@ +89,5 @@
> Suggest to use uuid[i] directly instead of adding uuid.
Sure.

> @@ +157,5 @@
> > +    aReturn = nullptr;
> I think what we should do here is reset to empty string?
Sorry for useless code. I just want to make sure the return string is empty when I throw the exception. I will remove them.

> ::: dom/bluetooth/common/webapi/BluetoothUUID.h
> > +#include "mozilla/Attributes.h"
> > +#include "mozilla/DOMEventTargetHelper.h"
> Why is it needed?
> @@ +24,5 @@
> > +class BluetoothSignal;
> Why is it needed?
Oops, I forget to remove them. Sorry for that.

> @@ +39,5 @@
> > +  BluetoothUUID(nsPIDOMWindow* aOwner);
> I haven't see use cases in your patch accessing this constructor, could you
> share that why is this need to be public?
Sorry, I just neglect that I should put the constructor in private. 
 
> @@ +66,5 @@
> > +  static NS_IMETHODIMP ResolveUUIDName(const GlobalObject& aGlobal, 
> nsresult?
I think it should be 'nsresult', but I reference the return type from [1]. I will change the return type of |ResolveUUIDName| into 'nsresult'. 

[1] https://dxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.cpp#321

	Tom Tung [:tt, :ttung] Assignee
	Comment 6 • 9 years ago

Hi Jocelyn,

Please help me to review the patch again. 
In this patch, I revise the previous patch based on comments. Besides, I remove useless type conversion in |CanonicalUUID| and remove unnecessary parameter 'prefix' in |ResolveUUIDName|.

	Tom Tung [:tt, :ttung] Assignee
	Comment 7 • 9 years ago

Hi Jocelyn,

Sorry that upload another patch in a short time. Please ignore the previous patch. In this patch, I add a preference check for |interface BluetoothUUID| in the |BluetoothUUID.webidl| file.

	Jocelyn Liu [:jocelyn] [:joliu]
	Comment 8 • 9 years ago

Comment on attachment 8711595 [details] [diff] [review]
Implement UUID helper - dom/bluetooth & dom/webidl & dom/binding change - v2-1

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

Sorry for my delay here, please see my comments below.

::: dom/bluetooth/common/BluetoothGattUUIDName.h
@@ +13,5 @@
> +  uint16_t uuid;
> +};
> +
> +BluetoothGattUUIDName ServiceTable[] = {
> +  /*

/**

@@ +143,5 @@
> +  {NS_LITERAL_STRING("http_status_code"), 0x2AB8},
> +  {NS_LITERAL_STRING("http_security"), 0x2ABB},
> +  {NS_LITERAL_STRING("humidity"), 0x2A6F},
> +  {NS_LITERAL_STRING("ieee_11073-20601_regulatory_certification_data_list"),
> +   0x2A2A},

nit: two spaces indent.

::: dom/bluetooth/common/webapi/BluetoothUUID.cpp
@@ +5,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "BluetoothUtils.h"
> +
> +#include "mozilla/dom/BluetoothUUIDBinding.h"

I think this one and BluetoothCommon.h are already included in BluetoothUUID.h, could you double check if some of include statements here are redundant?

@@ +6,5 @@
> +
> +#include "BluetoothUtils.h"
> +
> +#include "mozilla/dom/BluetoothUUIDBinding.h"
> +#include "mozilla/dom/UnionTypes.h" /* StringOrUnsignedLong */

alphabetic order

@@ +7,5 @@
> +#include "BluetoothUtils.h"
> +
> +#include "mozilla/dom/BluetoothUUIDBinding.h"
> +#include "mozilla/dom/UnionTypes.h" /* StringOrUnsignedLong */
> +

remove this empty line.

@@ +18,5 @@
> +using namespace mozilla;
> +using namespace mozilla::dom;
> +
> +USING_BLUETOOTH_NAMESPACE
> +

how about add // static before this block

@@ +38,5 @@
> +  MOZ_ASSERT(aOwner);
> +}
> +
> +BluetoothUUID::~BluetoothUUID()
> +{}

nit: put '}' in the next line.

@@ +82,5 @@
> +
> +/**
> + * Check whether the UUID(aString) is valid or not
> + */
> +bool isValidUUID(const nsAString& aString)

'I'sValidUUID

@@ +113,5 @@
> +                               BluetoothAttribute aAtt,
> +                               nsString& aReturn)
> +{
> +  if (aName.IsUnsignedLong()) {
> +    // The value of argument is an 'alias'

// aName is a 16-bit or 32-bit UUID alias.

@@ +124,5 @@
> +      // The value of argument is a valid UUID String
> +      aReturn.Assign(aString);
> +    } else if ((aAtt == SERVICE && mUUIDServiceTable.Get(aString, &alias)) ||
> +               (aAtt == CHARACTERISTIC &&
> +                mUUIDCharacteristicTable.Get(aString, &alias)) ||

two spaces indent here.

@@ +126,5 @@
> +    } else if ((aAtt == SERVICE && mUUIDServiceTable.Get(aString, &alias)) ||
> +               (aAtt == CHARACTERISTIC &&
> +                mUUIDCharacteristicTable.Get(aString, &alias)) ||
> +               (aAtt == DESCRIPTOR &&
> +                mUUIDDescriptorTable.Get(aString, &alias))) {

two spaces indent.

@@ +127,5 @@
> +               (aAtt == CHARACTERISTIC &&
> +                mUUIDCharacteristicTable.Get(aString, &alias)) ||
> +               (aAtt == DESCRIPTOR &&
> +                mUUIDDescriptorTable.Get(aString, &alias))) {
> +      // The value of argument can find in Table

// The UUID string can be mapped to a known UUID alias.

@@ +160,5 @@
> +
> +  if (ResolveUUIDName(aGlobal, aName, SERVICE, aReturn) ==
> +      NS_ERROR_DOM_SYNTAX_ERR) {
> +    aRv.ThrowDOMException(NS_ERROR_DOM_SYNTAX_ERR,
> +                          NS_ConvertUTF16toUTF8(aReturn));

Suggest to use the result to throw instead of hard-coded here, it would be better to handle all kinds of error result instead of SYNTAX_ERR only.

@@ +161,5 @@
> +  if (ResolveUUIDName(aGlobal, aName, SERVICE, aReturn) ==
> +      NS_ERROR_DOM_SYNTAX_ERR) {
> +    aRv.ThrowDOMException(NS_ERROR_DOM_SYNTAX_ERR,
> +                          NS_ConvertUTF16toUTF8(aReturn));
> +    return;

Shouldn't we empty the aReturn first before return?

@@ +210,5 @@
> +                             nsString& aReturn)
> +{
> +  char uuidStr[37];
> +
> +  // Defualt UUID: alias + "-0000-1000-8000-00805f934fb"

// Convert to 128-bit-UUID: alias + "-0000-1000-8000-00805f934fb".

::: dom/bluetooth/common/webapi/BluetoothUUID.h
@@ +7,5 @@
> +#ifndef mozilla_dom_bluetooth_BluetoothUUID_h
> +#define mozilla_dom_bluetooth_BluetoothUUID_h
> +
> +#include "mozilla/dom/BluetoothUUIDBinding.h"
> +

remove this empty line.

@@ +9,5 @@
> +
> +#include "mozilla/dom/BluetoothUUIDBinding.h"
> +
> +#include "mozilla/dom/bluetooth/BluetoothCommon.h"
> +

DITTO.

@@ +41,5 @@
> +                               JS::Handle<JSObject*> aGivenProto) override;
> +
> +  static void GetService(const GlobalObject& aGlobal,
> +                         const StringOrUnsignedLong& aName,
> +                         nsString& aReturn, ErrorResult& aRv);

I think we should use nsAString& here for function parameters, and so as below?
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#String_Guidelines

@@ +66,5 @@
> +    CHARACTERISTIC,
> +    DESCRIPTOR
> +  };
> +
> +  static nsresult ResolveUUIDName(const GlobalObject& aGlobal,

Please add comments to describe your method and all the parameters and return value.

::: dom/webidl/BluetoothUUID.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is
> + * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html

wrong link?

	Tom Tung [:tt, :ttung] Assignee
	Comment 9 • 9 years ago

Hi Jocelyn,

Sorry for so many problems in last patch. In this patch, I resolved the coding style errors, checked the include statements and removed redundancies, rewrote some comments and exception handles. Please help me to review this patch.

	Tom Tung [:tt, :ttung] Assignee
	Comment 10 • 9 years ago

Hi Jocelyn,

Sorry for updating the patch suddenly. Please ignore the last patch. In this patch, I add comments for describing the |ResolveUUIDName|.

	Jocelyn Liu [:jocelyn] [:joliu]
	Comment 11 • 9 years ago

Comment on attachment 8714153 [details] [diff] [review]
Implement UUID helper - dom/bluetooth & dom/webidl & dom/binding change - v3-1

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

Overall looks good to me.
r=me after addressing comments below.

Thanks,
Jocelyn

::: dom/bluetooth/common/webapi/BluetoothUUID.cpp
@@ +129,5 @@
> +      // The UUID string can be mapped to a known UUID alias.
> +      CanonicalUUID(aGlobal, alias, aReturn);
> +    } else {
> +      // Exception, Syntax error, assign aReturn the error message
> +      aReturn.AssignLiteral("Invalid name: It can be a 32-bit UUID alias,"

space after ','

@@ +130,5 @@
> +      CanonicalUUID(aGlobal, alias, aReturn);
> +    } else {
> +      // Exception, Syntax error, assign aReturn the error message
> +      aReturn.AssignLiteral("Invalid name: It can be a 32-bit UUID alias,"
> +                            "128-bit valid UUID (lower-case hex characters) or"

space after 'or'

@@ +136,5 @@
> +      return NS_ERROR_DOM_SYNTAX_ERR;
> +    }
> +  } else {
> +    // Exception, Syntax error, assign aReturn the error message
> +    aReturn.AssignLiteral("Invalid name: incorrect type. It can be a"

remove a, and make sure you have a space after 'be'.

::: dom/bluetooth/common/webapi/BluetoothUUID.h
@@ +57,5 @@
> +  static void InitServiceTable();
> +  static void InitCharacteristicTable();
> +  static void InitDescriptorTable();
> +
> +  enum BluetoothAttribute {

nit: suggest to rename as GattAttribute

@@ +69,5 @@
> +   * on its bluetooth attribute.
> +   *
> +   * @param aGlobal [in] a Global object for static attributes.
> +   * @param aName [in] a UUID string or 16-bit/32-bit UUID alias.
> +   * @param aAtt [in] a Bluetooth Gatt Attribute.

@param aAtt [in] a GATT attribute type.

	Tom Tung [:tt, :ttung] Assignee
	Comment 12 • 9 years ago

Hi Jocelyn,

Thanks a lot for your time and valuable comments! This patch address review comments.

	Tom Tung [:tt, :ttung] Assignee
	Comment 13 • 9 years ago

Comment on attachment 8714174 [details] [diff] [review]
Bug 1238845 - Implement UUID helper - dom/bluetooth & dom/webidl & dom/binding change. r=jocelyn

Hi Boris,

We are working on aligning our GATT(BLE) API with W3C sepc. My approach here is providing functions for mapping from a service/characteristic/descriptor name or 16-bit/32-bit UUID alias to 128-bit UUID. Besides, it throws a DOM exception(SYNTAX ERROR) while the syntax is incorrect. The corresponding W3C spec can be found in [1]. Could you help to review my patch?

[1] https://webbluetoothcg.github.io/web-bluetooth/#bluetoothuuid 

Thanks,
Tom

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 9 years ago

Comment on attachment 8714174 [details] [diff] [review]
Bug 1238845 - Implement UUID helper - dom/bluetooth & dom/webidl & dom/binding change. r=jocelyn

For W3C specs, we typically try to put their objects directly into the mozilla::dom namespace.  Is there a reason to not do that here?

>+BluetoothGattUUIDName ServiceTable[] = {

These tables are going to trigger a ton of static constructors, no?

It would be better to just store const char* in the static table and then either convert to UTF-16 when setting up the big hashtable or store ASCII in the hashtable and convert tine UTF-16 input to UTF-8 when doing the hashtable lookup.

Also, this table should be static, not an extra global visibility variable, right?  You can probably leave it in the header if you want, or move the declaration into the C++ and use preprocessor tricks on some headers to set up the table entries.

>+// static
>+nsDataHashtable<nsStringHashKey, uint32_t> BluetoothUUID::mUUIDServiceTable;

This will trigger shutdown leaks, no?  You probably need to make this a pointer to object and delete it during shutdown.

>+nsresult
>+BluetoothUUID::ResolveUUIDName(const GlobalObject& aGlobal,

Every single consumer is using ErrorResult to handle exceptions.  This function should too, instead of creating extra impedance mismatches at the callers.

Then, for example, BluetoothUUID::GetService would end with the single line:

  ResolveUUIDName(aGlobal, aName, SERVICE, aReturn, aRv);

instead of the 6 lines it has now.  And you'd just directly throw the relevant syntax error on the ErrorResult.

Also, the aGlobal argument is unused, right?  Why is it there?

>+    aReturn.AssignLiteral("Invalid name: incorrect type. It can be "
>+                          "String or Unsigned long.");

This case can't arise short of some C++ caller screwing up.  Feel free to just MOZ_ASSERT that it does not arise.

>+BluetoothUUID::CanonicalUUID(const GlobalObject& aGlobal, uint32_t aAlias,

Doesn't need aGlobal.

>+  snprintf(uuidStr, sizeof(uuidStr), "%.8x-%.4x-%.4x-%.4x-%.8x%.4x",
>+           aAlias, 0x0000, 0x1000, 0x8000, 0x00805f9b, 0x34fb);

Why not just put those last 5 numbers as part of the format string, since they're already constatns in exactly the format you want?

r=me with the above addressed.

	Tom Tung [:tt, :ttung] Assignee
	Comment 15 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #14)

Hi Boris,

Sorry for my late reply. Thanks for your review and feedback. I learned a lot from it and addressed into my patch. However, there are somethings that I am not sure how to do or I don't understand exactly. Could you help me to figure them out? 

> These tables are going to trigger a ton of static constructors, no?
There is no static constructors in C++ as far as I know. Did you mean initialize the static object or something else?

> >+// static
> >+nsDataHashtable<nsStringHashKey, uint32_t> BluetoothUUID::mUUIDServiceTable;
> This will trigger shutdown leaks, no?  You probably need to make this a
> pointer to object and delete it during shutdown.

I am not sure how to delete it during shutdown. Do you mean that I need to handle a observer for observing shutdown message and delete the pointer such as method in [1]?

> 
> >+nsresult
> >+BluetoothUUID::ResolveUUIDName(const GlobalObject& aGlobal,
> Also, the aGlobal argument is unused, right?  Why is it there?

The aGlobal the generate by webidl due to static function attributes [2], but I did not use it.  

[1]https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothGattManager.cpp#4132-4140
[2]https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#C_reflections_of_WebIDL_operations_%28methods%29

Thanks,
Tom

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 9 years ago

> Sorry for my late reply.

Less than a day doesn't count as late.  :)

> There is no static constructors in C++ as far as I know.

Er, I meant static initializers.  See https://groups.google.com/forum/#!topic/mozilla.dev.platform/q_YK1f0Dpwg and similar.  Note that we have automated tests that will complain if you add more static initializers...

> Do you mean that I need to handle a observer for observing shutdown message and delete the pointer

Yes.


> The aGlobal the generate by webidl due to static function attributes

Yes, but the functions I commented on are not being called by the webidl codegen.  The ones that are being called from codegen need the GlobalObject argument.  The ones that you create as internal helpers don't need that argument unless they plan to use it.

	Tom Tung [:tt, :ttung] Assignee
	Comment 17 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #16)
Hi Boris,

Thank you for your reply, I add an observer for handling shutdown event into my patch and change the type of table to "static" and its entries to "const char*". 
But I have some follow up questions.

> Yes, but the functions I commented on are not being called by the webidl
> codegen.  The ones that are being called from codegen need the GlobalObject
> argument.  The ones that you create as internal helpers don't need that
> argument unless they plan to use it.
Yes, but the function |canonicalUUID| is external [1]. Since I need the call it in |ResolveUUIDName|, I need to have the GlobalObject argument |ResolveUUIDName|.

In comment #14,
> Also, this table should be static, not an extra global visibility variable, right?  You can probably
> leave it in the header if you want, or move the declaration into the C++ and use preprocessor tricks on
> some headers to set up the table entries.
Does "leave it in the header" mean that I can keep it in |BluetoothGattUUIDName.h| by addressing other comments?

Does "use preprocessor tricks" mean that I could try to move it into |BluetoothUUID.cpp| and use it by macro such as "#define, #ifdef"?

Thanks, 
Tom
 

[1] https://webbluetoothcg.github.io/web-bluetooth/#standardized-uuids

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 9 years ago

> Yes, but the function |canonicalUUID| is external [1].

Oh, I see.  That's pretty annoying.  Add a comment to the function declaration for ResolveUUIDName that says it only has the arg because it needs to call CanonicalUUID, please.

> Does "leave it in the header" mean that I can keep it in |BluetoothGattUUIDName.h|

Yes, since it's only included in one place.

> Does "use preprocessor tricks" mean that I could try to move it into |BluetoothUUID.cpp|
> and use it by macro such as "#define, #ifdef"?

Yes, by having a header that invokes the macro and #defining it in the cpp to whatever you want.  We use this in various other places in Gecko; see the many *List.h header files.  But I think in this case just adding static would be fine.

	Tom Tung [:tt, :ttung] Assignee
	Comment 19 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #18)
Hi Boris,

Thanks for the review and valuable feedback. I kept the table in |BluetoothGattUUIDName.h| and add a comment to the function declaration |ResolveUUIDName|.

	Tom Tung [:tt, :ttung] Assignee
	Comment 20 • 9 years ago

Comment on attachment 8715686 [details] [diff] [review]
Bug 1238845 - Implement UUID helper - dom/bluetooth & dom/webidl & dom/binding change. r=jocelyn, r=bz

Hi Jocelyn,

Since I am afraid that I may make some mistakes or do some bad implementation, could you help me to review the patch again? 

In this patch, I changed the namespace of |BluetoothUUID| and |BluetoothGattUUIDName| from mozilla::dom::bluetooth to the mozilla::dom. I also change the type of tables in |BluetoothGattUUIDName| to static. For avoiding tons of static initialization, I changed the structure member |name| in |BluetoothGattUUIDName| from "nsString" to "const char*". For avoiding shut down leaks, I implement |Init()|, |Uninit()|, |Observe()| and |HandleShutdown()|. |Init()| will be called when the class is constructed and |Uninit()| as the class is destructed. Besides, I changed the implementation of the method for creating table and finding entry. I create a function |GetTable| for replacing origin method. Furthermore, I added some comments and put some assert in it.

Thanks,
Tom

	Tom Tung [:tt, :ttung] Assignee
	Comment 21 • 9 years ago

Comment on attachment 8715686 [details] [diff] [review]
Bug 1238845 - Implement UUID helper - dom/bluetooth & dom/webidl & dom/binding change. r=jocelyn, r=bz

Hi Boris,

Since I did quite a lot change for my patch and I may misunderstand about your comments, could you help me to review this patch one more time?

In this patch, I changed the namespace of |BluetoothUUID| and |BluetoothGattUUIDName| from mozilla::dom::bluetooth to the mozilla::dom. I also change the type of tables in |BluetoothGattUUIDName| to static. For avoiding tons of static initialization, I changed the structure member |name| in |BluetoothGattUUIDName| from "nsString" to "const char*". For avoiding shut down leaks, I implement |Init()|, |Uninit()|, |Observe()| and |HandleShutdown()|. |Init()| will be called when the class is constructed and |Uninit()| as the class is destructed. Besides, I changed the implementation of the method for creating table and finding entry. I create a function |GetTable| for replacing origin method. Furthermore, I added some comments and put some assert in it.

Thanks,
Tom

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 9 years ago

Comment on attachment 8715686 [details] [diff] [review]
Bug 1238845 - Implement UUID helper - dom/bluetooth & dom/webidl & dom/binding change. r=jocelyn, r=bz

>+++ b/dom/bindings/Bindings.conf

You don't need any changes to this file at all, since your new nativeType value is the default value that would be used by the bindings codegen anyway.

>+++ b/dom/bluetooth/common/webapi/BluetoothUUID.cpp
>+bool BluetoothUUID::mInShutdown = false;

sInShutdown?

>+  BluetoothUUID::sUUIDServiceTable = nullptr;

nullptr is the default value anyway (statics are 0-initialized), right?

>+BluetoothUUID::Init()

This doesn't make sense to me.  This will add every single BluetoothUUID instance as a shutdown observer, no?  Worse yet, it's doing that from the constructor, which is a terrible idea because the object has a zero refcount at that point and the observer service might cause it to be deleted.

And it's having the observer service hold a strong ref to the object, so every instance of BluetoothUUID leaks...

It might be simplest to just delete these hashtables in a function called by nsLayoutStatics::Shutdown.  I think that should be early enough for our purposes, especially since these tables don't hold on to anything other than strings.

>+  sUUIDServiceTable->Clear();
>+  sUUIDServiceTable = nullptr;

This leaks the actual hashtable, right?  You just want:

  delete sUUIDServiceTable;

>+      errorStr.AssignLiteral("Invalid name: It can be a 32-bit UUID alias, "

Just used NS_NAMED_LITERAL_CSTRING if that's what you want here.

>+  if (aAttr == SERVICE && sUUIDServiceTable) {
>+    if (sUUIDServiceTable->Count() == 0) {
>+      InitServiceTable();

How could the count possibly be 0 there?  I don't think it can.  SAme for the other branches in this function.

Also a bunch of else-after-return here...  The way I might have structured this is like so:

  nsDataHashtable<nsStringHashKey, uint32_t>** tableSlot;
  if (aAttr == SERVICE) {
    tableSlot = &sUUIDServiceTable;
  } else if (aAttr == CHARACTERISTIC) {
    tableSlot = &sUUIDCharacteristicTable;
  } else {
    MOZ_ASSERT(aAttr == DESCRIPTOR);
    tableSlot = &sUUIDDescriptorTable;
  }
  if (!*tableSlot) {
    // Do the branching on aAttr again, calling Init*Table as needed,
    // though if you really wanted to you could do it via
    // pointer-to-method set in the branching above.
  }
  return (*tableSlot)->Get(aString, &alias);

You don't have to do it that way, of course, but please do deal with the useless count == 0 checks.

>+class BluetoothUUID final : public nsWrapperCache
>+                          , public nsIObserver

Did you test this in a debug build?  This should have failed the asserts in BluetoothUUIDBinding::Wrap because the canonical nsISupports needs to be the first thing we inherit from (whether via nsIObserver or not).

r=me with the above issues fixed.

	Tom Tung [:tt, :ttung] Assignee
	Comment 23 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #22)
Hi Boris,

Thanks again! I always learn a lot from your feedback!
> >+++ b/dom/bindings/Bindings.conf
> You don't need any changes to this file at all, since your new nativeType
> value is the default value that would be used by the bindings codegen anyway.

Oh, I didn't notice that I don't need to put it into |Bindings.conf| since it is default value.

> >+++ b/dom/bluetooth/common/webapi/BluetoothUUID.cpp
> >+bool BluetoothUUID::mInShutdown = false;
> sInShutdown?

Yes, "sInShutdown" is much better since other member variables start with "s".

> >+  BluetoothUUID::sUUIDServiceTable = nullptr;
> nullptr is the default value anyway (statics are 0-initialized), right?

Yes, I will remove the useless assignment.
 
> >+BluetoothUUID::Init()
> This doesn't make sense to me.  This will add every single BluetoothUUID
> instance as a shutdown observer, no?  Worse yet, it's doing that from the
> constructor, which is a terrible idea because the object has a zero refcount
> at that point and the observer service might cause it to be deleted.
> And it's having the observer service hold a strong ref to the object, so
> every instance of BluetoothUUID leaks...
> It might be simplest to just delete these hashtables in a function called by
> nsLayoutStatics::Shutdown.  I think that should be early enough for our
> purposes, especially since these tables don't hold on to anything other than
> strings.

Sorry for that I misunderstood about what you meant and bad implementation. I will rewrite the method for shutdown leaks. Did you mean that I can create a |HandShutdown()| to delete the pointer to table and put it at |nsLayoutStatics::Shutdown| without handle the observer and register for observing?

 
> >+  sUUIDServiceTable->Clear();
> >+  sUUIDServiceTable = nullptr;
> This leaks the actual hashtable, right?  You just want:
>   delete sUUIDServiceTable;

Did you meant that I could just delete the pointer without emptying the table?

> >+      errorStr.AssignLiteral("Invalid name: It can be a 32-bit UUID alias, " 
> Just used NS_NAMED_LITERAL_CSTRING if that's what you want here.

That a good idea. How about just call "NS_LITERAL_CSTRING" in second parameter of |throwDOMException()| without create a "nsACString" variable?


> >+  if (aAttr == SERVICE && sUUIDServiceTable) {
> >+    if (sUUIDServiceTable->Count() == 0) {
> >+      InitServiceTable(); 
> How could the count possibly be 0 there?  I don't think it can.  SAme for
> the other branches in this function.

I thought there might have a situation that it new a pointer to table without initialization, but I was wrong. I will remove it.

> Also a bunch of else-after-return here...  The way I might have structured
> this is like so:
>   nsDataHashtable<nsStringHashKey, uint32_t>** tableSlot;
>   if (aAttr == SERVICE) {
>     tableSlot = &sUUIDServiceTable;
>   } else if (aAttr == CHARACTERISTIC) {
>     tableSlot = &sUUIDCharacteristicTable;
>   } else {
>     MOZ_ASSERT(aAttr == DESCRIPTOR);
>     tableSlot = &sUUIDDescriptorTable;
>   }
>   if (!*tableSlot) {
>     // Do the branching on aAttr again, calling Init*Table as needed,
>     // though if you really wanted to you could do it via
>     // pointer-to-method set in the branching above.
>   }
>   return (*tableSlot)->Get(aString, &alias);
> You don't have to do it that way, of course, but please do deal with the
> useless count == 0 checks.

This method looks great! I will apply them into |GetTable()|.

> >+class BluetoothUUID final : public nsWrapperCache
> >+                          , public nsIObserver
> 
> Did you test this in a debug build?  This should have failed the asserts in
> BluetoothUUIDBinding::Wrap because the canonical nsISupports needs to be the
> first thing we inherit from (whether via nsIObserver or not).

I test every patches in debug build. It is strange that there is no any error happens, but I will still change the order of them. 

Thanks,
Tom

	Tom Tung [:tt, :ttung] Assignee
	Comment 24 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #22)
Sorry for creating another comment, but I also confusing about when to use |nsLayoutStatics::Shutdown| and |addObserve|? Could you explain it to me so that I can do better next time? 

Thanks

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 9 years ago

> Did you mean that I can create a |HandShutdown()| to delete the pointer to table
> and put it at |nsLayoutStatics::Shutdown| without handle the observer
> and register for observing?

I assume you mean HandleShutdown, not HandShutdown?  Yes, this is what I mean.

> Did you meant that I could just delete the pointer without emptying the table?

Correct.  The table's destructor will empty it.

> How about just call "NS_LITERAL_CSTRING" in second parameter

That's fine too, yes.

> Sorry for creating another comment, but I also confusing about when
> to use |nsLayoutStatics::Shutdown|

No need to apologize; that stuff is non-obvious.

The shutdown sequence, in simplified terms, goes like this.  We decide we want to shut down.  We send out the XPCOM shutdown notification.  After this notification is sent, generally things like createInstance and getService will not work correctly.  This notification is a good place to drop references to things you're holding, in general, especially if they have destructors that plan to do any interesting work (e.g. using other services), because after this point this interesting work is likely not possible to do.  Then after that we go on to shut down XPCOM modules, etc.

We also have an nsLayoutStatics object.  This is actually a reference-counted object.  When its reference count goes to 0, it calls nsLayoutStatics::Shutdown().  There's a reference to this object that gets dropped from layout module shutdown (so _after_ the XPCOM shutdown notification processing is done), but other references can live past that point, depending on exactly when garbage and cycle collection run and who's holding on to what objects.

In your case, deleting the tables from nsLayoutStatics::Shutdown is safe because of the following two properties:

1)  The deletion will not try to run any "interesting" code in the sense of accessing XPCOM services.  It just cleans up the string and hashtable memory.

2)  The hashtables don't hold on to anything that can hold a reference to nsLayoutStatics.  This means that them being alive won't prevent nsLayoutStatics::Shutdown from being called.

If either of those conditions failed to hold, we would need to delete the hashtables from the observer notification instead.

Does that make sense?

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 9 years ago

Oh, and I guess the point is that using the observer approach is always OK, but it's more complicated to get it right.

	Tom Tung [:tt, :ttung] Assignee
	Comment 27 • 9 years ago

Hi Boris,

Sorry for my late reply. Thanks for your review and your feedback!
(In reply to Boris Zbarsky [:bz] from comment #26)
> Oh, and I guess the point is that using the observer approach is always OK,
> but it's more complicated to get it right.
I got it!! Thanks for resolving my questions!

In this patch, I addressed all the comments and I assigned deleted pointers to "nullptr" at the function |HandleShutdown| for safety.

Tom

	Tom Tung [:tt, :ttung] Assignee
	Comment 28 • 9 years ago

Update the patch due to bug 1241764. Replace nsPIDOMWindow with nsPIDOMWindowInner and remove a comment in this patch.

	Tom Tung [:tt, :ttung] Assignee
	Comment 29 • 9 years ago

Sorry for updating the patch again. Since the |BluetoothUUID.h| && |BluetoothUUIDBinding| will be bulit only when |MOZ_B2G_BT| is set, set the macro to check the flag is set or not in |nsLayloutStatics.cpp|.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cdc0103772f&filter-tier=1&filter-tier=2&filter-tier=3

	Tom Tung [:tt, :ttung] Assignee
	Comment 30 • 9 years ago

(In reply to Tom Tung from comment #29)
try: [1]

Most of failures can become success after retriggering them, the remaining failures are listed as following: 
  1. Gecko has failed to build in "Mulet OS X opt" and Mochitests(1, 3, 4, 5) in "Mulet Linux x64 opt" have failed at [2].
  2. All the mochitests at "B2G ICS Emulator opt & debug" are busted in many other jobs in Treeherder, such as [3] [4].
  3. Marionette WebAPI tests in "Mulet Linux x64 opt" and "B2G ICS Emulator opt" are failed in many other jobs, such as: [3] [4].

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce9c0d2b0d96&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=17385017
[2]: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b0d4a61fc0f0
[3]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d39c3be130ef&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=17370183
[4]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e4a230b5dcd&filter-tier=1&filter-tier=2&filter-tier=3

	Pulsebot
	Comment 31 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6ebde846f2d

	Carsten Book [:Tomcat]
	Comment 32 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c6ebde846f2d