Closed
Bug 499828
Opened 15 years ago
Closed 14 years ago
kill LAZY_ADD and use async storage instead.
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla2.0b3
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: mak, Assigned: mak)
References
Details
Attachments
(1 file, 2 obsolete files)
37.21 KB,
patch
|
beltzner
:
approval2.0+
|
Details | Diff | Splinter Review |
Would be cool if we could throw away LAZY_ADD and use async storage.
Comment 1•15 years ago
|
||
Insofar as this causes bug 550778, this should probably block 1.9.3.
blocking2.0: --- → ?
Comment 2•15 years ago
|
||
Doesn't seem like this blocks by itself. Bug 550778 is the blocker, if this fixes it then awesome.
blocking2.0: ? → -
Assignee | ||
Comment 3•14 years ago
|
||
Actually, while we should absolutely make async frecency calculation before final, this bug can be fixed as soon as visits and titles are async. Indeed async visits and titles don't pass through lazy messages at all and icons are already async.
frecency does not pass through lazy messages and will probably hurt till we fix it, but is unrelated to this.
Also Bug 550778 should disappear regardless status of this bug once visits are added async.
So next steps (not necessarily in this order) are:
- fix this (bye bye laziness)
- make frecency async
- upgrade to sqlite 3.7.0
- use WAL journaling
- kill temp tables
- evaluate and kill any remaining sync step that could still exist in the full process.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•14 years ago
|
||
Assignee | ||
Comment 5•14 years ago
|
||
additional fixes, since Shawn is reviewing the first wip patch, this is like an interdiff, will merge patches after review.
Assignee | ||
Updated•14 years ago
|
Attachment #458681 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•14 years ago
|
Attachment #458757 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 6•14 years ago
|
||
PS: tryserver notified me of further test changes, so some more test change will be needed.
Assignee | ||
Comment 7•14 years ago
|
||
failure is still in test_bug_461710.html
Assignee | ||
Comment 8•14 years ago
|
||
requesting blocking because all of this stuff is redundant and blocks completing removal of temp tables (that should be a blocker itself)
blocking2.0: - → ?
Comment 9•14 years ago
|
||
Comment on attachment 458681 [details] [diff] [review]
wip patch
+++ b/docshell/test/chrome/bug293235_window.xul
- // Because of LAZY_ADD, we will not be notified for three seconds
- // Wait for four seconds just to be safe.
- setTimeout(nextTest, 4000);
+ // Because adding visits is async, we will not be notified imemdiately.
+ setTimeout(nextTest, 1000);
Should we maybe change this to listen for the proper notification here? Using a setTimeout seems risky.
+++ b/toolkit/components/places/src/AsyncFaviconHelpers.cpp
@@ -332,7 +332,7 @@ GetEffectivePageStep::HandleResult(mozIS
rv = row->GetUTF8String(0, spec);
FAVICONSTEP_FAIL_IF_FALSE_RV(NS_SUCCEEDED(rv), rv);
// We always want to use the bookmark uri.
- rv = mStepper->mPageURI->SetSpec(spec);
+ rv = NS_NewURI(getter_AddRefs(mStepper->mPageURI), spec);
How was this even OK before? I don't see where we were even setting mStepper->mPageURI, so we should have been crashing, right?
+++ b/toolkit/components/places/src/nsNavHistory.cpp
@@ -5503,22 +5434,18 @@ nsNavHistory::SetPageTitle(nsIURI* aURI,
if (aTitle.IsEmpty()) {
nsString voidString;
voidString.SetIsVoid(PR_TRUE);
+ rv = SetPageTitleInternal(aURI, voidString);
We should add a comment here as to why we use a void string (so it is stored as null in the db)
@@ -5793,18 +5720,6 @@ nsNavHistory::Observe(nsISupports *aSubj
if (prefService)
prefService->SavePrefFile(nsnull);
Huh...any idea why we do this? This should happen already AFAIK during shutdown...
+++ b/toolkit/components/places/tests/browser/browser_bug399606.js
@@ -107,7 +105,7 @@ function test() {
content.location.href = uri;
}
else {
- setTimeout(confirm_results, LAZY_ADD_TIMER * 2.5);
+ setTimeout(confirm_results, 0);
At the very least here, we should be using executeSoon, but I think we really want to make this use an observer, right (more reliable + more likely to not be random orange).
+++ b/toolkit/components/places/tests/head_common.js
@@ -288,7 +288,6 @@ function check_no_bookmarks() {
/**
* Sets title synchronously for a page in moz_places synchronously.
- * History.SetPageTitle uses LAZY_ADD so we can't rely on it.
We should make this method use setPageTitle now since we can rely on it instead of duplicating code.
+++ b/toolkit/components/places/tests/mochitest/bug_411966/redirect.js
@@ -156,8 +156,7 @@ function checkDB(data){
var referrer = this.mChannel.QueryInterface(Ci.nsIHttpChannel).referrer;
ghist.addURI(this.mChannel.URI, true, true, referrer);
- // We have to wait since we use lazy_add, lazy_timer is 3s
- setTimeout("checkDBOnTimeout()", 4000);
+ setTimeout(checkDBOnTimeout, 0);
We do not need the timeout here at all. I don't even think this test needs to query the database directly anymore.
+++ b/toolkit/components/places/tests/mochitest/test_bug_461710.html
@@ -98,9 +101,12 @@ function loadNextTest() {
- // Because of LAZY_ADD, the page won't be marked as visited until three seconds,
- // so wait for four seconds to be safe
- setTimeout(handleLoad, LAZY_ADD_TIMER * 2);
Ha! This comment was so wrong...
+++ b/toolkit/components/places/tests/unit/test_404630.js
try {
- faviconService.setAndLoadFaviconForPage(uri("http://www.mozilla.com"), null, false);
+ PlacesUtils.favicons.setAndLoadFaviconForPage(
+ null, uri("http://www.mozilla.com/favicon.ico"), false
+ );
+ do_throw("should throw because page param is null");
+ } catch (ex) {
+ do_check_true(true);
Would prefer you have a variable like |exceptionCaught| that you set to false at first, and then set to true in the catch. Then outside, check that it is true.
+ try {
+ PlacesUtils.favicons.setAndLoadFaviconForPage(
+ uri("http://www.mozilla.com"), null, false
+ );
do_throw("should throw because favicon param is null");
- } catch (ex) {}
+ } catch (ex) {
+ do_check_true(true);
+ }
ditto here
r=sdwilsh with comments addressed
Attachment #458681 -
Flags: review?(sdwilsh) → review+
Comment 10•14 years ago
|
||
Comment on attachment 458757 [details] [diff] [review]
additional tests fixes on top of wip patch
r=sdwilsh
Attachment #458757 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #9)
> +++ b/toolkit/components/places/src/AsyncFaviconHelpers.cpp
> @@ -332,7 +332,7 @@ GetEffectivePageStep::HandleResult(mozIS
> rv = row->GetUTF8String(0, spec);
> FAVICONSTEP_FAIL_IF_FALSE_RV(NS_SUCCEEDED(rv), rv);
> // We always want to use the bookmark uri.
> - rv = mStepper->mPageURI->SetSpec(spec);
> + rv = NS_NewURI(getter_AddRefs(mStepper->mPageURI), spec);
> How was this even OK before? I don't see where we were even setting
> mStepper->mPageURI, so we should have been crashing, right?
Actually mPageURI exists, but mMutable is false here now, it was not before, I guess lazy_add was making us act later when that was not a problem. I don't know nsIURI internals enough to figure out all cases where mMutable is set to false.
(In reply to comment #9)
> @@ -5793,18 +5720,6 @@ nsNavHistory::Observe(nsISupports *aSubj
> if (prefService)
> prefService->SavePrefFile(nsnull);
> Huh...any idea why we do this? This should happen already AFAIK during
> shutdown...
IIRC I did tests in the past and it was not saved without this piece, I think it was due to the fact we were shutting down at xpcom-shutdown. Now that we shutdown earlier it should not be an issue. But I'd prefer not touching it here.
Assignee | ||
Comment 12•14 years ago
|
||
addressed comments, all tests passing locally and now building on tryserver
Attachment #458681 -
Attachment is obsolete: true
Attachment #458757 -
Attachment is obsolete: true
Assignee | ||
Comment 13•14 years ago
|
||
Comment on attachment 459025 [details] [diff] [review]
rolled up patch v1.0
tryserver gave greeen, this is ready to land, low risk since is mostly code removal, with test fixes.
Attachment #459025 -
Flags: approval2.0?
Comment 14•14 years ago
|
||
Comment on attachment 459025 [details] [diff] [review]
rolled up patch v1.0
a=beltzner, someone should still come by and make the blocking decision, though, to ensure we catch it if it regresses/gets reopened.
Attachment #459025 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 15•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
Comment 16•14 years ago
|
||
blocking+, due to blocking of bug 552023, which blocks bug 563538, which is a blocker.
blocking2.0: ? → betaN+
You need to log in
before you can comment on or make changes to this bug.
Description
•