Last Comment Bug 290254 - search engine isn't validated for the first time until updateCheckDays after first use
: search engine isn't validated for the first time until updateCheckDays after ...
Status: RESOLVED FIXED
: fixed1.8
Product: SeaMonkey
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: O. Atsushi (Torisugari)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-04-13 17:37 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2011-08-05 22:31 PDT (History)
10 users (show)
asa: blocking1.8b5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (3.49 KB, patch)
2005-08-18 21:57 PDT, O. Atsushi (Torisugari)
mconnor: review+
neil: superreview+
asa: approval1.8b5+
Details | Diff | Review
Patch (4.06 KB, patch)
2005-09-24 06:09 PDT, O. Atsushi (Torisugari)
neil: superreview+
Details | Diff | Review
Patch (4.08 KB, patch)
2005-09-25 20:13 PDT, O. Atsushi (Torisugari)
mconnor: review+
mconnor: superreview+
mconnor: approval1.8b5+
Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-04-13 17:37:28 PDT
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)
Comment 1 O. Atsushi (Torisugari) 2005-08-18 21:57:43 PDT
Created attachment 193127 [details] [diff] [review]
Patch

Per comment #0.

I doubt this bug is really blocking 1.5.
Comment 2 Asa Dotzler [:asa] 2005-09-02 12:41:42 PDT
Comment on attachment 193127 [details] [diff] [review]
Patch

mike or ben, can you guys review this?
Comment 3 Mike Connor [:mconnor] 2005-09-13 13:35:13 PDT
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)
Comment 4 neil@parkwaycc.co.uk 2005-09-22 16:46:26 PDT
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 ;-)
Comment 5 O. Atsushi (Torisugari) 2005-09-22 18:31:12 PDT
(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.
Comment 6 O. Atsushi (Torisugari) 2005-09-24 06:09:34 PDT
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.
Comment 7 neil@parkwaycc.co.uk 2005-09-25 13:40:45 PDT
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
Comment 8 Mike Connor [:mconnor] 2005-09-25 19:53:46 PDT
Can we get a nits-picked version of this patch for landing ASAP?
Comment 9 O. Atsushi (Torisugari) 2005-09-25 19:59:55 PDT
(In reply to comment #8)
> Can we get a nits-picked version of this patch for landing ASAP?

I'll try it.
Comment 10 O. Atsushi (Torisugari) 2005-09-25 20:13:21 PDT
Created attachment 197382 [details] [diff] [review]
Patch
Comment 11 Mike Connor [:mconnor] 2005-09-26 08:02:26 PDT
Comment on attachment 197382 [details] [diff] [review]
Patch

carrying over r+sr+a, changes made as requested
Comment 12 Mike Connor [:mconnor] 2005-09-26 09:06:06 PDT
Fixed branch and trunk.  Thanks for the patch!
Comment 13 O. Atsushi (Torisugari) 2005-09-26 19:06:55 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.