574120 - Electrolysis for Android

Status: RESOLVED FIXED

(Core :: IPC, defect)

Comment 1

[Michael Wu [:mwu]]

Attachment: Add android support to ipc code

Comment 2

[Michael Wu [:mwu]]

Attachment: Add android support to ipc code, v2

Accidentally changed timegm -> timegm64.

Comment 3

[Michael Wu [:mwu]]

Attachment: Add android electrolysis support to nsFrameLoader/TabChild

Comment 4

[Michael Wu [:mwu]]

Attachment: Add electrolysis support to android embedding code

Comment 5

[Michael Wu [:mwu]]

Comment on attachment 453565 [details] [diff] [review]
Add android electrolysis support to nsFrameLoader/TabChild

I managed to remove a bit of critical code here that prevents things from crashing. Will post a new patch.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 453542 [details] [diff] [review]
Add android support to ipc code, v2

Shame on Mozilla for needing this, and Android for making it an unnecessary PITA.  But looks fine to me.

Comment 7

[Michael Wu [:mwu]]

Attachment: Everything else

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 453920 [details] [diff] [review]
Everything else


>+#elif defined(ANDROID)
>+#  warning IMPLEMENT ME
>+
>+  mChildProcess->SendcreateWidget(0);
> #elif defined(XP_MACOSX)
> #  warning IMPLEMENT ME


why the warning?

How will not know which window is the right context here?

>+#elif defined(XP_MACOSX) || defined (ANDROID)
> #  warning IMPLEMENT ME

better than an error, but don't we have to do something here?


>+#ifdef ANDROID
>+    baseWindow->InitWindow((void *)0x1234, 0, 0, 0, 0, 0);

I love magic.  I don't like magic constants.


> 
>+#elif defined(ANDROID)
>+#  warning This is a placeholder
>+typedef unsigned long MagicWindowHandle;
>+

# warning?

Comment?


> #include "mozilla/ipc/BrowserProcessSubThread.h"
>+#include "mozilla/Omnijar.h"
> #include <sys/stat.h>


wrap the #include with a OMNIJAR #ifdef
 


>+#ifdef MOZ_OMNIJAR
>+    nsCAutoString omnijarPath;
>+    if (mozilla::OmnijarPath())
>+      mozilla::OmnijarPath()->GetNativePath(omnijarPath);
>+    newEnvVars["OMNIJAR_PATH"] = omnijarPath.get();
>+#endif

good idea to add a comment here why this needs to be set in the env var. (e.g.  Setting this env var so that the child process can find it.  See nsAppRunner.cpp in ScopedXPCOMStartup::Initialize.)


Looks okay except the build time warnings.  I don't think you should spit out warnings for know issues.  Instead, add comments "XXX - Bug ##### - We need to fix this because blah"

Comment 9

[Michael Wu [:mwu]]

(In reply to comment #8)
> (From update of attachment 453920 [details] [diff] [review])
> 
> >+#elif defined(ANDROID)
> >+#  warning IMPLEMENT ME
> >+
> >+  mChildProcess->SendcreateWidget(0);
> > #elif defined(XP_MACOSX)
> > #  warning IMPLEMENT ME
> 
> 
> why the warning?
> 
> How will not know which window is the right context here?
> 
> >+#elif defined(XP_MACOSX) || defined (ANDROID)
> > #  warning IMPLEMENT ME
> 
> better than an error, but don't we have to do something here?
> 
> 
> >+#ifdef ANDROID
> >+    baseWindow->InitWindow((void *)0x1234, 0, 0, 0, 0, 0);
> 
> I love magic.  I don't like magic constants.
> 
The magic in the window handling here is because the InitWindow impl exits early if passed null. The android widget code mostly disables itself at runtime in the child process. As far as I can tell, this window doesn't really do much other than exist for the convenience of other code that expects it. I guess the real drawing is done elsewhere with the canvas..

> > #include "mozilla/ipc/BrowserProcessSubThread.h"
> >+#include "mozilla/Omnijar.h"
> > #include <sys/stat.h>
> 
> 
> wrap the #include with a OMNIJAR #ifdef
> 
mozilla/Omnijar.h is generally safe to include. If MOZ_OMNIJAR isn't set, it ifdef's itself out.

> >+#ifdef MOZ_OMNIJAR
> >+    nsCAutoString omnijarPath;
> >+    if (mozilla::OmnijarPath())
> >+      mozilla::OmnijarPath()->GetNativePath(omnijarPath);
> >+    newEnvVars["OMNIJAR_PATH"] = omnijarPath.get();
> >+#endif
> 
> good idea to add a comment here why this needs to be set in the env var. (e.g. 
> Setting this env var so that the child process can find it.  See
> nsAppRunner.cpp in ScopedXPCOMStartup::Initialize.)
> 
Sounds good.

> Looks okay except the build time warnings.  I don't think you should spit out
> warnings for know issues.  Instead, add comments "XXX - Bug ##### - We need to
> fix this because blah"
Ok will do.

Comment 10

[Michael Wu [:mwu]]

Attachment: Everything else, v2

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 453936 [details] [diff] [review]
Everything else, v2

I still have strong reservations about the "fake pointer", but if anything bad happens we can find this value pretty easily... and we know where you live. ;-)

Comment 12

[Michael Wu [:mwu]]

http://hg.mozilla.org/projects/electrolysis/rev/1727bfc11147 (android support for ipc)
http://hg.mozilla.org/projects/electrolysis/rev/8e4ceb3b4da5 (everything else)
http://hg.mozilla.org/mozilla-central/rev/549c24723dde (embedding ipc support)