Closed Bug 1146542 Opened 6 years ago Closed 5 years ago

Firefox OS tabs not found after connection

Categories

(DevTools Graveyard :: WebIDE, defect)

defect
Not set
normal

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)

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.
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)
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 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 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+
Attachment #8582757 - Flags: feedback?(jfong) → feedback+
Attachment #8582758 - Flags: feedback?(jfong) → feedback+
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!
(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.
(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!
https://hg.mozilla.org/mozilla-central/rev/0cabefa3e330
https://hg.mozilla.org/mozilla-central/rev/5dfc78816549
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Depends on: 1149289
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.