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

RESOLVED FIXED

Status

SeaMonkey
Search
RESOLVED FIXED
13 years ago
6 years ago

People

(Reporter: dbaron, Assigned: O. Atsushi (Torisugari))

Tracking

({fixed1.8})

Trunk
fixed1.8
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

4.06 KB, patch
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
4.08 KB, patch
mconnor
: review+
mconnor
: superreview+
mconnor
: approval1.8b5+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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)

Updated

13 years ago
Flags: blocking-aviary1.1+
(Assignee)

Comment 1

12 years ago
Created attachment 193127 [details] [diff] [review]
Patch

Per comment #0.

I doubt this bug is really blocking 1.5.
Attachment #193127 - Flags: review?(timeless)

Updated

12 years ago
Flags: blocking-aviary1.5+ → blocking1.8b5?

Comment 2

12 years ago
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+

Updated

12 years ago
Flags: blocking1.8b5? → blocking1.8b5+

Updated

12 years ago
Whiteboard: [needs SR neil]

Comment 4

12 years ago
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+
(Assignee)

Comment 5

12 years ago
(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.

Updated

12 years ago
Attachment #193127 - Flags: approval1.8b5?
(Assignee)

Comment 6

12 years ago
Created attachment 197252 [details] [diff] [review]
Patch

(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
(Assignee)

Updated

12 years ago
Attachment #197252 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 7

12 years ago
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+

Updated

12 years ago
Attachment #193127 - Flags: approval1.8b5? → approval1.8b5+
Can we get a nits-picked version of this patch for landing ASAP?
Assignee: search → torisugari
(Assignee)

Comment 9

12 years ago
(In reply to comment #8)
> Can we get a nits-picked version of this patch for landing ASAP?

I'll try it.
(Assignee)

Comment 10

12 years ago
Created attachment 197382 [details] [diff] [review]
Patch
Attachment #197382 - Flags: approval1.8b5?
(Assignee)

Updated

12 years ago
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
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Whiteboard: [needs SR neil]
(Assignee)

Comment 13

12 years ago
(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.