991376 - The variables inspection popup shouldn't open when hovering js literals

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Victor Porof [:vporof][:vp]]

When hovering "foo", you don't want to see \""foo\"" in a variable inspection popup. You already see what it is :)

Comment 1

[Victor Porof [:vporof][:vp]]

And for people who are really troubled by the poor escaping in comment 0, I obviously meant, "\"foo\"".

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8400964 [details] [diff] [review]
v1

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

Make sure you fix the typo, and I wouldn't mind getting rid of the specific tests FWIW.

::: browser/devtools/debugger/debugger-panes.js
@@ +1814,5 @@
>    /**
> +   * Specifies whether literals can be (redundantly) inspected in a popup.
> +   * This behavior is deprecated, but still tested in a few places.
> +   */
> +  _ignoreLiterals: true,

Do you think it's still worth keeping the behavior and tests around though?

::: browser/devtools/debugger/debugger.xul
@@ +56,5 @@
>               oncommand="DebuggerView.Filtering._doFileSearch()"/>
>      <command id="globalSearchCommand"
>               oncommand="DebuggerView.Filtering._doGlobalSearch()"/>
>      <command id="functionSearchCommand"
> +             oncommand="DepbuggerView.Filtering._doFunctionSearch()"/>

Eeek!

::: browser/devtools/debugger/test/browser_dbg_variables-view-popup-15.js
@@ +25,5 @@
> +      ok(false, "The variable inspection popup shouldn't have been opened.");
> +    });
> +
> +    reopenVarPopup(panel, { line: 17, ch: 27 });
> +    yield waitForTime(1000);

Hmmm, this could give false negatives, for example when we've broken that code and the popup is going to show up at some point, but the machine is overloaded and stalls. The alternative would be to reach deep into the implementation code from the test and infer the code path taken. Not sure if it's worth the effort though for something that we'll eventually catch.

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #3)
> Comment on attachment 8400964 [details] [diff] [review]
> v1
> 
> Review of attachment 8400964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Make sure you fix the typo, and I wouldn't mind getting rid of the specific
> tests FWIW.
> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +1814,5 @@
> >    /**
> > +   * Specifies whether literals can be (redundantly) inspected in a popup.
> > +   * This behavior is deprecated, but still tested in a few places.
> > +   */
> > +  _ignoreLiterals: true,
> 
> Do you think it's still worth keeping the behavior and tests around though?

It's just one test. I don't know actually, it might be? I don't see *too much* keeping this around, at least for a short while.

> ::: browser/devtools/debugger/debugger.xul
> @@ +56,5 @@
> >               oncommand="DebuggerView.Filtering._doFileSearch()"/>
> >      <command id="globalSearchCommand"
> >               oncommand="DebuggerView.Filtering._doGlobalSearch()"/>
> >      <command id="functionSearchCommand"
> > +             oncommand="DepbuggerView.Filtering._doFunctionSearch()"/>
> 
> Eeek!
> 

Damn, there's some accidental changes in this patch. I'll remove them.

Thanks for the review!

Comment 5

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/a17657a71b01

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out (along with the other patches in that push) in https://hg.mozilla.org/integration/fx-team/rev/fc4b6e71a8e7 for causing browser-chrome-1 tests to start leaking by adding tests, which pushed other tests into a different chunk, making them leak.

Comment 7

[Victor Porof [:vporof][:vp]]

Please backout the backout at some point.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

*shuffles the deck*
https://hg.mozilla.org/integration/fx-team/rev/57a393d69f32

Comment 9

[Victor Porof [:vporof][:vp]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #8)
> *shuffles the deck*
> https://hg.mozilla.org/integration/fx-team/rev/57a393d69f32

Thank you.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/57a393d69f32