Closed
Bug 1135191
Opened 9 years ago
Closed 9 years ago
Move runtime popup panel to a panel with toggle
Categories
(DevTools Graveyard :: WebIDE, defect)
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
| Tracking | Status | |
|---|---|---|
| firefox40 | --- | fixed |
People
(Reporter: jfong, Assigned: jfong)
References
Details
Attachments
(1 file, 10 obsolete files)
|
125.20 KB,
patch
|
jfong
:
review+
|
Details | Diff | Splinter Review |
No description provided.
|
Assignee | |
Updated•9 years ago
|
Summary: Move runtime popup panel to an iframe with toggle → Move runtime popup panel to a panel with toggle
|
|
Assignee | |
Comment 1•9 years ago
|
||
I ended up making the test_autoconnect_runtime.html pass with some changes that you can see there - just verifying that it is okay for this patch. Also I believe I fixed https://bugzilla.mozilla.org/show_bug.cgi?id=1143028 through this patch but also need you to see if that's correct.
Attachment #8577311 -
Flags: feedback?(jryans)
|
|
Assignee | |
Comment 2•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5264c09b35b2
Comment on attachment 8577311 [details] [diff] [review] Bug1135191.patch Review of attachment 8577311 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/webide/content/project-listing.js @@ +28,5 @@ > function onAppManagerUpdate(event, what) { > switch (what) { > + case "connection": > + case "project": > + case "project-validated": You need to reconcile these new case entries with the ones at the bottom. After you reach the first "break;" for given case, the rest of the switch is ignored. Besides, these new lines seem to duplicate functionality for some cases. There should only ever be one occurrence of a given case in a single switch statement. @@ +35,5 @@ > + break; > + case "project-is-not-running": > + case "project-is-running": > + case "list-tabs-response": > + projectList.updateCommands(); Please read what |projectList.updateCommands| does, and decide what events would actually matter. What state changes can cause the function to behave differently? Those are the (only!) times it needs to be called. Since you are breaking up an existing method, you should not assume that all of the same events are correct for the individual pieces. ::: browser/devtools/webide/content/runtime-listing.js @@ +29,5 @@ > +}); > + > +function onAppManagerUpdate(event, what) { > + switch (what) { > + case "runtimelist": Look carefully at this list, especially if the action button are moved back into webide.js. ::: browser/devtools/webide/content/runtime-listing.xhtml @@ +11,5 @@ > + > +<html xmlns="http://www.w3.org/1999/xhtml"> > + <head> > + <meta charset="utf8"/> > + <link rel="stylesheet" href="chrome://webide/skin/project-listing.css" type="text/css"/> Shouldn't this frame get a separate CSS file? If there are common parts, make a file for those that they both link to. ::: browser/devtools/webide/content/runtime-panel.js @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +let RuntimePanel = { > + // TODO: Expand function to save toggle state. > + toggle: function(sidebarsEnabled, triggerPopup) { This is a confusing API (same goes for ProjectPanel) because it blends two independent behaviors, and it's keyed only off a boolean. Boolean params should generally be avoided[1], since they make it much harder to read the call site later and understand what's happening. Even now, while reviewing this code, I forgot what it means! Additionally, the boolean is not even used if sidebars are enabled. So, what about: toggleSidebar() showPopup() and then webide.js calls the correct one based on whether sidebars are enabled or not. I think that would be easier to read. [1]: http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html ::: browser/devtools/webide/content/webide.js @@ +309,5 @@ > let win = document.querySelector("window"); > win.classList.add("busy") > win.classList.add("busy-undetermined"); > + if (!runtimeList.sidebarsEnabled) { > + runtimeList.updateCommands(); This covers the non-sidebar case, but the sidebar case also needs to know when WebIDE's busy state changes. I think right thing is for webide.js to emit an event, which is new behavior, since all messages have only come from app-manager.js so far in the UI. So, you then listen for a "busy" event in the frame. @@ +413,1 @@ > _actionsToLog: new Set(), Add a new section header above here, such as: /********** ACTIONS **********/ @@ +961,5 @@ > UI.openProject(); > }, > > showTroubleShooting: function() { > + runtimeList.showTroubleShooting(); This is a function of the overall UI, they should not be moved to |runtimeList|. |runtimeList| can tell the main document to do this as needed. @@ +966,4 @@ > }, > > showAddons: function() { > + runtimeList.showAddons(); This is a function of the overall UI, they should not be moved to |runtimeList|. |runtimeList| can tell the main document to do this as needed. ::: browser/devtools/webide/modules/app-manager.js @@ +137,5 @@ > this.update("connection"); > }, > > get connected() { > + return this.connection && Revert this, it's not needed with changes from bug 1143028. @@ +432,5 @@ > > isMainProcessDebuggable: function() { > // Fx <37 exposes chrome tab actors on RootActor > // Fx >=37 exposes a dedicated actor via attachProcess request > + return this.connection && Revert this, it's not needed with changes from bug 1143028. ::: browser/devtools/webide/modules/project-list.js @@ +115,5 @@ > } > }, > > _buildProjectPanelTabs: function() { > + if (!AppManager.tabStore) { This can be removed once we're calling |AM.init|. @@ +186,5 @@ > + doc.querySelector("#new-app").setAttribute("disabled", "true"); > + doc.querySelector("#packaged-app").setAttribute("disabled", "true"); > + doc.querySelector("#hosted-app").setAttribute("disabled", "true"); > + } else { > + parentDoc.querySelector("#cmd_newApp").setAttribute("disabled", "true"); It seems more logical to use |doc| here, since that's always "the document I am loaded in". For the case of no sidebars, doc === parentDoc anyway. It seems clearer to stick with |doc| everywhere when possible. ::: browser/devtools/webide/modules/runtime-list.js @@ +23,5 @@ > +module.exports = RuntimeList = function(window, parentWindow) { > + EventEmitter.decorate(this); > + this._doc = window.document; > + this._UI = parentWindow.UI; > + this._Cmds = parentWindow.Cmds; Remove this, it doesn't seem to be used. @@ +32,5 @@ > + if (this._sidebarsEnabled) { > + this._panelNodeEl = "button"; > + } > + > + return this; You don't need to return anything from a constructor function since we're calling it with |new|. Please clean up |ProjectList| too. @@ +44,5 @@ > + get sidebarsEnabled() { > + return this._sidebarsEnabled; > + }, > + > + updateRuntimeButton: function() { Does this belong here? It's not inside the frame. The project button is still in webide.js, and I'd think this belongs there too. @@ +187,5 @@ > + > + parentDoc.querySelector("#cmd_showRuntimePanel").removeAttribute("disabled"); > + > + // Action commands > + let playCmd = parentDoc.querySelector("#cmd_play"); What is your reasoning for moving the action commands here? They aren't moving to a new frame, so I was guessing they'd be left in webide.js (or at least into some module that only it uses and not the new frames). @@ +228,5 @@ > + } > + } > + > + // Remove command > + let removeCmdNode = parentDoc.querySelector("#cmd_removeProject"); Why is this here? ::: browser/devtools/webide/runtime-panel.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public This seems like a half-copied file in the wrong place, seems like it should be removed? ::: browser/devtools/webide/test/test_autoconnect_runtime.html @@ +78,5 @@ > + deferred = promise.defer(); > + win.AppManager.connection.once( > + win.Connection.Events.CONNECTED, > + () => deferred.resolve()); > + items[0].click(); This test change is not correct. The feature is supposed to connect automatically if your last runtime is still available this time. Since it's already chosen earlier in the test, we should not need this block in the test.
Attachment #8577311 -
Flags: feedback?(jryans)
|
|
Assignee | |
Comment 4•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #3) > > ::: browser/devtools/webide/content/runtime-listing.xhtml > @@ +11,5 @@ > > + > > +<html xmlns="http://www.w3.org/1999/xhtml"> > > + <head> > > + <meta charset="utf8"/> > > + <link rel="stylesheet" href="chrome://webide/skin/project-listing.css" type="text/css"/> > > Shouldn't this frame get a separate CSS file? If there are common parts, > make a file for those that they both link to. Since there are more common parts than not, I'll just rename the css file to be more generic. Something like panel-listing.css maybe? > > ::: browser/devtools/webide/content/runtime-panel.js > @@ +3,5 @@ > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > + > > +let RuntimePanel = { > > + // TODO: Expand function to save toggle state. > > + toggle: function(sidebarsEnabled, triggerPopup) { > > This is a confusing API (same goes for ProjectPanel) because it blends two > independent behaviors, and it's keyed only off a boolean. > > Boolean params should generally be avoided[1], since they make it much > harder to read the call site later and understand what's happening. Even > now, while reviewing this code, I forgot what it means! > > Additionally, the boolean is not even used if sidebars are enabled. > > So, what about: > > toggleSidebar() > showPopup() Yep, makes sense. > ::: browser/devtools/webide/modules/runtime-list.js > @@ +187,5 @@ > > + > > + parentDoc.querySelector("#cmd_showRuntimePanel").removeAttribute("disabled"); > > + > > + // Action commands > > + let playCmd = parentDoc.querySelector("#cmd_play"); > > What is your reasoning for moving the action commands here? > > They aren't moving to a new frame, so I was guessing they'd be left in > webide.js (or at least into some module that only it uses and not the new > frames). Originally it was just to move the entire function over to the new module but it seems best to just split the original function from webide.js up into smaller pieces (where some are part of the module and some stay in webide.js). > > ::: browser/devtools/webide/runtime-panel.js > @@ +1,1 @@ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > This seems like a half-copied file in the wrong place, seems like it should > be removed? Do you mean the license info? > > ::: browser/devtools/webide/test/test_autoconnect_runtime.html > @@ +78,5 @@ > > + deferred = promise.defer(); > > + win.AppManager.connection.once( > > + win.Connection.Events.CONNECTED, > > + () => deferred.resolve()); > > + items[0].click(); > > This test change is not correct. The feature is supposed to connect > automatically if your last runtime is still available this time. > > Since it's already chosen earlier in the test, we should not need this block > in the test. Ah yeah, this is the one that I said I had problems with running the test on. It would stall on the "runtime-apps-found" event and the test would eventually time out. I'll see what the fixes based on your feedback will do on resolving it.
Flags: needinfo?(jryans)
|
|
Assignee | |
Comment 5•9 years ago
|
||
To follow up, would making two functions for toggle() for the panel be needed if we will subsequently remove the popup portion shortly after this bug?
(In reply to Jen Fong-Adwent [:ednapiranha] from comment #5) > To follow up, would making two functions for toggle() for the panel be > needed if we will subsequently remove the popup portion shortly after this > bug? Yes, I think we should still do it because the current version confusing to read. Even if the code it only around for a short time, I don't want to impair readability. Plus things always seem to live longer than we expect... :)
(In reply to Jen Fong-Adwent [:ednapiranha] from comment #4) > (In reply to J. Ryan Stinnett [:jryans] from comment #3) > > > > ::: browser/devtools/webide/content/runtime-listing.xhtml > > @@ +11,5 @@ > > > + > > > +<html xmlns="http://www.w3.org/1999/xhtml"> > > > + <head> > > > + <meta charset="utf8"/> > > > + <link rel="stylesheet" href="chrome://webide/skin/project-listing.css" type="text/css"/> > > > > Shouldn't this frame get a separate CSS file? If there are common parts, > > make a file for those that they both link to. > > Since there are more common parts than not, I'll just rename the css file to > be more generic. Something like panel-listing.css maybe? Sure, that's fine too. > > ::: browser/devtools/webide/modules/runtime-list.js > > @@ +187,5 @@ > > > + > > > + parentDoc.querySelector("#cmd_showRuntimePanel").removeAttribute("disabled"); > > > + > > > + // Action commands > > > + let playCmd = parentDoc.querySelector("#cmd_play"); > > > > What is your reasoning for moving the action commands here? > > > > They aren't moving to a new frame, so I was guessing they'd be left in > > webide.js (or at least into some module that only it uses and not the new > > frames). > > Originally it was just to move the entire function over to the new module > but it seems best to just split the original function from webide.js up into > smaller pieces (where some are part of the module and some stay in > webide.js). Yes, splitting it up seems best. > > ::: browser/devtools/webide/runtime-panel.js > > @@ +1,1 @@ > > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > > > This seems like a half-copied file in the wrong place, seems like it should > > be removed? > > Do you mean the license info? No, I mean this entire file should be removed. There is: * browser/devtools/webide/runtime-panel.js (this file, with content that still says project panel, that does not appear to used) * browser/devtools/webide/content/runtime-panel.js (a different file, it's actually used, please keep it) > > ::: browser/devtools/webide/test/test_autoconnect_runtime.html > > @@ +78,5 @@ > > > + deferred = promise.defer(); > > > + win.AppManager.connection.once( > > > + win.Connection.Events.CONNECTED, > > > + () => deferred.resolve()); > > > + items[0].click(); > > > > This test change is not correct. The feature is supposed to connect > > automatically if your last runtime is still available this time. > > > > Since it's already chosen earlier in the test, we should not need this block > > in the test. > > Ah yeah, this is the one that I said I had problems with running the test > on. It would stall on the "runtime-apps-found" event and the test would > eventually time out. I'll see what the fixes based on your feedback will do > on resolving it. If it still fails, I would guess we need to add an event somewhere that add an implicit relationship before.
Flags: needinfo?(jryans)
I noticed a follow up fix that's needed after the project list changes:
For runtime apps, we fetch their icons in the background asynchronously. We get them pretty fast, but they don't see to get draw into the project list anymore. That's because there was never an event after this is done, since we'd previously redraw the project list each time the button was clicked.
So, do something like:
1. Change app-manager.js#onConnectionChanged:
front.fetchIcons();
to:
front.fetchIcons().then(() => this.update("runtime-app-icons"))
2. Update the project list for this new event|
|
Assignee | |
Comment 9•9 years ago
|
||
Updated patch with a note: fetchIcons doesn't seem to redraw the icons in time after the promise is returned unless I call setTimeout with 1000 ms. Not sure why the delay is necessary.
Attachment #8577311 -
Attachment is obsolete: true
Attachment #8580209 -
Flags: review?(jryans)
|
|
Assignee | |
Comment 10•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa19e4e188ab
Comment on attachment 8580209 [details] [diff] [review] Bug1135191.patch Review of attachment 8580209 [details] [diff] [review]: ----------------------------------------------------------------- Issues noticed in non-sidebar mode: 1. The toolbox styles seem wrong when connected to FxOS, since it should take up the whole space below the toolbar, but it no longer does 2. The runtime app icons are not appearing 3. If you disconnect from a simulator after choosing a runtime app, the project button becomes disabled and unusable 4. The device settings icon disappeared (you changed the class I think) 5. The runtime apps remain shown in the project list after disconnecting from a runtime (even re-opening WebIDE did not clear them) 6. The action buttons are active when no project is selected Issues noticed in sidebar mode: #1 above, though I am not sure what's correct now, given the toolbox placement, see #7. #5 above. 7. I just now noticed the position of the toolbox when using sidebars... Should it be inside the "body" area, so with the sidebars around, or take up the full width as it does now? I can't decide. ::: browser/devtools/webide/content/runtime-listing.js @@ +31,5 @@ > + AppManager.off("app-manager-update", onAppManagerUpdate); > +}); > + > +window.parent.addEventListener("busy", function onBusyUpdate() { > + window.parent.removeEventListener("busy", onBusyUpdate); This should be removed in |unload|, since there can be many |busy| events for the life of the page, not just one. Anyway, you might change to event-emitter, we'll see. @@ +36,5 @@ > + runtimeList.updateCommands(); > +}, true); > + > +window.parent.addEventListener("unbusy", function onUnbusyUpdate() { > + window.parent.removeEventListener("unbusy", onUnbusyUpdate); Same here. @@ +45,5 @@ > + if (!runtimeList.sidebarsEnabled) { > + return; > + } > + > + switch (what) { This list might change slightly, see longer discussion in |webide.js|. ::: browser/devtools/webide/content/webide.js @@ +12,5 @@ > const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}); > const {require} = devtools; > const {Services} = Cu.import("resource://gre/modules/Services.jsm"); > const {AppProjects} = require("devtools/app-manager/app-projects"); > const {Connection} = require("devtools/client/connection-manager"); Check first, but this appears unused now. @@ -72,2 @@ > > - this.updateCommands(); I think we still want to call this on init, since |UI| still contains commands for action buttons, etc. @@ +92,5 @@ > this.autoSelectProject(); > + if (!projectList.sidebarsEnabled) { > + projectList.update(); > + } > + if (!runtimeList.sidebarsEnabled) { Why is this needed? It wasn't about runtimes before... @@ +139,5 @@ > window.removeEventListener("focus", this.onfocus, true); > AppManager.off("app-manager-update", this.appManagerUpdate); > AppManager.destroy(); > projectList = null; > + runtimeList = null; |projectList| and |runtimeList| should have destroy methods that are called here which null out the window references and other things that you pass in at |init| time. @@ +170,5 @@ > // when run at the end of `init`. ¯\(°_o)/¯ > showDoorhanger({ window, type: "deveditionpromo", anchor: document.querySelector("#deck") }); > }, > > appManagerUpdate: function(event, what, details) { Looking at this list again, it seems unfortunate that we're duplicate the event handling for sidebar vs. non-sidebar, since it's hard to get right with even one list. It would be nice for the |runtime/project-list| *modules* to listen, instead of each of the windows that contain them. That way, it seems like we'd end up with only one list instead of two, and the modules always get the right events, no matter window loads them. (Or do the lists differ in some way that I am missing?) Right now, it's kind of indirect for no particular reason. Does that seem like too large of a change to ask for? @@ +218,2 @@ > this.updateCommands(); > + this.updateRemoveProjectButton(); Do we need this here? It seems to only check |selectedProject|, so I think it only needs to wait on |project|, like it already does above. @@ -188,2 @@ > this.updateCommands(); > - projectList.update(); Should this be preserved so we request the list of remote tabs? I *think* it's only needed for |list-tabs-response|, not the other two. @@ +220,5 @@ > break; > case "runtime-details": > this.updateRuntimeButton(); > break; > case "runtime-changed": Nit: It would be nice to rename this "runtime", since all the other events mean "X-changed", but you don't have to do this now. @@ +236,1 @@ > this.updateCommands(); I think it may be safe to remove this: |UI.updateCommands| changes when the following change: * project * connection * project running but I don't think it cares about validation. @@ +236,4 @@ > this.updateCommands(); > this.updateProjectButton(); > this.updateProjectEditorHeader(); > + this.updateRemoveProjectButton(); Do we need this here? It seems to only check |selectedProject|, so I think it only needs to wait on |project|, like it already does above. @@ +254,1 @@ > break; Shouldn't we add the |runtime-apps-icons| case? @@ +309,5 @@ > win.classList.add("busy-undetermined"); > + if (!runtimeList.sidebarsEnabled) { > + runtimeList.updateCommands(); > + } > + document.dispatchEvent(busyEvent); Would it reduce event confusion if we use the same approach here as we do in app-manager, based on event-emitter? Clients in child frames would do |window.parent.UI.on(...)| to listen. @@ +412,5 @@ > }, > > /********** RUNTIME **********/ > > + updateCommands: function() { I think this function belongs back under the "COMMANDS" section header that you removed. Or at least, it's not just about RUNTIME. ::: browser/devtools/webide/modules/app-manager.js @@ +119,5 @@ > this._appsFront = null; > } > this._listTabsResponse = null; > } else { > + let self = this; Are you sure this is needed? It should not be when using an arrow function. @@ +138,5 @@ > .then(() => { > this.checkIfProjectIsRunning(); > this.update("runtime-apps-found"); > + front.fetchIcons().then(() => { > + setTimeout(() => self.update("runtime-apps-icons"), 1000); If I understand correctly, the |setTimeout| is needed for XUL popup wierdness. So, we should emit the event immediately since that's when the icons really are ready. Then, when handling the event, we can choose to use the |setTimeout| if needed. Otherwise, every event consumer gets a delayed event unnecessarily. Also, did you try a timeout value of 0? Can't remember if you said you did. That's what's used in |project-panel.js|, hopefully the same (terrible) thing can work in this case? ::: browser/devtools/webide/modules/runtime-list.js @@ +21,5 @@ > +module.exports = RuntimeList = function(window, parentWindow) { > + EventEmitter.decorate(this); > + this._doc = window.document; > + this._UI = parentWindow.UI; > + this._Cmds = parentWindow.Cmds; Depending on |_Cmds| still feels strange to me... but I don't have a great answer. I suppose this is another thing that can be cleaned up a bit when the old mode is removed. ::: browser/devtools/webide/test/sidebars/chrome.ini @@ +5,5 @@ > ../head.js > > [test_duplicate_import.html] > [test_import.html] > +[test_addons.html] There are quite a few tests of runtime code missing here... Shouldn't you copy them too? Come to think of it, there seem be some project ones missing too, why's that? ::: browser/devtools/webide/themes/webide.css @@ +150,5 @@ > min-width: 200px; > max-width: 400px; > } > > +#project-listing-panel, #runtime-listing-panel { Can we give each of these sets of things a common class instead of listing both IDs?
Attachment #8580209 -
Flags: review?(jryans) → review-
|
|
Assignee | |
Comment 12•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #11) > Comment on attachment 8580209 [details] [diff] [review] > Bug1135191.patch > > Review of attachment 8580209 [details] [diff] [review]: > ----------------------------------------------------------------- > > Issues noticed in sidebar mode: > > #1 above, though I am not sure what's correct now, given the toolbox > placement, see #7. > #5 above. > 7. I just now noticed the position of the toolbox when using sidebars... > Should it be inside the "body" area, so with the sidebars around, or take up > the full width as it does now? I can't decide. > I think for now it should take up the full width - it's easy to toggle to hide it and show the panels again. > ::: browser/devtools/webide/content/webide.js > @@ +12,5 @@ > > const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}); > > const {require} = devtools; > > const {Services} = Cu.import("resource://gre/modules/Services.jsm"); > > const {AppProjects} = require("devtools/app-manager/app-projects"); > > const {Connection} = require("devtools/client/connection-manager"); > > Check first, but this appears unused now. > This is needed for the tests, where win.Connection is called. > @@ +138,5 @@ > > .then(() => { > > this.checkIfProjectIsRunning(); > > this.update("runtime-apps-found"); > > + front.fetchIcons().then(() => { > > + setTimeout(() => self.update("runtime-apps-icons"), 1000); > > If I understand correctly, the |setTimeout| is needed for XUL popup > wierdness. So, we should emit the event immediately since that's when the > icons really are ready. Then, when handling the event, we can choose to use > the |setTimeout| if needed. Otherwise, every event consumer gets a delayed > event unnecessarily. > > Also, did you try a timeout value of 0? Can't remember if you said you did. > That's what's used in |project-panel.js|, hopefully the same (terrible) > thing can work in this case? > Yep, tried that and it did not work. I'm going to leave this unchanged so you can help test on the next feedback cycle - I also tried to move the event outside of the setTimeout and that didn't work either. > > ::: browser/devtools/webide/test/sidebars/chrome.ini > @@ +5,5 @@ > > ../head.js > > > > [test_duplicate_import.html] > > [test_import.html] > > +[test_addons.html] > > There are quite a few tests of runtime code missing here... Shouldn't you > copy them too? > > Come to think of it, there seem be some project ones missing too, why's that? > Ah, forgot to add them into the list.
|
|
Assignee | |
Comment 13•9 years ago
|
||
One thing I haven't been able to fix "properly" - the fetchIcons call. Tried moving the event outside of setTimeout and it didn't work; also tried setting the timeout to 0 and that did not work. Not really why, even though the URLs for each icon have all been retrieved.
Attachment #8580209 -
Attachment is obsolete: true
Attachment #8583893 -
Flags: feedback?(jryans)
|
|
Assignee | |
Comment 14•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=144340aa587f
Comment on attachment 8583893 [details] [diff] [review] Bug1135191.patch Review of attachment 8583893 [details] [diff] [review]: ----------------------------------------------------------------- The larger UX issues I noted last time appear resolved, so that's great! Please rebase this against bug 1146542 which should land soon. Also, let's talk about this event design in case it's unclear. I avoid reading the events in detail, since I expect it to change heavily. ::: browser/devtools/webide/content/project-listing.js @@ +38,1 @@ > projectList.update(); Hmm, this was not quite what I expecting after last round. You've added a new layer of indirection, so most things now look like: 1. Something changed, app-manager.js emits an event 2. webide.js gets event, emits a project-update event 3. project-listing.js gets event, call projectList.update Maybe I was not specific enough in my feedback last time? I was imagining that *project-list.js* (the module, not the frame JS file project-listing.js) would listen to AppManager events directly. That way, all events are handled there. This means quite a few events can be removed from webide.js, for example. With such a change, events would look like: 1. Something changed, app-manager.js emits an event 2. project-list.js gets event, calls this.update (or similar) Am I missing something? Is there a reason for everything to flow through webide.js? Additionally, bug 1146542 which you gave feedback on renames a number of events, so I think I'll skip reviewing the exact events for this round, since they will change anyway, and we should resolve the design discussion above first. ::: browser/devtools/webide/modules/app-manager.js @@ +136,5 @@ > }) > .then(() => { > this.checkIfProjectIsRunning(); > + front.fetchIcons().then(() => { > + this.update("runtime-apps-found"); Why is this event *after* the icons now? @@ +137,5 @@ > .then(() => { > this.checkIfProjectIsRunning(); > + front.fetchIcons().then(() => { > + this.update("runtime-apps-found"); > + setTimeout(() => this.update("runtime-apps-icons"), 1000); The real issue here is a bug in |fetchIcons|. It uses promise.all, but that method aborts when sees the first rejection. And several apps do reject, since they have no icon, so it just skips the rest after that. So, import DevToolsUtils.js into app-actor-front.js: const DevToolsUtils = devtools.require("devtools/toolkit/DevToolsUtils.js"); and change |fetchIcons| to use |DevToolsUtils.settleAll| which is meant for exactly this sort of case. ::: browser/devtools/webide/test/sidebars/chrome.ini @@ +6,5 @@ > > [test_duplicate_import.html] > [test_import.html] > +[test_addons.html] > +[test_autoconnect_runtime.html] Is it simpler to just add all of them? I still think you've skipped quite a lot of tests that are impacted by the changes made here: test_fullscreenToolbox test_manifestUpdate test_newapp test_runtime (I guess you copied this one, but did not list it here?) test_telemetry They all seem to dive into different panels and use various elements. They could be impacted by changes here. And what about the browser tests? Still none added?
Attachment #8583893 -
Flags: feedback?(jryans)
|
|
Assignee | |
Comment 16•9 years ago
|
||
Attachment #8583893 -
Attachment is obsolete: true
Attachment #8588616 -
Flags: feedback?(jryans)
Comment on attachment 8588616 [details] [diff] [review] Bug1135191.patch Review of attachment 8588616 [details] [diff] [review]: ----------------------------------------------------------------- There still seem to be many things missing here from what we have discussed... Is there an issue you were hoping for me to debug before finishing the patch, or is this meant to be ready for review? Adding a comment when attaching an updated patch on what's changed / current status always helps, especially for large things! ::: browser/devtools/webide/content/webide.js @@ +174,2 @@ > this.autoConnectRuntime(); > + if (!runtimeList.sidebarsEnabled) { I am confused... I thought we agreed to move watching for such events into the module? I will stop reviewing here until we discuss this further. ::: browser/devtools/webide/test/sidebars/chrome.ini @@ +1,1 @@ > [DEFAULT] I think this chrome test list seems reasonable. For the 3rd time, where are the browser tests? (The |browser.ini| list is empty for sidebars.) We should include browser_tabs.js for sidebars.
Attachment #8588616 -
Flags: feedback?(jryans)
|
|
Assignee | |
Comment 18•9 years ago
|
||
Couple of things: - After the clarification, moved the event listeners to the modules for both AppManager and WebIDE. Feedback on this would be good to see if this is the ideal direction we want to go with. If it looks good, feel free to treat this patch iteration as the full review. - Tests all pass but I am having a strange issue in test_runtime.html with the popup version (sidebars disabled) if I run the entire suite - it hangs on "Toolbox promise exists" and times out before continuing the rest of the tests. If I run this test by itself it does not time out. In both situations, the test have no errors.
Attachment #8588616 -
Attachment is obsolete: true
Attachment #8589351 -
Flags: feedback?(jryans)
Comment on attachment 8589351 [details] [diff] [review] Bug1135191.patch Review of attachment 8589351 [details] [diff] [review]: ----------------------------------------------------------------- Okay, looking much better design wise! The event setup here feels right to me. I did not notice any other major issues while testing. All seem explained by things I've commented below. ::: browser/devtools/webide/content/webide.js @@ +198,5 @@ > case "project-stopped": > case "project-started": > case "runtime-global-actors": > this.updateCommands(); > + this.updateRemoveProjectButton(); I don't think this is needed here, as |updateRemoveProjectButton| only depend on whether there is a project. So, it should only need the |project| event. @@ +376,2 @@ > > + update: function(what, details) { Please document the possible events, just like we do in AppManager now. @@ +437,5 @@ > > + let projectPanelCmd = document.querySelector("#cmd_showProjectPanel"); > + > + if (document.querySelector("window").classList.contains("busy")) { > + projectPanelCmd.setAttribute("disabled", "true"); This should be moved back into the set at the top of this function. As written, we never reach here when busy, since we already returned during the top block above. ::: browser/devtools/webide/modules/app-manager.js @@ +185,5 @@ > }) > .then(() => { > this.checkIfProjectIsRunning(); > this.update("runtime-targets", { type: "apps" }); > + front.fetchIcons().then(() => this.update("runtime-apps-icons")); Please document this new event in the |update| method's list. ::: browser/devtools/webide/modules/project-list.js @@ +26,5 @@ > > if (this._sidebarsEnabled) { > this._panelNodeEl = "div"; > + this.onWebIDEUpdate = this.onWebIDEUpdate.bind(this); > + this._UI.on("webide-update", this.onWebIDEUpdate); Why is this only needed when sidebars are enabled? I would think it's needed in both modes. @@ +43,5 @@ > get sidebarsEnabled() { > return this._sidebarsEnabled; > }, > > + appManagerUpdate: function(event, what, details) { You should also listen for the new "runtime-app-icons" here. @@ +47,5 @@ > + appManagerUpdate: function(event, what, details) { > + // Got a message from app-manager.js > + // See AppManager.update() for descriptions of what these events mean. > + switch (what) { > + case "connection": Why is "connection" needed? It was not needed previously. @@ +65,5 @@ > + }, > + > + onWebIDEUpdate: function(event, what, details) { > + if (what == "busy" || what == "unbusy") { > + this.updateCommands; You need () to call the function. ::: browser/devtools/webide/modules/runtime-list.js @@ +29,5 @@ > + > + if (this._sidebarsEnabled) { > + this._panelNodeEl = "button"; > + this.onWebIDEUpdate = this.onWebIDEUpdate.bind(this); > + this._UI.on("webide-update", this.onWebIDEUpdate); As with |ProjectList|, I think you need this in both modes. @@ +62,5 @@ > + }, > + > + onWebIDEUpdate: function(event, what, details) { > + if (what == "busy" || what == "unbusy") { > + this.updateCommands; You need () to call the function. ::: browser/devtools/webide/test/head.js @@ +189,5 @@ > targetBrowser.removeTab(aTab); > return deferred.promise; > } > > +function connectToLocalRuntime(aWindow, bWindow) { Nit: Names like |aWindow| are from an older style we don't use anymore where the "a" means "argument". It's preferred to use meaningful, more descriptive names instead of the "a" prefix. Having said that, I don't follow the purpose of this... In each test we are only even in sidebars off OR sidebars on. So, don't we always know the correct window in the test? ::: browser/devtools/webide/test/sidebars/browser.ini @@ +4,5 @@ > support-files = > ../head.js > + > +[browser_tabs.js] > +skip-if = os == 'win' || e10s # Bug 1149289 - Intermittent on Windows, Bug 1072167 - browser_tabs.js test fails under e10s Intermittent on Windows was fixed, please rebase and reset this to the same value as the other browser.ini file. ::: browser/devtools/webide/test/sidebars/browser_tabs.js @@ +22,5 @@ > + > + let tab = yield addTab(TEST_URI); > + > + let win = yield openWebIDE(); > + let winIframeProject = win.document.querySelector("#project-listing-panel-details").contentWindow; Maybe you should make these two available as helpers in head.js? Just a suggestion. @@ +58,5 @@ > + winIframeRuntime.document.querySelectorAll(".runtime-panel-item-other")[1].click(); > + return deferred.promise; > +} > + > +function selectTabProject(win, winIframeProject) { Be sure to update this test from the original copy to fix the Windows intermittent here as well. ::: browser/devtools/webide/test/sidebars/test_telemetry.html @@ +209,5 @@ > + > + let win; > + > + SimpleTest.registerCleanupFunction(() => { > + Task.spawn(function*() { I think we discussed moving this work back into the end of the test, since right now it happens async, and you saw it interfere with other tests, right?
Attachment #8589351 -
Flags: feedback?(jryans) → feedback+
|
|
Assignee | |
Comment 20•9 years ago
|
||
Almost there I think! A couple of notes on this version: 1. I've added helper functions to call the panels in the tests in head.js as per your suggestion - I also reused the getRuntimeFrame function from this in the connectToLocalRuntime call instead of passing a second window object - let me known if that's a better solution or if you prefer some other method. 2. Didn't need to move any changes from test_telemetry.html - it seems to work fine now. 3. My only issue now is a failure of window leaks in try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=e53243438ed7 Wasn't able to figure out what I had around that was causing that problem and was hoping you might have an idea?
Attachment #8589351 -
Attachment is obsolete: true
Attachment #8594267 -
Flags: feedback?(jryans)
Comment on attachment 8594267 [details] [diff] [review] Bug1135191.patch Review of attachment 8594267 [details] [diff] [review]: ----------------------------------------------------------------- Not sure about the test issues off hand, have you tried narrowing it down to a specific test? Let me know what you've tried so far, so I have a better idea of where to start. ::: browser/devtools/webide/modules/project-list.js @@ +51,5 @@ > + switch (what) { > + case "project-removed": > + case "runtime-apps-icons": > + case "runtime-targets": > + case "update-tabs": Where did "update-tabs" come from? This does not appear to be an event AppManager emits. @@ +63,5 @@ > + }, > + > + onWebIDEUpdate: function(event, what, details) { > + if (what == "busy" || what == "unbusy") { > + this.update(details); The main |update| function doesn't check the busy state. This suggests it does not belong here. ::: browser/devtools/webide/test/head.js @@ +190,5 @@ > targetBrowser.removeTab(aTab); > return deferred.promise; > } > > +function getRuntimeFrame(win) { Maybe getRuntimeDocument to be clear about what this returns? @@ +194,5 @@ > +function getRuntimeFrame(win) { > + return win.document.querySelector("#runtime-listing-panel-details").contentDocument; > +} > + > +function getProjectFrame(win) { ...Document here too, I think. ::: browser/devtools/webide/test/sidebars/browser_tabs.js @@ +22,5 @@ > + > + let tab = yield addTab(TEST_URI); > + > + let win = yield openWebIDE(); > + let winIframeProject = getProjectFrame(win); These two variables are documents, not windows, so adjust the naming. ::: browser/devtools/webide/test/sidebars/test_addons.html @@ +96,5 @@ > + > + ok(!Devices.helperAddonInstalled, "Helper not installed"); > + > + let win = yield openWebIDE(true); > + let winIframe = getRuntimeFrame(win); Not a window, rename. ::: browser/devtools/webide/test/sidebars/test_autoconnect_runtime.html @@ +27,5 @@ > + DebuggerServer.addBrowserActors(); > + } > + > + let win = yield openWebIDE(); > + let winIframe = getRuntimeFrame(win); Not a window, rename. ::: browser/devtools/webide/test/sidebars/test_autoselect_project.html @@ +27,5 @@ > + DebuggerServer.addBrowserActors(); > + } > + > + let win = yield openWebIDE(); > + let winIframeRuntime = getRuntimeFrame(win); Not windows, rename both. ::: browser/devtools/webide/test/sidebars/test_device_permissions.html @@ +29,5 @@ > + > + let win = yield openWebIDE(); > + > + let permIframe = win.document.querySelector("#deck-panel-permissionstable"); > + let winIframe = getRuntimeFrame(win); Not window, rename. ::: browser/devtools/webide/test/sidebars/test_device_preferences.html @@ +29,5 @@ > + > + let win = yield openWebIDE(); > + > + let prefIframe = win.document.querySelector("#deck-panel-devicepreferences"); > + let winIframe = getRuntimeFrame(win); Not window, rename. ::: browser/devtools/webide/test/sidebars/test_device_settings.html @@ +34,5 @@ > + > + let win = yield openWebIDE(); > + > + let settingIframe = win.document.querySelector("#deck-panel-devicesettings"); > + let winIframe = getRuntimeFrame(win); Not window, rename. ::: browser/devtools/webide/test/sidebars/test_duplicate_import.html @@ +19,5 @@ > > Task.spawn(function*() { > Services.prefs.setBoolPref("devtools.webide.sidebars", true); > let win = yield openWebIDE(); > + let winIframe = getProjectFrame(win); Not window, rename. ::: browser/devtools/webide/test/sidebars/test_fullscreenToolbox.html @@ +30,5 @@ > + Task.spawn(function* () { > + Cu.import("resource://gre/modules/devtools/dbg-server.jsm"); > + Services.prefs.setBoolPref("devtools.webide.sidebars", true); > + let win = yield openWebIDE(); > + let winIframeProject = getProjectFrame(win); Not windows, rename both. ::: browser/devtools/webide/test/sidebars/test_import.html @@ +19,5 @@ > > Task.spawn(function*() { > Services.prefs.setBoolPref("devtools.webide.sidebars", true); > let win = yield openWebIDE(); > + let winIframe = getProjectFrame(win); Not window, rename. ::: browser/devtools/webide/test/sidebars/test_runtime.html @@ +48,5 @@ > DebuggerServer.addBrowserActors(); > } > > win = yield openWebIDE(); > + let winIframeRuntime = getRuntimeFrame(win); Not windows, rename both. ::: browser/devtools/webide/test/sidebars/test_telemetry.html @@ +234,5 @@ > + // Cycle once, so we can test for multiple opens > + yield cycleWebIDE(); > + > + win = yield openWebIDE(); > + let winIframe = getRuntimeFrame(win); Not window, rename.
Attachment #8594267 -
Flags: feedback?(jryans) → feedback+
|
|
Assignee | |
Comment 22•9 years ago
|
||
Renamed and fixed up based on your last comments.
As for the failing test it is specifically for browser_tabs.js. I did some changes to test on try and also tried the following to see what would happen:
./mach mochitest-devtools browser/devtools/webide --run-until-failure --repeat 15
Not sure why but it appears that running a browser_tabs.js test within the sidebars directory AND in the main test directory seem to interfere with each other. The run-until-failure showed some issues where docRuntime.querySelectorAll(".runtime-panel-item-other")[1].click() in connectToLocal would not exist and I was able to fix that with forcing a preference setting in the main test: Services.prefs.setBoolPref("devtools.webide.sidebars", false)
Second observation was that I got the "1 tab available - Got 2, expected 1" failure on both browser_tabs.js files and that was fixed by putting in a yield nextTick() after the yield removeTab() call.
Anyway, not sure if either are the right solutions but they seem to resolve the run-until-failure problems.
As for the original try failure I linked to in the previous patch, it is unfortunately still around :\
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c423945f597e&filter-searchStr=OS%20X%2010.6%20debug%20Mochitest%20M%20Mochitest%20DevTools%20Browser%20Chrome%20dt4
If we click on the orange failure, we see a few leaks:
TEST-UNEXPECTED-FAIL | browser/devtools/webide/test/sidebars/browser_tabs.js | leaked 2 window(s) until shutdown [url = chrome://webide/content/runtime-listing.xhtml]
TEST-UNEXPECTED-FAIL | browser/devtools/webide/test/sidebars/browser_tabs.js | leaked 2 window(s) until shutdown [url = chrome://webide/content/project-listing.xhtml]
TEST-UNEXPECTED-FAIL | browser/devtools/webide/test/sidebars/browser_tabs.js | leaked 1 window(s) until shutdown [url = about:blank]
TEST-UNEXPECTED-FAIL | browser/devtools/webide/test/sidebars/browser_tabs.js | leaked 1 window(s) until shutdown [url = chrome://webide/content/webide.xul]
but only for OSX 10.6 dt4. Any ideas what could be happening?
Attachment #8594267 -
Attachment is obsolete: true
Attachment #8596281 -
Flags: feedback?(jryans)
Comment on attachment 8596281 [details] [diff] [review] Bug1135191.patch Review of attachment 8596281 [details] [diff] [review]: ----------------------------------------------------------------- Okay, I think the code looks good! One nit below, plus the testing failure aren't quite right, so we need to fix those first. I've already got one fix that I'll attach, and then I'll keep investigating the rest. ::: browser/devtools/webide/test/sidebars/test_addons.html @@ +96,5 @@ > + > + ok(!Devices.helperAddonInstalled, "Helper not installed"); > + > + let win = yield openWebIDE(true); > + let winDoc = getRuntimeDocument(win); What is a |winDoc|? Use |docRuntime|.
Attachment #8596281 -
Flags: feedback?(jryans) → review+
With this extra patch, browser_tabs will clean up properly, and we don't need to clear the pref in the non-sidebar version, or wait for nextTick. It turns out that we were registering clean up functions incorrectly for browser tests (chrome tests, the majority, were fine). Feel free to roll this up into your patch or land separately at the same time as yours, whichever you like. Here's a try run to see if this improves the other problems on 10.6: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4c59cc45052
|
|
Assignee | |
Comment 25•9 years ago
|
||
Merging the resolved browser_tabs.js cleanup patch with original patch
Attachment #8596281 -
Attachment is obsolete: true
Attachment #8596793 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 26•9 years ago
|
||
This patch fixes the window leak error (plus some other minor updates) - let me know if anything should be changed on it - if all is good, I'll merge this one with the former patch. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=deabd1bfab91
Attachment #8598039 -
Flags: review?(jryans)
Comment on attachment 8598039 [details] [diff] [review] fix-browser-tab-window-leak.patch Review of attachment 8598039 [details] [diff] [review]: ----------------------------------------------------------------- This all looks good to me.
Attachment #8598039 -
Flags: review?(jryans) → review+
|
|
Assignee | |
Comment 28•9 years ago
|
||
Attachment #8596838 -
Attachment is obsolete: true
Attachment #8598039 -
Attachment is obsolete: true
Attachment #8598253 -
Flags: review+
|
|
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
|
||
Comment 30•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dd7a16593cf0
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
|
||
Updated•6 years ago
|
Product: Firefox → DevTools
|
|
||
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•