Closed
Bug 1146542
Opened 9 years ago
Closed 9 years ago
Firefox OS tabs not found after connection
Categories
(DevTools Graveyard :: WebIDE, defect)
DevTools Graveyard
WebIDE
Tracking
(firefox39 fixed)
RESOLVED
FIXED
Firefox 39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: jryans, Assigned: jryans)
References
Details
(Keywords: regression)
Attachments
(2 files, 3 obsolete files)
20.75 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
26.50 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
If I open a browser tab in Firefox OS after connecting it WebIDE, it does not appear in the project list until disconnecting and reconnecting. My suspicion is this was broken by bug 1126432.
Assignee | ||
Updated•9 years ago
|
Blocks: 1126432
Keywords: regression
Assignee | ||
Comment 1•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdf7f2c93d2c
Assignee | ||
Comment 2•9 years ago
|
||
This generally cleans up and documents the app manager events. A few of them are renamed to fit the common naming scheme.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Attachment #8582757 -
Flags: review?(poirot.alex)
Attachment #8582757 -
Flags: feedback?(jfong)
Assignee | ||
Comment 3•9 years ago
|
||
This restore updates to the runtime tab list after bug 1126432. For the moment, we have to re-list the tabs every time the project list is shown, because we don't have any events that track tab navigations for the entire set of tabs.
Attachment #8582758 -
Flags: review?(poirot.alex)
Attachment #8582758 -
Flags: feedback?(jfong)
Comment 4•9 years ago
|
||
Comment on attachment 8582758 [details] [diff] [review] Restore tab list changes for non-sidebar case Review of attachment 8582758 [details] [diff] [review]: ----------------------------------------------------------------- Read all my comments for both patches before replying/coding ;) I think there is something great to do with project-list, that would help us being more project agnostic! Also the name may be misleading. This isn't really related to projects which are local. It is more runtime-targets or something alike. ::: browser/devtools/webide/modules/app-manager.js @@ +110,5 @@ > * project-is-not-running: > * The selected project is not running on the connected runtime. > + * project-list: > + * The list of available projects has changed, or any of the user-visible > + * details (like names) for the non-selected runtimes has changed. `project-list` may dispatch also an argument with the project type being changed, if there is just one kind modified. So that we could only update one type. I imagine that's fast enough to update everything, everytime, on our laptops, but I could also imagine it becoming flickery on slow hardware with a big app/tab list. ::: browser/devtools/webide/modules/tab-store.js @@ +81,5 @@ > }, > > _onTabListChanged: function() { > + this.listTabs().then(() => this.emit("tab-list")) > + .catch(console.error); Unfortunately, console API is dumb, you need: console.error.bind(console)
Attachment #8582758 -
Flags: review?(poirot.alex) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8582757 [details] [diff] [review] Clean up and describe app-manager events Review of attachment 8582757 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/webide/modules/app-manager.js @@ +102,5 @@ > + * when such information is available. > + * project: > + * The selected project has changed. > + * project-is-running: > + * The selected project is running on the connected runtime. is -> starts ? It's not clear if we mean that it is a running state change with such sentence ? @@ +115,5 @@ > + * runtime: > + * The selected runtime has changed. > + * runtime-actors: > + * The list of actors available from the runtime is now available, so we > + * can test for features like preferences and settings. I don't know if that's crystal clear. May be we should mention this is global actors (to distinguish from tab actors for the app/project) @@ +116,5 @@ > + * The selected runtime has changed. > + * runtime-actors: > + * The list of actors available from the runtime is now available, so we > + * can test for features like preferences and settings. > + * runtime-apps-found: small-nit: While you are at renaiming, doesn't "found" sounds weird to you? Actually, while reviewing the second patch it looks like this event should be project-list with "apps" as argument?
Attachment #8582757 -
Flags: review?(poirot.alex) → review+
Updated•9 years ago
|
Attachment #8582757 -
Flags: feedback?(jfong) → feedback+
Updated•9 years ago
|
Attachment #8582758 -
Flags: feedback?(jfong) → feedback+
Assignee | ||
Comment 6•9 years ago
|
||
Current patches leak on debug builds... Will post a fix soon where closing a tab instead of opening a new one and closing both resolved it. :/ Oh well!
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #5) > Comment on attachment 8582757 [details] [diff] [review] > Clean up and describe app-manager events > > Review of attachment 8582757 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/webide/modules/app-manager.js > @@ +102,5 @@ > > + * when such information is available. > > + * project: > > + * The selected project has changed. > > + * project-is-running: > > + * The selected project is running on the connected runtime. > > is -> starts ? > It's not clear if we mean that it is a running state change with such > sentence ? Makes sense, I've renamed the events started / stopped and updated the description. > @@ +115,5 @@ > > + * runtime: > > + * The selected runtime has changed. > > + * runtime-actors: > > + * The list of actors available from the runtime is now available, so we > > + * can test for features like preferences and settings. > > I don't know if that's crystal clear. May be we should mention this is > global actors (to distinguish from tab actors for the app/project) Makes sense, I named it "runtime-global-actors" and clarified the description. > @@ +116,5 @@ > > + * The selected runtime has changed. > > + * runtime-actors: > > + * The list of actors available from the runtime is now available, so we > > + * can test for features like preferences and settings. > > + * runtime-apps-found: > > small-nit: While you are at renaiming, doesn't "found" sounds weird to you? > > Actually, while reviewing the second patch it looks like this event should > be project-list with "apps" as argument? Yes, seems reasonable to change to "project-list". I'll switch to that in the second patch.
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #4) > Comment on attachment 8582758 [details] [diff] [review] > Restore tab list changes for non-sidebar case > > Review of attachment 8582758 [details] [diff] [review]: > ----------------------------------------------------------------- > > Read all my comments for both patches before replying/coding ;) > > I think there is something great to do with project-list, > that would help us being more project agnostic! > Also the name may be misleading. This isn't really related to projects which > are local. > It is more runtime-targets or something alike. Yeah, I see what you mean. "runtime-targets" sounds good, so I've changed to that. > ::: browser/devtools/webide/modules/app-manager.js > @@ +110,5 @@ > > * project-is-not-running: > > * The selected project is not running on the connected runtime. > > + * project-list: > > + * The list of available projects has changed, or any of the user-visible > > + * details (like names) for the non-selected runtimes has changed. > > `project-list` may dispatch also an argument with the project type being > changed, if there is just one kind modified. So that we could only update > one type. > I imagine that's fast enough to update everything, everytime, on our > laptops, but I could also imagine it becoming flickery on slow hardware with > a big app/tab list. Makes sense, I've added type data to focus the update. > ::: browser/devtools/webide/modules/tab-store.js > @@ +81,5 @@ > > }, > > > > _onTabListChanged: function() { > > + this.listTabs().then(() => this.emit("tab-list")) > > + .catch(console.error); > > Unfortunately, console API is dumb, you need: console.error.bind(console) Right, thanks!
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8582757 -
Attachment is obsolete: true
Attachment #8584873 -
Flags: review+
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8582758 -
Attachment is obsolete: true
Attachment #8584874 -
Flags: review+
Assignee | ||
Comment 11•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05088d22bfd2
Assignee | ||
Comment 12•9 years ago
|
||
Fixed a few more test failures. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c106e68b185
Attachment #8584874 -
Attachment is obsolete: true
Attachment #8584920 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
remote: https://hg.mozilla.org/integration/fx-team/rev/0cabefa3e330 remote: https://hg.mozilla.org/integration/fx-team/rev/5dfc78816549
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0cabefa3e330 https://hg.mozilla.org/mozilla-central/rev/5dfc78816549
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
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
•