Either implement mozIDOMApplicationEvent in C++, or make WebApps use CustomEvent

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: smaug, Assigned: fabrice)

Tracking

unspecified
mozilla16
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#399

DOMEvents will be forced to be C++ only. Right now properly implemented DOM Events
are already C++ only, but apparently there is at least one JS implementation too
(which doesn't work at all with event dispatching code).

Updated

5 years ago
Component: Web Apps → DOM: Mozilla Extensions
Product: Firefox → Core
QA Contact: webapps → general
(Assignee)

Comment 1

5 years ago
Created attachment 633215 [details] [diff] [review]
patch

I had to refactor the IDL to move the Application object definition in its own file since it is use in the dictionnary, hence the include change to nsGlobalWindow.h
Assignee: nobody → fabrice
Attachment #633215 - Flags: review?(bugs)
(Reporter)

Comment 2

5 years ago
Comment on attachment 633215 [details] [diff] [review]
patch


>+[scriptable, uuid(b70b84f1-7ac9-4a92-bc32-8b6a7eb7879e)]
>+interface mozIDOMApplication  : nsISupports
Nit, extra space before :


>+{
>+  readonly attribute jsval manifest;
>+  readonly attribute DOMString manifestURL;
>+  readonly attribute nsIArray receipts; /* an array of strings */
Not about this bug, but huh. Why do we expose nsIArray to web. We really shouldn't do that.


But I don't understand why you need to move Application interface.
(Assignee)

Comment 3

5 years ago
(In reply to Olli Pettay [:smaug] from comment #2)

> Not about this bug, but huh. Why do we expose nsIArray to web. We really
> shouldn't do that.

Yep, I plan to move that to a jsval.

> 
> But I don't understand why you need to move Application interface.

since the dictionnary is :
{
 mozIDOMApplication application;
}

and mozIDOMApplication is not a built-in type, the code generator that uses dictionary_helper_gen.conf looks for a "mozIDOMApplication.h" header file.
(Reporter)

Comment 4

5 years ago
Couldn't you add mozIDOMApplication to exclude_automatic_type_include
and perhaps nsIDOMApplicationRegistry.h to special_includes
(Reporter)

Comment 5

5 years ago
...in dictionary_helper_gen.conf
(Assignee)

Comment 6

5 years ago
(In reply to Olli Pettay [:smaug] from comment #5)
> ...in dictionary_helper_gen.conf

I didn't know about these. I'll update my patch, thanks!
(Assignee)

Comment 7

5 years ago
Created attachment 633285 [details] [diff] [review]
patch v2

Updated to use exclude_automatic_type_include.
Attachment #633215 - Attachment is obsolete: true
Attachment #633215 - Flags: review?(bugs)
Attachment #633285 - Flags: review?(bugs)
(Reporter)

Comment 8

5 years ago
Comment on attachment 633285 [details] [diff] [review]
patch v2

>+nsDOMMozApplicationEvent::GetApplication(mozIDOMApplication **aApplication)
Nit, mozIDOMApplication **aApplication


>+private:
>+  nsCOMPtr<mozIDOMApplication> mApplication;
Almost missed... you have strong pointer to Application, so you need to add it to
cycle collection. (You can check nsDOMCustomEvent as an example. It has mDetail as a strong member variable.)
Attachment #633285 - Flags: review?(bugs) → review-
(Assignee)

Comment 9

5 years ago
Created attachment 633297 [details] [diff] [review]
patch v3

Now with cycle collection for mApplication.
Attachment #633285 - Attachment is obsolete: true
Attachment #633297 - Flags: review?(bugs)
(Reporter)

Updated

5 years ago
Attachment #633297 - Flags: review?(bugs) → review+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6927b157cb3
https://hg.mozilla.org/mozilla-central/rev/b6927b157cb3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Duplicate of this bug: 765287
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.