Predictions could use XHR to find directory contents from a server for example.

Things that need completion: - filenames (could be filtered (e.g. dirs only/type x only) and could be new in which case they are unpredictable) - css strings - nodes by id - page resources - prefs - javascript properties - commands - true/false - parameter supplied values - web pages / domains What are the differences between these sets? - size: There are many prefs/filenames, but there are few boolean values. At the low end this affects UI choice (checkbox, radio, select) but at the high end it could make a theoretically bounded set unbounded due to memory constraints - sync / async: Some resources may require XHR to access and can not be discovered asynchronously, others can - streamed: Some sets, once known are entirely known, some sets may only become known in dribs and drabs. All streamed sets must be asynchronous - bounded / unbounded: The set existing filenames is bounded, the set of potential filenames (i.e. for saveas) is not - ordering: Some sets are inherently ordered (perhaps alphabetically) some by historical use (commands) and some by context (the most likely boolean value in a 'set' command is the opposite of the current value) - matching: For some sub-string matching is vital (urls) and for others (commands) it's less important - synonyms: yes/no are useful synonyms for true/false, files could have canonical representations other than the one typed, however prefs are inherently canonical - changing: some resources change over time (files, commands) and the set of values isn't fully known when the type is determined, but is known at display time, may change during display The exchange between the UI and the assignment is basically: - Give me basic details which help me configure how I should look. Currently this is done by allowing the fields to claim a type without reference to the parameter that uses the type. It's like this because types can be nested (e.g. array parameters), however we could add knowledge of the parameter in - Here's what the user has typed - When you have suggestions, send them here Perhaps all that is needed is for us to remove Assignment.getPredictions and instead have Assignment.[add|remove]PredictionListener where a PredictionListener has clear|add|remove functions.

Moving GCLI bugs to Developer Tools: Console. Filter on 'baked beans are off'.

Triage. Filter on PEGASUS.

Triage. Filter on PEGASUS.

Triage. Filter on PEGASUS.

FWIW this blocks Orion plug-ins from contributing useful custom types because they're not able to provide completion suggestions quickly enough (even if asynchronous computation is not required) since plug-ins run in a separate iframe that is accessed via window.postMessage.

Thanks Grant. Realistically, with current resource this isn't going to get done this cycle (ff17) but I've marked it for ff18.

New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"

This bug was not fixed with Firefox 18.

Patches inbound. I'm going to split them up for review purposes, but combine them before committing because they unit tests do not pass with half of them applied.

This code is being reviewed in https://github.com/campd/gcli/pull/65. I will ask for r+ here when the review in that pull request is done or near done, and when we've got r+ in the other parts of this patch.

This patch: - updates the commandline tests to use the new async version of helpers.js - removes the private browsing hack - adds a createEnvironment helper to CommandUtils

This patch does the following: - Move commands from commandline to debugger - Introduce a getPanel helper to abstract some code from the commands - Update the unit tests to the new async helpers.js - Comment out browser_dbg_cmd_break.js for bug 722727 Previously browser_dbg_cmd_break.js test didn't run properly as a result of a bug. I fixed that bug, but don't want to enable the test while there is an underlying problem I have a patch in progress for but 722727, but it's not there yet, and I don't want to hold this up.

This patch makes the following changes: - Update the unit tests to the new async helpers.js for the following: inspector, responsivedesign, styleeditor - Tidy up tests in 'shared'; lots of waste, helpers.js not needed, etc - Fix the "edit" command

Comment on attachment 719427 [details] [diff] [review] Changes to commandline (part 2). Upload 1 Review of attachment 719427 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Just one nit, an incomplete comment. ::: browser/devtools/shared/DeveloperToolbar.jsm @@ +142,5 @@ > + }, > + > + /** > + * A helper function to create > + */ To create what?

Comment on attachment 719434 [details] [diff] [review] Changes to other sections (part 4). Upload 1 Review of attachment 719434 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. A couple small things and a couple "fagan" comments about naming that are just suggestions. ::: browser/devtools/inspector/test/browser_inspector_cmd_inspect.js @@ +7,5 @@ > "test/browser_inspector_cmd_inspect.html"; > > function test() { > + helpers.withToolbarTab(TEST_URI, function(options) { > + return helpers.audit(options, [ Doesn't need changing, but fyi on a glance it's hard to tell what this test is doing, "audit" is vague. ::: browser/devtools/inspector/test/helpers.js @@ +59,2 @@ > */ > +helpers.withTab = function(url, callback, options) { The name "withTab" doesn't really imply that it will be opening a new tab, which is a big part of what it does. @@ +59,5 @@ > */ > +helpers.withTab = function(url, callback, options) { > + var deferred = Promise.defer(); > + > + waitForExplicitFinish(); Calling`waitForExplicitFinish()` in this helper function in a separate module seems like it could cause confusion, especially when the matching `finish()`is in the test itself. ::: browser/devtools/styleeditor/CmdEdit.jsm @@ +42,5 @@ > exec: function(args, context) { > + let target = context.environment.target; > + return gDevTools.showToolbox(target, "styleeditor").then(function(toolbox) { > + let styleEditor = toolbox.getCurrentPanel(); > + styleEditor.selectStyleSheet(args.resource.element, args.line, 0); change '0' -> '1' , or leave it out, because it's 1-indexed. I think this works by chance, but would be good to change.

Comment on attachment 719433 [details] [diff] [review] Changes to debugger (part 3). Upload 1 Review of attachment 719433 [details] [diff] [review]: ----------------------------------------------------------------- Can't helpers.js be placed into shared/ and reused across all of our tools? Aren't its contents identical in all directories? ::: browser/devtools/debugger/test/Makefile.in @@ +10,5 @@ > > include $(DEPTH)/config/autoconf.mk > > MOCHITEST_BROWSER_TESTS = \ > + $(browser_dbg_cmd_break.js disabled until bug 722727 is fixed) \ If the test is bogus but passes, you may want to leave it enabled just to avoid bitrot that you will have to deal with later. Your call, obviously.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #16) > ::: browser/devtools/shared/DeveloperToolbar.jsm > @@ +142,5 @@ > > + }, > > + > > + /** > > + * A helper function to create > > + */ > > To create what? Looks like it's good at creating review comments :) Now: /** * A helper function to create the environment object that is passed to * GCLI commands. */

(In reply to Panos Astithas [:past] from comment #18) > Comment on attachment 719433 [details] [diff] [review] > Changes to debugger (part 3). Upload 1 > > Review of attachment 719433 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can't helpers.js be placed into shared/ and reused across all of our tools? > Aren't its contents identical in all directories? They are. I followed the instructions in the mochitest docs, but couldn't get it to work and after an hour or so of failing, I decided to tackle it later. I raised bug 846696 so we don't forget. > ::: browser/devtools/debugger/test/Makefile.in > @@ +10,5 @@ > > > > include $(DEPTH)/config/autoconf.mk > > > > MOCHITEST_BROWSER_TESTS = \ > > + $(browser_dbg_cmd_break.js disabled until bug 722727 is fixed) \ > > If the test is bogus but passes, you may want to leave it enabled just to > avoid bitrot that you will have to deal with later. Your call, obviously. The test was bogus and the code it tested was broken. The test is no longer bogus, but the code it tests is still broken, so I think I need to leave it commented out. Thanks.

(In reply to Heather Arthur [:harth] from comment #17) > Comment on attachment 719434 [details] [diff] [review] > Changes to other sections (part 4). Upload 1 > > Review of attachment 719434 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. A couple small things and a couple "fagan" comments about naming > that are just suggestions. > > ::: browser/devtools/inspector/test/browser_inspector_cmd_inspect.js > @@ +7,5 @@ > > "test/browser_inspector_cmd_inspect.html"; > > > > function test() { > > + helpers.withToolbarTab(TEST_URI, function(options) { > > + return helpers.audit(options, [ > > Doesn't need changing, but fyi on a glance it's hard to tell what this test > is doing, "audit" is vague. Hmm, yes. I'll see if I can think of something better, but off the top of my head I'm a bit stuck. Perhaps 'performAudit' might be clearer? > ::: browser/devtools/inspector/test/helpers.js > @@ +59,2 @@ > > */ > > +helpers.withTab = function(url, callback, options) { > > The name "withTab" doesn't really imply that it will be opening a new tab, > which is a big part of what it does. Yeah - this replaced an addTab method, and they co-existed for a while, but now that it's gone, I guess I can revert to the old name. > @@ +59,5 @@ > > */ > > +helpers.withTab = function(url, callback, options) { > > + var deferred = Promise.defer(); > > + > > + waitForExplicitFinish(); > > Calling`waitForExplicitFinish()` in this helper function in a separate > module seems like it could cause confusion, especially when the matching > `finish()`is in the test itself. This is tricky. You're right about the confusion, however withTab can't work without waitForExplicitFinish() so it seems cruel to force users of withTab call waitForExplicitFinish() themselves. I took a look at what other tests do. It seems that debugger, webconsole, styleinspector, shared and framework all have some implicit call to waitForExplicitFinish where the others make it explicit. So I'm torn. Anyone else have opinions? > ::: browser/devtools/styleeditor/CmdEdit.jsm > @@ +42,5 @@ > > exec: function(args, context) { > > + let target = context.environment.target; > > + return gDevTools.showToolbox(target, "styleeditor").then(function(toolbox) { > > + let styleEditor = toolbox.getCurrentPanel(); > > + styleEditor.selectStyleSheet(args.resource.element, args.line, 0); > > change '0' -> '1' , or leave it out, because it's 1-indexed. I think this > works by chance, but would be good to change. Done.

(In reply to Joe Walker [:jwalker] from comment #21) > (In reply to Heather Arthur [:harth] from comment #17) > > @@ +59,5 @@ > > > */ > > > +helpers.withTab = function(url, callback, options) { > > > + var deferred = Promise.defer(); > > > + > > > + waitForExplicitFinish(); > > > > Calling`waitForExplicitFinish()` in this helper function in a separate > > module seems like it could cause confusion, especially when the matching > > `finish()`is in the test itself. > > This is tricky. You're right about the confusion, however withTab can't work > without waitForExplicitFinish() so it seems cruel to force users of withTab > call waitForExplicitFinish() themselves. > > I took a look at what other tests do. It seems that debugger, webconsole, > styleinspector, shared and framework all have some implicit call to > waitForExplicitFinish where the others make it explicit. > > So I'm torn. Anyone else have opinions? That's right, we call waitForExplicitFinish in head.js in the debugger. Are there classes of tests that don't need it at all? I'm kind of surprised it's not common to all of our tools.

waitForExplicitFinish() should be called by each test, according to bug 610953.

(In reply to Mihai Sucan [:msucan] from comment #23) > waitForExplicitFinish() should be called by each test, according to bug > 610953. Gavin might be complaining about having head.js call waitForExplicitFinish() at load time, which is understandable - it means that all tests are forced to call finish explicitly. What this does is to call waitForExplicitFinish only when addTab/withTab is called - which you need to do anyway because that function is inherently async. Also over 2 years at NEW, might mean we don't care about the advice too much! -- I propose to do nothing right now, and put it on the agenda for next Thursday. There doesn't seem to be much consensus and I'm a bit reluctant to change quite a bit of code for a marginal thing.

This patch is a combination of the patches split out for review. It contains the review fixes plus a couple of test fixes.

This patch contains a couple of hacks to work around the promise stack problem from bug 842347.

See bug 849168 for screenshot/try problem.

Comment on attachment 722211 [details] [diff] [review] Promise hacks These are the workarounds for the promise stack issues.

Patch to land.

Fixes completion display issue

https://hg.mozilla.org/integration/fx-team/rev/a9d191946bb2 https://tbpl.mozilla.org/?tree=Fx-Team&rev=a9d191946bb2