978611 - Get rid of legacy dictionaries

Status: RESOLVED FIXED

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

Description

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Stop using legacy dictionaries from MmsMessage

This is the last user of legacy dictionaries.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Remove lagacy dictionaries

Comment 2

[Masatoshi Kimura [:emk]]

Try runs:
https://tbpl.mozilla.org/?tree=Try&rev=f6ee2f518736
https://tbpl.mozilla.org/?tree=Try&rev=438734657380

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8384314 [details] [diff] [review]
Part 1: Stop using legacy dictionaries from MmsMessage

Oh, you add something similar back... hmm.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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.)

Comment 7

[Masatoshi Kimura [:emk]]

(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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Masatoshi Kimura [:emk]]

Hm, according to bug 915368, MmsDeliveryInfo should be copyable (but MmsAttachment is not).

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Part 1: Stop using legacy dictionaries from MmsMessage

- Moved idl::MmsAttachment into MmsMessage.
- Removed idl::MmsDeliveryInfo because it was not needed.

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Remove lagacy dictionary from B2G

This is basically a dead code.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Get rid of legacy dictionaries: events and codegen

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 14

[Masatoshi Kimura [:emk]]

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.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 17

[Vicamo Yang [:vicamo][: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.

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Move lagacy dictionaries to WebIDL dictionaries for B2G

Also I have changed obscure jsvals to actual types.

Comment 20

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68c678368a08
https://hg.mozilla.org/integration/mozilla-inbound/rev/28938408b0e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/327b5e8a6708

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68c678368a08
https://hg.mozilla.org/mozilla-central/rev/28938408b0e9
https://hg.mozilla.org/mozilla-central/rev/327b5e8a6708