987089 - Land ProjectEditor in browser/devtools

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Girish Sharma [:Optimizer]]

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

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: itchpad-WIP.patch

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.

Comment 3

[Brian Grinstead [:bgrins]]

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`

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: itchpad-WIP.patch

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

Comment 5

[Heather Arthur [:harth]]

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 6

[Joe Walker [:jwalker]]

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 ([e-mail])

Comment 7

[Paul Rouget [:paul]]

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

Comment 8

[Paul Rouget [:paul]]

(erf, no, sorry)

Comment 9

[Paul Rouget [:paul]]

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

Comment 10

[Paul Rouget [:paul]]

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?

Comment 11

[Paul Rouget [:paul]]

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

Comment 12

[Brian Grinstead [:bgrins]]

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

Comment 13

[Paul Rouget [:paul]]

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");

Comment 14

[Paul Rouget [:paul]]

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;

Comment 15

[Paul Rouget [:paul]]

Can I get an overview of the Project API?

Comment 16

[Paul Rouget [:paul]]

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

Comment 17

[Paul Rouget [:paul]]

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

Comment 18

[Brian Grinstead [:bgrins]]

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

Comment 19

[Paul Rouget [:paul]]

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

Comment 20

[Brian Grinstead [:bgrins]]

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

Comment 21

[Brian Grinstead [:bgrins]]

Attachment: itchpad-WIP2.patch

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"
      });
    });

Comment 22

[Paul Rouget [:paul]]

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

Comment 23

[Paul Rouget [:paul]]

File names should not wrap when the sidebar is resized.

Comment 24

[Paul Rouget [:paul]]

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

Comment 25

[Paul Rouget [:paul]]

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

Comment 26

[Paul Rouget [:paul]]

I've updated fx-team, and now I get:

> Error: Module `itchpad/itchpad` is not found at resource://gre/modules/commonjs/itchpad/itchpad.js

Comment 27

[Paul Rouget [:paul]]

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

Comment 28

[Brian Grinstead [:bgrins]]

Attachment: itchpad-WIP3.patch

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

Comment 29

[Brian Grinstead [:bgrins]]

Attachment: itchpad.patch

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 30

[Paul Rouget [:paul]]

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.

Comment 31

[Brian Grinstead [:bgrins]]

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

Comment 32

[Brian Grinstead [:bgrins]]

Attachment: itchpad-review-changes.patch

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.

Comment 33

[Brian Grinstead [:bgrins]]

Attachment: itchpad-2.patch

Folded patch

Comment 34

[Paul Rouget [:paul]]

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

Comment 35

[Paul Rouget [:paul]]

Attachment: itchpad integration in app manager v2

Comment 36

[Paul Rouget [:paul]]

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

Comment 37

[Brian Grinstead [:bgrins]]

Attachment: itchpad-part1.patch

Patch that lands itchpad in browser/devtools

Comment 38

[Brian Grinstead [:bgrins]]

Attachment: itchpad-part2.patch

Patch that integrates itchpad with the app manager v2

Comment 39

[Paul Rouget [:paul]]

Attachment: integration with app manager

Rebased + minor fixes (support for toggling itchpad).

Comment 40

[Paul Rouget [:paul]]

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

Comment 41

[Paul Rouget [:paul]]

Also, a xul:tree works in RTL.

Comment 42

[Girish Sharma [:Optimizer]]

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

Comment 43

[Paul Rouget [:paul]]

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

Comment 44

[Girish Sharma [:Optimizer]]

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

Comment 45

[Paul Rouget [:paul]]

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

Comment 46

[Brian Grinstead [:bgrins]]

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

Comment 47

[Paul Rouget [:paul]]

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

Comment 48

[Paul Rouget [:paul]]

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

Comment 49

[Paul Rouget [:paul]]

Should I start reviewing this patch or should I wait?

Comment 50

[Brian Grinstead [:bgrins]]

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.

Comment 51

[Brian Grinstead [:bgrins]]

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

[Paul Rouget [:paul]]

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?

Comment 53

[Brian Grinstead [:bgrins]]

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

Comment 54

[Brian Grinstead [:bgrins]]

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.

Comment 55

[Brian Grinstead [:bgrins]]

Attachment: itchpad-part1.patch

Comment 56

[Brian Grinstead [:bgrins]]

Attachment: itchpad-part2.patch

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

Comment 57

[Girish Sharma [:Optimizer]]

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

Comment 58

[Brian Grinstead [:bgrins]]

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

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 60

[Brian Grinstead [:bgrins]]

Attachment: itchpad-part2.patch

Removed extra logging

Comment 61

[Paul Rouget [:paul]]

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

Comment 62

[Brian Grinstead [:bgrins]]

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

[Heather Arthur [:harth]]

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.

Comment 64

[Brian Grinstead [:bgrins]]

Attachment: itchpad-part1-r=paul.patch

Rebased and updated based on Comment 63

Comment 65

[Brian Grinstead [:bgrins]]

Attachment: itchpad-part2-r=jryans.patch

Rebased

Comment 66

[Brian Grinstead [:bgrins]]

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

Comment 67

[Brian Grinstead [:bgrins]]

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

Comment 68

[Paul Rouget [:paul]]

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.

Comment 69

[Victor Porof [:vporof][:vp]]

It's something that we will see.

Comment 70

[Paul Rouget [:paul]]

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

How so?

Comment 71

[Brian Grinstead [:bgrins]]

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

Comment 72

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 73

[Paul Rouget [:paul]]

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

Comment 74

[Brian Grinstead [:bgrins]]

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

Comment 75

[Brian Grinstead [:bgrins]]

Attachment: itchpad-part1-r=paul.patch

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

Comment 76

[Joe Walker [:jwalker]]

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

Comment 77

[Brian Grinstead [:bgrins]]

(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

Comment 78

[Brian Grinstead [:bgrins]]

Attachment: projecteditor-part1-r=paul.patch

Comment 79

[Brian Grinstead [:bgrins]]

Attachment: projecteditor-part2-r=jryans.patch

Comment 80

[Brian Grinstead [:bgrins]]

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

Comment 81

[Brian Grinstead [:bgrins]]

Attachment: projecteditor-css-fix.patch

Fixes a bc1 orange that I missed on the try push

Comment 82

[Brian Grinstead [:bgrins]]

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

Comment 83

[Carsten Book [:Tomcat]]

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

Comment 84

[Francesco Lodolo [:flod]]

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

Comment 85

[Ed Morley [:emorley]]

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

Comment 86

[Ed Morley [:emorley]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=40104538&tree=Mozilla-Central

Comment 87

[Brian Grinstead [:bgrins]]

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?

Comment 88

[Ed Morley [:emorley]]

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

Comment 89

[Brian Grinstead [:bgrins]]

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

Comment 90

[Ed Morley [:emorley]]

Sounds good, thank you :-)

Comment 91

[Ed Morley [:emorley]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=40112906&tree=Fx-Team

Comment 92

[Brian Grinstead [:bgrins]]

Attachment: projecteditor-part-1-r=paul.patch

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

Comment 93

[Brian Grinstead [:bgrins]]

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

Comment 94

[Brian Grinstead [:bgrins]]

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

Comment 95

[Carsten Book [:Tomcat]]

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

Comment 96

[Alexander L. Slovesnik]

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.

Comment 97

[Brian Grinstead [:bgrins]]

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

Comment 98

[Sebastian Zartner [:sebo]]

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

Comment 99

[Brian Grinstead [:bgrins]]

(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