These getHas methods have low value. Mano said in https://bugzilla.mozilla.org/show_bug.cgi?id=500391#c11: >> // URI >>- if (NS_SUCCEEDED(aQuery->GetHasUri(&hasIt)) && hasIt) >>+ if (NS_SUCCEEDED(aQuery->GetHasUri(&hasIt)) && hasIt) { >> BindStatementURI(statement, index.For("uri"), aQuery->Uri()); >>+ if (aQuery->UriIsPrefix()) { >>+ nsCAutoString uriString; >>+ aQuery->Uri()->GetSpec(uriString); > >At first, I though it would crash if if uriisprefix is set, but uri itself >isn't, but then I saw the hasIt check. I think we should rather check >aQuery->Uri() directly (actually, please file a bug to remove these hasIt* >methods altogether). I think he is right, and we do something similar in Annotations service, there is even worst since we often repeat 2 times the same annotation query to see if we have an anno and then to get the anno. The query time there is not trascurable, so there is not much value in doing that also.

todo: 1. actually run the tests 2. rev uuid

Comment on attachment 436549 [details] [diff] [review] v1 >diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp >--- a/toolkit/components/places/src/nsNavHistory.cpp >+++ b/toolkit/components/places/src/nsNavHistory.cpp >@@ -2274,18 +2274,18 @@ nsNavHistory::GetUpdateRequirements(cons > PRBool* aHasSearchTerms) > { > NS_ASSERTION(aQueries.Count() > 0, "Must have at least one query"); > > // first check if there are search terms > *aHasSearchTerms = PR_FALSE; > PRInt32 i; > for (i = 0; i < aQueries.Count(); i ++) { >- aQueries[i]->GetHasSearchTerms(aHasSearchTerms); >- if (*aHasSearchTerms) >+ nsAutoString searchTerms; >+ if (NS_SUCCEEDED(aQueries[i]->GetSearchTerms(searchTerms))) > break; before breaking you should update the return value (aHasSearchTerms) that is still set to false >@@ -2343,87 +2343,96 @@ PRBool > nsNavHistory::EvaluateQueryForNode(const nsCOMArray<nsNavHistoryQuery>& aQueries, > nsNavHistoryQueryOptions* aOptions, > nsNavHistoryResultNode* aNode) > { > // lazily created from the node's string when we need to match URIs > nsCOMPtr<nsIURI> nodeUri; > > for (PRInt32 i = 0; i < aQueries.Count(); i ++) { >- PRBool hasIt; > nsCOMPtr<nsNavHistoryQuery> query = aQueries[i]; > > // --- begin time --- >- query->GetHasBeginTime(&hasIt); >- if (hasIt) { >- PRTime beginTime = NormalizeTime(query->BeginTimeReference(), >- query->BeginTime()); >+ PRTime beginTime; >+ if (NS_SUCCEEDED(query->GetBeginTime(&beginTime))) { >+ PRUint32 beginTimeReference; >+ NS_WARN_IF_FALSE( >+ NS_SUCCEEDED(query->GetBeginTimeReference(&beginTimeReference)), >+ "GetBeginTimeReference threw"); use an rv here, it's a bit unreadable also, iirc NS_WARN_IF_FALSE is no-op in optimized builds, thus your code disappears. >+ PRTime endTime; >+ if (NS_SUCCEEDED(query->GetEndTime(&endTime))) { >+ PRUint32 endTimeReference; >+ NS_WARN_IF_FALSE( >+ NS_SUCCEEDED(query->GetEndTimeReference(&endTimeReference)), >+ "GetEndTimeReference threw"); >+ >+ endTime = NormalizeTime(endTimeReference, endTime); ditto >- PRBool hasSearchTerms = PR_FALSE; >- for (PRInt32 i = 0; i < aQueries.Count() && !hasSearchTerms; i++) { >- aQueries[i]->GetHasSearchTerms(&hasSearchTerms); >+ nsresult searchTermsRetval = NS_OK; >+ for (PRInt32 i = 0; i < aQueries.Count() && >+ NS_SUCCEEDED(searchTermsRetval); i++) { >+ nsAutoString searchTerms; >+ searchTermsRetval = aQueries[i]->GetSearchTerms(searchTerms); > } > > nsCAutoString tagsSqlFragment; > GetTagsSqlFragment(GetTagsFolder(), > NS_LITERAL_CSTRING("h.id"), >- hasSearchTerms, >+ NS_SUCCEEDED(searchTermsRetval), > tagsSqlFragment); why don't you just set hasSearchTerms = NS_SUCCEEDED(rv) inside the loop? would be more readable and save some change >diff --git a/toolkit/components/places/src/nsNavHistoryQuery.cpp b/toolkit/components/places/src/nsNavHistoryQuery.cpp > /* attribute PRTime beginTime; */ > NS_IMETHODIMP nsNavHistoryQuery::GetBeginTime(PRTime *aBeginTime) > { >+ if (mBeginTimeReference == TIME_RELATIVE_EPOCH && mBeginTime == 0) >+ return NS_ERROR_NOT_AVAILABLE; is not 0 from EPOCH a possible valid value? i guess if we should not init mBeginTime to -1 instead... >+ > *aBeginTime = mBeginTime; > return NS_OK; > } you should update idl adding "@throws if..." I'm unsure if it's better for implementers if we throw or we return an invalid value (like an empty search term or a negative time). I have a doubt (i'm practically sure) that the hasXXX were mostly to avoid having to try catch in js code. Can you ask dietrich about this please? Btw usually user builds a query and executes it, use-cases for examining a query are restrict. > /* attribute long beginTimeReference; */ > NS_IMETHODIMP nsNavHistoryQuery::GetBeginTimeReference(PRUint32* _retval) > { >+ if (mBeginTimeReference == TIME_RELATIVE_EPOCH && mBeginTime == 0) >+ return NS_ERROR_NOT_AVAILABLE; >+ > *_retval = mBeginTimeReference; > return NS_OK; > } ditto > /* attribute AUTF8String domain; */ > NS_IMETHODIMP nsNavHistoryQuery::GetDomain(nsACString& aDomain) > { >+ // Note that empty but not void is still a valid query (local files). >+ if (!mDomain.IsVoid()) >+ return NS_ERROR_NOT_AVAILABLE; in this case, maybe IsEmpty is better, no reasons to query on an empty nor void domain not ready for prime-time. i'll be away next days, you can get feedback from adw (he aleady worked on this code iirc) and final review from dietrich if this should be ready before my return.

I don't think throwing from attributews will make a nice experience for js consumers.