Web Inspector: Fix multiple console.assert stripping issues
authorjoepeck@webkit.org <joepeck@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Mar 2014 00:26:17 +0000 (00:26 +0000)
committerjoepeck@webkit.org <joepeck@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Mar 2014 00:26:17 +0000 (00:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=130166

Reviewed by Timothy Hatcher.

There were a couple console.assert stripping issues in production.
One line required a semicolon so was avoiding getting stripped.
One resulted in a logic change, when stripping the only statement
of a control flow block. Add a warning for such cases.

* Scripts/remove-console-asserts.pl:
Add warning for a console.assert being the only statement in a control flow block
without braces. When it is stripped it may change the flow of the function.

* Scripts/remove-console-asserts-dryrun.rb: Added.
Add a script to quickly test running remove console asserts on our files, to
help catch errors not in a production build and in the original non-combined
files, so you can more easily fix issues.

* UserInterface/Controllers/DOMTreeManager.js:
(WebInspector.DOMTreeManager.prototype._updateContentFlowFromPayload):
Convert the for loop into a single console.assert statement.

* UserInterface/Views/DataGrid.js:
(WebInspector.DataGrid.prototype.removeChild):
Add missing semicolon.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@165516 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/Scripts/remove-console-asserts-dryrun.rb [new file with mode: 0755]
Source/WebInspectorUI/Scripts/remove-console-asserts.pl
Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js
Source/WebInspectorUI/UserInterface/Views/DataGrid.js

index 45e7a8a..d934ad1 100644 (file)
@@ -1,3 +1,32 @@
+2014-03-12  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Web Inspector: Fix multiple console.assert stripping issues
+        https://bugs.webkit.org/show_bug.cgi?id=130166
+
+        Reviewed by Timothy Hatcher.
+
+        There were a couple console.assert stripping issues in production.
+        One line required a semicolon so was avoiding getting stripped.
+        One resulted in a logic change, when stripping the only statement
+        of a control flow block. Add a warning for such cases.
+
+        * Scripts/remove-console-asserts.pl:
+        Add warning for a console.assert being the only statement in a control flow block
+        without braces. When it is stripped it may change the flow of the function.
+
+        * Scripts/remove-console-asserts-dryrun.rb: Added.
+        Add a script to quickly test running remove console asserts on our files, to
+        help catch errors not in a production build and in the original non-combined
+        files, so you can more easily fix issues.
+
+        * UserInterface/Controllers/DOMTreeManager.js:
+        (WebInspector.DOMTreeManager.prototype._updateContentFlowFromPayload):
+        Convert the for loop into a single console.assert statement.
+
+        * UserInterface/Views/DataGrid.js:
+        (WebInspector.DataGrid.prototype.removeChild):
+        Add missing semicolon.        
+
 2014-03-12  Brian Burg  <bburg@apple.com>
 
         Web Inspector: convert model tests and inspector-test.js to use Test.html
diff --git a/Source/WebInspectorUI/Scripts/remove-console-asserts-dryrun.rb b/Source/WebInspectorUI/Scripts/remove-console-asserts-dryrun.rb
new file mode 100755 (executable)
index 0000000..3ecff58
--- /dev/null
@@ -0,0 +1,23 @@
+#!/usr/bin/ruby
+
+require "find"
+
+$verbose = ARGV.include?("--verbose");
+$remove_console_asserts_path = File.expand_path File.join(File.dirname(__FILE__), "remove-console-asserts.pl")
+$web_inspector_user_interface_path = File.expand_path File.join(File.dirname(__FILE__), "..", "UserInterface")
+
+Find.find($web_inspector_user_interface_path) do |path|
+  # Skip directories, External, Images, and non-js.
+  next if File.directory?(path)
+  next if path =~ /\/(External|Images)\//
+  next if path !~ /\.js$/
+
+  # Run remove-console-asserts on each file.
+  puts "Checking: #{path} ..." if $verbose
+  output = %x{ perl '#{$remove_console_asserts_path}' --input-script '#{path}' --output-script /dev/null }
+  if !output.empty?
+    puts "#{File.basename(path)}:"
+    puts output
+    puts
+  end
+end
index 8c5430f..b65bac1 100755 (executable)
@@ -19,10 +19,27 @@ unless (defined $inputScriptFilename and defined $outputScriptFilename) {
 open IN, $inputScriptFilename or die;
 our ($out, $tempFilename) = tempfile(UNLINK => 0) or die;
 
+our $previousLine = "";
 while (<IN>) {
+    # Warn about console.assert in control flow statement without braces. Can change logic when stripped.
+    if (/console\.assert/) {
+        if ($previousLine =~ /^\s*(for|if|else|while|do)\b/ && $previousLine !~ /\{\s*$/) {
+            print "WARNING: console.assert inside control flow statement without braces on line: $.: $_";
+        }
+    }
+
     s/\s*console\.assert\(.*\);\s*//g;
     print $out $_;
-    print "WARNING: Multi-line console.assert on line $.: $_" if $_ =~ /\s*console\.assert\(/;
+    $previousLine = $_ if $_ !~ /^\s*$/;
+
+    # If console.assert is still on the line, either we missed a semicolon or it is multi-line. These did not get stripped.
+    if ($_ =~ /\s*console\.assert\(/) {
+        if ($_ =~ /\)\s*$/) {
+            print "WARNING: console.assert missing trailing semicolon on line $.: $_" ;
+        } else {
+            print "WARNING: Multi-line console.assert on line $.: $_" ;
+        }
+    }
 }
 
 close $out;
index 3f3e06d..a1e06b5 100644 (file)
@@ -552,8 +552,7 @@ WebInspector.DOMTreeManager.prototype = {
     _updateContentFlowFromPayload: function(contentFlow, flowPayload)
     {
         console.assert(contentFlow.contentNodes.length === flowPayload.content.length);
-        for (var i = 0; i < contentFlow.contentNodes.length; ++i)
-            console.assert(contentFlow.contentNodes[i].id === flowPayload.content[i]);
+        console.assert(contentFlow.contentNodes.every(function(node, i) { node.id === flowPayload.content[i]; }));
 
         // FIXME: Collect the regions from the payload.
         contentFlow.overset = flowPayload.overset;
index c1c0e70..7106206 100644 (file)
@@ -772,7 +772,7 @@ WebInspector.DataGrid.prototype = {
         if (this.children.length <= 0)
             this.hasChildren = false;
 
-        console.assert(!child.isPlaceholderNode, "Shouldn't delete the placeholder node.")
+        console.assert(!child.isPlaceholderNode, "Shouldn't delete the placeholder node.");
     },
 
     removeChildren: function()