Closed Bug 730670 Opened 10 years ago Closed 10 years ago

Do not redirect in buglist.cgi to improve performance

Categories

(Bugzilla :: Query/Bug List, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: LpSolit, Assigned: LpSolit)

References

Details

(Keywords: perf)

Attachments

(2 files, 2 obsolete files)

Attached patch patch, v1 (obsolete) — Splinter Review
While playing with Devel::NYTProf, I realized that typing some search criteria in the QuickSearch box loaded buglist.cgi twice. This is due to $cgi->redirect_search_url, whose goal is to cleanup the URL and to fix the list_id parameter. But the drawback is that this means reloading buglist.cgi a 2nd time, with a time penalty of the order of 1 second on my machine (I use mod_cgi).

Instead of that, we should use the same fix as in bug 577720, i.e. use history.replaceState(). With my patch, it now takes 1 second instead of 2 to get the buglist when using a simple criteria in the QuickSearch box.

There is no need to fix other list/list.*.tmpl templates, because [% urlquerypart %] already contains the sanitized data.
Attachment #600764 - Flags: review?(glob)
Attachment #600764 - Flags: review?(dkl)
Comment on attachment 600764 [details] [diff] [review]
patch, v1

while the code works, this will cause the url to regress on IE, and any other browser which doesn't support history.replaceState
Attachment #600764 - Flags: review?(glob) → review-
Attached patch patch, v2 (obsolete) — Splinter Review
Restoring support for older browsers which do not support history.replaceState.
Attachment #600764 - Attachment is obsolete: true
Attachment #600870 - Flags: review?(glob)
Attachment #600764 - Flags: review?(dkl)
Comment on attachment 600870 [details] [diff] [review]
patch, v2

> <form name="queryform" method="get" action="buglist.cgi">
> <input type="hidden" name="query_format" value="specific">
> <input type="hidden" name="order" value="relevance desc">
>+<script type="text/javascript">
>+  if (history && history.replaceState)
>+    document.write('<input type="hidden" name="no_redirect" value="1">');
>+</script>

don't use document.write(), always create the hidden no_redirect field, and set its value with javascript.
Attachment #600870 - Flags: review?(glob) → review-
Attached patch patch, v2.1Splinter Review
Attachment #600870 - Attachment is obsolete: true
Attachment #600875 - Flags: review?(glob)
Comment on attachment 600875 [details] [diff] [review]
patch, v2.1

>+<script type="text/javascript">
>+  if (history && history.replaceState) {
>+    var no_redirect = document.getElementById("no_redirect");
>+    no_redirect.value = 1;
>+  }
>+</script>

i'd be happier with:
  document.getElementById("no_redirect").value = '1';
but what you've done is also fine :)


r=glob
Attachment #600875 - Flags: review?(glob) → review+
Thanks! :)
Flags: approval+
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified buglist.cgi
modified Bugzilla/CGI.pm
modified template/en/default/global/common-links.html.tmpl
modified template/en/default/list/list.html.tmpl
modified template/en/default/search/form.html.tmpl
modified template/en/default/search/search-specific.html.tmpl
Committed revision 8133.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 600875 [details] [diff] [review]
patch, v2.1

Review of attachment 600875 [details] [diff] [review]:
-----------------------------------------------------------------

::: template/en/default/list/list.html.tmpl
@@ +39,5 @@
> +      [%~ END %]
> +    [% END %]
> +  [% END %]
> +
> +  if (history && history.replaceState) {

Why not just use window.History (with a capital H) like we did for show_bug.cgi?
(In reply to Max Kanat-Alexander from comment #8)
> Why not just use window.History (with a capital H) like we did for
> show_bug.cgi?

you're probably thinking of custom-search, not show_bug.
it's quite different, and window.History unfortunately won't work here.
Blocks: 761199
While testing another patch, I realized that pages no longer passed HTML4 validation, due to a duplicated no_redirect ID. This patch fixes this problem.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified template/en/default/global/common-links.html.tmpl
Committed revision 8335.
Blocks: 819371
Blocks: 875217
You need to log in before you can comment on or make changes to this bug.