Fix Acid3 Test 48: LINKTEST, Chromium side....

Fix Acid3 Test 48: LINKTEST, Chromium side....

R=brettw
BUG=http://crbug.com/231
BUG=http://crubg.com/5160
TEST=ExpireHistoryTest.ArchiveSomeOldHistory
TEST=ExpireHistoryTest.ExpiringVisitsReader
TEST=VisitedLinkTest.Listener
TEST=VisitedLinkTest.Resizing
TEST=VisitedLinkRelayTest.*
TEST=VisitedLinkEventsTest.*



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19910

[brettw, 2009-05-27 23:39:06]

http://codereview.chromium.org/113591/diff/114/1140
File chrome/browser/history/expire_history_backend.cc (right):

http://codereview.chromium.org/113591/diff/114/1140#newcode431
Line 431: if (is_early_expiration_run_) threshold /= kEarlyExpirationDivisor;
We normally use two lines for this kind of thing unless there's a strong reason
not to.

http://codereview.chromium.org/113591/diff/114/1140#newcode457
Line 457: if (is_early_expiration_run_)
This conditional should have {} since the arms are more than one line.

http://codereview.chromium.org/113591/diff/114/1140#newcode458
Line 458: main_db_->GetVisitsInRangeForTransition(Time(), effective_threshold,
The problem with this is that it will go through all visits brute-force from the
threshold to the end of the database. Most of these won't be subframes, so there
will be at least a decent amount of work.

On my profile, this will be about 8000 items, which it seems like we should no
be doing every 30 seconds.

One idea I had is if we store the time at which there are no subframe
navigations after that, the query could be pretty good since we would only have
to brute-force check each non-subframe once for all time.  Maybe we shoudl ask
shess if he has a brilliant idea.

[dglazkov, 2009-06-02 18:33:36]

Comments addressed. Added storing early expiration threshold in the meta table.
Please take a look.

[dglazkov, 2009-06-04 17:10:14]

BTW, all tests pass on all platforms now.

[brettw, 2009-06-04 18:15:32]

http://codereview.chromium.org/113591/diff/3014/3034
File chrome/browser/history/expire_history_backend.cc (right):

http://codereview.chromium.org/113591/diff/3014/3034#newcode461
Line 461: if (is_early_expiration_run_) {
It seems weird to me that half of the early-expiration logic is outside of this
function, and half is inside. How about passing a bool to this function "bool
auto_subframes_only". That way all uses of the is_early_expiration_run is in one
place.

http://codereview.chromium.org/113591/diff/3014/3034#newcode530
Line 530: return more_to_expire || !is_early_expiration_run_;
This is wrong. The idea here is that most of the time, we don't actually have
anything to expire. We shouldn't be waking  up the thread every minute to check.

When we're doing an early expiration, this will force a regular check the next
time even if there is nothing to do.

Maybe we need to track whether there is more regular expirations, and more
subframe expirations, and then schedule the opposite one next only if there was
more data from the previous run of that pass.

http://codereview.chromium.org/113591/diff/3014/3038
File chrome/browser/history/history_database.cc (right):

http://codereview.chromium.org/113591/diff/3014/3038#newcode196
Line 196: // Set to some non-zero time, to avoid re-retrieval.
some non-zero time -> a very early non zero time so it's before all history, but
not zero to avoid re-retrieval.

http://codereview.chromium.org/113591/diff/3014/3038#newcode205
Line 205: const Time &threshold) {
I would just use Time rather than a const ref here, since it's just a number,
and elsewhere in here we also pass by value.

[dglazkov, 2009-06-12 20:36:17]

Comments addressed. 

To make things pretties, I introduced the concept of a work queue into the
ExpireHistoryBackend, which allows us to have many types of history expiration
tasks and more flexibility in how they are processed. Check it out.

http://codereview.chromium.org/113591/diff/3014/3038
File chrome/browser/history/history_database.cc (right):

http://codereview.chromium.org/113591/diff/3014/3038#newcode196
Line 196: // Set to some non-zero time, to avoid re-retrieval.
On 2009/06/04 18:15:32, brettw wrote:
> some non-zero time -> a very early non zero time so it's before all history,
but
> not zero to avoid re-retrieval.

Done.

http://codereview.chromium.org/113591/diff/3014/3038#newcode205
Line 205: const Time &threshold) {
On 2009/06/04 18:15:32, brettw wrote:
> I would just use Time rather than a const ref here, since it's just a number,
> and elsewhere in here we also pass by value.

Done.

[brettw, 2009-06-15 23:35:46]

I don't have any comments on the substantial logic of this patch. It looks good.

I would like some unittests unittest for the code in profile.cc and
browser_render_process_host which is kind of complicated. There are several
levels of coalescing and caching, and different things happen for visible,
hidden, and just-becoming visible processes.

http://codereview.chromium.org/113591/diff/6035/6055
File chrome/browser/history/expire_history_backend.cc (right):

http://codereview.chromium.org/113591/diff/6035/6055#newcode29
Line 29: const int kEarlyExpirationAdvanceDays = 45;
Can you make this 30?

http://codereview.chromium.org/113591/diff/6035/6055#newcode244
Line 244: static const AutoSubframeVisitsReader reader;
These readers would make more sense to me as members on the EXpireHB class. You
can make them be lazily-initialized scoped_ptr<ExpiringVisitsReader) to avoid
putting the decls in the header.

http://codereview.chromium.org/113591/diff/6035/6049
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/113591/diff/6035/6049#newcode51
Line 51:
////////////////////////////////////////////////////////////////////////////////
Can you remove the ASCII art? I don't usually like these, and there is no
delimiter for the end of this section. There's only two things in it, so I would
just move the comment to right above the class without the horizontal dividers.

http://codereview.chromium.org/113591/diff/6035/6049#newcode71
Line 71: i != RenderProcessHost::end(); i++) {
You should normally use ++i for iterators which is more efficient: i++ involves
a copy.

Same twice below.

http://codereview.chromium.org/113591/diff/6035/6046
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/113591/diff/6035/6046#newcode80
Line 80:
////////////////////////////////////////////////////////////////////////////////
I wouldn't add these dividers.

http://codereview.chromium.org/113591/diff/6035/6046#newcode131
Line 131: // to renderer based on widget visibility.
Can you add a comment here about how this is different from the coalescer in
profile.cc?

http://codereview.chromium.org/113591/diff/6035/6046#newcode537
Line 537: visited_link_updater_->Buffer(links);
Can you remove the blank lines around this statement? When you have a blank line
between every statement it doesn't help anything.

http://codereview.chromium.org/113591/diff/6035/6046#newcode551
Line 551: channel()->Send(msg);
Why not just Send(new ViewMsg_VisitedLink_Reset())?

http://codereview.chromium.org/113591/diff/6035/6039
File chrome/browser/visitedlink_master.h (right):

http://codereview.chromium.org/113591/diff/6035/6039#newcode57
Line 57: VisitedLinkMaster(base::Thread* file_thread,
Can you add a comment about whether the listener can be NULL?

http://codereview.chromium.org/113591/diff/6035/6038
File chrome/browser/visitedlink_perftest.cc (right):

http://codereview.chromium.org/113591/diff/6035/6038#newcode36
Line 36: DummyVisitedLinkEventListener() { }
According to the style guide, empty curly brackets on the same line shouldn't
have a space in the middle.

http://codereview.chromium.org/113591/diff/6035/6052
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/113591/diff/6035/6052#newcode194
Line 194: for (size_t i = 0; i < fingerprints.size(); ++i)
The indenting is messed up.

[dglazkov, 2009-06-16 21:22:39]

Comments addressed, except for the unit tests. Working on these now...

http://codereview.chromium.org/113591/diff/6035/6055
File chrome/browser/history/expire_history_backend.cc (right):

http://codereview.chromium.org/113591/diff/6035/6055#newcode29
Line 29: const int kEarlyExpirationAdvanceDays = 45;
On 2009/06/15 23:35:47, brettw wrote:
> Can you make this 30?

Done.

http://codereview.chromium.org/113591/diff/6035/6055#newcode244
Line 244: static const AutoSubframeVisitsReader reader;
I was trying to save space :). Done.

http://codereview.chromium.org/113591/diff/6035/6049
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/113591/diff/6035/6049#newcode51
Line 51:
////////////////////////////////////////////////////////////////////////////////
On 2009/06/15 23:35:47, brettw wrote:
> Can you remove the ASCII art? I don't usually like these, and there is no
> delimiter for the end of this section. There's only two things in it, so I
would
> just move the comment to right above the class without the horizontal
dividers.

:)-\-< 

:)-/-<

:)-|-<

Done.

http://codereview.chromium.org/113591/diff/6035/6049#newcode71
Line 71: i != RenderProcessHost::end(); i++) {
On 2009/06/15 23:35:47, brettw wrote:
> You should normally use ++i for iterators which is more efficient: i++
involves
> a copy.
> 
> Same twice below.

Done.

http://codereview.chromium.org/113591/diff/6035/6046
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/113591/diff/6035/6046#newcode80
Line 80:
////////////////////////////////////////////////////////////////////////////////
On 2009/06/15 23:35:47, brettw wrote:
> I wouldn't add these dividers.

Done.

http://codereview.chromium.org/113591/diff/6035/6046#newcode131
Line 131: // to renderer based on widget visibility.
On 2009/06/15 23:35:47, brettw wrote:
> Can you add a comment here about how this is different from the coalescer in
> profile.cc?

Done.

http://codereview.chromium.org/113591/diff/6035/6046#newcode537
Line 537: visited_link_updater_->Buffer(links);
On 2009/06/15 23:35:47, brettw wrote:
> Can you remove the blank lines around this statement? When you have a blank
line
> between every statement it doesn't help anything.

Done.

http://codereview.chromium.org/113591/diff/6035/6046#newcode551
Line 551: channel()->Send(msg);
On 2009/06/15 23:35:47, brettw wrote:
> Why not just Send(new ViewMsg_VisitedLink_Reset())?

Done.

http://codereview.chromium.org/113591/diff/6035/6039
File chrome/browser/visitedlink_master.h (right):

http://codereview.chromium.org/113591/diff/6035/6039#newcode57
Line 57: VisitedLinkMaster(base::Thread* file_thread,
On 2009/06/15 23:35:47, brettw wrote:
> Can you add a comment about whether the listener can be NULL?

Done.

http://codereview.chromium.org/113591/diff/6035/6038
File chrome/browser/visitedlink_perftest.cc (right):

http://codereview.chromium.org/113591/diff/6035/6038#newcode36
Line 36: DummyVisitedLinkEventListener() { }
On 2009/06/15 23:35:47, brettw wrote:
> According to the style guide, empty curly brackets on the same line shouldn't
> have a space in the middle.

Done.

http://codereview.chromium.org/113591/diff/6035/6052
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/113591/diff/6035/6052#newcode194
Line 194: for (size_t i = 0; i < fingerprints.size(); ++i)
On 2009/06/15 23:35:47, brettw wrote:
> The indenting is messed up.

Done.

[brettw, 2009-06-24 23:33:46]

http://codereview.chromium.org/113591/diff/8075/8098
File chrome/browser/history/expire_history_backend.h (right):

http://codereview.chromium.org/113591/diff/8075/8098#newcode48
Line 48: VisitVector* visits, int max_visits) const = 0;
Indent 1 more space.

http://codereview.chromium.org/113591/diff/8075/8080
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/113591/diff/8075/8080#newcode1547
Line 1547: // what the TabContents' URL getter does.
The old code used GetURL for the reasons outlined in this comment. Are you sure
you wanted to change it to params.url. What about the conditions mentioned in
the comment?

http://codereview.chromium.org/113591/diff/8075/8106
File chrome/browser/visitedlink_event_listener.h (right):

http://codereview.chromium.org/113591/diff/8075/8106#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
Just 2009 is sufficient (same in .cc)

http://codereview.chromium.org/113591/diff/8075/8106#newcode12
Line 12: #include "chrome/browser/visitedlink_master.h"
Switch the order of these.

[dglazkov, 2009-07-01 22:42:32]

http://codereview.chromium.org/113591/diff/8075/8098
File chrome/browser/history/expire_history_backend.h (right):

http://codereview.chromium.org/113591/diff/8075/8098#newcode48
Line 48: VisitVector* visits, int max_visits) const = 0;
On 2009/06/24 23:33:46, brettw wrote:
> Indent 1 more space.

Done.

http://codereview.chromium.org/113591/diff/8075/8080
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/113591/diff/8075/8080#newcode1547
Line 1547: // what the TabContents' URL getter does.
You're right! Rearranged to do the right thing.

http://codereview.chromium.org/113591/diff/8075/8106
File chrome/browser/visitedlink_event_listener.h (right):

http://codereview.chromium.org/113591/diff/8075/8106#newcode1
Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
On 2009/06/24 23:33:46, brettw wrote:
> Just 2009 is sufficient (same in .cc)

Done.

http://codereview.chromium.org/113591/diff/8075/8106#newcode12
Line 12: #include "chrome/browser/visitedlink_master.h"
On 2009/06/24 23:33:46, brettw wrote:
> Switch the order of these.

Done.

[brettw, 2009-07-01 23:15:10]

LGTM

[dglazkov, 2009-07-02 21:55:30]

Please take one more teensy-weensy look. The only change is how we
WaitForCoalescense in visitedlink_unittest.cc

[brettw, 2009-07-02 22:00:56]

LGTM