GNOME Bugzilla – Bug 683550
Add a new urls-visited signal that is called in bulks
Last modified: 2012-09-10 05:15:09 UTC
Currently, starting epiphany with already 50 pages or more in the session makes the overview unusable for a few seconds. This is because the frecent store is being updated for every single emission of ::visit-url. Changing this signal to be called only in batches would not be right, since we are allowing clients to stop this signal emission if they want to prevent the url to be added to the history. That's why I've opted for adding a new signal ::urls-visited that will be emitted in a low priority idle. The following patches both add the signal and enable its usage in both the frecent and the history window models.
Created attachment 223722 [details] [review] ephy-history-service: add urls-visited signal This signal is emitted in bulks and is intended for clients that are not interested in every single URL that is visited but only to know that visits have happened.
Created attachment 223723 [details] [review] Use the new EphyHistoryService::urls-visited signal Since both the frecent store and the history window don't need to be updated immediately after a url is visited, we can use ::urls-visited instead. The advantage of this is that we reduce considerably the load when updating both models when loading many pages at the time, specially during startup.
Hey, this seems fine, but you basically had me convinced about doing the update in an idle by the client in response to ::visit-url. Did you change your mind?
Review of attachment 223722 [details] [review]: Anyway, assuming the idea here is to put the idle business in the signal emission so you don't have to duplicate it elsewhere. ::: lib/history/ephy-history-service.c @@ +148,3 @@ + + self->priv->queue_urls_visited_id = + g_idle_add_full (G_PRIORITY_LOW, (GSourceFunc) emit_urls_visited, self, NULL); No space in the cast. @@ +198,3 @@ + g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, + 0); I think it would be good to write a short explanation about the difference between the two signals.
Review of attachment 223722 [details] [review]: Oh, you probably want to remove the idle on dispose, too, otherwise you might emit the signal when the object is dead.
Review of attachment 223723 [details] [review]: Looks good.
(In reply to comment #3) > Hey, this seems fine, but you basically had me convinced about doing the update > in an idle by the client in response to ::visit-url. Did you change your mind? I think we had agreed that relying on good usage in the client side was not necessarily a good idea. More than one user could probably need to deal with idles, etc.
(In reply to comment #4) > I think it would be good to write a short explanation about the difference > between the two signals. I wrote that explanation in the commit log, but I can of course add it as a documentation comment.
(In reply to comment #5) > Review of attachment 223722 [details] [review]: > > Oh, you probably want to remove the idle on dispose, too, otherwise you might > emit the signal when the object is dead. Good catch, I forgot that. I'll push tomorrow with these fixes, thanks for reviewing.
Attachment 223722 [details] pushed as 399c91b - ephy-history-service: add urls-visited signal Attachment 223723 [details] pushed as c81e0a4 - Use the new EphyHistoryService::urls-visited signal