Closed Bug 103778 Opened 23 years ago Closed 23 years ago

templatize buglist.cgi

Categories

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

Product:
Bugzilla
Component:
Query/Bug List
Version:
2.15
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.16

People

(Reporter: myk, Assigned: myk)

References

Details

Attachments

(2 files, 27 obsolete files)

patch v22: more review fixes
105.31 KB, patch
bbaetz
: review+
gerv
: review+
Details | Diff | Splinter Review
TT flush patch for 2.07
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: 23 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.

Attachment

General

Creator:
Created:
Updated:
Size: