Closed Bug 978611 Opened 6 years ago Closed 6 years ago

Get rid of legacy dictionaries

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(3 files, 3 obsolete files)

This is the last user of legacy dictionaries.
Attachment #8384314 - Flags: review?(bugs)
Attachment #8384315 - Flags: review?(bugs)
Comment on attachment 8384314 [details] [diff] [review]
Part 1: Stop using legacy dictionaries from MmsMessage

> MmsMessage::MmsMessage(int32_t                          aId,
>                        uint64_t                         aThreadId,
>                        const nsAString&                 aIccId,
>                        DeliveryState                    aDelivery,
>-                       const nsTArray<MmsDeliveryInfo>& aDeliveryInfo,
>+                       const nsTArray<idl::MmsDeliveryInfo>& aDeliveryInfo,
You're removing idl::MmsDeliveryInfo and yet use it here explicitly.
I must be missing something.
Attachment #8384314 - Flags: review?(bugs) → review-
Comment on attachment 8384314 [details] [diff] [review]
Part 1: Stop using legacy dictionaries from MmsMessage

Oh, you add something similar back... hmm.
Attachment #8384314 - Flags: review- → review?(bugs)
Comment on attachment 8384314 [details] [diff] [review]
Part 1: Stop using legacy dictionaries from MmsMessage

But don't like this setup. We still effectively use those dictionaries, 
they just aren't codegen'ed.

Why can't we use mozilla::dom::Mms* dictionaries?
Attachment #8384314 - Flags: review?(bugs) → review-
Comment on attachment 8384315 [details] [diff] [review]
Part 2: Remove lagacy dictionaries

Could you please split this to two.
EventInit stuff and codegen removal in one patch and then
b2g specific stuff like all the nsIDOMMobileConnection & co changes in
one patch, since I think someone more familiar with b2g needs to approve 
those changes (although they should be just fine.)
Attachment #8384315 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 8384314 [details] [diff] [review]
> Part 1: Stop using legacy dictionaries from MmsMessage
> 
> But don't like this setup. We still effectively use those dictionaries, 
> they just aren't codegen'ed.

I think there's no point keeping legacy codegen only for two structures. Also, .Init() stuff was moved to WebIDL codegen.

> Why can't we use mozilla::dom::Mms* dictionaries?

Because WebIDL dictionaries are non-copyable. So I had to define something private to hold data.
Well, at least don't use mozilla::idl,  but either mozilla or mozilla::dom namespace and name
those struct to be something different than what we have in webidl.
Hm, according to bug 915368, MmsDeliveryInfo should be copyable (but MmsAttachment is not).
- Moved idl::MmsAttachment into MmsMessage.
- Removed idl::MmsDeliveryInfo because it was not needed.
Attachment #8384314 - Attachment is obsolete: true
Attachment #8384580 - Flags: review?(vyang)
This is basically a dead code.
Attachment #8384315 - Attachment is obsolete: true
Attachment #8384581 - Flags: review?(vyang)
Comment on attachment 8384582 [details] [diff] [review]
Part 3: Get rid of legacy dictionaries: events and codegen

I wouldn't mind if khuey takes a look at the xpidl.py changes.
They look ok to me.
Attachment #8384582 - Flags: review?(bugs) → review+
Comment on attachment 8384582 [details] [diff] [review]
Part 3: Get rid of legacy dictionaries: events and codegen

Could you take a look xpidl.py part?
It is just a backout of bug 721569.
Attachment #8384582 - Flags: review?(khuey)
Comment on attachment 8384580 [details] [diff] [review]
Part 1: Stop using legacy dictionaries from MmsMessage

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

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +104,5 @@
>    for (uint32_t i = 0; i < len; i++) {
>      MmsDeliveryInfo info;
>      const MmsDeliveryInfoData &infoData = aData.deliveryInfo()[i];
>  
>      // Prepare |info.receiver|.

nit: mReceiver

@@ +137,2 @@
>  
>      // Prepare |info.deliveryTimestamp|.

ditto

@@ +140,2 @@
>  
>      // Prepare |info.readStatus|.

ditto

@@ +161,2 @@
>  
>      // Prepare |info.readTimestamp|.

ditto

::: js/xpconnect/src/dictionary_helper_gen.conf
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  # Dictionary interface name, interface file name
>  dictionaries = [
> +     [ 'SmsThreadListItem', 'nsIMobileMessageCallback.idl' ]

We don't need this anymore since bug 849739.  It's safe to remove it as well.
Attachment #8384580 - Flags: review?(vyang) → review+
Comment on attachment 8384581 [details] [diff] [review]
Part 2: Remove lagacy dictionary from B2G

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

::: dom/icc/interfaces/SimToolKit.idl
@@ -1,1 @@
> -/* This Source Code Form is subject to the terms of the Mozilla Public

Hi, I think we can move all the definitions here into "dom/webidl/MozStkCommandEvent.webidl" and comment them out.  Two reasons I really want to keep them: 1) the documentation in this file still means a lot especially as a reference to STK command/response binding, 2) we may still need some of them when we convert ICC to webidl/ipdl.

::: dom/mobileconnection/interfaces/nsIDOMMobileConnection.idl
@@ -697,5 @@
>    readonly attribute unsigned short serviceClass;
>  };
> -
> -
> -dictionary MozCallBarringOption

Please move to dom/webidl/MozMobileConnection.webidl and comment it out.  We'll need it in bug 898445 - converting mozMobileConnection to webidl and probably bug 889737.

@@ -726,5 @@
> -   */
> -  unsigned short serviceClass;
> -};
> -
> -dictionary DOMMMIResult

ditto. We'll probably never have to instantiate a DOMMMIResult instance, but the documentation here is still precious for me.

@@ -750,5 @@
> -   */
> -  jsval additionalInformation;
> -};
> -
> -dictionary DOMCLIRStatus

ditto.

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ -9,5 @@
>  /**
> - * Provide the detailed options for specifying different kinds of data filtering
> - * in getSamples function.
> - */
> -dictionary NetworkStatsGetOptions

Please move to "dom/webidl/MozNetworkStats.webidl". Used when converting mozNetworkStats to webidl.

@@ -35,5 @@
>     */
>    readonly attribute DOMString id;
>  };
>  
> -dictionary NetworkStatsAlarmOptions

ditto.
Attachment #8384581 - Flags: review?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> ::: js/xpconnect/src/dictionary_helper_gen.conf
> @@ +3,5 @@
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> >  # Dictionary interface name, interface file name
> >  dictionaries = [
> > +     [ 'SmsThreadListItem', 'nsIMobileMessageCallback.idl' ]
> 
> We don't need this anymore since bug 849739.  It's safe to remove it as well.

Seems it has been removed in the 2nd part.  Thank you.
Also I have changed obscure jsvals to actual types.
Attachment #8384581 - Attachment is obsolete: true
Attachment #8386414 - Flags: review?(vyang)
Attachment #8386414 - Flags: review?(vyang) → review+
Duplicate of this bug: 895397
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.