Firefox OS tabs not found after connection

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: jryans, Assigned: jryans)

Tracking

({regression})

unspecified
Firefox 39
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

4 years ago
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

4 years ago
Blocks: 1126432
Keywords: regression
Assignee

Comment 2

4 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

4 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 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+
Assignee

Comment 6

4 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

4 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

4 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

4 years ago
Attachment #8582757 - Attachment is obsolete: true
Attachment #8584873 - Flags: review+
Assignee

Comment 10

4 years ago
Attachment #8582758 - Attachment is obsolete: true
Attachment #8584874 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0cabefa3e330
https://hg.mozilla.org/mozilla-central/rev/5dfc78816549
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Depends on: 1149289
Assignee

Updated

4 years ago
Depends on: 1149820

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.