Currently, the Command Line uses eval() to evaluate commands [1]. Each command is wrapped in a "with (_FirebugCommandLine) {}" clause. Moreover, there are treatments (kind of hacks) to correct the source, the lineNumber, etc...

I experiment the use of evalInGlobalWithBindings() as a replacement. Some advantages: - it gets rid of the "with(_FirebugCommandLine)" statement thanks to the bindings. It implies that in case of error, the user only sees their own command. - the treatments before displaying the exception are cleaner - others ?

The current prototype of what I am making is quite functional, but I may need help to support getters commands ("help", "$0", "$1"): https://github.com/fflorent/firebug/compare/issue6290 https://github.com/fflorent/firebug/commit/6be190a5171ea02a8db1dbcb44ab9757dd5d8afe

So should I go further so we can finally merge the changes with master?

Thanks in advance.

Florent

[1] https://github.com/firebug/firebug/blob/5ec3db426712aea9d7cfd2b43afde8b9c21149f6/extension/content/firebug/console/commandLineExposed.js#L195

NB: I also experimented the use of Cu.evalInSandbox("with (window){ ... }", sandbox);. But the "with (window) { ... }" statement does not cover the new affectations ("var i = 0;" won't add window.i).

This probably is a good idea if only because we can get rid of window._FirebugCommandLine. A few points:

• This makes us require the debugger to evaluate code. :( It's not a huge deal currently because the console already requires it, but I'd want to get of that... What would be really good to have (also for the WebConsole and the closure inspector) would be an API for "get a Debugger.Object for this global without adding it as a debuggee". Should file a bug on that.

• We need to test this carefully (I'm particularly thinking about errors and error locations).

• It will pave the way to issue 5321.

• The patch adds a line "/* EXPRESSION EVALUATED USING THE FIREBUG COMMAND LINE: */" as part of the source of errors. Two questions about this: Is this a good idea? Should it be localized? I don't have much of an opinion without testing it, but we should discuss it.

• We also need to discuss how the command line API will work with the remote debugging protocol.

NB: I also experimented the use of Cu.evalInSandbox("with (window){ ... }", sandbox);. But the "with (window) { ... }" statement does not cover the new affectations ("var i = 0;" won't add window.i). This is basically what the WebConsole is currently doing, and indeed they plan to switch to evalInGlobalWithBindings for this and other reasons.

I like this effort.

We should make sure we sync our work related to JSD2 and I believe all the changes should go to JSD2 branch.

We should discuss on today's call.

Honza

A question: I see that we use event mechanisms for the expression evaluations. Does anyone know what they are for?

I wonder if they are still relevant if use evalInSandboxWithBindings.

Thanks in advance, Florent

I believe that's a remnant from when directly touching things on user-level objects (and in this case even calling "eval") posed a security risk, so one had to add a level of indirection and perform the evaluation etc. from some different scope (but I'm not sure on the details). See also the now-removed commandLineInjected.js file.

In any case, I doubt it's still relevant.

Here are some commits since the last time: - the one from last week: https://github.com/firebug/firebug/commit/cc07cdd9b43aedc54302671b5a7609f594dace04 - the commit of this evening: https://github.com/firebug/firebug/commit/779cdb1124ec1175ab674976401c590ee4868bb3

To sum up the last one: - I get rid of the event mechanism for: * evaluating expressions * calling commands (through notifyFirebug) * treating the returned value (also through notifyFirebug) - A big cleanup! (hoping it doesn't break anything) - Dropping _FirebugCommandLine - I migrate the "FirebugCommandLine" constructor to console/commandLineExposed.js - I improve the exception handling by reverting quite all the changes made in this branch - I "play" with a solution we could implement for the issue 6141 (not working yet; and we have to discuss it) - Questions asked directly via some comments

I tried to be careful on that commit. But first, could you tell me if you agree the principle of that cleanup?

Thanks in advance, Florent

Another commit, in order to improve the exception handling: https://github.com/firebug/firebug/commit/53feac78d692c59a17446bab6877d63cf726265c + https://github.com/firebug/firebug/commit/3a33feb218259096b5f41ac6d821cf37f44d8304 (minor additions)

This commit attempts to correct the stack traces, the line number and the filename when the exception is thrown by a command.

I don't know if that covers the whole cases for the exceptions.

And there is one that AFAIK with which we can't print the right stack trace: 1. Type and run: function a() { throw new Error("error"); } 2. Type and run: a()

Because evalInGlobalsWithBindings is adding "@debugger eval code:" in the stack trace for each reference of evaluated code.

So I wonder whether it is possible to replace "debugger eval code" to "data:text/javascript,:" (like Firebug does for the filename). Should I open a bugzilla ticket to ask it to the JSD2 team?

Florent

I have ported the firebug/debugger/debuggerLib module into JSD2 branch at: https://github.com/firebug/firebug/commit/cb729d8e0576df927551a9073e038a3af841cc66

One suggestion, I think the order of arguments for DebuggerLib.getDebuggeeGlobal() should be changed to: (context, global).

This way we can call the function with |context| only - in cases when the global is |context.window|

Thoughts?

Honza

One suggestion, I think the order of arguments for DebuggerLib.getDebuggeeGlobal() should be changed to: (context, global).

Okay for me.

Florent

Cool done at: https://github.com/firebug/firebug/commit/e230f81ae8aa93856f49cde23458b4231934714f

Also, I noticed that 'debugee' variables use 'd' prefix. I like it, but we should also keep the camel case syntax rule. So, for example: dGlobal, dWin, etc.

Honza

Here are my further comments:

commandLineExposed.js:

1) 'commands' variable overwrites and existing 'commands' global defined at the top of the file. The global is used later in the file, it's a bit confusing. https://github.com/firebug/firebug/blob/4702d8ebdf703258b90ccb392b96a75d13bbe585/extension/content/firebug/console/commandLineExposed.js#L76

2) A 'command' variable is redeclared within a for-loop (and not in other too) https://github.com/firebug/firebug/blob/4702d8ebdf703258b90ccb392b96a75d13bbe585/extension/content/firebug/console/commandLineExposed.js#L135 In general we should declare vars at the place where they are used, i.e. within the a loop

3) removeConflictingNames changes the cached 'commandLine' object https://github.com/firebug/firebug/blob/4702d8ebdf703258b90ccb392b96a75d13bbe585/extension/content/firebug/console/commandLineExposed.js#L253 Possible conflicting names can change during the page life time. So, the original list of binding should be always cleaned-up (otherwise, resolved conflicts will remain removed).

4) context.window: we should always try to prefer context.getCurrentGlobal() if possible. Should we do it even here? https://github.com/firebug/firebug/blob/4702d8ebdf703258b90ccb392b96a75d13bbe585/extension/content/firebug/console/commandLineExposed.js#L249

The real problem is that the command line doesn't work for me now so, I can't properly test. It would be great to finish this since I could use the code for Watch & DOM panel editing.

Honza

The real problem is that the command line doesn't work for me now Not sure what I did before, but now it seems to work and I can test it! Honza

This way we can call the function with |context| only - in cases when the global is |context.window| Why would we do that? If anything we should be moving away from context.window and the subtle breakage for cross-origin iframes that comes with it.

4) context.window: we should always try to prefer context.getCurrentGlobal() if possible. Should we do it even here? evaluate() should take a window to evaluate in, I think. If that doesn't work, yes, context.getCurrentGlobal().

2 little commits: - https://github.com/firebug/firebug/commit/08c7964707ad193366804640579f459deb524e15#L2L146 - https://github.com/firebug/firebug/commit/d1787bfa133d03c1dd7b64534bbb900d6d4649a7

3) removeConflictingNames changes the cached 'commandLine' object Possible conflicting names can change during the page life time. So, the original list of binding should be always cleaned-up (otherwise, resolved conflicts will remain removed).

Actually, it is. With commandLineCache.get, we get the reference to commandLine back, so the list of bindings is also cleaned-up the next time we get commandLine through the Weak Map.

Typing these commands gave me the good result: 1) window.copy = "test"; 2) copy; // => "test" 3) delete window.copy; 4) copy; // => ReferenceError: copy is not defined

Note that for step 1), if you type |var copy = "test";|, that won't work, and that was already reported for Firebug 1.11: see issue 6269.

4) context.window: we should always try to prefer context.getCurrentGlobal() if possible. Should we do it even here? I applied what suggested Simon (passing the window as an argument).

Florent

Actually, it is. With commandLineCache.get, we get the reference to commandLine back, so the list of bindings is also cleaned-up the next time we get commandLine through the Weak Map. But you always get a reference to the same (and already modified) commandLine object, which is wrong. Well, in any case, here is my STR:

1) Load http://janodvarko.cz/firebug/jsd2/tests/commandLine.html 2) Switch to the Console panel 3) Type: dir(window) -> list of window props logged -> OK 4) Click Define dir() button on the page (defines dir() function on the page) 5) Type: dir(window) -> the function from the page executed 6) Click Remove dir() button on the page (removes the dir() function on the page) 7) Type: dir(window) -> ReferenceError: dir is not defined -> BUG

Honza

Florent, as soon as you merging issue6291 branch into jsd2 branch, can you do it as one commit only? This way it'll be easier to have an overview of all changes you actually did.

Thanks Honza

Actually that is wanted. If the user deletes a variable, they expect its value to be undeclared, I think.

Otherwise, the command should be still available through "firebug.dir" (proposed in some issue, I am not behind a PC to look for it).

Florent

Sure for the commit Le 20 mars 2013 08:37, a �crit :

Actually that is wanted. If the user deletes a variable, they expect its value to be undeclared, I think. No, it isn't wanted. We want to keep the same behavior as it was there before. Please fix it, thanks. Honza

No, it isn't wanted.

NB: By "That is wanted", I actually meant "That is intended". Sorry for that confusion, I didn't mean to be rude by imposing anything.

We want to keep the same behavior as it was there before. Corrected here: https://github.com/firebug/firebug/commit/e53c1f1a1ec8af12cecc62814e95edd9a2cb30bd

I tested my work using FBTests. I mostly tested the commandline/ tests, and got these errors: - debug / undebug ; monitor / unmonitor: Firebug.Debugger.monitorFunction isn't implemented yet. - closure inspector: we need to take time to update the FBTest. But basic manual tests work as expected. - console/breakOnError/breakOnError.js: the button "break on error" is disabled

And about console/ tests, I mostly get errors that I also get with the JSD2 branch. I got two regressions detected by the Test Console, though, they look minor (see logs attached).

I created that branch, Honza, so I let you decide when to merge the work with yours (it looks ready to me, in case it is urgent for you): https://github.com/firebug/firebug/commit/issue6291_merge

Florent

NB: By "That is wanted", I actually meant "That is intended". Sorry for that confusion, I didn't mean to be rude by imposing anything. Don't worry, I didn't see or feel any rude behavior :-)

It's just that when we doing refactoring we should always preserve the existing behavior. If there is something we would like to change we should file a new issue and discuss it.

Corrected here: https://github.com/firebug/firebug/commit /e53c1f1a1ec8af12cecc62814e95edd9a2cb30bd Works great now!

• debug / undebug ; monitor / unmonitor: Firebug.Debugger.monitorFunction isn't implemented yet.
• closure inspector: we need to take time to update the FBTest. But basic manual tests work as expected.
• console/breakOnError/breakOnError.js: the button "break on error" is disabled yep, we need to fix these things. Will your test (we discussed yesterday) also cover closure inspector?

I created that branch, Honza, so I let you decide when to merge the work with yours (it looks ready to me, in case it is urgent for you): https://github.com/firebug/firebug/commit /issue6291_merge Thanks, I'll probably merge it today.

Great job Florent, thanks!

Honza

So, I tried to merge issue6291_merge into jsd2 and ran into some issues.

First this scenario stopped to work (in jsd2 branch) 0) Use jsd2 branch 1) load http://janodvarko.cz/firebug/jsd2/tests/issue6291.html 2) select the Script panel 3) create a breakpoint at line 24, click the button on the page, hit the bp 4) Check out the watch panel, expand "ed" object, edit its "age" property 5) it doesn't work after the merge

Also, Firebug.CommandLine.evaluateInDebugFrame uses FirebugCommandLineAPI object, which is no longer available.

Finally, could you try to merge with (cloned) master and run all tests again? This way we should catch all possible problems.

Honza

I did some changes (in jsd2) branch so, the test case probably works, but the FirebugCommandLineAPI in evaluateInDebugFrame is still a valid question.

Also I put some logic into Debugger.evaluate at: https://github.com/firebug/firebug/commit/92b955d9fb4d68a0460197baff304d3e3c10e661 .. and I believe it should somehow share API with the CommandLine if possible.

Honza

It's just that when we doing refactoring we should always preserve the existing behavior. If there is something we would like to change we should file a new issue and discuss it. Okay, I understand.

Will your test (we discussed yesterday) also cover closure inspector? No. Instead, I rather suggest adapting the existing tests, and maybe completing it.

Finally, could you try to merge with (cloned) master and run all tests again? This way we should catch all possible problems. Okay, I'll do that this week-end.

Also I put some logic into Debugger.evaluate at: https://github.com/firebug/firebug/commit/92b955d9fb4d68a0460197baff304d3e3c10e661 .. and I believe it should somehow share API with the CommandLine if possible. issue 5321? Or you also mean to support it for the watch expressions?

Note that if we want issue 5321 (I agree with the suggestion), I can adapt the CommandLine.evaluate function a little for that.

Oh, by the way, issue 5321 made me think that because of evalInGlobalWithBindings, |"use strict";| has a particular behaviour in the Command Line, as described here: https://developer.mozilla.org/en-US/docs/SpiderMonkey/JS_Debugger_API_Reference/Debugger.Object#evalInGlobalWithBindings%28%29

So, this test case gives an unexpected result: 1. Open Firebug on this page 2. Enable and switch to the console 3. type : "use strict"; var i = 0; 4. type : window.i; // => undefined (VS. 0 in the current version)

Florent

First this scenario stopped to work (in jsd2 branch) ... Also, Firebug.CommandLine.evaluateInDebugFrame uses FirebugCommandLineAPI object, which is no longer available.

Yes, you pointed out the good mistake. Corrected here: https://github.com/firebug/firebug/commit/d9b998b7bddc088c71966b48bb5b4b7e5fad3bca Does it solve the issue of the scenario?

Here is the merge with master: https://github.com/fflorent/firebug/commits/issue6291_master

I am intending to solve test case for issues 5655 and 6116 when I work on issue 6268.

The big mystery for me remains the test case for issue 5033 and commandLine/api/cd.html ... For the latter, something weird happens with the returned |dglobal| object here (in case of win is an iframe): https://github.com/firebug/firebug/blob/e230f81ae8aa93856f49cde23458b4231934714f/extension/content/firebug/debugger/debuggerLib.js#L62

So I experimented this, and it works: https://github.com/fflorent/firebug/commit/d45b6a246e44eeefa731c6a03d6791bcce636421 (NB: It is not a solution I propose, just an experiment to understand what happens)

Florent

The failure of the FBTest commandLine/api/cd.html seems to be due to a conflict with the ShareMeNot, as explained in the comments of this commit: https://github.com/fflorent/firebug/commit/d45b6a246e44eeefa731c6a03d6791bcce636421#commitcomment-2885624 . I have left the trick to make it work with this addon, since there can be others in this case... At least, until that we know what cause this. Or should I revert it?

There remain 5 failures from the FBTests with the issue6291_master branch: - issues 5655 and 6116 that, as said above, I will fix when working on issue 6268 (which is the next step). - issue 5033 and issue 3645: they deal with the old js/ code, and for some reason, fail after the merge. Should I spend time on they to see what causes the failures anyway? - issue 5878 whose FBTest has to be rewritten

I updated the issue6291_merge branch: https://github.com/firebug/firebug/commit/issue6291_merge

Florent

I cloned: https://github.com/fflorent/firebug/commits/issue6291_master

And after running all tests I am seeing these failures: https://getfirebug.com/testresults/?dburi=http://firebug.couchone.com/&dbname=firebug2&userheaderid=3d83be1bd0a3e97d31303e2937000174

I don't see any failures when running tests on the current master. Note that you need to test with Firefox 21 due to bug: https://bugzilla.mozilla.org/show_bug.cgi?id=854139

The failure of the FBTest commandLine/api/cd.html seems to be due to a conflict with the ShareMeNot, It's ok, to test on a profile with only Firebug (and FBTest+FBTrace) installed

I have left the trick to make it work with this addon, since there can be others in this case... At least, until that we know what cause this. Or should I revert it? You can keep it as long as it doesn't have any side effects. Can you report a bug in ShareMeNot issue list (if it exists)? Perhaps they could fix it be for our Firebug JSD2 is released. Could you create a simple test extension that can be used to repro the problem? Or, if you provide detailed explanation of the problem, I can do it. I think, we should report a bugzilla bug.

• issue 5033 and issue 3645 : they deal with the old js/ code, and for some reason, fail after the merge. Should I spend time on they to see what causes the failures anyway? Yes please, we need to do analyses so, we have some idea what is the problem. We need to fix them on JSD2 branch after merge anyway. In case of 5033, the debugger seems to be working, it's just the debugger UI is confused. The issue 3645 is just about one more stack frame.

• issues 5655 and 6116 that, as said above, I will fix when working on issue 6268 (which is the next step). I think we should fix them. It's only related to the wrong source link, no?

I think it's important to finish this issue, make the changes stable and merge - before doing anything else. Otherwise, the patch will grow and it'll be harder and harder to maintain and review it.

• issue 5878 whose FBTest has to be rewritten Do you mean issue 5873?

Here is what I think you should do:

• Continue development on issue6291_master and fix tests: 5873, 6116, 5655, 5033
• Post a link to every commit you do + a description here so, anyone can follow your changes.
• As soon as you are done I'll review again the entire patch
• Consequently we can merge into JSD2 branch
• Create FBTests as we already discussed (those tests are part of the JSD2 branch)

Honza

One more note, if the test failures are all fixed we could consider merging with master (1.12) and then with jsd2 branch.

honza

Re FireClosure test failures:

[ERROR] The completion popup should show the right list of closure variables. (was: $,$$,$0,$1,$n,$x,catched,cd,clear,copy,debug,dir,dirxml,help,helper,include,inspect,keys,local,monitor,monitorEvents,param,profile,profileEnd,table,traceAll,traceCalls,undebug,unmonitor,unmonitorEvents,untraceAll,untraceCalls,unused,values,withVar, expected: catched,helper,local,param,unused,withVar)

Need to fix closureInspector.js to ignore the new scope (there's an if-test (scope.type === "with" && scope.getVariable("profileEnd")) to detect and hide command line built-ins from scopes of functions defined on the command line).

[ERROR] Verify: _FirebugCommandLine.%cmd SHOULD BE Error: permission denied to access cross origin scope (was: ReferenceError: _FirebugCommandLine is not defined, expected: Error: permission denied to access cross origin scope)

fix the test to say cd.%context or so.

That commit fixes the failures for issues 5655 and 6116: https://github.com/firebug/firebug/commit/3c9932eedc38e5d8dcfab38c4b0842fc9156e10a

This fix should be temporary, if we decide to integrate issue 6268 and the commit mentioned in the comments.

We are almost there (at least, I hope!):

Fix for tests for issues 5655 and 6116: - https://github.com/firebug/firebug/commit/3c9932eedc38e5d8dcfab38c4b0842fc9156e10a

Fixes for the tests of issue 5873 (thanks Simon!): - https://github.com/firebug/firebug/commit/ccfbd87dce9e2cc8cc0c671e2fdd90432802fa41 - https://github.com/firebug/firebug/commit/d617943c7a9d3a649ca7ba0713cace2b99598e4e

Some comments + code reorganization: - https://github.com/firebug/firebug/commit/6a72222b261dfdc8e79a6446e55b5f0f68140c93

An important fix that prevents expressions that call breakpoint'ed functions to freeze some elements of the Firebug UI (like the panels), which partially fixes the failures for issue 3645: - https://github.com/firebug/firebug/commit/e6a7a2fbff20967419349f425713ae9da2a17c18 So indeed, it was wised to continue investigating what caused the failures. Note that I am not sure what that fix does :). I have only guessed what could happen and fixed it by remembering the event mechanisms there were before my work. So perhaps there is a better solution? And I should create an FBTest to check the UI responsiveness when a script execution is paused (the problem was only a side-effect of the failure of the FBTest for issue 3645).

Some more fixes related to the issue6291_master only: - for tests for issue 3645: https://github.com/fflorent/firebug/commit/370ea219863aa3080d12878f63b54e4c40974e42 - for tests for issue 5033 (not sure it is the good way): https://github.com/fflorent/firebug/commit/b68c6ab29a1a8c7cd010630a6f9b6261db803cf6

Some minor commits: - https://github.com/firebug/firebug/commit/599c7fd - https://github.com/firebug/firebug/commit/d4b5b05955ddb59267924f97536e7cc59cff3daa

(I think that's it)

Remaining tasks: - report the bug for the conflict with ShareMeNot (or find a way to reproduce the conflict with a sample add-on) - write some FBTests

Florent

Another commit: - https://github.com/firebug/firebug/commit/08e318e

To fix Simon's remark: https://github.com/firebug/firebug/commit/ccfbd87dce9e2cc8cc0c671e2fdd90432802fa41#commitcomment-2922902

NB.: I haven't updated the issue6291_merge branch yet.

Florent

About the potential extension breakages:

Based on https://mxr.mozilla.org/addons/search?string=CommandLine&case=on&find=.js&findi=.js&filter=^[^\0]*%24&hitlimit=&tree=addons :

• Acebug Editor (https://mxr.mozilla.org/addons/source/279230/): -> It has some code there, related to exception treatments: https://mxr.mozilla.org/addons/source/279230/chrome/content/aceEditor.js#637 I am not sure about the purpose, but maybe we should contact the authors (Harutyun Amirjanyan and Michael Ratcliffe) to be sure there is no regression. Otherwise, I don't see code that could enter in conflict with the changes.
• Firebug Autocompleter (https://mxr.mozilla.org/addons/source/203494): -> I don't see any possible regression caused by the changes (at least, I don't see any direct call to Firebug.CommandLine.evaluate, Firebug.CommandLine.injector, Firebug.CommandLine.CommandHandler, ...).

No particular results for this two search patterns: - https://mxr.mozilla.org/addons/search?string=closureinspector&find=.js&findi=.js&filter=^[^\0]%24&hitlimit=&tree=addons - https://mxr.mozilla.org/addons/search?string=firebug%2Fconsole%2Fcommandline&find=.js&findi=.js&filter=^[^\0]%24&hitlimit=&tree=addons

And I don't see particular potential breakages with these addons either: - consoleexport
- eventbug
- extension-examples
- httpmonitor
- netexport
- swarms

New branch (another one, sorry): issue6291_master_merge (one-commit branch based on master): https://github.com/firebug/firebug/tree/issue6291_master_merge

Florent

Did you change the API for Firebug.CommandLine.evaluate?

https://github.com/firebug/firebug/tree/issue6291_master_merge GitHub's acting weird, but does that include all the relevant changes?

Did you change the API for Firebug.CommandLine.evaluate? Not directly. I just changed the CommandLine.evaluateByEventPassing method (renamed evaluateInGlobal). The parameters are the same, but since CommandLineExposed has been quite fully reworked, we're not sure whether some specific hacks would run in the future (like the one found for Acebug Editor)...

GitHub's acting weird, but does that include all the relevant changes? I have missed things when I pushed this branch first. This is fixed now.

Florent

(the evil is in the details): when one type and enter the "debugger;" statement in the Command Editor (so not in the Command Line mode), the latter disappears.

Fortunately, switching the tab and coming back to the Console one make the Command Editor reappear.

Apparently, that is because of Firebug.SourceFile.getSourceFileByScript. I'll try to fix that this week-end.

By the way, FBTests are started.

Florent

Issue 6349 has been merged into this issue.

I created some FBTests there (in bulk): https://github.com/firebug/firebug/commit/9607cf083f0eeb41a989bca204c866ff89f1ccf

And updated the existing one for cd(), so it checks the error raised when the parameter is not a window: https://github.com/firebug/firebug/commit/b5e3724044032e974c3258eafcae2681c11af1ec

The issue about the |debugger;| statement is fixed, but only for the issue6291_master branch (I am not sure how to fix it in issue6291). An FBTest has been added in the commit above to check that: https://github.com/firebug/firebug/commit/d01c3a80b29f43959a1983b01d729cded1bdc9d0

So issue6291_master_merge has been updated: https://github.com/firebug/firebug/compare/issue6291_master_merge

Remaining questions: - should we keep that comment at the top of error sources?: https://github.com/firebug/firebug/blob/9607cf083f0eeb41a989bca204c866ff89f1ccfe/extension/content/firebug/console/commandLineExposed.js#L343 - There is still a problem when evaluating commands using "use strict"; (that's a regression) => could we ask for adding an option to JSD2's eval* methods, like evalInGlobalWithBindings(expr, bindings, {strictnessInExpressionOnly: true, ...})? (I can ask for that) - I think we should be able to specify the source each time we evaluate an expression with evalInGlobalWithBindings => could we propose an option to JSD2's eval* methods? like evalInGlobalWithBindings(expr, bindings, {source: expr});

Florent

Issue 3741 has been merged into this issue.

There are some commits in issue6291_master that are specific to the master branch, because the modified code is removed in the jsd2 branch: - fix failure for issue 5033: https://github.com/firebug/firebug/commit/b68c6ab29a1a8c7cd010630a6f9b6261db803cf6 - fix failure for issue 3645: https://github.com/firebug/firebug/commit/370ea219863aa3080d12878f63b54e4c40974e42 - fix evaluating "debugger;" hides the Command Editor: https://github.com/firebug/firebug/commit/issue6291_master/d01c3a80b29f43959a1983b01d729cded1bdc9d0

Except these ones, the issue6291 and the issue6291_master branches should share the same commits.

Remaining task for me: contact the ShareMeNot author to investigate the cause of the conflict.

Florent

Remaining task for me: contact the ShareMeNot author to investigate the cause of the conflict.

Done: https://groups.google.com/forum/?fromgroups=#!topic/sharemenot/535W0pEcsQE

Florent

1) I updated issue6291_master branch (merged with master) at: issue6291_master https://github.com/firebug/firebug/commit/c7d25a466299006cfb63515ec161f4354d6e3ef2

2) I also ran tests with issue6291_master and I am seeing 5 failures https://getfirebug.com/testresults?userheaderid=3cebd2143cba47f764a7ead4261ed98a Do you know why these tests fail?

Honza

Hmm. I don't know what would have caused this. I'll take a look.

Did you also tried on aurora?

Florent

Aurora: https://getfirebug.com/testresults/?dburi=http://firebug.couchone.com/&dbname=firebug2& userheaderid=adf6a5d0150e88265a0d5fc4d9000ab8 (only Fireclosure fails)

Btw. all tests pass with firebug/master on Firefox 23.

Honza

Aurora: https://getfirebug.com/testresults/?dburi=http://firebug.couchone.com/&dbname=firebug2& userheaderid=adf6a5d0150e88265a0d5fc4d9000ab8 (only Fireclosure fails)

Btw. all tests pass with firebug/master on Firefox 23.

Honza

(only Fireclosure fails) Have you merged with origin/issue6291_master?

For the rest of the errors, indeed I also get them. If we comment line 45 and 46 of this file, we don't get the error: https://github.com/firebug/firebug/blob/c7d25a466299006cfb63515ec161f4354d6e3ef2/extension/content/firebug/debugger/debuggerLib.js#L45

It looks like the object returned by unsafeDereference() don't have all the properties.

Simple STR: 1. Open Firebug 2. Enable and switch to the console panel 3. execute: var test = "ok"; 4. type (don't execute): window.tes |=> the auto-completion does not propose "window.test"

It looks like the user-defined properties are not taken into account. I will fill a bug to Mozilla.

Florent

What happens if you try "obj.unwrap().unsafeDereference()"?

The same :-(

Florent

XPCNativeWrapper.unwrap(obj.unsafeDereference())...?

A stand-alone test case would be nice; auto-completion does some weird stuff.

Yep, the comment #51 solves the problem. Thanks Simon!

Patch committed at: https://github.com/firebug/firebug/commit/1406e83e363e7e75f730983d9d8b3a1a379023f6

The only failing tests is commandLine/5873/issue5873.html, Issue 5873: Integrate FireClosure https://getfirebug.com/testresults/?dburi=http://firebug.couchone.com/&dbname=firebug2&userheaderid=9cd61e255694fc2ef5d66fd8f714c46d

Simon, do you know about that?

Otherwise, I think we can merge to master. Florent, can you do that please? I think it should be done as one commit.

Could you also merge to JSD2 branch? (ideally today, I need it for jsd2)

Thanks! Honza

Yep, well done Simon, thanks!

The only failing tests is commandLine/5873/ issue5873 .html, Issue 5873 : Integrate FireClosure

I had to update this FBTest (the update is on the issue5873_master branch). Did you try this version?

Otherwise, I think we can merge to master. Florent, can you do that please? I think it should be done as one commit. No problem :).

Could you also merge to JSD2 branch? (ideally today, I need it for jsd2) Hard for me before 19:00 (UTC+1).

So I propose these merges: - issue6291_master => master - issue6291 => jsd2

Florent

Comment deleted

Sounds good Florent, thanks! Honza

Oh, we forgot one point: should we have a header for the source of the errors? https://github.com/firebug/firebug/blob/974cdb0b0d978adc54a84ed9c289a0d1f2677f6b/extension/content/firebug/console/commandLineExposed.js#L341

STR: 1. Open Firebug on this page 2. Enable and switch to the console panel 3. Execute: alert() 4. Click on the source of the error |=> the source of the error appears. With what I propose, there is this header comment: /* EXPRESSION EVALUATED USING THE FIREBUG COMMAND LINE: */

I have no strong opinion about that question. I propose it by default and it is localized. But I don't have a strong opinion, it is up to you if you don't want it.

Just be aware to remove to adapt these two lines (remove the two "+ 1") in case of you want to remove that: https://github.com/firebug/firebug/blob/issue6291/extension/content/firebug/console/commandLineExposed.js#L353 https://github.com/firebug/firebug/blob/issue6291/extension/content/firebug/console/commandLineExposed.js#L365

Otherwise, Honza, here are the two branches containing the one-commits you want: - merge with master: https://github.com/firebug/firebug/blob/issue6291_master_merge/ - merge with jsd2: https://github.com/firebug/firebug/blob/issue6291_merge/

Sorry for the inconvenient.

Florent

should we have a header for the source of the errors? Sound like a good idea to me. Also the source link could already tell so by showing e.g. "Command Line (line 2)" instead of the non-descriptive "/* EXP...alert() (line 2)" in this case. Though this may be done in a later step.

Sebastian

Sound good to me too. Please create separate issue report for it. Honza

Just found the related issue 2444, which could used for that. Question is, what's the use case to show the user the evaluated code?

Sebastian

Syntax errors, at least. But I don't know if that falls under the same code path.

In theory also breakpoints, but command line sources aren't shown in the Script panel at the moment.

Branch: https://github.com/firebug/firebug/blob/issue6291_master_merge/ Merged to master at: https://github.com/firebug/firebug/commit/990719838d09a0aed26c8957d5137aa1ab636cd9

Branch: https://github.com/firebug/firebug/blob/issue6291_merge/ Merged to jsd2 at: https://github.com/firebug/firebug/commit/35e4bc7f89f3f16a758462f26577c3c149e2a21b

Finally, I merged master->jsd2 at: https://github.com/firebug/firebug/commit/8ed93c4d91c7613521c7a040d6ac48535d99c551

Florent, could you please review this last merge? I had to resolve some conflicts and it would be good to have some verification, thanks.

I run tests and all looks good.

I guess that the last thing is to merge issue5873_master branch

Honza

Florent, could you please review this last merge? I had to resolve some conflicts and it would be good to have some verification, thanks.

That's done: - in master and in jsd2 my commit has reverted 986efb32 (sorry). Fixed with https://github.com/firebug/firebug/commit/73276433044c237b270e44a7b95c54b709750ba3 and https://github.com/firebug/firebug/commit/d937ac1e3033954c48f311fff53f3e6426f97969 - in jsd2, after the merge, there were two redundant pieces of code. Fixed with https://github.com/firebug/firebug/commit/ce3ed38f9cf7d0a9512952a4ae1b15dc6f4826f1

I'll take a further look to the other merges this tomorrow or this week-end too.

I guess that the last thing is to merge issue5873_master branch I think you meant issue6291_master, don't you? Since you merged issue6291_master_merge, you don't have to ;).

Florent

Two new commits (fix an accidental revert + minor fixes): - on master: https://github.com/firebug/firebug/commit/3ae8e4a834cb7ef6a1c89328371f5a5e66d55b5d - on jsd2: https://github.com/firebug/firebug/commit/d774a0d5eca8e16533e9dbd5fd555d8efe32dcff

Florent

Issue 6416 has been merged into this issue.

Issue 3741 has been merged into this issue.

Issue 3741 has been merged into this issue.

This issue has been fixed in Firebug 1.12 alpha 5 https://getfirebug.com/releases/firebug/1.12/firebug-1.12.0a5.xpi

Please try it and let us know if it works for you.

Thanks for the help!

Honza

From my side, I confirm I can now execute code in console on https://github.com/ which was blocked previously (Fbug 1.11.2) subject to CSP violation.

Thanks! Jakub

Thanks for verifying!

Sebastian

Issue 6431 has been merged into this issue.

Issue 6629 has been merged into this issue.