352615 - [PatchReader] Group empty cells together to reduce the size of the diff page

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect, P3)

Description

[Adam Guthrie]

Apparently a <table> is created and <tr>s are used for each /line/ of a patch. This seems wrong to me... there's got to be a way we can do this that isn't reminiscent of the '90s.

Comment 1

[Frédéric Buclin]

The main problem is the size of the generated page:

Viewing process_bug.cgi and Bug.pm with context=file generates a page which is 1Mb in size despite both files are much smaller (48 Kb and 113 Kb respectively).

We should find a way to decrease the size of the page by at least a factor of 2.

Comment 2

[Guy Pyrzak]

LpSolit is referring to https://bugzilla.mozilla.org/attachment.cgi?id=297468&action=diff&headers=1&context=file

I think the way to fix this is to do some smart stuff, ie if there is a + without a - then you could do a rowspan. Otherwise this would be kinda rough.

Comment 3

[Frédéric Buclin]

Attachment: proof of concept using <br>, v1

In this patch, I group cells which are of the same kind (context, changed, removed, or added) using <br> for each line within each block. This makes pages to be 40-60% smaller (which is a huge improvement for the bandwidth).

The display is fine except that row numbers are no longer aligned with the patch content itself, especially when some lines are wrapped. That's because a newline in a cell cannot adapt itself based on what happens in another cell besides it. So I doubt we can make things better compared to what we have currently, unfortunately (unless there is some magical CSS rule which can fix this for us, but I'm pessimistic).

Comment 4

[Frédéric Buclin]

Attachment: patch, v1

I only use blocks where there is no line numbering so that the UI is not affected at all. Compared to my previous patch, the page size is only reduced by ~6-10% on average, with a higher gain when the patch has a lot of new/removed lines.

Another possible improvement would be to remove <pre> and add some CSS rules to existing classes, but this doesn't work well (Firefox, Opera, Safari, Internet Explorer and Google Chrome behave differently), so I leave that for another bug. Also, putting class="num" in <col> instead of repeating it for each row gives weird results. I'm not sure modern browsers support <col> and <colgroup> very well.

So I will take this patch for now.

Comment 5

[Frédéric Buclin]

Morphing the bug summary to better reflect what I did in my patch.

Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.128; previous revision: 1.127
done
Checking in template/en/default/attachment/diff-file.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/diff-file.html.tmpl,v  <--  diff-file.html.tmpl
new revision: 1.8; previous revision: 1.7
done