Joe Walker [:jwalker] Assignee
	Description • 13 years ago

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

	Joe Walker [:jwalker] Assignee
	Comment 1 • 13 years ago

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.

	Joe Walker [:jwalker] Assignee
	Comment 2 • 13 years ago

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

	Joe Walker [:jwalker] Assignee
	Comment 3 • 13 years ago

Triage. Filter on PEGASUS.

	Joe Walker [:jwalker] Assignee
	Comment 4 • 13 years ago

Triage. Filter on PEGASUS.

	Joe Walker [:jwalker] Assignee
	Comment 5 • 13 years ago

Triage. Filter on PEGASUS.

	Grant Gayed
	Comment 7 • 12 years ago

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.

	Joe Walker [:jwalker] Assignee
	Comment 8 • 12 years ago

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

	Joe Walker [:jwalker] Assignee
	Comment 9 • 12 years ago

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

	Joe Walker [:jwalker] Assignee
	Comment 10 • 12 years ago

This bug was not fixed with Firefox 18.

	Joe Walker [:jwalker] Assignee
	Comment 11 • 12 years ago

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.

	Joe Walker [:jwalker] Assignee
	Comment 12 • 12 years ago

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.

	Joe Walker [:jwalker] Assignee
	Comment 13 • 12 years ago

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

	Joe Walker [:jwalker] Assignee
	Comment 14 • 12 years ago

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.

	Joe Walker [:jwalker] Assignee
	Comment 15 • 12 years ago

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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 16 • 12 years ago

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?

	Heather Arthur [:harth]
	Comment 17 • 12 years ago

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.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 18 • 12 years ago

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.

	Joe Walker [:jwalker] Assignee
	Comment 19 • 12 years ago

(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.
   */

	Joe Walker [:jwalker] Assignee
	Comment 20 • 12 years ago

(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.

	Joe Walker [:jwalker] Assignee
	Comment 21 • 12 years ago

(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.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 22 • 12 years ago

(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.

	Mihai Sucan [:msucan]
	Comment 23 • 12 years ago

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

	Joe Walker [:jwalker] Assignee
	Comment 24 • 12 years ago

(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.

	Joe Walker [:jwalker] Assignee
	Comment 25 • 12 years ago

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

	Joe Walker [:jwalker] Assignee
	Comment 26 • 12 years ago

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

	Joe Walker [:jwalker] Assignee
	Comment 27 • 12 years ago

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

	Joe Walker [:jwalker] Assignee
	Comment 28 • 12 years ago

See bug 849168 for screenshot/try problem.

	Joe Walker [:jwalker] Assignee
	Comment 29 • 12 years ago

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

	Joe Walker [:jwalker] Assignee
	Comment 30 • 12 years ago

Comment on attachment 722211 [details] [diff] [review]
Promise hacks

These are the workarounds for the promise stack issues.

	Joe Walker [:jwalker] Assignee
	Comment 31 • 12 years ago

Patch to land.

	Joe Walker [:jwalker] Assignee
	Comment 32 • 12 years ago

Fixes completion display issue

	Joe Walker [:jwalker] Assignee
	Comment 33 • 12 years ago

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

	Joe Walker [:jwalker] Assignee
	Comment 34 • 12 years ago

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

	Paul Rouget [:paul]
	Comment 35 • 12 years ago

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