150337 - [FIX] Previous button doubles factor

Status: NEW

(SeaMonkey :: Search, defect)

Description

[Matthew A. McGuigan]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0) Gecko/20020605
BuildID:    2002060511

Previous button doubles factor and sends user back two pages in results instead
of the previous results page.

Reproducible: Always
Steps to Reproduce:
1.Use any search plugin that has both the Next and Previous buttons enabled
using a factor.  Most of them use a factor.
2.Search for something that will return several pages of results.
3.Use the Next button to go forward more than one page in the results.
4.Use the Previous button.

Actual Results:  Using Previous button displays the results from the page prior
to the previous page.

Expected Results:  Previous button should go back one page in the results.

Comment 1

[Sam Schinke]

This problem of moving double the factor when pressing the previous button 
should probably be listed as a seperate bug, but I believe I've found it's 
cause.

The "showMoreResults" function in search-panel.js:

http://lxr.mozilla.org/seamonkey/source/xpfe/components/search/resources/search-
panel.js#996

And the "computeIndex" function in nsInternetSearchService.cpp:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/search/src/nsInternetSea
rchService.cpp#4482

Both decrement the "page" index by one when hitting "previous". "page" is then 
multiplied by "factor".

This side-effect of computeIndex is well-hidden by the fact that computeIndex 
is called via "getInputs" in InternetSearchService.cpp (line 4475). getInputs 
is called by "GetInternetSearchURL" in InternetSearchService.cpp, which is the 
function that is called from search-panel.js.

getInputs doesn't increment the page number when asked for the _next_ index, so 
it is somewhat inconsistent. Either the SearchService or the search-panel 
should be responsible for tracking the current index, not both.

Anyways, that's my interpretation of that oddity. It can be solved by removing 
lines 4659 to 4664 in nsInternetSearchService.cpp, but there might be other 
things that rely on this side-effect. It can also be solved by removing lines1 
011 and 1012 in search-panel.js, but that would probably be bad "practice" to 
rely on an undocumented side-effect of a function buried three calls deep.

Sam

Comment 2

[Met - Martin Hassman]

I have found independently the same solution as Sam Schinke.
The page number is incrementing and decrementing in search-panel.js, so the
other decrementing of page in nsInternetSearchService.cpp is wrong.

I have looked for any possible interferences and there is no problem.

Comment 3

[Met - Martin Hassman]

Attachment: patch removing wrong page decreasing

Adding a patch. Can someone review it?

Comment 4

[Erich 'Ricky' Iseli]

Comment on attachment 127433 [details] [diff] [review]
patch removing wrong page decreasing

Samir, can you please review this attachment?

Comment 5

[Erich 'Ricky' Iseli]

Removing blocking 123569 since this bug has not been pushed out or futured.
Let's wait for Samirs review and try to get a super-review first.

Comment 6

[Met - Martin Hassman]

Attachment: remove incorrect page decreasing

The same patch as attachment 127433 [details] [diff] [review], but better formed and make against actual
file version in CVS.

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: patch

Update to the previous patch which appears to have bit rotten. Thanks.

Comment 8

[Robert Strong (they/them - no direct email)]

I have included this patch in the patch for bug 150333 since they both affect
the same file and are both simple patches.

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch

This removes the page decrement as the earlier patch did and also removes the
direction arg which was used for the decrement and is no longer needed. I
checked lxr for other consumers and did not find any.

Comment 10

[Andrew Schultz]

*** Bug 285539 has been marked as a duplicate of this bug. ***

Comment 11

[Andrew Schultz]

Comment on attachment 127433 [details] [diff] [review]
patch removing wrong page decreasing

clearing stale requests