Closed Bug 913665 Opened 11 years ago Closed 10 years ago

Detect minified files and pretty print them by default

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: rik, Assigned: b4bomsy)

References

Details

Attachments

(2 files, 13 obsolete files)

25.27 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
2.43 KB, patch
fitzgen
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
Once we get pretty-printing, it will be nice to automatically do it for files that look minified. One less click for the general use case.
Priority: -- → P3
Would love to work on this.
(In reply to Hubert B Manilla from comment #1)
> Would love to work on this.

We need bug 916180 fixed before this can land, but once that happens, this should be pretty straight forward.

We need a heuristic for determining if a file is minified or not. Ideally we should not have to parse the source. Things that might be useful:

* Does the file end with ".min.js"?

* What is the median line length of the file? (This breaks when a multi line license comment is on top of one really long minified line)

* What is the mean line length in the file? (This breaks if there are base 64 encoded data URIs embedded as strings in the code)

* Ratio of avg (mean or median) line length over number of lines?

As far as UI goes, I think we should have a notification box (banner thing) across the top of the whole source editor that says "hey, we pretty printed this file for you, you can <undo> that if you want".
Assignee: nobody → b4bomsy
No longer blocks: 916180
Depends on: 916180
(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> We need a heuristic for determining if a file is minified or not. Ideally we
> should not have to parse the source. Things that might be useful:
If you need more heuristics, I'm sure the WebKit codebase can help.

> As far as UI goes, I think we should have a notification box (banner thing)
> across the top of the whole source editor that says "hey, we pretty printed
> this file for you, you can <undo> that if you want".
That sounds very noisy. Just marking the toggle icon as "activated" should be enough information.
(In reply to Anthony Ricaud (:rik) from comment #3)
> That sounds very noisy. Just marking the toggle icon as "activated" should
> be enough information.

I think I disagree - I think a relatively unobtrusive notification box would go a long way to explain what the heck just happened, especially the first time it happens. Maybe we could also offer the ability to turn the notification off?
(In reply to Jeff Griffiths (:canuckistani) from comment #4)
> I think I disagree - I think a relatively unobtrusive notification box would
> go a long way to explain what the heck just happened, especially the first
> time it happens.
What is surprising or confusing? This is just nice indenting. If the heuristics are good enough, no one should disable pretty-printing in practice.
To me, a notification is interrupting the mental flow of debugging, you have to read the box instead of benefiting from the tool doing something nice for you.

> Maybe we could also offer the ability to turn the notification off?
More buttons? :(
(In reply to Anthony Ricaud (:rik) from comment #5)
> > Maybe we could also offer the ability to turn the notification off?
> More buttons? :(

This button can be on the notification itseld like we do in responsive mode when you enable touch events. It goes like:

<icon />If touch event listeners have been added earlier, the page needs to be reloaded<spacer flex="1"/>[Reload][Never show again]<close button/>
(In reply to Anthony Ricaud (:rik) from comment #5)
> (In reply to Jeff Griffiths (:canuckistani) from comment #4)
> > I think I disagree - I think a relatively unobtrusive notification box would
> > go a long way to explain what the heck just happened, especially the first
> > time it happens.
> What is surprising or confusing? This is just nice indenting. If the
> heuristics are good enough, no one should disable pretty-printing in
> practice.
> To me, a notification is interrupting the mental flow of debugging, you have
> to read the box instead of benefiting from the tool doing something nice for
> you.
> 
My view is the source file is loaded into the debugger in an original state (maybe minified). if we have to alter the state by pretty printing it automatically, It would be necessary to let the user know about the changes each time it happens.

> > Maybe we could also offer the ability to turn the notification off?
> More buttons? :(

Or maybe we could display the notification and after a couple of secs , ti goes off automatically. :(
But we are letting the user know that we did something. By having a button mark enabled and also by having the whole source look nicer. This is already two signals. Don't make me read unnecessary messages when all I want is to debug something.

Here's a proposal. Let's ship it with only this and if people complain, we can add this notification in a later version. (my bet is no one will complain)
(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> As far as UI goes, I think we should have a notification box (banner thing)
> across the top of the whole source editor that says "hey, we pretty printed
> this file for you, you can <undo> that if you want".

(In reply to Jeff Griffiths (:canuckistani) from comment #4)
> (In reply to Anthony Ricaud (:rik) from comment #3)
> > That sounds very noisy. Just marking the toggle icon as "activated" should
> > be enough information.
> 
> I think I disagree - I think a relatively unobtrusive notification box would
> go a long way to explain what the heck just happened, especially the first
> time it happens. Maybe we could also offer the ability to turn the
> notification off?

Notifications... they're very noisy and shift the content. We used to have a couple of them in the inspector and in the style editor. People complained. We removed them (it was a little more intrusive though).

What's the point of warning the user? To let him know? If the pretty print button is active, and we actually pretty printed the code, it's super obvious that we have pretty-printed the file, no need to tell the user.

The only information you get is "btw, the file was originally minified".  To me, this sounds like useless noise.

(In reply to Anthony Ricaud (:rik) from comment #8)
> Here's a proposal. Let's ship it with only this and if people complain, we
> can add this notification in a later version. (my bet is no one will
> complain)

I like that.
no notifications, please. No notifications with extra buttons and checkboxes you have to think about to not see again.
(In reply to Anthony Ricaud (:rik) from comment #8)
> Here's a proposal. Let's ship it with only this and if people complain, we
> can add this notification in a later version. (my bet is no one will
> complain)

I agree with this, assuming other issues are worked out.
Why isn't the pretty-print button a checkbox, like the pause/resume button? The blue color would make it more obvious when minification was performed.
(In reply to Panos Astithas [:past] from comment #12)
> Why isn't the pretty-print button a checkbox, like the pause/resume button?
> The blue color would make it more obvious when minification was performed.

Because we can't toggle pretty printing yet, but will soon, at which point this would make sense. See bug 916180.
Hubert, if you are still interested in working on this, bug 916180 just landed and so we can move forward with this bug. If you're not still interested, please un-assign yourself from the bug :)

Cheers!
Nice being waiting..i still want to work on it. On heuristics, would we need more than one or we just need one reliable ?
(In reply to Hubert B Manilla from comment #15)
> Nice being waiting..i still want to work on it. On heuristics, would we need
> more than one or we just need one reliable ?

We just need reliability. As Anthony said in comment 3, it might be worth looking at the webkit code to get an idea what heuristic(s) they use.

I think that median line length, where we only consider lines that aren't comments, would be a pretty good way to sniff minified code.

Also, we don't have to be perfect (especially in the first iteration). Its ok if we have a couple false negatives, false positives are a bit more annoying. We can always land something that is pretty good, and then follow it up with patches to make it even better.
Also, might look at the mean/median amount of indentation on each line. This would probably work well even if we didn't filter out lines that are only comments.
(In reply to Nick Fitzgerald [:fitzgen] from comment #16)
> (In reply to Hubert B Manilla from comment #15)
> > Nice being waiting..i still want to work on it. On heuristics, would we need
> > more than one or we just need one reliable ?
> 
> We just need reliability. As Anthony said in comment 3, it might be worth
> looking at the webkit code to get an idea what heuristic(s) they use.

What i found within the webkit source
https://github.com/WebKit/webkit/blob/master/Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js#L298-L312

* At least one line in the source should be above the AutoFormatMinimumLineLength (500) limit?
500 characters? That seems excessive, but okay. I can see how some non-minified code might have 1-2 very long lines because someone pasted in data. I like Nick's idea that median line length could be a factor?
Depends on: 921163
Depends on: 930141
No longer depends on: 921163
I believe the two bugs I just added as dependencies are important to fix before enabling this. Auto-pretty printing is nice is it gets out of the way and I feel "no comments" and "5 seconds to open a file" are preventing this.
(In reply to Anthony Ricaud (:rik) from comment #20)
> I believe the two bugs I just added as dependencies are important to fix
> before enabling this. Auto-pretty printing is nice is it gets out of the way
> and I feel "no comments" and "5 seconds to open a file" are preventing this.

We can have this behavior behind a pref, and default the pref to false if I don't fix bug 930141 before Hubert fixes this bug.

The lack of comments shouldn't block this because if it is minified anyways, there will maybe be a license block, but that's it. Also, prefing it off is still applicable.
No longer depends on: 930141
Attached patch wip.patch (obsolete) — Splinter Review
Would we need a test for this?
Attachment #829936 - Flags: feedback?(nfitzgerald)
(In reply to Hubert B Manilla from comment #22)
> Created attachment 829936 [details] [diff] [review]
> wip.patch
> 
> Would we need a test for this?

Yes, we will need a test. Will take a look at the patch tomorrow :)
Comment on attachment 829936 [details] [diff] [review]
wip.patch

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

Looks like you are on the right path!

Tests I would like to see:

* That we correctly auto pretty print a minified file
  * The file should have a big block comment, similar to how license blocks are often found on minified files.

* That if we auto pretty print a small source that only ends up with one indented line after pretty printing, select a different source, and select the pretty printed source again, we don't accidentally toggle pretty printing off.

Small nit: please capitalize the first word and punctuate in your comments :)

::: browser/devtools/debugger/debugger-panes.js
@@ +686,5 @@
>      DebuggerView.setEditorLocation(sourceItem.value);
>  
> +    DebuggerController.SourceScripts.getText(sourceItem.attachment.source).then(([, aText]) => {
> +      if (SourceUtils.isMinified(aText)){
> +        this.togglePrettyPrint();

You should check if the source is already pretty printed, and avoid even calling getText if it is.

Can you create a new boolean preference (say "devtools.debugger.auto-pretty-print") and only automatically pretty print if the pref is true?

This should also be added to the debugger options dropdown.

Define the pref here: http://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#1107

Grab the pref in the front end here: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#1882

Add the dropdown item in the options here: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-toolbar.js#189

and here: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger.xul#160

@@ +1054,5 @@
> +    let indentCount = 0;
> +    // strip comments
> +    aText = aText.replace(/\/\*[\S\s]*?\*\/|\/\/.+/g, "");
> +    // use 10 lines
> +    while(lines < 11){

Can we make this a constant instead of hardcoding 10 lines? We might want to tweak this in the future and that is easier if it is in a variable.

@@ +1065,5 @@
> +      }
> +      lineStartIndex = lineEndIndex + 1;
> +      lines++;
> +    }
> +    return indentCount < 2;

Why 2? Why not 3? Why not 1? Should this be a constant instead of hard coded?

Would appreciate more comments on higher level approach of what heuristic we are using to determine if the file is minified. This could be a good start:

  // Use the presence (or lack of presence) of indentation to determine
  // if the JS text is minified or not.
Attachment #829936 - Flags: feedback?(nfitzgerald) → feedback+
Attachment #829936 - Attachment is obsolete: true
Attachment #8340711 - Flags: feedback?(nfitzgerald)
Attachment #8340711 - Attachment is obsolete: true
Attachment #8340711 - Flags: feedback?(nfitzgerald)
Attachment #8340712 - Flags: feedback?(nfitzgerald)
Attachment #8340712 - Flags: feedback?(past)
Hey Hubert, I've been on vacation, but I'm back now and will take a look at this today. Sorry for the delay!
Comment on attachment 8340712 [details] [diff] [review]
BUG 913665 - Detect and auto pretty print minified files, include tests

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

Overall this is coming along really well.

I mostly have a bunch of little style nits, but I also have a couple small concerns about the |isMinified| helper and single line comments.

Looking forward to the next iteration of the patch!

::: browser/devtools/debugger/debugger-panes.js
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
> +const SAMPLE_SIZE = 30; // no of lines
> +const INDENT_COUNT_THRESHOLD = 20; // percentage

Nit: Please add a comment above these declarations saying they are used to detect minification for automatic pretty printing. Just to give a little context.

@@ +691,4 @@
>      // The container is not empty and an actual item was selected.
>      DebuggerView.setEditorLocation(sourceItem.value);
>  
> +    if(Prefs.autoPrettyPrint && !sourceClient.isPrettyPrinted){

It would be awesome if we could avoid checking if the source is minified if we have already checked before and determined that it is not. Perhaps a WeakMap that maps from a sourceClient to a boolean indicating whether or not the source is minified?

@@ +1050,5 @@
>      return /\.jsm?$/.test(this.trimUrlQuery(aUrl)) ||
>             aContentType.contains("javascript");
>    },
> +  /**
> +   * Determines is the source text is minified.

Nit: that first "is" should be "if". Also, either add a blank line between this sentence and the next or remove the single newline between them.

@@ +1051,5 @@
>             aContentType.contains("javascript");
>    },
> +  /**
> +   * Determines is the source text is minified.
> +   * Using the percentage no. of the indented lines from a sample set of lines to determine if the js file

Nit: please wrap lines at 80 columns.

@@ +1065,3 @@
>  
> +    // Strip comments.
> +    aText = aText.replace(/\/\*[\S\s]*?\*\/|\/\/.+/g, "");

This regexp doesn't strip the newline following a // comment (if it exists), but it probably should, shouldn't it? Otherwise we will still loop over the comment's line, even though it is empty.

@@ +1065,5 @@
>  
> +    // Strip comments.
> +    aText = aText.replace(/\/\*[\S\s]*?\*\/|\/\/.+/g, "");
> +
> +    while(lines < SAMPLE_SIZE){

Style nit: please add a space between "while" and "(" and between ")" and "{".

@@ +1076,5 @@
> +      }
> +      lineStartIndex = lineEndIndex + 1;
> +      lines++;
> +    }
> +    return (indentCount * 100) < INDENT_COUNT_THRESHOLD;

Your comparing the number of indented lines found to a percentage. Type conversion error! It should be

> return ((indentCount / lines) * 100) < INDENT_COUNT_THRESHOLD;

right?

But |lines| can be zero if there is only one line in the script, so the |lines++| should be moved to the top of the loop, or the loop condition could be re-written as

> while (lines++ < SAMPLE_SIZE) {

@@ +1077,5 @@
> +      lineStartIndex = lineEndIndex + 1;
> +      lines++;
> +    }
> +    return (indentCount * 100) < INDENT_COUNT_THRESHOLD;
> +  },

Style nit: blank line after this new method before the next one.

::: browser/devtools/debugger/debugger-toolbar.js
@@ +255,5 @@
>     */
>    _onPopupHidden: function() {
>      window.emit(EVENTS.OPTIONS_POPUP_HIDDEN);
>    },
> +  /**

Nit: newlines before and after this method.

::: browser/devtools/debugger/test/browser.ini
@@ +27,5 @@
> +  code_ugly-5.js
> +  code_ugly-6.js
> +  code_ugly-7.js
> +  doc_auto-pretty-print.html
> +  doc_auto-pretty-print-01.html

-01 and -02 if you want.

::: browser/devtools/debugger/test/browser_dbg_auto-pretty-print-01.js
@@ +1,1 @@
> +/* -*- Mode: javascript; js-indent-level: 2; -*- */

2 spaces instead of tabs in this, and all other files.

@@ +2,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> +* Test auto pretty printing is not accidentally toggled off

Nit: the * on this line should match up with the first * on the line above and so on for the next lines in this comment too.

@@ +35,5 @@
> +			.then(testSourceIsUgly)
> +			.then(() => {
> +				const finished = waitForSourceShown(gPanel, gFirstSourceLabel);
> +				return finished;
> +			})

Could simplify this to

> .then(() => waitForSourceShown(gPanel, gFirstSourceLabel))

which is a bit cleaner.

::: browser/devtools/debugger/test/browser_dbg_auto-pretty-print.js
@@ +1,1 @@
> +/* -*- Mode: javascript; js-indent-level: 2; -*- */

2 spaces, not tabs.

@@ +73,5 @@
> +}
> +
> +function disableAutoPrettyPrint(){
> +	gOptions._autoPrettyPrint.setAttribute("checked", "false");
> +	gOptions._toggleAutoPrettyPrint();

You need to call

> gOptions._onPopupHidden();

after you toggle the pretty print, and whenever you are dealing with the options menu. I wish we had a better way to deal with this stuff :(

::: browser/devtools/debugger/test/code_ugly-5.js
@@ +3,5 @@
> + * Copyright Test Inc.
> + *
> + * Licensed under the Apache License, Version 2.0
> + * http://www.apache.org/licenses/LICENSE-2.0
> + */

Can you also add this comment but with single line "//" style comments to ensure that they get ignored as well? (See my other comment about how the regexp doesn't remove those newlines).

> // Copyright Test Inc.
> //
> // etc...
> // etc...
Attachment #8340712 - Flags: feedback?(nfitzgerald)
Comment on attachment 8340712 [details] [diff] [review]
BUG 913665 - Detect and auto pretty print minified files, include tests

Nick is already on this.
Attachment #8340712 - Flags: feedback?(past)
Attached patch wip.patch (obsolete) — Splinter Review
Attachment #8340712 - Attachment is obsolete: true
Attachment #8347230 - Flags: feedback?(nfitzgerald)
Attached patch wip.patch (obsolete) — Splinter Review
Attachment #8347230 - Attachment is obsolete: true
Attachment #8347230 - Flags: feedback?(nfitzgerald)
Attachment #8347238 - Flags: feedback?(nfitzgerald)
Attached patch n1.patch (obsolete) — Splinter Review
Attachment #8347238 - Attachment is obsolete: true
Attachment #8347238 - Flags: feedback?(nfitzgerald)
Attachment #8347265 - Flags: feedback?(nfitzgerald)
Attached patch n2.patch (obsolete) — Splinter Review
Attachment #8347265 - Attachment is obsolete: true
Attachment #8347265 - Flags: feedback?(nfitzgerald)
Attachment #8347268 - Flags: feedback?(nfitzgerald)
Attached patch try (obsolete) — Splinter Review
Attachment #8347268 - Attachment is obsolete: true
Attachment #8347268 - Flags: feedback?(nfitzgerald)
Attachment #8347270 - Flags: feedback?(nfitzgerald)
Attached patch try (obsolete) — Splinter Review
Attachment #8347270 - Attachment is obsolete: true
Attachment #8347270 - Flags: feedback?(nfitzgerald)
Attachment #8347290 - Flags: feedback?(nfitzgerald)
Attached patch wip.patch (obsolete) — Splinter Review
Sorry Nick..
Attachment #8347290 - Attachment is obsolete: true
Attachment #8347290 - Flags: feedback?(nfitzgerald)
Attachment #8347302 - Flags: feedback?(nfitzgerald)
Haha no problems. About to take a look, just a couple quick notes:

* If you want to share the try results for a push, just drop the link in the comments field when you upload the patch

* Use "review?" when you think a patch is ready to land and want the final go ahead, and "feedback?" when you just want some quick feedback on a specific thing, or the approach you are taking, or what have you.
Comment on attachment 8347302 [details] [diff] [review]
wip.patch

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

Looks good. Content-wise I this is almost ready to land, just a couple very small comments. Style-wise this patch needs work. Just make the style look like the rest of the code, and if you have specific questions feel free to ask me. I shouldn't have to tell you to use 2 space indents and to put a space between "if" and "(" anymore.

::: browser/devtools/debugger/debugger-panes.js
@@ +388,5 @@
>      }
>  
>      const resetEditor = ([{ url }]) => {
>        // Only set the text when the source is still selected.
> +    if (url == this.selectedValue) {

Please undo the un-indentation of this line.

@@ +694,4 @@
>      // The container is not empty and an actual item was selected.
>      DebuggerView.setEditorLocation(sourceItem.value);
>  
> +    if(Prefs.autoPrettyPrint && SourceUtils.isSourceMinified(sourceClient)){

C'mon man! We have spaces before "(" and after ")" with while/if/for/etc! I've nitted you on this a few times already! Just Use Spaces! :D

@@ +698,5 @@
> +      DebuggerController.SourceScripts.getText(source).then(([, aText]) => {
> +        if (SourceUtils.isMinified(aText)){
> +          this.togglePrettyPrint();
> +        }
> +      });

Sorry for not catching this earlier, but we should add some error handling to this.

  .then(null, e => DevToolsUtils.reportException("_onSourceSelect", e));

@@ +1069,5 @@
> +
> +    // Strip comments.
> +    aText = aText.replace(/\/\*[\S\s]*?\*\/|\/\/(.+|\n)/g, "");
> +
> +    while ( lines++ < SAMPLE_SIZE ) {

We don't do spaces on the inside, just the outside:

  while (lines++ < SAMPLE_SIZE) {

@@ +1071,5 @@
> +    aText = aText.replace(/\/\*[\S\s]*?\*\/|\/\/(.+|\n)/g, "");
> +
> +    while ( lines++ < SAMPLE_SIZE ) {
> +      lineEndIndex = aText.indexOf("\n", lineStartIndex);
> +      if(lineEndIndex == -1){

_ is a space that needs to be added:

  if_(lineEndIndex == -1)_{

@@ +1074,5 @@
> +      lineEndIndex = aText.indexOf("\n", lineStartIndex);
> +      if(lineEndIndex == -1){
> +         break;
> +      }
> +      if(/^\s+/.test(aText.slice(lineStartIndex, lineEndIndex))){

Again:

  if_(...)_{

@@ +1103,5 @@
> +   */
> +  isSourceMinified: function(sourceClient){
> +    let cachedClient = this._minifyCache.get(sourceClient);
> +    if (cachedClient) {
> +      return cachedClient;

This is supposed to be

  return cachedClient.isPrettyPrinted;

right?

::: browser/devtools/debugger/test/browser_dbg_auto-pretty-print-01.js
@@ +11,5 @@
> +let gFirstSourceLabel = "code_ugly-5.js";
> +let gSecondSourceLabel = "code_ugly-6.js";
> +
> +function test(){
> +	initDebugger(TAB_URL).then(([aTab, aDebuggee, aPanel]) => {

2 space indents, not tabs, remember?

::: browser/devtools/debugger/test/browser_dbg_auto-pretty-print-02.js
@@ +14,5 @@
> +let gFirstSourceLabel = "code_ugly-6.js";
> +let gSecondSourceLabel = "code_ugly-7.js";
> +
> +function test(){
> +	initDebugger(TAB_URL).then(([aTab, aDebuggee, aPanel]) => {

2 space indents.

@@ +34,5 @@
> +			.then(testSourceIsUgly)
> +			.then(() => {
> +				const finished = waitForSourceShown(gPanel, gFirstSourceLabel);
> +				return finished;
> +			})

This would be better written as

  .then(() => waitForSourceShown(gPanel, gFirstSourceLabel));

::: browser/devtools/debugger/test/doc_auto-pretty-print-01.html
@@ +2,5 @@
> +     http://creativecommons.org/publicdomain/zero/1.0/ -->
> +<!DOCTYPE html>
> +<html>
> +<head>
> +	<meta charset="utf-8" />

2 space indents.

::: browser/devtools/debugger/test/doc_auto-pretty-print-02.html
@@ +2,5 @@
> +     http://creativecommons.org/publicdomain/zero/1.0/ -->
> +<!DOCTYPE html>
> +<html>
> +<head>
> +	<meta charset="utf-8" />

2 space indents.

::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
@@ +42,5 @@
>  <!ENTITY debuggerUI.sources.prettyPrint "Prettify Source">
>  
> +<!-- LOCALIZATION NOTE (debuggerUI.autoPrettyPrint): This is the lable for the
> +  - checkbox that toggles auto pretty print. -->
> +<!ENTITY debuggerUI.autoPrettyPrint     "Auto pretty print">

Is "Auto prettify minified sources" too long? We are auto pretty printing every source, just the minified ones, and I think we should convey that if possible. We should use "prettify" instead of "pretty print" for consistency with what we use now in the pretty print button's tooltip.
Attachment #8347302 - Flags: feedback?(nfitzgerald)
 Sorry for being slack with the spacing...
> 
> @@ +1103,5 @@
> > +   */
> > +  isSourceMinified: function(sourceClient){
> > +    let cachedClient = this._minifyCache.get(sourceClient);
> > +    if (cachedClient) {
> > +      return cachedClient;
> 
> This is supposed to be
> 
>   return cachedClient.isPrettyPrinted;
> 
> right?
> 
No not really. What is stored in the cache is the sourceClient.isPrettyPrinted value not the sourceClient, maybe i should change the name of the variable, so it is clearer?
Comment on attachment 8347302 [details] [diff] [review]
wip.patch

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

(In reply to Hubert B Manilla from comment #39)
> No not really. What is stored in the cache is the
> sourceClient.isPrettyPrinted value not the sourceClient, maybe i should
> change the name of the variable, so it is clearer?

Ah yes, woops I was misreading the code. Yeah, I think we could just call that variable |cached|, the |Client| part is kind of misleading.

Also, we are caching the wrong thing in the WeakMap. We are caching whether or not it is pretty printed, we should be caching whether or not we think it is minified based on our heuristics. Does that make sense? The name of the method is correct, but the thing it does is incorrect. See below.

Thanks for following through with this bug!

::: browser/devtools/debugger/debugger-panes.js
@@ +1107,5 @@
> +      return cachedClient;
> +    }
> +    let val = !sourceClient.isPrettyPrinted;
> +    this._minifyCache.set(sourceClient, val);
> +    return val;

This should be

>  let val = SourceUtils.isMinified(textForSourceClient);
>  this._minifyCache.set(sourceClient, val);
>  return val;

We want to minimize the amount of times we run the |isMinified| heuristics; accessing the |isPrettyPrinted| getter isn't expensive or redundant.

I expect this will require some slight changes to the logic in |_onSelect| as well.
(In reply to Nick Fitzgerald [:fitzgen] from comment #40)
> Comment on attachment 8347302 [details] [diff] [review]
> wip.patch
> 
> Review of attachment 8347302 [details] [diff] [review]:
> -----------------------------------------------------------------
>  
> Also, we are caching the wrong thing in the WeakMap. We are caching whether
> or not it is pretty printed, we should be caching whether or not we think it
> is minified based on our heuristics. Does that make sense? The name of the
> method is correct, but the thing it does is incorrect. See below.

Ok..yes this makes sense, i thought about this at some point

> 
> Thanks for following through with this bug!

its being fun, loving it.
A patch coming up..
Attached patch wip.patch (obsolete) — Splinter Review
Attachment #8347302 - Attachment is obsolete: true
Attachment #8349045 - Flags: review?(nfitzgerald)
Comment on attachment 8349045 [details] [diff] [review]
wip.patch

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

This is getting very close!

Applied the patch locally and played with it a bit. It correctly detected and pretty printed minified sources! Yay!

Two things, though:

1. We *are* accidentally toggling the pretty printing off when a source is already pretty printed. Check the comments below and point 2.

2. I ran the tests and browser_dbg_auto-pretty-print-02.js is failing. At least we know that it catches the error it is checking for! Here is a log:

> 3:34.12 *** End BrowserChrome Test Results ***
> 3:34.49 INFO | runtests.py | Application ran for: 0:03:31.243481
> 3:34.49 INFO | zombiecheck | Reading PID log: /var/folders/14/l4lqpmvs5sncftp1c0ym243w0000gp/T/tmpBCq5h9pidlog
>Could not kill process, could not find pid: 64003, assuming it's already dead
> 3:34.50 WARNING | leakcheck | refcount logging is off, so leaks can't be detected!
> 3:34.50 runtests.py | Running tests: end.
> 3:34.50 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_auto-pretty-print-02.js | The auto-pretty-print pref should be on. - Got false, expected true
> 3:34.50 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_auto-pretty-print-02.js | The Auto pretty print menu item should be checked. - Got false, expected true
> 3:34.50 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_auto-pretty-print-02.js | Test timed out
> 3:34.50 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_auto-pretty-print-02.js | Found a tab after previous test timed out: http://example.com/browser/browser/devtools/debugger/test/doc_auto-pretty-print-02.html
> 3:34.50 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_auto-pretty-print-02.js | leaked until shutdown [nsGlobalWindow #44 data:text/html;charset=utf8,<!DOCTYPE%20html><html%20dir='ltr'>%20%20<head>%20%20%20%20<style>%20%20%20%20%20%20html,%20body%20{%20height:%20100%;%20}%20%20%20%20%20%20body%20{%20margin:%200;%20overflow:%20hidden;%20}%20%20%20%20%20%20.CodeMirror%20{%20width:%20100%;%20height:%20100%%20!important;%20line-height:%20normal!important}%20%20%20%20</style>%20%20%20%20<link%20rel='stylesheet'%20href='chrome://browser/skin/devtools/common.css'>%20%20%20%20<link%20rel='stylesheet'%20href='chrome://browser/content/devtools/codemirror/codemirror.css'>%20%20%20%20<link%20rel='stylesheet'%20href='chrome://browser/content/devtools/codemirror/dialog.css'>%20%20%20%20<link%20rel='stylesheet'%20href='chrome://browser/content/devtools/codemirror/mozilla.css'>%20%20</head>%20%20<body%20class='theme-body%20devtools-monospace'></body></html>]

The leak and "found tab" stuff is *probably* just because the test didn't pass, but double check that you are declaring all of your gGlobal variables before the test, and also setting them to null after the test.

::: browser/devtools/debugger/debugger-panes.js
@@ +780,4 @@
>      // The container is not empty and an actual item was selected.
>      DebuggerView.setEditorLocation(sourceItem.value);
>  
> +    if (Prefs.autoPrettyPrint) {

Don't want to accidentally toggle off pretty printing. Do:

  if (Prefs.autoPrettyPrint && !sourceClient.isPrettyPrinted) {

@@ +1101,5 @@
>  
>    /**
> +   * Determines if the source text is minified by using
> +   * the percentage indented of a subset of lines
> +   * @param string aText

Small nit: add a newline between the overview and the @param stuff.

@@ +1166,5 @@
> +    let val = this._minifyCache.get(sourceClient);
> +    if (typeof val === "undefined") {
> +      val = undefined;
> +    }
> +    return val;

Let's combine |getSourceState|, |isSourceMinified|, and |isMinified|. Callers shouldn't care whether the value they are getting back is cached or not, that should be an internal implementation detail of the function.

  isMinified: function (sourceClient, text) {
    if (this._isMinifiedCache.has(sourceClient)) {
      return this._isMinifiedCache.get(sourceClient);
    }

    let isMinified;

    // Run hueristics based on amount of indentation, etc,
    // and store result in isMinified.

    this._isMinifiedCache.set(sourceClient, isMinified);
    return isMinified;
  }

::: browser/devtools/debugger/debugger.xul
@@ +74,5 @@
>               oncommand="DebuggerView.WatchExpressions._onCmdAddExpression()"/>
>      <command id="removeAllWatchExpressionsCommand"
>               oncommand="DebuggerView.WatchExpressions._onCmdRemoveAllExpressions()"/>
> +    <command id="toggleAutoPrettyPrint"
> +              oncommand="DebuggerView.Options._toggleAutoPrettyPrint()"/>

Small nit: please align "id" and "oncommand".

::: browser/devtools/debugger/test/browser_dbg_auto-pretty-print-01.js
@@ +52,5 @@
> +}
> +
> +function testSecondSourceLabel(){
> +  ok(gSources.containsValue(EXAMPLE_URL + gSecondSourceLabel),
> +  	"Second source url is correct.");

Small nit: please align so that the parameters are on the same column, like this:

>  ok(gSources.containsValue(EXAMPLE_URL + gSecondSourceLabel),
>     "Second source url is correct.");

Here and throughout the rest of the test files.

::: browser/devtools/debugger/test/browser_dbg_auto-pretty-print-02.js
@@ +2,5 @@
> +  http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Test auto pretty printing is not accidentally toggled off
> + * for a source with a single indented line

I think this is a little bit better of a description of the test for people reading it in the future:

"Test that auto pretty printing doesn't accidentally toggle pretty printing off when we switch to a minified source that is already pretty printed."

::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
@@ +51,5 @@
>  <!ENTITY debuggerUI.sources.toggleBreakpoints "Enable/disable all breakpoints">
>  
>  <!-- LOCALIZATION NOTE (debuggerUI.pauseExceptions): This is the label for the
>    -  checkbox that toggles pausing on exceptions. -->
> +<!ENTITY debuggerUI.pauseExceptions    "Pause on exceptions">

Please undo this accidental change.
Attachment #8349045 - Flags: review?(nfitzgerald)
> Small nit: please align so that the parameters are on the same column, like this:

Ah, so you are mixing spaces and tabs, which is why the review had it mis-aligned but looks fine here. Please use only spaces!

|M-x untabify| if you use emacs ;)
Attached patch wip.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=fb92580680da
Attachment #8349045 - Attachment is obsolete: true
Attachment #8350014 - Flags: review?(nfitzgerald)
When this feature is complete, this one could be related: 
https://bugzilla.mozilla.org/show_bug.cgi?id=952127
Looks like your try push is orange:

> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_auto-pretty-print-02.js | The auto-pretty-print pref should be on. - Got false, expected true
> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_auto-pretty-print-02.js | The Auto pretty print menu item should be checked. - Got false, expected true 

Will glance at the latest iteration of the patch and give feedback, when you've fixed that up flag me for review again with a new try push and I'll do a full review.
Comment on attachment 8350014 [details] [diff] [review]
wip.patch

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

This looks good! Once you have a green try push, flag me for one last quick once over and we can land this!

::: browser/devtools/debugger/test/browser.ini
@@ +67,5 @@
>    sjs_random-javascript.sjs
>    testactors.js
>  
> +[browser_dbg_auto-pretty-print-01.js]
> +[browser_dbg_auto-pretty-print-02.js]

Woops, just noticed these tests should be below |browser_dbg_aaa_run_first_leaktest.js|

::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
@@ +51,5 @@
>  <!ENTITY debuggerUI.sources.toggleBreakpoints "Enable/disable all breakpoints">
>  
>  <!-- LOCALIZATION NOTE (debuggerUI.pauseExceptions): This is the label for the
>    -  checkbox that toggles pausing on exceptions. -->
> +<!ENTITY debuggerUI.pauseExceptions    "Pause on exceptions">

Nit: This line is still changed for no reason.
Attachment #8350014 - Flags: review?(nfitzgerald)
Comment on attachment 8350014 [details] [diff] [review]
wip.patch

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

::: browser/devtools/debugger/test/browser_dbg_auto-pretty-print-01.js
@@ +78,5 @@
> +  is(gPrefs.autoPrettyPrint, false,
> +    "The auto-pretty-print pref should be off.");
> +  isnot(gOptions._autoPrettyPrint.getAttribute("checked"), "true",
> +       "The Auto pretty print menu item should not be checked.");
> +}

You probably need to turn auto pretty printing back on at the end of the test, or else the pref will persist and -02 will fail because auto pretty printing is off.
(In reply to Nick Fitzgerald [:fitzgen] from comment #49)
> Comment on attachment 8350014 [details] [diff] [review]
> wip.patch
> 
> Review of attachment 8350014 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/test/browser_dbg_auto-pretty-print-01.js
> @@ +78,5 @@
> > +  is(gPrefs.autoPrettyPrint, false,
> > +    "The auto-pretty-print pref should be off.");
> > +  isnot(gOptions._autoPrettyPrint.getAttribute("checked"), "true",
> > +       "The Auto pretty print menu item should not be checked.");
> > +}
> 
> You probably need to turn auto pretty printing back on at the end of the
> test, or else the pref will persist and -02 will fail because auto pretty
> printing is off.

 oh. yeah. i missed this because i was running the tests seperately. 
thanks
an updated patch comming up.
(In reply to Hubert B Manilla from comment #50)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #49)
> > Comment on attachment 8350014 [details] [diff] [review]
> > wip.patch
> > 
> > Review of attachment 8350014 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/devtools/debugger/test/browser_dbg_auto-pretty-print-01.js
> > @@ +78,5 @@
> > > +  is(gPrefs.autoPrettyPrint, false,
> > > +    "The auto-pretty-print pref should be off.");
> > > +  isnot(gOptions._autoPrettyPrint.getAttribute("checked"), "true",
> > > +       "The Auto pretty print menu item should not be checked.");
> > > +}
> > 
> > You probably need to turn auto pretty printing back on at the end of the
> > test, or else the pref will persist and -02 will fail because auto pretty
> > printing is off.
> 
>  oh. yeah. i missed this because i was running the tests seperately. 
> thanks
> an updated patch comming up.

Its good to run all debugger mochitests as a last sanity check every time you upload a patch for review:

  ./mach mochitest-browser browser/devtools/debugger/test
The patch is ready, just watching my try push. i got a failed test but it looks like more of a metro browser thing not too sure what it is. could you have a glance.
https://tbpl.mozilla.org/?tree=Try&rev=81cc95808222
Attached patch wip.patchSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=81cc95808222
Attachment #8350014 - Attachment is obsolete: true
Attachment #8350703 - Flags: review?(nfitzgerald)
There are a couple of failed tests, which seem like bugs, but i'm not too sure about them.
The patch tests are all green.
(In reply to Hubert B Manilla from comment #54)
> There are a couple of failed tests, which seem like bugs, but i'm not too
> sure about them.
> The patch tests are all green.

When you click on the failing test(s) the following are true:

* the failing test is unrelated to any code you changed

* TPBL has a suggested intermittent failure bug that looks like it describes the failure you are seeing

* You don't have a failure on every run on each platform of the particular failing test suite (ie xpcshell/mochitest/etc)

You can be reasonably sure that it is an existing, known intermittant bug. Point 2 is the biggest hint, especially if the failure in the logs matches the description for the suggested bug. Of course, if you ever aren't sure, just ask :)

In this case, it looks like the failures you were seeing were all intermittant. Unfortunately, it is pretty rare to have a push that has zero intermittants :(
Comment on attachment 8350703 [details] [diff] [review]
wip.patch

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

LGTM! Thanks for sticking with this through all the revisions, Hubert!
Attachment #8350703 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen] from comment #55)
> (In reply to Hubert B Manilla from comment #54)
> > There are a couple of failed tests, which seem like bugs, but i'm not too
> > sure about them.
> > The patch tests are all green.
> 
> When you click on the failing test(s) the following are true:
> 
> * the failing test is unrelated to any code you changed
> 
> * TPBL has a suggested intermittent failure bug that looks like it describes
> the failure you are seeing
> 
> * You don't have a failure on every run on each platform of the particular
> failing test suite (ie xpcshell/mochitest/etc)
> 
> You can be reasonably sure that it is an existing, known intermittant bug.
> Point 2 is the biggest hint, especially if the failure in the logs matches
> the description for the suggested bug. Of course, if you ever aren't sure,
> just ask :)
> 
> In this case, it looks like the failures you were seeing were all
> intermittant. Unfortunately, it is pretty rare to have a push that has zero
> intermittants :(

Thanks for the insight
(In reply to Nick Fitzgerald [:fitzgen] from comment #56)
> Comment on attachment 8350703 [details] [diff] [review]
> wip.patch
> 
> Review of attachment 8350703 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM! Thanks for sticking with this through all the revisions, Hubert!
 
Cheers Nick for all your help!
Comment on attachment 8350703 [details] [diff] [review]
wip.patch

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

> try: -b do -p all -u all -t none

For future reference, it's appreciated to have a commit message in the patch that explain what the patch does. I'll add one myself this time.
https://hg.mozilla.org/mozilla-central/rev/2f9043292e63
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Hi, in bug 918017 comment 19 someone noticed that the patch for this bug used the wrong entity name for the accesskey. Can you please fix?
Ok thanks. Will fix it.
Attached patch Fix wrong entity (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=1ac40d6aa9de
Attachment #8358857 - Flags: review?(nfitzgerald)
Attached patch Fix wrong entitySplinter Review
Attachment #8358857 - Attachment is obsolete: true
Attachment #8358857 - Flags: review?(nfitzgerald)
Attachment #8358858 - Flags: review?(nfitzgerald)
Attachment #8358858 - Flags: review?(nfitzgerald) → review+
Attachment #8358858 - Flags: checkin+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.