Closed Bug 103778 Opened 22 years ago Closed 21 years ago

templatize buglist.cgi

Categories

(Bugzilla :: Query/Bug List, defect, P1)

2.15
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: myk, Assigned: myk)

References

Details

Attachments

(2 files, 27 obsolete files)

105.31 KB, patch
bbaetz
: review+
gerv
: review+
Details | Diff | Splinter Review
7.41 KB, patch
Details | Diff | Splinter Review
buglist.cgi should be templatized.
Blocks: bz-template
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.16
Attached patch work in progress (obsolete) — Splinter Review
Blocks: 104025
Does this do incremental output?  I think we should use template fragments to
output a line at a time.
Attached patch work in progress v2 (obsolete) — Splinter Review
Blocks: 82561
The patch is ready for review.  It's big.  I made very few changes to the query
section, so reviewers can review those in the patch, but the rest of it should
be reviewed in the patched CGI itself, since almost everything else is rewritten.
Keywords: patch, review
A test installation for these changes is located at the following URL:

http://landfill.tequilarista.org/bztemplate/

Here's a few comments to be going on with.

tweakform:

Would it not be possible to factor out a lot of the selection widgets into a
block with three params - "Platform" (display string), "rep_platform" (name of
<select>) and "platform" (list name) ?

You are using wrap=hard. What's the current status on server-side wrapping?

html.atml:

>   [% IF keywords.size %]
...
> [% IF groups.size > 0 %]

Make up your mind :-)

> .secure class 

We need to make this styling conditional on not having bug groups turned on, as
we discussed.

buglist.cgi:

You have indentation issues with your Stash and my $vars = ... sections. 

my $serverpush = should probably be:
my $serverpush = exists $ENV{'HTTP_USER_AGENT'} && 
                 $ENV{'HTTP_USER_AGENT'} =~ /Mozilla.[3-9]/ && 
                 $ENV{'HTTP_USER_AGENT'} !~ /[Cc]ompatible/ && 
                 $outputformat eq 'html';
or something like that.                                  

> if (grep(/^cmd-/, keys(%::FORM))) {

Surely it's better and quicker to do:
if (grep(/&cmd-/, $::buffer) {
? And is it a problem that there's still HTML here embedded in the .cgi?

> # !!! Figure out if there is any reason to believe $supptablesref
> # and $wherepartref will ever be used and remove them if not!

Remove them. I did. I added a parameter which allowed you to specify whether you
wanted SQL giving you the COUNT(*) or the actual bugs. But maybe that can wait
for my checkin.

> sub Error {
>     my ($str) = @_;
>     DisplayError($str);
>     exit;
> }

And the point of this is...? It's particularly confusing in combination with:
> return Error("foo");
in some functions.

> # 3. Shortened Name: (optional) an SQL expression
> "substring(bugs.bug_severity,1,3)" 

If it's SQL you should capitalise SUBSTRING. Otherwise it looks like Perl or JS
or something. It confused me, at any rate.

Are you sure that only status and short_desc should be HTML-escaped? Are there
not other things that should be? Do we need a definitive list of which database
fields allow <, > and &, so we can reject on input for those that don't and
filter on output for those that do?

>    print "Content-Disposition: inline; filename=bugzilla_bug_list.html\n";

Can we not think of something more useful? Particularly if this is a named query.

Items for the newsgroups:
We need to decide on an output format template naming scheme.
We need to decide how much you indent run-on lines.
We should settle on a comment formatting style as well.

More to come later, maybe.

Gerv
Mass change problems:

- I can't change the Component or Version - the dropdowns are empty apart from
the Don't Change string.
- I'm getting Target Milestone-related multichange stuff but I don't think
landfill has that turned on.
- The default value for the Resolution field is empty, rather than
--do_not_change--, which it should be. Using this, I managed to resolve a bug to
"" resolution. This is also bad, because it probably implies people can corrupt
the DB by hacking the HTML. (bug 299 on landfill)

Gerv
> Would it not be possible to factor out a lot of the selection widgets into a
> block with three params - "Platform" (display string), "rep_platform" (name of
> <select>) and "platform" (list name) ?

Done, except that I didn't include the label name ("Platform") since the label
structure varies.


> You are using wrap=hard. What's the current status on server-side wrapping?

Wrapping is handled by process_bug.cgi, which I am not attacking in this patch,
but see bug 11901.


> html.atml:

> > [% IF keywords.size %]
> > ...
> > [% IF groups.size > 0 %]

> Make up your mind :-)

Made up.  I'm going with the second approach because it is more obvious.


> > .secure class

> We need to make this styling conditional on not having bug groups turned on, as
> we discussed.

Done.  For those not in the know, the idea is that it makes sense to style
secure bugs differently if you are not using bug (product) groups because there
are likely to be only a few secure bugs, but it doesn't make sense to style them
differently on an installation using bug (product) groups because on those
installations it is likely that all bugs are secure (i.e. members of their
product's group).  Installations can, of course, override this policy by hacking
the template.


> buglist.cgi:

> You have indentation issues with your Stash and my $vars = ... sections.

They look good to me.


> my $serverpush = should probably be:
> my $serverpush = exists $ENV{'HTTP_USER_AGENT'} &&
>                  $ENV{'HTTP_USER_AGENT'} =~ /Mozilla.[3-9]/ &&
>                  $ENV{'HTTP_USER_AGENT'} !~ /[Cc]ompatible/ &&
>                  $outputformat eq 'html';
> or something like that.

Why?  and/or operators at the ends of lines is discouraged per the Perl style
guide, and a progression of indentation is intuitive and easy to read.


> > if (grep(/^cmd-/, keys(%::FORM))) {

> Surely it's better and quicker to do:
> if (grep(/&cmd-/, $::buffer) {

Do you mean "if ($::buffer =~ /&cmd-/)"?  If so, done.


> ? And is it a problem that there's still HTML here embedded in the .cgi?

Not really, it's a minor issue, but I broke it all out into separate templates
(global/ forward.atml and message.atml).


> > # !!! Figure out if there is any reason to believe $supptablesref
> > # and $wherepartref will ever be used and remove them if not!

> Remove them. I did.

Done.


> I added a parameter which allowed you to specify whether you
> wanted SQL giving you the COUNT(*) or the actual bugs. But maybe that can wait
> for my checkin.

Yep.


> > sub Error {
> > my ($str) = @_;
> > DisplayError($str);
> > exit;
> > }

> And the point of this is...?
> It's particularly confusing in combination with:
> > return Error("foo");
> in some functions.

The point of this is not to go hacking up the query stuff making it harder for
you to review, but if you don't mind, then done.  I like clean code too. :-)


> > # 3. Shortened Name: (optional) an SQL expression
> > "substring(bugs.bug_severity,1,3)"

> If it's SQL you should capitalise SUBSTRING. Otherwise it looks like Perl or JS
> or something. It confused me, at any rate.

Done.


> Are you sure that only status and short_desc should be HTML-escaped? Are there
> not other things that should be? Do we need a definitive list of which database
> fields allow <, > and &, so we can reject on input for those that don't and
> filter on output for those that do?

My list is the same as the current list, so it's probably right (unless you know
of bugs on this).  However, here's a simpler solution: filter everything.  It's
relatively cheap, especially since we have to execute a conditional statement on
every value otherwise (and then still filter some of them).  Done.


> > print "Content-Disposition: inline; filename=bugzilla_bug_list.html\n";

> Can we not think of something more useful? Particularly if this is a named
> query.

Ironically it doesn't even work with server push.  This is a separate
enhancement request though.


> Items for the newsgroups:
> We need to decide on an output format template naming scheme.

Message posted to n.p.m.webtools.


> We need to decide how much you indent run-on lines.

The problem with this is that there are numerous types of run-on lines, so it is
difficult to specify a single style that accommodates them all.  Even the Perl
style guide has very little to say on the subject beyond "Break long lines after
an operator (but before 'and' and 'or', even when spelled '&&' and '||'." and
"Line up corresponding items vertically."  Following those guidelines leaves a
lot of room for interpretation, f.e. (taken from my own code, and keep in mind
that "foo", "bar", etc. represent long strings that necessitate breaking up the
lines):

Hashes:

my $foo =
  {
    'foo' => $foo ,
    'bar' => $bar ,
    'baz' =>
      {
          'small' => 3 ,
         'bigger' => 7 ,
        'biggest' => 42 ,
      } ,
    'biz' => $biz ,
  };

Multiple Conditionals:

do blah
  if $foo
    || $bar
      && $baz;

Question Mark Conditionals:

$foo ? $bar
     : $baz;

Strings:

DisplayError("You can't do that!  It can't be done!
  it isn't legal!  I can't believe you would even
  think about it!");

  -or-

DisplayError("You can't do that!  It can't be done!
              it isn't legal!  I can't believe you
              would even think about it!");


> We should settle on a comment formatting style as well.

Is this really necessary?

> - I can't change the Component or Version - the dropdowns are empty apart from
> the Don't Change string.
> - I'm getting Target Milestone-related multichange stuff but I don't think
> landfill has that turned on.

Hmm, it works on my local workstation.  I have to look into this more.

> - The default value for the Resolution field is empty, rather than
> --do_not_change--, which it should be. Using this, I managed to resolve a bug to
> "" resolution. This is also bad, because it probably implies people can corrupt
> the DB by hacking the HTML. (bug 299 on landfill)

Looking into this.
In addition to all the other stuff I fixed, I also placed template stuff into
globals.pl so it can be available to all scripts.
> - I can't change the Component or Version - the dropdowns are empty apart
> from the Don't Change string.

Fixed.

> - I'm getting Target Milestone-related multichange stuff but I don't think
> landfill has that turned on.

Fixed.

> - The default value for the Resolution field is empty, rather than
> --do_not_change--, which it should be.

Done.  New patch ready for review.
Attachment #56234 - Flags: review-
Comment on attachment 56234 [details] [diff] [review]
patch v5: more fixes; apply with -p0 option

Haven't gone over it 100%, but I've looked at most of the buglist.cgi changes and
noticed the following:

>Index: buglist.cgi
...
>+if ($::FORM{'regetlastlist'} && !$::COOKIE{'BUGLIST'}) {
>+    DisplayError(qq|Sorry, I seem to have lost the cookie that recorded
>+                    the results of your last query.  You will have to start
>+                    over at the <a href="query.cgi">query page</a>.|);
>+    exit;
>+}
...
>+# If the user is retrieving the last bug list they looked at, hack the buffer
>+# storing the query string so that it looks like a query retrieving those bugs.
>+if ($::FORM{'regetlastlist'}) {
>+    $::FORM{'bug_id'} = join(',', split(/:/, $::COOKIE{'BUGLIST'}));
>+    $order = 'reuse last sort' unless $order;
>+    $::buffer = "bug_id=$::FORM{'bug_id'}&order=" . url_quote($order);
>+}

It seems to me that we could combine both of these if $::FORM{'regetlastlist'}
statements to one place.


>+# This can happen if there's an old bookmark to a query, but it does not
>+# really matter since 'doit' is the default command and happens anyway
>+# if no command is defined.
>+my $command = $::FORM{'cmdtype'} || 'doit';

Then why include it if it doesn't matter?

>+    UserInGroup("editbugs")
>+      || DisplayError("Sorry, you do not have sufficient privileges to edit
>+                       a bunch of bugs at once.")
>+        && exit;

I've never really liked this style of && and || 's... to me an if statment would
be so much more readable.

>+        my $htmlstr = html_quote($str);
>+        DisplayError("The string <tt>$str</tt> is not a legal date.");

You printed the wrong variable, should be |<tt>$htmlstr</tt>|

>     return time2str("%Y/%m/%d %H:%M:%S", $date);

I don't know if it really matters or not, but shouldn't that be %Y-%m-%d?

>+sub DiffDate {
>+    my ($date) = @_;
>+    my $age = time() - $date;
>+    my ($s,$m,$h,$d,$mo,$y,$wd)= localtime $date;
>+    if( $age < 18*60*60 ) {
>+        $date = sprintf "%02d:%02d:%02d", $h,$m,$s;
>+	  } elsif( $age < 6*24*60*60 ) {
>+        $date = sprintf "%s %02d:%02d", $weekday[$wd],$h,$m;
>+    } else {
>+        $date = sprintf "%04d-%02d-%02d", 1900+$y,$mo+1,$d;
>+    }
>+    return $date;
>+}

This sub has some style issues.  (It's also the reason for my above comment about
the date format).

>     ParseUrlString($urlstr, \%F, \%M);

I've never really understood why buglist seems to keep its own copy of %::FORM and
%::MFORM in %F and %M.
Myk I think it would be better if the default format stuff was done by having a
default format atml which INCLUDEs a html format atml.  That way the
administrator can change the default format, and the code is simpler.

I'd also like to be convinced that templatising this doesn't delay the initial
receival of data by the user.  Specifically, can DBI/MySQL return partial data
when it isn't all available, so processing can begin?  If so, templatisation
would delay processing of the output until the query completed.
Why are all the style rules for teh buglist !important?

Also, should we consider using classnames like bugzilla_secure, so that people
can style them manually via userChrome.css? I know that someone is going to
dislike the coloring
Comment on attachment 56666 [details] [diff] [review]
patch v6: resolves known issues (apply with -p0 option to patch)

Jake said:

>It seems to me that we could combine both of these if $::FORM{'regetlastlist'}
>statements to one place.

Done.

>>+# This can happen if there's an old bookmark to a query, but it does not
>>+# really matter since 'doit' is the default command and happens anyway
>>+# if no command is defined.
>>+my $command = $::FORM{'cmdtype'} || 'doit';
>
>Then why include it if it doesn't matter?

Good point.  Removed.

>>+    UserInGroup("editbugs")
>>+      || DisplayError("Sorry, you do not have sufficient privileges to edit
>>+                       a bunch of bugs at once.")
>>+        && exit;
>
>I've never really liked this style of && and || 's... to me an if statment would
>be so much more readable.

I disagree in general, but this one is kind of ugly.  Fixed.

>>+        my $htmlstr = html_quote($str);
>>+        DisplayError("The string <tt>$str</tt> is not a legal date.");
>
>You printed the wrong variable, should be |<tt>$htmlstr</tt>|

Fixed.

>> return time2str("%Y/%m/%d %H:%M:%S", $date);
>
>I don't know if it really matters or not, but shouldn't that be %Y-%m-%d?

It seems to work fine the current way, but the MySQL documentation says dashes
are the standard format, so fixed.

http://www.mysql.com/doc/D/A/DATETIME.html

>>+sub DiffDate {

>This sub has some style issues.  (It's also the reason for my above comment about
>the date format).

Yup.  This would be a good thing to clean up in a separate bug
(I think there may even be a bug for it already).

>> ParseUrlString($urlstr, \%F, \%M);
>
>I've never really understood why buglist seems to keep its own copy of %::FORM and
>%::MFORM in %F and %M.

I don't know either, but I don't want to hack the query generation portion
of the script too much.  This patch is about templatization of the output;
let's save work on the query stuff for a future patch.


Matty said:

>Myk I think it would be better if the default format stuff was done by having a
>default format atml which INCLUDEs a html format atml.  That way the
>administrator can change the default format, and the code is simpler.

In some cases this would work, but in the case of buglist.cgi the script itself
needs to know the format being requested (f.e. when deciding whether or not
to use server push, abbreviate query results, and return all fields or only
those appearing in the user's COLUMNLIST cookie).  Without hard-coding
the default format in the script, the script would have no way of knowing
what format is being requested and whether or not it should do anything special.

>I'd also like to be convinced that templatising this doesn't delay the initial
>receival of data by the user.  Specifically, can DBI/MySQL return partial data
>when it isn't all available, so processing can begin?  If so, templatisation
>would delay processing of the output until the query completed.

DBI/MySQL can only return data once the query has been processed, and since
query processing tends to make up the majority of time spent in the script,
templatization will not affect the user's initial reception of data.


Bradley said:

>Why are all the style rules for teh buglist !important?

Not sure.  I copied those from the old version.  The styles seem to work without
them, so I have removed them.

>Also, should we consider using classnames like bugzilla_secure, so that people
>can style them manually via userChrome.css? I know that someone is going to
>dislike the coloring

Done.
Blocks: 58731
No longer blocks: 58731
Myk, I know this is almost wrapped up, but one thing that would be very nice
would be making buglist.cgi colorization play nice with CSS - some people really
would like to customize or turn off colouring, and right now it's impossibly. My
suggestion:

a) Add this section to <HEAD>

<STYLE TYPE="text/css">
<!--
.Bugzilla_buglist_severity_enhancement { font-style: italic; }
.Bugzilla_buglist_severity_blocker { color: red; font-weight: bold; }
.Bugzilla_buglist_severity_critical { color: red; }
-->
</STYLE>

b) Change the $customstyle attributions to:

        if ($severity) {
            if ($severity eq "enh") {
                $customstyle = "Bugzilla_buglist_severity_enhancement";
            }
            if ($severity eq "blo") {
                $customstyle = "Bugzilla_buglist_severity_blocker";
            }
            if ($severity eq "cri") {
                $customstyle = "Bugzilla_buglist_severity_critical";
            }
       pnl "<TR VALIGN=TOP ALIGN=LEFT CLASS='$customstyle'><TD>";

You might want to change the class names; me and Christopher thought them up on
IRC. But the general principle should be similar.

Is this okay for this round?
Discussing with mpt, and as per his suggestions for bug 36013, AND caillon, AND
sicking, I suggest the class names be:

.bmo_severity_critical
.bmo_severity_blocker
.bmo_severity_enhancement

This makes things more readable, and allows people to customize different
bugzilla installations:

<caillon> kiko: yes.  if RedHat uses red backgrounds for their buglists, for
example, then setting my userContent.css to do tr.severity-critical {color: red
} would suck

I understand this is a stopgap for a larger problem, but we are getting
complaints towards the impossibility of disabling colours, and this is my
suggestion for a fix that will avoid changing classnames again in the future.
> .bmo_severity_critical
> .bmo_severity_blocker
> .bmo_severity_enhancement

That means we'd either have to have a param for or try to use code to come up
with a short name for the Bugzilla installation.  Although such a param/function
would also be useful for bug 37339 (so you don't end up w/multiple tabs simply
labeled "Bugzilla" if you are interested in more than one installation of
Bugzilla enough to add it to your sidebar).
Surely using an install name implies someone would want to style different
installations differently. I think this need is sufficiently rare not to bother
with it.

Gerv
I agree with Gerv, and I also don't see any cause for concern about overlap in
field values used as class names, so the "severity_" part of "severity_value"
isn't needed.
Blocks: 109064
Blocks: 105724
Blocks: 107379
Attached patch patch v7: ready to go (-p0) (obsolete) — Splinter Review
This version:

1. Uses a better naming convention for template files along with corresponding
code in globals.pl to make supporting multiple output formats easy.  See my
post today in n.p.m.webtools
(news://news.mozilla.org:119/3BF576BD.5030206@mozilla.org) for more info on the
naming convention used.

2. Standardizes class names so they all start with "bz_".

3. Moves the HTML format-specific code that abbreviates column titles and field
values into the HTML format template.

4. Adds a "serverpush" flag for disabling server push.

5. Right-aligns the "ID" column in the bug list (provided the browser supports
the HTML 4.01 "col" element).

6. Implements a simple format that displays only the HTML bug list (no header,
footer, quip, etc.).

7. Changes from 200 to 100 bugs per discrete table and fixes the problem where
the first table would have one less bug.

Ready for re-review.
Attachment #52657 - Attachment is obsolete: true
Attachment #54000 - Attachment is obsolete: true
Attachment #54245 - Attachment is obsolete: true
Attachment #55169 - Attachment is obsolete: true
Attachment #55174 - Attachment is obsolete: true
Attachment #56037 - Attachment is obsolete: true
Attachment #56040 - Attachment is obsolete: true
Attachment #56234 - Attachment is obsolete: true
Attachment #56666 - Attachment is obsolete: true
We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
blocks a blocker so it's a blocker
Severity: normal → blocker
Priority: -- → P1
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
Blocks: 110770
Blocks: 100094
This version integrates the fix for bug 12284 and applies cleanly against the
tip.  A test installation is available:

http://landfill.tequilarista.org/bztemplate/
Attachment #58186 - Attachment is obsolete: true
Blocks: 48707
Grr. I have a partial review for this, and I _still_ haven't posted it. I suck. 

However, I seem to remember there's some serious stuff - a misspelled
SQLQuote(), and it doesn't work in some common tests - so even before I produce
it you might want to look at this patch again.

Gerv
What simple tests does it fail?
I'm going to have to redo my review to get better data (all my own fault.) I'll
try and get to that this week.

New job, moving house, Christmas presents, Christmas cards... <sigh>

Gerv
Terribly sorry for the delay...

- The patch has rotted somewhat. New patch coming up. (You might want to check
it's OK, though.)
- Are we not using .tmpl rather than .atml?
- Also, I thought we were using file extensions on everything
- I'm surprised by "html.atml". I thought we were going for e.g.
buglist.html.tmpl. Also, for forward compatibility, should we be renaming all
the current ones to conform to the agreed convention? Renaming them later will
just cause checksetup.pl bloat, and mean we may have to mess with people's
custom templates, which would be bad.
- license headers required on all files
- Were we going to make the secure styling dependent on buggroups?
- </form> should be in html.atml - I think we should match tags in template
files:
[% IF dotweark %]
  [% PROCESS buglist/tweakform %]
  </form>
[% END %]

- Use <label> on the dotweak checkboxes?

- Capitalised and unformatted HTML in tweakform. 
- You should be using a BLOCK for each of the nearly-identical select widgets -
see the sort of thing I do in query.atml and (better) the enter_bug.tmpl patch.
If you make the passed-in list variable names the same as the form names (except
for "component" :-( ) you are laughing.

- keywords.size is a bad test for keywords - we can be using keywords even if
there are none on the bug. I use a boolean, "use_keywords" .
    if (@::legal_keywords) {
        $vars->{'use_keywords'} = 1;
Same may well apply for groups.

- We should be sharing the knob code between show_bug.cgi and buglist.cgi. If
you factor it out in its current form, I'll generalise it ('bug' or 'bugs',
etc.). 
- "" appears in the resolution list 

- "To make changes to a bunch of bugs at once:" -> "Multiple bugs", please :-)

- Your "contains" method addition is extremely cool; I will adopt that :-)

- How did we decide to lay out multi-line boolean statements? I can't remember,
but I don't think it was like you are doing it. I could be wrong, though, and I
don't have the coding standards to hand.

- sub Error {
    my ($str) = @_;
    DisplayError($str);
    exit;
}
This seems a bit pointless. Doing DisplayError($foo) && exit at the site of the
call would be a lot clearer.

- GenerateSQL() is needed in other files - particularly, for the new charting
stuff. I'd like to move it to globals.pl and rename it to URLToSQL() or
something like that. But I'll do that if you like :-)

- Columns: there's not much point being able to shorten the resolution field to
3 chars if "Resolution" is 10 chars. I suggest you provide a short column title.

Hope that's a good start.

Gerv
I can't get the templates in the patch without annoying CVS-adding. :-(

Gerv
Attachment #59254 - Attachment is obsolete: true
I vote for converting all .atml templates to .tmpl before the 2.16 release. Gerv
is right: doing it later is just asking for trouble.
Gerv,

I think you may have been reviewing a very old version of the patch.  Many of
your issues were fixed in more recent versions, including version 8.  This
patch addresses the rest of them.

>- The patch has rotted somewhat. New patch coming up. (You might want to check

>it's OK, though.)

Version 8 applies cleanly with some offsets, and version 9 should apply
completely cleanly.  Is there anything in version 8a I should copy over?

>- Are we not using .tmpl rather than .atml?

Yep.

>- Also, I thought we were using file extensions on everything

Yep.

>- I'm surprised by "html.atml". I thought we were going for e.g.
>buglist.html.tmpl.

Yep.

> Also, for forward compatibility, should we be renaming all
>the current ones to conform to the agreed convention? Renaming them later will

>just cause checksetup.pl bloat, and mean we may have to mess with people's
>custom templates, which would be bad.

Yep, but that's another bug.

>- license headers required on all files

Done.

>- Were we going to make the secure styling dependent on buggroups?

Yep, this is done in stylesheet.css.tmpl.

>- </form> should be in html.atml - I think we should match tags in template
>files:
>[% IF dotweark %]
>  [% PROCESS buglist/tweakform %]
>  </form>
>[% END %]

Done

>- Use <label> on the dotweak checkboxes?

Done for all form fields for which it is possible, although bug 28657, bug
106344, and bug 107621 may limit its effectiveness with Mozilla for the time
being.

>- Capitalised and unformatted HTML in tweakform. 

Fixed.

>- You should be using a BLOCK for each of the nearly-identical select widgets
-
>see the sort of thing I do in query.atml and (better) the enter_bug.tmpl
patch.
>If you make the passed-in list variable names the same as the form names
>(except
>for "component" :-( ) you are laughing.

Yep.  I am laughing?

>- keywords.size is a bad test for keywords - we can be using keywords even if
>there are none on the bug. I use a boolean, "use_keywords" .
>    if (@::legal_keywords) {
>	 $vars->{'use_keywords'} = 1;
>Same may well apply for groups.

Done, except that it doesn't apply for groups AFAIK.

>- We should be sharing the knob code between show_bug.cgi and buglist.cgi. If
>you factor it out in its current form, I'll generalise it ('bug' or 'bugs',
>etc.). 

When you say "factor it out", do you mean "put that portion of the code into a
separate template file" or something more complicated?

>- "" appears in the resolution list 

Fixed.

>- "To make changes to a bunch of bugs at once:" -> "Multiple bugs", please :-)


Done.

>- Your "contains" method addition is extremely cool; I will adopt that :-)

Be my guest. :-)  In my patch it is part of a global template object
instantiated in globals.pl, so you can adopt it without having to add the code
to your own patch.

>- How did we decide to lay out multi-line boolean statements? I can't
remember,
>but I don't think it was like you are doing it. I could be wrong, though, and
I
>don't have the coding standards to hand.

Me neither, but I think I'm doing it right.  There are no multi-line statements
of mine in the buglist.cgi code, and the ones in the templates look fine to me.


>- sub Error {
>    my ($str) = @_;
>    DisplayError($str);
>    exit;
>}
>This seems a bit pointless. Doing DisplayError($foo) && exit at the site of
the
>call would be a lot clearer.

Done.

>- GenerateSQL() is needed in other files - particularly, for the new charting
>stuff. I'd like to move it to globals.pl and rename it to URLToSQL() or
>something like that. But I'll do that if you like :-)

Go for it.

>- Columns: there's not much point being able to shorten the resolution field
to
>3 chars if "Resolution" is 10 chars. I suggest you provide a short column
>title.

Hunh?  The title of Resolution is "Result" and it seems about the same width
(or less) than some four-character capitalized resolution abbreviations (like
FIXE).

>Hope that's a good start.

Yes, thank you!  And as your reward ;-), please take a look at this new patch!
OK - here's more :-)

I'm concerned about the performance impact of doing all that template
preparation, particularly the output formats stuff, for all scripts when many
could get away with something a bit more lightweight.

Would it be possible to have a set of functions in globals.pl,
InitBasicTemplate(), InitAdvancedTemplate(), InitOutputFormats() or the like,
which can be called at the top of CGIs which require them? This way, we can
avoid doing all the work when we don't need to.

Line 118ish of buglist.cgi: "a bunch of bugs" ;-)

Moving the quip list into the DB should be trivial after this patch and mine to
quips.cgi have both gone in.

We should say how long (> 4000) "too long for Bugzilla's tiny mind" is. And
capitalise "Bugzilla". And add "on individual bugs" to the end of that sentence.

Also, is that figure still safe in this age of six-digit bug numbers? What's the
maximum size a cookie can be? Can we use multiple cookies to get around it in a
backwardly-compatible way?

The Content-Disposition filename should have the named query name in it, if one
was used. And I think a datestamp might be good, too. How about either:
bugs_2001-01-01.html or AssignedToMe_2001-01-01.html , depending on whether
there's a named query?

Should the simple format be changed to have no style? Or no colouring style, at
any rate?

Style nit:
[% PROCESS columntitle column = columns.$id%] 
->
[% PROCESS columntitle column = columns.$id %]

In general, feel free to add more carriage returns to long lines in templates,
and line things up - it makes them much easier to read. Try and keep inside 80
columns in almost all circumstances. This particularly applies to the knob stuff.

Email addresses such as QA Contact and Assignee should be "de-gisburned", i.e.
abbreviated at e.g. 45 characters using the "abbreviations" hash.

In the knob stuff, please add a comment to separate out the selectmenu BLOCK, as
is done elsewhere. These blocks could be improved by making the name of the list
variable which holds the data the same as the column name (with the sad
exception of "component") and then rolling the menu name into the BLOCK -
something like:

[% PROCESS selectmenu sel = { title => "Target Milestone",
                              menuname => "target_milestone" } %]

Then access ${sel.menuname} for your data. Do you get what I'm getting at? This
is a pattern I've followed in several of my templates.

(default_format.html.tmpl) The <hr size="10"> looks odd. Why did you do that?

Clicking column headings to sort doesn't seem to work for me.

"Uncheck All" and "Check All" should be on a newline.

The "formats" thing doesn't seem to work; adding "format=rdf_format" or
"format=rdf" to the query string produces:

"The rdf_format output format is not supported by this script. Supported formats
are ."

(sic).

Gerv
OK, more comments :-)

I'm concerned about the performance impact of doing all that template
preparation, particularly the output formats stuff, for all scripts when many
could get away with something a bit more lightweight.

Would it be possible to have a set of functions in globals.pl,
InitBasicTemplate(), InitAdvancedTemplate(), InitOutputFormats() or the like,
which can be called at the top of CGIs which require them? This way, we can
avoid doing all the work when we don't need to.

Line 118ish of buglist.cgi: "a bunch of bugs" ;-)

Moving the quip list into the DB should be trivial after this patch and mine to
quips.cgi have both gone in.

We should say how long (> 4000) "too long for Bugzilla's tiny mind" is. And
capitalise "Bugzilla". And add "on individual bugs" to the end of that sentence.

Also, is that figure still safe in this age of six-digit bug numbers? What's the
maximum size a cookie can be? Can we use multiple cookies to get around it in a
backwardly-compatible way?

The Content-Disposition filename should have the named query name in it, if one
was used. And I think a datestamp might be good, too. How about either:
bugs_2001-01-01.html or AssignedToMe_2001-01-01.html , depending on whether
there's a named query?

Should the simple format be changed to have no style? Or no colouring style, at
any rate?

Style nit:
[% PROCESS columntitle column = columns.$id%] 
->
[% PROCESS columntitle column = columns.$id %]

In general, feel free to add more carriage returns to long lines in templates,
and line things up - it makes them much easier to read. Try and keep inside 80
columns in almost all circumstances. This particularly applies to the knob stuff.

Email addresses such as QA Contact and Assignee should be "de-gisburned", i.e.
abbreviated at e.g. 45 characters using the "abbreviations" hash.

In the knob stuff, please add a comment to separate out the selectmenu BLOCK, as
is done elsewhere. These blocks could be improved by making the name of the list
variable which holds the data the same as the column name (with the sad
exception of "component") and then rolling the menu name into the BLOCK -
something like:

[% PROCESS selectmenu sel = { title => "Target Milestone",
                              menuname => "target_milestone" } %]

Then access ${sel.menuname} for your data. Do you get what I'm getting at? This
is a pattern I've followed in several of my templates.

(default_format.html.tmpl) The <hr size="10"> looks odd. Why did you do that?

Clicking column headings to sort doesn't seem to work for me.

"Uncheck All" and "Check All" should be on a newline.

The "formats" thing doesn't seem to work; adding "format=rdf_format" or
"format=rdf" to the query string produces:

"The rdf_format output format is not supported by this script. Supported formats
are ."

(sic).

Gerv
First of all, thanks for the thorough review!

>I'm concerned about the performance impact of doing all that template
>preparation, particularly the output formats stuff, for all scripts when many
>could get away with something a bit more lightweight.
>
>Would it be possible to have a set of functions in globals.pl,
>InitBasicTemplate(), InitAdvancedTemplate(), InitOutputFormats() or the like,
>which can be called at the top of CGIs which require them? This way, we can
>avoid doing all the work when we don't need to.

The compilation cost is minimal, and the execution cost is only incurred when a
user submits a request of a script that uses formats and specifies a
non-default one.


>Line 118ish of buglist.cgi: "a bunch of bugs" ;-)

Heh, a remnant from the old buglist.cgi.  Fixed. :-)


>We should say how long (> 4000) "too long for Bugzilla's tiny mind" is. And
>capitalise "Bugzilla". And add "on individual bugs" to the end of that
>sentence.
>
>Also, is that figure still safe in this age of six-digit bug numbers? What's
>the maximum size a cookie can be? Can we use multiple cookies to get around it

>in a backwardly-compatible way?

A cookie HTTP response header line can be up to 4K in size, which limits
cookies to about that much length (minus a few bytes for the other information
in the header line). It doesn't make a lot of sense to tell the user how many
characters long the bug list can be, since they have no sense how many bugs
that is, and we can't give them a bug estimate since it varies so significantly
with bug number length, so I left the number out of the message (but added the
phrase "on individual bugs" to it).

Multiple cookies could run into number-of-cookie limitations (30 per site in
some browsers), plus they would could be a pain to implement.  Bug 111999 is
the right fix for this problem.


>The Content-Disposition filename should have the named query name in it, if
one
>was used. And I think a datestamp might be good, too. How about either:
>bugs_2001-01-01.html or AssignedToMe_2001-01-01.html , depending on whether
>there's a named query?

Done using format name-yyyy-mm-dd.html with named queries taken into
consideration.


>Should the simple format be changed to have no style? Or no colouring style,
at
>any rate?

Good question.	Simple format was requested in bug 104025, so I posted this
question to the reporter in that bug.


>Style nit:
>[% PROCESS columntitle column = columns.$id%] 
>->
>[% PROCESS columntitle column = columns.$id %]

Yuck.  Agreed and fixed.


>In general, feel free to add more carriage returns to long lines in templates,

>and line things up - it makes them much easier to read. Try and keep inside 80

>columns in almost all circumstances. This particularly applies to the knob
>stuff.

Done in almost all circumstances. ;-)


>Email addresses such as QA Contact and Assignee should be "de-gisburned", i.e.

>abbreviated at e.g. 45 characters using the "abbreviations" hash.

Done using the custom filter "truncate", which somewhat imitates the
complicated-to-use String plugin method of the same name but without the
plugin's always-show-ellipsis bug.


>In the knob stuff, please add a comment to separate out the selectmenu BLOCK,
>as is done elsewhere.

Done.


>These blocks could be improved by making the name of the
>list variable which holds the data the same as the column name (with the sad
>exception of "component") and then rolling the menu name into the BLOCK -
>something like:
>
>[% PROCESS selectmenu sel = { title => "Target Milestone",
>			       menuname => "target_milestone" } %]
>
>Then access ${sel.menuname} for your data. Do you get what I'm getting at?
This
>is a pattern I've followed in several of my templates.

Adding the menu title is tough because titles in the "change multiple bugs"
form are not uniform (f.e. some have anchors), and field names are ugly as Perl
variable names.


>(default_format.html.tmpl) The <hr size="10"> looks odd. Why did you do that?

I did it because that's how it is currently done.  You're right, it looks bad. 
Fixed (removed it and moved the quip above the HR that was above it).

>Clicking column headings to sort doesn't seem to work for me.

Fixed.


>"Uncheck All" and "Check All" should be on a newline.

These buttons make some sense next to a statement about how many bugs there are
in the list.  I wouldn't mind moving them if it makes them much more visible,
since users often want to change all bugs on the list but forget to click the
"Check All" button, but a newline really doesn't do much to change this.


>The "formats" thing doesn't seem to work; adding "format=rdf_format" or
>"format=rdf" to the query string produces:
>
>"The rdf_format output format is not supported by this script. Supported
>formats are ."

You need to run checksetup.pl after applying this patch so the list of content
types gets added to localconfig, then add "format=rdf".
Attachment #62607 - Attachment is obsolete: true
Attachment #63788 - Attachment is obsolete: true
+   "rdf" => "text/xml" , 

When are we supposed to use text/xml and when application/xml? 

The latest patch actually doesn't apply - buglist.cgi fails in a fairly
spectacular way. I would attempt the merging, but I figure you know this code
backwards, whereas I don't. I'm not sure what checkin upset it, but could you
remerge and repost?

Sorry for the delay which has led this to happen.

Gerv
>+   "rdf" => "text/xml" , 
>
>When are we supposed to use text/xml and when application/xml? 

The RFC on XML media types in general says to use text/xml for documents that
are "readable by casual users" and application/xml for documents that aren't.

ftp://ftp.isi.edu/in-notes/rfc3023.txt

The revised RDF grammar working draft specifies application/rdf+xml but cautions
that this is not yet a registered media type and should not yet be used.

http://www.w3.org/TR/rdf-syntax-grammar/

Mozilla, in line with the working draft, recognizes text/xml, application/xml,
and the non-standard text/rdf but does not recognize application/rdf+xml.  Of
these, "application/xml" seems like the best bet since RDF content is certainly
not readable by the casual user.  The XML format output by Bug.pm, on the other
hand, is quite readable, so I'll leave the xml -> "text/xml" mapping.

>The latest patch actually doesn't apply - buglist.cgi fails in a fairly
>spectacular way. I would attempt the merging, but I figure you know this code
>backwards, whereas I don't. I'm not sure what checkin upset it, but could you
>remerge and repost?

On its way.


Attached patch patch v11: applies cleanly (obsolete) — Splinter Review
This patch:

1. applies cleanly (updated to latest version in CVS with bbaetz's taint
changes integrated/redone);
2. modifies template names based on the latest conversations in n.p.m.webtools
plus removes filename extensions (but not the ".tmpl" extension) from template
fragments, since those fragments derive their content type from their
containing template(s);
3. uses global variables instead of "our" declarations in globals.pl (our
doesn't work on Perl 5.005);
4. modifies format utilities in globals.pl to work with new template names;
5. replaces usages of forward.html.tmpl with message.html.tmpl;
6. gives RDF files the "application/xml" content type.
Attachment #66028 - Attachment is obsolete: true
This version is exactly the same as the previous one except that it changes the
RDF namespace URI for Bugzilla:

from: http://www.mozilla.org/projects/bugzilla/rdf#
  to: http://www.bugzilla.org/rdf#
Attachment #67266 - Attachment is obsolete: true
This patch actually does what the last patch was supposed to do.  Brain fart,
sorry.
Attachment #67267 - Attachment is obsolete: true
*** Bug 109064 has been marked as a duplicate of this bug. ***
Comment on attachment 67269 [details] [diff] [review]
patch v13: actually changes the namespace

This is pretty good, so r=gerv. :-) However, if you have to rev this again,
here are some thoughts:

GetOutputFormats(). Do we need to tie the names of the files to the name of the
directory they are in? I can't see the advantage of doing that.

> menuname="target_milestone"

It's a nit, but I (at any rate) have been doing 'menuname = "target_milestone"'
throughout my templates.

If you have a second, search and replace <hr> to <hr />, and the same for <br>.
The <input> tag is the third big one for doing this.

+  [% PROCESS tableheader %]
+
+  [% FOREACH bug = bugs %]
+
+    [% IF loop.count() % 100 == 1 && loop.count() > 1 %]
+      </table>
+      <table class="bz_buglist" cellspacing="0" cellpadding="4" width="100%">
+      [% PROCESS tableheader %]
+    [% END %]
+
+    [% PROCESS tablerow %]
+
+  [% END %]

You could also do:
+  <table>
+  [% FOREACH bug = bugs %]
+
+    [% IF loop.count() % 100 == 0 %]
+      </table>
+      <table class="bz_buglist" cellspacing="0" cellpadding="4" width="100%">
+      [% PROCESS tableheader %]
+    [% END %]
+
+    [% PROCESS tablerow %]
+
+  [% END %]
+  </table>

which is a touch hacky (extra <table></table> at the top), but easier to
understand the gist of for the non-programmer.

Gerv
Attachment #67269 - Flags: review+
Blocks: 36013
>This is pretty good, so r=gerv. :-)

Cool. :-)


>GetOutputFormats(). Do we need to tie the names of the files to the name of the
>directory they are in? I can't see the advantage of doing that.

No, we don't have to do this.  I'll add an additional "template name" parameter
to that function and have the function use the name of the script as the
template name if the parameter is undefined.


>> menuname="target_milestone"
>
>It's a nit, but I (at any rate) have been doing 'menuname = "target_milestone"'
>throughout my templates.

Ok, I'll fix these.


>If you have a second, search and replace <hr> to <hr />, and the same for <br>.
>The <input> tag is the third big one for doing this.

Ok, will do.


>You could also do:
  ...
>which is a touch hacky (extra <table></table> at the top), but easier to
>understand the gist of for the non-programmer.

Ooh, I like that.  Ok, I'll do this too.
This patch implements Gerv's recommendations in his last review with the
exception of the change in how bug list tables are broken up, because it caused
the first table to contain one less than the correct number of bugs.
Attachment #67269 - Attachment is obsolete: true
+    [% IF loop.count() % 100 == 0 %]
+      </table>
+      <table class="bz_buglist" cellspacing="0" cellpadding="4" width="100%">
+      [% PROCESS tableheader %]
+    [% END %]

This will not leave 99 bugs in the first division. It will fire at loop.count()
== 0, and then print bug 0, and it will fire at loop.count == 100, and then
print bug 100. Between those two, there are exactly 100 bugs (numbers 0 through
99 in the list.)

Gerv
Gets Gerv's proposed loop.count() code working (loop.count() is a one-based
index, so loop.count() % 100 must be compared to one rather than zero).
Attachment #67735 - Attachment is obsolete: true
Blocks: 96101
Whether the loop is 0 or 1 based, it still shouldn't fire for index 0 or 1. 
There needs to be some kind of check (loop.count() > 0) that enforces this, or
we'll end up with extra tables.

I also thought that all zero tests in templates were going to be made explicit,
so as to promote understanding of the code.  If so, lines like the following
need to be fixed:

[% NEXT IF loop.count() % 2 %]

Attached patch patch v16: prettier loops (obsolete) — Splinter Review
This version re-implements the bug list loop yet again to be as easy as
possible to understand with minimal repetition of code and no extra tables (at
the cost of an extra conditional).  Also, I fixed the zero tests to be
explicit, since the conditional is testing for the numeric zero rather than the
boolean zero.
Attachment #67794 - Attachment is obsolete: true
In the hope that v16 is close to stable, I have tried to use this patch on our
bugzilla installation. It seems that IO/Dir.pm is now required, but
checksetup.pl does not check for it. Are there any other new dependencies on
external modules?
For now, I had to comment out its usage in globals.pl.

But then, I get another compilation error:
bugzilla@leibniz [~/bugzilla] /opt/perl5.0.0.4/bin/perl -cT buglist.cgi.NEW
Can't modify subroutine entry in scalar assignment at buglist.cgi.NEW line 604,
near ""$statusdefstable.name";"

Line 604 is the non-comment line in this paragraph:
             # When the operator is changedbefore, changedafter, changedto,
             # or changedby, $f appears in the query as "fielddefs.name = '$f'",
             # so it must be the exact name of the table/field as they appear
             # in the fielddefs table (i.e. attachstatusdefs.name).  For all
             # other operators, $f appears in the query as "$f = value", so it
             # should be the name of the table/field with the correct table
             # alias for this chart entry (f.e. attachstatusdefs_0.name).
             $f = ($t =~ /^changed/) ? "attachstatusdefs.name" :
"$statusdefstable.name";


Any ideas?
If there are more changes required, it would be very nice if they could be made
available as a small diff against a v16-patched version, instead of (or at least
in addition to) a all-in-one-more-than-100-KB patch. Merging with my other diffs
is everything else but easy, and I'd hate to have to back it out completely :-)
This patch removes the IO::Dir requirement, since IO::Dir doesn't appear to be
a standard part of Perl 5.005.	It should also fix the problem with the
subroutine for handling queries on attachment statuses by removing the errant
code (I got this patch mixed up with another one).
Attachment #67878 - Attachment is obsolete: true
File::Spec version 0.6 does not seem to provide method "splitpath", which is
used in globals.pl.
Requiring a newer version of File::Spec sucks, especially since Perl 5.005
comes with the older version.  Nevertheless, breaking cross-platform
compatibility also sucks, especially now that we are trying to become more
cross-platform compatible.  This patch adds a requirement for File::Spec v0.82
to checksetup.pl.
Attachment #67901 - Attachment is obsolete: true
Ok, now that I have installed File::Spec v0.82, the new buglist.cgi seems to
work. But, there is a huge performance regression:

(Reloading index.cgi takes about 3 seconds here.)
 
Benchmark 1: running buglist.cgi without any parameters, returning 326 bugs.

- With the old buglist.cgi, it takes about 3 seconds until the background of the
browser is painted white; after about 4 seconds there is the initial page
header, and after 4-5 seconds the page is loaded completely. So, it takes a
total of 4-5 seconds from clicking reload till the complete result is displayed,
and the first visible sign already appears after 3 seconds. This feels fast.

- With the new buglist.cgi, there is no visible sign until the complete buglist
appears completely after about 17-20 seconds (about 19 seconds on average). This
feels very slow.

Machine data:
sun4u sparc SUNW,Ultra-2, single cpu, solaris 2.6, 
perl, version 5.005_02 built for sun4-solaris-thread  

I know that precompiling the template will help a bit, but still...
Is there a way to use templates incrementally for each table row?

For small result sets, the differences are less problematic:

Benchmark 2: Running a quicksearch query with 5 bugs in the result.

- With the old buglist.cgi, the result appears after 2.5-3 seconds.

- With the new buglist.cgi, the result appears after 4 seconds.

I'm worried how long it will take for a query on bugzilla.mozilla.org to tell me
that the result of my query is something between 1000 and 2000 bugs...
Comment on attachment 68119 [details] [diff] [review]
patch v18: requires File::Spec v0.82

Another data point: A query returning 81 bugs takes about 8 seconds here.

And here's a quote from http://www.bugzilla.org/roadmap.html#design :

> Speed and efficiency should be maintained at all costs. One of Bugzilla's
> major attractions is its lightweight implementation and speed. Minimize calls
> into the database whenever possible, don't generate speed sucking HTML, don't
> fetch more data than you need to, etc, etc. 

In this case, I really agree with this design principle.

Marking needs-work.
Attachment #68119 - Flags: review-
A possible short-term workaround may be to truncate the list at 50-100 bugs, and
have a link "Show complete list (slow!)". But this would still be a loss in
usability.

Besides template precompilation, I think there should really be a way to create
output incrementally, so that the user gets some feedback before all data is
processed. And in this case, if the user clicks the "Stop" button, the script
definitely should not continue running until it is completed.
I don't think incremental output is particularly difficult, and I'm not suprised
this has performance problems (see comment #2).
I did some tests, loading buglist.cgi 50 times with no params locally based on
landfill's data from a few weeks back (432 bugs).

The original code: 1 min, 15 sec
template code: 3 min, 16 sec
template code with compilation + TT bugfix: 2 min, 43 sec

See bug 97832 for the toolkit bugfix, and other discussion on the testing strategy

I then tried a query which just did bugidtype=include&bug_id=1, again for 50 times:

original: 1 min 6 sec
template: 1 min 37 sec
template + compilation: 1 min 8 sec

(The resulting html was smaller originally - 3651 vs 4672 with the template.
This was mainly whitespace)

A query with ?field0-0-0=bug_id&type0-0-0=lessthan&value0-0-0=500 gave roughly
the same results as the first run (ie within 10 seconds each way) - its not some
strange parsing bug, I think.

So the templates aren't really the overhead - 2 seconds for the one bug query is
easily the margin of error, and I'd imagine that the template overhead should be
roughly a constant.

With the all bugs query, I did notice that there was a longer time before
results started coming. Lots of little template would help with that, but
without compilation it would probably be really really slow. It also still
wouldn't address the underlying problem, whatever that may end up being.

One note is that the perl which the template is compiled into has lots of lines
just concatenating single characters (usually \n). Could that be a slowdown? Or
would the perl compiler optimise it, rather than making lots of calls to the io
routines?
Since bz was down, I then ran a profiler. With compiled templates:

Total Elapsed Time = 2.751766 Seconds
         User Time = 2.072512 Seconds
Exclusive Times
%Time ExclSec CumulS #Calls sec/call Csec/c  Name
 27.6   0.573  3.690    477   0.0012 0.0077  Template::Provider::__ANON__
 22.2   0.461  0.689     24   0.0192 0.0287  main::BEGIN
 11.1   0.230  0.290  23341   0.0000 0.0000  Template::Stash::XS::get
 7.24   0.150  0.141   3526   0.0000 0.0000  Template::Iterator::get_next
 6.76   0.140  0.132   3025   0.0000 0.0000  Template::Filters::__ANON__
 5.31   0.110  0.102   3024   0.0000 0.0000  main::__ANON__
 5.31   0.110  0.102   3060   0.0000 0.0000  Template::Context::filter
 3.38   0.070  0.089    449   0.0002 0.0002  Template::Config::load
 3.38   0.070  0.119      9   0.0078 0.0132  Template::BEGIN
 2.41   0.050  3.746    475   0.0001 0.0079  Template::Context::process
 1.93   0.040  0.040      5   0.0080 0.0080  Template::Provider::_load_compiled
 1.45   0.030  0.021   3645   0.0000 0.0000  diagnostics::unescape
 1.45   0.030  0.040     14   0.0021 0.0028  Template::Provider::BEGIN
 0.97   0.020  0.020     33   0.0006 0.0006  Exporter::import
 0.97   0.020  0.016   1738   0.0000 0.0000  Template::Iterator::AUTOLOAD

With the original code:

Total Elapsed Time = 3.460685 Seconds
         User Time = 2.430685 Seconds
Exclusive Times
%Time ExclSec CumulS #Calls sec/call Csec/c  Name
 53.4   1.300  1.290   5186   0.0003 0.0002  main::pnl
 18.1   0.442  0.694     24   0.0184 0.0289  main::BEGIN
 2.47   0.060  0.129      9   0.0067 0.0144  Template::BEGIN
 1.65   0.040  0.033   3645   0.0000 0.0000  diagnostics::unescape
 1.65   0.040  0.037    467   0.0001 0.0001  main::FetchSQLData
 1.65   0.040  0.050      6   0.0067 0.0083  Template::Config::load
 0.82   0.020  0.019    432   0.0000 0.0000  main::html_quote
 0.82   0.020  0.020      3   0.0067 0.0067  File::Spec::Unix::BEGIN
 0.82   0.020  0.020      3   0.0067 0.0067  diagnostics::BEGIN
 0.82   0.020  0.020      8   0.0025 0.0025  base::import
 0.82   0.020  0.020      8   0.0025 0.0025  DBI::BEGIN
 0.82   0.020  0.050     14   0.0014 0.0035  Template::Provider::BEGIN
 0.41   0.010  0.010      1   0.0100 0.0100  Template::Stash::new
 0.41   0.010  0.010      2   0.0050 0.0050  DBI::_setup_driver
 0.41   0.010  0.010      1   0.0100 0.0100  Date::Parse::gen_parser

This claims that the original code took longer, which is obviously false. Take
this with a grain of salt... It does appear to show that the orignal code was a
bit inefficient, mind you. And indeed, removing that function and just doing it
directly decreased the time taken according to the profiler. It didn't change
the total output time though. Please don't ask me what this all means.

Oh, and this is using the XS Template::Stash, BTW
> So the templates aren't really the overhead 

Well. What my data shows is that there is a constant overhead *per row* (at
least without precompilation). This means the more bugs there are in the result,
the bigger the slowdown. Up to now, you could easily display long, long buglists
in almost no time, almost instantly. No matter if the result was "Zarro boogs",
or 8000 bugs, you would get the html page really fast.
Now things are different. Processing the output of a row now takes some
non-negligible amount of time. For a single row, this is not much, and you'll
hardly measure a difference. But for 50 rows, it *is* measurable. and for
several hundred bugs, it can easily take longer than the database query. You can
"feel" the linear dependency between the number of bugs in a result list, and
the time it takes to display it. Before, you did not feel it because the factor
was almost zero, and the html for a row was written so fast that it seemed that
the bottleneck was not the server, but the browser on the client side. This is a
fundamental change. 
Of course, there are still some queries where the database work outweighs the
time it takes to display the result. But the easier the query, and the larger
the result, the more likely it is that most of the waiting time is now caused by
the template processing.
The original code:                          1 min, 15 sec
template code with compilation + TT bugfix: 2 min, 43 sec

If a query with 400 bugs is still more than twice as slow, even with
precomilation turned on, then we should think about not templatizing the row
output at all. Maybe the buglist rows are the one performance-critical spot in
bugzilla that should not be templatized at all. I guess that most of the load on
a bugzilla server like b.m.o is generated by buglist queries, and it does not
seem to be a good idea to double that load.
Or, maybe there could be a preference, and for b.m.o the usage of templates for
rows should be turned off. There could be one template for everything up to the
start of the table, and one template for everything after the main table; this
probably wouldn't be so much of a problem.

I'm going through this now. The first thing I noticed is that whitespace
trimming doesn't appear to happen arround template comments. Does anyone know if
this is expected behaviour? Otherwise I'll cook up a test case and send it in as
a bug report (I have POST_CHOMP, PRE_CHOMP, and TRIM set - removing the comment
line makes it work)
Actually, I've changed my mind after some discussion on the TT list. What we're
actually saying is that there will be an additional overhead of about .003
seconds per bug. If we assume that the average query has 100 bugs, then thats .3
seconds per query. So yes, listing all 124000 bugs will take much longer.

However, on any vaguely complicated query the db overhead will be greater. IOW,
I don't think that this is an issue, really. The % sounds a lot greater, but if
I was running this not off local host it probably wouldn't be (I'm not set up to
do this test, though) 

Is there a way to get the template toolkit to do incremental output for us?
On the patch itsself, you should use vars $template and $vars, and then not use
the $:: form.
Blocks: 124127
> there will be an additional overhead of about .003 seconds per bug. 

Based on my benchmarks above, the additional overhead is about .05 seconds per
bug. We are probably using different machines, though. Are you saying that I
will have to upgrade my hardware? This is not an option, sorry.
No, I'm saying that the % diff will be different if the db has to do any work.
Try timing on a standard query which does not return the majority of bugs.
You are right that the percentage of the time spent for template processing is
less for complicated queries. But still, on my database with less than 400 bugs
all queries are quite fast. You are right that for result sets of 10 or 20 bugs
there is absolutely no problem. With buglists of 50 bugs it's still ok. But
imagine people on b.m.o getting the list of open bugs for the next milestone;
this is easily more than 1000 bugs. (And this _is_ a real-world example, believe
me.) Or take the list of all open layout bugs. The most important point is not
that it takes too long to display the result completely, but that you get a hint
that your query is much too broad only _after_ all the rows have already been
processed. Thus you have no chance to distinguish between a complicated query
using a lot of time inside mysql, and a large resultset, until after you have
gotten your result. Sometimes, I press the "Stop" button of my browser if b.m.o
takes too long (20-30 seconds or more) to respond; I then check my query if
there is something too complicated, and try a simpler one. But with this patch
it's possible that my query is just fine, it's just that there are a lot of
matches.

If we don't have an immediate fix for this, we should still land this, but only
after renaming the current buglist.cgi to buglist.old.cgi . This way, those
installations that have performance problems with the new one can continue to
use the old one until we have a speedy solution for this problem.
I think I've found a problem: The stars ("*") next to bug numbers of
confidential bugs (i.e. bugs only visible by members of a certain set of groups)
do not seem to appear on my buglists.
No longer blocks: 96101
I see that the star for "secure" bugs is missing on purpose:

> [%# Give secure bugs a noticable style to distinguish them from the others.
>   # Only do this for installations that are not using bug groups, since 
>   # almost all bugs are likely to be secure on installations that do use 
>   # bug groups, making any special styling a nuisance. 
>   #%]

While we have quite a lot of public bugs (that would appear without a star
anyway), the situation may be different on other installations, so I do not
insist on putting the stars back. :-)
Oh, and this fails the tests because the template test tries to open all the
template file without substituting for the filetype. The test should use
File::Find, or a glob, or something.
This is a patch to the Template Toolkit that implements the FLUSH directive for
flushing generated content to the output stream (i.e. sending partial results
of template processing back to the user).  To use this patch, download it along
with Template Toolkit 2.06d:

http://www.template-toolkit.org/download/Template-Toolkit-2.06d.tar.gz

Untar/gunzip it, change to the installation directory, and patch it:

tar xzf Template-Toolkit-2.06d.tar.gz
cd Template-Toolkit-2.06d
patch -p1 < location-of-patch-file

Then enter the parser sub-directory and rebuild the TT grammar module:

cd parser/
./yc

Note: you must have the Perl module Parse::Yapp installed along with the "yapp"
program, which should be provided by Parse::Yapp but didn't exist on my RedHat
7.2 box with Parse::Yapp pre-installed.  I force-installed Parse::Yapp again
from CPAN (CPAN told me I was "up-to-date" when I tried to install without
forcing), and that gave me "yapp".

Then go back one level to the TT installation directory and configure, make,
and install the patched TT:

cd ..
perl Makefile.pl
make
make test
make install

The patched TT may fail some tests.  Note that this is very early code.  It may
not work as expected.  It may fry all your data.  Use at your own risk!  Etc.

Now that you have installed it, add the [% FLUSH %] directive to places in your
templates where you want output sent back to the user (f.e. before/after every
bug row on the bug list page).	Note that very strange things can happen
(reordered content) depending on where you place the directive.  For the bug
list I was able to get it to work by placing it underneath the "[% BLOCK
tablerow %]" line in table.tmpl.  Your mileage may vary.

Anyway, experiment and try some performance tests to see how it feels. 
Naturally, flushing the data does nothing to speed up actual performance; it
still takes the same amount of time (if not more) to completely process the
page.  What this procedure *could* do, however, is speed up *apparent*
performance by showing users partial data before the entire page is generated. 
Keeping that in mind, try it out and see if it improves apparent performance.
Keywords: patch, review
Sorry, I won't be able to test this (short of time).
This patch implements a variety of performance improvements to maximize the
performance of the templatized version of the script.  Improvements include:

1. pre-compilation;
2. XS stash;
3. structural changes to templates;
4. incremental flushing to output stream (ignored unless TT is patched to
support it);
5. use of Bugzilla HTML-escaping routine in place of TT equivalent.

Non-performance related improvements include:

1. fixed a bug preventing certain column titles from appearing;
2. fixed bug 124127 about sorting by date with older versions of MySQL.

With this patch performance is quite good.  Since the new version of the CGI
script gets to the point of displaying something slightly quicker than the old
version, some queries will be faster.  With TT patched to support incremental
output, perhaps many queries will appear faster, since the old version does
*not* actually output incrementally but rather builds the entire bug table(s)
and then outputs it (any appearance to the contrary is due to the speed of your
network connection).

Nevertheless, for simple queries that return a lot of bugs, the new version
will remain slower overall than the old version.  This is hard to get around:
the new version does a lot more than the old version and in a more extensible
way, and we want the things the new version gives us.  If performance of this
version is still inadequate, then we need to look for performance improvements
elsewhere, perhaps in TT, definitely in mod_perl.
Attachment #68119 - Attachment is obsolete: true
Precompilation is a separate bug which I have a patch for. Please don't add it
here.

Also, the XS stash is a compile option, so you can't rely on it being present.
We should just document that enabling the stash by default is a really really
really good idea.

Is the plan to revert to the TT html_quote function if that speed issue is
resolved?
Per bbaetz' requests, this version does not specify use of the XS stash or
template pre-compilation.  re: the HTML filter, the idea is to use our own
until it's better to switch to the TT one, either because the TT version gets
faster, Bugzilla gets faster, or we need the extended escaping features of the
TT version.

bbaetz, can you review this?
Attachment #72876 - Attachment is obsolete: true
This patch fixes a bug in the previous patch implementing flushing to the
output stream.	With this patch, PROCESS directives with additional arguments
will work correctly.
Attachment #71799 - Attachment is obsolete: true
Some performance tests loading a 1000 row bug list with simple criteria (bug
numbers between 1001 and 2000) over a moderate-speed network connection
(~20-25KB/sec) connecting over a distance (Mountain View CA -> Budapest, Hungary):

                        Content Appears (secs)     Page Complete (secs)
original                5,5,5,5,5                  44,44,40,42,52 (44.4/42.5)
templatized             13,13,13,13,14             58,57,53,51,55 (54.8)
templatized w/flush     7,6,6,6,6                  46,43,45,44,45 (44.6)

(time to content appearance measured by human looking at clock; time to page
completion measured by Mozilla browser and rounded to nearest second)

(numbers in parentheses are mean averages; second value for original template
discards last result as possible fluke)

(all performance optimizations enabled, including XS stash, template
pre-compilation, and simple HTML filter)

A surprising result is that periodically flushing the output stream increases
overall performance in addition to decreasing the time to initial display of
content.  In fact, overall performance of the flushing version is on par with
the original non-templatized version.

There is one more performance optimization that could probably be performed on
this code: incremental retrieval of data from the database handler.  The current
code (as well as the original, pre-templatized code) slurps all the data from
the handler before displaying anything.  This is necessary because it needs to
know all bug numbers in the result before displaying HTTP headers in order to
add the "BUGLIST" cookie (used for navigating the bug list) to the headers.

Bug 111999 is about storing the bug list in the database rather than in a
cookie, and after that bug is fixed this additional performance optimization can
be attempted.  This would involve passing FetchSQLData() into the Template
Toolkit (along with DiffDate and any other function that manipulates data after
it gets returned from the database) and calling it for each bug.

It is likely that doing this will increase performance, but it may not be
necessary if mod_perl support gets checked in before then.
Are we going to have to get people to apply the FLUSH patch themselves, or are
you actively pushing to get it into the next version of TT?

(Same question for all the other TT fixes we appear to need.)

Gerv
I'm actively pushing to get FLUSH into TT, but no response yet from the TT
maintainer (Andy Wardley).  Worst-case scenario we'll supply and recommend the
patch to installations.  TT will silently ignore FLUSH directives if it doesn't
support them, so it isn't necessary to apply the patch to get this version of
buglist.cgi working.

What other TT fixes do we need?
Gerv, can you review this patch?
Comment on attachment 72955 [details] [diff] [review]
patch v20: doesn't specify XS stash and compile dir

A few brief comments before church; I'll come back and do a proper review soon.

>+# Use standard Perl libraries for cross-platform file/directory manipulation.
>+use File::Spec;
>+   

You can use File::Spec::Functions if you want to be able to write 
e.g. "catdir()" instead of "File::Spec->catdir()" everywhere.

>+        opendir(SUBDIR, $dirname) || next;
>+        my @files = readdir SUBDIR;
>+        closedir SUBDIR;
<snip>

We are doing these filesystem accesses for every buglist.cgi invocation, right?
Is that bad?

>+/* Style secure bugs if the installation is not using bug groups.
>+ * Installations that *are* using bug groups are likely to be using
>+ * them for almost all bugs, in which case special styling is not
>+ * informative and generally a nuisance.
>+ */
>+.bz_secure { color: black; background-color: grey; }

There currently doesn't seem to be any "if" about it :-) Also, you
should probably use TT comments in this file.

>+    <link href="css/buglist.css" rel="stylesheet" type="text/css" />

What did we decide about styles (e.g. colouring) on the simple buglist?

>+[% style_url = "css/buglist.css" %]

Should CSS files live in the same directory as the templates to which they
apply (or global, if they are global)?

Gerv
Comment on attachment 72955 [details] [diff] [review]
patch v20: doesn't specify XS stash and compile dir

I'll go look through this now. Initial comments:

Why a separate css sheet? This means that people can't replace it easily.

I'm a bit concerned that Andy Wardley hasn't commented on your patch (I notice
that your speed improivmenst stuff didn't go onto the list, BTW). What if he
doesn't like the syntax, and we ship, and then this causes errors with 2.16 +
TT 2.07? I havne't seen a comment from him on the TT list for a couple of
weeks.

The only other template stuff is the precompilation stuff, AFAIK.

Why the change to CGI.pl? All your calls have a message in them.

Where is your rtf dtd? (And why rtf not xml, btw?)

OK, thats the non buglist.cgi/template stuff.
Comment on attachment 72955 [details] [diff] [review]
patch v20: doesn't specify XS stash and compile dir

As well as the previous comments:

>Index: buglist.cgi
>==================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v
>retrieving revision 1.156
>diff -u -r1.156 buglist.cgi
>--- buglist.cgi	20 Jan 2002 01:44:36 -0000	1.156
>+++ buglist.cgi	7 Mar 2002 11:24:55 -0000

> 
> # Shut up misguided -w warnings about "used only once".  "use vars" just
> # doesn't work for me.
>-
> sub sillyness {
>     my $zz;
>     $zz = $::db_name;
>@@ -42,57 +49,140 @@
>     $zz = $::userid;
>     $zz = @::components;
>     $zz = @::default_column_list;
>-    $zz = @::legal_keywords;
>     $zz = @::legal_platform;
>     $zz = @::legal_priority;
>     $zz = @::legal_product;
>+    $zz = @::legal_keywords;

Aren't we trying to keep these in alphabetical order?

>     $zz = @::settable_resolution;
>     $zz = @::legal_severity;
>     $zz = @::versions;
>     $zz = @::target_milestone;
>-    $zz = %::proddesc;
>+    $zz = @::dontchange;
> };
> 

>+if ($::buffer =~ /&cmd-/) {
>+    my $url = "query.cgi?$::buffer#chart";
>+    print "Refresh: 0; URL=$url\n";

print "Location: $url\n"; would be better

>+# If the user requested a bug list in HTML format, include 

Why only html?

>+# a Content-Disposition header so the user's browser can suggest 
>+# a reasonable filename when the user saves their bug list to disk.  
> 
> 

>+sub GetGroupsByGroupSet {
>+    my ($groupset) = @_;
>+
>+    return if !$groupset;
>+
>+    SendSQL("
>+        SELECT  bit, name, description, isactive
>+          FROM  groups
>+         WHERE  bit & $groupset != 0

Nit: (bit & $groupset) != 0

> 
>+################################################################################
>+# Query Generation
>+################################################################################
> 

>@@ -172,10 +309,11 @@
>         my $c = trim($F{'votes'});
>         if ($c ne "") {
>             if ($c !~ /^[0-9]*$/) {
>-                return Error("The 'At least ___ votes' field must be a\n" .
>-                             "simple number. You entered \"" .
>-                             html_quote($c) . "\", which\n" .
>-                             "doesn't cut it.");
>+                my $htmlc = html_quote($c);
>+                DisplayError("The <em>At least ___ votes</em> field must be
>+                              a simple number.  You entered <kbd>$htmlc</kbd>,
>+                              which does not cut it.");

does not -> doesn't.


>             push(@specialchart, ["changedin",
>                                  "lessthan", $c + 1]);
>@@ -364,9 +504,9 @@
>          },
> 
>          "^cc," => sub {
>-            push(@supptables,                                                  
>+            push(@supptables,
>               ("LEFT JOIN cc cc_$chartid ON bugs.bug_id = cc_$chartid.bug_id LEFT JOIN profiles map_cc_$chartid ON cc_$chartid.who = map_cc_$chartid.userid"));
>-            $f = "map_cc_$chartid.login_name";  
>+            $f = "map_cc_$chartid.login_name";
>          },

I don't suppose I could get you to skip the whitespace changes here, to avoid
conflict with one of my patches? ;)

>-    my $query =  ("SELECT " . join(', ', @fields) .
>+    my $query =  ("SELECT DISTINCT " . join(', ', @fields) .
>                   " FROM $suppstring" .
>                   " WHERE " . join(' AND ', (@wherepart, @andlist)) .
>                   " GROUP BY bugs.bug_id");

Why the change? if you group bug bugs.bug_id, then you're distinct by
defintion. ACtually, only having
DISTINCT is more portable, since ANSI SQL only lets you select fields which
were in teh group by clause.
Doing otherwise is a mysql extension.

>+################################################################################
>+# Command Execution
>+################################################################################
> 

>-<TITLE>What a hack.</TITLE>
>-<A HREF="$url">Loading your query named <B>$::FORM{'namedcmd'}</B>...</A>
>-};
>+        print "Refresh: 0; URL=$url\n";

Again, is there a reason Location: isn't used?

>+        print "Content-Type: text/html\n\n";
>+        # Generate and return the UI (HTML page) from the appropriate template.
>+        $vars->{'title'} = "Loading your query named <b>$::FORM{'namedcmd'}</b>";

HTML in the <title> is silly.

More later.
Attachment #72955 - Flags: review-
Comment on attachment 72955 [details] [diff] [review]
patch v20: doesn't specify XS stash and compile dir

>+################################################################################
>+# Column Definition
>+################################################################################
> 

>+DefineColumn("id"                , "bugs.bug_id"                      , "ID"                );
>+DefineColumn("groupset"          , "bugs.groupset"                    , ""                  );

So people can now display groupset? Hmm....

Why no header then?

>+################################################################################
>+# Display Column Determination
>+################################################################################

>+# IMPORTANT! Never allow the groupset column to be displayed!
>+@displaycolumns = grep($_ ne 'groupset', @displaycolumns);

Silly question: Why not? IF they can see the bug, then they have to be in all
teh relevent groups
anyway.

>+################################################################################
>+# HTTP Header Generation
>+################################################################################
> 

>+# Generate HTTP headers
>+if ($format->{'extension'} eq "html") {
>+    print "Content-Type: text/html\n";
>+
>+    print "Content-Disposition: inline; filename=$filename\n";
>+    if ($order) {
>+        my $qorder = url_quote($order);
>+        print "Set-Cookie: LASTORDER=$qorder ; path=/; expires=Sun, 30-Jun-2029 00:00:00 GMT\n";

Again, why only do this for html?

>+    if (length($bugids) < 4000) {
>+        print "Set-Cookie: BUGLIST=$bugids\n";
>+    } else {
>+        print "Set-Cookie: BUGLIST=\n";
>+        $vars->{'toolong'} = 1;

Ditto.

>+    # Loop over each include directory in reverse so that format files
>+    # earlier in the path override files with the same name later in
>+    # the path (i.e. "custom" formats override "default" ones).
>+    foreach my $path (reverse @$includepath) {
>+        # Get the list of files in the given sub-directory if it exists.
>+        my $dirname = File::Spec->catdir($path, $subdir);
>+        opendir(SUBDIR, $dirname) || next;
>+        my @files = readdir SUBDIR;
>+        closedir SUBDIR;
>+        
>+        # Loop over each file in the sub-directory looking for format files
>+        # (files whose name looks like SCRIPT-FORMAT.EXT.tmpl).

Can't you just use a glob on the filesystem?

>+sub ValidateOutputFormat {

>+    # Validate the output format requested by the user.
>+    if (!$formats->{$format}) {
>+        my $escapedname = html_quote($format);
>+        DisplayError("The <em>$escapedname</em> output format is not 
>+          supported by this script.  Supported formats (besides the 
>+          default HTML format) are <em>" . 
>+          join("</em>, <em>", map(html_quote($_), keys(%$formats))) . 
>+          "</em>.");

Not all scripts will support html. (xml.cgi being the main one)

>Index: template/default/buglist/buglist-simple.html.tmpl
>===================================================================
>RCS file: template/default/buglist/buglist-simple.html.tmpl
>diff -N template/default/buglist/buglist-simple.html.tmpl
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ template/default/buglist/buglist-simple.html.tmpl	7 Mar 2002 11:25:03 -0000

Where is this file used?

>+++ template/default/buglist/buglist.html.tmpl	7 Mar 2002 11:25:03 -0000
>@@ -0,0 +1,161 @@

>+[%############################################################################%]
>+[%# Template Initialization                                                  #%]
>+[%############################################################################%]
>+
>+[% DEFAULT title = "Bug List" %]
>+[% title = title FILTER html %]

Why the FILTER?

>Index: template/default/global/header
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/template/default/global/header,v
>retrieving revision 1.5
>diff -u -r1.5 header
>--- template/default/global/header	3 Feb 2002 09:28:48 -0000	1.5
>+++ template/default/global/header	7 Mar 2002 11:25:03 -0000
>@@ -17,6 +17,8 @@
>       <style type="text/css">
>         [% style %]
>       </style>
>+    [% ELSIF style_url %]
>+      <link href="[% style_url %]" rel="stylesheet" type="text/css" />
>     [% END %]

should that be an ELSIF? You could have both - the url first, then locally
overridden rules afterwards.

>   </head>
>   <body [% Param('bodyhtml') %][% " " %][% extra %]>

OK, now let me test the thing.
Havne't tested with the TT pathc, but it looks mainly fine.

The bz_secure styling is a bit much though, I think. It attracts the eye too much.
Attached patch patch v21: review fixes (obsolete) — Splinter Review
Gerv wrote:

>We are doing these filesystem accesses for every buglist.cgi invocation,
right?

Nope, we are doing them only when the user requests a non-default format.

>>+.bz_secure { color: black; background-color: grey; }
>
>There currently doesn't seem to be any "if" about it :-) Also, you
>should probably use TT comments in this file.

The "if" is now in table.tmpl, and this is an actual CSS file rather than
a template that generates CSS.	I did it this way because it templatization
of this code is unnecessary, removing templatization makes buglist.cgi more
performant, and including CSS via a LINK tag makes it more easily overridable.

>What did we decide about styles (e.g. colouring) on the simple buglist?

We didn't decide, and it's not possible to know without more feedback
from users of this version, so let's leave it the way it is for now
and see what happens when people start using it.

>Should CSS files live in the same directory as the templates to which they
>apply (or global, if they are global)?

If they are templates that generate CSS, then they should probably live
in the same directory as the templates to which they apply.  Otherwise,
if they are CSS files, they should live in the top-level css/ directory.


bbaetz wrote:

>Why a separate css sheet? This means that people can't replace it easily.

It's even easier to replace a separate style sheet, and it improves 
performance because TT doesn't have to parse it.  Also, there is nothing 
in the current style sheet that needs to be parsed (the single conditional 
that was in there before makes just as much sense in table.tmpl).

>I'm a bit concerned that Andy Wardley hasn't commented on your patch (I notice

>that your speed improivmenst stuff didn't go onto the list, BTW). What if he
>doesn't like the syntax, and we ship, and then this causes errors with 2.16 +
>TT 2.07? I havne't seen a comment from him on the TT list for a couple of
>weeks.

Nothing Andy does will cause errors in Bugzilla.  TT is configured by default
(and in Bugzilla) to ignore directives it doesn't recognize, so a TT 
installation that doesn't support the FLUSH directive will silently ignore
it.

The worst that can happen if Andy doesn't like the syntax and it changes
(or doesn't make the next TT release) is that Bugzilla installations using
a vanilla TT build will not benefit from the FLUSH performance improvements.

This is not a tragedy, despite the numbers I posted earlier showing 
significant improvements with FLUSH, because those numbers are for the
worst-case scenario only, and the vast majority of cases will not experience
those performance problems.

>Why the change to CGI.pl? All your calls have a message in them.

At some point I had a bug in my code where I didn't have a message and the
call to DisplayError failed or threw a warning because $message was undefined.
My code is fixed now, but I left the fix in to guard against future errors.

>Where is your rtf dtd? (And why rtf not xml, btw?)

I don't have an RDF DTD yet.  I did RDF because I needed to implement
something to demonstrate the new multi-format capabilities of Bugzilla 
and because Mozilla can consume RDF meta-data in a variety of useful ways.

>Aren't we trying to keep these in alphabetical order?

Not that I know of, but it makes sense.  Done.

>print "Location: $url\n"; would be better

This is a good candidate for a follow-up bug, as I'm wary of fixing 
even more "problems" with the current code.

>Why only html?

Bad assumptions about user behavior.  Corrected.

>Nit: (bit & $groupset) != 0

Fixed.

>does not -> doesn't.

It doesn't matter much, but in principle contractions are to be avoided
in formal (as opposed to conversational) text.	Surprised that this isn't 
a nit, but fixed (as is a second occurence you didn't notice).

>I don't suppose I could get you to skip the whitespace changes here, to avoid
>conflict with one of my patches? ;)

Skipped.  I'm not sure it'll help your patch since I ream buglist.cgi quite
hard, but here you are.  Those spaces are crazy; clean them up in your patch!

>>-    my $query =  ("SELECT " . join(', ', @fields) .
>>+    my $query =  ("SELECT DISTINCT " . join(', ', @fields) .
>> " FROM $suppstring" .
>> " WHERE " . join(' AND ', (@wherepart, @andlist)) .
>> " GROUP BY bugs.bug_id");

>Why the change? if you group bug bugs.bug_id, then you're distinct by
>defintion. ACtually, only having
>DISTINCT is more portable, since ANSI SQL only lets you select fields which
>were in teh group by clause.
>Doing otherwise is a mysql extension.

My mistake.  I intended to remove GROUP BY in favor of DISTINCT, not include
both at once.  Fixed.

>HTML in the <title> is silly.

Fixed.

>So people can now display groupset? Hmm....

Nope, but groupset does get selected in the query, and the query and display
code sections use the same list of columns.

>Why no header then?

Because the groupset doesn't get displayed, and headers are generally only
useful when displaying the column.  Nevertheless, in reality these headers
are actually column titles, and those should always be defined, so fixed.

>Silly question: Why not? IF they can see the bug, then they have to be in all
>teh relevent groups
>anyway.

Silly answer: because. :-)  Seriously, though, you are probably right, but
this one is better fixed in a separate bug in case there are security
implications we haven't yet realized with this change.

>>+    if (length($bugids) < 4000) {
>>+	   print "Set-Cookie: BUGLIST=$bugids\n";
>>+    } else {
>>+	   print "Set-Cookie: BUGLIST=\n";
>>+	   $vars->{'toolong'} = 1;
>
>Ditto. [why do this only for HTML]

Because non-HTML clients are likely to ignore the cookie and do their own
version of bug list navigation.  For example, the RDF output doesn't include
"Next" and "Previous" links because any client based on the RDF data is
going to have its own built-in mechanism for doing this.

Of course, you never know what people will come up with (f.e. an XML format
converted into HTML via XSLT), but until someone does this (and especially
since that cookie is going away in favor of a database-based bug list) it 
would be better to save the up-to-4K of network bandwidth on each transaction 
for those clients that don't use this data.

>Can't you just use a glob on the filesystem?

I don't think a glob would help much in this case since the code would need 
to extract the format name and file extension via a regular expression anyway.

>Not all scripts will support html. (xml.cgi being the main one)

xml.cgi will go away once show_bug.cgi supports formats (and turns into
bug.cgi in the process).  Is there any other case of this at the moment?

>>+++ template/default/buglist/buglist-simple.html.tmpl 7 Mar 2002 11:25:03
-0000
>
>Where is this file used?

It isn't used anywhere inside Bugzilla; it is meant to be used outside Bugzilla

in an IFRAME embedded in a page talking about some component and its bugs.

>+[% title = title FILTER html %]

>Why the FILTER?

No good reason.  Removed.

>should that be an ELSIF? You could have both - the url first, then locally
>overridden rules afterwards.

Good point.  Fixed.

>The bz_secure styling is a bit much though, I think. It attracts the eye too
much.

I agree, but I'm stuck on a good CSS-based solution.  Suggestions?

Otherwise, this is ready for re-review.
Attachment #72955 - Attachment is obsolete: true
I'm more worried about FLUSH taking a paramater, or something.

>does not -> doesn't.

> It doesn't matter much, but in principle contractions are to be avoided
> in formal (as opposed to conversational) text.  Surprised that this isn't 
> a nit, but fixed (as is a second occurence you didn't notice).

Yeah, I'm sort of variable with labeling thisngs as nits or not ;)

I prefer the less formal approach.

> Skipped.  I'm not sure it'll help your patch since I ream buglist.cgi quite
> hard, but here you are.  Those spaces are crazy; clean them up in your patch!

You didn't change the backend pasring/query generation part of the code, so it
should still apply.

>> Where is this file used?

>It isn't used anywhere inside Bugzilla; it is meant to be used outside Bugzilla
>in an IFRAME embedded in a page talking about some component and its bugs.

So, how is one meant to get access to this template then? There has to be some
perl code which uses it. &simple=1 ?

[bz_secure]
>I agree, but I'm stuck on a good CSS-based solution.  Suggestions?

You can use :after to add the * in, if you have a class on the bug_id column.
Thats css3 only, though, but ISTR ie supports that. NS4 won't, though, but I
didn't test what the current code looks like in ns4 anyway. Hixie has some wierd
tests involving cascading and changes to the css specs which make stuff show in
some browsers but not others - he uses it for the red vs green backgrounds in
his evil css tests. If you want to go that way, we can probably work arround it.

Won't get to look at code til at least tomorrow
>So, how is one meant to get access to this template then? There has to be some
>perl code which uses it. &simple=1 ?

&format=simple

>You can use :after to add the * in, if you have a class on the bug_id column.

I don't want to use an asterisk at all though.  Asterisks are an archaic form of
hypertext from the printed material world, HTML and CSS give us much better
means of distinguishing and annotating data.  Ideally I would color the text of
secure bugs red, but critical and blocker bugs are already marked in that
manner.  Hmm, perhaps a red background would work.
Comment on attachment 73509 [details] [diff] [review]
patch v21: review fixes

>+# Generate a reasonable filename for the user agent to suggest to the user
>+# when the user saves the bug list.  Uses the name of the remembered query
>+# if available. 

Either Mozilla doesn't support this, or it isn't working in my build - 
20020301 Linux. I get "buglist.cgi" for a remembered query named "A".

>+    # This is stupid.  We really really need to move the quip list into the DB!

Sidenote: should we be doing this soon? If b.m.o's comments file is large,
it could be a perf win.

>+# Column:     ID                    Name                                 Title              
>+DefineColumn("id"                , "bugs.bug_id"                      , "ID"                );
<snip>

There's a bit too much whitespace in this list - some columns are quite a
bit wider than their widest member. Tighten it up?

Check that lot out, and r=gerv. The remaining bugs in this, if any, will
be found far faster by people testing and banging on it than be bbaetz and
I re-reading the patch for the 22nd time ;-)

Gerv
Attachment #73509 - Flags: review+
This patch fixes the problem with filename suggestions (had to add a
Content-Disposition header to the headers generated when we do server push) and
removes extraneous space from the DefineColumn calls.

The quips bug is 67950.  It hasn't had any action for a while, and it *should*
be easy to do, but it's also susceptible to feature creep judging from the
comments on it.

One more change per bbaetz's review: lightened the grey used to delineate
secure bugs so that it is not as harsh on the eyes.

Ready for that r=gerv. :-)
Attachment #73509 - Attachment is obsolete: true
Comment on attachment 73656 [details] [diff] [review]
patch v22: more review fixes

r=bbaetz, but it fails tests because there is no template called
buglist/$format->{'template'}

You need a separate patch tto ignore templates with $ in the name, I suppose
Attachment #73656 - Flags: review+
Comment on attachment 73656 [details] [diff] [review]
patch v22: more review fixes

Ah, Bugzilla's back up :-)

r=gerv.

Gerv
Attachment #73656 - Flags: review+
Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.136; previous revision: 1.135
done
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.157; previous revision: 1.156
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.127; previous revision: 1.126
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.148; previous revision: 1.147
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/css/buglist.css,v
done
Checking in css/buglist.css;
/cvsroot/mozilla/webtools/bugzilla/css/buglist.css,v  <--  buglist.css
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/buglist/buglist-rdf.rdf.tmpl,v
done
Checking in template/default/buglist/buglist-rdf.rdf.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/buglist/buglist-rdf.rdf.tmpl,v
 <--  buglist-rdf.rdf.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/buglist/buglist-simple.html.tmpl,v
done
Checking in template/default/buglist/buglist-simple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/buglist/buglist-simple.html.tmpl,v
 <--  buglist-simple.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/buglist/buglist.html.tmpl,v
done
Checking in template/default/buglist/buglist.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/buglist/buglist.html.tmpl,v
 <--  buglist.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/buglist/change-form.tmpl,v
done
Checking in template/default/buglist/change-form.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/buglist/change-form.tmpl,v 
<--  change-form.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/buglist/server-push.html.tmpl,v
done
Checking in template/default/buglist/server-push.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/buglist/server-push.html.tmpl,v
 <--  server-push.html.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/default/buglist/table.tmpl,v
done
Checking in template/default/buglist/table.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/buglist/table.tmpl,v  <-- 
table.tmpl
initial revision: 1.1
done
Checking in template/default/global/header;
/cvsroot/mozilla/webtools/bugzilla/template/default/global/header,v  <--  header
new revision: 1.6; previous revision: 1.5
done
Checking in template/default/global/message.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/global/message.html.tmpl,v 
<--  message.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Blocks: 130373
Blocks: 125798
No longer blocks: 109064
Blocks: 28884
This patch also makes $outstream the last parameter in the context->process
function to avoid a potential problem identified by Andy Wardley on the TT
mailing list and removes unnecessary use of that parameter.
Attachment #72993 - Attachment is obsolete: true
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.