Closed Bug 842682 Opened 11 years ago Closed 10 years ago

Use the debugger API for autocomplete and provide suggestions from the selected stackframe scope

Categories

(DevTools :: Console, defect, P3)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Firefox 28

People

(Reporter: msucan, Assigned: chstath)

References

Details

(Whiteboard: [autocomplete])

Attachments

(1 file, 4 obsolete files)

Use the debugger API for autocomplete suggestions
34.11 KB, patch
Details | Diff | Splinter Review 
Follow-up from bug 783499: we did not move the autocomplete over to the debugger API because it works as it is while the debugger is paused.

As soon as possible we should move the autocomplete implementation over to the debugger API and provide completion suggestions for the currently selected stackframe (from the debugger).
It would be really easy to produce a list of local names in scope, using frame.environment, environment.names, and environment.parent. Try out this code in the JavaScript shell:

var g = newGlobal();

var dbg = new Debugger(g);

dbg.onDebuggerStatement = function (frame) {
  print("locals:");
  for (let env = frame.environment; env; env = env.parent) {
    print(env.names());
  }
}

g.eval("function f(arg1, arg2) {                                      \n" +
       "  var var1, var2;                                             \n" +
       "  return function g(arg3, arg4) {                             \n" +
       "    var var3, var4;                                           \n" +
       "    debugger;                                                 \n" +
       "  }                                                           \n" +
       "}                                                             \n" +
       "                                                              \n" +
       "function h(g) {                                               \n" +
       "  g();                                                        \n" +
       "}                                                             \n" +
       "                                                              \n" +
       "h(f(10,20));                                                  \n");


Here's the output I get:

$ obj-bug/dist/bin/js -f ~/moz/localnames.js
locals:
arg3,arg4,var3,var4,arguments
g
arg1,arg2,var1,var2,arguments
Object,Function,eval,Reflect,Debugger,PerfMeasurement,version,revertVersion,options,load,loadRelativeToScript,evaluate,run,readline,print,printErr,putstr,dateNow,help,quit,assertEq,setDebug,setDebuggerHandler,setThrowHook,trap,untrap,line2pc,pc2line,throwError,disassemble,dis,disfile,dissrc,dumpHeap,dumpObject,notes,findReferences,build,intern,clone,getpda,getslx,toint32,evalcx,evalInFrame,shapeOf,resolver,arrayInfo,snarf,read,readRelativeToScript,system,compile,parse,syntaxParse,timeout,elapsed,parent,decompileFunction,decompileBody,decompileThis,thisFilename,wrap,wrapWithProto,serialize,deserialize,newGlobal,enableStackWalkingAssertion,getMaxArgs,objectEmulatingUndefined,gc,gcparam,getBuildConfiguration,countHeap,oomAfterAllocations,makeFinalizeObserver,finalizeCount,gcPreserveCode,gczeal,schedulegc,selectforgc,verifyprebarriers,verifypostbarriers,gcstate,deterministicgc,gcslice,validategc,fullcompartmentchecks,nondeterministicGetWeakMapKeys,internalConst,isProxy,dumpHeapComplete,terminate,enableSPSProfilingAssertions,disableSPSProfiling,displayName,isAsmJSCompilationAvailable,isAsmJSModule,isAsmJSFunction,inParallelSection,setObjectMetadataCallback,setObjectMetadata,getObjectMetadata,it,custom,customRdOnly,customNative,FakeDOMObject,f,h,undefined,Array,Boolean,Date,Math,isNaN,isFinite,parseFloat,parseInt,NaN,Infinity,Number,String,escape,unescape,uneval,decodeURI,encodeURI,decodeURIComponent,encodeURIComponent,Error,InternalError,EvalError,RangeError,ReferenceError,SyntaxError,TypeError,URIError,RegExp,Iterator,StopIteration,JSON,Int8Array,Uint8Array,Int16Array,Uint16Array,Int32Array,Uint32Array,Float32Array,Float64Array,Uint8ClampedArray,DataView,ArrayBuffer,WeakMap,Map,Set,Proxy,Intl,constructor,toSource,toString,toLocaleString,valueOf,watch,unwatch,hasOwnProperty,isPrototypeOf,propertyIsEnumerable,__defineGetter__,__defineSetter__,__lookupGetter__,__lookupSetter__
$
Priority: -- → P3
Whiteboard: [autocomplete]
Bug 686937 modified the relevant code a bit. Two followup tasks that we should do here are to make sure that JSTermHelpers are added using _getJSTermHelpers, and cache the returned helpers object for the lifetime of the global (i.e. only recreate it on page navigation).
Hi, I would like to work on this bug. Any guidance is appreciated. Thanks.
Hello Christos! Thanks for your interest on working on this bug.

1. please take a look at WCA_onAutocomplete() from toolkit/devtools/server/actors/webconsole.js.
2. also see JSPropertyProvider() from browser/devtools/webconsole/utils.js.

This bug will be more challenging to fix.

General overview: at this point when we send autocomplete requests from the web console client we only send the text to complete and cursor location. I suggest you also include the frame actor ID - similar to how we include it for JS eval requests. See JST_requestEvaluation() in browser/devtools/webconsole/webconsole.js.

Next, you need to update the webconsole actor WCA_onAutocomplete() to use the frame actor ID to get the actual Debugger.Frame object from it. You can see one such example in WCA_evalWithDebugger().

Once you have the frame.environment you can give it to JSPropertyProvider, instead of |this.window| (in WCA_onAutocomplete). This is the case when the user does autocomplete requests while the debugger is paused and we have a frame. If the debugger is not paused, then you do not have any frame. In such case, you use the Debugger.Object for |this.window| -- see the Debugger.Object.asEnvironment() method.

JSPropertyProvider logic needs to be updated to assume the Debugger.Environment, not a raw JS object. You can list all of the properties in the environment and it's parents as explained in comment #1.

Docs for Debugger.Environment:
https://developer.mozilla.org/en-US/docs/SpiderMonkey/JS_Debugger_API_Reference/Debugger.Environment

You will also need to read stuff from:
https://developer.mozilla.org/en-US/docs/SpiderMonkey/JS_Debugger_API_Reference

Please let me know if you have more questions. Thank you!
Assignee: nobody → chstath
Status: NEW → ASSIGNED
Hello Mihai and thanks for all the info. I have done the first case (with the paused debugger) but the other one troubles me a bit. I use

(1) let dbgWindow = this._getDebuggerGlobal(this.window);

to get the Debugger.Object for |this.window|

but then I call

(2) dbgWindow.asEnviroment()

and I get "TypeError: dbgWindow.asEnvironment is not a function". I am not sure if (1) is the correct way to get the Debugger.Object and maybe this is the cause of the error. Any suggestions ?

Thanks
(please use the needinfo? flag and ask for needinfo from me in such cases - bugzilla sends me a better email notification)

According to https://developer.mozilla.org/en-US/docs/SpiderMonkey/JS_Debugger_API_Reference/Debugger.Object asEnvironment() should work. Maybe Panos can chime in?

try dbgWindow.environment. This should also work, according to the MDN page. I'm not sure what is the supposed difference between the two ways.

Thanks for your work! I'm looking forward to see the patch.
(In reply to Mihai Sucan [:msucan] from comment #6)
> (please use the needinfo? flag and ask for needinfo from me in such cases -
> bugzilla sends me a better email notification)
> 
> According to
> https://developer.mozilla.org/en-US/docs/SpiderMonkey/
> JS_Debugger_API_Reference/Debugger.Object asEnvironment() should work. Maybe
> Panos can chime in?

> 
> try dbgWindow.environment. This should also work, according to the MDN page.
> I'm not sure what is the supposed difference between the two ways.

I tried it and is 'undefined'.
According to the docs, 

"If the referent is a function that is debuggee code, a Debugger.Environment instance representing the lexical environment enclosing the function when it was created. If the referent is a function proxy or not debuggee code, this is undefined."

The asEnvironment() call should work though, but it doesn't.

"If the referent is a global object, return the Debugger.Environment instance representing the referent as a variable environment for evaluating code. If the referent is not a global object, throw a TypeError."

The |this.window| is a global object, isn't it?

> 
> Thanks for your work! I'm looking forward to see the patch.
Flags: needinfo?(past)
Flags: needinfo?(mihai.sucan)
I don't think asEnvironment() is implemented yet, I'll verify it with Jim tomorrow. But since you already have a handle to a Debugger.Object that refers to the page global, why not just use getOwnPropertyNames() on it?
Flags: needinfo?(past)
Yup, it's not implemented. Jim had an additional cool hack that could work (eval an empty function and get back a Debugger.Object that has an environment property on it), but that could be problematic if onEnterFrame hooks are in place. I think using getOwnPropertyNames() is the simplest thing to do.
Since we cannot use Debugger.Object.asEnvironment() why don't we keep the existing approach for the case of the un-paused debugger and use the new one only for the paused case. What is the difference between calling getOwnPropertyNames() on Debugger.Object that refers to the page global and calling getOwnPropertyNames() on |window| the way JSPropertyProvider does now?

Another question: all existing tests pass. I need to test the case where the debugger is paused. Is there a relevant test that I can be based upon?

Thanks a lot!
(apologies for the delay -- been swamped in work and other tasks)


(In reply to Christos Stathis from comment #10)
> Since we cannot use Debugger.Object.asEnvironment() why don't we keep the
> existing approach for the case of the un-paused debugger and use the new one
> only for the paused case. What is the difference between calling
> getOwnPropertyNames() on Debugger.Object that refers to the page global and
> calling getOwnPropertyNames() on |window| the way JSPropertyProvider does
> now?

This would allow the JSPP to have two slightly different code paths. Also, the current approach of using the raw JS content object is risky / easy to break. We should switch to the debugger API, which is a lot more reliable and safer.

Panos's suggestion makes sense: just use the dbgWindow Debugger.Object of |window|. You can do getOwnPropertyNames() on that.

> Another question: all existing tests pass. I need to test the case where the
> debugger is paused. Is there a relevant test that I can be based upon?

Yes, see:
  browser_eval_in_debugger_stackframe.js
  browser_console_variables_view_while_debugging.js
  browser_console_variables_view_while_debugging_and_inspecting.js

> Thanks a lot!

Thank you very much for your work and patience! I am looking forward to review the patch!
Flags: needinfo?(mihai.sucan)
Hello Mihai, sorry for the long delay (troubles with my build configuration, conflicting python versions etc).

I wrote a test for the case of the paused debugger based on the tests that you mentioned and I noticed that sometimes due to the caching, the autocomplete results are not what they should be. For example consider the following: There is a variable |foo| in the global scope and a |foobar| defined within a function. If the user types |foo| while in the global scope the result is cached. Then when the debugger is paused within the function, the user types |foo| but she/he gets only |foo| and not |foobar| as expected because due to the caching, a request is not sent. Of course, this case is rare and happens only if there are variables with names that start with the same letters and there are already cached results. Is this behavior accepted? Should I file a separate bug to fix the autocomplete caching ? Any suggestion is welcome.

Another strange behavior that I noticed is the following: If you place a breakpoint in a function and you call that function from the console, then the debugger is paused as expected but the web console stops working correctly. In particular, nothing is evaluated and this behaviour continues even after pressing F8 and un-pause the debugger.
Flags: needinfo?(mihai.sucan)
(In reply to Christos Stathis from comment #12)
> Hello Mihai, sorry for the long delay (troubles with my build configuration,
> conflicting python versions etc).

Nothing to worry about. Thanks for the work you are doing!


> I wrote a test for the case of the paused debugger based on the tests that
> you mentioned and I noticed that sometimes due to the caching, the
> autocomplete results are not what they should be. For example consider the
> following: There is a variable |foo| in the global scope and a |foobar|
> defined within a function. If the user types |foo| while in the global scope
> the result is cached. Then when the debugger is paused within the function,
> the user types |foo| but she/he gets only |foo| and not |foobar| as expected
> because due to the caching, a request is not sent. Of course, this case is
> rare and happens only if there are variables with names that start with the
> same letters and there are already cached results. Is this behavior
> accepted? Should I file a separate bug to fix the autocomplete caching ? Any
> suggestion is welcome.

This is something we should fix in this bug, since autocomplete for the selected stackframe scope is something new and thus the bug is "new". You should simply clear the cache of suggestions when the selected stackframe changes. Whenever you send the autocomplete request you also send the frame actor ID, so you can store that alongside the suggestions you receive. In JST__updateCompletionResult() you check the suggestions cache - also add the check for the frame actor ID so you can clear the cache if it changed (if the current frame actor ID != the last frame actor ID for the cached suggestions).


> Another strange behavior that I noticed is the following: If you place a
> breakpoint in a function and you call that function from the console, then
> the debugger is paused as expected but the web console stops working
> correctly. In particular, nothing is evaluated and this behaviour continues
> even after pressing F8 and un-pause the debugger.

Please open a bug with steps to reproduce. This is interesting.

Thanks for reporting these findings!
Flags: needinfo?(mihai.sucan)
The first one is fixed and the second was actually my mistake. The patch is almost ready. I need to see why a couple of tests fail, but all relevant tests pass. The code needs serious cleaning and combing, so I won't submit it for review immediately, but I will attach it here for anyone who would like to have a look.
The JSPropertyProvider uses two functions: isIteratorOrGenerator(obj) and isNonNativeGetter(obj, prop). It is not obvious to me how these should be modified now that |obj| is a Debugger.Object. It seems that some tests fail because of them.
Flags: needinfo?(past)
Flags: needinfo?(mihai.sucan)
Can't you just rewrite them to use the Debugger API? Which tests are failing?
Flags: needinfo?(past)
Those two functions access the obj in ways that are not available now that the obj is hidden inside a Debugger.Object. For example, they do comparisons like |type of obj == "something"|. Probably I am missing something here, but I didn't see any obvious way to do the same things with the Debugger.Object. The tests fail because for now I have just omitted those functions. I will investigate further and come up later with more specific questions.
(In reply to Christos Stathis from comment #17)
> Those two functions access the obj in ways that are not available now that
> the obj is hidden inside a Debugger.Object. For example, they do comparisons
> like |type of obj == "something"|. Probably I am missing something here, but
> I didn't see any obvious way to do the same things with the Debugger.Object.
> The tests fail because for now I have just omitted those functions. I will
> investigate further and come up later with more specific questions.

The isIteratorOrGenerator() and isNonNativeGetter() functions are used only to check properties if they can be accessed safely. We wanted to avoid the arbitrary execution of code from content objects.

The Debugger.Object makes handling of JS objects from content code safe. So, in JSPP I doubt youneed to use those two functions.

Which tests fail? I'd like to check them and provide more concrete suggestions on how to proceed. Thanks!
Flags: needinfo?(mihai.sucan)
I removed the two functions, I 'll clean the code a bit and I 'll attach the patch for testing (hopefully today).
These tests still fail:
browser/devtools/webconsole/test/browser_console_dead_objects.js
browser/devtools/webconsole/test/browser_webconsole_bug_585991_autocomplete_keys.js
browser/devtools/webconsole/test/browser_webconsole_bug_632347_iterators_generators.js
browser/devtools/webconsole/test/browser_webconsole_property_provider.js

The last three directly call the JSProperyProvider and are expected to fail because JSPropertyProvider has been completely rewritten, so the tests need to be modified.

The first one troubles me most. I 'm still investigating and any suggestion would be helpful.
Flags: needinfo?(past)
Flags: needinfo?(mihai.sucan)
First of all, thank you for this patch. It's a really good start. It also seems to be working well.

The dead objects test checks if the web console breaks when working with dead objects, or not.

DeadObjects are cross-compartment wrappers that point to objects which have been destroyed. This can only happen with a privileged web console or browser console instance when you hold a reference to some object that comes from a different compartment. See bug 883649 for details.

I believe you need to add some Cu.isDeadWrapper() checks in the code you wrote and bail out of autocomplete when you reach a dead object.

The autocomplete keys test most likely fails because the list of properties has changed. I'm curious how.

The iterators and generators test can be updated as follows: do not pass the raw window object to the JSPropertyProvider(), instead use the debugger API [1] to get a Debugger.Object for the window object. You can do |dbgWindow = dbg.makeGlobalObjectReference(window)| then pass dbgWindow to JSPP.

You can do the same for the _property_provider.js test.


[1] https://developer.mozilla.org/en-US/docs/SpiderMonkey/JS_Debugger_API_Guide
Flags: needinfo?(mihai.sucan)
Mihai's feedback should be enough to get you going, but I would recommend that you posted the actual errors you got from the tests in the future, in order to receive more concrete feedback.
Flags: needinfo?(past)
(In reply to Mihai Sucan [:msucan] from comment #21)
> First of all, thank you for this patch. It's a really good start. It also
> seems to be working well.
> 
> The dead objects test checks if the web console breaks when working with
> dead objects, or not.
> 
> DeadObjects are cross-compartment wrappers that point to objects which have
> been destroyed. This can only happen with a privileged web console or
> browser console instance when you hold a reference to some object that comes
> from a different compartment. See bug 883649 for details.
> 
> I believe you need to add some Cu.isDeadWrapper() checks in the code you
> wrote and bail out of autocomplete when you reach a dead object.

This is fixed. I think that CU.isDeadWrapper() expects an object and I only have a Debugger.Object but I tried catching the exception at the exact point of failure and now it works.

> 
> The autocomplete keys test most likely fails because the list of properties
> has changed. I'm curious how.

This is really strange. This test creates an object "foo" with 4 properties and it expects 18 suggestions for "foo.". It fails because it gets only 14. The 4 missing suggestions are the 4 properties of the foo object. The weird thing is that if I do the same steps "by hand" on the console, it works!

> 
> The iterators and generators test can be updated as follows: do not pass the
> raw window object to the JSPropertyProvider(), instead use the debugger API
> [1] to get a Debugger.Object for the window object. You can do |dbgWindow =
> dbg.makeGlobalObjectReference(window)| then pass dbgWindow to JSPP.
> 
> You can do the same for the _property_provider.js test.

This is what I was thinking until I realized that I don't know how to get a reference to the debugger from within a test :-) How do I get "dbg" ?
> 
> 
> [1]
> https://developer.mozilla.org/en-US/docs/SpiderMonkey/JS_Debugger_API_Guide
Flags: needinfo?(past)
Flags: needinfo?(mihai.sucan)
(In reply to Christos Stathis from comment #23)
> > I believe you need to add some Cu.isDeadWrapper() checks in the code you
> > wrote and bail out of autocomplete when you reach a dead object.
> 
> This is fixed. I think that CU.isDeadWrapper() expects an object and I only
> have a Debugger.Object but I tried catching the exception at the exact point
> of failure and now it works.

To get the raw JS object from a Debugger.Object you can do dbgObject.unsafeDereference(). Then you can use Cu.isDeadWrapper() which is preferred over the use of a try-catch block.

> > The autocomplete keys test most likely fails because the list of properties
> > has changed. I'm curious how.
> 
> This is really strange. This test creates an object "foo" with 4 properties
> and it expects 18 suggestions for "foo.". It fails because it gets only 14.
> The 4 missing suggestions are the 4 properties of the foo object. The weird
> thing is that if I do the same steps "by hand" on the console, it works!

This probably means it's a bug that needs to be fixed. ;)


> > The iterators and generators test can be updated as follows: do not pass the
> > raw window object to the JSPropertyProvider(), instead use the debugger API
> > [1] to get a Debugger.Object for the window object. You can do |dbgWindow =
> > dbg.makeGlobalObjectReference(window)| then pass dbgWindow to JSPP.
> > 
> > You can do the same for the _property_provider.js test.
> 
> This is what I was thinking until I realized that I don't know how to get a
> reference to the debugger from within a test :-) How do I get "dbg" ?

You cannot use the webconsole actor Debugger object, but you can create your own as explained in the link I gave you:

https://developer.mozilla.org/en-US/docs/SpiderMonkey/JS_Debugger_API_Guide
Flags: needinfo?(mihai.sucan)
(In reply to Mihai Sucan [:msucan] from comment #24)
> (In reply to Christos Stathis from comment #23)
> > > I believe you need to add some Cu.isDeadWrapper() checks in the code you
> > > wrote and bail out of autocomplete when you reach a dead object.
> > 
> > This is fixed. I think that CU.isDeadWrapper() expects an object and I only
> > have a Debugger.Object but I tried catching the exception at the exact point
> > of failure and now it works.
> 
> To get the raw JS object from a Debugger.Object you can do
> dbgObject.unsafeDereference(). Then you can use Cu.isDeadWrapper() which is
> preferred over the use of a try-catch block.
> 
This is fixed.

> > > The autocomplete keys test most likely fails because the list of properties
> > > has changed. I'm curious how.
> > 
> > This is really strange. This test creates an object "foo" with 4 properties
> > and it expects 18 suggestions for "foo.". It fails because it gets only 14.
> > The 4 missing suggestions are the 4 properties of the foo object. The weird
> > thing is that if I do the same steps "by hand" on the console, it works!
> 
> This probably means it's a bug that needs to be fixed. ;)
> 
It 's a bug introduced by the patch so I 'm investigating.

> 
> > > The iterators and generators test can be updated as follows: do not pass the
> > > raw window object to the JSPropertyProvider(), instead use the debugger API
> > > [1] to get a Debugger.Object for the window object. You can do |dbgWindow =
> > > dbg.makeGlobalObjectReference(window)| then pass dbgWindow to JSPP.
> > > 
> > > You can do the same for the _property_provider.js test.
> > 
> > This is what I was thinking until I realized that I don't know how to get a
> > reference to the debugger from within a test :-) How do I get "dbg" ?
> 
> You cannot use the webconsole actor Debugger object, but you can create your
> own as explained in the link I gave you:
> 
> https://developer.mozilla.org/en-US/docs/SpiderMonkey/JS_Debugger_API_Guide
Sorry, I didn't see that. Now it is also fixed.

So, only the autocomplete_keys test fails. I hope to have that fixed soon.
(In reply to Christos Stathis from comment #23)
> This is really strange. This test creates an object "foo" with 4 properties
> and it expects 18 suggestions for "foo.". It fails because it gets only 14.
> The 4 missing suggestions are the 4 properties of the foo object. The weird
> thing is that if I do the same steps "by hand" on the console, it works!

It sounds like the expando properties are set on a wrapper and not on the debuggee object, if they are invisible to the debugger API. If you have a Debugger.Object returned from a call to unwrap(), see if the expandos are present on the original object.
Flags: needinfo?(past)
In the autocomplete keys test, I had to change the way the foo object is created. Instead of content.wrappedJSObject.foobarBug585991=..., I did jsterm.execute("window.foobarBug585991=...") and the test now passes. We need to investigate why it didn't pass initialy, but if the change is acceptable I could request a review now.
Attachment #813311 - Attachment is obsolete: true
Flags: needinfo?(mihai.sucan)
Attachment #819325 - Flags: review?(mihai.sucan)
The test change seems fine to me, but since Mihai will have to read the changes closely anyway, might as well make it a formal review.
Flags: needinfo?(mihai.sucan)
Comment on attachment 819325 [details] [diff] [review]
This version passes all tests. However, I made a modification to the autocomplete keys test in order to pass.

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

Thank you for your patch! This is really good stuff.

Please remove all of the trailing whitespace added by the patch.

::: browser/devtools/webconsole/test/browser.ini
@@ +231,5 @@
>  [browser_webconsole_output_order.js]
>  [browser_webconsole_property_provider.js]
>  [browser_webconsole_scratchpad_panel_link.js]
>  [browser_webconsole_view_source.js]
> +[browser_autocomplete_in_debugger_stackframe.js]

browser_webconsole_...

::: browser/devtools/webconsole/test/browser_webconsole_bug_585991_autocomplete_keys.js
@@ +22,5 @@
> +  //   "item0": "value0",
> +  //   "item1": "value1",
> +  //   "item2": "value2",
> +  //   "item3": "value3",
> +  // };

Please remove this comment - unneeded.

@@ +27,4 @@
>  
>    jsterm = HUD.jsterm;
> +
> +  jsterm.execute("window.foobarBug585991={'item0': 'value0','item1': 'value1','item2': 'value2','item3': 'value3'}");

Format this string to be more readable: break it into multiple lines, and add spaces as needed.

::: browser/devtools/webconsole/test/browser_webconsole_bug_632347_iterators_generators.js
@@ +16,5 @@
>  function consoleOpened(HUD) {
>    let tools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
>    let JSPropertyProvider = tools.require("devtools/toolkit/webconsole/utils").JSPropertyProvider;
>  
> +  Cu.import("resource://gre/modules/jsdebugger.jsm");

The Cu.import() will bring the exported symbols from the jsm into the global of the test. The global of the test is the browser chrome window.

You should do something like:

let tmp = Cu.import("...", {});

The second argument tells the import function into which object to put the imported symbols. Then you can do tmp.addDebuggerToGlobal().

@@ +17,5 @@
>    let tools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
>    let JSPropertyProvider = tools.require("devtools/toolkit/webconsole/utils").JSPropertyProvider;
>  
> +  Cu.import("resource://gre/modules/jsdebugger.jsm");
> +  addDebuggerToGlobal(content.wrappedJSObject);

You should add the debugger global to |tmp|.

::: browser/devtools/webconsole/test/browser_webconsole_property_provider.js
@@ +17,5 @@
>    browser.removeEventListener("load", testPropertyProvider, true);
>    let tools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
>    let JSPropertyProvider = tools.require("devtools/toolkit/webconsole/utils").JSPropertyProvider;
>  
> +  Cu.import("resource://gre/modules/jsdebugger.jsm");

Same comments as in the iterators test.

::: browser/devtools/webconsole/webconsole.js
@@ +4201,5 @@
>        this._autocompleteQuery = null;
>        this._autocompleteCache = null;
>      }
>  
> +    this._lastFrameActorId = frameActor;

Maybe you should update the _lastFrameActorId only when the autocomplete results are received (_receiveAutocompleteProperties), or am I mistaken?

::: toolkit/devtools/server/actors/webconsole.js
@@ +154,5 @@
>  
>    /**
> +   * The JSTerm Helpers names cache.
> +   */
> +  jsTermHelpersCache: null,

s/jsTerm/_jsterm/

@@ +628,5 @@
> +      obj = this.dbg.makeGlobalObjectReference(this.window);
> +    }
> +
> +    let result = JSPropertyProvider(obj, aRequest.text,
> +                                              aRequest.cursor, frameActorId) || {};

nit: indentation mismatch. Align |aRequest.cursor| with |obj|.

::: toolkit/devtools/webconsole/client.js
@@ +130,5 @@
>        to: this._actor,
>        type: "autocomplete",
>        text: aString,
>        cursor: aCursor,
> +      frameActorId: aFrameActor,

s/frameActorId:/frameActor/

::: toolkit/devtools/webconsole/utils.js
@@ +752,5 @@
> +
> +  let environment = null, dbgObject = null;
> +
> +  /*
> +   * If the debugger is paused, then aScope is a Debugger.Environment

While this code works, I find it a bit confusing for me, and hard to follow.

I suggest you pass only one type of aScope to JSPP: always a Debugger.Object. Every Debugger.Environment has a .object property which represents the environment as a debugger object. In the web console actor you can get the environment.object and pass it to JSPP.

According to the docs "declarative" environments do not have an accessible |object| property [1], but for those cases you can get the parent environment, then parentEnv.object.

With this approach JSPP can be simplified. Can you please try it?

[1] https://developer.mozilla.org/en-US/docs/SpiderMonkey/JS_Debugger_API_Reference/Debugger.Environment

@@ +815,5 @@
>          if (!prop) {
>            return null;
>          }
>  
> +        try {

Please avoid this kind of big try-catch blocks.

@@ +839,3 @@
>  
> +      // If the final property is a primitive, it won't have a 'getOwnPropertyNames' function
> +      if (!obj.getOwnPropertyNames) {

You can tell the difference between a primitive value and a Debugger.Object by checking |typeof obj|. If |obj| is type "object", then it is a Debugger.Object. Do note that |null| also returns "object" for the type check.

if (typeof obj != 'object' || obj === null) { ...

@@ +978,5 @@
> +  let names = Object.create(null);
> +  let c = MAX_COMPLETIONS;
> +  for (let dbg = aDbgObject; dbg; dbg = dbg.proto) {
> +    let raw = dbg.unsafeDereference();
> +    if (Cu.isDeadWrapper(raw) || WCU.isIteratorOrGenerator(raw)) {

The isDeadWrapper check is sufficient. You don't need to skip iterators/generators because the debugger API *should* protect us from executing content scripts by mistake.

@@ +981,5 @@
> +    let raw = dbg.unsafeDereference();
> +    if (Cu.isDeadWrapper(raw) || WCU.isIteratorOrGenerator(raw)) {
> +      return null;
> +    }
> +    let ownNames = WCU.unwrap(dbg).getOwnPropertyNames();

You shouldn't need to unwrap dbg here. Why do you need this?
Attachment #819325 - Flags: review?(mihai.sucan)
(In reply to Mihai Sucan [:msucan] from comment #29)
> Comment on attachment 819325 [details] [diff] [review]
> This version passes all tests. However, I made a modification to the
> autocomplete keys test in order to pass.
> 
> Review of attachment 819325 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for your patch! This is really good stuff.
> 
> Please remove all of the trailing whitespace added by the patch.

Done.

> 
> ::: browser/devtools/webconsole/test/browser.ini
> @@ +231,5 @@
> >  [browser_webconsole_output_order.js]
> >  [browser_webconsole_property_provider.js]
> >  [browser_webconsole_scratchpad_panel_link.js]
> >  [browser_webconsole_view_source.js]
> > +[browser_autocomplete_in_debugger_stackframe.js]
> 
> browser_webconsole_...

Done

> 
> :::
> browser/devtools/webconsole/test/
> browser_webconsole_bug_585991_autocomplete_keys.js
> @@ +22,5 @@
> > +  //   "item0": "value0",
> > +  //   "item1": "value1",
> > +  //   "item2": "value2",
> > +  //   "item3": "value3",
> > +  // };
> 
> Please remove this comment - unneeded.

Done.

> 
> @@ +27,4 @@
> >  
> >    jsterm = HUD.jsterm;
> > +
> > +  jsterm.execute("window.foobarBug585991={'item0': 'value0','item1': 'value1','item2': 'value2','item3': 'value3'}");
> 
> Format this string to be more readable: break it into multiple lines, and
> add spaces as needed.

Done.

> 
> :::
> browser/devtools/webconsole/test/
> browser_webconsole_bug_632347_iterators_generators.js
> @@ +16,5 @@
> >  function consoleOpened(HUD) {
> >    let tools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> >    let JSPropertyProvider = tools.require("devtools/toolkit/webconsole/utils").JSPropertyProvider;
> >  
> > +  Cu.import("resource://gre/modules/jsdebugger.jsm");
> 
> The Cu.import() will bring the exported symbols from the jsm into the global
> of the test. The global of the test is the browser chrome window.
> 
> You should do something like:
> 
> let tmp = Cu.import("...", {});
> 
> The second argument tells the import function into which object to put the
> imported symbols. Then you can do tmp.addDebuggerToGlobal().

Done.

> 
> @@ +17,5 @@
> >    let tools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> >    let JSPropertyProvider = tools.require("devtools/toolkit/webconsole/utils").JSPropertyProvider;
> >  
> > +  Cu.import("resource://gre/modules/jsdebugger.jsm");
> > +  addDebuggerToGlobal(content.wrappedJSObject);
> 
> You should add the debugger global to |tmp|.

Done.

> 
> ::: browser/devtools/webconsole/test/browser_webconsole_property_provider.js
> @@ +17,5 @@
> >    browser.removeEventListener("load", testPropertyProvider, true);
> >    let tools = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> >    let JSPropertyProvider = tools.require("devtools/toolkit/webconsole/utils").JSPropertyProvider;
> >  
> > +  Cu.import("resource://gre/modules/jsdebugger.jsm");
> 
> Same comments as in the iterators test.

Done.

> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +4201,5 @@
> >        this._autocompleteQuery = null;
> >        this._autocompleteCache = null;
> >      }
> >  
> > +    this._lastFrameActorId = frameActor;
> 
> Maybe you should update the _lastFrameActorId only when the autocomplete
> results are received (_receiveAutocompleteProperties), or am I mistaken?

Autocomplete results are received in other cases as well when the frame has not been changed. The _lastFrameActorId has to be updated only when the frame actor is changed. I moved this line further down, just before the call to _receiveAutocompleteProperties or I could just add a |if (_lastFrameActorId != frameActor| and then do the update.

> 
> ::: toolkit/devtools/server/actors/webconsole.js
> @@ +154,5 @@
> >  
> >    /**
> > +   * The JSTerm Helpers names cache.
> > +   */
> > +  jsTermHelpersCache: null,
> 
> s/jsTerm/_jsterm/

Done.

> 
> @@ +628,5 @@
> > +      obj = this.dbg.makeGlobalObjectReference(this.window);
> > +    }
> > +
> > +    let result = JSPropertyProvider(obj, aRequest.text,
> > +                                              aRequest.cursor, frameActorId) || {};
> 
> nit: indentation mismatch. Align |aRequest.cursor| with |obj|.

Done.

> 
> ::: toolkit/devtools/webconsole/client.js
> @@ +130,5 @@
> >        to: this._actor,
> >        type: "autocomplete",
> >        text: aString,
> >        cursor: aCursor,
> > +      frameActorId: aFrameActor,
> 
> s/frameActorId:/frameActor/

Actually the |aFrameActor| should be renamed to |aFrameActorId| because that 's what it is: an id.

> 
> ::: toolkit/devtools/webconsole/utils.js
> @@ +752,5 @@
> > +
> > +  let environment = null, dbgObject = null;
> > +
> > +  /*
> > +   * If the debugger is paused, then aScope is a Debugger.Environment
> 
> While this code works, I find it a bit confusing for me, and hard to follow.
> 
> I suggest you pass only one type of aScope to JSPP: always a
> Debugger.Object. Every Debugger.Environment has a .object property which
> represents the environment as a debugger object. In the web console actor
> you can get the environment.object and pass it to JSPP.
> 
> According to the docs "declarative" environments do not have an accessible
> |object| property [1], but for those cases you can get the parent
> environment, then parentEnv.object.
> 
> With this approach JSPP can be simplified. Can you please try it?

I did it and then the browser_autocomplete_in_debugger_stackframe.js fails. I guess the reason is that when the debugger is paused within a function call, the environment is a "declarative" one and by using the parent, all variables defined in the function are missing. I will attach the modified patch for you to test.

> 
> [1]
> https://developer.mozilla.org/en-US/docs/SpiderMonkey/
> JS_Debugger_API_Reference/Debugger.Environment
> 
> @@ +815,5 @@
> >          if (!prop) {
> >            return null;
> >          }
> >  
> > +        try {
> 
> Please avoid this kind of big try-catch blocks.
> 
> @@ +839,3 @@
> >  
> > +      // If the final property is a primitive, it won't have a 'getOwnPropertyNames' function
> > +      if (!obj.getOwnPropertyNames) {
> 
> You can tell the difference between a primitive value and a Debugger.Object
> by checking |typeof obj|. If |obj| is type "object", then it is a
> Debugger.Object. Do note that |null| also returns "object" for the type
> check.
> 
> if (typeof obj != 'object' || obj === null) { ...
> 
> @@ +978,5 @@
> > +  let names = Object.create(null);
> > +  let c = MAX_COMPLETIONS;
> > +  for (let dbg = aDbgObject; dbg; dbg = dbg.proto) {
> > +    let raw = dbg.unsafeDereference();
> > +    if (Cu.isDeadWrapper(raw) || WCU.isIteratorOrGenerator(raw)) {
> 
> The isDeadWrapper check is sufficient. You don't need to skip
> iterators/generators because the debugger API *should* protect us from
> executing content scripts by mistake.
> 
> @@ +981,5 @@
> > +    let raw = dbg.unsafeDereference();
> > +    if (Cu.isDeadWrapper(raw) || WCU.isIteratorOrGenerator(raw)) {
> > +      return null;
> > +    }
> > +    let ownNames = WCU.unwrap(dbg).getOwnPropertyNames();
> 
> You shouldn't need to unwrap dbg here. Why do you need this?
Flags: needinfo?(mihai.sucan)
Thanks for your reply Christos.

(In reply to Christos Stathis from comment #30)
> > ::: toolkit/devtools/webconsole/client.js
> > @@ +130,5 @@
> > >        to: this._actor,
> > >        type: "autocomplete",
> > >        text: aString,
> > >        cursor: aCursor,
> > > +      frameActorId: aFrameActor,
> > 
> > s/frameActorId:/frameActor/
> 
> Actually the |aFrameActor| should be renamed to |aFrameActorId| because that
> 's what it is: an id.

Good point, but please make the suggested change. We use 'frameActor' without 'id' in the name for the 'evaluateJS' request packet.


> > ::: toolkit/devtools/webconsole/utils.js
> > @@ +752,5 @@
> > > +
> > > +  let environment = null, dbgObject = null;
> > > +
> > > +  /*
> > > +   * If the debugger is paused, then aScope is a Debugger.Environment
> > 
> > While this code works, I find it a bit confusing for me, and hard to follow.
> > 
> > I suggest you pass only one type of aScope to JSPP: always a
> > Debugger.Object. Every Debugger.Environment has a .object property which
> > represents the environment as a debugger object. In the web console actor
> > you can get the environment.object and pass it to JSPP.
> > 
> > According to the docs "declarative" environments do not have an accessible
> > |object| property [1], but for those cases you can get the parent
> > environment, then parentEnv.object.
> > 
> > With this approach JSPP can be simplified. Can you please try it?
> 
> I did it and then the browser_autocomplete_in_debugger_stackframe.js fails.
> I guess the reason is that when the debugger is paused within a function
> call, the environment is a "declarative" one and by using the parent, all
> variables defined in the function are missing. I will attach the modified
> patch for you to test.

Ah, yes, I see the problem. Then I believe we need a better way to structure/simplify the code.

How about you pass both env and dbgObj from the console actor to JSPP? To avoid the aScope overload.

Ideas I would try: when you have the environment, only pass it, let the dbgObj argument to be null, and vice-versa. Then instead of having getMatchedPropsInEnvironment(), getMatchedPropsInDbgObject(), getPropertyInDebuggerObject(), getVariableInEnvironment()... you should just have two functions: getNamesForEnvironment() and getNamesForDebuggerObject() which give you the properties/variables for env/dbgObj, as an array. The for-loop that goes through the property names that are split by '.' can be simplified to only work with those arrays. Similarly, the code that tries to match a partial property name can work with the arrays, instead of specialized functions.

Also, do note that when you have env, you only need to pop() the first element from the array (after you do split('.')), then getNamesFromEnvironment(), then do a partial or full match for the variable name, in the returned array. At this point you will either have a list of potential matches (partial matching), a list of suggestions, or you have a Debugger.Object as a full match. All this can be a distinct function, that is invoked before you get into the for-loop for digging deeper into the property names you have from split('.').

Any ideas you might have to simplify that code are welcome. When it's ready, please attach the updated patch for review. Thank you!
Flags: needinfo?(mihai.sucan)
(In reply to Mihai Sucan [:msucan] from comment #29)
 
> @@ +978,5 @@
> > +  let names = Object.create(null);
> > +  let c = MAX_COMPLETIONS;
> > +  for (let dbg = aDbgObject; dbg; dbg = dbg.proto) {
> > +    let raw = dbg.unsafeDereference();
> > +    if (Cu.isDeadWrapper(raw) || WCU.isIteratorOrGenerator(raw)) {
> 
> The isDeadWrapper check is sufficient. You don't need to skip
> iterators/generators because the debugger API *should* protect us from
> executing content scripts by mistake.

If I remove the isIteratorOrGenerator check then the browser_webconsole_bug_632347_iterators_generators.js test fails.
(In reply to Mihai Sucan [:msucan] from comment #31)

> > > ::: toolkit/devtools/webconsole/utils.js
> > > @@ +752,5 @@
> > > > +
> > > > +  let environment = null, dbgObject = null;
> > > > +
> > > > +  /*
> > > > +   * If the debugger is paused, then aScope is a Debugger.Environment
> > > 
> > > While this code works, I find it a bit confusing for me, and hard to follow.
> > > 
> > > I suggest you pass only one type of aScope to JSPP: always a
> > > Debugger.Object. Every Debugger.Environment has a .object property which
> > > represents the environment as a debugger object. In the web console actor
> > > you can get the environment.object and pass it to JSPP.
> > > 
> > > According to the docs "declarative" environments do not have an accessible
> > > |object| property [1], but for those cases you can get the parent
> > > environment, then parentEnv.object.
> > > 
> > > With this approach JSPP can be simplified. Can you please try it?
> > 
> > I did it and then the browser_autocomplete_in_debugger_stackframe.js fails.
> > I guess the reason is that when the debugger is paused within a function
> > call, the environment is a "declarative" one and by using the parent, all
> > variables defined in the function are missing. I will attach the modified
> > patch for you to test.
> 
> Ah, yes, I see the problem. Then I believe we need a better way to
> structure/simplify the code.
> 
> How about you pass both env and dbgObj from the console actor to JSPP? To
> avoid the aScope overload.
> 
> Ideas I would try: when you have the environment, only pass it, let the
> dbgObj argument to be null, and vice-versa. Then instead of having
> getMatchedPropsInEnvironment(), getMatchedPropsInDbgObject(),
> getPropertyInDebuggerObject(), getVariableInEnvironment()... you should just
> have two functions: getNamesForEnvironment() and getNamesForDebuggerObject()
> which give you the properties/variables for env/dbgObj, as an array. The
> for-loop that goes through the property names that are split by '.' can be
> simplified to only work with those arrays. Similarly, the code that tries to
> match a partial property name can work with the arrays, instead of
> specialized functions.

I 'm not sure I understand. Will the getNamesForXXX return an array of strings or an array of Debugger.Object ? Will it consider the parent/prototype chain or return names just for the given env/dbgObject ? Additionally, getPropertyInDebuggerObject calls the getOwnPropertyDescriptor(prop) and returns the desc.value and if it does not exist, it tries the desc.get. How will I do that if I work only with an array of names ?

> 
> Also, do note that when you have env, you only need to pop() the first
> element from the array (after you do split('.')), then
> getNamesFromEnvironment(), then do a partial or full match for the variable
> name, in the returned array. At this point you will either have a list of
> potential matches (partial matching), a list of suggestions, or you have a
> Debugger.Object as a full match. All this can be a distinct function, that
> is invoked before you get into the for-loop for digging deeper into the
> property names you have from split('.').

If getNamesForEnvironment gives me a list of names for the given environment *and* its parent environments, then this list may contain duplicates (that are different Debugger.Objects). How do I do the match then ? Additionaly, the property array that results from the split('.') must be treated differently when it has length>1 than the case of lenght==1. If its lenght==1 then I can have a partial or full match. If the length>1 then for the first property only a full match is acceptable.

> 
> Any ideas you might have to simplify that code are welcome. When it's ready,
> please attach the updated patch for review. Thank you!
Flags: needinfo?(mihai.sucan)
Christos: I haven't tried my own suggestions, indeed some of the ideas might not work. Feel free to make any changes you see fit to make the code simpler - if possible. If it's not possible, then don't worry to much - the main concern is to get all the test passing and to get it to work.


(In reply to Christos Stathis from comment #32)
> (In reply to Mihai Sucan [:msucan] from comment #29)
>  
> > @@ +978,5 @@
> > > +  let names = Object.create(null);
> > > +  let c = MAX_COMPLETIONS;
> > > +  for (let dbg = aDbgObject; dbg; dbg = dbg.proto) {
> > > +    let raw = dbg.unsafeDereference();
> > > +    if (Cu.isDeadWrapper(raw) || WCU.isIteratorOrGenerator(raw)) {
> > 
> > The isDeadWrapper check is sufficient. You don't need to skip
> > iterators/generators because the debugger API *should* protect us from
> > executing content scripts by mistake.
> 
> If I remove the isIteratorOrGenerator check then the
> browser_webconsole_bug_632347_iterators_generators.js test fails.

The test might need to be further updated then. Let it fail, submit an updated patch and I will look into it. I can't tell you offhand how to fix the test. I need to see how and why it fails.

Thank you!
Flags: needinfo?(mihai.sucan)
Mihai, this is a version of the patch without the call to WCU.isIteratorOrGenerator as you suggested. The test browser_webconsole_bug_632347_iterators_generators.js now fails. Take a look to see what might be the problem. I also did a small refactoring to the JSPropertyProvider that makes the logic more understandable. However, I don't believe it can be any simpler.
Attachment #819325 - Attachment is obsolete: true
Attachment #830450 - Flags: feedback?(mihai.sucan)
Comment on attachment 830450 [details] [diff] [review]
Modifcations based on comments #29, #31, #34

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

Christos, thanks for your patience here!

I did check what is the expected behavior now that you switched the autocomplete code to the debugger API. This is working fine, the failures in the test are normal.

In bug 632347 the autocomplete code did cause content script to execute for iterators, generators and, iirc, in other bugs we had issues with getters. In that bug we did patches to guard against such cases, to prevent looping into such objects. The debugger API allows us to inspect such objects without side effects - it was designed as such.

The test needs to be updated to expect completions.
Attachment #830450 - Flags: feedback?(mihai.sucan) → feedback+
Last failing test fixed
Attachment #830450 - Attachment is obsolete: true
Attachment #832548 - Flags: review?(mihai.sucan)
Comment on attachment 832548 [details] [diff] [review]
Use the debugger API for autocomplete suggestions

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

Thanks for the updates. Giving this patch r+ with the comments below addressed. Also, please rebase the patch on top of the latest fx-team/m-c repo.

::: browser/devtools/webconsole/test/browser_webconsole_bug_632347_iterators_generators.js
@@ +18,5 @@
>    let JSPropertyProvider = tools.require("devtools/toolkit/webconsole/utils").JSPropertyProvider;
>  
> +  let jsdebugger = Cu.import("resource://gre/modules/jsdebugger.jsm", {});
> +  jsdebugger.addDebuggerToGlobal(content.wrappedJSObject);
> +  let dbg = new content.wrappedJSObject.Debugger;

Please change to:

jsdebugger.addDebuggerToGlobal(jsdebugger);
let dbg = new jsdebugger.Debugger;

You might also want s/jsdebugger/tmp/

@@ +27,5 @@
>  
>    // Make sure autocomplete does not walk through iterators and generators.
>    let result = win.gen1.next();
> +  let completion = JSPropertyProvider(dbgWindow, null, "gen1.");
> +  is(completion.matches.length, 19, "Got 19 matches for gen1");

This test will fail every time a new property is added to the list (or removed). Please simply check that length > 0.

@@ +34,5 @@
>  
>    result = win.gen2.next();
>  
> +  completion = JSPropertyProvider(dbgWindow, null, "gen2.");
> +  is(completion.matches.length, 19, "Got 19 matches for gen2");

Same as above.

@@ +74,5 @@
>      { name: "gen2", isGenerator: true },
>      { name: "iter1", isIterator: true },
>      { name: "iter2", isIterator: true },
>    ], { webconsole: aWebconsole }).then(function() {
> +    delete this.addDebuggerToGlobal;

This is not needed because addDebuggerToGlobal is not added to |this|.

::: browser/devtools/webconsole/test/browser_webconsole_property_provider.js
@@ +19,5 @@
>    let JSPropertyProvider = tools.require("devtools/toolkit/webconsole/utils").JSPropertyProvider;
>  
> +  let jsdebugger = Cu.import("resource://gre/modules/jsdebugger.jsm", {});
> +  jsdebugger.addDebuggerToGlobal(content.wrappedJSObject);
> +  let dbg = new content.wrappedJSObject.Debugger;

See comment in the iterators test for addDebuggerToGlobal.

::: toolkit/devtools/server/actors/webconsole.js
@@ +153,5 @@
>    consoleProgressListener: null,
>  
>    /**
> +   * The JSTerm Helpers names cache.
> +   */

nit: add @private and @type info.

@@ +607,5 @@
> +    let dbgObject = null;
> +    let environment = null;
> +
> +    /*
> +    * This is the case of the paused debugger

nit: all comments inside functions should use single-line style of comments //, not block comments.

::: toolkit/devtools/webconsole/client.js
@@ +121,5 @@
>     *        Cursor location inside the string. Index starts from 0.
>     * @param function aOnResponse
>     *        The function invoked when the response is received.
> +   * @param string aFrameActor
> +   *        The id of the frame actor that made the call

nit: end sentence with period.

(for consistency)

::: toolkit/devtools/webconsole/utils.js
@@ +745,5 @@
>   *              matchProp: Last part of the inputValue that was used to find
>   *                         the matches-strings.
>   *            }
>   */
> +function JSPropertyProvider(aDbgObject, anEnvironment, aInputValue, aCursor, aFrameActorId)

The jsdoc comment is missing the description for the aFrameActorId parameter.
Attachment #832548 - Flags: review?(mihai.sucan) → review+

        Attachment #832548 -
        Attachment is obsolete: true

    
    

    
  
Hello Mihai,

thank you for the review. I have done all of your comments except the last one. I just deleted the aFrameActorId parameter as it was just forgotten there. It is not actually used.

Thanks again

    
    

    
  
Can you push the patch to try, to see if all tests pass in all platforms (at least mochitest-bc and mochitest-oth)? See here for details:

https://wiki.mozilla.org/ReleaseEngineering/TryServer

    
    

    
  
Christos, thanks for the updated patch!

Panos, here is a try push: https://tbpl.mozilla.org/?tree=Try&rev=15615142f5fa - I am not sure if Christos has try push access.

    
    

    
  
Try push looks green.

Landed: https://hg.mozilla.org/integration/fx-team/rev/73b76faa9653

Thanks Christos for the patch and for your patience.
Whiteboard: [autocomplete] → [autocomplete][fixed-in-fx-team]

    
  
Blocks: 943496

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/73b76faa9653
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [autocomplete][fixed-in-fx-team] → [autocomplete]
Target Milestone: --- → Firefox 28

    
  
Keywords: verifyme

    
    

    
  
Verified as fixed on Firefox 28 beta 6 under Win 7 64-bit and Ubuntu 32-bit.
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: petruta.rasa

    
  
Product: Firefox → DevTools

          You need to log in
          before you can comment on or make changes to this bug.