Status

()

Core
IPC
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mwu, Assigned: mwu)

Tracking

Trunk
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

8 years ago
Created attachment 453538 [details] [diff] [review]
Add android support to ipc code
Attachment #453538 - Flags: review?(jones.chris.g)
(Assignee)

Comment 2

8 years ago
Created attachment 453542 [details] [diff] [review]
Add android support to ipc code, v2

Accidentally changed timegm -> timegm64.
Attachment #453538 - Attachment is obsolete: true
Attachment #453542 - Flags: review?(jones.chris.g)
Attachment #453538 - Flags: review?(jones.chris.g)
(Assignee)

Comment 3

8 years ago
Created attachment 453565 [details] [diff] [review]
Add android electrolysis support to nsFrameLoader/TabChild
Attachment #453565 - Flags: review?(benjamin)
(Assignee)

Comment 4

8 years ago
Created attachment 453574 [details] [diff] [review]
Add electrolysis support to android embedding code
Attachment #453574 - Flags: review?(blassey.bugs)
(Assignee)

Comment 5

8 years ago
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+
(Assignee)

Updated

8 years ago
Blocks: 574486
(Assignee)

Comment 7

8 years ago
Created attachment 453920 [details] [diff] [review]
Everything else
Attachment #453920 - Flags: review?(doug.turner)

Comment 8

8 years ago
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-
(Assignee)

Comment 9

8 years ago
(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.
(Assignee)

Comment 10

8 years ago
Created attachment 453936 [details] [diff] [review]
Everything else, v2
Attachment #453920 - Attachment is obsolete: true
Attachment #453936 - Flags: review?(doug.turner)
(Assignee)

Updated

8 years ago
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+
(Assignee)

Updated

8 years ago
Depends on: 574717
(Assignee)

Updated

8 years ago
No longer depends on: 563749
(Assignee)

Comment 12

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.