Closed Bug 912447 Opened 6 years ago Closed 6 years ago

[app manager] land the app manager front end

Categories

(DevTools :: WebIDE, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: paul, Assigned: paul)

References

Details

(Keywords: dev-doc-complete)

Attachments

(20 files, 2 obsolete files)

2.73 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
1.52 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
2.89 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
4.66 KB, patch
miker
: review+
Details | Diff | Splinter Review
12.99 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
17.75 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
13.00 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
27.16 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
1.82 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
17.02 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
2.31 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
27.52 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
896 bytes, patch
ochameau
: review+
Details | Diff | Splinter Review
1.51 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
1.73 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
2.00 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
2.64 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
5.18 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
15.37 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
14.19 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch Patch C: appmgr_jars.diff (obsolete) — Splinter Review
Attached patch Patch F: appmgr_utils.diff (obsolete) — Splinter Review
Attachment #799438 - Flags: review?(poirot.alex)
Attachment #799434 - Flags: review?(mratcliffe)
Comment on attachment 799430 [details] [diff] [review]
Patch B: appmgr_wallpaper.diff

This will fail with Firefox Desktop and Fennec. In a follow up, we will return the persona.
Attachment #799430 - Flags: review?(poirot.alex)
Attachment #799428 - Flags: review?(poirot.alex)
Attachment #799439 - Flags: review?(poirot.alex)
More patches to come.

Todo:
- make sure all the files have the license header
- file a bug for persona support
- tests (probably follow up)
Comment on attachment 799428 [details] [diff] [review]
Patch A: appmgr_logs.diff

Review of attachment 799428 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/client/connection-manager.js
@@ +217,5 @@
>    },
>  
>    _clientConnect: function () {
>      let transport;
>      if (!this._host) {

While you are at switching from this._host to this.host,
I think you can also change this one.
Attachment #799428 - Flags: review?(poirot.alex) → review+
Attachment #799434 - Flags: review?(mratcliffe) → review+
Comment on attachment 799430 [details] [diff] [review]
Patch B: appmgr_wallpaper.diff

Review of attachment 799430 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/device.js
@@ +148,5 @@
> +  getWallpaper: method(function() {
> +    let deferred = promise.defer();
> +    this._getSetting("wallpaper.image").then((blob) => {
> +      let win = Services.wm.getMostRecentWindow(DebuggerServer.chromeWindowType);
> +      let reader = new win.FileReader();

Please try to avoid depending on DOM windows unless it is explicitely needed.
  let CC = Components.Constructor;
  let FileReader = CC("@mozilla.org/files/filereader;1");
  let reader = FileReader();

@@ +151,5 @@
> +      let win = Services.wm.getMostRecentWindow(DebuggerServer.chromeWindowType);
> +      let reader = new win.FileReader();
> +      let conn = this.conn;
> +      reader.addEventListener("loadend", function() {
> +        let str = new LongStringActor(conn, reader.result);

`loadend` fires also in case of error with a reader.result being null.
LongStringActor is most likely going to throw in such scenario.
That would be cool to resolve the promise with an error in this case.
Attachment #799430 - Flags: review?(poirot.alex) → review+
Comment on attachment 799438 [details] [diff] [review]
Patch G: appmgr_connection_footer.diff

Review of attachment 799438 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/app-manager/content/connection-footer.js
@@ +10,5 @@
> +const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +const {require} = devtools;
> +
> +const {ConnectionManager, Connection} = require("devtools/client/connection-manager");
> +const EventEmitter = require("devtools/shared/event-emitter");

EventEmitter isn't used.

@@ +70,5 @@
> +      let display = computedStyle.display; // Save display value
> +      document.documentElement.style.display = "none";
> +      window.getComputedStyle(document.documentElement).display; // Flush
> +      document.documentElement.style.display = display; // Restore
> +    }

Please add a comment about this trick.

::: browser/devtools/app-manager/content/connection-footer.xhtml
@@ +40,5 @@
> +              <button class="right" onclick="UI.editConnectionParameters()">&connection.changeHostAndPort;</button>
> +              <div id="start-simulator-box" template='{"type":"attribute","path":"simulators.versions.length","name":"simulators-count"}'>
> +                <span>&connection.or;</span>
> +                <button id="start-simulator-button" class="action-primary" onclick="UI.startSimulator()">&connection.startSimulator;</button>
> +              </div>

Shouldn't we remove simulator UI, or add the related JS code in connection-footer.js?
Attachment #799438 - Flags: review?(poirot.alex) → review+
Attachment #799509 - Attachment description: appmgr_projects.diff → Patch J: appmgr_projects.diff
These are all the patches I'd like to land in this bug (A -> I).
Attachment #799431 - Flags: review?(poirot.alex)
Attachment #799433 - Flags: review?(poirot.alex)
Attachment #799435 - Attachment is obsolete: true
Attachment #799513 - Flags: review?(poirot.alex)
Forgot the images. I like images.
Attachment #799517 - Flags: review?(poirot.alex)
Duplicate of this bug: 898487
Duplicate of this bug: 900447
Comment on attachment 799439 [details] [diff] [review]
Patch H: appmgr_device.diff

Review of attachment 799439 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/app-manager/content/device.js
@@ +10,5 @@
> +const {require} = devtools;
> +
> +const {ConnectionManager, Connection} = require("devtools/client/connection-manager");
> +const {getDeviceFront} = require("devtools/server/actors/device");
> +const EventEmitter = require("devtools/shared/event-emitter");

EventEmitter isn't used.

@@ +19,5 @@
> +window.addEventListener("message", function(event) {
> +  try {
> +    let message = JSON.parse(event.data);
> +    if (message.name == "connection") {
> +      let cid = +message.cid;

`+`?

::: browser/themes/shared/devtools/app-manager/device.css
@@ +20,5 @@
> +}
> +
> +[hidden] {
> +  display: none!important;
> +}

Isn't it already set by default?

@@ +87,5 @@
> +}
> +
> +.app[running="false"] > .app-buttons > .button-start,
> +.app[running="true"] > .app-buttons > .button-stop,
> +.app[running="true"] > .app-buttons > .button-restart,

There is not .button-restart in html file.

@@ +101,5 @@
> +  background-color: #3498DB;
> +  color: #FFF;
> +}
> +
> +.button-install,

Nor any .button-install

@@ +340,5 @@
> +}
> +
> +#notConnectedMessage {
> +  margin: 50px auto;
> +}

Can we merge these two rules?
Attachment #799439 - Flags: review?(poirot.alex) → review+
Comment on attachment 799513 [details] [diff] [review]
Patch F: appmgr_utils.diff

Review of attachment 799513 [details] [diff] [review]:
-----------------------------------------------------------------

The ObvservableObject story looks worse and worse, we should followup on this and ensure that performances are fine and we do not overuse memory, nor leak.

::: browser/devtools/app-manager/content/utils.js
@@ +27,5 @@
> +          finalStore.object[key] = stores[key].object,
> +          stores[key].on("set", function(event, path, value) {
> +            finalStore.emit("set", [key].concat(path), value);
> +          });
> +        })(key);

nit: I tend to find it more readable to avoid using such anonymous function, and extract the code out of the for loop in a named function.
That would do something like this:
  for(let key in stores) {
    finalStore.object[key] = ...;
    forwardSetEvent(key, stores[key], finalStore);
  }

@@ +29,5 @@
> +            finalStore.emit("set", [key].concat(path), value);
> +          });
> +        })(key);
> +      }
> +      return finalStore;

The indentation looks wrong here
Attachment #799513 - Flags: review?(poirot.alex) → review+
Duplicate of this bug: 894352
Comment on attachment 799517 [details] [diff] [review]
Patch J: appmgr_imgs.diff

Review of attachment 799517 [details] [diff] [review]:
-----------------------------------------------------------------

We miss noise.png
Attachment #799517 - Flags: review?(poirot.alex) → review-
Blocks: 894354
Depends on: 911781
Todo:
- make sure all the files have the license header
- file a bug for persona support
- tests (probably follow up)
- clean up the SVG files
Attachment #799509 - Flags: review?(poirot.alex)
Attachment #799508 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot (:ochameau) from comment #24)
> Comment on attachment 799517 [details] [diff] [review]
> Patch J: appmgr_imgs.diff
> 
> Review of attachment 799517 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We miss noise.png

I don't think so. Did you use splinter review? Look at the patch itself.
Comment on attachment 799508 [details] [diff] [review]
Patch I: appmgr_index.diff

Review of attachment 799508 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/app-manager/content/index.xul
@@ +27,5 @@
> +        <button class="button device-button" onclick="selectTab('device')">&index.device;</button>
> +      </vbox>
> +      <hbox id="tab-panels" flex="1">
> +        <iframe flex="1" class="panel projects-panel" src="chrome://browser/content/devtools/app-manager/projects.xhtml#hideFooter"/>
> +        <iframe flex="1" class="panel device-panel" src="chrome://browser/content/devtools/app-manager/device.xhtml#hideFooter"/>

Can we get rid of hideFooter and never try to display the connection footer in any panel document?
Or is it any usefull for futur developments?

::: browser/themes/shared/devtools/app-manager/index.css
@@ +24,5 @@
> +  background-color: transparent;
> +  color: white;
> +  cursor: pointer;
> +  text-align: center;
> +  -moz-box-align: end;  

There is a trailing space here.

@@ +64,5 @@
> +  background-repeat: no-repeat, no-repeat;
> +  background-size: 160px 160px, 2px 80px;
> +}
> +
> +

Unecessary newline here.
Attachment #799508 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot (:ochameau) from comment #13)
> Shouldn't we remove simulator UI, or add the related JS code in
> connection-footer.js?

This will happen later.
(In reply to Alexandre Poirot (:ochameau) from comment #21)
> @@ +19,5 @@
> > +window.addEventListener("message", function(event) {
> > +  try {
> > +    let message = JSON.parse(event.data);
> > +    if (message.name == "connection") {
> > +      let cid = +message.cid;
> 
> `+`?

I want a number. I could use parseInt().

> ::: browser/themes/shared/devtools/app-manager/device.css
> @@ +20,5 @@
> > +}
> > +
> > +[hidden] {
> > +  display: none!important;
> > +}
> 
> Isn't it already set by default?

Not in html.
(In reply to Alexandre Poirot (:ochameau) from comment #27)
> ::: browser/devtools/app-manager/content/index.xul
> @@ +27,5 @@
> > +        <button class="button device-button" onclick="selectTab('device')">&index.device;</button>
> > +      </vbox>
> > +      <hbox id="tab-panels" flex="1">
> > +        <iframe flex="1" class="panel projects-panel" src="chrome://browser/content/devtools/app-manager/projects.xhtml#hideFooter"/>
> > +        <iframe flex="1" class="panel device-panel" src="chrome://browser/content/devtools/app-manager/device.xhtml#hideFooter"/>
> 
> Can we get rid of hideFooter and never try to display the connection footer
> in any panel document?
> Or is it any usefull for futur developments?

I think we should be able to use the device inspector without the app manager.
In my next patch, I'll propose a better implementation of this.
Comment on attachment 799509 [details] [diff] [review]
Patch J: appmgr_projects.diff

Review of attachment 799509 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/app-manager/content/projects.js
@@ +19,5 @@
> +window.addEventListener("message", function(event) {
> +  try {
> +    let json = JSON.parse(event.data);
> +    if (json.name == "connection") {
> +      let cid = +json.cid;

`+` again?
If that another cast trick, please use a long explicit way of casting.

@@ +62,5 @@
> +  },
> +
> +  _selectFolder: function() {
> +    let fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
> +    fp.init(window, "Select a webapp folder", Ci.nsIFilePicker.modeGetFolder);

I'd imagine we want to make this string localizable

@@ +76,5 @@
> +      return;
> +    AppProjects.addPackaged(folder)
> +               .then(function (project) {
> +                 UI.validate(project);
> +                 UI.select(project.location);

UI.select -> UI.selectProject

@@ +85,5 @@
> +    let urlInput = document.querySelector("#url-input");
> +    let manifestURL = urlInput.value;
> +    AppProjects.addHosted(manifestURL)
> +               .then(function (project) {
> +                 UI.validate(project);

We should also call `UI.selectProject(project.location)`

@@ +118,5 @@
> +                if (validation.manifest) {
> +                  project.name = validation.manifest.name;
> +                  project.icon = UI._getLocalIconURL(project, validation.manifest);
> +                  project.manifest = validation.manifest;
> +                  project.manifestAsString = JSON.stringify(validation.manifest);

Can we remove manifest and manifestAsString until we start using them?

@@ +125,5 @@
> +                project.validationStatus = "valid";
> +
> +                if (validation.errors.length > 0) {
> +                  project.errorsCount = validation.errors.length;
> +                  project.errors = validation.errors.join(",\n ");

We should improve this. \n won't have expected effect as we inject errors and warnings as textContent.
Each error/warning should be displayed on a new line.
But we can do that in a followup.

@@ +128,5 @@
> +                  project.errorsCount = validation.errors.length;
> +                  project.errors = validation.errors.join(",\n ");
> +                  project.validationStatus = "error";
> +                } else {
> +                  project.errors = "";

We should also reset errorsCount and warningsCount to 0

@@ +129,5 @@
> +                  project.errors = validation.errors.join(",\n ");
> +                  project.validationStatus = "error";
> +                } else {
> +                  project.errors = "";
> +                }

Please move error after warning,
so that validationStatus is set to error if we have both error and warnings

@@ +183,5 @@
> +
> +    });
> +  },
> +
> +  _getTargetForApp: function(manifest) { // FIXME <- already in device.js

What about refering to bug 912476 in the meantime?
I'd expect to expose and share such helper in a front for webapps actor.

@@ +391,5 @@
> +              button.disabled = false;
> +            });
> +          });
> +    });
> +  },

Please remove all code related to install and the install button.
It is going to land via bug 911785, when dependencies are landed.

@@ +417,5 @@
> +      // Not found
> +      return;
> +    }
> +
> +    let oldButton = document.querySelector("#project-list .selected");

We may introduced nested elements with selected class,
we should be more precise with this selector.
For example: #project-list .project-item.selected

::: browser/themes/shared/devtools/app-manager/projects.css
@@ +388,5 @@
> +
> +[status="warning"] > .project-item-warnings,
> +[status="error"] > .project-item-errors,
> +[status="warning"] > .project-warnings,
> +[status="error"] > .project-errors {

These rules prevent displaying errors and warning when we have both errors and warnings.
Attachment #799509 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot (:ochameau) from comment #33)
> @@ +118,5 @@
> > +                if (validation.manifest) {
> > +                  project.name = validation.manifest.name;
> > +                  project.icon = UI._getLocalIconURL(project, validation.manifest);
> > +                  project.manifest = validation.manifest;
> > +                  project.manifestAsString = JSON.stringify(validation.manifest);
> 
> Can we remove manifest and manifestAsString until we start using them?

We use manifest. But not manifestAsString.

> @@ +125,5 @@
> > +                project.validationStatus = "valid";
> > +
> > +                if (validation.errors.length > 0) {
> > +                  project.errorsCount = validation.errors.length;
> > +                  project.errors = validation.errors.join(",\n ");
> 
> We should improve this. \n won't have expected effect as we inject errors
> and warnings as textContent.
> Each error/warning should be displayed on a new line.
> But we can do that in a followup.

Follow up.

> ::: browser/themes/shared/devtools/app-manager/projects.css
> @@ +388,5 @@
> > +
> > +[status="warning"] > .project-item-warnings,
> > +[status="error"] > .project-item-errors,
> > +[status="warning"] > .project-warnings,
> > +[status="error"] > .project-errors {
> 
> These rules prevent displaying errors and warning when we have both errors
> and warnings.

Damn, I didn't think about supporting warning and errors at the same time.

I will address these 2 last issues in a followup.
Todo:
- file a bug for persona support
- tests (probably follow up)
- improve support for warnings and errors (see comment 40) (follow up)
Attachment #799562 - Flags: review?(poirot.alex)
Attachment #799564 - Flags: review?(poirot.alex)
Attachment #799597 - Flags: review?(poirot.alex)
Attachment #799598 - Flags: review?(poirot.alex)
Attachment #799599 - Flags: review?(poirot.alex)
Attachment #799600 - Flags: review?(poirot.alex)
Attachment #799601 - Flags: review?(poirot.alex)
Attachment #799603 - Flags: review?(poirot.alex)
Attachment #799625 - Flags: review?(poirot.alex)
Todo:
- file a bug for persona support
- tests (probably follow up)
- improve support for warnings and errors (see comment 40) (follow up)
- make sure the wallpaper changes work
Comment on attachment 799431 [details] [diff] [review]
Patch C: appmgr_jars.diff

Review of attachment 799431 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/linux/jar.mn
@@ +229,5 @@
> +  skin/classic/browser/devtools/app-manager/connection-footer.css     (../shared/devtools/app-manager/connection-footer.css)
> +  skin/classic/browser/devtools/app-manager/index.css                 (../shared/devtools/app-manager/index.css)
> +  skin/classic/browser/devtools/app-manager/device.css                (../shared/devtools/app-manager/device.css)
> +  skin/classic/browser/devtools/app-manager/projects.css              (../shared/devtools/app-manager/projects.css)
> +  skin/classic/browser/devtools/app-manager/app-manager-common.css    (../shared/devtools/app-manager/app-manager-common.css)

here and in two other themes's jar.mn, we should drop this line, 
as none of the other patches add this file.
(In reply to Alexandre Poirot (:ochameau) from comment #44)
> Comment on attachment 799431 [details] [diff] [review]
> Patch C: appmgr_jars.diff
> 
> Review of attachment 799431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/linux/jar.mn
> @@ +229,5 @@
> > +  skin/classic/browser/devtools/app-manager/connection-footer.css     (../shared/devtools/app-manager/connection-footer.css)
> > +  skin/classic/browser/devtools/app-manager/index.css                 (../shared/devtools/app-manager/index.css)
> > +  skin/classic/browser/devtools/app-manager/device.css                (../shared/devtools/app-manager/device.css)
> > +  skin/classic/browser/devtools/app-manager/projects.css              (../shared/devtools/app-manager/projects.css)
> > +  skin/classic/browser/devtools/app-manager/app-manager-common.css    (../shared/devtools/app-manager/app-manager-common.css)
> 
> here and in two other themes's jar.mn, we should drop this line, 
> as none of the other patches add this file.

Erf, that was an old patch.
Monsieur Chameau, I thought a pretty github commit list might make your life easier:

https://github.com/paulrouget/mozilla-central/compare/mozilla:fx-team...app-manager-rebase-fx-team
Blocks: appmgr_v1
Updated jar.mn.
Attachment #799431 - Attachment is obsolete: true
Attachment #799431 - Flags: review?(poirot.alex)
Attachment #800046 - Flags: review?(poirot.alex)
Comment on attachment 799517 [details] [diff] [review]
Patch J: appmgr_imgs.diff

Review of attachment 799517 [details] [diff] [review]:
-----------------------------------------------------------------

Ok splinter review wasn't showing noise.png...
Attachment #799517 - Flags: review- → review+
Comment on attachment 799433 [details] [diff] [review]
Patch D: appmgr_locales.diff

Review of attachment 799433 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/devtools/app-manager.dtd
@@ +40,5 @@
> +<!ENTITY projects.installApp "Install">
> +<!ENTITY projects.startApp "Start">
> +<!ENTITY projects.stopApp "Stop">
> +<!ENTITY projects.debugApp "Debug">
> +<!ENTITY projects.deleteApp "Delete">

This key doesn't seem to be used.
Attachment #799433 - Flags: review?(poirot.alex) → review+
Comment on attachment 799562 [details] [diff] [review]
addendum 1: add license headers

Review of attachment 799562 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/app-manager/app-validator.js
@@ +1,4 @@
> +/* 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/. */
> +

I included this in patch from bug 911781
Attachment #799562 - Flags: review?(poirot.alex) → review+
Attachment #799564 - Flags: review?(poirot.alex) → review+
Attachment #799597 - Flags: review?(poirot.alex) → review+
Attachment #799598 - Flags: review?(poirot.alex) → review+
Attachment #799599 - Flags: review?(poirot.alex) → review+
Attachment #799600 - Flags: review?(poirot.alex) → review+
Comment on attachment 799601 [details] [diff] [review]
patch_H_update.diff (only changes)

Review of attachment 799601 [details] [diff] [review]:
-----------------------------------------------------------------

Can you also replace the `+` by parseInt?

Also, allow me to disagree with the [hidden] {display:none} rule.
From what I can see it is enable by default on html:
  data:text/html,<div hidden=true>foo</div>
Attachment #799601 - Flags: review?(poirot.alex) → review+
Attachment #799603 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot (:ochameau) from comment #52)
> Also, allow me to disagree with the [hidden] {display:none} rule.
> From what I can see it is enable by default on html:
>   data:text/html,<div hidden=true>foo</div>

My bad. I thought that was XUL only.
Attachment #799625 - Flags: review?(poirot.alex) → review+
Attachment #800046 - Flags: review?(poirot.alex) → review+
Thanks for all the reviews Alex!

https://hg.mozilla.org/integration/fx-team/rev/ac59fd15fbcb
https://hg.mozilla.org/mozilla-central/rev/ac59fd15fbcb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Depends on: 913366
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.