Closed Bug 1163342 Opened 9 years ago Closed 9 years ago

WebIDE new project wizard silently fails / gets stuck in endless loop if target directory not writable

Categories

(DevTools Graveyard :: WebIDE, defect)

defect
Not set
normal

Tracking

(firefox40 affected, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: callahad, Assigned: waffles, Mentored)

Details

(Keywords: DevAdvocacy, Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 2 obsolete files)

Steps to Reproduce:

1. Open the WebIDE
2. Choose New App, HelloWorld Example, give it the name "hello"
3. Click "OK"
4. In the File Picker that opens, navigate to a folder that you do *not* have write access to, like /System on Mac OS X.
5. Click "OK" to select that folder

What Should Happen:

1. An error appears, informing me that the project directory could not be created.

What Actually Happens:

1. The directory picker disappears, and I'm back at the wizard dialog from Step 3.
I don't see any helpful diagnostic messages when I launch Firefox from the terminal, either.

This happened in a real-world workshop when an attendee accidentally did a `sudo mkdir` rather than `mkdir` when setting up a parent for his FxOS projects.
Yes, let's add an error message for this.  Marking as a good first bug.
Mentor: jryans
Whiteboard: [good first bug][lang=js]
Hey Ryan, I'd like to work on this bug. Can you get me started? BTW, I've done some 2 or 3 bugs. So, we don't have to worry about setting things up. You can get directly get me into the bug. :)
Flags: needinfo?(jryans)
Great!  The failing line is from newapp.js[1] when it tries to create the new folder.

Instead of just calling console.error, I think we should call the main WebIDE window's reportError facility, with something like:

window.parent.UI.reportError(...)

and then close the new app dialog.

Or, alternatively, we could make a space in the new app dialog for showing errors, so that we don't have to close the dialog and lose the selections the user was making.

Let me know if you have further questions!

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/newapp.js#135
Flags: needinfo?(jryans)
Attached patch webIDE (obsolete) — Splinter Review
I tried building Firefox replacing that line. But well, that dialog box "disappearance" can't be fixed. I mean, it doesn't show any effect even after rebuilding the browser. You can have a look at my patch.
Flags: needinfo?(jryans)
Comment on attachment 8605204 [details] [diff] [review]
webIDE

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

For the next patch, it's fine to use feedback? or review? on the patch instead of needinfo?.  Just as long as some flag gets set, so I don't forget to reply. :)

::: browser/devtools/webide/content/newapp.js
@@ +133,5 @@
>  
>    try {
>      folder.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
>    } catch(e) {
> +    window.parent.UI.reportError(e);

Ah, you are right, my mistake!

In this case, you can use:

let win = Services.wm.getMostRecentWindow("devtools:webide");

to get the main WebIDE window.

from there, to report the error:

win.UI.reportError(<SOME STRING ID>);

You'll need to add a new string in webide.properties for the error message.

It will appear behind the dialog.  We could either close the current dialog, or just let it be behind the open dialog.
Attached patch webIDE (obsolete) — Splinter Review
Okay, that took care of the error. Now, a nice dropdown thing just shows up indicating an error. BTW, what do you think of my <error variable> and its string? (I admit, I'm not that good with naming stuff). And, I think it might be better to close the dialog, because it's possible that the user might miss the warning, as it occurs in the inactive window behind. Well, I can remove it if you want. :)
Attachment #8605204 - Attachment is obsolete: true
Attachment #8605356 - Flags: review?(jryans)
Comment on attachment 8605356 [details] [diff] [review]
webIDE

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

The changes look good to me!  But, something is wrong with your patch's header, it seems to include an error message or something:

message. HG: -- HG: user: Ravi Shankar <wafflespeanut@gmail.com> HG: branch 'default' HG: changed
browser/devtools/webide/content/newapp.js

If you do not yet have try access, I can submit your patch once there's a fixed version attached.  (You should consider applying for access!)
Attachment #8605356 - Flags: review?(jryans) → review+
Attached patch webIDESplinter Review
Yeah, I didn't notice it until now. I've cleared it now, and I've also pushed it to the try-server for testing it on all platforms (https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e478bc12e10). I've got a question now. Should this test be ran using xpcshell or mochitest? I wasn't sure, and so I left it.
Attachment #8605356 - Attachment is obsolete: true
Attachment #8606601 - Flags: review?(jryans)
Comment on attachment 8606601 [details] [diff] [review]
webIDE

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

Okay, the cleaned up patch header looks good!

Typically for most front end DevTools work (like WebIDE), I'll use the following try syntax:

-b do -p linux64,macosx64,win32 -u mochitest-dt,mochitest-o,mochitest-e10s-devtools-chrome -t none
Attachment #8606601 - Flags: review?(jryans) → review+
Oh, I didn't know that. I've pushed that to the try server now :)
(https://treeherder.mozilla.org/#/jobs?repo=try&revision=b077ec1d4860)
The try-server builds are successful :)
Flags: needinfo?(jryans)
Great, let's land it! :)
Flags: needinfo?(jryans)
Keywords: checkin-needed
Assignee: nobody → wafflespeanut
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/d7d2abe89060
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Awesome! Thanks for mentoring me Ryan :)
Flags: needinfo?(jryans)
(In reply to Ravi Shankar [:waffles] from comment #16)
> Awesome! Thanks for mentoring me Ryan :)

Sure, it was good working with you on this!  I know you've already looked at a few other bugs, so perhaps you've already found some others to work on, but feel free to attempt whatever sounds good.  You can also ask in #devtools for more specific advice.
Flags: needinfo?(jryans)
Thanks again! And yeah, I've been working on a few bugs. Currently, I've equipped myself with 3 other bugs :)
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: