1174071 - [bluetooth2] Remove 'required' keyword for Bluetooth*EventInit dictionary members.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Jocelyn Liu [:jocelyn] [:joliu]]

Currently mochitest-3 will fail since we didn't provide 'required' members in the eventInit for some bluetooth events, for example, BluetoothPairingEvent.
And some of these members cannot be constructed by JS, so we cannot assign a valid value in the mochitest.
According to Olli's suggestion in Bug 1167064 Comment 26, we should remove those required limitation for these members.
As I stated in Bug 1167064 Comment 27, I don't see strong reasons that we need to make this restriction for web pages and especially we cannot restrict it for some cases.
IMO, it's reasonable to remove all 'required' members in our eventInit dictionaries in our case.

Comment 1

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1174071 - Remove 'required' keyword for Bluetooth*EventInit dictionary members.

Comment 2

[Ben Tian (inactive)]

Comment on attachment 8621483 [details] [diff] [review]
Bug 1174071 - Remove 'required' keyword for Bluetooth*EventInit dictionary members.

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

r=me with comment addressed.

::: dom/bluetooth/bluetooth2/BluetoothLeDeviceEvent.cpp
@@ +95,5 @@
> +  const uint8_t* data = nullptr;
> +  size_t length = 0;
> +
> +  if (aEventInitDict.mScanRecord.WasPassed()) {
> +    const auto& a = aEventInitDict.mScanRecord.Value();

Replace |a| with a meaningful name instead.

::: dom/webidl/BluetoothLeDeviceEvent.webidl
@@ +20,1 @@
>    short  rssi = 0;

nit: remove extra space.

Comment 3

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1174071 - Remove 'required' keyword for Bluetooth*EventInit dictionary members. r=btian

Hi Boris,

Would it be OK for asking your help to review my patch?
It's came from the question I asked in Bug 1167064.
If you're not available for doing reviews, please feel free to let me know.

Thanks,
Jocelyn

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8621524 [details] [diff] [review]
Bug 1174071 - Remove 'required' keyword for Bluetooth*EventInit dictionary members. r=btian

r=me assuming creating an empty array buffer with null data pointer does the right thing.  Please add some tests for these constructors!

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Not doing reviews right now from comment #4)
> Comment on attachment 8621524 [details] [diff] [review]
> Bug 1174071 - Remove 'required' keyword for Bluetooth*EventInit dictionary
> members. r=btian
> 
> r=me assuming creating an empty array buffer with null data pointer does the
> right thing.  Please add some tests for these constructors!

Hi Boris,

Thanks for your comment, I revisited this part, and I think using ArrayBuffer? here might be more reasonable.
I am thinking of revising the constructor of LeDeviceEvent to only create array buffer if it is mScanRecord is not null.
How does this sound to you?

About the test, doesn't test_all_synthetic_events.html test these constructors?

Besides this, in bluetooth component, we use marionette tests to test the flow from enable/disable BT to pairing with devices, which will fire |BluetoothPairingEvent| and |BluetoothAttributeEvent| during the test flow, but we have not yet cover |LeDeviceEvent| and |GattCharacteristicEvent|.

Thanks,
Jocelyn

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #5)
> 
> Hi Boris,
> 
> Thanks for your comment, I revisited this part, and I think using
> ArrayBuffer? here might be more reasonable.
> I am thinking of revising the constructor of LeDeviceEvent to only create
> array buffer if it is mScanRecord is not null.
typo here, sorry, I mean 'only create the array buffer if mScanRecord is not null'.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> How does this sound to you?

I don't have a strong opinion, because I'm not sure how this API is used in practice.  But yes, making this nullable with default null would certainly work. 

> doesn't test_all_synthetic_events.html test these constructors

Well, it tests that you can run them.  It doesn't test anything about what the result looks like, right?  e.g. it would not check that you in fact got an empty arraybuffer.

Comment 8

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Not doing reviews right now from comment #7)
> > How does this sound to you?
> 
> I don't have a strong opinion, because I'm not sure how this API is used in
> practice.  But yes, making this nullable with default null would certainly
> work. 
> 
> > doesn't test_all_synthetic_events.html test these constructors
> 
> Well, it tests that you can run them.  It doesn't test anything about what
> the result looks like, right?  e.g. it would not check that you in fact got
> an empty arraybuffer.

Yes, you were right, thanks for the explanation.
I suppose this would probably be done by adding test items in test_eventctors.html?
Would it be OK to split this part into a followup bug?

Thanks,
Jocelyn

Comment 9

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #8)
> (In reply to Not doing reviews right now from comment #7)
> > > How does this sound to you?
> > 
> > I don't have a strong opinion, because I'm not sure how this API is used in
> > practice.  But yes, making this nullable with default null would certainly
> > work. 
> > 
> > > doesn't test_all_synthetic_events.html test these constructors
> > 
> > Well, it tests that you can run them.  It doesn't test anything about what
> > the result looks like, right?  e.g. it would not check that you in fact got
> > an empty arraybuffer.
> 
> Yes, you were right, thanks for the explanation.
> I suppose this would probably be done by adding test items in
> test_eventctors.html?
> Would it be OK to split this part into a followup bug?
> 
> Thanks,
> Jocelyn

The reason I would like to do it in a followup bug is because we didn't cover all BT events in this bug.
I would like to file a bug to add all BT events into test_eventctors.html.

Use ni here in case you didn't see the bug mail.

Thanks,
Jocelyn

Comment 10

[Boris Zbarsky [:bzbarsky]]

Followup bug is fine.

Comment 11

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1174071 - Remove 'required' keyword for Bluetooth*EventInit dictionary members. r=btian, r=bz

- Make "LeDeviceEvent.scanRecord" nullable.
- Bug 1174312 has been filed for testing event constructions.

Thanks for your time, Ben and Boris.

Comment 12

[Jocelyn Liu [:jocelyn] [:joliu]]

No try server result since bluetooth2 won't be build on try server.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/78a1a93d3904

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78a1a93d3904

Comment 15

[Maria Angeles Oteo (:oteo)]

As NGA Program Manager suggested, let's replace the NGA-Sx milestones with FxOS-Sx ones (more generic ones), once Bug 1174794 has already landed