550778 - After the load of the page was completed, A timing to change in visited color is considerably delayed.

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US;
rv:1.9.3a3pre) Gecko/20100306 Minefield/3.7a3pre ID:20100306035513
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US;
rv:1.9.3a3pre) Gecko/20100306 Minefield/3.7a3pre ID:20100306035513

After the road of the page was completed, A timing to change in visited color is considerably delayed  (it takes about 1～2sec).

Is this behavior permitted?

Steps to Reproduce:
1. Start Minefield with new profile.
2. Go https://bugzilla.mozilla.org/
3. Open link ( "Search" on top of page )in background by mouse middle button click.

Actual Results:
  After the road of the page was completed, the timing of changing color of the link is considerably delayed.

Expected Results:
 The color of the link should change in visited immediately.


regression window:
works:
http://hg.mozilla.org/mozilla-central/rev/ad9729975289
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100224 Minefield/3.7a2pre ID:20100224075828

fails:
http://hg.mozilla.org/mozilla-central/rev/639b98ae11a8
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100224 Minefield/3.7a2pre ID:20100224094223

pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ad9729975289&tochange=639b98ae11a8

Comment 1

[Alice0775 White]

Sorry, spelling mistake in commen #0
s/After the road of the page/After the load of the page/g

Comment 2

[Shawn Wilsher :sdwilsh]

I would say that this is intentional from bug 461199 and is likely wontfix.

Comment 3

[Alice0775 White]

The new behavior worsens bodily sensation speed remarkably.

Firefox should give priority to the processing of the place where a user pays attention to.
I think that async processing becomes the opposite effect

Comment 4

[Boris Zbarsky [:bzbarsky]]

Shawn, it's not clear to me why the delay is as long as it is in this case.  Worth looking into?  1-2 secs is a _very_ long time...

Comment 5

[Shawn Wilsher :sdwilsh]

A slow hard drive or a slow computer could be a factor here, but I'd imagine page load is much faster now.  Knowing more about the system would certainly be useful.

Comment 6

[Alice0775 White]

(In reply to comment #4)
> Shawn, it's not clear to me why the delay is as long as it is in this case. 
> Worth looking into?  1-2 secs is a _very_ long time...

Even Internet Explorer 8 is much faster to changing visit color.
A color changes after almost click instantly.

(In reply to comment #5)
> A slow hard drive or a slow computer could be a factor here, but I'd imagine
> page load is much faster now.  Knowing more about the system would certainly be
> useful.
Even if 1sec becomes fast the loading of the page,because I open in a background, it does not matter.
The change of the foreground document paying my attention to is more impressive

I feel that the movement of the browser blunted obviously.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> (In reply to comment #4)
> > Shawn, it's not clear to me why the delay is as long as it is in this case. 
> > Worth looking into?  1-2 secs is a _very_ long time...
> 
> Even Internet Explorer 8 is much faster to changing visit color.
> A color changes after almost click instantly.
Oh - this is after a click?  This has nothing to do with bug 461199 then and it's because of our lazy timer on AddURI:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#5050

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm.  Why do we have that timer, exactly?  And why do we apply it to link navigations?  And why didn't this bite us before?

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> Hmm.  Why do we have that timer, exactly?  And why do we apply it to link
> navigations?  And why didn't this bite us before?
As far as I know, this always bit us in the case where you open a link in a new tab.  I'm on slow wireless right now so downloading old builds to test this is difficult.

We have that time, as I understand it, because when places was enabled by default, it regressed Tp.  The timer fixed the regression.  The current places team would love to get rid of it, but we need some more async APIs to pull that off.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I have a hard time reconciling comment 9's "this always bit us" with comment 0's "here is the regression range".

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> I have a hard time reconciling comment 9's "this always bit us" with comment
> 0's "here is the regression range".
Right, and I was concerned about that too, but I remember seeing bugs about this go by in the past too.  I'll look at older builds in a bit.

Comment 12

[Shawn Wilsher :sdwilsh]

I thought maybe the docshell would have notified earlier, but none of that code would have changed with bug 461199:
http://hg.mozilla.org/mozilla-central/rev/4ae1f164e1f0 (only changes to docshell)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Actually, docshell did notify earlier.  It's still notifying... but nothing is listening to the notification.  See nsDocShell::AddToGlobalHistory (which is doing an "is this visited?" check before adding, then notifying if not visited).

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> Actually, docshell did notify earlier.  It's still notifying... but nothing is
> listening to the notification.  See nsDocShell::AddToGlobalHistory (which is
> doing an "is this visited?" check before adding, then notifying if not
> visited).
So, yeah.  We used to listen to that notification, but then we said that we weren't going to use it for link coloring anymore and the IHistory implementation would have to take care of it.  This won't be a problem if we fix places by getting rid of LAZY_ADD (which I think we need to do for 1.9.3).

Comment 15

[Boris Zbarsky [:bzbarsky]]

> This won't be a problem if we fix places by getting rid of LAZY_ADD 

Sold.  Do we have a bug on that, or is that this bug?

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> Sold.  Do we have a bug on that, or is that this bug?
We should probably get a new bug for that;  I'll file that later today and update this bug.

Comment 17

[Marco Bonardo [:mak]]

 Bug 499828 -  kill LAZY_ADD and use async storage instead.

Comment 18

[Dave Townsend [:mossop]]

Sounds like this blocks, though maybe it needs to block beta1 for reasons I haven't spotted.

Comment 19

[Marco Bonardo [:mak]]

async addvisits skips lazy_add, so that should already be enough to take care of this delay.

Comment 20

[Marco Bonardo [:mak]]

this is indeed fixed. Thanks go to Benjamin Stover for the awesomeness of his work in implementing async visits.