1477596 - Show runtime information

Status: RESOLVED FIXED

(DevTools :: about:debugging, enhancement)

Description

[Daisuke Akatsuka (:daisuke)]

This is the splitted bug of bug 1471795.
In this, implement to show runtime information of selected runtime.

Comment 1

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477596 - Part 1: Implement basis of DebugTargetsPanel.

Review commit: https://reviewboard.mozilla.org/r/258682/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258682/

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477596 - Part 2: Add updateRuntimeInfo action and reducer.

Review commit: https://reviewboard.mozilla.org/r/258684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258684/

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477596 - Part 3: Show runtime info.

Review commit: https://reviewboard.mozilla.org/r/258688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258688/

Comment 4

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994081 [details]
Bug 1477596 - Part 2: Add updateRuntimeInfo action and reducer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258684/diff/1-2/

Comment 5

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994082 [details]
Bug 1477596 - Part 3: Show runtime info.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258688/diff/1-2/

Comment 6

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994080 [details]
Bug 1477596 - Part 1: Implement basis of DebugTargetsPanel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258682/diff/1-2/

Comment 7

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994081 [details]
Bug 1477596 - Part 2: Add updateRuntimeInfo action and reducer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258684/diff/2-3/

Comment 8

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994082 [details]
Bug 1477596 - Part 3: Show runtime info.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258688/diff/2-3/

Comment 9

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994080 [details]
Bug 1477596 - Part 1: Implement basis of DebugTargetsPanel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258682/diff/2-3/

Comment 10

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994081 [details]
Bug 1477596 - Part 2: Add updateRuntimeInfo action and reducer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258684/diff/3-4/

Comment 11

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994082 [details]
Bug 1477596 - Part 3: Show runtime info.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258688/diff/3-4/

Comment 12

[Daisuke Akatsuka (:daisuke)]

I'm sorry. Please let me update these patches since I found extra class and a place where it's better to use <section> instead of <div>. I'll update soon.

Comment 13

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994080 [details]
Bug 1477596 - Part 1: Implement basis of DebugTargetsPanel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258682/diff/3-4/

Comment 14

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994081 [details]
Bug 1477596 - Part 2: Add updateRuntimeInfo action and reducer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258684/diff/4-5/

Comment 15

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994082 [details]
Bug 1477596 - Part 3: Show runtime info.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258688/diff/4-5/

Comment 16

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994080 [details]
Bug 1477596 - Part 1: Implement basis of DebugTargetsPanel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258682/diff/4-5/

Comment 17

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994081 [details]
Bug 1477596 - Part 2: Add updateRuntimeInfo action and reducer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258684/diff/5-6/

Comment 18

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8994082 [details]
Bug 1477596 - Part 3: Show runtime info.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258688/diff/5-6/

Comment 19

[Julian Descottes [:jdescottes]]

Attachment: Bug 1477596 - Alternate proposal for architecting our state, actions and reducers

Review commit: https://reviewboard.mozilla.org/r/259906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259906/

Comment 20

[Julian Descottes [:jdescottes]]

[Note: there is a lot of things I don't really like about my patch, but I figured I'd throw it here asap so that we can start discussing and taking decisions]

As discussed with both of you I'd like us to pause for a moment and discuss architecture here. I uploaded another version of Daisuke's patch that try to rely more heavily on actions and reducers. So maybe we can compare the two and use this as a basis for our discussion.

Before diving into code reviews, it feels like we need to agree on a few things here:
- what goes into our state?
- which actions do we need
- component hierarchy

1/ What goes into our state

In think we should have the concept of list of runtimes. We only have the local firefox runtime for now so we could also completely skip this. 

The other element we need is the selected runtime. That's where I am not sure how to organize things. The could simply have an id/pointer/whatever that allows us to find the selected runtime in the list of runtimes. But we could also have an object with more information. I think that what motivates this is that once you select a runtime, you need to retrieve more information. That's the approach I took here in order to exercise doing an async action (to get the runtime info in my patch). However I'd be fine with just storing an id/index allowing us to find the runtime in the list of runtimes.

For now I don't think we need anything else in the state?

2/ Actions we need

In theory we don't need any here, since nothing is dynamic! Again I'm using the patch to exercise a bit. So looking at what's coming, the list of runtimes will be updated and the selected runtime will also be able to be updated. So one action for each would make sense. Again I'd be fine with landing something with 0 actions here since we are not using any.

3/ Component hierarchy

I admit I start being a bit confused between, the runtimes, the runtime info etc... 

Right now we have:

App
  RuntimesPane
    RuntimeItem
  DebugTargetsPane
    RuntimeInfo

What about

App
  RuntimesList
    RuntimeItem
  RuntimeDetails
    RuntimeInfo
    (then below we will have RuntimeTargets I guess etc...)

Or anything else, but the DebugTargetsPane in the middle is tripping me up!

Comment 21

[Belén [:ladybenko]]

Attachment: Bug 1477596 - alternate proposal for architecture in handling Runtimes (DO NOT MERGE)

Review commit: https://reviewboard.mozilla.org/r/259970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259970/

Comment 22

[Belén [:ladybenko]]

Hi, I also added an alternative approach (it's easier to discuss around code!). It's similar to Julian's, the main differences are:

- No use of `Runtime` / `ThisFirefox` objects
- More explicit about how to handle async actions (dispatching subactions). Note that the failure state has not been covered (yet)
- Use of an index to pass to the sidebar to select a runtime (although I don't have a hard preference about this one, as long as we are not repeating info already available somewhere else in the state).

PS: Some stuff is a rough around the edges, sorry about that, but it's Friday and it's a bit late already :)

Comment 23

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477596: Show runtime information.

Review commit: https://reviewboard.mozilla.org/r/260900/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260900/

Comment 24

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1477596 - Part 2: Show runtime information for 'this firefox'.

Review commit: https://reviewboard.mozilla.org/r/260902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260902/

Comment 25

[Julian Descottes [:jdescottes]]

Clearing review flags for Belén, let's keep the reviews to 1 reviewer at the moment.

Comment 26

[Julian Descottes [:jdescottes]]

Comment on attachment 8996910 [details]
Bug 1477596: Show runtime information.

https://reviewboard.mozilla.org/r/260900/#review267958

::: devtools/client/aboutdebugging-new/src/components/App.css:5
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
>  
>  .app {

We found it helpful in some devtools panels to add ascii art representations of the layout.

Examples:

https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/devtools/client/application/src/components/App.css#38-49

https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/devtools/client/application/src/components/Worker.css#5-16

https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/devtools/client/themes/webconsole.css#495-520

Comment 27

[Julian Descottes [:jdescottes]]

Comment on attachment 8996911 [details]
Bug 1477596 - Part 2: Show runtime information for 'this firefox'.

https://reviewboard.mozilla.org/r/260902/#review267956

I think we should fold both commits together, otherwise it's a bit strange to see an <article> being added in changeset1 without any title or content.

::: devtools/client/aboutdebugging-new/src/components/RuntimePage.js:16
(Diff revision 1)
> +
> +const Services = require("Services");
> +
>  class RuntimePage extends PureComponent {
>    render() {
> -    return dom.article();
> +    return dom.article(

I am unsure about using <article> here, while the <h> is in another component, and we don't have any content other than the title for now. We can revisit later.

Comment 28

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8996910 [details]
Bug 1477596: Show runtime information.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260900/diff/1-2/

Comment 29

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8996910 [details]
Bug 1477596: Show runtime information.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260900/diff/2-3/

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6592552f9471
Show runtime information. r=jdescottes

Comment 31

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6592552f9471