Closed Bug 834193 Opened 7 years ago Closed 7 years ago

Implement CFStateChangeEvent, DataErrorEvent, USSDReceivedEvent using codegenerator

Categories

(Core :: DOM: Events, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Duplicate of this bug: 763326
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → vyang
Attachment #705914 - Flags: review?(bugs)
Attachment #705914 - Flags: review?(anygregor)
Comment on attachment 705914 [details] [diff] [review]
v1

>diff --git a/dom/base/nsDOMClassInfo.cpp b/dom/base/nsDOMClassInfo.cpp
...
> #ifdef MOZ_B2G_RIL
> #include "nsIDOMMobileConnection.h"
>+#include "nsIDOMUSSDReceivedEvent.h"
>+#include "nsIDOMDataErrorEvent.h"
Actually, you shouldn't need these #includes.
nsDOMClassInfo.cpp defines MOZ_GENERATED_EVENTS_INCLUDES and then #include "GeneratedEvents.h"
so the interfaces for the generated events are included automatically.
(you can see GeneratedEvents.h in <objdir>/js/xpconnect/src/GeneratedEvents.h)
Attachment #705914 - Flags: review?(bugs) → review+
Blocking this per comment #32 in bug 827280.
Depends on: 827280
Rewrite CFStateChangeEvent as well after bug 827280.
Summary: Implement DataErrorEvent, USSDReceivedEvent using codegenerator → Implement CFStateChangeEvent, DataErrorEvent, USSDReceivedEvent using codegenerator
Attached patch v2 (obsolete) — Splinter Review
1) Address review comment in comment #3.
2) Rewrite CFStateChangeEvent as well
Attachment #705914 - Attachment is obsolete: true
Attachment #705914 - Flags: review?(anygregor)
Attachment #706849 - Flags: review?(bugs)
Comment on attachment 706849 [details] [diff] [review]
v2

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

::: js/xpconnect/src/event_impl_gen.conf.in
@@ +32,5 @@
>      'MozWifiStatusChangeEvent',
>      'MozWifiConnectionInfoEvent',
>      'MozCellBroadcastEvent',
> +    'USSDReceivedEvent',
> +    'DataErrorEvent',

Should these two actually be in a MOZ_B2G_RIL ifdef?
Comment on attachment 706849 [details] [diff] [review]
v2

Yes, USSDReceivedEvent and DataErrorEvent should e b2g only stuff, but 
very unfortunately they are already exposed in desktop (release) builds.
Please file a followup to fix that and make it block bug Bug 734145.
This bug could be just about using codegenerator for various events.

So, update uuid of nsIDOMCFStateChangeEvent and keep the old behavior
of USSDReceivedEvent and DataErrorEvent where they are exposed to desktop.
Attachment #706849 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #8)
> Yes, USSDReceivedEvent and DataErrorEvent should e b2g only
> stuff, but very unfortunately they are already exposed in
> desktop (release) builds. Please file a followup to fix that
> and make it block bug Bug 734145. This bug could be just about
> using codegenerator for various events.

Hei, I've already moved all the three events in MOZ_B2G_RIL in this patch. Is there still anything other than uuid update of nsIDOMCFStateChangeEvent to be done?

> So, update uuid of nsIDOMCFStateChangeEvent and keep the old
> behavior of USSDReceivedEvent and DataErrorEvent where they
> are exposed to desktop.

Same above. Is it still necessary to keep old behaviour?
Well, it is a change to behavior in desktop build.
Event if those events aren't really usable in desktop build, one can detect them.
"DataErrorEvent" in window is true.

At least get a review from someone who reviewed Bug 734145.
Attached patch v3 (obsolete) — Splinter Review
Update uuid of nsIDOMCFStateChangeEvent only. As review comment #10 said, maybe Bent can help give some feedbacks on removing USSDReceivedEvent and DataErrorEvent from destkop build?
Attachment #706849 - Attachment is obsolete: true
Attachment #706867 - Flags: review?(bent.mozilla)
Comment on attachment 706867 [details] [diff] [review]
v3

Mounir, do you mind have a review on these changes?
Attachment #706867 - Flags: review?(bent.mozilla) → review?(mounir)
Comment on attachment 706867 [details] [diff] [review]
v3

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

Olli should review that. Sorry for the delay Vicamo.
Attachment #706867 - Flags: review?(mounir) → review?(bugs)
Comment on attachment 706867 [details] [diff] [review]
v3

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

Oh... He already reviewed it. Just land that then. His review his enough ;)
Attachment #706867 - Flags: review?(bugs)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> Created attachment 706867 [details] [diff] [review]
> v3
> 
> Update uuid of nsIDOMCFStateChangeEvent only. As review comment #10 said,
> maybe Bent can help give some feedbacks on removing USSDReceivedEvent and
> DataErrorEvent from destkop build?

I do not think there would be any problem with that. r=me if you need.
(In reply to Mounir Lamouri (:mounir) from comment #13)
> Sorry for the delay Vicamo.

You don't have to. I know you probably get a ton of review requests :)
Attached patch v4 (obsolete) — Splinter Review
Rebase only. Try https://tbpl.mozilla.org/?tree=Try&rev=acb0585c406c
Attachment #710522 - Flags: review+
Attachment #706867 - Attachment is obsolete: true
Attached patch v5Splinter Review
Rebase after bug 835148
Attachment #710522 - Attachment is obsolete: true
Attachment #711231 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/26f2a61829ac
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.