Closed Bug 574120 Opened 10 years ago Closed 10 years ago

Electrolysis for Android

Categories

(Core :: IPC, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mwu, Assigned: mwu)

References

Details

Attachments

(3 files, 3 obsolete files)

No description provided.
Attached patch Add android support to ipc code (obsolete) — Splinter Review
Attachment #453538 - Flags: review?(jones.chris.g)
Accidentally changed timegm -> timegm64.
Attachment #453538 - Attachment is obsolete: true
Attachment #453542 - Flags: review?(jones.chris.g)
Attachment #453538 - Flags: review?(jones.chris.g)
Attachment #453565 - Flags: review?(benjamin)
Attachment #453574 - Flags: review?(blassey.bugs)
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.
Attachment #453565 - Attachment is obsolete: true
Attachment #453565 - Flags: review?(benjamin)
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.
Attachment #453542 - Flags: review?(jones.chris.g) → review+
Attachment #453574 - Flags: review?(blassey.bugs) → review+
Blocks: 574486
Attached patch Everything else (obsolete) — Splinter Review
Attachment #453920 - Flags: review?(doug.turner)
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"
Attachment #453920 - Flags: review?(doug.turner) → review-
(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.
Attachment #453920 - Attachment is obsolete: true
Attachment #453936 - Flags: review?(doug.turner)
Depends on: 563749
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. ;-)
Attachment #453936 - Flags: review?(doug.turner) → review+
Depends on: 574717
No longer depends on: 563749
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)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.