Last Comment Bug 761729 - Either implement mozIDOMApplicationEvent in C++, or make WebApps use CustomEvent
: Either implement mozIDOMApplicationEvent in C++, or make WebApps use CustomEvent
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla16
Assigned To: [:fabrice] Fabrice Desré
:
Mentors:
: 765287 (view as bug list)
Depends on:
Blocks: 761613
  Show dependency treegraph
 
Reported: 2012-06-05 11:46 PDT by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2013-04-04 13:53 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (14.05 KB, patch)
2012-06-14 11:48 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Review
patch v2 (12.87 KB, patch)
2012-06-14 14:40 PDT, [:fabrice] Fabrice Desré
bugs: review-
Details | Diff | Review
patch v3 (13.31 KB, patch)
2012-06-14 15:12 PDT, [:fabrice] Fabrice Desré
bugs: review+
Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-05 11:46:03 PDT
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).
Comment 1 [:fabrice] Fabrice Desré 2012-06-14 11:48:03 PDT
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
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-14 12:00:14 PDT
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.
Comment 3 [:fabrice] Fabrice Desré 2012-06-14 12:07:11 PDT
(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.
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-14 12:16:32 PDT
Couldn't you add mozIDOMApplication to exclude_automatic_type_include
and perhaps nsIDOMApplicationRegistry.h to special_includes
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-14 12:16:58 PDT
...in dictionary_helper_gen.conf
Comment 6 [:fabrice] Fabrice Desré 2012-06-14 12:26:57 PDT
(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!
Comment 7 [:fabrice] Fabrice Desré 2012-06-14 14:40:08 PDT
Created attachment 633285 [details] [diff] [review]
patch v2

Updated to use exclude_automatic_type_include.
Comment 8 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-14 14:54:26 PDT
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.)
Comment 9 [:fabrice] Fabrice Desré 2012-06-14 15:12:33 PDT
Created attachment 633297 [details] [diff] [review]
patch v3

Now with cycle collection for mApplication.
Comment 10 [:fabrice] Fabrice Desré 2012-06-14 16:04:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6927b157cb3
Comment 11 Ed Morley [:emorley] 2012-06-15 05:57:31 PDT
https://hg.mozilla.org/mozilla-central/rev/b6927b157cb3
Comment 12 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-15 10:08:38 PDT
*** Bug 765287 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.