Closed Bug 912900 Opened 12 years ago Closed 11 years ago

Remote Fennec browser tabs in App Manager

Categories

(DevTools Graveyard :: WebIDE, defect, P1)

x86
All
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 30

People

(Reporter: paul, Assigned: rtnpro)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=js][mentor=jryans][mentor=paul][not-that-easy-but-fun][needs-coverage])

Attachments

(4 files, 10 obsolete files)

14.17 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
341.85 KB, image/jpeg
Details
262.95 KB, image/png
Details
140.22 KB, image/jpeg
Details
We only show the installed apps. We could also show the browser's tabs, and the bookmarks.
Priority: -- → P3
Priority: P3 → P2
Whiteboard: [good-first-bug][lang=js][mentor=paul][not-that-easy-but-fun]
Hi Paul, I want to work on this bug. Can you give some pointers on how to start? Regards, Sayan
(In reply to sayan.chowdhury2012 from comment #1) > Hi Paul, > > I want to work on this bug. Can you give some pointers on how to start? > > Regards, > Sayan Hi Sayan. This is not simple to implement. I'd be happy if you try to implement that, but just be warned that it might take some time to get this right. And if you realize this is too much for you now, that's not a problem, we'll find you simpler bugs :) In about:config, if you set `devtools.debugger.remote-enabled` to true, you'll see a "Connect…" item in the Web Developers menu. Once connected, you see a list of "Available remote tabs". Clicking on one of these tabs will start the devtools. We want to merge this feature inside the app manager. You'll find the code of the connection screen there: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/connect/ We need a "Tabs" tab in the device section, where we'd list the available tab. Look at how we list installed apps, that will help. Again, this is a pretty big project. If you think it's too early for you to work on this, it's not a problem. Take a look at the code and tell me.
Whiteboard: [good-first-bug][lang=js][mentor=paul][not-that-easy-but-fun] → [good first bug][lang=js][mentor=paul][not-that-easy-but-fun]
Hey, Since I have already worked on a couple of fixes, I'd like to take on this bug. In the mean time, Sayan can work some other simpler bugs. Regards, rtnpro
Assignee: nobody → rtnpro
Hi, Yes, you can take this bug, i am having troubles setting up the project on a laptop because of insufficient system requirements. I may take up easy bugs later as soon as i set up the project. Regards, Sayan
Hey Paul, I was able to successfully connect remotely to Firefox browser in my Android phone. I was also (somehow) able to connect to my Firefox OS Simulator from the "connect" tool in the following manner: 1. Start simulator from "App manger" 2. Copy Firefox OS Simulator's port number, say 53789, from App Manager's console log. 3. And then in the "Connect" tool, connect to localhost:53789 But, there I see no **tabs**, but only **Main process** although I had multiple tabs open in my simulator's browser along with multiple other apps. When you said "We want to merge this feature inside the app manager.", did you mean to merge the connect tool with App Manager so that it display open tabs in the browser in Firefox OS phone? Regards, rtnpro
Flags: needinfo?(paul)
(In reply to rtnpro from comment #5) > Hey Paul, > > I was able to successfully connect remotely to Firefox browser in my Android > phone. I was also (somehow) able to connect to my Firefox OS Simulator from > the "connect" tool in the following manner: > 1. Start simulator from "App manger" > 2. Copy Firefox OS Simulator's port number, say 53789, from App Manager's > console log. > 3. And then in the "Connect" tool, connect to localhost:53789 > > But, there I see no **tabs**, but only **Main process** although I had > multiple tabs open in my simulator's browser along with multiple other apps. > > When you said "We want to merge this feature inside the app manager.", did > you mean to merge the connect tool with App Manager so that it display open > tabs in the browser in Firefox OS phone? Firefox OS doesn't expose tabs yet. Only Firefox Desktop and Firefox for Android do. But only Firefox expose Apps. So basically, after this bug, depending on what we connect to, we'll only see tabs, or only see apps. So let's show the tabs from Android in the app manager. That's a good start.
Flags: needinfo?(paul)
Hey Paul, Is there some documentation on how Firefox's, especially, devtools' code is organized. Currently, the way foo.js works with foo.xhtml looks very abstruse. Also, is there a way to do something like 'console.log' to print debug logs somewhere (may be browser console or stdout, etc.)? Also, is **connection** in this http://dxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/device-store.js#31 equivalent to http://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/connect/connect.js#59? Can I do something like ``gClient = new DebuggerClient(connection);`` in http://dxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/device-store.js as in http://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/connect/connect.js#65? Regards, rtnpro
Flags: needinfo?(paul)
(In reply to rtnpro from comment #7) > Hey Paul, > > Is there some documentation on how Firefox's, especially, devtools' code is > organized. Currently, the way foo.js works with foo.xhtml looks very > abstruse. Hmm, not really. What is it you don't understand exactly? I encourage you to come by IRC. > Also, is there a way to do something like 'console.log' to print > debug logs somewhere (may be browser console or stdout, etc.)? console.log() should work. dump() too. Otherwise: Cu.reportError(). > Also, is **connection** in this > http://dxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/ > device-store.js#31 equivalent to > http://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/ > connect/connect.js#59? There's no "connection" object in connect.js. Connect.js doesn't use connection-manager. > Can I do something like ``gClient = new DebuggerClient(connection);`` in > http://dxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/ > device-store.js as in > http://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/ > connect/connect.js#65? "connection" from > const {Connection} = require("devtools/client/connection-manager"); is a wrapper around the DebuggerClient. See: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/connection-manager.js This is what you should use. `connection.client` is the equivalient of `new DebuggerClient(…)`. You can get the list of tabs like that: > connection.client.listTabs((res) => { > // res.tabs is an array of tabs > }); Hope that helps.
Flags: needinfo?(paul)
Hey Paul, Apologies for the long pause on implementing this feature :( I have worked on some code on this. The patch is very primitive. I am submitting it so that you can review it and help me proceed in the correct direction. I am currently getting the following error with my patch: DBG-CLIENT: Got: { "error": "unrecognizedPacketType", "message": "Actor \"conn14.9\" does not recognize the packet type \"getDeviceFirefoxTabs\"", "from": "conn14.9" }
Attachment #8355641 - Flags: review?(paul)
Flags: needinfo?(paul)
Comment on attachment 8355641 [details] [diff] [review] Show connected device's Firefox's tabs in App Manager toolkit/devtools/server/actors/device.js Here, you're adding a new method to list tabs, but there's already one: client.listTabs. No need to change anything there. >+ _getDeviceFirefoxTabs: function () { >+ return this._deviceFront.getDeviceFirefoxTabs() >+ .then(json => { >+ let tabs = []; >+ this._connection.client.listTabs((resp) => { >+ this.object.tabs = resp.tabs; >+ json.tabs = resp.tabs; >+ }); >+ }); >+ >+ } No need to use deviceFront. `getDeviceFirefoxTabs` is called from `feedStore` right? Look where `feedstore` is called. A listTabs has already been done there. You need to save the listTabs response (`resp`) as `this.listTabsResponse = resp`, and in getDeviceFirefoxTabs, you reuse it to populate the array. Hope that helps!
Attachment #8355641 - Flags: review?(paul) → feedback-
Hi Paul, Can you assign me some simpler bugs to start working on. I have properly configured the project on my system. Regards, Sayan
(In reply to sayan.chowdhury2012 from comment #11) > Can you assign me some simpler bugs to start working on. I have properly > configured the project on my system. See there: http://www.joshmatthews.net/bugsahoy/?devtools=1
Flags: needinfo?(paul)
Priority: P2 → P1
Blocks: dbg-remote
Hey Paul, _getFirefoxTabs() is being called from _feedStore(). How do I display the tabs without deviceFront? Both the permissions table and apps tab seem to use deviceFront to display their content. Also, I am getting the following error: https://gist.github.com/rtnpro/8305424 I am also updating the patch to reflect my latest local code. Please help. Regards, rtnpro
Flags: needinfo?(paul)
Attached patch app_manager_firefox_tabs.patch (obsolete) — Splinter Review
Attachment #8355641 - Attachment is obsolete: true
Attachment #8356709 - Flags: review?(paul)
ping?
(In reply to rtnpro from comment #13) > Hey Paul, > > _getFirefoxTabs() is being called from _feedStore(). How do I display the > tabs without deviceFront? Both the permissions table and apps tab seem to > use deviceFront to display their content. Also, I am getting the following > error: > https://gist.github.com/rtnpro/8305424 In _feedStore, before doing app-related things, you need to make sure apps are supported (Fennec doesn't support app debugging). To do so, I think you need to check that "this._webAppsActor" is not null. You should have a deviceFront though.
Flags: needinfo?(paul)
Attachment #8356709 - Flags: review?(paul)
Attached patch app_manager_firefox_tabs.patch (obsolete) — Splinter Review
This patch lists open tabs from android's firefox browser. TODO: - Beautify - Open toolbox for the tab - write tests
Attachment #8356709 - Attachment is obsolete: true
Attachment #8363744 - Flags: review?(paul)
Flags: needinfo?(paul)
(In reply to rtnpro from comment #17) > Created attachment 8363744 [details] [diff] [review] > app_manager_firefox_tabs.patch > > This patch lists open tabs from android's firefox browser. > > TODO: > - Beautify > - Open toolbox for the tab > - write tests Very cool! I'll look at this patch asap (might only happen next week).
Flags: needinfo?(paul)
Hey Paul, I am now able to display the list of remote device's Firefox's tabs in App Manager. Now, I need to allow the user to open the toolbox on clicking a tab. I think calling UI.openToolbox() with a TAB object from ``this.object.firefoxTabs`` array should do. However, I am not sure how to access the TAB object inside template #firefox-tab-template in ``browser/devtools/app-manager/content/device.xhtml``. Please help. Thanks, rtnpro
Flags: needinfo?(paul)
There's no obvious way to re-use the current code, but I guess this should work: > onclick="UI.openToolboxForTab(this); And you'll need to implement openToolboxForTab yourself. Something like that: > openToolboxForTab: function(aNode) { > // Poor way to get the index of the node > let index = Array.prototype.indexOf.apply(aNode.parentNode.children, [aNode]); > let tab = this.listTabsResponse.tabs[index]; > let options = { > form: tab, > client: this.connection.client, > chrome: false > }; > devtools.TargetFactory.forRemoteTab(options).then((target) => { > top.UI.openAndShowToolboxForTarget(target, tab.url, null); > }); > }, We don't have the favicon of the apge, so we need to use "null" for now. We'll need to fix that later. Tell me if this works.
Flags: needinfo?(paul)
Comment on attachment 8363744 [details] [diff] [review] app_manager_firefox_tabs.patch Looks good so far. You will need to hide the tabs if there's no tabs available. And don't use "firefox" but "browser" in the name of your variables. In you <a>, you access a dataset. There's no dataset defined here. + this.object.tabs = []; Is this actually useful? Also, this needs to be dynamic. When the tablist change, the UI need to change too.
Attachment #8363744 - Flags: review?(paul)
(In reply to Paul Rouget [:paul] from comment #21) > Comment on attachment 8363744 [details] [diff] [review] > app_manager_firefox_tabs.patch > > Looks good so far. You will need to hide the tabs if there's no tabs > available. > > And don't use "firefox" but "browser" in the name of your variables. OK. > In you <a>, you access a dataset. There's no dataset defined here. I know, but how do I access the TAB object here? > > + this.object.tabs = []; > > Is this actually useful? This is not necessary. I will clean it up :) > > Also, this needs to be dynamic. When the tablist change, the UI need to > change too. Any leads on how to make the UI reactive? Is there any signal emitted when the tabs on the remote browser change? Or do I have to poll on it? Regards, rtnpro
Flags: needinfo?(paul)
(In reply to rtnpro from comment #22) > > In you <a>, you access a dataset. There's no dataset defined here. > I know, but how do I access the TAB object here? You can't. But you can figure the index of the tab from the position of the node. My function above does that. > > Also, this needs to be dynamic. When the tablist change, the UI need to > > change too. > > Any leads on how to make the UI reactive? Is there any signal emitted when > the tabs on the remote browser change? Or do I have to poll on it? client.addListener("tabListChanged", updateUI);
Flags: needinfo?(paul)
Status: NEW → ASSIGNED
@rtnpro, any update on this? We would very much like to get this feature in the next release, if you're not sure you can get that done in time, do you want me to re-assign you to a simpler bug?
Attached patch app_manager_firefox_tabs.patch (obsolete) — Splinter Review
@paul, I am almost done with this implementation. I am displaying the tab links properly, and I am able to call the function to open toolbox for the tab from the app manager. However, I am getting an error when trying to open the toolbox. I am pasting the error below for your perusal. You'll know where I am doing it wrong. Please help me to solve this issue. DBG-CLIENT: Got: { "error": "unknownError", "message": "error occurred while processing 'listRunningApps: TypeError: chromeWindow.shell is undefined\nStack: WebappsActor.prototype._appFrames@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webapps.js:744\nWebappsActor.prototype.listRunningApps@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webapps.js:766\nDSC_onPacket@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js:1019\nDT__processIncoming/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/transport.js:201\nmakeInfallible/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/DevToolsUtils.js:75\n" } onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'listRunningApps: TypeError: chromeWindow.shell is undefined\nStack: WebappsActor.prototype._appFrames@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webapps.js:744\nWebappsActor.prototype.listRunningApps@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webapps.js:766\nDSC_onPacket@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js:1019\nDT__processIncoming/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/transport.js:201\nmakeInfallible/<@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/DevToolsUtils.js:75\n"} Stack: DC_onPacket/<@resource://gre/modules/devtools/dbg-client.jsm:668 resolve@resource://gre/modules/commonjs/sdk/core/promise.js:118 then@resource://gre/modules/commonjs/sdk/core/promise.js:43 then@resource://gre/modules/commonjs/sdk/core/promise.js:153 DC_onPacket@resource://gre/modules/devtools/dbg-client.jsm:717 DT__processIncoming/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/server/transport.js:201 makeInfallible/<@resource://gre/modules/devtools/DevToolsUtils.jsm -> resource://gre/modules/devtools/DevToolsUtils.js:76 --DOMWINDOW == 34 (0x11ed5e8b8) [pid = 16637] [serial = 26] [outer = 0x110654cb8] [url = about:blank] --DOMWINDOW == 33 (0x10ee690b8) [pid = 16637] [serial = 31] [outer = 0x10ee674b8] [url = about:blank] --DOMWINDOW == 32 (0x110654cb8) [pid = 16637] [serial = 25] [outer = 0x0] [url = about:blank] JavaScript error: resource://gre/modules/devtools/dbg-client.jsm, line 607: Error: 'attach' request packet has no destination.
Attachment #8363744 - Attachment is obsolete: true
Attachment #8371040 - Flags: review?(paul)
Flags: needinfo?(paul)
This shouldn't happen. Make sure to update your Fennec build (we fixed this bug the 10th of January).
Flags: needinfo?(paul)
Attachment #8371040 - Flags: review?(paul)
Attached patch app_manager_firefox_tabs.patch (obsolete) — Splinter Review
Hey @paul, I got opening toolbox for remote Firefox tabs working. Please review the patch and suggest me how to improve it and write tests for it. Also, I'd like to know what event is triggered when the content of a tab in remote Firefox changes? Regards, rtnpro
Attachment #8371040 - Attachment is obsolete: true
Attachment #8371717 - Flags: review?(paul)
Flags: needinfo?(paul)
Comment on attachment 8371717 [details] [diff] [review] app_manager_firefox_tabs.patch Review of attachment 8371717 [details] [diff] [review]: ----------------------------------------------------------------- Paul suggested I review your patch, as he has a lot in his queue at the moment. First let me say that it's great to see this feature working, so good work! :) I have a few comments generally: * In comment 21, Paul mentioned you should replace "Firefox" with "Browser" in your variables, and you really need to do that in a lot of places. Search for all usages of "Firefox" in your patch, and replace them with "Browser", i.e. in variables / CSS / strings. * You asked how to detect when the remote tab changes. I don't think there is a simple way to do this currently, so I think it would be best to file a new bug to add this part, so we don't block this feature while working out how to do that. As far as tests go, I don't know that there is much that you can be expected to write tests for here currently. Unfortunately, we don't yet have the testing infrastructure in place that would actually connect the App Manager to some device (like Android with Fennec), so it's hard for me to imagine how you'd write a test for this. We're hoping to make this story better soon. Feel free to flag me for reviews on future patch revisions in place of Paul. ::: browser/devtools/app-manager/content/device.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > const Cu = Components.utils; > +Cu.import('resource://gre/modules/XPCOMUtils.jsm'); Do you need this? Doesn't seem like it's used anywhere. @@ +7,2 @@ > Cu.import("resource://gre/modules/Services.jsm"); > +//Cu.import("resource:///modules/devtools/gDevTools.jsm"); Should be fine to delete this line, since you've got the other version below. @@ +7,4 @@ > Cu.import("resource://gre/modules/Services.jsm"); > +//Cu.import("resource:///modules/devtools/gDevTools.jsm"); > +Cu.import("resource://gre/modules/devtools/dbg-client.jsm"); > +let {gDevTools} = Cu.import("resource:///modules/devtools/gDevTools.jsm", {}); let -> const @@ +159,5 @@ > }, > > + openToolboxForActor: function (actor) { > + let _this = this; > + function _openToolbox(form, chrome=false) { Instead of this nested function, add a function called something like "_getTargetForTab" to the UI object. Most likely this function should return a promise, so you can do something like: getTargetForTab(...).then(target => { top.UI.openAndShowToolboxForTarget(...); }, console.error); which would be very similar to what "openToolbox" above does for an app. @@ +167,5 @@ > + chrome: chrome > + }; > + devtools.TargetFactory.forRemoteTab(options).then((target) => { > + let hostType = devtools.Toolbox.HostType.WINDOW; > + gDevTools.showToolbox(target, "webconsole", hostType); By using this approach, the toolbox is opened in a new window, instead of in an "App Manager tab" like it does when debugging a Firefox OS app. Take a look at how "openToolbox" above opens the toolbox for an app, so you can use the same technique here. While you're at it, it makes sense to rename "openToolbox" above to "openToolboxForApp". @@ +171,5 @@ > + gDevTools.showToolbox(target, "webconsole", hostType); > + }); > + } > + this.connection.client.listTabs( > + response => { Nit: Move this to the line above @@ +174,5 @@ > + this.connection.client.listTabs( > + response => { > + let tab; > + this.listTabsResponse = response; > + for (n in this.listTabsResponse.tabs) { In comment 20, Paul included a code snippet that tries to find the index of the node, instead of looping over them like you are here. Did you try Paul's technique? ::: browser/devtools/app-manager/content/device.xhtml @@ +43,5 @@ > <a target="_blank" href="&device.permissionsHelpLink;"> > <button class="help">&device.help;</button> > </a> > </div> > + <div onclick="UI.setTab('firefox-tabs')" class="tab sidebar-item firefox-tabs" title="&device.firefoxTabsTooltip;">&device.firefoxTabs;</div> "Firefox tabs" -> "Browser Tabs" @@ +69,5 @@ > <div class="deny-label" title="&device.denyTooltip;">&device.deny;</div> > </div> > </div> > + <div class="tabpanel firefox-tabs"> > + <h1>Firefox tabs</h1> Once you've changed to browser tabs above, you can use the same entity here for translation. @@ +91,5 @@ > </template> > > + <template id="firefox-tab-template"> > + <div class="firefox-tab"> > + <span class="firefox-tab-url" template='{"type":"textContent","path":"url"}'></span> It would probably be good to show both the tab's page title and the URL, since you already get both anyway. Try showing the page title as the main info, and then the URL in a smaller font below it. @@ +93,5 @@ > + <template id="firefox-tab-template"> > + <div class="firefox-tab"> > + <span class="firefox-tab-url" template='{"type":"textContent","path":"url"}'></span> > + <div class="firefox-tab-buttons"> > + <button class="button-inspect" template='{"type":"attribute","path":"actor","name":"data-actor"}' onclick="UI.openToolboxForActor(this.dataset.actor)" style="display: inline-block;">&device.firefoxTabInspectButtonLabel;</button> Change the label to "device.debugTab", and its content should say "Debug", like we do for an app. Also, let's add a tooltip for this button as well. ::: browser/devtools/app-manager/device-store.js @@ +64,5 @@ > }, > > + _onTabListChanged: function() { > + if (this._connection.status == Connection.Status.CONNECTED) { > + this._listTabs(); This function should be able to just call _listTabs. It doesn't need to do these other things. @@ +74,4 @@ > _listTabs: function() { > this._connection.client.listTabs((resp) => { > this._deviceFront = getDeviceFront(this._connection.client, resp); > + this._listTabsResponse = resp; Seems fine to just store the tabs directly here, so: this.object.tabs = resp.tabs; so we don't need to save _listTabsResponse temporarily. This eliminates the need for _getDeviceFirefoxTabs as well. @@ +109,5 @@ > this.object.permissions = permissionsArray; > }); > }, > + > + _updateUI: function (_this) { Is this used? @@ +115,5 @@ > + }, > + > + _getDeviceFirefoxTabs: function () { > + this.object.firefoxTabs = this._listTabsResponse.tabs; > + this._connection.client.addListener('tabListChanged', this._onTabListChanged); Add this listener just above the call to _listTabs on status change. As it is, you are adding many listeners (once each time you list tabs). ::: browser/devtools/framework/connect/connect.js @@ -36,5 @@ > if (port) { > document.getElementById("port").value = port; > } > > - let form = document.querySelector("#connection-form form"); There shouldn't be any reason to change this file, so please revert this change. ::: browser/locales/en-US/chrome/browser/devtools/app-manager.dtd @@ +32,5 @@ > <!ENTITY device.permissions "Permissions"> > <!ENTITY device.permissionsTooltip "View a table of the permissions accessible to the different types of apps"> > <!ENTITY device.permissionsHelpLink "https://developer.mozilla.org/docs/Web/Apps/App_permissions"> > +<!ENTITY device.firefoxTabs "Firefox tabs"> > +<!ENTITY device.firefoxTabsTooltip "Open tabs in connected device's Firefox."> Firefox -> browser Nit: Tooltips shouldn't end with periods ::: browser/themes/shared/devtools/app-manager/device.css @@ +141,5 @@ > +/***************** FIREFOX TAB BUTTONS *****************/ > + > + > + > +.firefox-tab-buttons { Where possible, you should try to refactor and reuse existing style blocks, rather than just duplicating them.
Attachment #8371717 - Flags: review?(paul)
Whiteboard: [good first bug][lang=js][mentor=paul][not-that-easy-but-fun] → [good first bug][lang=js][mentor=paul][mentor=jryans][not-that-easy-but-fun]
Thanks @jryans for the detailed review :) I have updated the patch as per your review comments. I have manually tested it at my end and things seem to be working fine. I am now opening the debug console as an app manager tab instead of a window. However, I need an image for this new tab type. Let's implement the reactivity for remote browser tabs as a different task.
Attachment #8371717 - Attachment is obsolete: true
Attachment #8378575 - Flags: review?(jryans)
Flags: needinfo?(jryans)
Cool! The Dev Tools team is at a team meetup this week, so I'll take a look at this some time next week.
Flags: needinfo?(jryans)
@jryans, No problem, please take your time :) In the mean time, I had some feedback to your last code review. (In reply to J. Ryan Stinnett [:jryans] from comment #28) > @@ +74,4 @@ > > _listTabs: function() { > > this._connection.client.listTabs((resp) => { > > this._deviceFront = getDeviceFront(this._connection.client, resp); > > + this._listTabsResponse = resp; > > Seems fine to just store the tabs directly here, so: > > this.object.tabs = resp.tabs; > > so we don't need to save _listTabsResponse temporarily. This eliminates the > need for _getDeviceFirefoxTabs as well. Removed _getDeviceFirefoxTabs. > @@ +109,5 @@ > > this.object.permissions = permissionsArray; > > }); > > }, > > + > > + _updateUI: function (_this) { > > Is this used? Not now at least, so I've removed this. > @@ +115,5 @@ > > + }, > > + > > + _getDeviceFirefoxTabs: function () { > > + this.object.firefoxTabs = this._listTabsResponse.tabs; > > + this._connection.client.addListener('tabListChanged', this._onTabListChanged); > > Add this listener just above the call to _listTabs on status change. As it > is, you are adding many listeners (once each time you list tabs). Removing this listener for now. Let's solve this tab reactivity issue in a better way in a new task. Regards, rtnpro
Comment on attachment 8378575 [details] [diff] [review] Allow debugging of tabs in remote firefox browser from app manager Review of attachment 8378575 [details] [diff] [review]: ----------------------------------------------------------------- For future reviews, needinfo? is not necessary when you are setting the review? flag for the same person. ::: browser/devtools/app-manager/content/device.js @@ +162,5 @@ > + client: this.connection.client, > + chrome: chrome > + }; > + let deferred = promise.defer(); > + devtools.TargetFactory.forRemoteTab(options).then((target) => { This extra then() wrapper is not needed, just return devtools.TargetFactory.forRemoteTab(options), since that is a promise itself. @@ +173,5 @@ > + > + openToolboxForTab: function (aNode) { > + let _this = this; > + let index = Array.prototype.indexOf.apply( > + aNode.parentNode.children, [aNode]); This expression is not quite right... Please test out having more than one tab. You'll see that it's not getting the index correctly. In general, please test your patches more thoroughly before submitting them for review. @@ +176,5 @@ > + let index = Array.prototype.indexOf.apply( > + aNode.parentNode.children, [aNode]); > + this.connection.client.listTabs( > + response => { > + this.listTabsResponse = response; Do you need to save the response on this? It doesn't seem like it is used outside of this function... @@ +178,5 @@ > + this.connection.client.listTabs( > + response => { > + this.listTabsResponse = response; > + let tab = this.listTabsResponse.tabs[index]; > + _this._getTargetForTab(tab).then(target => { You don't need to capture "this" with an arrow function. Just use it directly. ::: browser/devtools/app-manager/content/device.xhtml @@ +70,5 @@ > </div> > </div> > + <div class="tabpanel browser-tabs"> > + <h1>&device.browserTabs;</h1> > + <div class="browser-tabs-body"> Seems like this wrapper element is not needed and can be removed. @@ +93,5 @@ > + <template id="browser-tab-template"> > + <div class="browser-tab"> > + <div class="browser-tab-details"> > + <p template='{"type":"textContent","path":"title"}'></p> > + <small template='{"type":"textContent","path":"url"}'></small> Change this to a <p> with a class and use CSS to reduce the font size, instead of the <small> tag. ::: browser/devtools/app-manager/device-store.js @@ +97,5 @@ > }); > } > this.object.permissions = permissionsArray; > }); > + } Seems like the listener for "tabListChanged" disappeared when you made other changes. Even though we are missing page navigation for now, we still want this listener, so we can detect when tabs are added or removed. See my previous review for where this should go. ::: browser/locales/en-US/chrome/browser/devtools/app-manager.dtd @@ +31,5 @@ > <!ENTITY device.installedAppsTooltip "View a list of apps installed on the device. Some apps, such as certified apps, may be excluded from this view."> > <!ENTITY device.permissions "Permissions"> > <!ENTITY device.permissionsTooltip "View a table of the permissions accessible to the different types of apps"> > <!ENTITY device.permissionsHelpLink "https://developer.mozilla.org/docs/Web/Apps/App_permissions"> > +<!ENTITY device.browserTabs "Browser tabs"> tabs -> Tabs @@ +32,5 @@ > <!ENTITY device.permissions "Permissions"> > <!ENTITY device.permissionsTooltip "View a table of the permissions accessible to the different types of apps"> > <!ENTITY device.permissionsHelpLink "https://developer.mozilla.org/docs/Web/Apps/App_permissions"> > +<!ENTITY device.browserTabs "Browser tabs"> > +<!ENTITY device.browserTabsTooltip "Open tabs in the browser of the connected device."> Let's go with "View a list of tabs in the browser of the connected device". Also, no period at the end. @@ +33,5 @@ > <!ENTITY device.permissionsTooltip "View a table of the permissions accessible to the different types of apps"> > <!ENTITY device.permissionsHelpLink "https://developer.mozilla.org/docs/Web/Apps/App_permissions"> > +<!ENTITY device.browserTabs "Browser tabs"> > +<!ENTITY device.browserTabsTooltip "Open tabs in the browser of the connected device."> > +<!ENTITY device.browserTabDebugButtonLabel "Debug"> Nit: Rename this "device.debugBrowserTab" for parity with other buttons. @@ +34,5 @@ > <!ENTITY device.permissionsHelpLink "https://developer.mozilla.org/docs/Web/Apps/App_permissions"> > +<!ENTITY device.browserTabs "Browser tabs"> > +<!ENTITY device.browserTabsTooltip "Open tabs in the browser of the connected device."> > +<!ENTITY device.browserTabDebugButtonLabel "Debug"> > +<!ENTITY device.browserTabDebugButtonTooltip "Click here to debug this tab from the remote browser"> Change text to "Open the Developer Tools connected to this browser tab on the device", for parity with apps tooltip. (No period). Nit: Rename to "device.debugBrowserTabTooltip". ::: browser/themes/shared/devtools/app-manager/device.css @@ +307,5 @@ > } > > > > +/***************** APPS & BROWSER TABS *****************/ Good job reusing styles this time!
Attachment #8378575 - Flags: review?(jryans) → review-
Comment on attachment 8378575 [details] [diff] [review] Allow debugging of tabs in remote firefox browser from app manager Review of attachment 8378575 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/app-manager/content/device.xhtml @@ +69,5 @@ > <div class="deny-label" title="&device.denyTooltip;">&device.deny;</div> > </div> > </div> > + <div class="tabpanel browser-tabs"> > + <h1>&device.browserTabs;</h1> Remove this header text. This should be similar to the list of apps, which doesn't have a header here.
Comment on attachment 8378575 [details] [diff] [review] Allow debugging of tabs in remote firefox browser from app manager Review of attachment 8378575 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/app-manager/content/device.js @@ +155,5 @@ > top.UI.openAndShowToolboxForTarget(target, app.name, app.iconURL); > }, console.error); > }, > > + _getTargetForTab: function (form, chrome=false) { You don't need this argument, as it is never supplied. Just set chrome to false where it is used.
Whiteboard: [good first bug][lang=js][mentor=paul][mentor=jryans][not-that-easy-but-fun] → [good first bug][lang=js][mentor=jryans][mentor=paul][not-that-easy-but-fun]
This implements the above suggested changes of which the main ones are: - Open debug console for proper tab when multiple remote tabs exist. - Re implement updating tab list when new tabs are added. However, Removing a remote tab breaks the tab list interface in AppManager. Following is the traceback I am getting when I close a remote browser tab: [89146] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /Users/rtnpro/Mozilla/devtools/fx-team/dom/events/nsContentEventHandler.cpp, line 96 DBG-CLIENT: Got: { "from": "root", "type": "tabListChanged" } DBG-CLIENT: Got: { "error": "unknownError", "message": "error occurred while processing 'listTabs: TypeError: this.window is undefined\nStack: BTA_form@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:590\n@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/root.js:271\nresolve@resource://gre/modules/commonjs/sdk/core/promise.js:118\nthen@resource://gre/modules/commonjs/sdk/core/promise.js:43\nthen@resource://gre/modules/commonjs/sdk/core/promise.js:153\n@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/root.js:245\nDSC_onPacket@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js:1019\n@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/transport.js:201\n@resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/DevToolsUtils.js:75\n", "from": "root" } Please help me to resolve this issue.
Attachment #8378575 - Attachment is obsolete: true
Attachment #8388082 - Flags: review?(jryans)
(In reply to rtnpro from comment #35) > However, Removing a remote tab breaks the tab list interface in AppManager. This issue doesn't seem specific to your patch. Instead, it seems like there's a general problem with the way we're listening for tabs to close on Android. You're just the first to try listening for it. :) It would be great if you could file a new bug about this problem, and we can solve it there.
Comment on attachment 8388082 [details] [diff] [review] Show remote browser tabs in AppManager for remote debugging Review of attachment 8388082 [details] [diff] [review]: ----------------------------------------------------------------- Okay, almost there! Just a few things left to fix. ::: browser/devtools/app-manager/content/device.js @@ +173,5 @@ > + this.connection.client.listTabs( > + response => { > + let tab = response.tabs[index]; > + this._getTargetForTab(tab).then(target => { > + top.UI.openAndShowToolboxForTarget(target, tab.name, ''); |tab.name| should be |tab.title| (there is no name property). For the last argument (the icon), let use the "default app" icon: "chrome://browser/skin/devtools/app-manager/default-app-icon.png" Save that as a const DEFAULT_APP_ICON at the top of this file and use it here. ::: browser/devtools/app-manager/device-store.js @@ +70,2 @@ > _listTabs: function() { > this._connection.client.listTabs((resp) => { Right now, we get an error back when removing a tab. The UI gets messed up since this callback happily saves a bunch of junk in the error case. It doesn't solve the root cause (which we'll leave for a separate bug), but at least the UI won't be left in bad state. Not being able to |listTabs| is fairly serious, so I'd suggest dropping the connection in this case, something like: > if (resp.error) { > this._connection.disconnect(); > } ::: browser/locales/en-US/chrome/browser/devtools/app-manager.dtd @@ +34,5 @@ > <!ENTITY device.permissionsHelpLink "https://developer.mozilla.org/docs/Web/Apps/App_permissions"> > +<!ENTITY device.browserTabs "Browser Tabs"> > +<!ENTITY device.browserTabsTooltip "View a list of tabs in the browser of the connected device"> > +<!ENTITY device.debugBrowserTab "Debug"> > +<!ENTITY device.browserTabDebugButtonTooltip "Open the Developer Tools connected to this browser tab on the device"> I'll repeat from last time: Nit: Rename to "device.debugBrowserTabTooltip". Please try to look through all review comments carefully.
Attachment #8388082 - Flags: review?(jryans)
Hey @jryans, I am grateful for your help and support. Without it, I could not have made it :) (In reply to J. Ryan Stinnett [:jryans] from comment #37) > Comment on attachment 8388082 [details] [diff] [review] > Okay, almost there! Just a few things left to fix. > > ::: browser/devtools/app-manager/content/device.js > @@ +173,5 @@ > > + this.connection.client.listTabs( > > + response => { > > + let tab = response.tabs[index]; > > + this._getTargetForTab(tab).then(target => { > > + top.UI.openAndShowToolboxForTarget(target, tab.name, ''); > > |tab.name| should be |tab.title| (there is no name property). For the last > argument (the icon), let use the "default app" icon: > > "chrome://browser/skin/devtools/app-manager/default-app-icon.png" > > Save that as a const DEFAULT_APP_ICON at the top of this file and use it > here. OK. > ::: browser/devtools/app-manager/device-store.js > @@ +70,2 @@ > > _listTabs: function() { > > this._connection.client.listTabs((resp) => { > > Right now, we get an error back when removing a tab. The UI gets messed up > since this callback happily saves a bunch of junk in the error case. > > It doesn't solve the root cause (which we'll leave for a separate bug), but Ok, I will open another bug for this and we'll work on it there. > at least the UI won't be left in bad state. Not being able to |listTabs| is > fairly serious, so I'd suggest dropping the connection in this case, > something like: > > > if (resp.error) { > > this._connection.disconnect(); > > } I will try to fix this as suggested. > ::: browser/locales/en-US/chrome/browser/devtools/app-manager.dtd > @@ +34,5 @@ > > <!ENTITY device.permissionsHelpLink "https://developer.mozilla.org/docs/Web/Apps/App_permissions"> > > +<!ENTITY device.browserTabs "Browser Tabs"> > > +<!ENTITY device.browserTabsTooltip "View a list of tabs in the browser of the connected device"> > > +<!ENTITY device.debugBrowserTab "Debug"> > > +<!ENTITY device.browserTabDebugButtonTooltip "Open the Developer Tools connected to this browser tab on the device"> > > I'll repeat from last time: > > Nit: Rename to "device.debugBrowserTabTooltip". > > Please try to look through all review comments carefully. I am sorry, I somehow missed it :( I will be more careful from now on when going through reviews. I will update the patch in a while, test things at my end and push it. Thanks, rtnpro
In addition to the fixes and features added in the previous patches, this patch includes: 1. Fix for handling error when a remote browser tab is closed. The tab list remains the same as before rather than breaking down. However, the remote browser tab list is not functional anymore. 2. Add icon for app manager tabs for debugging remote browser tab. 3. Some typo fixes.
Attachment #8388082 - Attachment is obsolete: true
Attachment #8388707 - Flags: review?(jryans)
(In reply to J. Ryan Stinnett [:jryans] from comment #36) > This issue doesn't seem specific to your patch. Instead, it seems like > there's a general problem with the way we're listening for tabs to close on > Android. You're just the first to try listening for it. :) It would be > great if you could file a new bug about this problem, and we can solve it > there. Hmm, perhaps I closed bug 813545 too soon? Although we shouldn't care any more about tabDetached packets.
(In reply to Panos Astithas [:past] from comment #40) > (In reply to J. Ryan Stinnett [:jryans] from comment #36) > > This issue doesn't seem specific to your patch. Instead, it seems like > > there's a general problem with the way we're listening for tabs to close on > > Android. You're just the first to try listening for it. :) It would be > > great if you could file a new bug about this problem, and we can solve it > > there. > > Hmm, perhaps I closed bug 813545 too soon? Although we shouldn't care any > more about tabDetached packets. Not sure if it's directly related or not... It seems like at the moment you close a tab, we try to build the tab list again, but the tab being closed is still included, even though it has no accessible window object, so it blows up trying to build the tab form. Perhaps we need a different event...? Or maybe we just need to test the window and filter these out. Not sure yet.
Whiteboard: [good first bug][lang=js][mentor=jryans][mentor=paul][not-that-easy-but-fun] → [good first bug][lang=js][mentor=jryans][mentor=paul][not-that-easy-but-fun][needs-coverage]
Comment on attachment 8388707 [details] [diff] [review] Show remote browser tabs in AppManager for remote debugging (Final) Review of attachment 8388707 [details] [diff] [review]: ----------------------------------------------------------------- Okay, just one nit to fix, and this is ready to land! Once you've made this fix, you can copy my r+ to the updated patch. After filing the new bug for tab closing error, you can set the keyword "checkin-needed" on this bug (in the keywords field) and someone will land this patch for you. ::: browser/devtools/app-manager/device-store.js @@ +70,4 @@ > _listTabs: function() { > this._connection.client.listTabs((resp) => { > + if (resp.error) { > + this._connection.disconnect(); Nit: It's best to return early[1] when there's an error, so you don't need to wrap the rest of the function in an "else" block. So, add a return after this line. [1]: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Return_from_errors_immediately
Attachment #8388707 - Flags: review?(jryans) → review+
Final version of this patch to show remote browser tabs in App Manager for debugging.
Attachment #8388707 - Attachment is obsolete: true
Attachment #8389386 - Flags: review+
I've reported the bug related to tab closing error here: https://bugzilla.mozilla.org/show_bug.cgi?id=982281. I am now marking this bug as 'checkin-needed'.
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js][mentor=jryans][mentor=paul][not-that-easy-but-fun][needs-coverage] → [good first bug][lang=js][mentor=jryans][mentor=paul][not-that-easy-but-fun][needs-coverage][fixed-in-fx-team]
Flags: needinfo?(rtnpro)
Whiteboard: [good first bug][lang=js][mentor=jryans][mentor=paul][not-that-easy-but-fun][needs-coverage][fixed-in-fx-team] → [good first bug][lang=js][mentor=jryans][mentor=paul][not-that-easy-but-fun][needs-coverage]
Hmm, well, wasn't expecting that! rtnpro, looks like you'll need to update the failing test to expect an empty tab list too. After that's done, I'll submit the patch to try to run these tests again before requesting another checkin attempt.
Fixed failing tests for: ./mach mochitest-chrome browser/devtools/app-manager/test/
Attachment #8389386 - Attachment is obsolete: true
Attachment #8390706 - Flags: checkin?(jryans)
Flags: needinfo?(rtnpro)
Comment on attachment 8390706 [details] [diff] [review] Show remote browser tabs in AppManager for remote debugging (with tests fixed) I'm running a try push for this now... However you should also fix up your commit message. I thought I had mentioned this before, but can't seem to find it anymore. The message should always start with "Bug 912900 - " and end with "r=jryans" (using this as an example). Try: https://tbpl.mozilla.org/?tree=Try&rev=35f13ac8a6af
(In reply to J. Ryan Stinnett [:jryans] from comment #49) > I'm running a try push for this now... However you should also fix up your > commit message. I thought I had mentioned this before, but can't seem to > find it anymore. The message should always start with "Bug 912900 - " and > end with "r=jryans" (using this as an example). FWIW, as I understand it, sheriffs will add that information when they commit patches on your behalf. They might even have tools to do so automatically!
(In reply to Myk Melez [:myk] [@mykmelez] from comment #50) > (In reply to J. Ryan Stinnett [:jryans] from comment #49) > > > I'm running a try push for this now... However you should also fix up your > > commit message. I thought I had mentioned this before, but can't seem to > > find it anymore. The message should always start with "Bug 912900 - " and > > end with "r=jryans" (using this as an example). > > FWIW, as I understand it, sheriffs will add that information when they > commit patches on your behalf. They might even have tools to do so > automatically! Ah, well, fair enough then! :) Once try looks green, we can mark the bug as checkin-needed again.
Comment on attachment 8390706 [details] [diff] [review] Show remote browser tabs in AppManager for remote debugging (with tests fixed) Okay, try run looks good!
Attachment #8390706 - Flags: review+
Hey @jryans, I have updated the commit message as you have mentioned. Regards, rtnpro
Attachment #8390706 - Attachment is obsolete: true
Attachment #8391459 - Flags: checkin?(jryans)
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js][mentor=jryans][mentor=paul][not-that-easy-but-fun][needs-coverage] → [good first bug][lang=js][mentor=jryans][mentor=paul][not-that-easy-but-fun][needs-coverage][fixed-in-fx-team]
Comment on attachment 8391459 [details] [diff] [review] Show remote browser tabs in AppManager for remote debugging In the future, please just use the checkin-needed bug keyword :)
Attachment #8391459 - Flags: checkin?(jryans) → checkin+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][mentor=jryans][mentor=paul][not-that-easy-but-fun][needs-coverage][fixed-in-fx-team] → [good first bug][lang=js][mentor=jryans][mentor=paul][not-that-easy-but-fun][needs-coverage]
Target Milestone: --- → Firefox 30
Summary: [app manager] should display any available targets in device.xhtml → Remote browser tabs in App Manager
Keywords: verifyme
Attached image Browser Tabs.jpg
I've installed the 1.2 to 1.5 Firefox OS simulators on latest Aurora 30.0a2 (and latest Nightly) 2014-04-27. I can see the "Browser Tabs" tab in the "Device" section, but the page is not populated, no matter how many tabs I open in Browser app. Using the Developer -> Connect.. option, I have: Available remote tabs: Available remote processes: Main Process -> which opens a new separate web console Can someone please indicate how this bug can be verified? Thank you!
I just updated the bug title to include an important hint... ;) This bug actually doesn't help with FxOS, only Fennec. It's still a bit tricky to connect to Fennec in general[1]. You'll need to: 1. Enable Developer Tools in Fennec 2. Connect phone 3. Manually forward ports using ADB 4. In the App Manager, you can't have any "ADB Helper" addons enabled, because you need to use the manual connection section 5. Edit it to say "localhost:6000" 6. Connect After all that, you should see a list of Fennec tabs in the Devices pane. We're definitely aware this is currently tricky to use. :) Also, see the list of bugs this blocks, as there are some known issue filed already. [1]: https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging/Firefox_for_Android#Connecting
Summary: Remote browser tabs in App Manager → Remote Fennec browser tabs in App Manager
I've tested with latest Nightly on Android and Firefox 29(release) on desktop, and I see the tabs from device in the Remote window. However, this is strange, but I see the same behaviour with Firefox 25(release) on both Android and on Desktop. Please tell me what am I missing.
(In reply to Mihai Pop from comment #59) > Created attachment 8426257 [details] > Screenshot from Firefox 25 > > I've tested with latest Nightly on Android and Firefox 29(release) on > desktop, and I see the tabs from device in the Remote window. However, this > is strange, but I see the same behaviour with Firefox 25(release) on both > Android and on Desktop. Please tell me what am I missing. This is the "Connect" screen, not the App Manager. See the docs[1] for more about the App Manager. [1]: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Using_the_App_Manager
Attached image BrowserTabs.jpg
Thanks for the detailed steps, Ryan! Mihai and I managed to connect to Fennec using App Manager and could see and debug the remote tabs. FF builds: Firefox 30 beta 7 and latest Nightly 2014-05-22 Environment: Win 7, 64-bit
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: