Created attachment 218830 [details] [review]
lookingGlass: Use a more standard chevron

'>>>' is used by the REPL history (as well as plenty of other REPLs).
Using 'js>>>' to enter things makes the history look unaligned and strange.

Created attachment 218831 [details] [review]
lookingGlass: Remove old signal handler

This 'selected' signal is from the days of the "Heirarchy" tab, before
it was replaced by the "Windows" tab. Those were the days.

Created attachment 218832 [details] [review]
link: Remove

The looking glass is the only consumer of the Link, and it's such a
basic wrapper around a button that it's not worth it.

Created attachment 218833 [details] [review]
lookingGlass: Don't use a global

As part of wanting to reuse some of the looking glass components,
don't use Main.lookingGlass, but instead pass the parent around.
Don't adjust the evaluator just yet, though. We'll split it into
a separate class soon.

Created attachment 218834 [details] [review]
lookingGlass: Use one red border effect

This makes the code a bit cleaner, and reduces the chances of
having a border effect "leaking", or having two at the same time.

Created attachment 218835 [details] [review]
lookingGlass: Don't use a command header

Instead, explicitly create a custom scope for the eval, which has the
declarations we need. This prevents leaking unintended things into the
eval call, and also will help us not rely on a global LookingGlass for
'it' and 'r'.

Created attachment 218836 [details] [review]
lookingGlass: Split off the Evaluator tab into its own class

Created attachment 218837 [details] [review]
lookingGlass: Keep track of tabs, not children

Code cleanups

Created attachment 218838 [details] [review]
lookingGlass: Align the inspector results to the bottom

Created attachment 218839 [details] [review]
util: Remove unused imports

Review of attachment 218830 [details] [review]:

I think I originally had it that way but someone asked it to change...anyways this sounds fine =)

Review of attachment 218831 [details] [review]:

Confirmed.

Review of attachment 218832 [details] [review]:

Ah, that's some old code dead now, nice.

Review of attachment 218833 [details] [review]:

Ok; I'm curious what the reuse is.

Review of attachment 218834 [details] [review]:

Odd that we had it in two places.  Ok.

Review of attachment 218835 [details] [review]:

::: js/misc/jsParse.js
@@ +2,3 @@
 
+function scopedEval(scope, expr) {
+    let func = new Function('with(this) { return ' + expr + '; }');

So the JS compiler people really, really hate "with", because it makes all sorts of optimizations much harder.

https://developer.mozilla.org/en/JavaScript/Reference/Statements/with

"Using with is not recommended, and is forbidden in ECMAScript 5 strict mode."

In this case, can't we expand the complete text of GLOBAL_SCOPE into each scopedEval, rather than evaluating it at global scope and passing that object down as "with" context?

Review of attachment 218836 [details] [review]:

::: js/ui/lookingGlass.js
@@ +1167,3 @@
             return;
 
+        if (!Main.pushModal(this.actor))

Hm, does it really work to call Main.pushModal() on a container actor and not the entry itself?

Review of attachment 218837 [details] [review]:

Looks fine.

Review of attachment 218838 [details] [review]:

The second hunk has nothing to do with alignment, but eh...I won't mind if you sneak it in.

Review of attachment 218839 [details] [review]:

Go go gadget commit.

(In reply to comment #16)
> Review of attachment 218835 [details] [review]:
> 
> ::: js/misc/jsParse.js
> @@ +2,3 @@
> 
> +function scopedEval(scope, expr) {
> +    let func = new Function('with(this) { return ' + expr + '; }');
> 
> So the JS compiler people really, really hate "with", because it makes all
> sorts of optimizations much harder.
> 
> https://developer.mozilla.org/en/JavaScript/Reference/Statements/with
> 
> "Using with is not recommended, and is forbidden in ECMAScript 5 strict mode."

I hate 'with' too, but I also hate 'eval' and things. I don't know of any way to lock the scope of an eval, so that 'a = global.get_window_actors()' doesn't get punted to the global object.

> In this case, can't we expand the complete text of GLOBAL_SCOPE into each
> scopedEval, rather than evaluating it at global scope and passing that object
> down as "with" context?

Something that does assignment, like our 'a = global.get_window_actors()' above, won't stick otherwise.

(In reply to comment #17)
> Review of attachment 218836 [details] [review]:
> 
> ::: js/ui/lookingGlass.js
> @@ +1167,3 @@
>              return;
> 
> +        if (!Main.pushModal(this.actor))
> 
> Hm, does it really work to call Main.pushModal() on a container actor and not
> the entry itself?

Since I navigate focus to the tab itself, afterwards in selectIndex, yes.
(In reply to comment #19)
> Review of attachment 218838 [details] [review]:
> 
> The second hunk has nothing to do with alignment, but eh...I won't mind if you
> sneak it in.

It does, though. Since I'm adding an actor, I can't just use get_children().length and destroy the first child, otherwise the bin will be destroyed.

(In reply to comment #21)

> I hate 'with' too, but I also hate 'eval' and things. 

The main problem with JavaScript's eval is that it might affect local scope, which means it has the same problems as "with".  See:

http://whereswalden.com/2011/01/10/new-es5-strict-mode-support-new-vars-created-by-strict-mode-eval-code-are-local-to-that-code-only/

Other languages like Scheme don't have that problem.

> I don't know of any way
> to lock the scope of an eval, so that 'a = global.get_window_actors()' doesn't
> get punted to the global object.

Right.  That concern makes sense...if you think it's worth using "with" for that, then that's OK by me.  But if we were moving towards strict mode (which might make sense eventually), we'd have to drop "with".

(Feel free to commit either way)

Attachment 218830 [details] pushed as 8b81f23 - lookingGlass: Use a more standard chevron
Attachment 218831 [details] pushed as 4f7c554 - lookingGlass: Remove old signal handler
Attachment 218832 [details] pushed as 75d0362 - link: Remove
Attachment 218833 [details] pushed as d0807c8 - lookingGlass: Don't use a global
Attachment 218834 [details] pushed as 360e6e7 - lookingGlass: Use one red border effect

Comment on attachment 218839 [details] [review]
util: Remove unused imports

Attachment 218839 [details] pushed as 76472b8 - util: Remove unused imports

Let's close this as the eval changes were bad.