Closed Bug 657595 Opened 10 years ago Closed 9 years ago

GCLI type conversion should be a type->type affair not just arg->type

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: jwalker, Assigned: jwalker)

References

Details

Attachments

(1 file, 2 obsolete files)

This will help while converting data for output display.
Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.
Component: Developer Tools → Developer Tools: Console
Triage. Filter on PEGASUS.
Priority: -- → P3
No longer blocks: GCLI-FUTURE
GCLI Triage.
Target Milestone: --- → Firefox 16
Target Milestone: Firefox 16 → Firefox 17
Triage, filter on TEABAGS
Target Milestone: Firefox 17 → Future
New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"
Component: Developer Tools: Console → Developer Tools: Graphic Commandline and Toolbar
Attached patch v1 (obsolete) — Splinter Review
The changes to gcli.jsm should be familiar to you.
Assignee: nobody → jwalker
Status: NEW → ASSIGNED
Attachment #727626 - Flags: review?(mratcliffe)
Comment on attachment 727626 [details] [diff] [review]
v1

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

Looks good to me, r+.

::: browser/devtools/commandline/gcli.jsm
@@ +10903,5 @@
>  
>      promisedDirectTabText.resolve(directTabText);
>      promisedArrowTabText.resolve(arrowTabText);
>      promisedEmptyParameters.resolve(emptyParameters);
> +  }.bind(this), onError);

Should this not be:
}.bind(this), util.errorHandler);
?
Attachment #727626 - Flags: review?(mratcliffe) → review+
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #10)
> Comment on attachment 727626 [details] [diff] [review]
> v1
> 
> Review of attachment 727626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, r+.
> 
> ::: browser/devtools/commandline/gcli.jsm
> @@ +10903,5 @@
> >  
> >      promisedDirectTabText.resolve(directTabText);
> >      promisedArrowTabText.resolve(arrowTabText);
> >      promisedEmptyParameters.resolve(emptyParameters);
> > +  }.bind(this), onError);
> 
> Should this not be:
> }.bind(this), util.errorHandler);
> ?

Good spot, but in this case no. We don't want to see the error message, we want to pass it on to the promises. onError is a function to do that:

  // If anything goes wrong, we pass the error on to all the child promises
  var onError = function(ex) {
    promisedDirectTabText.reject(ex);
    promisedArrowTabText.reject(ex);
    promisedEmptyParameters.reject(ex);
  };

Thanks
Attached patch v2 (obsolete) — Splinter Review
Try breakage was down to forgetting to copy the helpers.js changes across.
Attachment #727626 - Attachment is obsolete: true
Attached patch v3Splinter Review
Comment out a failing test.

That's OK we're fixing it in bug 784790 which is following this one down the landing strip.
Attachment #728117 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f952eebd4cbd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Depends on: 1032696
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.