If you search for "Remote debugging Firefox OS" you'll end up at [1], which tells you to use the App Manager. However, as the name implies, it seems like this tool is really for debugging Firefox OS apps, not web pages or web apps. It would be nice if there was a way to debug webpages in the browser via the App Manager. (Searching through this component I found Bug 965047 which taught me how to debug certified apps (via [2]). This works... but only if you're interested in remote debugging the browser app's chrome, etc., not a webpage displayed in the browser. The actual page is in an iframe that you can't inspect/debug: https://cloudup.com/cit9XFIDigk) [1] <https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging#Firefox_OS> [2] <https://developer.mozilla.org/en-US/Firefox_OS/Using_the_App_Manager#Debugging_Certified_Apps>

The only way I know how to debug JS on an actual Firefox OS device is to connect to an app (Marketplace, for example) and manually change "location.href" via the console. This _kind of_ works, but the viewport is obviously not right. You can also manually edit /etc/hosts and point to a proxy serving local content but that's pretty painful. If <chrome://browser/content/devtools/connect.xhtml> is supposed to work with Firefox OS, maybe I need to open a bug on that instead?

I'd love to see this!

With Alex's frame switching work, it's now possible to debug regular websites on FxOS! 1. Enable certified debugging 2. Attach to the System app (the separate Browser app is no longer used) 3. In the toolbox options, (gear icon) be sure that the "Select an iframe..." toolbox button is enabled 4. If you have a webpage open, you should see it one of the iframes in the frame list So that's quite cool! However, we should really expose these pages as the tab list the debugger expects, and also they should be accessible without certified debugging.

Ahah I wasn't expecting that. Also this is bit unexpected as the frame should be living in another process?!? Do they? Are we lucky with some CPOW?! Anyway, it may be easier to fix the original issue than figure this out...

(In reply to Alexandre Poirot [:ochameau] from comment #5) > Ahah I wasn't expecting that. Also this is bit unexpected as the frame > should be living in another process?!? > Do they? Are we lucky with some CPOW?! > > Anyway, it may be easier to fix the original issue than figure this out... Haha, I am not quite sure how it's possible... But I've spent a little time looking at the original problem anyway, so I may see how far I get.

Well, maybe I was cheating... I tried this in a simulator, which is only one process. :kats tried it on device (from IRC) and they did not see the frames. So, perhaps that explains things a bit.

I've got this working on device locally, will attach patches tomorrow.

Similar to AppFrames.jsm, I made BrowserFrames to list and observe Gaia's browser frames.

Here I add the tab list actor support. I'm using the message manager to keep the parent process "shim" for a given tab updated, as previously it would only record the title and URL on the first tab list iteration and never update it. f? only for now, as I have no tests yet.

Comment on attachment 8492503 [details] [diff] [review] Part 1: Access browser frames Review of attachment 8492503 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure it is worth duplicating AppFrames.jsm. You may remember that when I introduced AppFrames.jsm I named it Frames.jsm as I knew we would be interested in other frames at some point. I will poke vivien about that, as I think I'm not reviewer of this code? ::: b2g/components/SystemAppProxy.jsm @@ +144,5 @@ > + return []; > + } > + > + let frames = systemAppFrame.contentDocument > + .querySelectorAll(".browser iframe[mozbrowser]"); I think it would be safier to do "iframe:not([mozapp])"

Comment on attachment 8492504 [details] [diff] [review] Part 2: B2G tab list actors Looks great!

Comment on attachment 8492503 [details] [diff] [review] Part 1: Access browser frames Vivien, any thoughts on this? Would you prefer I just extend the existing AppFrames.jsm?

Comment on attachment 8492503 [details] [diff] [review] Part 1: Access browser frames Review of attachment 8492503 [details] [diff] [review]: ----------------------------------------------------------------- Too much duplicated code imo. It will likely end up out of sync and will ask more work to be maintained.

I've merged browser frames into the app frames observer, and moved it back to Frames.jsm.

This is unchanged from the last version, other than to update for the Frames API changes in part 1. Should I add a test using an approach like your app frames mock? Should it live in b2g or toolkit/devtools? Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=72e4e8d6450d

Fixed many test errors on desktop. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=560d131743c2

Comment on attachment 8496026 [details] [diff] [review] Part 1: Access browser frames (v2) Review of attachment 8496026 [details] [diff] [review]: ----------------------------------------------------------------- I think I can live with these small duplications highlighted in next comments, but I quite like the idea of having a very generic Frames.jsm module that just exposes all frames. ::: b2g/components/Frames.jsm @@ +123,5 @@ > + e.stack + '\n'); > + } > + }); > + return; > + } Instead of duplicating this code I would call only one callback, onFrameDestroyed and let the callback figure out if it consider to given frame or not. It would require tuning each callsite of Frames... but I think it will be globally easier. Simplier implementation of this module and also easier to track any kind of frame. @@ +146,4 @@ > > + listApps: () => SystemAppProxy.getAppFrames(), > + > + listBrowsers: () => SystemAppProxy.getBrowserFrames(), Same thing here, we could just expose all frame and let the caller figure out which frame it takes care of. ::: b2g/components/SystemAppProxy.jsm @@ +149,5 @@ > + let list = []; > + for (let i = 0; i < frames.length; i++) { > + list.push(frames[i]); > + } > + return list; Same comment than Frames, we could just expose all frames via a single method and let the caller filter out.

Comment on attachment 8498296 [details] [diff] [review] Part 2: B2G tab list actors (v3) Review of attachment 8498296 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, works great, some code abstraction tweaks suggested. May require some tweaks according to part1 comments. ::: b2g/components/DebuggerActors.js @@ +15,5 @@ > +XPCOMUtils.defineLazyGetter(this, "Frames", function() { > + const { Frames } = > + Cu.import("resource://gre/modules/Frames.jsm", {}); > + return Frames; > +}); Note that there is an helper for jsm: XPCOMUtils.defineLazyModuleGetter(this, "Frames", "resource://gre/modules/Frames.jsm"); @@ +34,5 @@ > +}; > + > +B2GTabList.prototype._getChildren = function() { > + return Frames.listBrowsers(); > +}; This feels like a hack. _getChildren receive a window reference and do not care about it. Wouldn't it be better to expose a _getBrowsers function on BrowserTabList and put for (let win of allAppShellDOMWindows(DebuggerServer.chromeWindowType)) for (let browser of this._getChildren(win)) yield browser; in the generic one, and return Frames.listBrowsers() in the overloaded one? @@ +38,5 @@ > +}; > + > +B2GTabList.prototype._isRemoteBrowser = function() { > + return true; > +}; Is there a reason to overload this BrowserTabList method? It feels like a lie, today all content frames are OOP, but even it is unlikely to change, it may happen when hacking gecko/gaia.

(In reply to Alexandre Poirot [:ochameau] from comment #18) > Comment on attachment 8496026 [details] [diff] [review] > Part 1: Access browser frames (v2) > > Review of attachment 8496026 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: b2g/components/Frames.jsm > @@ +123,5 @@ > > + e.stack + '\n'); > > + } > > + }); > > + return; > > + } > > Instead of duplicating this code I would call only one callback, > onFrameDestroyed and let the callback figure out if it consider to given > frame or not. > It would require tuning each callsite of Frames... but I think it will be > globally easier. Simplier implementation of this module and also easier to > track any kind of frame. Should we still do the first / last app frame tracking in Frames.jsm? If so, do we always pass true for that arg for browser frames then?

(In reply to J. Ryan Stinnett [:jryans] from comment #20) > Should we still do the first / last app frame tracking in Frames.jsm? Yes. It feel weird as Frames.jsm would be more generic to any kind of frames, but I don't think it is justified to make an abstraction on top of it, just for that. > If so, do we always pass true for that arg for browser frames then? I would pass undefined/null as it has no meaning for those frames.

(In reply to Alexandre Poirot [:ochameau] from comment #18) > Comment on attachment 8496026 [details] [diff] [review] > Part 1: Access browser frames (v2) > > Review of attachment 8496026 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think I can live with these small duplications highlighted in next > comments, > but I quite like the idea of having a very generic Frames.jsm module that > just exposes all frames. Okay, I've refactored this to just expose all the frames as you suggested.

(In reply to Alexandre Poirot [:ochameau] from comment #19) > Comment on attachment 8498296 [details] [diff] [review] > Part 2: B2G tab list actors (v3) > @@ +34,5 @@ > > +}; > > + > > +B2GTabList.prototype._getChildren = function() { > > + return Frames.listBrowsers(); > > +}; > > This feels like a hack. _getChildren receive a window reference and do not > care about it. > Wouldn't it be better to expose a _getBrowsers function on BrowserTabList > and put > for (let win of allAppShellDOMWindows(DebuggerServer.chromeWindowType)) > for (let browser of this._getChildren(win)) > yield browser; > in the generic one, and return Frames.listBrowsers() in the overloaded one? Hmm, but there are other pieces of the |getList| loop that reference the |win| variable, so that would now be hidden from them. It's just that b2g case is a bit simpler than many windows on desktop Firefox, so some pieces can be ignored in the b2g case. I've left this as-is (after refactoring for part 1 changes). > @@ +38,5 @@ > > +}; > > + > > +B2GTabList.prototype._isRemoteBrowser = function() { > > + return true; > > +}; > > Is there a reason to overload this BrowserTabList method? > It feels like a lie, today all content frames are OOP, but even it is > unlikely to change, it may happen when hacking gecko/gaia. Fair enough, I changed this to check the frame's |remote| attribute. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e70dd4b2ef8c

Comment on attachment 8501897 [details] [diff] [review] Part 2: B2G tab list actors (v4, ochameau: r+) Review of attachment 8501897 [details] [diff] [review]: ----------------------------------------------------------------- I would really like to see us not hacking and have a piece of art abstraction ;) I gave some hints on how to do so. ::: b2g/components/DebuggerActors.js @@ +44,5 @@ > +}; > + > +B2GTabList.prototype._isRemoteBrowser = function(frame) { > + return frame.getAttribute("remote"); > +}; Given that's the same implementation than BrowserTabList, you don't need to overload it. ::: toolkit/devtools/server/actors/webbrowser.js @@ +311,2 @@ > } else { > actor = new BrowserTabActor(this._connection, browser, win.gBrowser); You can easily get rid of win on this scope. Here you can get gBrowser via browser.getTabBrowser(). (You may even get rid of 3rd BrowserTabActor argument and fetch the tabbrowser from browser only when needed) @@ +314,5 @@ > actorPromises.push(promise.resolve(actor)); > } > > // Set the 'selected' properties on all actors correctly. > actor.selected = (win === topXULWindow && browser === selectedBrowser); And here, _getSelectedBrowser() should only return the selected browser from the top level xul window. Do: let selectedBrowser = this._getSelectedBrowser(topXulWindow); before the for() loop.

Comment on attachment 8501882 [details] [diff] [review] Part 1: Access browser frames (v3) Review of attachment 8501882 [details] [diff] [review]: ----------------------------------------------------------------- r+ for all but b2g/components/* f+ for b2g/components/* Note to vivien, we would just need your review for b2g/components/! ::: b2g/components/Frames.jsm @@ +28,5 @@ > Services.obs.addObserver(this, 'message-manager-disconnect', false); > > + SystemAppProxy.getFrames().forEach(frame => { > + let mm = frame.QueryInterface(Ci.nsIFrameLoaderOwner) > + .frameLoader.messageManager; nit: indentation looks wrong. @@ +58,5 @@ > // get a ref to the app <iframe> > frameLoader.QueryInterface(Ci.nsIFrameLoader); > let frame = frameLoader.ownerElement; > + let mm = frame.QueryInterface(Ci.nsIFrameLoaderOwner) > + .frameLoader.messageManager; nit: indentation

Comment on attachment 8501882 [details] [diff] [review] Part 1: Access browser frames (v3) Review of attachment 8501882 [details] [diff] [review]: ----------------------------------------------------------------- r+ wit nits. ::: b2g/components/Frames.jsm @@ +58,5 @@ > // get a ref to the app <iframe> > frameLoader.QueryInterface(Ci.nsIFrameLoader); > let frame = frameLoader.ownerElement; > + let mm = frame.QueryInterface(Ci.nsIFrameLoaderOwner) > + .frameLoader.messageManager; I would say this line and the comment that has been changed should not be changed in the patch. Their are identically except that they are splitted in 2 lines. There is really no need for them in the patch and they will just make |blame| more painful. It's OK to fix indent and stuff when this is really close to the code your changes, but otherwise it seems too much :) @@ +72,5 @@ > > onMessageManagerCreated: function (mm, frame) { > this._frames.set(mm, frame); > > + let isFirstAppFrame = null; nit: Should it be |false| instead of |null| ? It seems like a boolean to me. @@ +99,5 @@ > } > > this._frames.delete(mm); > > + let isLastAppFrame = null; Boolean ? ::: b2g/components/SystemAppProxy.jsm @@ +125,2 @@ > let list = [systemAppFrame]; > + let frames = systemAppFrame.contentDocument.querySelectorAll('iframe'); Should we do iframe[mozbrowser] or are we fine to debug any particular iframes ? (Both sounds OK to me for now).

Comment on attachment 8501882 [details] [diff] [review] Part 1: Access browser frames (v3) Review of attachment 8501882 [details] [diff] [review]: ----------------------------------------------------------------- r+ wit nits (really). ::: b2g/components/Frames.jsm @@ +58,5 @@ > // get a ref to the app <iframe> > frameLoader.QueryInterface(Ci.nsIFrameLoader); > let frame = frameLoader.ownerElement; > + let mm = frame.QueryInterface(Ci.nsIFrameLoaderOwner) > + .frameLoader.messageManager; I would say this line and the comment that has been changed should not be changed in the patch. Their are identically except that they are splitted in 2 lines. There is really no need for them in the patch and they will just make |blame| more painful. It's OK to fix indent and stuff when this is really close to the code your changes, but otherwise it seems too much :) @@ +72,5 @@ > > onMessageManagerCreated: function (mm, frame) { > this._frames.set(mm, frame); > > + let isFirstAppFrame = null; nit: Should it be |false| instead of |null| ? It seems like a boolean to me. @@ +99,5 @@ > } > > this._frames.delete(mm); > > + let isLastAppFrame = null; Boolean ? ::: b2g/components/SystemAppProxy.jsm @@ +125,2 @@ > let list = [systemAppFrame]; > + let frames = systemAppFrame.contentDocument.querySelectorAll('iframe'); Should we do iframe[mozbrowser] or are we fine to debug any particular iframes ? (Both sounds OK to me for now).

(In reply to Alexandre Poirot [:ochameau] from comment #25) > Comment on attachment 8501882 [details] [diff] [review] > Part 1: Access browser frames (v3) > > Review of attachment 8501882 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: b2g/components/Frames.jsm > @@ +28,5 @@ > > Services.obs.addObserver(this, 'message-manager-disconnect', false); > > > > + SystemAppProxy.getFrames().forEach(frame => { > > + let mm = frame.QueryInterface(Ci.nsIFrameLoaderOwner) > > + .frameLoader.messageManager; > > nit: indentation looks wrong. Reverted. > @@ +58,5 @@ > > // get a ref to the app <iframe> > > frameLoader.QueryInterface(Ci.nsIFrameLoader); > > let frame = frameLoader.ownerElement; > > + let mm = frame.QueryInterface(Ci.nsIFrameLoaderOwner) > > + .frameLoader.messageManager; > > nit: indentation Reverted. (In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #26) > Comment on attachment 8501882 [details] [diff] [review] > Part 1: Access browser frames (v3) > > Review of attachment 8501882 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: b2g/components/Frames.jsm > @@ +58,5 @@ > > // get a ref to the app <iframe> > > frameLoader.QueryInterface(Ci.nsIFrameLoader); > > let frame = frameLoader.ownerElement; > > + let mm = frame.QueryInterface(Ci.nsIFrameLoaderOwner) > > + .frameLoader.messageManager; > > I would say this line and the comment that has been changed should not be > changed in the patch. Their are identically except that they are splitted in > 2 lines. There is really no need for them in the patch and they will just > make |blame| more painful. > > It's OK to fix indent and stuff when this is really close to the code your > changes, but otherwise it seems too much :) Reverted my indentation changes. > @@ +72,5 @@ > > > > onMessageManagerCreated: function (mm, frame) { > > this._frames.set(mm, frame); > > > > + let isFirstAppFrame = null; > > nit: Should it be |false| instead of |null| ? It seems like a boolean to me. Left these two as null, as this is used to indicate "not an app frame". false / true values are used only when you do indeed have an app frame. > ::: b2g/components/SystemAppProxy.jsm > @@ +125,2 @@ > > let list = [systemAppFrame]; > > + let frames = systemAppFrame.contentDocument.querySelectorAll('iframe'); > > Should we do iframe[mozbrowser] or are we fine to debug any particular > iframes ? (Both sounds OK to me for now). I kept iframe for now, as there is no harm in it and could be useful later if we need to debug non-mozbrowser frames for some reason. If one is added, it will appear in browser tab list, but this can be tuned later if needed.

(In reply to Alexandre Poirot [:ochameau] from comment #24) > Comment on attachment 8501897 [details] [diff] [review] > Part 2: B2G tab list actors (v4, ochameau: r+) > > Review of attachment 8501897 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: b2g/components/DebuggerActors.js > @@ +44,5 @@ > > +}; > > + > > +B2GTabList.prototype._isRemoteBrowser = function(frame) { > > + return frame.getAttribute("remote"); > > +}; > > Given that's the same implementation than BrowserTabList, you don't need to > overload it. As discussed on IRC, changed BrowserTabList to this version and removed the override, since isRemoteBrowser does not exist on B2G. > ::: toolkit/devtools/server/actors/webbrowser.js > @@ +311,2 @@ > > } else { > > actor = new BrowserTabActor(this._connection, browser, win.gBrowser); > > You can easily get rid of win on this scope. > Here you can get gBrowser via browser.getTabBrowser(). > (You may even get rid of 3rd BrowserTabActor argument and fetch the > tabbrowser from browser only when needed) Okay, done. I've kept 3rd parameter, as the WebappRT implementation makes use of it. > @@ +314,5 @@ > > actorPromises.push(promise.resolve(actor)); > > } > > > > // Set the 'selected' properties on all actors correctly. > > actor.selected = (win === topXULWindow && browser === selectedBrowser); > > And here, _getSelectedBrowser() should only return the selected browser from > the top level xul window. > Do: let selectedBrowser = this._getSelectedBrowser(topXulWindow); > before the for() loop. Done. I created a |_getBrowsers| function using the generator approach you recommended earlier. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=255025120578

Fix test failures. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=325a70eeeac6

https://hg.mozilla.org/integration/fx-team/rev/7d7d64553c68 https://hg.mozilla.org/integration/fx-team/rev/4b9af1d6b1a8

https://hg.mozilla.org/mozilla-central/rev/7d7d64553c68 https://hg.mozilla.org/mozilla-central/rev/4b9af1d6b1a8