Tzu-Lin Huang [:dwi2][:tzhuang] Reporter
	Description • 9 years ago

Currently in gecko we use hard-coded string as origin of bluetooth app.
See https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothCommon.h#216

However, in build other than phone build, we might need to use other app to do bluetooth pairing task. Or further more, bluetooth app might change its app origin string in the future.

Hence we need a flexible way to specify origin of bluetooth app. We could explore all possible ways in this bug.

One possible way is that we could use prefs and let gaia decide which app origin of bluetooth app we'd like to use in build time.

	Ben Tian (inactive)
	Comment 2 • 9 years ago

Comment on attachment 8645549 [details] [diff] [review]
Use pref instead of pre-defined C string as origin of bluetooth app. (v1)

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

Please see my comment below.

::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
@@ +984,3 @@
>    NS_ENSURE_TRUE(doc, false);
>  
>    uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;

Suggest to revise with guardian clauses as following:

  uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
  doc->NodePrincipal()->GetAppStatus(&appStatus);
  if (appStatus != nsIPrincipal::APP_STATUS_CERTIFIED) {
    return false;
  }

  // Get the app origin of Bluetooth app from PrefService.
  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
  if (!prefs) {
    BT_WARNING("Failed to get preference service");
    return false;
  }

  nsCString prefOrigin;
  nsresult rv = prefs->GetCharPref("dom.bluetooth.app-origin",
                                   getter_Copies(prefOrigin));
  if (NS_FAILED(rv)) {
    BT_WARNING("Failed to get the pref value 'dom.bluetooth.app-origin'");
    return false;
  }

  nsAutoCString appOrigin;
  doc->NodePrincipal()->GetOriginNoSuffix(appOrigin);

  return appOrigin.Equals(prefOrigin);

@@ +989,5 @@
>    doc->NodePrincipal()->GetAppStatus(&appStatus);
>    doc->NodePrincipal()->GetOriginNoSuffix(appOrigin);
>  
> +  // Get the app origin of Bluetooth app from PrefService.
> +  nsCString prefOrigin;

Should we use |nsAutoCString| here?

@@ +993,5 @@
> +  nsCString prefOrigin;
> +  nsresult rv = NS_ERROR_FAILURE;
> +  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
> +  if (prefs) {
> +    rv = prefs->GetCharPref("dom.bluetooth.app-origin",

Define dom.bluetooth.app-origin in BluetoothCommon.h

	Jamin Liu [:jaliu][:Jamin] Assignee
	Comment 3 • 9 years ago

Thank Ben for reviewing the patch.

	Jamin Liu [:jaliu][:Jamin] Assignee
	Comment 4 • 9 years ago

(In reply to Ben Tian [:btian] from comment #2)
> Comment on attachment 8645549 [details] [diff] [review]
> Use pref instead of pre-defined C string as origin of bluetooth app. (v1)
> 
> Review of attachment 8645549 [details] [diff] [review]:
> @@ +989,5 @@
> >    doc->NodePrincipal()->GetAppStatus(&appStatus);
> >    doc->NodePrincipal()->GetOriginNoSuffix(appOrigin);
> >  
> > +  // Get the app origin of Bluetooth app from PrefService.
> > +  nsCString prefOrigin;
> 
> Should we use |nsAutoCString| here?
Good idea. :)

	Ben Tian (inactive)
	Comment 5 • 9 years ago

Comment on attachment 8645616 [details] [diff] [review]
Use pref instead of pre-defined C string as origin of bluetooth app. (v2)

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

r=me with comment addressed.

::: dom/bluetooth/BluetoothCommon.h
@@ +209,5 @@
>   */
>  #define SYS_MSG_BT_PAIRING_REQ                "bluetooth-pairing-request"
>  
>  /**
> + * The preference name of app origin of bluetooth app. The default value is

Simplify to

  The preference name of bluetooth app origin. ...

::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp
@@ +996,5 @@
> +    return false;
> +  }
> +
> +  nsAutoCString prefOrigin;
> +  nsresult rv = prefs->GetCharPref("dom.bluetooth.app-origin",

Replace with |PREF_BLUETOOTH_APP_ORIGIN|.

@@ +999,5 @@
> +  nsAutoCString prefOrigin;
> +  nsresult rv = prefs->GetCharPref("dom.bluetooth.app-origin",
> +                                   getter_Copies(prefOrigin));
> +  if (NS_FAILED(rv)) {
> +    BT_WARNING("Failed to get the pref value 'dom.bluetooth.app-origin'");

Replace with |PREF_BLUETOOTH_APP_ORIGIN| as [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluetooth2/BluetoothService.cpp?offset=100#145

	Jamin Liu [:jaliu][:Jamin] Assignee
	Comment 6 • 9 years ago

- revise previous patch based on #comment 5.

Thank Ben for reviewing the patch.

	Jamin Liu [:jaliu][:Jamin] Assignee
	Comment 7 • 9 years ago

Pushed to try server.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=151e591cb22f

	Jamin Liu [:jaliu][:Jamin] Assignee
	Comment 8 • 9 years ago

Add two spaces in front of and behind |PREF_BLUETOOTH_APP_ORIGIN| since C++11 requires a space between literal and identifier.

	Jamin Liu [:jaliu][:Jamin] Assignee
	Comment 9 • 9 years ago

The result of try server looks fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=23a2b409627a

P.S. I believe the build break of |B2G Desktop OS X debug| is caused by Bug 1191877 and Bug 1165763.

	Pulsebot
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/44084e8ee8dc

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/44084e8ee8dc