Closed Bug 1471795 Opened 6 years ago Closed 5 years ago

Implement "This Firefox" page in new design

Categories

(DevTools :: about:debugging, enhancement)

Product:
DevTools
Component:
about:debugging
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(6 files, 15 obsolete files)

Bug 1471795 - Part 1: Implement basis of 'This Firefox' page.
59 bytes, text/x-review-board-request
ladybenko
: review+
jdescottes
: review+
Details
Bug 1471795 - Part 2: Introduce Runtime class.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1471795 - Part 3: Implement runtimes pane.
59 bytes, text/x-review-board-request
ladybenko
: review+
jdescottes
: review+
Details
Bug 1471795 - Part 4: Implement an action of when runtime was selected.
59 bytes, text/x-review-board-request
ladybenko
: review+
jdescottes
: review+
Details
prototype1.png
564.34 KB, image/png
Details
Use links instead of buttons for temporary addons?
45.17 KB, image/png
Details 
Implement "This Firefox" page along to the following design.
https://mozilla.invisionapp.com/share/F9IVW9WPG4W#/screens/297684870
Currently, although I have tried the following, I would like to get feedback.
(I have not implement actions and styles yet.)

* Added new preference `devtools.aboutdebugging.ng-enabled` for new about:debugging.
* Added new directory devtools/client/aboutdebugging-ng/ for new about:debugging.
* Made basic components

The structure of components is following:
* App
    * RuntimeList: left pane
        * RuntimeItem
          is extended by ThisFirefoxItem
    * DebugPanel: right pane
        * AppInfo: Show app information
        * DebugTargetList
            * DebugTargetItem
                is extended by
                * ExtensionItem
                * TabItem
                * WorkerItem

The runtime classes are under aboutdebugging-ng/runtimes.
* Runtime
* ThisFirefox
Flags: needinfo?(jdescottes)
Also, is it better we make this bug to be meta and separate the bugs respectively?
e.g. 
* Implement basis of new about:debugging.
  Add preference, directory, index.html, aboutdebugging.css, aboutdebugging.js and so on.
* Implement RuntimeList
...
Thanks for the patch Daisuke, overall I think this looks good as a first step. Please find some early feedback below:

- I like having a dedicated Runtime class and extending it for each runtime type, sounds like a good idea here.

- We usually use "new" rather than "ng".

- We should not turn the pref on in Nightly until we have feature parity. Let's do that in another bug

- The overall folder structure makes sense, especially having a dedicated components/ folder. 
  However we had a meeting to discuss guidelines for React panels and the outcome was 
    https://docs.google.com/document/d/11CRu0F6GvviATF1FK5IwrJH0KtepLtJdNNFWPhYg7lo/edit# 
  It doesn't 100% apply to about:debugging since about:debugging is not a panel. 
  But it would be nice to still have the src/ and test/ subfolders. 
  For your current patch we would keep index.html, aboutdebugging.js and aboutdebugging.css under the top folder.
  The rest would move under src. 

- We also want to move reducers and actions under src/reducers/ and src/actions/

- Let's think quickly about bug structure. What about:
  - [meta] about:debugging ng:
    - about:debugging this firefox page <-- your bug
    - about:debugging USB runtime device page
    - about:debugging WiFi runtime device page
    - about:debugging Remote Location runtime device page
    - about:debugging list runtimes in sidebar
    - [meta] about:debugging connect page
      - about:debugging connect page USB section
      - about:debugging connect page WiFi section
      - about:debugging connect page RemoteLocation section
  Where you thinking about splitting your current bug further? 
  For instance, on bug to add worker targets, one bug to add tab targets etc... 
  That might make sense, since each of them will probably involve a sizeable chunk of code. 
  Or maybe we should approach this the other way around? 
  First add connections to all types of runtimes, and then add debugging targets?

Note that I think we can split much more than that (maybe we will have dedicated bugs to handle proper connection issues etc...) but for me it's difficult to think about it without working on it.
Flags: needinfo?(jdescottes)
Belén: keeping in mind this patch is just a work in progress, can you also have a look?
Flags: needinfo?(balbeza)
(In reply to Julian Descottes (PTO - back July 16th) [:jdescottes][:julian] from comment #4)
> Thanks for the patch Daisuke, overall I think this looks good as a first
> step. Please find some early feedback below:

Thank you very much, Julian!

> - I like having a dedicated Runtime class and extending it for each runtime
> type, sounds like a good idea here.

Okay, will proceed with this thinking.

> - We usually use "new" rather than "ng".

Okay.

> - We should not turn the pref on in Nightly until we have feature parity.
> Let's do that in another bug

Ah, sorry, that's my mistake.

> - The overall folder structure makes sense, especially having a dedicated
> components/ folder. 
>   However we had a meeting to discuss guidelines for React panels and the
> outcome was 
>    
> https://docs.google.com/document/d/
> 11CRu0F6GvviATF1FK5IwrJH0KtepLtJdNNFWPhYg7lo/edit# 
>   It doesn't 100% apply to about:debugging since about:debugging is not a
> panel. 
>   But it would be nice to still have the src/ and test/ subfolders. 
>   For your current patch we would keep index.html, aboutdebugging.js and
> aboutdebugging.css under the top folder.
>   The rest would move under src. 

Oh, the document is very helpful for me.
I'd proceed along to the document as much as possible.

> - We also want to move reducers and actions under src/reducers/ and
> src/actions/
> 
> - Let's think quickly about bug structure. What about:
>   - [meta] about:debugging ng:
>     - about:debugging this firefox page <-- your bug
>     - about:debugging USB runtime device page
>     - about:debugging WiFi runtime device page
>     - about:debugging Remote Location runtime device page
>     - about:debugging list runtimes in sidebar
>     - [meta] about:debugging connect page
>       - about:debugging connect page USB section
>       - about:debugging connect page WiFi section
>       - about:debugging connect page RemoteLocation section
>   Where you thinking about splitting your current bug further? 
>   For instance, on bug to add worker targets, one bug to add tab targets
> etc... 
>   That might make sense, since each of them will probably involve a sizeable
> chunk of code. 

Yeah, I had thought splitting this bug.

> Note that I think we can split much more than that (maybe we will have
> dedicated bugs to handle proper connection issues etc...) but for me it's
> difficult to think about it without working on it.

Please let me try to make all patches on this bug at first. 
Then, if we think we need to split, I'll do that.
Comment on attachment 8989929 [details]
Bug 1471795: wip.

https://reviewboard.mozilla.org/r/254940/#review263068

::: devtools/client/aboutdebugging-ng/aboutdebugging.css:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

In the Application panel, we switched to a BEM-like style to organise the CSS, maybe you find it helpful here too.

I'm writing some guidelines to share with the rest of the team. It's still a work in progress, but the BEM-ish style (and why) is already covered. You can take a look here https://gist.github.com/belen-albeza/86a8df1ae2a6f7e21a65e17e0131dc3a

::: devtools/client/aboutdebugging-ng/aboutdebugging.css:37
(Diff revision 1)
> +}
> +
> +.appinfo {
> +  align-items: center;
> +  display: flex;
> +  padding-top: 20px;

For `padding` and `margin`, the [current devtools guidelines](https://docs.firefox-dev.tools/contributing/css.html) advise to avoid setting specifically `-top`, `-bottom`, `-left` and `-right` and switch to `-block-start`, `block-bottom`, `block-inline-start` and `block-inline-end`.

::: devtools/client/aboutdebugging-ng/components/DebugTargetList.js:27
(Diff revision 1)
> +
> +    return dom.div(
> +      {
> +        className: "debug-target-list",
> +      },
> +      dom.div({}, title),

Maybe a heading tag would be more suitable for a title?

::: devtools/client/aboutdebugging-ng/store.js:17
(Diff revision 1)
> +  let shouldLog = false;
> +  let history;
> +
> +  // If testing, store the action history in an array
> +  // we'll later attach to the store
> +  if (flags.testing) {

I'm unsure about this. It feels odd to have testing code in the source itself. Isn't there a way to do this from the test files themselves?
Comment on attachment 8989929 [details]
Bug 1471795: wip.

https://reviewboard.mozilla.org/r/254940/#review263068

> In the Application panel, we switched to a BEM-like style to organise the CSS, maybe you find it helpful here too.
> 
> I'm writing some guidelines to share with the rest of the team. It's still a work in progress, but the BEM-ish style (and why) is already covered. You can take a look here https://gist.github.com/belen-albeza/86a8df1ae2a6f7e21a65e17e0131dc3a

Hi Belén! Thank you for the feedback!
Yep, I have been rewriting CSS while referring application panel after I got a feedback from Julian.

> For `padding` and `margin`, the [current devtools guidelines](https://docs.firefox-dev.tools/contributing/css.html) advise to avoid setting specifically `-top`, `-bottom`, `-left` and `-right` and switch to `-block-start`, `block-bottom`, `block-inline-start` and `block-inline-end`.

Indeed, it's better to use logical properties, thanks!

> Maybe a heading tag would be more suitable for a title?

Ah, indeed. I'll use h2 or h3 instead.

> I'm unsure about this. It feels odd to have testing code in the source itself. Isn't there a way to do this from the test files themselves?

Yeah, I have been rewriting this as well while reffering this store[1].

[1] https://searchfox.org/mozilla-central/source/devtools/client/application/src/create-store.js

Hopefully, I'd like to request the review again in this week!
Flags: needinfo?(balbeza)
(In reply to Belén [:ladybenko] from comment #7)
> ::: devtools/client/aboutdebugging-ng/aboutdebugging.css:37
> (Diff revision 1)
> > +}
> > +
> > +.appinfo {
> > +  align-items: center;
> > +  display: flex;
> > +  padding-top: 20px;
> 
> For `padding` and `margin`, the [current devtools
> guidelines](https://docs.firefox-dev.tools/contributing/css.html) advise to
> avoid setting specifically `-top`, `-bottom`, `-left` and `-right` and
> switch to `-block-start`, `block-bottom`, `block-inline-start` and
> `block-inline-end`.

I think the guidelines there are specifically about RTL so we only need to worry about *-left / *-right. Unless we're planning on doing a vertical text version of DevTools I believe *-top and *-bottom should be fine, and more readable too.
Attachment #8989929 - Attachment is obsolete: true
Attached image prototype1.png — 
I have implemented following things:

* Added new preference `devtools.aboutdebugging.new-enabled`
* Added new directory devtools/client/aboutdebugging-new/
* Runtime list (ThisFirefox only)
* Debug targets list
** show/inspect/update of tab list
** show/inspect/update of installed extension list
** show/inspect/update/install/remove/reload of temporary extension list
** show/inspect/update/ of worker list

I will add a patch for expand/collapse the tabs/extensions/workers on next week.
Also, we need to add tests.
Please let me do in another bug since it looks taking a time.
Assignee: nobody → dakatsuka
Thanks for the patches Daisuke, I'll try to go through them as quickly as possible. Since this is preffed off and not visible for now, we should handle most fixes in other bugs.
First some overall UI Feedback, without looking at the code. All of those can (and probably should) be handled as follow ups.

- Font size for debugging target is probably a bit big. Same feedback for icons.

- Inspect buttons that open a popup rather than a tab should have a different style? (or we should make the experience consistent)

- Not convinced about using the Inspect word everywhere. For (some) extensions and workers we don't even get an Inspector in the toolbox.

- Would be nice to have the number of items next to the category (eg "Temporary extensions (0)").

- Should we allow a category to be expanded if it has no items?

- Not sure what we should do about the "controls" for addons:
  * load temporary extension
  * enable addon debugging (was it removed from the new UI?)
Right now "load temporary extension" is inside the "Temporary extensions" container and is 
hidden when the container is collapsed. Maybe it should be positioned differently and always visible.
(To be checked with :victoria). In the mockups, they are only displayed for "This Firefox" and the "Extensions" category is not collapsible there.

- Related to that, the mockups had a single "Extensions" category, with a line to separate temporary extensions from other extensions. Here we have "Temporary extensions" and "Extensions".

- Same for Workers, the mockups only had one category for them.

- The mockups for "This Firefox" show only "Extensions". I can't remember why exactly. "Tabs" is really not helpful for "This Firefox", but we might still need workers (and process)

- When a temporary addon is added, we have 3 buttons "Inspect" "Reload" "Remove". Inspect should be the last one to be consistent with regular targets. On a sidenote it might be better to only keep "Inspect" as the main call to action/main button and have Reload and Remove as links? Will share a mockup and check with :victoria. 

- "This Firefox" feels too close to the left edge of the browser. Maybe too much margin on the top, not enough on the left? 

- Tab title just says "about:debugging" (compared to "Debugging with Firefox Developer Tools" for the current about:debugging)
related to my previous comment
Comment on attachment 8991848 [details]
Bug 1471795 - Part 1: Implement basis of 'This Firefox' page.

https://reviewboard.mozilla.org/r/256750/#review264582

::: devtools/client/aboutdebugging-new/aboutdebugging.js:13
(Diff revision 2)
> +  ChromeUtils.import("resource://devtools/client/shared/browser-loader.js", {});
> +const { require } = BrowserLoader({
> +  baseURI: "resource://devtools/client/aboutdebugging-new/",
> +  window,
> +});
> +const { Services } = require("resource://gre/modules/Services.jsm");

require("Services") should work here.

::: devtools/client/aboutdebugging-new/src/components/moz.build:10
(Diff revision 2)
> +with Files('**'):
> +    BUG_COMPONENT = ('DevTools', 'about:debugging')

I don't think we have to define this for subfolders since we already have it in devtools/client/aboutdebugging-new/moz.build

::: devtools/client/aboutdebugging-new/src/moz.build:9
(Diff revision 2)
> +with Files('**'):
> +    BUG_COMPONENT = ('DevTools', 'about:debugging')

Same comment
Attachment #8991848 - Flags: review?(jdescottes) → review+
Comment on attachment 8991849 [details]
Bug 1471795 - Part 2: Introduce Runtime class.

https://reviewboard.mozilla.org/r/256752/#review264588

::: devtools/client/aboutdebugging-new/src/components/App.js:11
(Diff revision 2)
>  
>  const { PureComponent } = require("devtools/client/shared/vendor/react");
>  const dom = require("devtools/client/shared/vendor/react-dom-factories");
> +const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> +
> +const ThisFirefox = require("../runtimes/this-firefox");

Not sure it's worth requiring this just for proptype checking. We rarely run react in development mode, so I would be fine with just having PropTypes.object

::: devtools/client/aboutdebugging-new/src/runtimes/moz.build:10
(Diff revision 2)
> +with Files('**'):
> +    BUG_COMPONENT = ('DevTools', 'about:debugging')

Same comment as for previous patch

::: devtools/client/aboutdebugging-new/src/runtimes/runtime.js:8
(Diff revision 2)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +/**
> + * This class represents the runtime such the remote Firefox.

nit: This class represents "a" runtime, such as "a" remote Firefox. (?)

::: devtools/client/aboutdebugging-new/src/runtimes/this-firefox.js:10
(Diff revision 2)
> +"use strict";
> +
> +const Runtime = require("./runtime");
> +
> +/**
> + * This class represents the Firefox which runs on same environment that opened

nits: 
the Firefox which runs -> the Firefox instance which runs

on same environment -> in the same environment

::: devtools/client/aboutdebugging-new/src/runtimes/this-firefox.js:14
(Diff revision 2)
> +  constructor() {
> +    super();
> +  }

nit: I think this is the same as not defining a constructor, but I guess you are complexifying this constructor in subsequent patches.
Attachment #8991849 - Flags: review?(jdescottes) → review+
Comment on attachment 8991850 [details]
Bug 1471795 - Part 3: Implement runtimes pane.

https://reviewboard.mozilla.org/r/256754/#review264594

Maybe we could have a runtime/ (and later debugtarget/ or target/) subfolder to keep the components/ folder easier to navigate?

::: devtools/client/aboutdebugging-new/src/components/RuntimeItem.css:5
(Diff revision 2)
> +.runtime-item {
> +}

I see this is still empty at the end of the patch series. We should drop this file for now.

::: devtools/client/aboutdebugging-new/src/components/RuntimeItem.js:15
(Diff revision 2)
> +
> +const Runtime = require("../runtimes/runtime");
> +
> +/**
> + * This component shows the runtime as item in RuntimesPane.
> + * Subclass of this component should implemnet following methods.

implemnet -> implement

::: devtools/client/aboutdebugging-new/src/components/RuntimeItem.js:16
(Diff revision 2)
> + * getClassName: which returns class name that is appended to this element.
> + * renderComponents: which returns component or component array.

you could make this explicit by actually adding empty implementations for each method that has to be implemented. With a comment "subclass" should implement etc ... 

You can go a step further by throwing if the dummy implementation is called.

However, as I said in another comment, I would like us to consider composition over inheritance, so let's wait until we have reached consensus on this topic before addressing this.

::: devtools/client/aboutdebugging-new/src/components/ThisFirefoxItem.js:11
(Diff revision 2)
> +
> +const l10n = require("../utils/l10n");
> +
> +const RuntimeItem = require("./RuntimeItem");
> +
> +class ThisFirefoxItem extends RuntimeItem {

I see you have used inheritance for components here and for DebugTargets as well. React seems to advocate against inheritance: https://reactjs.org/docs/composition-vs-inheritance.html

Let's keep this for now, but we need to discuss if we want to stick with inheritance here.

::: devtools/client/aboutdebugging-new/src/utils/moz.build:9
(Diff revision 2)
> +with Files('**'):
> +    BUG_COMPONENT = ('DevTools', 'about:debugging')

same comment as for other patches.

::: devtools/client/locales/en-US/aboutdebugging.properties:212
(Diff revision 2)
>  # LOCALIZATION NOTE (multiProcessWarningConfirmUpdate2):
>  # This string is displayed as a confirmation message when the user clicks on
>  # the multiProcessWarningUpdateLink in about:debugging#workers
>  multiProcessWarningConfirmUpdate2 = Opt out of multiple processes?
> +
> +# LOCALIZATION NOTE (runtims.thisFirefox):

typo: runtims -> runtimes

But I don't think we should localize the Panel right now. 

First reason is that we are very early in development and strings might change before we reach the version we want to ship. Plus we should explain in which UI this is displayed, and how localizers can test it. It should probably live in a different file from the existing about:debugging string. Reusing strings between the old and new about:debugging might also not be ok (confusing for localizers as they have several UIs they need to test when changing a string). Finally we probably want to use fluent-react here.

Can we remove all new strings (and hardcode them in their components instead) before landing. I don't think we should be reusing strings either, but we can address this in a follow where we discuss our localization strategy.

::: devtools/client/themes/images/firefox-logo-glyph.svg:1
(Diff revision 2)
> +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32"><title>newtab-firefox-gry</title><path d="M31.359,14.615h0c-.044-.289-.088-.459-.088-.459s-.113.131-.3.378A10.77,10.77,0,0,0,30.6,12.5a13.846,13.846,0,0,0-.937-2.411,10.048,10.048,0,0,0-.856-1.468q-.176-.263-.359-.51c-.57-.931-1.224-1.5-1.981-2.576a7.806,7.806,0,0,1-.991-2.685A10.844,10.844,0,0,0,25,4.607c-.777-.784-1.453-1.341-1.861-1.721C21.126,1.006,21.36.031,21.36.031h0S17.6,4.228,19.229,8.6a8.4,8.4,0,0,0,2.8,3.733c1.576,1.3,3.273,2.323,4.168,4.937a8.377,8.377,0,0,0-3.144-3.317,7.573,7.573,0,0,1,.6,3,7.124,7.124,0,0,1-8.711,6.94,6.561,6.561,0,0,1-1.765-.6,7.183,7.183,0,0,1-2.115-1.955l-.01-.017.126.046a6.5,6.5,0,0,0,.9.241,5.628,5.628,0,0,0,3.583-.423c1.126-.625,1.808-1.088,2.361-.905l.01,0c.54.172.966-.352.58-.9a2.94,2.94,0,0,0-2.848-1.112c-1.127.164-2.16.965-3.637.189a3.129,3.129,0,0,1-.277-.163c-.1-.057.317.087.22.022a7.33,7.33,0,0,1-.928-.554c-.022-.018.223.07.2.052a3.581,3.581,0,0,1-.968-.979,1.741,1.741,0,0,1-.066-1.554,1.371,1.371,0,0,1,.6-.564c.191.094.309.165.309.165s-.087-.16-.134-.244c.017-.006.032,0,.049-.011.167.072.537.26.732.375a1.016,1.016,0,0,1,.335.3s.067-.033.017-.173a.9.9,0,0,0-.346-.424l.016,0a2.94,2.94,0,0,1,.426.265,2.079,2.079,0,0,0,.17-.9,1.178,1.178,0,0,0-.069-.5c-.053-.1.03-.14.123-.035a.976.976,0,0,0-.079-.238v-.008h0s.053-.069.077-.094a1.43,1.43,0,0,1,.216-.176,9.973,9.973,0,0,1,1.465-.747c.414-.181.757-.319.827-.359a2.3,2.3,0,0,0,.293-.225,1.968,1.968,0,0,0,.66-1.14,1.6,1.6,0,0,0,.017-.178v-.05l0-.03v0l0-.012v0l0-.013h0c-.06-.225-.448-.394-2.476-.584a1.773,1.773,0,0,1-1.45-1.36l0,.009c-.029.074-.055.149-.081.225.026-.075.052-.15.081-.225l0-.016a5.138,5.138,0,0,1,1.986-2.466c.052-.042-.208.011-.156-.032a5.156,5.156,0,0,1,.53-.224c.091-.038-.39-.222-.815-.177a2.2,2.2,0,0,0-.756.178c.1-.086.4-.2.329-.2a4.865,4.865,0,0,0-1.542.583.314.314,0,0,1,.03-.14,2.4,2.4,0,0,0-.964.744,1.275,1.275,0,0,0,.01-.174,2.876,2.876,0,0,0-.473.444l-.009.007a6.285,6.285,0,0,0-3.517-.3l-.01-.009.012,0a2.943,2.943,0,0,1-.625-.7L6.1,5.852,6.081,5.83c-.077-.114-.156-.243-.237-.387-.058-.1-.117-.217-.176-.338,0-.008-.009-.011-.013-.012-.024,0-.041.111-.061.082l0-.006a4.308,4.308,0,0,1-.283-1.687l-.016.008a1.884,1.884,0,0,0-.714.934c-.061.137-.1.212-.14.287,0,.006,0-.01,0-.035.009-.069.039-.211.032-.2s-.012.019-.019.029a1.733,1.733,0,0,0-.251.372,2.355,2.355,0,0,0-.15.382c-.006.021,0-.018,0-.064s.009-.128,0-.111l-.022.043a9.5,9.5,0,0,0-.8,3.035A3.022,3.022,0,0,0,3.2,8.7v.016a6.628,6.628,0,0,0-.817,1.1,15.606,15.606,0,0,0-1.727,4.23,10.351,10.351,0,0,1,.925-1.621,15,15,0,0,0-1.045,5.5,14.233,14.233,0,0,1,.45-1.629A13.807,13.807,0,0,0,2.234,22.76a15.037,15.037,0,0,0,5.951,6.748h0a13.016,13.016,0,0,0,3.468,1.662c.162.059.326.117.494.173-.053-.021-.1-.044-.153-.067a15.7,15.7,0,0,0,4.5.662c5.394,0,7.175-2.054,7.339-2.259h0a2.73,2.73,0,0,0,.637-.856h0q.156-.064.315-.137l.067-.03.121-.057a11.312,11.312,0,0,0,2.277-1.426,5.5,5.5,0,0,0,2.123-3.1h0a1.938,1.938,0,0,0,.029-1.428q.083-.131.171-.28a12.706,12.706,0,0,0,1.907-6.181v-.006c0-.059,0-.118,0-.177A7.731,7.731,0,0,0,31.359,14.615Z" fill="#d7d7db"/></svg>

Missing license
Attachment #8991850 - Flags: review?(jdescottes) → review+
Comment on attachment 8991851 [details]
Bug 1471795 - Part 4: Implement an action of when runtime was selected.

https://reviewboard.mozilla.org/r/256756/#review264612

::: devtools/client/aboutdebugging-new/src/components/ThisFirefoxItem.css:18
(Diff revision 2)
>  .runtime-item--this-firefox::before {
>    background-image: url(chrome://devtools/skin/images/firefox-logo-glyph.svg);
>    background-repeat: none;
>    background-size: contain;
>    content: "";
> +  fill: #d7d7db;

That's --grey-30 since we have variables.css now.

::: devtools/client/aboutdebugging-new/src/components/ThisFirefoxItem.css:29
(Diff revision 2)
> +
> +.runtime-item--this-firefox--selected {
> +  color: var(--blue-55);
> +}
> +
> +.runtime-item--this-firefox--selected::before {

I'll let Belén comment about that but I think the convention is __ to separate components, and -- to separate states.

Which here would lead to 

  .runtime-item__this-firefox--selected
  
It will probably be easier to just do a CSS review & cleanup in a follow up bug.

::: devtools/client/aboutdebugging-new/src/store.js:12
(Diff revision 2)
> +const { createStore } = require("devtools/client/shared/vendor/redux");
> +
> +const rootReducer = require("./reducers/index");
> +const { RuntimesState } = require("./reducers/runtimes-state");
> +
> +module.exports = function() {

In other store.js / create-store.js files we do

exports.configureStore = ... 

We should do the same for consistency

::: devtools/client/themes/images/firefox-logo-glyph.svg:1
(Diff revision 2)
> -<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32"><title>newtab-firefox-gry</title><path d="M31.359,14.615h0c-.044-.289-.088-.459-.088-.459s-.113.131-.3.378A10.77,10.77,0,0,0,30.6,12.5a13.846,13.846,0,0,0-.937-2.411,10.048,10.048,0,0,0-.856-1.468q-.176-.263-.359-.51c-.57-.931-1.224-1.5-1.981-2.576a7.806,7.806,0,0,1-.991-2.685A10.844,10.844,0,0,0,25,4.607c-.777-.784-1.453-1.341-1.861-1.721C21.126,1.006,21.36.031,21.36.031h0S17.6,4.228,19.229,8.6a8.4,8.4,0,0,0,2.8,3.733c1.576,1.3,3.273,2.323,4.168,4.937a8.377,8.377,0,0,0-3.144-3.317,7.573,7.573,0,0,1,.6,3,7.124,7.124,0,0,1-8.711,6.94,6.561,6.561,0,0,1-1.765-.6,7.183,7.183,0,0,1-2.115-1.955l-.01-.017.126.046a6.5,6.5,0,0,0,.9.241,5.628,5.628,0,0,0,3.583-.423c1.126-.625,1.808-1.088,2.361-.905l.01,0c.54.172.966-.352.58-.9a2.94,2.94,0,0,0-2.848-1.112c-1.127.164-2.16.965-3.637.189a3.129,3.129,0,0,1-.277-.163c-.1-.057.317.087.22.022a7.33,7.33,0,0,1-.928-.554c-.022-.018.223.07.2.052a3.581,3.581,0,0,1-.968-.979,1.741,1.741,0,0,1-.066-1.554,1.371,1.371,0,0,1,.6-.564c.191.094.309.165.309.165s-.087-.16-.134-.244c.017-.006.032,0,.049-.011.167.072.537.26.732.375a1.016,1.016,0,0,1,.335.3s.067-.033.017-.173a.9.9,0,0,0-.346-.424l.016,0a2.94,2.94,0,0,1,.426.265,2.079,2.079,0,0,0,.17-.9,1.178,1.178,0,0,0-.069-.5c-.053-.1.03-.14.123-.035a.976.976,0,0,0-.079-.238v-.008h0s.053-.069.077-.094a1.43,1.43,0,0,1,.216-.176,9.973,9.973,0,0,1,1.465-.747c.414-.181.757-.319.827-.359a2.3,2.3,0,0,0,.293-.225,1.968,1.968,0,0,0,.66-1.14,1.6,1.6,0,0,0,.017-.178v-.05l0-.03v0l0-.012v0l0-.013h0c-.06-.225-.448-.394-2.476-.584a1.773,1.773,0,0,1-1.45-1.36l0,.009c-.029.074-.055.149-.081.225.026-.075.052-.15.081-.225l0-.016a5.138,5.138,0,0,1,1.986-2.466c.052-.042-.208.011-.156-.032a5.156,5.156,0,0,1,.53-.224c.091-.038-.39-.222-.815-.177a2.2,2.2,0,0,0-.756.178c.1-.086.4-.2.329-.2a4.865,4.865,0,0,0-1.542.583.314.314,0,0,1,.03-.14,2.4,2.4,0,0,0-.964.744,1.275,1.275,0,0,0,.01-.174,2.876,2.876,0,0,0-.473.444l-.009.007a6.285,6.285,0,0,0-3.517-.3l-.01-.009.012,0a2.943,2.943,0,0,1-.625-.7L6.1,5.852,6.081,5.83c-.077-.114-.156-.243-.237-.387-.058-.1-.117-.217-.176-.338,0-.008-.009-.011-.013-.012-.024,0-.041.111-.061.082l0-.006a4.308,4.308,0,0,1-.283-1.687l-.016.008a1.884,1.884,0,0,0-.714.934c-.061.137-.1.212-.14.287,0,.006,0-.01,0-.035.009-.069.039-.211.032-.2s-.012.019-.019.029a1.733,1.733,0,0,0-.251.372,2.355,2.355,0,0,0-.15.382c-.006.021,0-.018,0-.064s.009-.128,0-.111l-.022.043a9.5,9.5,0,0,0-.8,3.035A3.022,3.022,0,0,0,3.2,8.7v.016a6.628,6.628,0,0,0-.817,1.1,15.606,15.606,0,0,0-1.727,4.23,10.351,10.351,0,0,1,.925-1.621,15,15,0,0,0-1.045,5.5,14.233,14.233,0,0,1,.45-1.629A13.807,13.807,0,0,0,2.234,22.76a15.037,15.037,0,0,0,5.951,6.748h0a13.016,13.016,0,0,0,3.468,1.662c.162.059.326.117.494.173-.053-.021-.1-.044-.153-.067a15.7,15.7,0,0,0,4.5.662c5.394,0,7.175-2.054,7.339-2.259h0a2.73,2.73,0,0,0,.637-.856h0q.156-.064.315-.137l.067-.03.121-.057a11.312,11.312,0,0,0,2.277-1.426,5.5,5.5,0,0,0,2.123-3.1h0a1.938,1.938,0,0,0,.029-1.428q.083-.131.171-.28a12.706,12.706,0,0,0,1.907-6.181v-.006c0-.059,0-.118,0-.177A7.731,7.731,0,0,0,31.359,14.615Z" fill="#d7d7db"/></svg>
> +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32"><title>newtab-firefox-gry</title><path d="M31.359,14.615h0c-.044-.289-.088-.459-.088-.459s-.113.131-.3.378A10.77,10.77,0,0,0,30.6,12.5a13.846,13.846,0,0,0-.937-2.411,10.048,10.048,0,0,0-.856-1.468q-.176-.263-.359-.51c-.57-.931-1.224-1.5-1.981-2.576a7.806,7.806,0,0,1-.991-2.685A10.844,10.844,0,0,0,25,4.607c-.777-.784-1.453-1.341-1.861-1.721C21.126,1.006,21.36.031,21.36.031h0S17.6,4.228,19.229,8.6a8.4,8.4,0,0,0,2.8,3.733c1.576,1.3,3.273,2.323,4.168,4.937a8.377,8.377,0,0,0-3.144-3.317,7.573,7.573,0,0,1,.6,3,7.124,7.124,0,0,1-8.711,6.94,6.561,6.561,0,0,1-1.765-.6,7.183,7.183,0,0,1-2.115-1.955l-.01-.017.126.046a6.5,6.5,0,0,0,.9.241,5.628,5.628,0,0,0,3.583-.423c1.126-.625,1.808-1.088,2.361-.905l.01,0c.54.172.966-.352.58-.9a2.94,2.94,0,0,0-2.848-1.112c-1.127.164-2.16.965-3.637.189a3.129,3.129,0,0,1-.277-.163c-.1-.057.317.087.22.022a7.33,7.33,0,0,1-.928-.554c-.022-.018.223.07.2.052a3.581,3.581,0,0,1-.968-.979,1.741,1.741,0,0,1-.066-1.554,1.371,1.371,0,0,1,.6-.564c.191.094.309.165.309.165s-.087-.16-.134-.244c.017-.006.032,0,.049-.011.167.072.537.26.732.375a1.016,1.016,0,0,1,.335.3s.067-.033.017-.173a.9.9,0,0,0-.346-.424l.016,0a2.94,2.94,0,0,1,.426.265,2.079,2.079,0,0,0,.17-.9,1.178,1.178,0,0,0-.069-.5c-.053-.1.03-.14.123-.035a.976.976,0,0,0-.079-.238v-.008h0s.053-.069.077-.094a1.43,1.43,0,0,1,.216-.176,9.973,9.973,0,0,1,1.465-.747c.414-.181.757-.319.827-.359a2.3,2.3,0,0,0,.293-.225,1.968,1.968,0,0,0,.66-1.14,1.6,1.6,0,0,0,.017-.178v-.05l0-.03v0l0-.012v0l0-.013h0c-.06-.225-.448-.394-2.476-.584a1.773,1.773,0,0,1-1.45-1.36l0,.009c-.029.074-.055.149-.081.225.026-.075.052-.15.081-.225l0-.016a5.138,5.138,0,0,1,1.986-2.466c.052-.042-.208.011-.156-.032a5.156,5.156,0,0,1,.53-.224c.091-.038-.39-.222-.815-.177a2.2,2.2,0,0,0-.756.178c.1-.086.4-.2.329-.2a4.865,4.865,0,0,0-1.542.583.314.314,0,0,1,.03-.14,2.4,2.4,0,0,0-.964.744,1.275,1.275,0,0,0,.01-.174,2.876,2.876,0,0,0-.473.444l-.009.007a6.285,6.285,0,0,0-3.517-.3l-.01-.009.012,0a2.943,2.943,0,0,1-.625-.7L6.1,5.852,6.081,5.83c-.077-.114-.156-.243-.237-.387-.058-.1-.117-.217-.176-.338,0-.008-.009-.011-.013-.012-.024,0-.041.111-.061.082l0-.006a4.308,4.308,0,0,1-.283-1.687l-.016.008a1.884,1.884,0,0,0-.714.934c-.061.137-.1.212-.14.287,0,.006,0-.01,0-.035.009-.069.039-.211.032-.2s-.012.019-.019.029a1.733,1.733,0,0,0-.251.372,2.355,2.355,0,0,0-.15.382c-.006.021,0-.018,0-.064s.009-.128,0-.111l-.022.043a9.5,9.5,0,0,0-.8,3.035A3.022,3.022,0,0,0,3.2,8.7v.016a6.628,6.628,0,0,0-.817,1.1,15.606,15.606,0,0,0-1.727,4.23,10.351,10.351,0,0,1,.925-1.621,15,15,0,0,0-1.045,5.5,14.233,14.233,0,0,1,.45-1.629A13.807,13.807,0,0,0,2.234,22.76a15.037,15.037,0,0,0,5.951,6.748h0a13.016,13.016,0,0,0,3.468,1.662c.162.059.326.117.494.173-.053-.021-.1-.044-.153-.067a15.7,15.7,0,0,0,4.5.662c5.394,0,7.175-2.054,7.339-2.259h0a2.73,2.73,0,0,0,.637-.856h0q.156-.064.315-.137l.067-.03.121-.057a11.312,11.312,0,0,0,2.277-1.426,5.5,5.5,0,0,0,2.123-3.1h0a1.938,1.938,0,0,0,.029-1.428q.083-.131.171-.28a12.706,12.706,0,0,0,1.907-6.181v-.006c0-.059,0-.118,0-.177A7.731,7.731,0,0,0,31.359,14.615Z" fill="context-fill"/></svg>

Maybe we could move the fill as an attribute of the svg or at least as one of the first attributes of the path. Here it's hard to spot. We could also probably have a line break between <svg> and <path> and remove the <title>.
Attachment #8991851 - Flags: review?(jdescottes) → review+
Comment on attachment 8991852 [details]
Bug 1471795 - Part 5: Implement basis of DebugTargetsPanel.

https://reviewboard.mozilla.org/r/256758/#review264616

::: devtools/client/aboutdebugging-new/src/components/App.js:34
(Diff revision 2)
>      return dom.div(
>        {
>          className: "app",
>        },
> -      RuntimesPane({ selectedRuntime, thisFirefox })
> +      RuntimesPane({ selectedRuntime, thisFirefox }),
> +      selectedRuntime ? DebugTargetsPane({ runtime: selectedRuntime }) : null

The "Connect page" will be displayed here as well. Maybe we will need to use more generic terms when we introduce it. 

RuntimesPane -> Sidebar
DebugTarget -> ??
Attachment #8991852 - Flags: review?(jdescottes) → review+
Comment on attachment 8991849 [details]
Bug 1471795 - Part 2: Introduce Runtime class.

https://reviewboard.mozilla.org/r/256752/#review264588

> Not sure it's worth requiring this just for proptype checking. We rarely run react in development mode, so I would be fine with just having PropTypes.object

I had thought that it will be easier to understand which instance we handle.
What do you think?

> nit: I think this is the same as not defining a constructor, but I guess you are complexifying this constructor in subsequent patches.

Ah, indeed, we don't need in this patch, although the constructor actually add some code in another patch. I'll drop this.
Comment on attachment 8991850 [details]
Bug 1471795 - Part 3: Implement runtimes pane.

https://reviewboard.mozilla.org/r/256754/#review264594

Okay!

> I see this is still empty at the end of the patch series. We should drop this file for now.

Okay!

> typo: runtims -> runtimes
> 
> But I don't think we should localize the Panel right now. 
> 
> First reason is that we are very early in development and strings might change before we reach the version we want to ship. Plus we should explain in which UI this is displayed, and how localizers can test it. It should probably live in a different file from the existing about:debugging string. Reusing strings between the old and new about:debugging might also not be ok (confusing for localizers as they have several UIs they need to test when changing a string). Finally we probably want to use fluent-react here.
> 
> Can we remove all new strings (and hardcode them in their components instead) before landing. I don't think we should be reusing strings either, but we can address this in a follow where we discuss our localization strategy.

Yeah, indeed. I'll drop this, and change all codes that are referring l10n in this time.
Comment on attachment 8991851 [details]
Bug 1471795 - Part 4: Implement an action of when runtime was selected.

https://reviewboard.mozilla.org/r/256756/#review264612

> I'll let Belén comment about that but I think the convention is __ to separate components, and -- to separate states.
> 
> Which here would lead to 
> 
>   .runtime-item__this-firefox--selected
>   
> It will probably be easier to just do a CSS review & cleanup in a follow up bug.

Ah, I see. I intented `.runtime-item.this-firefox.selected` in normal class expression since the ThisFirefox is inherited RuntimeItem.
I wait for Belén's advice.

> In other store.js / create-store.js files we do
> 
> exports.configureStore = ... 
> 
> We should do the same for consistency

Okay.

> Maybe we could move the fill as an attribute of the svg or at least as one of the first attributes of the path. Here it's hard to spot. We could also probably have a line break between <svg> and <path> and remove the <title>.

Okay.
Comment on attachment 8991853 [details]
Bug 1471795 - Part 6: Implement RuntimeInformation.

https://reviewboard.mozilla.org/r/256760/#review264646

We should have a follow up dedicated to remove all usage of 
  componentWillMount
  componentWillReceiveProps
  componentWillUpdate

(as of now, there are just 2 places using componentWillReceiveProps, so we're not in bad shape :) )

::: devtools/client/aboutdebugging-new/src/components/DebugTargetsPane.js:30
(Diff revision 2)
> +      info: {},
> +    };
> +    this.update(props.runtime);
> +  }
> +
> +  componentWillReceiveProps(nextProps) {

This lifecycle method will soon be deprecated. We should try to write this new UI using the latest recommendations from React: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html

I am almost tempted to blacklist them via eslint for aboutdebugging-new/ so that we don't slip by mistake.

::: devtools/client/aboutdebugging-new/src/components/RuntimeInfo.js:11
(Diff revision 2)
> +
> +const { PureComponent } = require("devtools/client/shared/vendor/react");
> +const dom = require("devtools/client/shared/vendor/react-dom-factories");
> +const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> +
> +class RuntimeInfo extends PureComponent {

Each component should have a small comment describing its role

::: devtools/client/aboutdebugging-new/src/runtimes/this-firefox.js:7
(Diff revision 2)
>   * 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/. */
>  
>  "use strict";
>  
> +const { Services } = require("resource://gre/modules/Services.jsm");

require("Services")
Attachment #8991853 - Flags: review?(jdescottes) → review+
Comment on attachment 8991854 [details]
Bug 1471795 - Part 7: Implement tab list.

https://reviewboard.mozilla.org/r/256762/#review264652

Re: inheritance vs composition, here most of the code contained in TabItem is pure JS logic, not UI/Component related.

::: devtools/client/aboutdebugging-new/aboutdebugging.js:57
(Diff revision 2)
>    get mount() {
>      return document.getElementById("mount");
>    },
>  
> -  updateSelectedRuntime(runtime) {
> +  async updateSelectedRuntime(runtime) {
> +    await runtime.connect();

I wonder if the call to connect() should be done by the action? So that everytime we want to change the runtime, connect() will be called automatically.

::: devtools/client/aboutdebugging-new/src/components/DebugTargetItem.css:24
(Diff revision 2)
> +.debug-target-item__info__detail {
> +}

Still empty at the end of the series.
Attachment #8991854 - Flags: review?(jdescottes) → review+
Comment on attachment 8991855 [details]
Bug 1471795 - Part 8: Add debug button to tab list.

https://reviewboard.mozilla.org/r/256764/#review264708

::: devtools/client/aboutdebugging-new/src/components/DebugTargetItem.js:43
(Diff revision 2)
>    getName() {
>      throw new Error("Subclass of DebugTargetItem should override getName()");
>    }
>  
>    /**
> +   * Render components which trigger action.

-> plural: which (can) trigger actions.

::: devtools/client/aboutdebugging-new/src/runtimes/runtime.js:46
(Diff revision 2)
>    async getTabs() {
>      throw new Error("Subclass of Runtime should override getTabs()");
>    }
> +
> +  /**
> +   * Inspect given debug target of tab which can get by getTabs().

nit: rephrase to "Inspect the provided tab target"
Attachment #8991855 - Flags: review?(jdescottes) → review+
Comment on attachment 8991856 [details]
Bug 1471795 - Part 9: Update tab list ui when the actual tabs were changed.

https://reviewboard.mozilla.org/r/256766/#review264714

I am not an expert in React and Redux, but I feel uneasy with the current direction.

I feel like here, the runtime should dispatch an action when the list of tabs changes. 
Then maybe a reducer should take care of extracting the data we need for our components?

We can land like this, but I feel like we need a follow up to get some help designing our actions and reducers and avoid complexifying our components.

::: devtools/client/aboutdebugging-new/src/components/DebugTargetsPane.js:39
(Diff revision 2)
>      this.update(props.runtime);
>    }
>  
>    componentWillReceiveProps(nextProps) {
> +    const { runtime } = this.props;
> +    runtime.removeTabsUpdateListener(this.onTabsUpdate);

typo: onTabsUpdate -> onTabsUpdated

::: devtools/client/aboutdebugging-new/src/components/DebugTargetsPane.js:46
(Diff revision 2)
>      this.update(nextProps.runtime);
>    }
>  
> +  componentWillUnmount() {
> +    const { runtime } = this.props;
> +    runtime.removeTabsUpdateListener(this.onTabsUpdate);

Same comment
Attachment #8991856 - Flags: review?(jdescottes) → review+
Comment on attachment 8991857 [details]
Bug 1471795 - Part 10: Implement extension list.

https://reviewboard.mozilla.org/r/256768/#review264716

We need a follow up to reduce code duplication between the existing about:debugging and the new one.
That will also keep the components code simpler.

::: devtools/client/aboutdebugging-new/src/components/ExtensionItem.js:42
(Diff revision 2)
> +    if (!debugTarget.temporarilyInstalled ||
> +        !debugTarget.url ||
> +        !debugTarget.url.startsWith("file://")) {
> +      return null;
> +    }
> +
> +    let path = "";
> +    // Strip a leading slash from Windows drive letter URIs.
> +    // file:///home/foo ~> /home/foo
> +    // file:///C:/foo ~> C:/foo
> +    const windowsRegex = /^file:\/\/\/([a-zA-Z]:\/.*)/;
> +    if (windowsRegex.test(debugTarget.url)) {
> +      path = windowsRegex.exec(debugTarget.url)[1];
> +    } else {
> +      path = debugTarget.url.slice("file://".length);
> +    }

We started extracting modules in pure JS modules in about:debugging (/devtools/client/aboutdebugging/modules/addon.js). I think that's the right direction for this kind of code.

We can import the modules from about debugging and share the code.
Attachment #8991857 - Flags: review?(jdescottes) → review+
Comment on attachment 8991858 [details]
Bug 1471795 - Part 11: Add inspect button.

https://reviewboard.mozilla.org/r/256770/#review264718

::: devtools/client/aboutdebugging-new/src/runtimes/runtime.js:64
(Diff revision 2)
>    async getTabs() {
>      throw new Error("Subclass of Runtime should override getTabs()");
>    }
>  
>    /**
> -   * Inspect given debug target of tab which can get by getTabs().
> +   * Inspect given debug target of extension which can get by getTabs().

Wrong comment, mentions getTabs instead of getExtensions

::: devtools/client/aboutdebugging-new/src/runtimes/runtime.js:73
(Diff revision 2)
> +  async inspectExtension(_) {
> +    throw new Error("Subclass of Runtime should override inspectExtension()");
> +  }
> +
> +  /**
> +   * Inspect given debug target of tab which can get by getExtensions().

Same comment

::: devtools/client/aboutdebugging-new/src/runtimes/this-firefox.js:8
(Diff revision 2)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +const { BrowserToolboxProcess } =
> +  require("resource://devtools/client/framework/ToolboxProcess.jsm");

We need a follow up to review which requires could be switched to "lazy" requires (ie, only loading the file when actually used)
Attachment #8991858 - Flags: review?(jdescottes) → review+
Comment on attachment 8991859 [details]
Bug 1471795 - Part 12: Update extension list ui when the extensions were added or removed.

https://reviewboard.mozilla.org/r/256772/#review264724

::: devtools/client/aboutdebugging-new/src/components/TemporaryExtensionInstaller.js:16
(Diff revision 2)
> +const l10n = require("../utils/l10n");
> +
> +class TemporaryExtensionInstaller extends PureComponent {
> +  async install() {
> +    try {
> +      await installTemporaryExtension();

Depending on our unit testing solution this might make our components hard to unit test. To interact with heavy APIs such as this one, the webconsole uses an interesting approach. 

They have a prop which is called "serviceContainer" which provides functions to access APIs that are outside of the scope of the React application (such as installing an addon, here). This way it's very easy to mock the serviceContainer prop.

::: devtools/client/aboutdebugging-new/src/runtimes/this-firefox.js:26
(Diff revision 2)
>    constructor() {
>      super();
> +
> +    this.extensionsListeners = [];
> +
> +    AddonManager.addAddonListener(this);

Should this be done in the constructor or in connect() ?

::: devtools/client/aboutdebugging-new/src/runtimes/this-firefox.js:108
(Diff revision 2)
> +  onInstalled() {
> +    this.onExtensionsUpdated();
> +  }
> +
> +  /**
> +   * Mandatory callback as AddonManager listener.
> +   */
> +  onUninstalled() {
> +    this.onExtensionsUpdated();
> +  }
> +
> +  /**
> +   * Mandatory callback as AddonManager listener.
> +   */
> +  onEnabled() {
> +    this.onExtensionsUpdated();
> +  }
> +
> +  /**
> +   * Mandatory callback as AddonManager listener.
> +   */
> +  onDisabled() {
> +    this.onExtensionsUpdated();
> +  }

We can probably create a shared addonManagerListener helper that could be used from both the old and the new about:debugging. Should be handled in one of the follow-up bugs.
Attachment #8991859 - Flags: review?(jdescottes) → review+
Comment on attachment 8991853 [details]
Bug 1471795 - Part 6: Implement RuntimeInformation.

https://reviewboard.mozilla.org/r/256760/#review264646

> This lifecycle method will soon be deprecated. We should try to write this new UI using the latest recommendations from React: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html
> 
> I am almost tempted to blacklist them via eslint for aboutdebugging-new/ so that we don't slip by mistake.

Thanks. Will change to componentDidUpdate().
And the lint would be very helpful!

> Each component should have a small comment describing its role

okay!
Comment on attachment 8991854 [details]
Bug 1471795 - Part 7: Implement tab list.

https://reviewboard.mozilla.org/r/256762/#review264652

Yeah, indeed, we can change to composition. If need in this bug, please let me know. I'll do all of related component.

> I wonder if the call to connect() should be done by the action? So that everytime we want to change the runtime, connect() will be called automatically.

I see, let me do that.

> Still empty at the end of the series.

Yes, I'll drop.
Comment on attachment 8991857 [details]
Bug 1471795 - Part 10: Implement extension list.

https://reviewboard.mozilla.org/r/256768/#review264716

> We started extracting modules in pure JS modules in about:debugging (/devtools/client/aboutdebugging/modules/addon.js). I think that's the right direction for this kind of code.
> 
> We can import the modules from about debugging and share the code.

I had thought so. However, because most of functions in module/addons are provided in runtime.js, I extracted part of the function we need.
So as to keep the code simplar, I'd like to define parseFileUri function to this component, in this time.
Comment on attachment 8991859 [details]
Bug 1471795 - Part 12: Update extension list ui when the extensions were added or removed.

https://reviewboard.mozilla.org/r/256772/#review264724

> Depending on our unit testing solution this might make our components hard to unit test. To interact with heavy APIs such as this one, the webconsole uses an interesting approach. 
> 
> They have a prop which is called "serviceContainer" which provides functions to access APIs that are outside of the scope of the React application (such as installing an addon, here). This way it's very easy to mock the serviceContainer prop.

Oh, I'm sorry. This change should be in patch 13.
Also, in patch 13, the installTemporaryExtension is introduced in runtime.js
I'll drop this component from this patch, anyway.

> Should this be done in the constructor or in connect() ?

Indeed, in connect() is better!

> We can probably create a shared addonManagerListener helper that could be used from both the old and the new about:debugging. Should be handled in one of the follow-up bugs.

Okay.
Comment on attachment 8991859 [details]
Bug 1471795 - Part 12: Update extension list ui when the extensions were added or removed.

https://reviewboard.mozilla.org/r/256772/#review264724

> Indeed, in connect() is better!

Ah, if we move to connect(), need to add a function which detects that this runtime was unselected since need to remove the listener.
I'll add the mechanism.
Comment on attachment 8991859 [details]
Bug 1471795 - Part 12: Update extension list ui when the extensions were added or removed.

https://reviewboard.mozilla.org/r/256772/#review264724

> Ah, if we move to connect(), need to add a function which detects that this runtime was unselected since need to remove the listener.
> I'll add the mechanism.

Hmm, I moved runtime.connect() to the action in patch 7 though, it might be better to revert because we need to call disconnect() for previous selected runtime as well.
In this time, I add a parameter for previous runtime to updateSelectedRuntime() though, if you think weird, will revert this.
Comment on attachment 8991848 [details]
Bug 1471795 - Part 1: Implement basis of 'This Firefox' page.

https://reviewboard.mozilla.org/r/256750/#review264952

::: devtools/client/aboutdebugging-new/aboutdebugging.css:8
(Diff revision 2)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +@import "resource://devtools/client/aboutdebugging-new/src/components/App.css";
> +
> +html, body, #mount {
> +  height: 100%;

This probably could be simplified by setting the width/height of a single element (for isntance, in `.app` at the `App.css` file below) with viewport units (vw and vh) instead of 100% on three of them
Attachment #8991848 - Flags: review?(balbeza) → review+
I have updated patches 1 ~ 12 along to Julian's review.

Julian, could you check if needed?
Belén, thank you for the reviewing!
I'll fix your points tomorrow.
Comment on attachment 8991850 [details]
Bug 1471795 - Part 3: Implement runtimes pane.

https://reviewboard.mozilla.org/r/256754/#review264964

::: devtools/client/aboutdebugging-new/src/components/RuntimeItem.js:16
(Diff revision 2)
> + * getClassName: which returns class name that is appended to this element.
> + * renderComponents: which returns component or component array.

I'd give preference for composition if possible, too

::: devtools/client/aboutdebugging-new/src/components/RuntimesPane.css:7
(Diff revision 2)
> + * 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/. */
> +
> +.runtimes-pane {
> +  margin-block-start: 70px;
> +  width: 240px;

This isn't preventing the sidebar to actually be shorter than this amount (try to shrink the window size and you'll see the glitch), so I'm asssuming something else is messing this up.

::: devtools/client/aboutdebugging-new/src/components/RuntimesPane.js:25
(Diff revision 2)
> +  }
> +
> +  render() {
> +    const { thisFirefox } = this.props;
> +
> +    return dom.ul(

It's more semantic –and easier to layout with CSS afterwards– if the `ul` is wrapped in a higher-level container, like `section` or `aside`.

::: devtools/client/aboutdebugging-new/src/components/ThisFirefoxItem.css:5
(Diff revision 2)
> +/* 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/. */
> +
> +.runtime-item--this-firefox {

If we later add more items to the sidebar, it seems that this block of styles would also apply to them, so just a `.runtime-item` class would be better I think

::: devtools/client/aboutdebugging-new/src/components/ThisFirefoxItem.css:5
(Diff revision 2)
> +/* 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/. */
> +
> +.runtime-item--this-firefox {

if we later add more items to this sidebar, it seems that this block of styles would apply to them as well. In this case, just `.runtime-item` might be a better class and this could be moved to the `RuntimeItem` component instead.

::: devtools/client/aboutdebugging-new/src/components/ThisFirefoxItem.css:10
(Diff revision 2)
> +.runtime-item--this-firefox {
> +  align-items: center;
> +  display: flex;
> +  font-size: 20px;
> +  margin-inline-start: 24px;
> +  height: 48px;

I'm not sure why the height needs to be manually set.  It's also easier to deal with in the long run if we don't set this manually and instead add padding –so if we later change the `font-size` or `font-family`, or if a text spans over multiple lines (because of localization) it won't cause a wreck in the layout

::: devtools/client/aboutdebugging-new/src/components/ThisFirefoxItem.css:13
(Diff revision 2)
> +  font-size: 20px;
> +  margin-inline-start: 24px;
> +  height: 48px;
> +}
> +
> +.runtime-item--this-firefox::before {

A similiar reasoning to the above comment re: the class name. If the items of the sidebar has icons with the same sizes, this style should be shared and probable be in the `RuntimeItem` component

::: devtools/client/aboutdebugging-new/src/components/ThisFirefoxItem.js:11
(Diff revision 2)
> +
> +const l10n = require("../utils/l10n");
> +
> +const RuntimeItem = require("./RuntimeItem");
> +
> +class ThisFirefoxItem extends RuntimeItem {

It feels a bit overkill to have a distinct component for this, since it doesn't have special behaviour different to other runtime items –just the text content and the icon.

Maybe it'd be a good idea to have just one RuntimeItem component and use props and/or slots for the text and icons

::: devtools/client/themes/images/firefox-logo-glyph.svg:1
(Diff revision 2)
> +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32"><title>newtab-firefox-gry</title><path d="M31.359,14.615h0c-.044-.289-.088-.459-.088-.459s-.113.131-.3.378A10.77,10.77,0,0,0,30.6,12.5a13.846,13.846,0,0,0-.937-2.411,10.048,10.048,0,0,0-.856-1.468q-.176-.263-.359-.51c-.57-.931-1.224-1.5-1.981-2.576a7.806,7.806,0,0,1-.991-2.685A10.844,10.844,0,0,0,25,4.607c-.777-.784-1.453-1.341-1.861-1.721C21.126,1.006,21.36.031,21.36.031h0S17.6,4.228,19.229,8.6a8.4,8.4,0,0,0,2.8,3.733c1.576,1.3,3.273,2.323,4.168,4.937a8.377,8.377,0,0,0-3.144-3.317,7.573,7.573,0,0,1,.6,3,7.124,7.124,0,0,1-8.711,6.94,6.561,6.561,0,0,1-1.765-.6,7.183,7.183,0,0,1-2.115-1.955l-.01-.017.126.046a6.5,6.5,0,0,0,.9.241,5.628,5.628,0,0,0,3.583-.423c1.126-.625,1.808-1.088,2.361-.905l.01,0c.54.172.966-.352.58-.9a2.94,2.94,0,0,0-2.848-1.112c-1.127.164-2.16.965-3.637.189a3.129,3.129,0,0,1-.277-.163c-.1-.057.317.087.22.022a7.33,7.33,0,0,1-.928-.554c-.022-.018.223.07.2.052a3.581,3.581,0,0,1-.968-.979,1.741,1.741,0,0,1-.066-1.554,1.371,1.371,0,0,1,.6-.564c.191.094.309.165.309.165s-.087-.16-.134-.244c.017-.006.032,0,.049-.011.167.072.537.26.732.375a1.016,1.016,0,0,1,.335.3s.067-.033.017-.173a.9.9,0,0,0-.346-.424l.016,0a2.94,2.94,0,0,1,.426.265,2.079,2.079,0,0,0,.17-.9,1.178,1.178,0,0,0-.069-.5c-.053-.1.03-.14.123-.035a.976.976,0,0,0-.079-.238v-.008h0s.053-.069.077-.094a1.43,1.43,0,0,1,.216-.176,9.973,9.973,0,0,1,1.465-.747c.414-.181.757-.319.827-.359a2.3,2.3,0,0,0,.293-.225,1.968,1.968,0,0,0,.66-1.14,1.6,1.6,0,0,0,.017-.178v-.05l0-.03v0l0-.012v0l0-.013h0c-.06-.225-.448-.394-2.476-.584a1.773,1.773,0,0,1-1.45-1.36l0,.009c-.029.074-.055.149-.081.225.026-.075.052-.15.081-.225l0-.016a5.138,5.138,0,0,1,1.986-2.466c.052-.042-.208.011-.156-.032a5.156,5.156,0,0,1,.53-.224c.091-.038-.39-.222-.815-.177a2.2,2.2,0,0,0-.756.178c.1-.086.4-.2.329-.2a4.865,4.865,0,0,0-1.542.583.314.314,0,0,1,.03-.14,2.4,2.4,0,0,0-.964.744,1.275,1.275,0,0,0,.01-.174,2.876,2.876,0,0,0-.473.444l-.009.007a6.285,6.285,0,0,0-3.517-.3l-.01-.009.012,0a2.943,2.943,0,0,1-.625-.7L6.1,5.852,6.081,5.83c-.077-.114-.156-.243-.237-.387-.058-.1-.117-.217-.176-.338,0-.008-.009-.011-.013-.012-.024,0-.041.111-.061.082l0-.006a4.308,4.308,0,0,1-.283-1.687l-.016.008a1.884,1.884,0,0,0-.714.934c-.061.137-.1.212-.14.287,0,.006,0-.01,0-.035.009-.069.039-.211.032-.2s-.012.019-.019.029a1.733,1.733,0,0,0-.251.372,2.355,2.355,0,0,0-.15.382c-.006.021,0-.018,0-.064s.009-.128,0-.111l-.022.043a9.5,9.5,0,0,0-.8,3.035A3.022,3.022,0,0,0,3.2,8.7v.016a6.628,6.628,0,0,0-.817,1.1,15.606,15.606,0,0,0-1.727,4.23,10.351,10.351,0,0,1,.925-1.621,15,15,0,0,0-1.045,5.5,14.233,14.233,0,0,1,.45-1.629A13.807,13.807,0,0,0,2.234,22.76a15.037,15.037,0,0,0,5.951,6.748h0a13.016,13.016,0,0,0,3.468,1.662c.162.059.326.117.494.173-.053-.021-.1-.044-.153-.067a15.7,15.7,0,0,0,4.5.662c5.394,0,7.175-2.054,7.339-2.259h0a2.73,2.73,0,0,0,.637-.856h0q.156-.064.315-.137l.067-.03.121-.057a11.312,11.312,0,0,0,2.277-1.426,5.5,5.5,0,0,0,2.123-3.1h0a1.938,1.938,0,0,0,.029-1.428q.083-.131.171-.28a12.706,12.706,0,0,0,1.907-6.181v-.006c0-.059,0-.118,0-.177A7.731,7.731,0,0,0,31.359,14.615Z" fill="#d7d7db"/></svg>

I have been told in another review to add a license header to a SVG file, so it might be a good idea to do it here as well :)
Attachment #8991850 - Flags: review?(balbeza) → review+
Comment on attachment 8991850 [details]
Bug 1471795 - Part 3: Implement runtimes pane.

https://reviewboard.mozilla.org/r/256754/#review264998

::: devtools/client/aboutdebugging-new/src/components/runtime/ThisFirefoxItem.css:15
(Diff revision 3)
> +  height: 48px;
> +}
> +
> +.runtime-item--this-firefox::before {
> +  background-image: url(chrome://devtools/skin/images/firefox-logo-glyph.svg);
> +  background-repeat: none;

`background-repeat` is being overriden here so this isn't taking effect and there's a glitch when the window is shrinked. (I'm not sure if this is caused by a later patch)
Comment on attachment 8991851 [details]
Bug 1471795 - Part 4: Implement an action of when runtime was selected.

https://reviewboard.mozilla.org/r/256756/#review265000

::: devtools/client/aboutdebugging-new/src/components/runtime/RuntimeItem.js:38
(Diff revision 3)
>        className,
>      ];
>  
> +    if (runtime === selectedRuntime) {
> +      classes.push("runtime-item--selected");
> +      classes.push(`${ className }--selected`);

Question: what is this needed for?

::: devtools/client/aboutdebugging-new/src/components/runtime/ThisFirefoxItem.css:1
(Diff revision 3)
>  /* This Source Code Form is subject to the terms of the Mozilla Public

For the colors we're using, could we pick the semantic variables names instead of the raw colors?

For instance `theme-highlight-blue` or `theme-selection-background` (whatever fits best for the function this color is doing here) instead of `blue-55`, etc.
Attachment #8991851 - Flags: review?(balbeza) → review+
Comment on attachment 8991852 [details]
Bug 1471795 - Part 5: Implement basis of DebugTargetsPanel.

https://reviewboard.mozilla.org/r/256758/#review265010

::: devtools/client/aboutdebugging-new/src/components/App.css:12
(Diff revision 3)
>    margin: 0;
>    padding: 0;
>  }
>  
>  .app {
> +  display: flex;

Flex works, but the layout of the whole thing would be more manageable if we switched to CSS grid instead. I strongly suggest the move to Grid if you're OK with it :)
Attachment #8991852 - Flags: review?(balbeza) → review-
Comment on attachment 8991853 [details]
Bug 1471795 - Part 6: Implement RuntimeInformation.

https://reviewboard.mozilla.org/r/256760/#review265016

::: devtools/client/aboutdebugging-new/src/components/DebugTargetsPane.js:42
(Diff revision 3)
> +    this.updateRuntimeInfo(runtime);
> +  }
> +
> +  async updateRuntimeInfo(runtime) {
> +    const info = await runtime.getRuntimeInfo(runtime);
> +    this.setState({ info });

Question: is there a speficic reason we are using `setState` here instead of relying on actions/reducers?
Attachment #8991853 - Flags: review?(balbeza) → review+
Comment on attachment 8991854 [details]
Bug 1471795 - Part 7: Implement tab list.

https://reviewboard.mozilla.org/r/256762/#review265030

Hi! It's nice that we're switching to a BEM-like style, but I feel we are applying it wrong in some places. Some considerations:
- Let's not add class names that don't have any style rules associated in the CSS files. This is low-hang fruit that will make the codebase smaller and easier to maintain.
- We should think of styles in terms of re-usable components. So if we instantiate two –in theory "different"– components, if they share the same styling, they should share the same class names and css rules ([more info here](https://gist.github.com/belen-albeza/86a8df1ae2a6f7e21a65e17e0131dc3a#making-the-most-out-of-our-components))
- We shouldn't have class names that mimic or DOM structure (for instance, `debug-target-item__info__detail__tab-url`), unless needed ([further explanation here](https://gist.github.com/belen-albeza/86a8df1ae2a6f7e21a65e17e0131dc3a#naming-conventions))

I'm not sure if I managed to proper explain this, if you have any questions, please do ask :)

::: devtools/client/aboutdebugging-new/src/components/DebugTargetsPane.js:52
(Diff revision 3)
>    }
>  
> +  async updateTabs(runtime) {
> +    // Filter out closed tabs (represented as `null`).
> +    const tabs = (await runtime.getTabs()).filter(t => !!t);
> +    this.setState({ tabs });

Same question than in a previous patch: I'm a big confused on why we are using `setState` for some stuff, and Flux architecture for other stuff. I think I might be missing something, could you clarify? Thanks! :)

::: devtools/client/aboutdebugging-new/src/components/DebugTargetsPane.js:67
(Diff revision 3)
>        {
>          className: "debug-targets-pane",
>        },
> -      RuntimeInfo({ info })
> +      RuntimeInfo({ info }),
> +      DebugTargetList({
> +        className: "debug-target-list--tabs",

We're not using this class anywhere

::: devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetList.js:24
(Diff revision 3)
> +  }
> +
> +  render() {
> +    const { className, debugTargetItemComponent, debugTargets, title } = this.props;
> +
> +    return dom.div(

This should probably be a `<section>`

::: devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetList.js:30
(Diff revision 3)
> +      {
> +        className: `debug-target-list ${ className }`,
> +      },
> +      dom.h2(
> +        {
> +          className: `debug-target-list__title ${ className }__title`,

We're adding just redundant `<classname>__title` here, but this only makes sense if different targets list will have different styles for the title. Right now it is not the case, so maybe we could get rid off these (there are a bunch in the code)

::: devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetList.js:36
(Diff revision 3)
> +        },
> +        title
> +      ),
> +      dom.ul(
> +        {
> +          className: `debug-target-list__ul ${ className }__ul`,

same as above

::: devtools/client/aboutdebugging-new/src/components/debugtarget/TabItem.js:34
(Diff revision 3)
> +  renderDetailComponents() {
> +    const { debugTarget } = this.props;
> +
> +    return dom.div(
> +      {
> +        className: "debug-target-item__info__detail__tab-url",

We're not using this class
Attachment #8991854 - Flags: review?(balbeza) → review-
Comment on attachment 8991850 [details]
Bug 1471795 - Part 3: Implement runtimes pane.

https://reviewboard.mozilla.org/r/256754/#review264964

> This isn't preventing the sidebar to actually be shorter than this amount (try to shrink the window size and you'll see the glitch), so I'm asssuming something else is messing this up.

I could not reproduce this at least by this patch.
However, after applying patch 5, this reproduced.
Let me do in patch 5, thanks!

> It's more semantic –and easier to layout with CSS afterwards– if the `ul` is wrapped in a higher-level container, like `section` or `aside`.

Thanks, will use `section`.

> if we later add more items to this sidebar, it seems that this block of styles would apply to them as well. In this case, just `.runtime-item` might be a better class and this could be moved to the `RuntimeItem` component instead.

RuntimeItem intented to `This Firefox`, USB runtimes, WIFI runtimes and so on.
According to the mock[1], the view of WIFI runtime item (not "Connect a Device" menu) differs from `This Firefox`.

Ah, the composition style will be benefit for this case as well since we can combine the view and role respectively.
e.g.
MenuView x RuntimeItem = This Firefox item
MenuView x ActionItem = Connect a Device menu
WIFIView x RuntimeItem = WIFI runtime item
...

Let me implement this thinking in this patch, please!

[1] https://mozilla.invisionapp.com/share/F9IVW9WPG4W#/screens/297684870

> I'm not sure why the height needs to be manually set.  It's also easier to deal with in the long run if we don't set this manually and instead add padding –so if we later change the `font-size` or `font-family`, or if a text spans over multiple lines (because of localization) it won't cause a wreck in the layout

Indeed. Will drop.

> I have been told in another review to add a license header to a SVG file, so it might be a good idea to do it here as well :)

Thanks, I have added the license.
Comment on attachment 8991850 [details]
Bug 1471795 - Part 3: Implement runtimes pane.

https://reviewboard.mozilla.org/r/256754/#review264998

> `background-repeat` is being overriden here so this isn't taking effect and there's a glitch when the window is shrinked. (I'm not sure if this is caused by a later patch)

Let me do in patch 5.
Comment on attachment 8991851 [details]
Bug 1471795 - Part 4: Implement an action of when runtime was selected.

https://reviewboard.mozilla.org/r/256756/#review265000

> Question: what is this needed for?

Will drop.

> For the colors we're using, could we pick the semantic variables names instead of the raw colors?
> 
> For instance `theme-highlight-blue` or `theme-selection-background` (whatever fits best for the function this color is doing here) instead of `blue-55`, etc.

This is not DevTools panel. The variable will not be defined unless we set the theme class[1].
(I'm wondering if it might be better to re-define in another variable.css for about:debugging?)

[1] https://searchfox.org/mozilla-central/source/devtools/client/themes/variables.css#17
Comment on attachment 8991852 [details]
Bug 1471795 - Part 5: Implement basis of DebugTargetsPanel.

https://reviewboard.mozilla.org/r/256758/#review265010

> Flex works, but the layout of the whole thing would be more manageable if we switched to CSS grid instead. I strongly suggest the move to Grid if you're OK with it :)

I got it!
Status: NEW → ASSIGNED
Comment on attachment 8991853 [details]
Bug 1471795 - Part 6: Implement RuntimeInformation.

https://reviewboard.mozilla.org/r/256760/#review265016

> Question: is there a speficic reason we are using `setState` here instead of relying on actions/reducers?

Do you mean that extract this in action or reducer?
Actually, it looks this information can be extacted in action/reducer.
However, for example about tabs, we need to detect whether tabs were added or closed. We are listening in this component[1] though, I don't think this fits to action/reducer. For consistency of the way of getting something from runtime instance, I want to leave as it is.
What do you think?

[1] https://reviewboard.mozilla.org/r/256766/diff/3#index_header
Comment on attachment 8991854 [details]
Bug 1471795 - Part 7: Implement tab list.

https://reviewboard.mozilla.org/r/256762/#review265030

I want to change to composition style here as well.
I think, the CSS as well will be simple.
Please wait for a while.

> This should probably be a `<section>`

Thanks, will change.
Comment on attachment 8991853 [details]
Bug 1471795 - Part 6: Implement RuntimeInformation.

https://reviewboard.mozilla.org/r/256760/#review265348

::: devtools/client/aboutdebugging-new/src/components/RuntimeInfo.js:24
(Diff revision 3)
> +  }
> +
> +  render() {
> +    const { info } = this.props;
> +
> +    return dom.div(

This should probably be a `h1`. It's the "title"/heading of the panel, there's no other highger ranking heading, and in other parts there are `h2` around –which it's a bit weird when we don't have an `h1` above
Comment on attachment 8991853 [details]
Bug 1471795 - Part 6: Implement RuntimeInformation.

https://reviewboard.mozilla.org/r/256760/#review265016

> Do you mean that extract this in action or reducer?
> Actually, it looks this information can be extacted in action/reducer.
> However, for example about tabs, we need to detect whether tabs were added or closed. We are listening in this component[1] though, I don't think this fits to action/reducer. For consistency of the way of getting something from runtime instance, I want to leave as it is.
> What do you think?
> 
> [1] https://reviewboard.mozilla.org/r/256766/diff/3#index_header

I don't think the component should listen to tab list changes and I think I mentioned something similar in another review. I am not sure what is the best place to create those listeners, but having them in a component seems wrong. And a tab list change should dispatch an action that is then picked up by the component after the store is updated. 

However I advise to leave this as is until this lands and fix this immediately after in a follow up bug.
Comment on attachment 8991861 [details]
Bug 1471795 - Part 14: Add reload/delete button for temporary extension.

https://reviewboard.mozilla.org/r/256776/#review265364

::: devtools/client/aboutdebugging-new/src/components/debugtarget/ExtensionItem.js:31
(Diff revision 3)
> +      await runtime.sendRequest({
> +        to: debugTarget.actor,
> +        type: "reload"
> +      });

Not sure about introducing a generic "sendRequest" API here. Why not reloadExtension?
Attachment #8991861 - Flags: review?(jdescottes) → review+
I would like to propose to split this bug in several bugs. I initially wanted to land the whole series and then work on refactors for all the concerns raised. But now I think it will be easier for all of us to reach a consensus on the architecture if we can focus on smaller bugs, with a smaller scope.

My proposal would be to land part 1 to 4 here, and move all the rest to dedicated bugs:
- show runtime information (part 5 to 6)
- show tabs (part 7 to 9)
- show extensions (part 10 to 12)
- support installing/reloading/removing temporary extensions (part 13 to 14)
- show workers (part 15 to 17)
- add collapse feature (part 18)

The reason behind that is that we need to discuss patterns which are reused for tabs extensions and workers. It will be easier for us to discuss and get the "tabs" use case right if we only have tabs to discuss. And once we are happy with the way this UI is architected for tabs, we can move on to the rest.

I feel like it's better so that everyone can be involved and onboard with the architecture choices we will make for this UI.

Let me know what you think and thanks again for going through this huge task of creating the new UI :)
Flags: needinfo?(dakatsuka)
Thanks, I agree with you, Julian!
I file these bugs.
Flags: needinfo?(dakatsuka)
Attachment #8991852 - Attachment is obsolete: true
Attachment #8991853 - Attachment is obsolete: true
Attachment #8991854 - Attachment is obsolete: true
Attachment #8991855 - Attachment is obsolete: true
Attachment #8991855 - Flags: review?(balbeza)
Attachment #8991856 - Attachment is obsolete: true
Attachment #8991857 - Attachment is obsolete: true
Attachment #8991857 - Flags: review?(balbeza)
Attachment #8991858 - Attachment is obsolete: true
Attachment #8991859 - Attachment is obsolete: true
Attachment #8991860 - Attachment is obsolete: true
Attachment #8991860 - Flags: review?(jdescottes)
Attachment #8991860 - Flags: review?(balbeza)
Attachment #8991861 - Attachment is obsolete: true
Attachment #8991862 - Attachment is obsolete: true
Attachment #8991862 - Flags: review?(jdescottes)
Attachment #8991862 - Flags: review?(balbeza)
Attachment #8991863 - Attachment is obsolete: true
Attachment #8991863 - Flags: review?(jdescottes)
Attachment #8991863 - Flags: review?(balbeza)
Attachment #8991864 - Attachment is obsolete: true
Attachment #8991864 - Flags: review?(jdescottes)
Attachment #8992557 - Attachment is obsolete: true
Attachment #8992557 - Flags: review?(jdescottes)
Attachment #8992557 - Flags: review?(balbeza)
Julian, Belén, 
I have updated the patch 1 - 4 as well to introduce the composition style.
Could you re-review again, especially patch 3?

https://reviewboard.mozilla.org/r/256754/diff/3-4/
Flags: needinfo?(jdescottes)
Flags: needinfo?(balbeza)
Blocks: 1477596
Blocks: 1477597
No longer blocks: 1477597
I filed as followings:

* Bug 1477596 - Show runtime information
* Bug 1477597 - Show/update/debug tabs
* Bug 1477598 - Show/update/debug extensions
* Bug 1477600 - Support installing/reloading/removing temporary extensions
* Bug 1477602 - Show/update/debug workers
* Bug 1477603 - Implement expanding/collapsing feature for tabs/extensions/workers
Flags: needinfo?(jdescottes)
Attachment #8991848 - Flags: review+ → review?(jdescottes)
Attachment #8991849 - Flags: review+ → review?(jdescottes)
Attachment #8991850 - Flags: review+ → review?(jdescottes)
Attachment #8991851 - Flags: review+ → review?(jdescottes)
Attachment #8991848 - Flags: review?(jdescottes) → review+
Attachment #8991849 - Flags: review?(jdescottes) → review+
Comment on attachment 8991850 [details]
Bug 1471795 - Part 3: Implement runtimes pane.

https://reviewboard.mozilla.org/r/256754/#review265996

::: devtools/client/aboutdebugging-new/src/components/RuntimesPane.css:6
(Diff revision 5)
> +/* 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/. */
> +
> +.runtimes-pane {
> +  margin-block-start: 70px;

This margin leaks out of the containing block and forces an overflow on the `.app` element. If `.app` created a block formatting context, we wouldn't have this behavior.

::: devtools/client/aboutdebugging-new/src/components/RuntimesPane.js:34
(Diff revision 5)
> +      },
> +      dom.ul(
> +        {},
> +        RuntimeItem({
> +          runtime: thisFirefox,
> +          view: MenuView({

Why pass the component as a prop here? 

For instance if the icon and text can be retrieved from the runtime instance, we can merge MenuView in RuntimeItem. The render of RuntimeItem would look like

```
render() {
  const { runtime } = this.props;
  const icon = runtime.getIcon();
  const text = runtime.getText();

  return dom.li(
    { className: "runtime-item" },
    dom.div(
      { className: "menu-view" },
      dom.img({ 
        className: "menu-view__icon", 
        src: icon 
      }),
      text
    )
  );
}
```

But I am not sure we would still need to separate the <li> and the <div>. Maybe only one container element could be enough.

::: devtools/client/aboutdebugging-new/src/components/RuntimesPane.js:35
(Diff revision 5)
> +            icon: "chrome://devtools/skin/images/firefox-logo-glyph.svg",
> +            text: "This Firefox",

It might make sense to get this information from the runtime?

::: devtools/client/themes/images/firefox-logo-glyph.svg:4
(Diff revision 5)
> +<!-- 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/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32"><title>newtab-firefox-gry</title><path d="M31.359,14.615h0c-.044-.289-.088-.459-.088-.459s-.113.131-.3.378A10.77,10.77,0,0,0,30.6,12.5a13.846,13.846,0,0,0-.937-2.411,10.048,10.048,0,0,0-.856-1.468q-.176-.263-.359-.51c-.57-.931-1.224-1.5-1.981-2.576a7.806,7.806,0,0,1-.991-2.685A10.844,10.844,0,0,0,25,4.607c-.777-.784-1.453-1.341-1.861-1.721C21.126,1.006,21.36.031,21.36.031h0S17.6,4.228,19.229,8.6a8.4,8.4,0,0,0,2.8,3.733c1.576,1.3,3.273,2.323,4.168,4.937a8.377,8.377,0,0,0-3.144-3.317,7.573,7.573,0,0,1,.6,3,7.124,7.124,0,0,1-8.711,6.94,6.561,6.561,0,0,1-1.765-.6,7.183,7.183,0,0,1-2.115-1.955l-.01-.017.126.046a6.5,6.5,0,0,0,.9.241,5.628,5.628,0,0,0,3.583-.423c1.126-.625,1.808-1.088,2.361-.905l.01,0c.54.172.966-.352.58-.9a2.94,2.94,0,0,0-2.848-1.112c-1.127.164-2.16.965-3.637.189a3.129,3.129,0,0,1-.277-.163c-.1-.057.317.087.22.022a7.33,7.33,0,0,1-.928-.554c-.022-.018.223.07.2.052a3.581,3.581,0,0,1-.968-.979,1.741,1.741,0,0,1-.066-1.554,1.371,1.371,0,0,1,.6-.564c.191.094.309.165.309.165s-.087-.16-.134-.244c.017-.006.032,0,.049-.011.167.072.537.26.732.375a1.016,1.016,0,0,1,.335.3s.067-.033.017-.173a.9.9,0,0,0-.346-.424l.016,0a2.94,2.94,0,0,1,.426.265,2.079,2.079,0,0,0,.17-.9,1.178,1.178,0,0,0-.069-.5c-.053-.1.03-.14.123-.035a.976.976,0,0,0-.079-.238v-.008h0s.053-.069.077-.094a1.43,1.43,0,0,1,.216-.176,9.973,9.973,0,0,1,1.465-.747c.414-.181.757-.319.827-.359a2.3,2.3,0,0,0,.293-.225,1.968,1.968,0,0,0,.66-1.14,1.6,1.6,0,0,0,.017-.178v-.05l0-.03v0l0-.012v0l0-.013h0c-.06-.225-.448-.394-2.476-.584a1.773,1.773,0,0,1-1.45-1.36l0,.009c-.029.074-.055.149-.081.225.026-.075.052-.15.081-.225l0-.016a5.138,5.138,0,0,1,1.986-2.466c.052-.042-.208.011-.156-.032a5.156,5.156,0,0,1,.53-.224c.091-.038-.39-.222-.815-.177a2.2,2.2,0,0,0-.756.178c.1-.086.4-.2.329-.2a4.865,4.865,0,0,0-1.542.583.314.314,0,0,1,.03-.14,2.4,2.4,0,0,0-.964.744,1.275,1.275,0,0,0,.01-.174,2.876,2.876,0,0,0-.473.444l-.009.007a6.285,6.285,0,0,0-3.517-.3l-.01-.009.012,0a2.943,2.943,0,0,1-.625-.7L6.1,5.852,6.081,5.83c-.077-.114-.156-.243-.237-.387-.058-.1-.117-.217-.176-.338,0-.008-.009-.011-.013-.012-.024,0-.041.111-.061.082l0-.006a4.308,4.308,0,0,1-.283-1.687l-.016.008a1.884,1.884,0,0,0-.714.934c-.061.137-.1.212-.14.287,0,.006,0-.01,0-.035.009-.069.039-.211.032-.2s-.012.019-.019.029a1.733,1.733,0,0,0-.251.372,2.355,2.355,0,0,0-.15.382c-.006.021,0-.018,0-.064s.009-.128,0-.111l-.022.043a9.5,9.5,0,0,0-.8,3.035A3.022,3.022,0,0,0,3.2,8.7v.016a6.628,6.628,0,0,0-.817,1.1,15.606,15.606,0,0,0-1.727,4.23,10.351,10.351,0,0,1,.925-1.621,15,15,0,0,0-1.045,5.5,14.233,14.233,0,0,1,.45-1.629A13.807,13.807,0,0,0,2.234,22.76a15.037,15.037,0,0,0,5.951,6.748h0a13.016,13.016,0,0,0,3.468,1.662c.162.059.326.117.494.173-.053-.021-.1-.044-.153-.067a15.7,15.7,0,0,0,4.5.662c5.394,0,7.175-2.054,7.339-2.259h0a2.73,2.73,0,0,0,.637-.856h0q.156-.064.315-.137l.067-.03.121-.057a11.312,11.312,0,0,0,2.277-1.426,5.5,5.5,0,0,0,2.123-3.1h0a1.938,1.938,0,0,0,.029-1.428q.083-.131.171-.28a12.706,12.706,0,0,0,1.907-6.181v-.006c0-.059,0-.118,0-.177A7.731,7.731,0,0,0,31.359,14.615Z" fill="#d7d7db"/></svg>

Can we remove the title?
Attachment #8991850 - Flags: review?(jdescottes) → review+
Comment on attachment 8991849 [details]
Bug 1471795 - Part 2: Introduce Runtime class.

https://reviewboard.mozilla.org/r/256752/#review265994

::: devtools/client/aboutdebugging-new/aboutdebugging.js:20
(Diff revision 4)
>  const { createFactory } =
>    require("devtools/client/shared/vendor/react");
>  const { render, unmountComponentAtNode } =
>    require("devtools/client/shared/vendor/react-dom");
>  
> +const ThisFirefox = require("./src/runtimes/this-firefox");

I am wondering if we should keep this name or use something more generic, such as "CurrentRuntime" (and current-runtime.js etc...)
Comment on attachment 8991851 [details]
Bug 1471795 - Part 4: Implement an action of when runtime was selected.

https://reviewboard.mozilla.org/r/256756/#review266000

::: devtools/client/themes/images/firefox-logo-glyph.svg:5
(Diff revision 5)
>  <!-- 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/. -->
> -<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32"><title>newtab-firefox-gry</title><path d="M31.359,14.615h0c-.044-.289-.088-.459-.088-.459s-.113.131-.3.378A10.77,10.77,0,0,0,30.6,12.5a13.846,13.846,0,0,0-.937-2.411,10.048,10.048,0,0,0-.856-1.468q-.176-.263-.359-.51c-.57-.931-1.224-1.5-1.981-2.576a7.806,7.806,0,0,1-.991-2.685A10.844,10.844,0,0,0,25,4.607c-.777-.784-1.453-1.341-1.861-1.721C21.126,1.006,21.36.031,21.36.031h0S17.6,4.228,19.229,8.6a8.4,8.4,0,0,0,2.8,3.733c1.576,1.3,3.273,2.323,4.168,4.937a8.377,8.377,0,0,0-3.144-3.317,7.573,7.573,0,0,1,.6,3,7.124,7.124,0,0,1-8.711,6.94,6.561,6.561,0,0,1-1.765-.6,7.183,7.183,0,0,1-2.115-1.955l-.01-.017.126.046a6.5,6.5,0,0,0,.9.241,5.628,5.628,0,0,0,3.583-.423c1.126-.625,1.808-1.088,2.361-.905l.01,0c.54.172.966-.352.58-.9a2.94,2.94,0,0,0-2.848-1.112c-1.127.164-2.16.965-3.637.189a3.129,3.129,0,0,1-.277-.163c-.1-.057.317.087.22.022a7.33,7.33,0,0,1-.928-.554c-.022-.018.223.07.2.052a3.581,3.581,0,0,1-.968-.979,1.741,1.741,0,0,1-.066-1.554,1.371,1.371,0,0,1,.6-.564c.191.094.309.165.309.165s-.087-.16-.134-.244c.017-.006.032,0,.049-.011.167.072.537.26.732.375a1.016,1.016,0,0,1,.335.3s.067-.033.017-.173a.9.9,0,0,0-.346-.424l.016,0a2.94,2.94,0,0,1,.426.265,2.079,2.079,0,0,0,.17-.9,1.178,1.178,0,0,0-.069-.5c-.053-.1.03-.14.123-.035a.976.976,0,0,0-.079-.238v-.008h0s.053-.069.077-.094a1.43,1.43,0,0,1,.216-.176,9.973,9.973,0,0,1,1.465-.747c.414-.181.757-.319.827-.359a2.3,2.3,0,0,0,.293-.225,1.968,1.968,0,0,0,.66-1.14,1.6,1.6,0,0,0,.017-.178v-.05l0-.03v0l0-.012v0l0-.013h0c-.06-.225-.448-.394-2.476-.584a1.773,1.773,0,0,1-1.45-1.36l0,.009c-.029.074-.055.149-.081.225.026-.075.052-.15.081-.225l0-.016a5.138,5.138,0,0,1,1.986-2.466c.052-.042-.208.011-.156-.032a5.156,5.156,0,0,1,.53-.224c.091-.038-.39-.222-.815-.177a2.2,2.2,0,0,0-.756.178c.1-.086.4-.2.329-.2a4.865,4.865,0,0,0-1.542.583.314.314,0,0,1,.03-.14,2.4,2.4,0,0,0-.964.744,1.275,1.275,0,0,0,.01-.174,2.876,2.876,0,0,0-.473.444l-.009.007a6.285,6.285,0,0,0-3.517-.3l-.01-.009.012,0a2.943,2.943,0,0,1-.625-.7L6.1,5.852,6.081,5.83c-.077-.114-.156-.243-.237-.387-.058-.1-.117-.217-.176-.338,0-.008-.009-.011-.013-.012-.024,0-.041.111-.061.082l0-.006a4.308,4.308,0,0,1-.283-1.687l-.016.008a1.884,1.884,0,0,0-.714.934c-.061.137-.1.212-.14.287,0,.006,0-.01,0-.035.009-.069.039-.211.032-.2s-.012.019-.019.029a1.733,1.733,0,0,0-.251.372,2.355,2.355,0,0,0-.15.382c-.006.021,0-.018,0-.064s.009-.128,0-.111l-.022.043a9.5,9.5,0,0,0-.8,3.035A3.022,3.022,0,0,0,3.2,8.7v.016a6.628,6.628,0,0,0-.817,1.1,15.606,15.606,0,0,0-1.727,4.23,10.351,10.351,0,0,1,.925-1.621,15,15,0,0,0-1.045,5.5,14.233,14.233,0,0,1,.45-1.629A13.807,13.807,0,0,0,2.234,22.76a15.037,15.037,0,0,0,5.951,6.748h0a13.016,13.016,0,0,0,3.468,1.662c.162.059.326.117.494.173-.053-.021-.1-.044-.153-.067a15.7,15.7,0,0,0,4.5.662c5.394,0,7.175-2.054,7.339-2.259h0a2.73,2.73,0,0,0,.637-.856h0q.156-.064.315-.137l.067-.03.121-.057a11.312,11.312,0,0,0,2.277-1.426,5.5,5.5,0,0,0,2.123-3.1h0a1.938,1.938,0,0,0,.029-1.428q.083-.131.171-.28a12.706,12.706,0,0,0,1.907-6.181v-.006c0-.059,0-.118,0-.177A7.731,7.731,0,0,0,31.359,14.615Z" fill="#d7d7db"/></svg>
> +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32">
> +  <path fill="context-fill"

Ah maybe we can fold this change in the previous changeset to have a cleaner history?
Attachment #8991851 - Flags: review?(jdescottes) → review+
Comment on attachment 8991850 [details]
Bug 1471795 - Part 3: Implement runtimes pane.

https://reviewboard.mozilla.org/r/256754/#review265996

> Why pass the component as a prop here? 
> 
> For instance if the icon and text can be retrieved from the runtime instance, we can merge MenuView in RuntimeItem. The render of RuntimeItem would look like
> 
> ```
> render() {
>   const { runtime } = this.props;
>   const icon = runtime.getIcon();
>   const text = runtime.getText();
> 
>   return dom.li(
>     { className: "runtime-item" },
>     dom.div(
>       { className: "menu-view" },
>       dom.img({ 
>         className: "menu-view__icon", 
>         src: icon 
>       }),
>       text
>     )
>   );
> }
> ```
> 
> But I am not sure we would still need to separate the <li> and the <div>. Maybe only one container element could be enough.

I have thought that I wanted to use RuntimeItem for WIFI and USB runtime as well. Because, according to the mock, the UI of WIFI item is very different from "This Firefox".  So, I had thought that if we will introduce this mechanism, replacement the view easily.

I discussed with Julian on slack though, we start to think about this when we implement WIFI and USB runtimes.
As the result, for now, I'll fix according to Julian's suggestion.

> It might make sense to get this information from the runtime?

Yep, indeed!

> Can we remove the title?

Okay.
Comment on attachment 8991849 [details]
Bug 1471795 - Part 2: Introduce Runtime class.

https://reviewboard.mozilla.org/r/256752/#review265994

> I am wondering if we should keep this name or use something more generic, such as "CurrentRuntime" (and current-runtime.js etc...)

I discussed with Julian on slack, for now, we will leave this as it is.
(In reply to Daisuke Akatsuka (:daisuke) from comment #119)
> Comment on attachment 8991849 [details]
> Bug 1471795 - Part 2: Introduce Runtime class.
> 
> https://reviewboard.mozilla.org/r/256752/#review265994
> 
> > I am wondering if we should keep this name or use something more generic, such as "CurrentRuntime" (and current-runtime.js etc...)
> 
> I discussed with Julian on slack, for now, we will leave this as it is.

Just to give the rest of the context: I am afraid that "CurrentRuntime" would be confusing with the "Selected" runtime. And my main concern is about using the non-generic term "Firefox" here. But I don't have a good idea to suggest at the moment :)
Comment on attachment 8991850 [details]
Bug 1471795 - Part 3: Implement runtimes pane.

https://reviewboard.mozilla.org/r/256754/#review266378

Hi, thanks! I have some issues with the choice of inheritance, when just a base component would suffice. See comment below. Cheers! :)

::: devtools/client/aboutdebugging-new/src/components/App.css:5
(Diff revision 6)
>  /* 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/. */
>  
> +ul {

I'm not sure setting this here is a good idea. If we'd like a global "reset" of styles, maybe we could have a reset.css stylesheet and put all the reset code there. That way it's easier to locate :)

Or if it's just `ul`, maybe we could set this style to the lists that do need them.

::: devtools/client/aboutdebugging-new/src/runtimes/runtime.js:10
(Diff revision 6)
>  "use strict";
>  
>  /**
>   * This class represents a runtime, such as a remote Firefox.
>   */
>  class Runtime {

I don't think that we need to create subclasses for every item, but use props / slots to change the icons and text. In JSX-ish it would be:

```
<RuntimePane>
  <RuntimeItem icon="firefox-logo-glyph.svg">This Firefox</RuntimeItem>
  <RuntimeItem icon="another.svg">Another item</RuntimeItem>
</RuntimePane>
```
Attachment #8991850 - Flags: review+ → review-
Comment on attachment 8991851 [details]
Bug 1471795 - Part 4: Implement an action of when runtime was selected.

https://reviewboard.mozilla.org/r/256756/#review266382

::: devtools/client/aboutdebugging-new/src/components/runtime/RuntimeItem.css:13
(Diff revision 6)
>    font-size: 20px;
>    margin-inline-start: 24px;
>  }
>  
>  .runtime-item__icon {
> +  fill: var(--grey-30);

Could we create our own color names that have some semantic value based on `--grey-30`, `--blue-55`, etc.?
Comment on attachment 8991850 [details]
Bug 1471795 - Part 3: Implement runtimes pane.

https://reviewboard.mozilla.org/r/256754/#review266378

> I'm not sure setting this here is a good idea. If we'd like a global "reset" of styles, maybe we could have a reset.css stylesheet and put all the reset code there. That way it's easier to locate :)
> 
> Or if it's just `ul`, maybe we could set this style to the lists that do need them.

I wrote this while referencing to your code in apllication panel though, similar codes are in App.css[1]. 
Actually I'd like to write as following, because I wanted to affect only the <ul> inside our App.
```
.app ul {
}
```
However, is it ok for BENT style?

[1] https://searchfox.org/mozilla-central/source/devtools/client/application/src/components/App.css

> I don't think that we need to create subclasses for every item, but use props / slots to change the icons and text. In JSX-ish it would be:
> 
> ```
> <RuntimePane>
>   <RuntimeItem icon="firefox-logo-glyph.svg">This Firefox</RuntimeItem>
>   <RuntimeItem icon="another.svg">Another item</RuntimeItem>
> </RuntimePane>
> ```

This is not UI component class.
Also, I made RuntimeItem to be like above[1].

[1] Please see RuntimeItem in https://reviewboard.mozilla.org/r/256754/diff/5-6/
Comment on attachment 8991851 [details]
Bug 1471795 - Part 4: Implement an action of when runtime was selected.

https://reviewboard.mozilla.org/r/256756/#review266382

> Could we create our own color names that have some semantic value based on `--grey-30`, `--blue-55`, etc.?

For now, I don't feel that we need re-defining since the number of place where refers the color is a few, but okay, I'll define in App.css
Comment on attachment 8991850 [details]
Bug 1471795 - Part 3: Implement runtimes pane.

https://reviewboard.mozilla.org/r/256754/#review266378

> I wrote this while referencing to your code in apllication panel though, similar codes are in App.css[1]. 
> Actually I'd like to write as following, because I wanted to affect only the <ul> inside our App.
> ```
> .app ul {
> }
> ```
> However, is it ok for BENT style?
> 
> [1] https://searchfox.org/mozilla-central/source/devtools/client/application/src/components/App.css

I have dicussed with Belén.
As the result, we move this to aboutdebugging.css.
Also, will move css variables as well that I wrote in patch 4.
Comment on attachment 8991850 [details]
Bug 1471795 - Part 3: Implement runtimes pane.

https://reviewboard.mozilla.org/r/256754/#review266606
Attachment #8991850 - Flags: review?(balbeza) → review+
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1baabb560d94
Part 1: Implement basis of 'This Firefox' page. r=jdescottes,ladybenko
https://hg.mozilla.org/integration/autoland/rev/aaf16863c12e
Part 2: Introduce Runtime class. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/8ff2667759fd
Part 3: Implement runtimes pane. r=jdescottes,ladybenko
https://hg.mozilla.org/integration/autoland/rev/96cdfc3d1046
Part 4: Implement an action of when runtime was selected. r=jdescottes,ladybenko
\o/ Thanks everyone for landing the first piece of the new remote debugging experience!
Flags: needinfo?(balbeza)
Blocks: 1446574
No longer blocks: remote-debugging-ng-m0
You need to log in before you can comment on or make changes to this bug.