Closed
Bug 865207
Opened 12 years ago
Closed 11 years ago
Move webapps actor from /b2g/chrome/content to /toolkit/devtools/apps/
Categories
(DevTools :: Debugger, defect, P2)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 24
People
(Reporter: ochameau, Assigned: ochameau)
Details
Attachments
(1 file, 7 obsolete files)
(deleted),
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
The Web apps actor shouldn't be specific to b2g so that it also works on Desktop and Fennec.
If we want to build a new remote devtool to manage apps on all platform,
we should start asap moving the actor to a place where we can load it from all platform and also start tuning it to make it work nicely everywhere.
This bug is only about moving the actor, followup bugs will be required to make it work per platform.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Comment on attachment 743663 [details] [diff] [review]
Move webapps actor from /b2g/chrome/content to /toolkit/devtools/apps/
Patch on top of attachment 743634 [details] [diff] [review] from bug 844227.
The xpcshell test already pass on Desktop, need to give it a try on Fennec.
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #746531 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #743663 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #746533 -
Attachment is obsolete: true
Assignee | ||
Comment 6•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #748895 -
Attachment is obsolete: true
Updated•12 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #750070 -
Attachment is obsolete: true
Assignee | ||
Comment 8•12 years ago
|
||
Comment on attachment 752705 [details] [diff] [review]
Move webapps actor from /b2g/chrome/content to /toolkit/devtools/apps/
After many random issues to make this patch have working xpcshell tests on desktop and b2g platform... the patch is now ready!
So I'm moving the actor and its related test to devtools folder.
I also registered this actor as other ones in DebuggerServer.addBrowserActors, but try to load it lazily (at least its JSM dependencies, but I think we should provide something in dbg-server to load the actor JS file on demand).
Attachment #752705 -
Flags: review?(fabrice)
Attachment #752705 -
Flags: review?(dcamp)
Updated•12 years ago
|
Attachment #752705 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #752705 -
Attachment is obsolete: true
Attachment #752705 -
Flags: review?(dcamp)
Assignee | ||
Comment 10•12 years ago
|
||
Comment on attachment 752880 [details] [diff] [review]
Move webapps actor from /b2g/chrome/content to /toolkit/devtools/apps/
Sorry fabrice, I realized lately that my patch wasn't applying to master due to recent devtool mass refactoring...
Here is a new patch that work on last m-c changeset.
The main difference is that I had to remove all #ifdef as the new devtools layout isn't supporting preprocessing anymore.
So I mainly replaced various FileUtils.getDir(DIRRECTORY_NAME,...) with either DOMApplicationRegistry.getWebAppsBasePath() or DOMApplicationRegistry._getAppDir()
And I also had to move the #ifdef MOZ_OFFICIAL from the actor to webapps.jsm.
By the way, are you sure MOZ_OFFICIAL is the right env variable name to use?
I haven't been able to see any usage of it in m-c, nor b2g.
There is MOZ_OFFICIAL_BRANDING, but not just MOZ_OFFICIAL.
Attachment #752880 -
Flags: review?(fabrice)
Attachment #752880 -
Flags: review?(dcamp)
Assignee | ||
Comment 11•12 years ago
|
||
Comment 12•12 years ago
|
||
Comment on attachment 752880 [details] [diff] [review]
Move webapps actor from /b2g/chrome/content to /toolkit/devtools/apps/
Review of attachment 752880 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with comments addressed.
::: dom/apps/src/Webapps.jsm
@@ +83,5 @@
> appsFile: null,
> webapps: { },
> children: [ ],
> allAppsLaunchable: false,
> +#ifdef MOZ_OFFICIAL
You're right about using MOZ_OFFICIAL_BRANDING instead. MOZ_OFFICIAL is from gaia...
@@ +86,5 @@
> allAppsLaunchable: false,
> +#ifdef MOZ_OFFICIAL
> + get allowInstallingCertified() false,
> +#else
> + get allowInstallingCertified() true,
I would name that allowSideloadingCertified
Updated•12 years ago
|
Attachment #752880 -
Flags: review?(fabrice) → review+
Comment 13•12 years ago
|
||
Comment on attachment 752880 [details] [diff] [review]
Move webapps actor from /b2g/chrome/content to /toolkit/devtools/apps/
Review of attachment 752880 [details] [diff] [review]:
-----------------------------------------------------------------
::: b2g/chrome/content/shell.js
@@ +1012,5 @@
> if ("nsIProfiler" in Ci) {
> DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/profiler.js");
> }
> DebuggerServer.addActors("resource://gre/modules/devtools/server/actors/styleeditor.js");
> DebuggerServer.addActors('chrome://browser/content/dbg-browser-actors.js');
Don't you still want webapps actors on b2g? Where is it added now?
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 752880 [details] [diff] [review]
Move webapps actor from /b2g/chrome/content to /toolkit/devtools/apps/
Review of attachment 752880 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/devtools/server/main.js
@@ +183,5 @@
> if ("nsIProfiler" in Ci)
> this.addActors("resource://gre/modules/devtools/server/actors/profiler.js");
>
> this.addActors("resource://gre/modules/devtools/server/actors/styleeditor.js");
> + this.addActors("resource://gre/modules/devtools/server/actors/webapps.js");
dcamp: ^-- here!
The actor is now registered on all platforms.
It merely works on all platform, but needs some improvements on Android and Desktop to make it work nicely. Like ensuring registering intent on Android and start introducing webapps features in desktop (so far everything is made for webapprt, where apps are running in another process).
Comment 15•12 years ago
|
||
As of just recently, b2g doesn't reuse addBrowserActors. So you'll need to add that same line to shell.js too.
Comment 16•12 years ago
|
||
Comment on attachment 752880 [details] [diff] [review]
Move webapps actor from /b2g/chrome/content to /toolkit/devtools/apps/
... but r+ with that change. Thanks!
Attachment #752880 -
Flags: review?(dcamp) → review+
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #752880 -
Attachment is obsolete: true
Assignee | ||
Comment 18•12 years ago
|
||
Comment on attachment 755998 [details] [diff] [review]
Move webapps actor from /b2g/chrome/content to /toolkit/devtools/apps/ r=fabrice r=dcamp
Review comments addressed, rebase against last master, tested against b2g and fennec.
Ready to land!
Attachment #755998 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 19•12 years ago
|
||
Comment 20•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•