236025 - quicksearch.js:go_to() abuses sidebar flag.

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Christian Reis]

The go_to() function in quicksearch.js uses a flag named "sidebar" to indicate a
load_absolute_url() function should be used instead of the raw URL specified.
The variable name is silly, since the sidebar is only one possible place where
you'd want to convert the url specified to something else.  Bug 201235 derives
from a related problem, since create-guided.html.tmpl also specifies the flag
because it wants to load the search in an iframe and not in the main page.

Unless this code is changed to make this a lot clearer (which would be the best
solution), I suggest the flag be removed and the check be done for the presence
of a function, which I suggest should be called load_qs_url().

Comment 1

[Peter Henderson]

The sidebar flag seems to distinguish between loading the URL into the current
frame or into somewhere specific. Pseudocode:

goto(url) {
  if (sidebar == 1) {
    load_absolute_url(url);
  } else {
    document.location.href = url;
  }
}

This lets the sidebar define load_absolute_url() to load the url into the
content area instead of the current frame (which would be the sidebar area).
Other pages like the old guided bug entry page could define load_absolute_url()
to load the url into an iframe so the user could search while staying on the
same page and saving their partially-filled form.

However, bug 145588 changed the guided entry page so it doesn't use QuickSearch
anymore. The only two remaining places I can find that QuickSearch is used is on
the main page (index.cgi generated from index.html.tmpl) and in the sidebar
(sidebar.cgi generated from sidebar.xul.tmpl). The main page has a link called
[Help] that goes to quicksearch.html, and that has a link to
quicksearchhack.html, which I never noticed before, but these two pages both
seem to work identically to the main page.

So the sidebar flag was originally used to support the sidebar, then it was
abused for a bit by the guided entry page, and now we're back to just the
sidebar. Perhaps a better name would be use_custom_load_function or something
similar to emphasize the necessity of writing a load_absolute_url() function.
The only reason the load_absolute_url() function is there is to be parallel with
the sidebar's load_relative_url() function. We could even just say

if (sidebar == 1)
  content.location = url;

instead of messing with calling a load_absolute_url() function. That would, of
course, remove support for loading the results in an iframe. Alternatively, we
could change the QuickSearch() function to return an URL and let the calling
page deal with loading it where it wants to. Then we could remove the go_to()
function and sidebar flag altogether. This last way would break any other places
that use QuickSearch functionality, but it would remove the confusing need to
write a load_absolute_url() function.

Let me know which of
(1) rename sidebar flag
(2) make sidebar flag load into the content area directly instead of calling
load_absolute_url
(3) change QuickSearch() to return URL instead of loading it automatically

would be the best solution, and I can write a patch.

Comment 2

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.