2011-01-13 Ojan Vafai <ojan@chromium.org>
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Jan 2011 23:52:42 +0000 (23:52 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Jan 2011 23:52:42 +0000 (23:52 +0000)
        Reviewed by Adam Barth.

        add container divs for diff blocks
        https://bugs.webkit.org/show_bug.cgi?id=52400

        This will help simplify a lot of code in code-review.js
        and make side-by-side diffs better (i.e. put removed lines
        to the left of corresponding added lines).

        Also, allow for running the JS from a local file. Now you can modify code-review.js
        to point to a local file and then run:
        ruby prettify.rb < foo.diff > foo.html

        foo.html will load a dummy code review matching foo.diff.

        Before structure:
        Line
        Line remove
        Line add
        Line add
        Line

        After structure:
        DiffBlock
          DiffBlockPart shared
            Line shared
        DiffBlock
          DiffBlockPart remove
            Line remove
          DiffBlockPart add
            Line add
            Line add
        DiffBlock
          DiffBlockPart shared
            Line shared

        * PrettyPatch/PrettyPatch.rb:
        * code-review.js:

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

Websites/bugs.webkit.org/ChangeLog
Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb
Websites/bugs.webkit.org/code-review.js

index 33ed7db..310f782 100644 (file)
@@ -1,3 +1,44 @@
+2011-01-13  Ojan Vafai  <ojan@chromium.org>
+
+        Reviewed by Adam Barth.
+
+        add container divs for diff blocks
+        https://bugs.webkit.org/show_bug.cgi?id=52400
+
+        This will help simplify a lot of code in code-review.js
+        and make side-by-side diffs better (i.e. put removed lines
+        to the left of corresponding added lines).
+
+        Also, allow for running the JS from a local file. Now you can modify code-review.js
+        to point to a local file and then run:
+        ruby prettify.rb < foo.diff > foo.html
+
+        foo.html will load a dummy code review matching foo.diff.
+
+        Before structure:
+        Line
+        Line remove
+        Line add
+        Line add
+        Line
+
+        After structure:
+        DiffBlock
+          DiffBlockPart shared
+            Line shared
+        DiffBlock
+          DiffBlockPart remove
+            Line remove
+          DiffBlockPart add
+            Line add
+            Line add
+        DiffBlock
+          DiffBlockPart shared
+            Line shared
+
+        * PrettyPatch/PrettyPatch.rb:
+        * code-review.js:
+
 2011-01-12  Ojan Vafai  <ojan@chromium.org>
 
         Reviewed by Mihai Parparita.
index c73eb97..880b8a7 100644 (file)
@@ -204,20 +204,20 @@ h1 :hover {
     background-color: #fef;
 }
 
-.add {
+.Line.add {
     background-color: #dfd;
 }
 
-.add ins {
+.Line.add ins {
     background-color: #9e9;
     text-decoration: none;
 }
 
-.remove {
+.Line.remove {
     background-color: #fdd;
 }
 
-.remove del {
+.Line.remove del {
     background-color: #e99;
     text-decoration: none;
 }
@@ -395,7 +395,7 @@ body {
 }
 </style>
 <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js"></script> 
-<script src="code-review.js?version=21"></script>
+<script src="code-review.js?version=22"></script>
 EOF
 
     def self.revisionOrDescription(string)
@@ -568,6 +568,38 @@ END
         end
     end
 
+    class DiffBlock
+        attr_accessor :parts
+
+        def initialize(container)
+            @parts = []
+            container << self
+        end
+
+        def to_html
+            str = "<div class='DiffBlock'>\n"
+            str += @parts.collect{ |part| part.to_html }.join
+            str += "</div>\n"
+        end
+    end
+
+    class DiffBlockPart
+        attr_reader :className
+        attr :lines
+
+        def initialize(className, container)
+            @className = className
+            @lines = []
+            container.parts << self
+        end
+
+        def to_html
+            str = "<div class='DiffBlockPart %s'>\n" % @className
+            str += @lines.collect{ |line| line.to_html }.join
+            str += "</div>\n"
+        end
+    end
+
     class DiffSection
         def initialize(lines)
             lines.length >= 1 or raise "DiffSection.parse only received %d lines" % lines.length
@@ -575,36 +607,58 @@ END
             matches = START_OF_SECTION_FORMAT.match(lines[0])
             from, to = [matches[1].to_i, matches[2].to_i] unless matches.nil?
 
-            @lines = lines[1...lines.length].collect do |line|
+            @blocks = []
+            diff_block = nil
+            diff_block_part = nil
+
+            for line in lines[1...lines.length]
                 startOfLine = line =~ /^[-\+ ]/ ? 1 : 0
                 text = line[startOfLine...line.length].chomp
                 case line[0]
                 when ?-
-                    result = CodeLine.new(from, nil, text)
+                    if (diff_block_part.nil? or diff_block_part.className != 'remove')
+                        diff_block = DiffBlock.new(@blocks)
+                        diff_block_part = DiffBlockPart.new('remove', diff_block)
+                    end
+
+                    diff_block_part.lines << CodeLine.new(from, nil, text)
                     from += 1 unless from.nil?
-                    result
                 when ?+
-                    result = CodeLine.new(nil, to, text)
+                    if (diff_block_part.nil? or diff_block_part.className != 'add')
+                        # Put add lines that immediately follow remove lines into the same DiffBlock.
+                        if (diff_block.nil? or diff_block_part.className != 'remove')
+                            diff_block = DiffBlock.new(@blocks)
+                        end
+
+                        diff_block_part = DiffBlockPart.new('add', diff_block)
+                    end
+
+                    diff_block_part.lines << CodeLine.new(nil, to, text)
                     to += 1 unless to.nil?
-                    result
                 else
-                    result = CodeLine.new(from, to, text)
+                    if (diff_block_part.nil? or diff_block_part.className != 'shared')
+                        diff_block = DiffBlock.new(@blocks)
+                        diff_block_part = DiffBlockPart.new('shared', diff_block)
+                    end
+
+                    diff_block_part.lines << CodeLine.new(from, to, text)
                     from += 1 unless from.nil?
                     to += 1 unless to.nil?
-                    result
                 end
             end
 
-            @lines.unshift(ContextLine.new(matches[3])) unless matches.nil? || matches[3].empty?
-
             changes = [ [ [], [] ] ]
-            for line in @lines
-                if (!line.fromLineNumber.nil? and !line.toLineNumber.nil?) then
-                    changes << [ [], [] ]
-                    next
+            for block in @blocks
+                for block_part in block.parts
+                    for line in block_part.lines
+                        if (!line.fromLineNumber.nil? and !line.toLineNumber.nil?) then
+                            changes << [ [], [] ]
+                            next
+                        end
+                        changes.last.first << line if line.toLineNumber.nil?
+                        changes.last.last << line if line.fromLineNumber.nil?
+                    end
                 end
-                changes.last.first << line if line.toLineNumber.nil?
-                changes.last.last << line if line.fromLineNumber.nil?
             end
 
             for change in changes
@@ -630,11 +684,13 @@ END
                     change.last[i].operations = operations
                 end
             end
+
+            @blocks.unshift(ContextLine.new(matches[3])) unless matches.nil? || matches[3].empty?
         end
 
         def to_html
             str = "<div class='DiffSection'>\n"
-            str += @lines.collect{ |line| line.to_html }.join
+            str += @blocks.collect{ |block| block.to_html }.join
             str += "</div>\n"
         end
         
@@ -666,7 +722,7 @@ END
         end
 
         def classes
-            lineClasses = ["Line"]
+            lineClasses = ["Line", "LineContainer"]
             lineClasses << ["add"] unless @toLineNumber.nil? or !@fromLineNumber.nil?
             lineClasses << ["remove"] unless @fromLineNumber.nil? or !@toLineNumber.nil?
             lineClasses
index 81ef64e..f2cf8a9 100644 (file)
   // Attempt to activate only in the "Review Patch" context.
   if (window.top != window)
     return;
-  if (!window.location.search.match(/action=review/))
+
+  if (!window.location.search.match(/action=review/)
+      && !window.location.toString().match(/bugs\.webkit\.org\/PrettyPatch/))
     return;
+
   var attachment_id = determineAttachmentID();
   if (!attachment_id)
-    return;
+    console.log('No attachment ID');
 
   var next_line_id = 0;
   var files = {};
     this.id = nextLineID();
   }
 
-  function containerify() {
-    $(this).addClass('LineContainer');
-  }
-
   function hoverify() {
     $(this).hover(function() {
       $(this).addClass('hot');
   }
 
   function crawlDiff() {
-    $('.Line').each(idify).each(hoverify).each(containerify);
+    $('.Line').each(idify).each(hoverify);
     $('.FileDiff').each(function() {
       var file_name = $(this).children('h1').text();
       files[file_name] = this;
   }
 
   $('.lineNumber').live('click', function() {
-    var line = $(this).parent();
+    var line = $(this).parents('.Line');
     if (line.hasClass('commentContext'))
       trimCommentContextToBefore(previousLineFor(line));
   }).live('mousedown', function() {