1059930 - Attempting to open the same packaged app twice in WebIDE results in generic error

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[lyndsey]

To reproduce, open [hamburger/three-bar menu] > Developer > WebIDE. Select Project > Open Packaged App... and use the menu to browse for a packaged app. Repeat the opening of the packaged app, selecting exactly the same directory.

The WebIDE provides an informational popup with the error "Operation failed: importing packaged app". Inspecting the Ctrl-Shift-J console reveals an additional error: "Already added."

I would expect that this visual pop-up would give more specific information about the error, or, better yet, the WebIDE would open the selected project even though it has already been added.

Comment 2

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

Jordan, this might be another good one to try.  Let me know if you need any help!

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Looks like the UI waiting code just displays the operation name, not the failure, when something goes wrong. Scoping it out, but this patch should provide more detail on all errors, not just this one, so +1

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Any ideas, preferences on how to display the error message, if an error is supplied during the process?

"Operation Failed: [OPERATION]: [ERROR]"
"Operation Failed: connecting to runtime: Already added"

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

More of a general error UI update, with also feature of opening the existing app.

Comment 6

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

(In reply to Jordan Santell [:jsantell] [@jsantell] [PTO until September 4th] from comment #4)
> Any ideas, preferences on how to display the error message, if an error is
> supplied during the process?
> 
> "Operation Failed: [OPERATION]: [ERROR]"
> "Operation Failed: connecting to runtime: Already added"

Seems good!  Even fancier might be a "more details..." link that triggers the Browser Console to open (in case there's a stack trace or whatever), but then we're really moving away from this bug's main issue. :)

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1059930-webide-dupe-apps.patch

Includes better error printing, and selecting the already existing app when attempting to add a duplicate. The error is still shown, but we can hide this if that's more desirable.

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1059930-webide-dupe-apps.patch

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

https://tbpl.mozilla.org/?tree=Try&rev=a3bec24c688b

Comment 10

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

Comment on attachment 8485245 [details] [diff] [review]
1059930-webide-dupe-apps.patch

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

Looks good overall, just a few small things to tweak.

::: browser/devtools/webide/content/webide.js
@@ +997,5 @@
>      UI.selectDeckPanel("prefs");
>    },
> +};
> +
> +function getPackagedDirectory (location) {

I'd say let's put these new functions on the UI singleton, instead of having them as standalone functions.  At least that way we're not leaving more names on the global namespace than we need to.

@@ +1047,5 @@
> +    } catch (e) {
> +      if (e === "Already added") {
> +        // Select project that's already been added,
> +        // and throw to display "Already added" message
> +        AppManager.selectedProject = AppProjects.get(isPackaged ? source.path : source);

Assuming we change to stop throwing below, then we can just set the local |project| to this value and continue the code that will eventually set |AppManager.selectedProject|.

@@ +1048,5 @@
> +      if (e === "Already added") {
> +        // Select project that's already been added,
> +        // and throw to display "Already added" message
> +        AppManager.selectedProject = AppProjects.get(isPackaged ? source.path : source);
> +        throw e;

Since we're able to resolve the situation by selecting the existing project, let's skip throwing so there's no longer an error message.

::: browser/devtools/webide/test/head.js
@@ +84,5 @@
>  
>  function removeAllProjects() {
>    return Task.spawn(function* () {
>      yield AppProjects.load();
> +    // use a new array so we're not iterating over the same

Thanks for fixing this, hopefully it wasn't too annoying to debug...

I also had a fix in bug 916084, but I can never get that patch to land. :(

::: browser/devtools/webide/test/test_duplicate_import.html
@@ +18,5 @@
> +      window.onload = function() {
> +        SimpleTest.waitForExplicitFinish();
> +
> +        Task.spawn(function* () {
> +            let win = yield openWebIDE();

Nit: All these lines should be indented 2 spaces, not 4.  I am pretty sure some other file Paul added is doing this wrong, so he's to blame. ;)

@@ +36,5 @@
> +
> +            info("to call importPackagedApp(" + packagedAppLocation + ") again");
> +            let failed = null;
> +            try {
> +                yield win.Cmds.importPackagedApp(packagedAppLocation);

Nit: Again, only 2 spaces.

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

https://tbpl.mozilla.org/?tree=Try&rev=88b90f879486

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1059930-webide-dupe-apps.patch

Alright, added utils and addressed comments

Comment 13

[Paul Rouget [:paul]]

(In reply to J. Ryan Stinnett [:jryans] from comment #10)
> ::: browser/devtools/webide/test/head.js
> @@ +84,5 @@
> >  
> >  function removeAllProjects() {
> >    return Task.spawn(function* () {
> >      yield AppProjects.load();
> > +    // use a new array so we're not iterating over the same
> 
> Thanks for fixing this, hopefully it wasn't too annoying to debug...
> 
> I also had a fix in bug 916084, but I can never get that patch to land. :(

Which bug? (wrong bug number)

> ::: browser/devtools/webide/test/test_duplicate_import.html
> @@ +18,5 @@
> > +      window.onload = function() {
> > +        SimpleTest.waitForExplicitFinish();
> > +
> > +        Task.spawn(function* () {
> > +            let win = yield openWebIDE();
> 
> Nit: All these lines should be indented 2 spaces, not 4.  I am pretty sure
> some other file Paul added is doing this wrong, so he's to blame. ;)

erf :)

Comment 14

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

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #13)
> (In reply to J. Ryan Stinnett [:jryans] from comment #10)
> > ::: browser/devtools/webide/test/head.js
> > @@ +84,5 @@
> > >  
> > >  function removeAllProjects() {
> > >    return Task.spawn(function* () {
> > >      yield AppProjects.load();
> > > +    // use a new array so we're not iterating over the same
> > 
> > Thanks for fixing this, hopefully it wasn't too annoying to debug...
> > 
> > I also had a fix in bug 916084, but I can never get that patch to land. :(
> 
> Which bug? (wrong bug number)

Ah, I meant the telemetry patch (bug 916804).

Comment 15

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

Comment on attachment 8486091 [details] [diff] [review]
1059930-webide-dupe-apps.patch

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

Your new test passes, but because they skip the modal UI behavior (like our other tests), it misses the path that a real user hits.  The UI prompts need |window|, which isn't accessible in the new |utils.js|.

::: browser/devtools/webide/modules/utils.js
@@ +15,5 @@
> +function getPackagedDirectory (location) {
> +  let directory;
> +  if (!location) {
> +    let fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
> +    fp.init(window, Strings.GetStringFromName("importPackagedApp_title"), Ci.nsIFilePicker.modeGetFolder);

|window| is no longer available in this new module, so calling this throws an error.

Perhaps pass in |window| as an extra parameter to this function?  Or else you could move this back to the |webide.js| file instead.

@@ +31,5 @@
> +function getHostedURL (location) {
> +  let ret = { value: null };
> +
> +  if (!location) {
> +    Services.prompt.prompt(window,

|window| missing again.

::: browser/devtools/webide/test/test_duplicate_import.html
@@ +18,5 @@
> +      window.onload = function() {
> +        SimpleTest.waitForExplicitFinish();
> +
> +        Task.spawn(function* () {
> +        let win = yield openWebIDE();

Nit: Indent block of task 2 spaces

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1059930-webide-dupe-apps.patch

Ahh, here we go -- made all the above changes

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

https://tbpl.mozilla.org/?tree=Try&rev=3314c24325c2

Comment 18

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

Comment on attachment 8489449 [details] [diff] [review]
1059930-webide-dupe-apps.patch

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

Cool, this looks good to me!

Comment 19

[Paul Rouget [:paul]]

Should this land?

Comment 20

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

Yeah, though may need a rebase at this point...

Comment 21

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1059930-webide-dupe-apps.patch

Refactored!

Comment 22

[Jordan Santell [:jsantell] [@jsantell]]

Err, rebased

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/b866658e2e67

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b866658e2e67