Itchpad is currently developed as an extension here: https://github.com/bgrins/itchpad. We should pull it into m-c so it is part of the test suite and can be easily used from the rest of our code.

Note that I have started the work on the tree in bug 970517. Once it is complete, I will ask for API and UI feedback from you :)

Here is a patch that has ported the current repo over. Joe, can you take a look at the changes to Loader.jsm and the Makefile.in file? I've tried to model it similarly to how gcli works. Heather, I've added an itchpad-loader.xul and itchpad-loader.js file to make it easy for dev. Can you apply the patch and make sure chrome://browser/content/devtools/itchpad-loader.xul is looking like a reasonable replacement for what main.js was doing before. I've removed the window popup / menu item / keyboard shortcut since I'm thinking we will be focusing on embedding it for now, but let me know what you think about that also.

Note: steps to port code (since we may have to do it again following more cleanup on github): * Copy lib/ and chrome/ folder * In chrome/ delete itchpad.window.xul (but keep the newly added itchpad-loader.xul and itchpad-loader.js files) * Copy all of the require("plugins/*") from main.js into itchpad.js * In lib/ delete main.js * Prefix all local require() references with "itchpad/" * Change path to readdir.js in local.js to chrome://browser/content/devtools/readdir.js * Cross fingers, build, etc, then open `chrome://browser/content/devtools/itchpad-loader.xul`

Same patch - just fixes loader path (removes "lib" from path)

Comment on attachment 8398085 [details] [diff] [review] itchpad-WIP.patch Review of attachment 8398085 [details] [diff] [review]: ----------------------------------------------------------------- I love it. Just had to drag a folder onto the UI to get a project going.

Comment on attachment 8398085 [details] [diff] [review] itchpad-WIP.patch Review of attachment 8398085 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. I think we'll need to get r+ from a build peer like :gps (gps@mozilla.com)

itchpad/chrome/content/icon-sample.png missing?

(erf, no, sorry)

How to use itchpad-loader.xul and itchpad-loader.js?

Why do you use forceOwnRefreshDriver? I don't think we'll ever swap itchpad's iframes? I don't think itchpad-loader.xul should be able to close itself (itchpad-cmd-close). What's the use of itchpad-loader.xul? Do I need to use it or can I just create my own iframe?

I also see: console.error: Message: TypeError: this.selectedContainer is undefined Stack: TreeView<.getSelected@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/browser/modules/devtools/itchpad/tree.js:288:5 NewFile<.onCommand@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/browser/modules/devtools/itchpad/plugins/new/lib/new.js:31:1 Itchpad<.pluginDispatch/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/browser/modules/devtools/itchpad/itchpad.js:363:32 Itchpad<.pluginDispatch@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/browser/modules/devtools/itchpad/itchpad.js:361:5 Itchpad<.initPlugins/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/browser/modules/devtools/itchpad/itchpad.js:158:7

(In reply to Paul Rouget [:paul] from comment #10) > Why do you use forceOwnRefreshDriver? I don't think we'll ever swap > itchpad's iframes? > > I don't think itchpad-loader.xul should be able to close itself > (itchpad-cmd-close). > > What's the use of itchpad-loader.xul? Do I need to use it or can I just > create my own iframe? (In reply to Paul Rouget [:paul] from comment #9) > How to use itchpad-loader.xul and itchpad-loader.js? When embedding itchpad, you won't use itchpad-loader.js directly. That is more like a reference implementation that embeds it at chrome://browser/content/devtools/itchpad-loader.xul. So you will want to use code like what is in use at itchpad-loader.xul / itchpad-loader.js to embed it elsewhere. Specifically: const Itchpad = require("itchpad/itchpad"); const { Projects } = require("itchpad/project"); Projects.defaultProject().then(project => { let iframe = document.getElementById("someiframe"); let itchpad = Itchpad.Itchpad(iframe, project); }).then(null, console.error); Where someiframe is the iframe element that you are wanting to use to host itchpad.

When loading itchpad, I get this: > TypeError: redeclaration of var Promise promise.js:4 I use: > const Itchpad = require("itchpad/itchpad"); > const {Projects} = require("itchpad/project");

I had to do that: > diff --git a/browser/devtools/itchpad/lib/helpers/promise.js b/browser/devtools/itchpad/lib/helpers/promise.js > index b3f6e02..69b97dc 100644 > --- a/browser/devtools/itchpad/lib/helpers/promise.js > +++ b/browser/devtools/itchpad/lib/helpers/promise.js > @@ -1,5 +1,4 @@ > // ... until sdk/core/promise uses Promise.jsm... > > const { Cu } = require("chrome"); > -const { Promise } = Cu.import("resource://gre/modules/Promise.jsm", {}); > -module.exports = Promise; > +module.exports = Cu.import("resource://gre/modules/Promise.jsm", {}).Promise;

Can I get an overview of the Project API?

I'm mostly curious about what you call a "manifest" Projects.forManifest().

The File/Edit menu should not be part of itchpad.xul as it won't ever be a top level window.

(In reply to Paul Rouget [:paul] from comment #17) > The File/Edit menu should not be part of itchpad.xul as it won't ever be a > top level window. I'm assuming we will still want window-level menu items for things like find/replace, new file, etc. Instead of adding the menu in itchpad.xul would it make sense to pass a reference to a menu element when instantiating so that we could add them to that menu?

(In reply to Brian Grinstead [:bgrins] from comment #18) > (In reply to Paul Rouget [:paul] from comment #17) > > The File/Edit menu should not be part of itchpad.xul as it won't ever be a > > top level window. > > I'm assuming we will still want window-level menu items for things like > find/replace, new file, etc. Instead of adding the menu in itchpad.xul > would it make sense to pass a reference to a menu element when instantiating > so that we could add them to that menu? I don't know what's best, but the app manager has a menubar, and itchpad menus should live in this menubar, not in its own menubar.

(In reply to Paul Rouget [:paul] from comment #16) > I'm mostly curious about what you call a "manifest" Projects.forManifest(). (In reply to Paul Rouget [:paul] from comment #17) > The File/Edit menu should not be part of itchpad.xul as it won't ever be a > top level window. We talked about this a bit. We will remove all manifest functionality from itchpad for now and let the app manager take care of that, passing in needed options into itchpad.setProjectToSinglePath(path, options).

Second version of the patch. This includes a new interface and a few other fixes after talking with Paul and Heather. The code should be easier to reference now - you won't need to include the Project object. Some sample code would look like: const Itchpad = require("itchpad/itchpad"); let iframe = document.getElementById("my-iframe"); let itchpad = Itchpad.Itchpad(); itchpad.load(iframe).then(() => { itchpad.setProjectToSinglePath("path/to/folder", { name: "App name", version: ".1", iconUrl: "urltoicon", iframeSrc: "urltoprojectframe" }); });

It works pretty well. Thank you! `iframeSrc` is not very descriptive. Maybe `projectOverviewURL`. We also need to be able to add icons to the tree head (like warning or errors icons).

File names should not wrap when the sidebar is resized.

When the top item is selected, its color doesn't change (it stays "light blue" when it turns "dark blue").

The tree is not compact enough. xcode vs itchpad: http://i.imgur.com/DDxXd9j.png

I've updated fx-team, and now I get: > Error: Module `itchpad/itchpad` is not found at resource://gre/modules/commonjs/itchpad/itchpad.js

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #26) > I've updated fx-team, and now I get: > > > Error: Module `itchpad/itchpad` is not found at resource://gre/modules/commonjs/itchpad/itchpad.js My fault. Ignore.

Addressing Comments 22-27: * Switched iframeSrc to projectOverviewURL * Use dark blue color for project overview when selected * More compact tree, no wrapping on file / folder names, min-width for resizing * Made chrome://browser/content/devtools/itchpad-loader.xul use the app mgr API, using content/browser/devtools as the editing path. Still looking at how to copy a sample web app into a temp directory in order to run tests - specifically looking into getTestFilePath in app-manager tests

I'm going to start work on adding tests and more comments now, but going to ask for initial review of the code now. Things that have changes since last WIP: * Moved styles into themes/ folder * Added localized strings * Using selected bookmarks BG color for selected items in tree * Removed a couple more unused plugins We've discussed waiting to figure out the menubar situation until after both of the pieces (this and new app manager UI) have landed.

Comment on attachment 8402938 [details] [diff] [review] itchpad.patch Review of attachment 8402938 [details] [diff] [review]: ----------------------------------------------------------------- This is pretty hard to review. Can you add more comments? Some files don't have comments at all. Also, It'd be very useful to have some sort of public API (methods and events) documented somewhere (in the code, or here, in bugzilla, or maybe in MDN). Also, an overview of how this all work would help me to go through all these files. To me, it seems that there's a lot of not operational code. I would prefer if we could cover *only* what is useful for the app manager today, and add feature incrementally. Do you think it would help you if we land the new app manager UI first? ::: browser/devtools/itchpad/chrome/content/itchpad.xul @@ +21,5 @@ > +<!ENTITY % sourceEditorStrings SYSTEM "chrome://browser/locale/devtools/sourceeditor.dtd"> > +%sourceEditorStrings; > +]> > + > +<window id="itchpad-window" Can you use a <page>? @@ +26,5 @@ > + xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" class="theme-body" > + title="Itchpad" > + windowtype="devtools:itchpad" > + macanimcationtype="document" > + fullscreenbutton="true" I don't think this window will ever be a top level window, right? So these are not needed. @@ +67,5 @@ > + </menupopup> > + </popupset> > + > + <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/> > + <script type="application/javascript;version=1.8" src="chrome://browser/content/devtools/theme-switching.js"></script> Do we want to support themes for itchapd? Not sure it's really useful. Maybe as a followup? ::: browser/devtools/itchpad/lib/appmanager.js @@ +8,5 @@ > +const data = require("sdk/self").data; > + > +exports.modifyAppManager = function() { > + tabs.on("ready", (tab) => { > + if (tab.url == "about:app-manager") { What is this? We plan to drop about:app-manager and only introduce itchpad in the new app-manager. ::: browser/devtools/itchpad/lib/editors.js @@ +111,5 @@ > + > + save: function(resource) { > + return resource.save(this.editor.getText()).then(() => { > + this.editor.setClean(); > + emit(this, "save", resource); How do I get this event from Itchpad? ::: browser/devtools/itchpad/lib/event/scope.js @@ +2,5 @@ > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ > +/* 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/. */ > + Please add a short description of what all the files do, it will help me to review.

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #30) > Comment on attachment 8402938 [details] [diff] [review] > itchpad.patch > > Review of attachment 8402938 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is pretty hard to review. Can you add more comments? Some files don't > have comments at all. Also, It'd be very useful to have some sort of public > API (methods and events) documented somewhere (in the code, or here, in > bugzilla, or maybe in MDN). Also, an overview of how this all work would > help me to go through all these files. > > To me, it seems that there's a lot of not operational code. I would prefer > if we could cover *only* what is useful for the app manager today, and add > feature incrementally. > > Do you think it would help you if we land the new app manager UI first? > It would make it easier if we had the app manager there, but I don't think it is required (itchpad-loader is using the same functionality). > ::: browser/devtools/itchpad/chrome/content/itchpad.xul > @@ +21,5 @@ > > +<!ENTITY % sourceEditorStrings SYSTEM "chrome://browser/locale/devtools/sourceeditor.dtd"> > > +%sourceEditorStrings; > > +]> > > + > > +<window id="itchpad-window" > > Can you use a <page>? > > @@ +26,5 @@ > > + xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" class="theme-body" > > + title="Itchpad" > > + windowtype="devtools:itchpad" > > + macanimcationtype="document" > > + fullscreenbutton="true" > > I don't think this window will ever be a top level window, right? So these > are not needed. > > @@ +67,5 @@ > > + </menupopup> > > + </popupset> > > + > > + <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/> > > + <script type="application/javascript;version=1.8" src="chrome://browser/content/devtools/theme-switching.js"></script> > > Do we want to support themes for itchapd? Not sure it's really useful. Maybe > as a followup? > Made all of the markup changes > ::: browser/devtools/itchpad/lib/appmanager.js > @@ +8,5 @@ > > +const data = require("sdk/self").data; > > + > > +exports.modifyAppManager = function() { > > + tabs.on("ready", (tab) => { > > + if (tab.url == "about:app-manager") { > > What is this? > > We plan to drop about:app-manager and only introduce itchpad in the new > app-manager. I've removed this > > ::: browser/devtools/itchpad/lib/editors.js > @@ +111,5 @@ > > + > > + save: function(resource) { > > + return resource.save(this.editor.getText()).then(() => { > > + this.editor.setClean(); > > + emit(this, "save", resource); > > How do I get this event from Itchpad? > > ::: browser/devtools/itchpad/lib/event/scope.js > @@ +2,5 @@ > > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ > > +/* 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/. */ > > + > > Please add a short description of what all the files do, it will help me to > review. I've been working on adding more documentation and tests, and removing code that won't be needed for the app manager - I'll send over a new patch when I finish.

Changes made for review. Includes tests, and documentation for everything except for some of the plugins. Going to fold this into the first patch soon but leaving this here for future reference since it also removes quite a bit of functionality (like Pairs) that we will probably want later on.

Folded patch

Brian, this is my plan: - attach app manager v2 patch to bug 999417 soon. With no itchpad integration. - provide a patch to integrate itchpad (here or in bug 999417) - from here, you'll be able to experiment with your code in the app manager - you then might want to change some code and maybe strip down some code

Brian, this is how you can test itchpad in the app manager: 1) apply patch from bug 999417, and the 2 patches in this bug 2) add `ac_add_options --enable-fxide` to you mozconfig 3) build Open app manager in the tools menu. If you don't have any app code on your disk, go to about:config, and change this pref: > devtools.fxide.templatesURL to: > http://htmlpad.org/eTSTcUAhPj/ Then in the App menu, click on "create new app".

Patch that lands itchpad in browser/devtools

Patch that integrates itchpad with the app manager v2

Rebased + minor fixes (support for toggling itchpad).

So what's the story about the tree? Are we gonna keep tree.js? Optimizer is working on a tree widget, is it a thing we want to use here? Also, what about use a xul:tree? The whole app manager UI is in XUL. So it would fit better. Also, xul:trees solve many problems for free: keybindings, performance, drag'n drop support, folding, renaming, native theme...

Also, a xul:tree works in RTL.

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #40) > So what's the story about the tree? > > Are we gonna keep tree.js? > > Optimizer is working on a tree widget, is it a thing we want to use here? > > Also, what about use a xul:tree? The whole app manager UI is in XUL. So it > would fit better. Also, xul:trees solve many problems for free: keybindings, When I was trying to leverage xul:tree for storage manager, I was told that for the sake of long run, better not rely on xul by so many people. > performance, drag'n drop support, folding, renaming, native theme... I think HTML5's performance will be better. Many xul elements are backed up by simple html elements only. Drag n drop should also not be a problem. But does a tree need that ? I am not a fan of native themes in devtools. I would rather see the Itchpad in devtools styles. Renaming is quite easy in the TreeWidget. You just insert a custom DOM element in that location instead of a string. (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #41) > Also, a xul:tree works in RTL. So does the TreeWidget :)

(In reply to Girish Sharma [:Optimizer] from comment #42) > > Also, what about use a xul:tree? The whole app manager UI is in XUL. So it > > would fit better. Also, xul:trees solve many problems for free: keybindings, > > When I was trying to leverage xul:tree for storage manager, I was told that > for the sake of long run, better not rely on xul by so many people. Yep. I don't think we should have a xul:tree inside the toolbox. > > performance, drag'n drop support, folding, renaming, native theme... > > I think HTML5's performance will be better. Many xul elements are backed up > by simple html elements only. xul:tree is full C++, and highly optimized (it's the core widget of thunderbird). > Drag n drop should also not be a problem. But does a tree need that ? Yes. Drag a file from file manager to copy it in a folder. > I am not a fan of native themes in devtools. I would rather see the Itchpad > in devtools styles. The App Manager now switched to a native (XUL) theme. So Itchpad will need to blend.

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #43) > (In reply to Girish Sharma [:Optimizer] from comment #42) > > > Also, what about use a xul:tree? The whole app manager UI is in XUL. So it > > > would fit better. Also, xul:trees solve many problems for free: keybindings, > > > > When I was trying to leverage xul:tree for storage manager, I was told that > > for the sake of long run, better not rely on xul by so many people. > > Yep. I don't think we should have a xul:tree inside the toolbox. > > > > performance, drag'n drop support, folding, renaming, native theme... > > > > I think HTML5's performance will be better. Many xul elements are backed up > > by simple html elements only. > > xul:tree is full C++, and highly optimized (it's the core widget of > thunderbird). The backend is in C++ instead of JS, but frontend is XUL only. And sometimes the C++ to JS bridge becomes the bottleneck. Not that I am saying that it might be the case for xul:tree. > > Drag n drop should also not be a problem. But does a tree need that ? > > Yes. Drag a file from file manager to copy it in a folder. Ok. What I thought was a drag and drop where drag start and end are both on the tree. > > I am not a fan of native themes in devtools. I would rather see the Itchpad > > in devtools styles. > > The App Manager now switched to a native (XUL) theme. So Itchpad will need > to blend. My indirect intention was to say that I would rather see the new App Manager in devtools theme :)

(In reply to Girish Sharma [:Optimizer] from comment #44) > > > Drag n drop should also not be a problem. But does a tree need that ? > > > > Yes. Drag a file from file manager to copy it in a folder. > > Ok. What I thought was a drag and drop where drag start and end are both on > the tree. That too. > > > I am not a fan of native themes in devtools. I would rather see the Itchpad > > > in devtools styles. > > > > The App Manager now switched to a native (XUL) theme. So Itchpad will need > > to blend. > > My indirect intention was to say that I would rather see the new App Manager > in devtools theme :) That's not happening.

> > Drag n drop should also not be a problem. But does a tree need that ? > > Yes. Drag a file from file manager to copy it in a folder. > Drag and drop from the file manager into the tree should be doable either way - a simple implementation is included in the current patch under drag-drop-new.js. It doesn't seem like it would be that hard to move the dragover and drop listeners to all of the tree items instead of the whole tree. Is there an implementation of the xul tree somewhere that allows dropping from the filesystem that I can play with to get an idea of what it is doing? I don't have the drag/drop plugin enabled because (a) I think we can land it originally without that functionality and (b) we will need to think about and cover some edge cases. What happens if the dropped folder contains our current folder? What if a file from within the opened folder is dropped in - is that interpreted as a cut event or still a copy? How do we deal with symlinks?

Talked on IRC: we will keep the tree as it is and will switch to another implementation later (treewidget or xul:tree).

Apparently, there's a hidden sidebar on the right that I can pull: http://i.imgur.com/rWChYzO.png

Should I start reviewing this patch or should I wait?

Comment on attachment 8418868 [details] [diff] [review] itchpad-part1.patch Review of attachment 8418868 [details] [diff] [review]: ----------------------------------------------------------------- May as well start reviewing it. I'm still working out some platform specific test failures but the code isn't likely to change much.

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #48) > Apparently, there's a hidden sidebar on the right that I can pull: > http://i.imgur.com/rWChYzO.png Thanks, that isn't being used anymore. I've got that removed in development now (just removed the sidebar markup and the bottom part of itchpad._buildSidebar).

Comment on attachment 8418868 [details] [diff] [review] itchpad-part1.patch Review of attachment 8418868 [details] [diff] [review]: ----------------------------------------------------------------- * When an image has changed on disk, it's not updated in itchpad. * Does it work well on Windows? (tested only on Linux & Mac) * make sure all imports/require and all selectors are useful * The tree looks bad in RTL * I think we should hide the itchpad-toolbar and the editor-toolbar. Too many bars everywhere : http://i.imgur.com/ibeuSqv.png vs http://i.imgur.com/tSRCq5p.png This is great. I use it everyday, and it feels good. The code is great too. Many nits, so r-. ::: browser/devtools/itchpad/lib/helpers/event.js @@ +7,5 @@ > +/** > + * This file wraps EventEmitter objects to provide functions to forget > + * all events bound on a certain object. > + */ > + I like that. Maybe you want to improve the original implementation. ::: browser/devtools/itchpad/lib/helpers/l10n.js @@ +28,5 @@ > + get newLabel() { return getLocalizedString("itchpad.newLabel"); }, > + get selectFileLabel() { return getLocalizedString("itchpad.selectFileLabel"); }, > + get openFileLabel() { return getLocalizedString("itchpad.openFileLabel"); }, > + get openFolderLabel() { return getLocalizedString("itchpad.selectFolderLabel"); }, > +}; \ No newline at end of file Not saying is bad, but why do you need these properties? Can you just use getLocalizedString everywhere? ::: browser/devtools/itchpad/lib/helpers/promise.js @@ +5,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/** > + * This helper is a quick way to require() the Promise object from Promise.jsm. > + */ Do we still need this? ::: browser/devtools/itchpad/lib/itchpad.js @@ +34,5 @@ > +// Disabled Plugins. These are not used right now, but will be > +// needed soon to enable important editing features. > +// require("itchpad/plugins/open/lib/open"); > +// require("itchpad/plugins/drag-drop-new/lib/drag-drop-new"); > +// require("itchpad/plugins/find-and-replace/lib/plugin"); Apparently, "find" works. Even without this. @@ +282,5 @@ > + * Promise that is resolved once the project is ready to be used. > + */ > + setProjectToSinglePath: function(path, opts = {}) { > + this.project.customOpts = opts; > + this.project.projectType = "APP_MANAGER"; Feels weird that this is hard coded. @@ +483,5 @@ > + * @param ...args args > + * All remaining parameters are passed into the handler. > + */ > + pluginDispatch: function(handler, ...args) { > + // XXX: Memory leak when console.log an Editor here Can we fix it? ::: browser/devtools/itchpad/lib/plugins/app-manager/lib/plugin.js @@ +35,5 @@ > + versionLabel.className = "project-version-label"; > + image.className = "project-image"; > + > + let name = customOpts.name || resource.basename; > + let version = customOpts.version || "v0.0.1"; Can you kill the version label? We don't use it in the app manager. ::: browser/devtools/itchpad/lib/plugins/drag-drop-new/lib/drag-drop-new.js @@ +13,5 @@ > +const { ObjectClient } = Cu.import("resource://gre/modules/devtools/dbg-client.jsm", {}); > +const { EnvironmentClient } = Cu.import("resource://gre/modules/devtools/dbg-client.jsm", {}); > +const { OS } = Cu.import("resource://gre/modules/osfile.jsm", {}); > + > +var DragDropNew = Class({ Is that used? ::: browser/devtools/itchpad/lib/plugins/find-and-replace/lib/plugin.js @@ +6,5 @@ > + > +const { Class } = require("sdk/core/heritage"); > +const { registerPlugin, Plugin } = require("itchpad/plugins/core"); > + > +var FindAndReplace = Class({ Does this feature work? @@ +12,5 @@ > + > + init: function(host) { > + this.command = this.host.addCommand({ > + id: "find", > + key: "f", l10n ::: browser/devtools/itchpad/lib/plugins/image-view/lib/image-editor.js @@ +35,5 @@ > + this.emit("load"); > + } > +}); > + > +exports.ImageEditor = ImageEditor; Could we use a ImageDocument here? Then we could scroll and zoom. ::: browser/devtools/itchpad/lib/plugins/open/lib/open.js @@ +13,5 @@ > + > + init: function(host) { > + this.command = this.host.addCommand({ > + id: "cmd-open", > + key: "o", l10n? ::: browser/devtools/itchpad/lib/stores/local.js @@ +33,5 @@ > + defaultCategory: "js", > + > + initialize: function(path) { > + this.initStore(); > + this.window = Cc["@mozilla.org/appshell/appShellService;1"].getService(Ci.nsIAppShellService).hiddenDOMWindow; Services.appShell.hiddenDOMWindow @@ +127,5 @@ > + }.bind(this)); > + }, > + > + refreshLoop: function() { > + // XXX: Once Bug 958280 adds a watch function, will not need to forever loop here. The strategy we adopted in the App Manager (we also need to track when the project has changed) is to check the directory on focus. But I guess this works too. @@ +148,5 @@ > + return this._refreshDeferred.promise; > + } > + this._refreshDeferred = promise.defer(); > + > + let worker = this.worker = new ChromeWorker("chrome://browser/content/devtools/readdir.js"); First time I see a ChromeWorker. Neat :) ::: browser/devtools/itchpad/lib/stores/resource.js @@ +296,5 @@ > + try { > + this._contentType = mimeService.getTypeFromFile(new FileUtils.File(this.path)); > + } catch(ex) { > + if (ex.name !== "NS_ERROR_NOT_AVAILABLE") { > + console.error(ex, this.path); This dumps this in the console: Message: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMIMEService.getTypeFromFile]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/itchpad/stores/resource.js :: FileResource<.contentType :: line 297" data: no] /home/paul/Downloads/foobar/LICENSE ::: browser/devtools/itchpad/lib/tree.js @@ +14,5 @@ > +const { on, forget } = require("itchpad/helpers/event"); > +const { OS } = Cu.import("resource://gre/modules/osfile.jsm", {}); > + > +const HTML_NS = "http://www.w3.org/1999/xhtml"; > +const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; XUL_NS is never used. ::: browser/themes/shared/devtools/itchpad/itchpad.css @@ +44,5 @@ > + vertical-align: middle; > + display: inline-block; > +} > +#main-deck .sources-tree .side-menu-widget-item .file-icon { > + content: ""; Why `content`? @@ +164,5 @@ > + font-weight: bold; > + background: #ddf; > +} > + > +.refresh-button { Is that used? @@ +168,5 @@ > +.refresh-button { > + list-style-image: url("chrome://browser/skin/sync-16.png"); > +} > + > +.settings-button { Is that used? @@ +173,5 @@ > + list-style-image: url("chrome://browser/skin/devtools/option-icon.png"); > + -moz-image-region: rect(0px 16px 16px 0px); > +} > + > +.source-live { Is that used? @@ +187,5 @@ > +.source-live[selected=true] { > + -moz-image-region: rect(0px, 64px, 16px, 48px); > +} > + > +.source-project { Is that used? (I won't check all the selectors. Remove the ones that are never used). @@ +211,5 @@ > +#pair-choice:not([aspects~="live"]) > #pair-live { > + display: none; > +} > + > +.pane-toggle { Is that used?

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #52) > * When an image has changed on disk, it's not updated in itchpad. I will take a further look into this. It can get tricky when the content changed on disk and an editor is focused since we don't want to wipe out working changes. But in the case of leaving an editor and coming back we should be reloading. > * Does it work well on Windows? (tested only on Linux & Mac) I haven't used it, but the tests pass. I've gone through and improved the tests so that they are now passing on all platforms. Except for a perma orange on browser/browser/devtools/tilt/test/browser_tilt_02_notifications-seq.js for OSX and Windows (no idea how I managed this): https://tbpl.mozilla.org/?tree=Try&rev=cb87a9299a46. I've left comments on the points that I've already addressed in development, and will go back through with the others later. > ::: browser/devtools/itchpad/lib/helpers/promise.js > @@ +5,5 @@ > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > + > > +/** > > + * This helper is a quick way to require() the Promise object from Promise.jsm. > > + */ > > Do we still need this? > Not particularly, it is just a handy way to use require() for it and lowercases the name. > > @@ +483,5 @@ > > + * @param ...args args > > + * All remaining parameters are passed into the handler. > > + */ > > + pluginDispatch: function(handler, ...args) { > > + // XXX: Memory leak when console.log an Editor here > > Can we fix it? > > ::: browser/devtools/itchpad/lib/plugins/app-manager/lib/plugin.js > @@ +35,5 @@ > > + versionLabel.className = "project-version-label"; > > + image.className = "project-image"; > > + > > + let name = customOpts.name || resource.basename; > > + let version = customOpts.version || "v0.0.1"; > > Can you kill the version label? We don't use it in the app manager. Killed > > ::: browser/devtools/itchpad/lib/plugins/drag-drop-new/lib/drag-drop-new.js > @@ +13,5 @@ > > +const { ObjectClient } = Cu.import("resource://gre/modules/devtools/dbg-client.jsm", {}); > > +const { EnvironmentClient } = Cu.import("resource://gre/modules/devtools/dbg-client.jsm", {}); > > +const { OS } = Cu.import("resource://gre/modules/osfile.jsm", {}); > > + > > +var DragDropNew = Class({ > > Is that used? No, it is there to allow dropping folders / files into the tree but will need quite a bit of thought about different cases. So I can remove it for now. > ::: browser/devtools/itchpad/lib/itchpad.js > @@ +34,5 @@ > > +// Disabled Plugins. These are not used right now, but will be > > +// needed soon to enable important editing features. > > +// require("itchpad/plugins/open/lib/open"); > > +// require("itchpad/plugins/drag-drop-new/lib/drag-drop-new"); > > +// require("itchpad/plugins/find-and-replace/lib/plugin"); > > Apparently, "find" works. Even without this. > > > ::: browser/devtools/itchpad/lib/plugins/find-and-replace/lib/plugin.js > @@ +6,5 @@ > > + > > +const { Class } = require("sdk/core/heritage"); > > +const { registerPlugin, Plugin } = require("itchpad/plugins/core"); > > + > > +var FindAndReplace = Class({ > > Does this feature work? It's not enabled right now. Codemirror find is working, but we should have a better feature that lets you search across files, do replacement, etc. It seems important to tackle after the first landing - which is why it is left in even though it is disabled. > > ::: browser/devtools/itchpad/lib/stores/local.js > @@ +33,5 @@ > > + defaultCategory: "js", > > + > > + initialize: function(path) { > > + this.initStore(); > > + this.window = Cc["@mozilla.org/appshell/appShellService;1"].getService(Ci.nsIAppShellService).hiddenDOMWindow; > > Services.appShell.hiddenDOMWindow Replaced > > @@ +127,5 @@ > > + }.bind(this)); > > + }, > > + > > + refreshLoop: function() { > > + // XXX: Once Bug 958280 adds a watch function, will not need to forever loop here. > > The strategy we adopted in the App Manager (we also need to track when the > project has changed) is to check the directory on focus. But I guess this > works too. I'm fine with either - hopefully these are only temporary measures until we have file watching. > ::: browser/devtools/itchpad/lib/stores/resource.js > @@ +296,5 @@ > > + try { > > + this._contentType = mimeService.getTypeFromFile(new FileUtils.File(this.path)); > > + } catch(ex) { > > + if (ex.name !== "NS_ERROR_NOT_AVAILABLE") { > > + console.error(ex, this.path); > > This dumps this in the console: > > Message: [Exception... "Component returned failure code: 0x80004005 > (NS_ERROR_FAILURE) [nsIMIMEService.getTypeFromFile]" nsresult: "0x80004005 > (NS_ERROR_FAILURE)" location: "JS frame :: > resource://gre/modules/commonjs/toolkit/loader.js -> > resource:///modules/devtools/itchpad/stores/resource.js :: > FileResource<.contentType :: line 297" data: no] > /home/paul/Downloads/foobar/LICENSE > Catching that particular exception as well now. Added a LICENSE file to the mock data for tests and development also (this is failing b/c it has no extension, I guess). > ::: browser/devtools/itchpad/lib/tree.js > @@ +14,5 @@ > > +const { on, forget } = require("itchpad/helpers/event"); > > +const { OS } = Cu.import("resource://gre/modules/osfile.jsm", {}); > > + > > +const HTML_NS = "http://www.w3.org/1999/xhtml"; > > +const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; > > XUL_NS is never used. Removed > > ::: browser/themes/shared/devtools/itchpad/itchpad.css Fixed up all the CSS, removing unused selectors.

More comments (patch incoming): > * The tree looks bad in RTL Let's deal with the tree UI after landing (but before enabling) > * I think we should hide the itchpad-toolbar and the editor-toolbar. Too > many bars everywhere : http://i.imgur.com/ibeuSqv.png vs > http://i.imgur.com/tSRCq5p.png Hidden with CSS, but left status bars around so we can revisit the ux later. > ::: browser/devtools/itchpad/lib/helpers/l10n.js > @@ +28,5 @@ > > + get newLabel() { return getLocalizedString("itchpad.newLabel"); }, > > + get selectFileLabel() { return getLocalizedString("itchpad.selectFileLabel"); }, > > + get openFileLabel() { return getLocalizedString("itchpad.openFileLabel"); }, > > + get openFolderLabel() { return getLocalizedString("itchpad.selectFolderLabel"); }, > > +}; > \ No newline at end of file > > Not saying is bad, but why do you need these properties? Can you just use > getLocalizedString everywhere? Just because one of them is used more than once. I don't feel too strongly about it, I removed this and am just using getLocalizedString directly after I added the keybindings. > @@ +282,5 @@ > > + * Promise that is resolved once the project is ready to be used. > > + */ > > + setProjectToSinglePath: function(path, opts = {}) { > > + this.project.customOpts = opts; > > + this.project.projectType = "APP_MANAGER"; > > Feels weird that this is hard coded. Reorganized that part a bit to get rid of project type and use appManagerOpts. Also renamed that to setProjectToAppPath. > ::: browser/devtools/itchpad/lib/plugins/image-view/lib/image-editor.js > @@ +35,5 @@ > > + this.emit("load"); > > + } > > +}); > > + > > +exports.ImageEditor = ImageEditor; > > Could we use a ImageDocument here? Then we could scroll and zoom. > Can we wait until after landing for this? I'll start filing follow up bugs for these things. > ::: browser/devtools/itchpad/lib/plugins/open/lib/open.js > @@ +13,5 @@ > > + > > + init: function(host) { > > + this.command = this.host.addCommand({ > > + id: "cmd-open", > > + key: "o", > > l10n? > > @@ +12,5 @@ > > + > > + init: function(host) { > > + this.command = this.host.addCommand({ > > + id: "find", > > + key: "f", > > l10n > Added entries for localized keybindings in itchpad.properties.

Modified to use new method name for setProjectToAppPath. Asking jryans for review since paul wrote most of this.

Can we pretty please change the name "itchpad" to something little better and of context before landing this ?

(In reply to Girish Sharma [:Optimizer] from comment #57) > Can we pretty please change the name "itchpad" to something little better > and of context before landing this ? This was discussed at some point (I can't find the thread). At that time there was a suggestion to give it a better 'product' name. And the argument against renaming was that it was a component to be used inside of a bigger tool (the webide), instead of a user facing product itself. It sounds like you are suggesting to give it a more widgety name that matches what it is doing. Here is what it does: it lets you open a folder on disk and do basic editing and previewing of the existing files, including adding new / deleting existing files. It will also (in the future) negotiate the pairing of a resource on your disk with the same resource that is opened inside of a web app. I don't have any name suggestions though.

Comment on attachment 8423146 [details] [diff] [review] itchpad-part2.patch Review of attachment 8423146 [details] [diff] [review]: ----------------------------------------------------------------- Seems good to me! ::: browser/devtools/webide/content/webide.js @@ +315,5 @@ > + name: project.name, > + iconUrl: project.icon, > + projectOverviewURL: "chrome://webide/content/details.xhtml" > + }).then(() => { > + console.log("All resources have been loaded", [...itchpad.project.allResources()]); Is this actually useful? We should at least turn this off by default I would think...

Removed extra logging

Don't we want to land part2 only in bug 1011026?

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #61) > Don't we want to land part2 only in bug 1011026? I should probably change the terms there. I'm OK with it being enabled in the (non-enabled) app manager - that would actually help to get more eyes on it. But those are the things that should be fixed before it is shipped along with the app manager.

Comment on attachment 8423145 [details] [diff] [review] itchpad-part1.patch Review of attachment 8423145 [details] [diff] [review]: ----------------------------------------------------------------- Great job reducing the code. I'm personally down with the status bars, but that's just me. Also for names...maybe ProjectEditor? FileEditor? ::: browser/devtools/itchpad/chrome/content/itchpad-loader.js @@ +71,5 @@ > + */ > +function buildTempDirectoryStructure() { > + > + // return FileUtils.getDir("CurProcD", ["chrome", "browser", "content", "browser", "devtools"]).path; > + // return FileUtils.getDir("CurProcD", ["modules", "devtools", "itchpad", "samples", "webapp"]).path; seems like this is old commented-out code. ::: browser/devtools/itchpad/chrome/content/itchpad.xul @@ +9,5 @@ > +<?xml-stylesheet href="chrome://browser/skin/devtools/common.css" type="text/css"?> > +<?xml-stylesheet href="chrome://browser/skin/devtools/widgets.css" type="text/css"?> > +<?xml-stylesheet href="chrome://browser/skin/devtools/debugger.css" type="text/css"?> > +<?xml-stylesheet href="chrome://browser/content/devtools/markup-view.css" type="text/css"?> > +<?xml-stylesheet href="chrome://browser/skin/devtools/markup-view.css" type="text/css"?> Just in case you didn't know, there's debugger and markup view css files here. ::: browser/devtools/itchpad/lib/itchpad.js @@ +31,5 @@ > +require("itchpad/plugins/status-bar/lib/plugin"); > + > +// Disabled Plugins. These are not used right now, but will be > +// needed soon to enable important editing features. > +// require("itchpad/plugins/find-and-replace/lib/plugin"); Suggestion is still to take out what we don't use, and refer to history if we need to. ::: browser/devtools/itchpad/lib/plugins/new/lib/new.js @@ +7,5 @@ > +const { Class } = require("sdk/core/heritage"); > +const { registerPlugin, Plugin } = require("itchpad/plugins/core"); > +const { getLocalizedString } = require("itchpad/helpers/l10n"); > + > +// Handles the save command. "save" -> "new" ::: browser/devtools/itchpad/lib/plugins/save/lib/save.js @@ +26,5 @@ > + modifiers: "accel" > + }); > + > + // Wait until we can add things into the app manager menu > + // this.host.createMenuItem({ Take out commented out code maybe. Can always look at the history.

Rebased and updated based on Comment 63

Rebased

OK here is a push to try with these 2, along with the patch from Bug 1011652, which is still awaiting review. This fixes the chunking issue that was triggered by adding a new folder to the devtools test suite: https://tbpl.mozilla.org/?tree=Try&rev=a0a3c07c2350

This is ready to land pending any renaming. The suggestions I've seen for names are Itchpad, ProjectEditor, and FileEditor

We all know what itchpad is. The code is already here. It's not a name that the user will ever see. Don't rename it.

It's something that we will see.

(In reply to Victor Porof [:vporof][:vp] from comment #69) > It's something that we will see. How so?

At this point, the code is ready to land and the name is not publicly visible - marking checkin-needed. If we come up with a better name at some point we can move things around

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #70) > (In reply to Victor Porof [:vporof][:vp] from comment #69) > > It's something that we will see. > > How so? I don't have a horse in this race, but: can anyone who wasn't around before Firefox 4 figure out which tool hudservice.js belongs to? We could be in a similar state with itchpad.js in a few years.

I just want this to land, what ever the name is. Let's call that `filetree` or `ProjectEditor`.

(In reply to Panos Astithas [:past] from comment #72) > (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from > comment #70) > > (In reply to Victor Porof [:vporof][:vp] from comment #69) > > > It's something that we will see. > > > > How so? > > I don't have a horse in this race, but: can anyone who wasn't around before > Firefox 4 figure out which tool hudservice.js belongs to? We could be in a > similar state with itchpad.js in a few years. One difference here is that there is only one consumer of this right now, and I don't think that there would ever be more than a few. (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #73) > I just want this to land, what ever the name is. Let's call that `filetree` > or `ProjectEditor`. I agree. We've had months to have this discussion - and it has been brought up at least a couple of times before on IRC but without any new suggestions. At this point I just want to land the code one way or another.

Had to rebase. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3ea487a2c6ce

(In reply to Brian Grinstead [:bgrins] from comment #74) > (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from > comment #73) > > I just want this to land, what ever the name is. Let's call that `filetree` > > or `ProjectEditor`. > > I agree. We've had months to have this discussion - and it has been brought > up at least a couple of times before on IRC but without any new suggestions. > At this point I just want to land the code one way or another. +1 for projecteditor This is also a proxy +1 from dcamp and patrick via the scrum.

(In reply to Joe Walker [:jwalker] from comment #76) > (In reply to Brian Grinstead [:bgrins] from comment #74) > > (In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from > > comment #73) > > > I just want this to land, what ever the name is. Let's call that `filetree` > > > or `ProjectEditor`. > > > > I agree. We've had months to have this discussion - and it has been brought > > up at least a couple of times before on IRC but without any new suggestions. > > At this point I just want to land the code one way or another. > > +1 for projecteditor > This is also a proxy +1 from dcamp and patrick via the scrum. Try push with projecteditor: https://tbpl.mozilla.org/?tree=Try&rev=7a8cbf343826

https://hg.mozilla.org/integration/fx-team/rev/9d2ca972053c https://hg.mozilla.org/integration/fx-team/rev/b62cad86a3ad https://tbpl.mozilla.org/?tree=Fx-Team&rev=b62cad86a3ad

Fixes a bc1 orange that I missed on the try push

Pushed fix for bc-1 browser/base/content/test/general/browser_parsable_css.js: https://hg.mozilla.org/integration/fx-team/rev/06607dd59bba https://tbpl.mozilla.org/?tree=Fx-Team&rev=06607dd59bba

https://hg.mozilla.org/mozilla-central/rev/9d2ca972053c https://hg.mozilla.org/mozilla-central/rev/b62cad86a3ad https://hg.mozilla.org/mozilla-central/rev/06607dd59bba

> projecteditor.newLabel=New... For en-US you should always use "…" (single Unicode character) instead of "...". No need to change the string ID to fix it.

Unfortunately I've had to back this out for intermittent test failures on Windows 8: https://tbpl.mozilla.org/php/getParsedLog.php?id=40032240&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=40070328&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=40090222&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=40103586&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=40098568&tree=Mozilla-Central https://tbpl.mozilla.org/php/getParsedLog.php?id=40097633&tree=Mozilla-Central (See https://wiki.mozilla.org/Sheriffing/Test_Disabling_Policy#Exceptions) remote: https://hg.mozilla.org/integration/fx-team/rev/f129e408c2e8 remote: https://hg.mozilla.org/integration/fx-team/rev/43570d095051 remote: https://hg.mozilla.org/integration/fx-team/rev/2736701970ff

Looks like there is a duplicate file being made in the temp directory for testing in windows 8 (here called LICENSE-1). This is also probably also triggering the 'container is undefined' error. TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/projecteditor/test/browser_projecteditor_tree_selection.js | Resources came through in proper order - Got ProjectEditor|css|styles.css|data|img|icons|128x128.png|16x16.png|32x32.png|vector.svg|fake.png|js|script.js|index.html|LICENSE|LICENSE-1|README.md, expected ProjectEditor|css|styles.css|data|img|icons|128x128.png|16x16.png|32x32.png|vector.svg|fake.png|js|script.js|index.html|LICENSE|README.md Ed, have you seen errors on any other platform besides Windows 8?

Just Windows 8 opt-only so far (and the reason this wasn't seen sooner was that fewer of those have been run compared to other platforms due to coalescing/capacity, since this landed).

(In reply to Ed Morley [:edmorley UTC+0] from comment #88) > Just Windows 8 opt-only so far (and the reason this wasn't seen sooner was > that fewer of those have been run compared to other platforms due to > coalescing/capacity, since this landed). I talked with Paul - my plan is to disable Windows Opt tests for now and then to reenable in Bug 1014046. I have a try push out to make sure it is disabled as expected.

Sounds good, thank you :-)

This patch skips non-debug windows tests. It also includes the CSS fix from Attachment 8425674 [details] [diff] and the string change suggested in Comment 84

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=504396fae7a1

https://hg.mozilla.org/integration/fx-team/rev/447a8fd40ae9 https://hg.mozilla.org/integration/fx-team/rev/138fa90154ec https://tbpl.mozilla.org/?tree=Fx-Team&rev=138fa90154ec

https://hg.mozilla.org/mozilla-central/rev/447a8fd40ae9 https://hg.mozilla.org/mozilla-central/rev/138fa90154ec

Is it really needed to use indefinite article in "Open a File", "Select a Folder" and "Select a File"? I'm not native English speaker, but it looks inconsistent with Firefox en-US l10n. Firefox usually uses "Open <something>" and "Select <something>" without indefinite article.

(In reply to Alexander L. Slovesnik from comment #96) > Is it really needed to use indefinite article in "Open a File", "Select a > Folder" and "Select a File"? I'm not native English speaker, but it looks > inconsistent with Firefox en-US l10n. Firefox usually uses "Open > <something>" and "Select <something>" without indefinite article. I'm fine with changing them - these strings are not visible yet, but I've opened Bug 1015225 to enable them and to make the suggested string changes.

The WebIDE is already described at https://developer.mozilla.org/en-US/docs/Tools/WebIDE. Is there something else that needs to be added to the documentation related to this change? Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #98) > The WebIDE is already described at > https://developer.mozilla.org/en-US/docs/Tools/WebIDE. > Is there something else that needs to be added to the documentation related > to this change? No, I don't think so