Closed Bug 290254 Opened 19 years ago Closed 19 years ago

search engine isn't validated for the first time until updateCheckDays after first use

Categories

(SeaMonkey :: Search, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: torisugari)

Details

(Keywords: fixed1.8)

Attachments

(2 files, 1 obsolete file)

Because InternetSearchDataSource::validateEngine calls validateEngineNow(), we
don't check for updates for a search plugin until the first use updateCheckDays
*after the first time it's used*.  This seems broken.  If there's no last
validation date, we should validate immediately, i.e., the

       if (rv == NS_RDF_NO_VALUE)

section should be removed but then the next section (which compares the dates)
should all be within

       if (rv != NS_RDF_NO_VALUE)
Flags: blocking-aviary1.1+
Attached patch Patch (obsolete) — Splinter Review
Per comment #0.

I doubt this bug is really blocking 1.5.
Attachment #193127 - Flags: review?(timeless)
Flags: blocking-aviary1.5+ → blocking1.8b5?
Comment on attachment 193127 [details] [diff] [review]
Patch

mike or ben, can you guys review this?
Attachment #193127 - Flags: review?(timeless) → review?(mconnor)
Comment on attachment 193127 [details] [diff] [review]
Patch

looks good to me, should get SR from neil, and should use tabs here, not
spaces, since that's what this function is using (yes, I know this file is a
complete mess)
Attachment #193127 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #193127 - Flags: review?(mconnor)
Attachment #193127 - Flags: review+
Flags: blocking1.8b5? → blocking1.8b5+
Whiteboard: [needs SR neil]
Comment on attachment 193127 [details] [diff] [review]
Patch

> 	LL_I2L(million, PR_USEC_PER_SEC);
> 	LL_DIV(temp64, now64, million);
> 	PRInt32		now32;
> 	LL_L2I(now32, temp64);
I wonder whether it's worth moving this to the calculation where we actually
use it.

>+    nsAutoString lastCheckStr(lastCheckUni);
Hmm, our string fu is out of date, this should be an nsDependentString ;-)

>+    PRInt32 lastCheckInt=0, err=0;
>+    lastCheckInt = lastCheckStr.ToInteger(&err);
>+    if (err)
>+      return NS_ERROR_UNEXPECTED;
Heh, err should have been an nsresult to be returned on failure ;-)

>+    PRInt32 durationSecs = now32 - lastCheckInt;
>+
>+    // calculate duration since last validation and
>+    // just return if its too early to check again
Please move the calculation back after its comment. sr=me with this fixed (the
others are optional, in reverse order).

>-		printf("    Search engine '%s' is now queued to be validated via HTTP HEAD method.\n",
>-			engineURI, durationSecs);
>+    printf("    Search engine '%s' is now queued to be validated"
>+           " via HTTP HEAD method.\n",
>+           engineURI);
Please don't do random clean up. Either adopt the file and clean it properly,
or touch as little as you dare ;-)
Attachment #193127 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
(In reply to comment #4)

> >+    PRInt32 durationSecs = now32 - lastCheckInt;
> >+
> >+    // calculate duration since last validation and
> >+    // just return if its too early to check again
> Please move the calculation back after its comment. sr=me with this fixed (the
> others are optional, in reverse order).
> 
> >-		printf("    Search engine '%s' is now queued to be validated via HTTP HEAD
method.\n",
> >-			engineURI, durationSecs);
> >+    printf("    Search engine '%s' is now queued to be validated"
> >+           " via HTTP HEAD method.\n",
> >+           engineURI);
> Please don't do random clean up. Either adopt the file and clean it properly,
> or touch as little as you dare ;-)

It's not a random idea. If the debug message really needs |durationSecs|, 
I have to give it a bit larger scope. However, I'd rather like to keep it
inside if(rv != NS_ERROR_NO_VALUE){...}, because the main purpose of
this bug is to make it clear whether we should check that duration or not.

If "search engine isn't validated for the first time until updateCheckDays
after first use", |durationSecs| always had a valid value, but it doesn't
any longer.

I had to remove every |durationSecs| outside of
if( rv != NS_ERROR_NO_VALUE ){...}, and *accidentally*, the above printf
function was crappy.
Attachment #193127 - Flags: approval1.8b5?
Attached patch PatchSplinter Review
(In reply to comment #4)
> I wonder whether it's worth moving this to the calculation where we actually
> use it.
I agree. Moved.


> Hmm, our string fu is out of date, this should be an nsDependentString ;-)

> Heh, err should have been an nsresult to be returned on failure ;-)

> Please move the calculation back after its comment. sr=me with this fixed
(the
> others are optional, in reverse order).

Done.
Thank you reading my ugly patch.
Attachment #193127 - Attachment is obsolete: true
Attachment #197252 - Flags: superreview?(neil.parkwaycc.co.uk)
Comment on attachment 197252 [details] [diff] [review]
Patch

>+    PRInt32 lastCheckInt=0, err=0;
Would you mind putting spaces around the = signs please.

>+    lastCheckInt = nsDependentString(lastCheckUni).ToInteger(&err);
>+    // signed int32 -> unsigned int32
>+    if (err)
>+      return (nsresult) err;
That sucks. Which idiot made ToInteger take a PRInt32* and store a value
greater than INT_MAX into it? But I guess this ought to say if
(NS_FAILED(err)).

>-		printf("    Search engine '%s' is now queued to be validated via HTTP HEAD method.\n",
>-			engineURI, durationSecs);
Maybe if you'd just removed the durationSecs I'd have realized first time :-P
Attachment #197252 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Attachment #193127 - Flags: approval1.8b5? → approval1.8b5+
Can we get a nits-picked version of this patch for landing ASAP?
Assignee: search → torisugari
(In reply to comment #8)
> Can we get a nits-picked version of this patch for landing ASAP?

I'll try it.
Attached patch PatchSplinter Review
Attachment #197382 - Flags: approval1.8b5?
Attachment #197382 - Flags: review?(mconnor)
Comment on attachment 197382 [details] [diff] [review]
Patch

carrying over r+sr+a, changes made as requested
Attachment #197382 - Flags: superreview+
Attachment #197382 - Flags: review?(mconnor)
Attachment #197382 - Flags: review+
Attachment #197382 - Flags: approval1.8b5?
Attachment #197382 - Flags: approval1.8b5+
Fixed branch and trunk.  Thanks for the patch!
Status: NEW → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Whiteboard: [needs SR neil]
(In reply to comment #12)
> Fixed branch and trunk.  Thanks for the patch!

I had a network trouble right after sending the patch, yesterday.
I'm sorry for being offline during that checkin, and thank you.
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: