730670 - Do not redirect in buglist.cgi to improve performance

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement)

Description

[Frédéric Buclin]

Attachment: patch, v1

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.

Comment 1

[:glob ✱]

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

Comment 2

[Frédéric Buclin]

Attachment: patch, v2

Restoring support for older browsers which do not support history.replaceState.

Comment 3

[:glob ✱]

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.

Comment 4

[Frédéric Buclin]

Attachment: patch, v2.1

Comment 5

[: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

Comment 6

[Frédéric Buclin]

Thanks! :)

Comment 7

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/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.

Comment 8

[Max Kanat-Alexander]

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?

Comment 9

[:glob ✱]

(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.

Comment 10

[Frédéric Buclin]

Attachment: fix HTML4 validation

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://[e-mail]/bugzilla/trunk/
modified template/en/default/global/common-links.html.tmpl
Committed revision 8335.