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.

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.

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.

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?

These are all the patches I'd like to land in this bug (A -> I).

Forgot the images. I like images.

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?

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

Comment on attachment 799517 [details] [diff] [review] Patch J: appmgr_imgs.diff Review of attachment 799517 [details] [diff] [review]: ----------------------------------------------------------------- We miss noise.png

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

(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.

(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.

(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)

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

Updated jar.mn.

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...

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.

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

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>

(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.

Thanks for all the reviews Alex! https://hg.mozilla.org/integration/fx-team/rev/ac59fd15fbcb

\o/

I started this: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Using_the_App_Manager