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)
DevTools Graveyard
WebIDE
Tracking
(firefox40 affected, firefox41 fixed)
RESOLVED
FIXED
Firefox 41
People
(Reporter: callahad, Assigned: waffles, Mentored)
Details
(Keywords: DevAdvocacy, Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 2 obsolete files)
2.30 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•9 years ago
|
||
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]
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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.
Flags: needinfo?(jryans)
Assignee | ||
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
Oh, I didn't know that. I've pushed that to the try server now :) (https://treeherder.mozilla.org/#/jobs?repo=try&revision=b077ec1d4860)
Great, let's land it! :)
Flags: needinfo?(jryans)
Keywords: checkin-needed
Assignee: nobody → wafflespeanut
Status: NEW → ASSIGNED
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d7d2abe89060
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d7d2abe89060
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(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)
Assignee | ||
Comment 18•9 years ago
|
||
Thanks again! And yeah, I've been working on a few bugs. Currently, I've equipped myself with 3 other bugs :)
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•