It's very common that a page has a bunch of "eval" sources, especially during development, so right now it's very easy for the source listing to get cluttered since we show them right beside the file that introduced them. On IRC we agreed to move all unnamed (no sourceURL) evals to its own grouped called something like "evals" and force it to be the last group in the listing so they area always at the bottom.

Once we convert the source list UI into a proper tree it would be nice to show eval scripts as children of the scripts that introduced them. Until then, this seems like a good solution.

What Eddy said. Yesterday I was looking at these and counted one screen full (in a maximized devtools window) then counted the screens. At around 480 I got dragged away by something else then forgot about what I was doing and killed the session. So, again, what Eddy said ;)

Comment on attachment 8546077 [details] [diff] [review] 1111771.patch Review of attachment 8546077 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/debugger-panes.js @@ +10,5 @@ > const INDENT_COUNT_THRESHOLD = 5; // percentage > const CHARACTER_LIMIT = 250; // line character limit > > +// Maps known URLs to friendly source group names and put them at the > +// bottom of source list . @@ +181,5 @@ > + if(aSource.addonID) { > + group = aSource.addonID; > + } > + else if(aSource.introductionUrl) { > + group = 'Evals'; I feel like this should either be "eval" to match the non-localized function name, or it should be localized.

The (In reply to Nick Fitzgerald [:fitzgen] from comment #4) > > @@ +181,5 @@ > > + if(aSource.addonID) { > > + group = aSource.addonID; > > + } > > + else if(aSource.introductionUrl) { > > + group = 'Evals'; > > I feel like this should either be "eval" to match the non-localized function > name, or it should be localized. The whole functionality (which was already builtin to the source list) checks the name of the group to see if it's in KNOWN_SOURCE_GROUPS and moves it to the bottom of the list if it's in there. If we localize it, I'll have to rework this patch completely. I'm just not sure if it's worth it, but I can't speak for our localization efforts. We can just change it to `eval` if you want. If you really think this should be localized (though I still don't even see what "eval" would be changed to, unless we do a full title like "Evaluated Scripts"), that's fine, I'll just have to re-do this patch.

Some of us are drowning in this stuff. Either grab a bucket or ... Really.

(In reply to Russ from comment #6) > Some of us are drowning in this stuff. Either grab a bucket or ... > > Really. This will most likely land tomorrow; I think I've already figured out the l10n stuff. Not hard, just hadn't done it before. Also I was wrong about re-doing the patch, nick offered a solution over IRC that should work.

We will also uplift this and bug 1111058 to the Dev Edition channel immediately so all these fixes are available on the same train that eval integration is going out (and you'll pick them up very soon).

What??? Superb! Thank you!

Changed the eval group label to "Evaluated Sources" and made it localizable.

This patch introduces a new localizable string in a .properties file. I think I've seen a stuff in the release process about that so I just wanted to make that clear in case I need to flag anything else.

(In reply to James Long (:jlongster) from comment #12) > This patch introduces a new localizable string in a .properties file. I > think I've seen a stuff in the release process about that so I just wanted > to make that clear in case I need to flag anything else. Nothing special needed! It only gets awkward if you intended to uplift the change, as string changes generally aren't allowed to be uplifted.

Comment on attachment 8546229 [details] [diff] [review] 1111771.patch Approval Request Comment [Feature/regressing bug #]: 905700 [User impact if declined]: We landed support for debugging eval scripts in the debugger, but right now the user experience is pretty terrible, but it's already been moved to Aurora. We can't let this go out and wait to fix it 6 weeks later. eval sources saturate the debugger and make it impossible to find any normal scripts on the page. [Describe test coverage new/current, TBPL]: tests have been updated and passing on m-c [Risks and why]: Not much risk here [String/UUID change made/needed]: there is a localized string introduced in debugger.properties. I've heard that I may need to just make a patch that does not include this for Aurora, so it's un-localizable for now. Is that true?

(In reply to James Long (:jlongster) from comment #16) > [String/UUID change made/needed]: there is a localized string introduced in > debugger.properties. I've heard that I may need to just make a patch that > does not include this for Aurora, so it's un-localizable for now. Is that > true? We could also land a version of this patch without the localized string (use "`eval`s" instead) to aurora, if that makes it simpler to uplift.

Unfortunately, the bad UX of eval scripts was uplifted to beta. I didn't realize the uplift was last week so I would have really tried to push this through faster. :/ I really think we need to uplift this to beta, also. I'm going to do Nick's suggestion and make a patch that just uses "evals" that doesn't introduce a new localizable string. Luckily the patch is pretty small, so there's really not much risk, but the upside is it avoids a potential huge detriment to debugger.

This is the same patch without a localizable string.

Comment on attachment 8550550 [details] [diff] [review] 1111771-uplift.patch Approval Request Comment [Feature/regressing bug #]: 905700 [User impact if declined]: We landed support for debugging eval scripts in the debugger, but right now the user experience is pretty terrible, but it's already been moved to Aurora and Beta. We can't let this go out and wait to fix it 6 weeks later. eval sources saturate the debugger and make it impossible to find any normal scripts on the page. [Describe test coverage new/current, TBPL]: tests have been updated and passing on m-c [Risks and why]: Not much risk here [String/UUID change made/needed]:

(In reply to James Long (:jlongster) from comment #20) > Comment on attachment 8550550 [details] [diff] [review] > 1111771-uplift.patch > > Approval Request Comment > [Feature/regressing bug #]: 905700 > [User impact if declined]: We landed support for debugging eval scripts in > the debugger, but right now the user experience is pretty terrible, but it's > already been moved to Aurora and Beta. We can't let this go out and wait to > fix it 6 weeks later. eval sources saturate the debugger and make it > impossible to find any normal scripts on the page. > [Describe test coverage new/current, TBPL]: tests have been updated and > passing on m-c > [Risks and why]: Not much risk here > [String/UUID change made/needed]: Pike - I think this patch with the hard coded English string is preferable for Aurora and Beta. Can you please confirm? James - If you really think this needs to go to Beta (to avoid shipping the feature in the current state), please nom for Beta as well.

Yeah, we shouldn't uplift strings for this.

Comment on attachment 8550550 [details] [diff] [review] 1111771-uplift.patch Let's uplift this to Dev Edition with hard coded strings in 37. It would be good to assess the reception of this fix in Dev Edition if we're going to consider it for 36. Aurora+

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #23) > Comment on attachment 8550550 [details] [diff] [review] > 1111771-uplift.patch > > Let's uplift this to Dev Edition with hard coded strings in 37. It would be > good to assess the reception of this fix in Dev Edition if we're going to > consider it for 36. > > Aurora+ Agreed, that's why I didn't nominate for Beta quite yet. It would be good to uplift to Aurora first, wait a few days and make sure everything is ok. This is something everyone using our debugger is going to scream about if it hits release, to be honest. Go to any complex site (cnn, facebook, etc) and our debugger isn't very usable because it's filled up with eval scripts. I think it is pretty crucial that we get it to beta, where the eval debugger feature has been uplifted.

[Tracking Requested - why for this release]: Tracking to ensure we make a call on beta uplift before it's too late.

Guys, so, what do you want to do wrt beta? :)

(In reply to Sylvestre Ledru [:sylvestre] from comment #27) > Guys, so, what do you want to do wrt beta? :) James can make the call, I just didn't want this to get lost.

Definitely want to do this, just was waiting a little bit to make sure Aurora was working. I'll file the uplift.

Comment on attachment 8550550 [details] [diff] [review] 1111771-uplift.patch Approval Request Comment [Feature/regressing bug #]: 905700 [User impact if declined]: We landed support for debugging eval scripts in the debugger, but right now the user experience is pretty terrible, but it's already been moved to Beta. We can't let this go out and wait to fix it 6 weeks later. eval sources saturate the debugger and make it impossible to find any normal scripts on the page. [Describe test coverage new/current, TreeHerder]: tests have been updated and passing on m-c & aurora [Risks and why]: [String/UUID change made/needed]:

(In reply to James Long (:jlongster) from comment #30) > Comment on attachment 8550550 [details] [diff] [review] > 1111771-uplift.patch > > Approval Request Comment > [Feature/regressing bug #]: 905700 > [User impact if declined]: We landed support for debugging eval scripts in > the debugger, but right now the user experience is pretty terrible, but it's > already been moved to Beta. We can't let this go out and wait to fix it 6 > weeks later. eval sources saturate the debugger and make it impossible to > find any normal scripts on the page. > [Describe test coverage new/current, TreeHerder]: tests have been updated > and passing on m-c & aurora > [Risks and why]: > [String/UUID change made/needed]: // Maps known URLs to friendly source group names and put them at the "puts"