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)

Per comment #0. I doubt this bug is really blocking 1.5.

Comment on attachment 193127 [details] [diff] [review] Patch mike or ben, can you guys review this?

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 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 ;-)

(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.

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

Can we get a nits-picked version of this patch for landing ASAP?

(In reply to comment #8) > Can we get a nits-picked version of this patch for landing ASAP? I'll try it.

Comment on attachment 197382 [details] [diff] [review] Patch carrying over r+sr+a, changes made as requested

Fixed branch and trunk. Thanks for the patch!

(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.