* style has been updated to match the future visual design * support for the remote-enabled preference * support for connection timeout

Comment on attachment 687712 [details] [diff] [review] patch v1 Review of attachment 687712 [details] [diff] [review]: ----------------------------------------------------------------- This is cool. I assume this isn't the final design, so I'll refrain from commenting on the aspect itself. r=me with comments addressed. Some thoughts: 1. Why not "Connect Firefox Developer Tools to" instead of "Connect to remote device" (just curious) 2. In the mockups, there used to be a "Want to debug Firefox Desktop itself? Click [here] etc." in the host name and port number form screen, are we not doing that anymore? 3. If 2., why aren't we distinguishing between tabs and processes in the available remote objects screen? Showing "Connect" as the first option is rather confusing, I can't assume that it's the current tab. ::: browser/devtools/framework/connect/connect.css @@ +4,4 @@ > } > > body { > + font-family: Arial, serif; Arial is sans-serif, I think you'd be better with it as an alternative. @@ +11,5 @@ > + box-shadow: 0 2px 3px black; > + background-color: #3C3E40; > +} > + > +h1 { These rules are applied to pretty generic selectors, I wouldn't do that. But it's premature to advise against it, so ignore my concerns :) @@ +36,5 @@ > margin-right: 10px; > } > > #submit { > + margin-left: 162px; Hello magic value. Is it because rounded margins or borders? I'm not entirely comfortable with this, but ok. @@ -68,5 @@ > - animation-duration: 0.6s; > - animation-name: anim; > - animation-direction: alternate; > - animation-iteration-count: infinite; > - animation-timing-function: linear; But, but.. The bubbly thing was so awesome! ::: browser/devtools/framework/connect/connect.js @@ +13,5 @@ > Cu.import("resource://gre/modules/Services.jsm"); > Cu.import("resource://gre/modules/devtools/dbg-client.jsm"); > > let gClient; > +let timeout; s/timeout/gConnectionTimeout/g @@ +46,2 @@ > let host = document.getElementById("host").value; > + if (host) { We shouldn't be silent here. Either immediately show an error, or use the preferred host and port and refill the inputs. @@ +66,5 @@ > + */ > +function onConnectionReady(aType, aTraits) { > + clearTimeout(timeout); > + gClient.listTabs(function(aResponse) { > + let strings = Services.strings.createBundle("chrome://browser/locale/devtools/connection-screen.properties"); Move the string bundle in a lazy getter somewhere else. I don't think there's any need to create this bundle each time a connection succeeds (suppose I connect to the wrong device and hit 'refresh'). @@ +73,5 @@ > + > + let parent = document.getElementById("actors"); > + > + // Add Global Process debugging... > + let globals = JSON.parse(JSON.stringify(aResponse)); Why JSON.parse(JSON.stringify())? @@ +81,5 @@ > + // be there). > + > + // Add one entry for each open tab. > + for (let i = 0; i < aResponse.tabs.length; i++) { > + buildLink(aResponse.tabs[i], parent, i == aResponse.selected); How about a for..of, since there's no need for a counter. @@ +94,4 @@ > > + a.title = a.textContent = strings.GetStringFromName("remoteProcess"); > + a.className = "remote-process"; > + a.href = "#"; It's a bit weird to have anchors instead of buttons here, but ok. @@ +114,5 @@ > }); > } > > +/** > + * Build one button for an actor. It would be nice to have @param types for tab, parent and selected. @@ +123,5 @@ > + openToolbox(tab); > + } > + > + a.title = a.textContent = tab.title; > + a.href = "#"; Anchoring all the things! :) @@ +133,5 @@ > + parent.appendChild(a); > +} > + > +/** > + * An error occured. Let's show it and return to the first screen. @param all the arguments! ::: browser/devtools/framework/connect/connect.xhtml @@ +30,3 @@ > </label> > <label> > + <input class="devtools-toolbarbutton" id="submit" type="submit" value="&connect;"></input> The connection prompt used to have a "Don't ask me again" checkbox, to automatically attempt a connection to the preferred host and port. I think we should not regress this functionality, but fixing this in a followup is ok. ::: browser/locales/en-US/chrome/browser/devtools/connection-screen.properties @@ +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/. > + > +# LOCALIZATION NOTE These strings are used inside the devtools connection > +# screen, which is available from the Web Developer sub-menu -> 'Connect'. "connect…", not 'Connect' Why not just reuse the localization note from connection-screen.dtd?

I can shed some light to some bits that are my fault... (In reply to Victor Porof [:vp] from comment #2) > @@ +73,5 @@ > > + > > + let parent = document.getElementById("actors"); > > + > > + // Add Global Process debugging... > > + let globals = JSON.parse(JSON.stringify(aResponse)); > > Why JSON.parse(JSON.stringify())? Poor man's object.clone(). Because we are subsequently deleting properties of the response, and doing it on the original object would break the following loop. > @@ +81,5 @@ > > + // be there). > > + > > + // Add one entry for each open tab. > > + for (let i = 0; i < aResponse.tabs.length; i++) { > > + buildLink(aResponse.tabs[i], parent, i == aResponse.selected); > > How about a for..of, since there's no need for a counter. The counter is necessary to build a special label for the selected tab (see the i == aResponse.selected check). > ::: browser/devtools/framework/connect/connect.xhtml > @@ +30,3 @@ > > </label> > > <label> > > + <input class="devtools-toolbarbutton" id="submit" type="submit" value="&connect;"></input> > > The connection prompt used to have a "Don't ask me again" checkbox, to > automatically attempt a connection to the preferred host and port. I think > we should not regress this functionality, but fixing this in a followup is > ok. We should also have a more discoverable way to reset that checkbox to "please start asking me again". For instance, when you switch from debugging over USB to debugging over WiFi, you want to change the hostname, and if you switch back, you will need to change it again.

(In reply to Panos Astithas [:past] from comment #3) > > ::: browser/devtools/framework/connect/connect.xhtml > > @@ +30,3 @@ > > > </label> > > > <label> > > > + <input class="devtools-toolbarbutton" id="submit" type="submit" value="&connect;"></input> > > > > The connection prompt used to have a "Don't ask me again" checkbox, to > > automatically attempt a connection to the preferred host and port. I think > > we should not regress this functionality, but fixing this in a followup is > > ok. > > We should also have a more discoverable way to reset that checkbox to > "please start asking me again". For instance, when you switch from debugging > over USB to debugging over WiFi, you want to change the hostname, and if you > switch back, you will need to change it again. Having a "stop me" button while connecting seems like a good solution to me, which would revert back to the initial screen again (implicitly with the checkbox somewhere ready for us to untick).

(In reply to Victor Porof [:vp] from comment #4) > (In reply to Panos Astithas [:past] from comment #3) > > > ::: browser/devtools/framework/connect/connect.xhtml > > > @@ +30,3 @@ > > > > </label> > > > > <label> > > > > + <input class="devtools-toolbarbutton" id="submit" type="submit" value="&connect;"></input> > > > > > > The connection prompt used to have a "Don't ask me again" checkbox, to > > > automatically attempt a connection to the preferred host and port. I think > > > we should not regress this functionality, but fixing this in a followup is > > > ok. > > > > We should also have a more discoverable way to reset that checkbox to > > "please start asking me again". For instance, when you switch from debugging > > over USB to debugging over WiFi, you want to change the hostname, and if you > > switch back, you will need to change it again. > > Having a "stop me" button while connecting seems like a good solution to me, > which would revert back to the initial screen again (implicitly with the > checkbox somewhere ready for us to untick). This reminds me of our Toronto plan to add a Connect/Disconnect button to the debugger toolbar (which would now belong to the toolbox toolbar).

Comment on attachment 687712 [details] [diff] [review] patch v1 Review of attachment 687712 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/framework/connect/connect.js @@ +122,5 @@ > + a.onclick = function() { > + openToolbox(tab); > + } > + > + a.title = a.textContent = tab.title; Can you use tab.url if tab.title is undefined? That's the case in B2G at least.

Patch coming… (In reply to Victor Porof [:vp] from comment #2) > This is cool. I assume this isn't the final design, so I'll refrain from > commenting on the aspect itself. Please do. > Some thoughts: > 1. Why not "Connect Firefox Developer Tools to" instead of "Connect to > remote device" (just curious) I don't know. What's best? > 2. In the mockups, there used to be a "Want to debug Firefox Desktop itself? > Click [here] etc." in the host name and port number form screen, are we not > doing that anymore? > > 3. If 2., why aren't we distinguishing between tabs and processes in the > available remote objects screen? Showing "Connect" as the first option is > rather confusing, I can't assume that it's the current tab. The current UI is what has been decided for the v1. We can re-discuss that if you want (but not in this bug). > ::: browser/devtools/framework/connect/connect.css > @@ +4,4 @@ > > } > > > > body { > > + font-family: Arial, serif; > > Arial is sans-serif, I think you'd be better with it as an alternative. Done. > ::: browser/devtools/framework/connect/connect.js > @@ +13,5 @@ > > Cu.import("resource://gre/modules/Services.jsm"); > > Cu.import("resource://gre/modules/devtools/dbg-client.jsm"); > > > > let gClient; > > +let timeout; > > s/timeout/gConnectionTimeout/g done. > @@ +46,2 @@ > > let host = document.getElementById("host").value; > > + if (host) { > > We shouldn't be silent here. Either immediately show an error, or use the > preferred host and port and refill the inputs. I made the form unvalidable if we don't get value. So this is not needed anymore. > @@ +66,5 @@ > > + */ > > +function onConnectionReady(aType, aTraits) { > > + clearTimeout(timeout); > > + gClient.listTabs(function(aResponse) { > > + let strings = Services.strings.createBundle("chrome://browser/locale/devtools/connection-screen.properties"); > > Move the string bundle in a lazy getter somewhere else. I don't think > there's any need to create this bundle each time a connection succeeds > (suppose I connect to the wrong device and hit 'refresh'). Fixed. > The connection prompt used to have a "Don't ask me again" checkbox, to > automatically attempt a connection to the preferred host and port. I think > we should not regress this functionality, but fixing this in a followup is > ok. We can add that, but I think this might be confusing (how do you uncheck it?), and it saves only one click. Please file a follow-up if you think this is important. > ::: > browser/locales/en-US/chrome/browser/devtools/connection-screen.properties > @@ +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/. > > + > > +# LOCALIZATION NOTE These strings are used inside the devtools connection > > +# screen, which is available from the Web Developer sub-menu -> 'Connect'. > > "connect…", not 'Connect' > Why not just reuse the localization note from connection-screen.dtd? ok.(In reply to Panos Astithas [:past] from comment #6) > ::: browser/devtools/framework/connect/connect.js > @@ +122,5 @@ > > + a.onclick = function() { > > + openToolbox(tab); > > + } > > + > > + a.title = a.textContent = tab.title; > > Can you use tab.url if tab.title is undefined? That's the case in B2G at > least. Done.

I'm not sure to understand what we should do with the remote process button. Can I get the different possible screnarios? Who will click on the Connect button, and why.

Not distinguishing tabs and processes in the available remote objects screen may yield some confusion as to what "Connect" is.

The Remote Protocol allows all kinds of interactions with the debugging target, by having a client connect to a particular actor that implements some specific functionality. Right now, the two remotable tools we have, the console and the debugger, act differently depending on whether the selected actor corresponds to a tab or the global process. In the latter case, the web console behaves more like the Error Console, showing all errors and notifications, irrespective of the DOMWindow they come from. In a similar vein, the debugger puts the whole remote process in debug mode, making the remote instance run slower than before. After that preamble, I should note that the most common case by far for the Connect button will be remote debugging (in the broad sense) a Firefox for Android or Firefox OS instance. A less common case (but maybe more common in the future) will be remote debugging another desktop Firefox instance. Desktop Firefox implements all global- and tab-scoped actors, so both content and chrome debugging (in the broad sense) will work. Fennec and B2G currently implement content debugging fully, and chrome debugging only for the web console. Chrome JS debugging is not here yet, but we have plans to add it soon. In IRC I suggested to add a short explanatory text about the Remote Process item, for the benefit of those (currently expert users) who connect to that meaning to use the web console, but experiencing a performance hit from the chrome debugging that goes with it. Something along the lines of: "Selecting Remote Process will put the entire remote application in debug mode, enabling chrome debugging and a global console"

(In reply to Victor Porof [:vp] from comment #10) > Created attachment 693443 [details] > Connection screen > > Not distinguishing tabs and processes in the available remote objects screen > may yield some confusion as to what "Connect" is. How did this list come up? If this is the list of tabs in the same instance, then it is not something regular users will see. But even in that case, Connect corresponds to the current tab, and is a regular tab. This is how Paul could actually debug it[*] :-) *: and by "debug" I mean use the web console against it, because the JS debugger ignores chrome: scripts. In the not too distant future, "debug" would also include the Page Inspector and more.

I'll land this patch. Follow-up: bug 822702

(In reply to Panos Astithas [:past] from comment #12) > How did this list come up? If this is the list of tabs in the same instance, > then it is not something regular users will see. But even in that case, > Connect corresponds to the current tab, and is a regular tab. This is how > Paul could actually debug it[*] :-) > Of course. I'm not saying that we shouldn't display the Connect tab. I started a server in the same Firefox instance I was testing the connection screen, which I agree is a far fetched thing to do for a regular user. Still, as pointed out in bug 822702, some tab titles may have confusing names. It'd be great to have all those link obviously grouped as "Remote tabs".

(In reply to Victor Porof [:vp] from comment #14) > (In reply to Panos Astithas [:past] from comment #12) > > > How did this list come up? If this is the list of tabs in the same instance, > > then it is not something regular users will see. But even in that case, > > Connect corresponds to the current tab, and is a regular tab. This is how > > Paul could actually debug it[*] :-) > > > > Of course. I'm not saying that we shouldn't display the Connect tab. > I started a server in the same Firefox instance I was testing the connection > screen, which I agree is a far fetched thing to do for a regular user. > Still, as pointed out in bug 822702, some tab titles may have confusing > names. It'd be great to have all those link obviously grouped as "Remote > tabs". Yes, that make sense.