Last Comment Bug 777093 - Long script urls still confuse the debugger menulist
: Long script urls still confuse the debugger menulist
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: 12 Branch
: All All
: P2 normal (vote)
: Firefox 17
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-24 14:52 PDT by Victor Porof [:vporof][:vp]
Modified: 2012-08-03 06:48 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.83 KB, patch)
2012-07-25 04:49 PDT, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review
fail (5.20 KB, patch)
2012-08-01 09:08 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review

Comment 1 Victor Porof [:vporof][:vp] 2012-07-25 04:34:40 PDT
Yup, regression from patch in bug 771481 using sizetopopup="always" on the menulist.
Comment 2 Victor Porof [:vporof][:vp] 2012-07-25 04:49:06 PDT
Created attachment 645712 [details] [diff] [review]
v1

Asking Rob in for review.
Comment 3 Rob Campbell [:rc] (:robcee) 2012-07-31 08:42:18 PDT
Comment on attachment 645712 [details] [diff] [review]
v1

+const SCRIPTS_URL_MAX_LENGTH = 64; // chars

magic numbers! How did you arrive at 64? It's a nice power of two, a square and a cube. Its digits add up to 10. It has many extremely nice properties. But as a length of characters in a menu list?

+      ok(gDebugger.DebuggerView.Scripts.containsLabel(nanana + "Batman!" + ellipsis),
+        "Script (15) label is incorrect.");
+

haha. "wat"
Comment 4 Victor Porof [:vporof][:vp] 2012-07-31 09:14:21 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #3)
> Comment on attachment 645712 [details] [diff] [review]
> v1
> 
> +const SCRIPTS_URL_MAX_LENGTH = 64; // chars
> 

In addition to your valid arguments, 20 is dividable by 2 and 5, looks good and
(new Array(20).join(NaN) + "Batman!").length === 64.
Sorry.

In all seriousness, it feels like an acceptable number of chars to use and keep the menulist at an acceptable size. If you can think of other good algorithms to find a better number, please share.
Comment 5 Rob Campbell [:rc] (:robcee) 2012-07-31 12:46:55 PDT
nah, is cool.
Comment 6 Victor Porof [:vporof][:vp] 2012-08-01 09:08:25 PDT
Created attachment 647971 [details] [diff] [review]
fail
Comment 7 Victor Porof [:vporof][:vp] 2012-08-01 09:09:17 PDT
Comment on attachment 647971 [details] [diff] [review]
fail

Wrong bug!
Comment 8 Victor Porof [:vporof][:vp] 2012-08-01 14:01:04 PDT
https://hg.mozilla.org/integration/fx-team/rev/3e77bd3b1596
Comment 9 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-08-02 04:22:09 PDT
https://hg.mozilla.org/mozilla-central/rev/3e77bd3b1596
Comment 10 Ed Morley [:emorley] 2012-08-03 06:27:07 PDT
https://groups.google.com/d/topic/mozilla.dev.tree-management/2jB_s0SpJ80/discussion

Regression :( Tp5 No Network Row Major MozAfterPaint increase 12.4% on Linux Fx-Team
------------------------------------------------------------------------------------
    Previous: avg 243.519 stddev 16.676 of 30 runs up to revision fcb3934935e2
    New     : avg 273.667 stddev 2.315 of 5 runs since revision 834a0c1ef40b
    Change  : +30.148 (12.4% / z=1.808)
    Graph   : http://mzl.la/QmD6Pd
Comment 11 Panos Astithas [:past] 2012-08-03 06:48:21 PDT
I think from the graph it is evident that this regression is caused by the merge from m-c in changeset 8c63e260c4fa, which is conspicuously absent from the graph, but occurred right in the upward slope between revs f08a7ecc6d9f and 4f39921f782f.

This is way too low-level to be affected by debugger UI patches.

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