Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2856873002:
[Thumbnails DB] Allow setting last_requested time when accessing favicons. (Closed)
Description[Thumbnails DB] Allow setting last_requested time when accessing favicons.
This CL deals with favicons that are added without the user visiting the corresponding
page. The CL introduces the notion of "on-demand" favicons for them:
1) The CL reuses (and renames) an existing function in FaviconService for storing
such on-demand favicons and pushes this concept further to ThumbnailDatabase.
2) The CL adds a new function to touch (i.e. update last_requested time-stamp) for
"on-deman" favicons.
The functionality to clear unused on-demand favicons is left for a further CL
(https://codereview.chromium.org/2903573002/). Similarly, calling
TouchOnDemandFavicon from LargeIconService is also left for a later CL.
BUG=709471
Review-Url: https://codereview.chromium.org/2856873002
Cr-Commit-Position: refs/heads/master@{#481232}
Committed: https://chromium.googlesource.com/chromium/src/+/97b537bf1aba5ca6e106920faa65f140dd80df32
Patch Set 1 #Patch Set 2 : Minor touches #
Total comments: 7
Patch Set 3 : Rebase #Patch Set 4 : Filter out bookmarks #
Total comments: 1
Patch Set 5 : Filtering updates by bookmarks #Patch Set 6 : A new version: update the requested time explicitly #Patch Set 7 : Adapting and extending unit-tests #Patch Set 8 : Minor fixes #Patch Set 9 : Minor fixes #2 #Patch Set 10 : Splitting off clearing #
Total comments: 14
Patch Set 11 : Rebase #Patch Set 12 : Peter's comments #
Total comments: 34
Patch Set 13 : Peter's comments #2 #
Total comments: 6
Patch Set 14 : Rebase #Patch Set 15 : Brett's comment #Patch Set 16 : Peter's comments #Dependent Patchsets: Messages
Total messages: 57 (14 generated)
jkrcal@chromium.org changed reviewers: + pkotwicz@chromium.org
Peter, could you PTAL? https://codereview.chromium.org/2856873002/diff/20001/components/history/core... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2856873002/diff/20001/components/history/core... components/history/core/browser/history_backend.cc:1513: largest_icon.bitmap_id, /*access_time=*/Time::Now(), &last_updated, For all call 3 sites here, I need to pass Time() in incognito profiles, instead. What is the easiest way to find it out? Should I pass a |is_incognito| bit into the backend upon construction?
Sorry that I didn't get to the CL today. :( I'll take a look at it tomorrow
Sorry that I didn't get to the CL today. :( I'll take a look at it tomorrow
pkotwicz@chromium.org changed reviewers: + sky@chromium.org
Substituting myself with sky@ who will likely have an opinion The CL looks reasonable to me I did some digging and SetFaviconBitmapLastRequestedTime() does take longer than the database SELECT. The database SELECT on a Nexus 5x takes on average 50us and takes always less than 200us. SetFaviconBitmapLastRequestedTime() takes on average 300us and sometimes takes several milliseconds
Could you elaborate on how last_requested will be used?
On 2017/05/04 15:27:37, sky wrote: > Could you elaborate on how last_requested will be used? Sure. This is related to fetching favicons from a Google server (go/favicon-pe-google-server). - these icons get downloaded and cached without the user visiting the page, - previously, individual favicons get only deleted only for history entries getting expired, - last_requested will be used to garbage-collect such orphan favicons that have not been accessed for a while (you have previously acked this option in a discussion related to the design-doc)
Scott: I think that the |last_requested| field will be used as outlined in the "Removing expired favicons from the cache" section in https://docs.google.com/a/google.com/document/d/1rYLo_24nax7w5qEyJRpToaR9ykeW... The purpose of the field is to enable deleting icons which were stored in the database as a result of: LargeIconService::GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() -> FaviconService::SetLastResortFavicons()
Thanks for refreshing my memory! I'm at a meeting all day and will review either later tonight, or tomorrow morning. -Scott On Thu, May 4, 2017 at 8:34 AM, <jkrcal@chromium.org> wrote: > On 2017/05/04 15:27:37, sky wrote: > > Could you elaborate on how last_requested will be used? > > Sure. This is related to fetching favicons from a Google server > (go/favicon-pe-google-server > <https://goto.google.com/favicon-pe-google-server>). > - these icons get downloaded and cached without the user visiting the page, > - previously, individual favicons get only deleted only for history entries > getting expired, > - last_requested will be used to garbage-collect such orphan favicons that > have > not been accessed for a while > (you have previously acked this option in a discussion related to the > design-doc) > > https://codereview.chromium.org/2856873002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/04 15:45:57, sky wrote: > Thanks for refreshing my memory! I'm at a meeting all day and will review > either later tonight, or tomorrow morning. > > -Scott > > On Thu, May 4, 2017 at 8:34 AM, <mailto:jkrcal@chromium.org> wrote: > > > On 2017/05/04 15:27:37, sky wrote: > > > Could you elaborate on how last_requested will be used? > > > > Sure. This is related to fetching favicons from a Google server > > (go/favicon-pe-google-server > > <https://goto.google.com/favicon-pe-google-server>). > > - these icons get downloaded and cached without the user visiting the page, > > - previously, individual favicons get only deleted only for history entries > > getting expired, > > - last_requested will be used to garbage-collect such orphan favicons that > > have > > not been accessed for a while > > (you have previously acked this option in a discussion related to the > > design-doc) > > > > https://codereview.chromium.org/2856873002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Ok, thanks for the heads-up!
Remember that incognito profiles share the history backend/profile of the real profile. The scenario here is you've downloaded the favicon in a non-incognito profile (ignoring bookmarks), and then you visit the site in an incognito browser. Should that visit count toward keeping the favicon around? I'm inclined to think so. After all, the favicon is already there.
https://codereview.chromium.org/2856873002/diff/20001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:448: DeleteOrphanagedMappings(&db_); Drive by: Is it possible that this will end up deleting bookmark favicons on Android? In particular, this might end up deleting the nice large bookmark favicons and keep the version which was stored in the database via MergeFavicon()
Thanks for the comments! Scott, I am not sure about that. Do we download and cache favicons when in incognito? -> If yes, I agree that counting an incognito visit towards keeping the favicon around is consistent. However, I'd be surprised if Privacy is happy with this. -> If not, such an incognito visit would have a somewhat visible effect on your profile. I do not thing this is okay. I know this is quite paranoid because it is hard to make Chrome display the icon for something that you never visited in non-incognito (without fetching it on-demand, again). Furthermore, users hardly draw the line between icon being available & site being visited recently. From all the chats I had with Chrome Privacy here in Munich, such a level of paranoia seems standard to me. Do you feel strongly that not caring about incognito here is okay? If not, should I informally ask Chrome privacy? https://codereview.chromium.org/2856873002/diff/20001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:448: DeleteOrphanagedMappings(&db_); On 2017/05/04 18:34:19, pkotwicz wrote: > Drive by: Is it possible that this will end up deleting bookmark favicons on > Android? In particular, this might end up deleting the nice large bookmark > favicons and keep the version which was stored in the database via > MergeFavicon() Huh, correct! I've forgotten about that. I will address this problem in the next patch set. Thanks for spotting this!
On Fri, May 5, 2017 at 9:36 AM, <jkrcal@chromium.org> wrote: > Thanks for the comments! > > Scott, I am not sure about that. > Do we download and cache favicons when in incognito?' > We always download if we don't have the favicon in the cache. In incognito mode we only update the cache icon if the page is bookmarked. > > -> If yes, I agree that counting an incognito visit towards keeping the > favicon > around is consistent. However, I'd be surprised if Privacy is happy with > this. > -> If not, such an incognito visit would have a somewhat visible effect on > your > profile. I do not thing this is okay. > Again, the scenario I am referring to is that we *already* have the icon on disk, and there is no way for end users to detect the last time of usage of the favicon. > I know this is quite paranoid because it is hard to make Chrome display > the icon > for something that you never visited in non-incognito (without fetching it > on-demand, again). Furthermore, users hardly draw the line between icon > being > available & site being visited recently. From all the chats I had with > Chrome > Privacy here in Munich, such a level of paranoia seems standard to me. > > Do you feel strongly that not caring about incognito here is okay? > If not, should I informally ask Chrome privacy? > I freely admit I am not a privacy expert, so feel free to reach out to one of the privacy expects for their thoughts. Just be sure to clearly spell out we are not storing something new, just updating the visit time that is *not* shown to the user in anyway. -Scott > > > > https://codereview.chromium.org/2856873002/diff/20001/ > components/history/core/browser/thumbnail_database.cc > File components/history/core/browser/thumbnail_database.cc (right): > > https://codereview.chromium.org/2856873002/diff/20001/ > components/history/core/browser/thumbnail_database.cc#newcode448 > components/history/core/browser/thumbnail_database.cc:448: > DeleteOrphanagedMappings(&db_); > On 2017/05/04 18:34:19, pkotwicz wrote: > > Drive by: Is it possible that this will end up deleting bookmark > favicons on > > Android? In particular, this might end up deleting the nice large > bookmark > > favicons and keep the version which was stored in the database via > > MergeFavicon() > > Huh, correct! I've forgotten about that. I will address this problem in > the next patch set. Thanks for spotting this! > > https://codereview.chromium.org/2856873002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #3 (id:40001) has been deleted
I am sorry for the long delay. I've implemented the comment by Peter. I've also reached out to Chrome privacy. They insisted quite strongly that we should not leave any traces that could reveal your visits in incognito. Even though we do not show the dates to the user, they are stored in an unencrypted database that anyone else can read. https://codereview.chromium.org/2856873002/diff/20001/components/history/core... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2856873002/diff/20001/components/history/core... components/history/core/browser/history_backend.cc:1513: largest_icon.bitmap_id, /*access_time=*/Time::Now(), &last_updated, On 2017/05/02 13:57:25, jkrcal wrote: > For all call 3 sites here, I need to pass Time() in incognito profiles, instead. > What is the easiest way to find it out? Should I pass a |is_incognito| bit into > the backend upon construction? Because of the privacy discussion, I still believe I need to implement this. What is the easiest way to find out here that the user is in incognito? https://codereview.chromium.org/2856873002/diff/20001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/20001/components/history/core... components/history/core/browser/thumbnail_database.cc:448: DeleteOrphanagedMappings(&db_); On 2017/05/05 16:36:46, jkrcal wrote: > On 2017/05/04 18:34:19, pkotwicz wrote: > > Drive by: Is it possible that this will end up deleting bookmark favicons on > > Android? In particular, this might end up deleting the nice large bookmark > > favicons and keep the version which was stored in the database via > > MergeFavicon() > > Huh, correct! I've forgotten about that. I will address this problem in the next > patch set. Thanks for spotting this! I've reimplemented the code in order to filter out bookmarks. The favicons are now deleted one-by-one. I am wondering how to improve worst-case performance for deleting a set of favicons by id. Do you think that - setting up a transaction or - deleting in batches by "WHERE id IN (?,?,?,?,?,?,?,..,?)" with e.g. up to 100 ids per query would be worth it?
https://codereview.chromium.org/2856873002/diff/20001/components/history/core... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2856873002/diff/20001/components/history/core... components/history/core/browser/history_backend.cc:1513: largest_icon.bitmap_id, /*access_time=*/Time::Now(), &last_updated, On 2017/05/15 14:26:57, jkrcal wrote: > On 2017/05/02 13:57:25, jkrcal wrote: > > For all call 3 sites here, I need to pass Time() in incognito profiles, > instead. > > What is the easiest way to find it out? Should I pass a |is_incognito| bit > into > > the backend upon construction? > > Because of the privacy discussion, I still believe I need to implement this. > What is the easiest way to find out here that the user is in incognito? You will have to change callers to supply whether in incognito mode or not.
Thanks! https://codereview.chromium.org/2856873002/diff/20001/components/history/core... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2856873002/diff/20001/components/history/core... components/history/core/browser/history_backend.cc:1513: largest_icon.bitmap_id, /*access_time=*/Time::Now(), &last_updated, On 2017/05/15 15:57:13, sky wrote: > On 2017/05/15 14:26:57, jkrcal wrote: > > On 2017/05/02 13:57:25, jkrcal wrote: > > > For all call 3 sites here, I need to pass Time() in incognito profiles, > > instead. > > > What is the easiest way to find it out? Should I pass a |is_incognito| bit > > into > > > the backend upon construction? > > > > Because of the privacy discussion, I still believe I need to implement this. > > What is the easiest way to find out here that the user is in incognito? > > You will have to change callers to supply whether in incognito mode or not. Huh, now I see better why you pushed in the direction of being okay privacy-wise :| Do I get it right that the history service/backend/databases are shared between the profile and its incognito variant (so that I cannot pass the incognito bit into the ctor)? Is it then always the callers of HistoryService who make sure incognito is respected? Adding the incognito bit into whole GetFavicon* API feels really ugly. I am considering a different approach: - change the factory of FaviconService to create separate instances for the main profile and for incognito. FaviconService is quite a thin layer on top of HistoryBackend so it feels okay to me, memory-wise. - let the last_requested time be updated only from calls from non-icognito FaviconService. WDYT?
On Mon, May 15, 2017 at 11:36 AM, <jkrcal@chromium.org> wrote: > Thanks! > > > https://codereview.chromium.org/2856873002/diff/20001/ > components/history/core/browser/history_backend.cc > File components/history/core/browser/history_backend.cc (right): > > https://codereview.chromium.org/2856873002/diff/20001/ > components/history/core/browser/history_backend.cc#newcode1513 > components/history/core/browser/history_backend.cc:1513: > largest_icon.bitmap_id, /*access_time=*/Time::Now(), &last_updated, > On 2017/05/15 15:57:13, sky wrote: > > On 2017/05/15 14:26:57, jkrcal wrote: > > > On 2017/05/02 13:57:25, jkrcal wrote: > > > > For all call 3 sites here, I need to pass Time() in incognito > profiles, > > > instead. > > > > What is the easiest way to find it out? Should I pass a > |is_incognito| bit > > > into > > > > the backend upon construction? > > > > > > Because of the privacy discussion, I still believe I need to > implement this. > > > What is the easiest way to find out here that the user is in > incognito? > > > > You will have to change callers to supply whether in incognito mode or > not. > > Huh, now I see better why you pushed in the direction of being okay > privacy-wise :| > Actually, my thinking was more that before your change we would not have expired these icons, so if we were ok with privacy wise before than we should be now too. But I understand privacy desires. > Do I get it right that the history > service/backend/databases are shared between the profile and its > incognito variant (so that I cannot pass the incognito bit into the > ctor)? > Indeed. The history service is keyed off the *original* profile. Incognito profiles use the history service of the original profile. > > Is it then always the callers of HistoryService who make sure incognito > is respected? > > Yes. The lookup function takes a ServiceAccessType which hopes to protect again doing the wrong thing. > Adding the incognito bit into whole GetFavicon* API feels really ugly. I > am considering a different approach: > You don't need for all functions, only a couple of key ones, right? Passing an additional argument is hardly that bad. -Scott > - change the factory of FaviconService to create separate instances for > the main profile and for incognito. FaviconService is quite a thin layer > on top of HistoryBackend so it feels okay to me, memory-wise. > - let the last_requested time be updated only from calls from > non-icognito FaviconService. > > WDYT? > > https://codereview.chromium.org/2856873002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> > Adding the incognito bit into whole GetFavicon* API feels really ugly. I > > am considering a different approach: > > > > You don't need for all functions, only a couple of key ones, right? Passing > an additional argument is hardly that bad. Well, that's correct. I could for instance only do it for GetLargestRawFaviconForPageURL which is the only function used by LargeIconService which in turn fetches these favicons from Google. On the other hand, this somehow violates layering to me. (FaviconService / LargeIconService deciding which calls need to update last_requested time). If I wanted to do it fully, all getter functions in FaviconService are affected: GetFaviconImage GetRawFavicon GetFavicon GetFaviconImageForPageURL GetRawFaviconForPageURL GetLargestRawFaviconForPageURL GetFaviconForPageURL GetLargestRawFaviconForID UpdateFaviconMappingsAndFetch These have at least 20 different call sites (according to code search, probably more in reality) Many call sites are again from components where I do now know if I am in incognito so I would need to change a couple of factories. > > - change the factory of FaviconService to create separate instances for > > the main profile and for incognito. FaviconService is quite a thin layer > > on top of HistoryBackend so it feels okay to me, memory-wise. > > - let the last_requested time be updated only from calls from > > non-icognito FaviconService. > > > > WDYT? I am still curious about this proposal, WDYT? > > https://codereview.chromium.org/2856873002/
Jan, I still need to think some more about your questions w.r.t to the SQL statements My personal preference is exposing a method in FaviconService to update the last_requested_time. I think that the caller of LargeIconService::GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() knows best which icons it is necessary to update the "last_requested_time" for in order to prevent them from getting deleted. If there ends up being a lot of callers to GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() we can revisit this decision I think that we will have a visit in the history database for most of the future calls sites of GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() (e.g. iOS bookmarks)
Jan, I still need to think some more about your questions w.r.t to the SQL statements My personal preference is exposing a method in FaviconService to update the last_requested_time. I think that the caller of LargeIconService::GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() knows best which icons it is necessary to update the "last_requested_time" for in order to prevent them from getting deleted. If there ends up being a lot of callers to GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() we can revisit this decision I think that we will have a visit in the history database for most of the future calls sites of GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() (e.g. iOS bookmarks)
https://codereview.chromium.org/2856873002/diff/80001/components/history/core... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/80001/components/history/core... components/history/core/browser/thumbnail_database.cc:447: "icon_mapping.icon_id AND favicon_bitmaps.last_requested<?;")); It we end up setting last_requested for only some rows, you will need to change the WHERE statement to favicon_bitmaps.last_requested < ? AND favicon_bitmaps.last_updated < ? The SQL looks reasonable. I would compare the runtime of |delete_candidates| as one SQL statement vs as multiple SQL statements in order to determine which is better. My guess is that the single SQL statement is better We will likely need to check whether there is a visit associated with the page URL and avoid deleting the favicon if there is one
On Tue, May 16, 2017 at 11:03 AM, <jkrcal@chromium.org> wrote: > > > Adding the incognito bit into whole GetFavicon* API feels really ugly. > I > > > am considering a different approach: > > > > > > > You don't need for all functions, only a couple of key ones, right? > Passing > > an additional argument is hardly that bad. > > Well, that's correct. I could for instance only do it for > GetLargestRawFaviconForPageURL > which is the only function used by LargeIconService which in turn fetches > these > favicons from Google. > > On the other hand, this somehow violates layering to me. > (FaviconService / LargeIconService deciding which calls need to update > last_requested time). > > > If I wanted to do it fully, all getter functions in FaviconService are > affected: > GetFaviconImage > GetRawFavicon > GetFavicon > GetFaviconImageForPageURL > GetRawFaviconForPageURL > GetLargestRawFaviconForPageURL > GetFaviconForPageURL > GetLargestRawFaviconForID > UpdateFaviconMappingsAndFetch > > These have at least 20 different call sites (according to code search, > probably > more in reality) > Many call sites are again from components where I do now know if I am in > incognito so I would need to change a couple of factories. > > > > > - change the factory of FaviconService to create separate instances for > > > the main profile and for incognito. FaviconService is quite a thin > layer > > > on top of HistoryBackend so it feels okay to me, memory-wise. > > > - let the last_requested time be updated only from calls from > > > non-icognito FaviconService. > > > > > > WDYT? > > I am still curious about this proposal, WDYT? > That would make it hard to share any state between the two. So, no, I don't think we should do that. -Scott > > > > https://codereview.chromium.org/2856873002/ > > https://codereview.chromium.org/2856873002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks for the comments! I am not really happy about any of the options :| I looked deeper into the situation and realized that the UI of an incognito tab does not display any favicons, except for the bookmarks bar on desktop. I double-checked that with Chrome privacy and they confirmed that. What do you think about the following? - not storing updates to last_requested for bookmarked favicons and - not using last_requested for bookmarked favicons (as they are never deleted) (sketched in the code, unit-tests not updated) Would you feel comfortable about imposing such a restriction (that the incognito window may pull out only bookmarked favicons from the FaviconService)?
Favicons are unfortunately also shown for incognito tabs: - in the tab strip on desktop - in the tab switcher on Android
On 2017/05/19 18:49:57, pkotwicz wrote: > Favicons are unfortunately also shown for incognito tabs: > - in the tab strip on desktop > - in the tab switcher on Android I was aware of that but I wrongly understood FaviconDriver code: I thought it does not talk to FaviconService in incognito (unless the page is bookmarked). Which is not true. I am now leaning towards the proposal of Peter, with minor changes. I am half-way through implementing it. Quick summary follows: Favicon bitmaps have two modes of deletion. (1) the currently existing mode that deletes favicons along with visits and (2) a new mode for "on-demand" favicon bitmaps. - On demand favicon bitmaps are saved by a special setter (currently called FaviconService::SetLastResortFavicons) that directly marks them as expired and sets their last_requested time to current time. - LargeIconService that stores on-demand favicon bitmaps also updates their last_requested time (not touching normal icons that have last_requested==0). - Whenever a page with an on-demand favicon is visited, the bitmap is deleted and replaced by the real bitmap downloaded directly from the page. The new bitmap is thus not on-demand any more. - ClearOldOnDemandFavicons would then delete only on-demand favicon bitmaps (that have last_requested != 0). This way, I have a guarantee that an on-demand favicon does not have a (recent) visit and I can delete it without checking the visits table. Does it sg?
Description was changed from ========== [Thumbnails DB] Set last_requested time when accessing favicon bitmaps This CL implements setting last_requested time when a bitmap is accessed. The DB field has been available before but was never set in production code. In order to make the API error-proof, the public API for setting last requested time is removed and it is done automatically in each invocation of GetFaviconBitmap(). Furthermore, this CL implements clearing unused favicons (which is the only public API to test that last_requested times are indeed set). Calling this function is left for a further CL. BUG=709471 ========== to ========== [Thumbnails DB] Allow setting last_requested time when accessing favicons. This CL deals with favicons that are added without the user visiting the corresponding page. The CL introduces the notion of "on-demand" favicons for them: 1) The CL reuses (and renames) an existing function in FaviconService for storing such on-demand favicons and pushes this concept further to ThumbnailDatabase. 2) The CL adds a new function to touch (i.e. update last_requested time-stamp) for "on-deman" favicons. The functionality to clear unused on-demand favicons is left for a further CL (https://codereview.chromium.org/2903573002/). Similarly, calling TouchOnDemandFavicon from LargeIconService is also left for a later CL. BUG=709471 ==========
Description was changed from ========== [Thumbnails DB] Allow setting last_requested time when accessing favicons. This CL deals with favicons that are added without the user visiting the corresponding page. The CL introduces the notion of "on-demand" favicons for them: 1) The CL reuses (and renames) an existing function in FaviconService for storing such on-demand favicons and pushes this concept further to ThumbnailDatabase. 2) The CL adds a new function to touch (i.e. update last_requested time-stamp) for "on-deman" favicons. The functionality to clear unused on-demand favicons is left for a further CL (https://codereview.chromium.org/2903573002/). Similarly, calling TouchOnDemandFavicon from LargeIconService is also left for a later CL. BUG=709471 ========== to ========== [Thumbnails DB] Allow setting last_requested time when accessing favicons. This CL deals with favicons that are added without the user visiting the corresponding page. The CL introduces the notion of "on-demand" favicons for them: 1) The CL reuses (and renames) an existing function in FaviconService for storing such on-demand favicons and pushes this concept further to ThumbnailDatabase. 2) The CL adds a new function to touch (i.e. update last_requested time-stamp) for "on-deman" favicons. The functionality to clear unused on-demand favicons is left for a further CL (https://codereview.chromium.org/2903573002/). Similarly, calling TouchOnDemandFavicon from LargeIconService is also left for a later CL. BUG=709471 ==========
I uploaded the next version as promised in a message from yesterday. PTAL! (I've split off a follow-up change into https://codereview.chromium.org/2903573002/) https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... File components/history/core/browser/history_backend.cc (left): https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/history_backend.cc:1853: thumbnail_db_->SetFaviconOutOfDate(icon_id); This operation is now performed implicitly in ThumbnailDatabase::AddOnDemandFaviconBitmap().
I'll take a look at your CL tomorrow. Sorry for the dleay (Monday was a holiday for us)
No worries. No hurry as I am OOO Thu - Fri.
Your CL looks mostly good https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/history_backend.cc:1912: bool on_demand) { Maybe it would be clearer if you passed the |new_last_updated| and the |new_last_requested| times to this function. I think that this would make lines 1949 and 1952 clearer. https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:533: const gfx::Size& pixel_size) { Can you either: - add an enum parameter with whether this is an "on demand bitmap"? - add a parameter for the requested time I am personally not a fan of FillStatementForAddFaviconBitmap() sky@ might have a different opinion though. https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:558: // - keep track of last_requested: last read time is used for cache eviction. If a row has both non-zero |last_updated| and non-zero |last_requested| is it a "standard bitmap" or an "on-demand bitmap"? https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:610: Can you skip the SELECT and go straight to the UPDATE statement? "WHERE url=?" https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:618: time - base::TimeDelta::FromDays(kFaviconUpdateLastRequestedAfterDays); Why the upper bound on |last_requested|? Is this to make the SQL query faster?
https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:533: const gfx::Size& pixel_size) { On 2017/05/24 15:43:31, pkotwicz wrote: > Can you either: > - add an enum parameter with whether this is an "on demand bitmap"? > - add a parameter for the requested time > > I am personally not a fan of FillStatementForAddFaviconBitmap() > > sky@ might have a different opinion though. +1 to what Peter says.
Thanks! Could you please take another look? https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/history_backend.cc:1912: bool on_demand) { On 2017/05/24 15:43:31, pkotwicz wrote: > Maybe it would be clearer if you passed the |new_last_updated| and the > |new_last_requested| times to this function. I think that this would make lines > 1949 and 1952 clearer. This does not work well after merging AddFaviconBitmap with AddOnDemandFaviconBitmap. Do not my new comments clarify it enough? https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:533: const gfx::Size& pixel_size) { On 2017/05/24 18:20:15, sky wrote: > On 2017/05/24 15:43:31, pkotwicz wrote: > > Can you either: > > - add an enum parameter with whether this is an "on demand bitmap"? > > - add a parameter for the requested time > > > > I am personally not a fan of FillStatementForAddFaviconBitmap() > > > > sky@ might have a different opinion though. > > +1 to what Peter says. Done. https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:558: // - keep track of last_requested: last read time is used for cache eviction. On 2017/05/24 15:43:31, pkotwicz wrote: > If a row has both non-zero |last_updated| and non-zero |last_requested| is it a > "standard bitmap" or an "on-demand bitmap"? Oh, very good point. I changed other functions that update last_updated so that they now reset last_requested to 0. Your situation should never happen. I also added a DCHECK in GetFaviconBitmap(). https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:610: On 2017/05/24 15:43:31, pkotwicz wrote: > Can you skip the SELECT and go straight to the UPDATE statement? > > "WHERE url=?" > Not really because "url" is in the favicons table whereas I am updating the favicon_bitmaps table below (and I cannot join two tables in an UPDATE statement). https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:618: time - base::TimeDelta::FromDays(kFaviconUpdateLastRequestedAfterDays); On 2017/05/24 15:43:31, pkotwicz wrote: > Why the upper bound on |last_requested|? Is this to make the SQL query faster? Yes, it is because of performance. I did a small experiment. I run repeated transactions with one call to TouchOnDemandFavicon each: - with this where clause: ~ 10000 transations / second - without this clause: ~ 100 transactions / second
Patchset #11 (id:220001) has been deleted
I'll take a look at your CL tomorrow morning
LGTM with comments https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:610: I see now. https://codereview.chromium.org/2856873002/diff/200001/components/history/cor... components/history/core/browser/thumbnail_database.cc:618: time - base::TimeDelta::FromDays(kFaviconUpdateLastRequestedAfterDays); Thank you for adding the explanation https://codereview.chromium.org/2856873002/diff/260001/components/favicon/cor... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2856873002/diff/260001/components/favicon/cor... components/favicon/core/favicon_service.h:145: // - it is no-op if the bitmaps are not stored using SetOnDemandFavicons(); Nit: ';' -> '.' https://codereview.chromium.org/2856873002/diff/260001/components/favicon/cor... components/favicon/core/favicon_service.h:146: // - the updates of the last requested time have limited frequency for each Nit: add quotes around 'last requested time' https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/history_backend.cc:1915: FaviconBitmapType type) { Please add a comment saying that the this function assumes that SetFaviconBitmaps() is only called with FaviconBitmapType::ON_DEMAND for newly created |icon_id|s. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... File components/history/core/browser/history_service.h (right): https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/history_service.h:785: // - the updates of the last requested time have limited frequency for each Nit: Add quotes around "last requested time" https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... File components/history/core/browser/history_types.h (right): https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/history_types.h:540: // get requested. Nit: get -> were https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:510: << "the favicon " << bitmap_id << " does not have consistent timestamps"; I am not a big fan of DCHECKs in general. The main value of DCHECKs is to check behavior when running unit tests and browser tests https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:541: Can you do this instead: INSERT INTO favicon_bitmaps (icon_id, image_data, last_updated, last_requested, width, height) VALUES (?, ?, ?, ?, ?, ?) statement.BindInt(2, type == ON_VISIT ? time : 0); statement.BindInt(3, type == ON_VISIT ? 0 : time); https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:605: SQL_FROM_HERE, "SELECT id FROM favicons WHERE url=?")); Isn't it theoretically possible to have multiple entries in the favicons table (one for each icon type) for |icon_url|. I think that this function should handle this case https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:620: // are way faster than UPDATEs that really change some data). Nit: Merge this comment with the comment on line 611 https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... File components/history/core/browser/thumbnail_database.h (right): https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.h:134: // - the updates of the last requested time have limited frequency for each Nit: Add quotes around 'last requested time' https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.h:158: // AddFavicon and AddFaviconBitmap (of type FaviconBitmapType::ON_VISIT). Can you please add the type as a parameter? https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... File components/history/core/browser/thumbnail_database_unittest.cc (right): https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:233: base::Time time; Can you please rename this variable to something more meaningful? add_time perhaps? https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:258: base::Time time; Ditto https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:296: base::Time end = start + base::TimeDelta::FromDays(14); You should use kFaviconUpdateLastRequestedAfterDays here. Otherwise, it is confusing why 14 days was chosen https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:303: // Does not mess up with the last_updated field. Nit: 'mess up' -> 'mess' https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:336: ThumbnailDatabase db(NULL); Nit: NULL -> nullptr (here and elsewhere in the new test cases) https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:360: EXPECT_EQ(start, last_updated); // Does not mess up with last_updates. Nits: 'mess up' -> 'mess' 'last_udpates' -> 'last_updated'
Thanks, Peter! Scott, could you please take another look? https://codereview.chromium.org/2856873002/diff/260001/components/favicon/cor... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2856873002/diff/260001/components/favicon/cor... components/favicon/core/favicon_service.h:145: // - it is no-op if the bitmaps are not stored using SetOnDemandFavicons(); On 2017/06/07 17:40:52, pkotwicz wrote: > Nit: ';' -> '.' Done. https://codereview.chromium.org/2856873002/diff/260001/components/favicon/cor... components/favicon/core/favicon_service.h:146: // - the updates of the last requested time have limited frequency for each On 2017/06/07 17:40:52, pkotwicz wrote: > Nit: add quotes around 'last requested time' Done. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/history_backend.cc:1915: FaviconBitmapType type) { On 2017/06/07 17:40:52, pkotwicz wrote: > Please add a comment saying that the this function assumes that > SetFaviconBitmaps() is only called with FaviconBitmapType::ON_DEMAND for newly > created |icon_id|s. Done (in the header). https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... File components/history/core/browser/history_service.h (right): https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/history_service.h:785: // - the updates of the last requested time have limited frequency for each On 2017/06/07 17:40:52, pkotwicz wrote: > Nit: Add quotes around "last requested time" Done. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... File components/history/core/browser/history_types.h (right): https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/history_types.h:540: // get requested. On 2017/06/07 17:40:53, pkotwicz wrote: > Nit: get -> were Done. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:510: << "the favicon " << bitmap_id << " does not have consistent timestamps"; On 2017/06/07 17:40:53, pkotwicz wrote: > I am not a big fan of DCHECKs in general. The main value of DCHECKs is to check > behavior when running unit tests and browser tests Okay, removed (as this is not tested by unit-test, currently). https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:541: On 2017/06/07 17:40:53, pkotwicz wrote: > Can you do this instead: > > INSERT INTO favicon_bitmaps (icon_id, image_data, last_updated, last_requested, > width, height) VALUES (?, ?, ?, ?, ?, ?) > > statement.BindInt(2, type == ON_VISIT ? time : 0); > statement.BindInt(3, type == ON_VISIT ? 0 : time); > Ah, sure, much better. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:605: SQL_FROM_HERE, "SELECT id FROM favicons WHERE url=?")); On 2017/06/07 17:40:53, pkotwicz wrote: > Isn't it theoretically possible to have multiple entries in the favicons table > (one for each icon type) for |icon_url|. I think that this function should > handle this case Done. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.cc:620: // are way faster than UPDATEs that really change some data). On 2017/06/07 17:40:53, pkotwicz wrote: > Nit: Merge this comment with the comment on line 611 Done. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... File components/history/core/browser/thumbnail_database.h (right): https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.h:134: // - the updates of the last requested time have limited frequency for each On 2017/06/07 17:40:53, pkotwicz wrote: > Nit: Add quotes around 'last requested time' Done. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database.h:158: // AddFavicon and AddFaviconBitmap (of type FaviconBitmapType::ON_VISIT). On 2017/06/07 17:40:53, pkotwicz wrote: > Can you please add the type as a parameter? Hm, I wanted to avoid it because it has many call sites. Anyway, I agree that this way it is clearer. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... File components/history/core/browser/thumbnail_database_unittest.cc (right): https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:233: base::Time time; On 2017/06/07 17:40:53, pkotwicz wrote: > Can you please rename this variable to something more meaningful? > > add_time perhaps? Done. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:258: base::Time time; On 2017/06/07 17:40:53, pkotwicz wrote: > Ditto Done. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:296: base::Time end = start + base::TimeDelta::FromDays(14); On 2017/06/07 17:40:53, pkotwicz wrote: > You should use kFaviconUpdateLastRequestedAfterDays here. Otherwise, it is > confusing why 14 days was chosen Done. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:303: // Does not mess up with the last_updated field. On 2017/06/07 17:40:53, pkotwicz wrote: > Nit: 'mess up' -> 'mess' Done. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:336: ThumbnailDatabase db(NULL); On 2017/06/07 17:40:53, pkotwicz wrote: > Nit: NULL -> nullptr (here and elsewhere in the new test cases) Done. https://codereview.chromium.org/2856873002/diff/260001/components/history/cor... components/history/core/browser/thumbnail_database_unittest.cc:360: EXPECT_EQ(start, last_updated); // Does not mess up with last_updates. On 2017/06/07 17:40:53, pkotwicz wrote: > Nits: > 'mess up' -> 'mess' > 'last_udpates' -> 'last_updated' Done.
Still LGTM
https://codereview.chromium.org/2856873002/diff/280001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/280001/components/history/cor... components/history/core/browser/thumbnail_database.cc:609: time - base::TimeDelta::FromDays(kFaviconUpdateLastRequestedAfterDays); Nit: You can move the initialization of |max_time| outside the loop
https://codereview.chromium.org/2856873002/diff/280001/components/history/cor... File components/history/core/browser/thumbnail_database.h (right): https://codereview.chromium.org/2856873002/diff/280001/components/history/cor... components/history/core/browser/thumbnail_database.h:30: // All sooner updates are ignored. Nit: sooner -> earlier
sky@chromium.org changed reviewers: + brettw@chromium.org - sky@chromium.org
I'm about to go on vacation. sky->brettw
On 2017/06/09 23:50:07, sky OOO wrote: > I'm about to go on vacation. sky->brettw Brett, friendly ping!
owners lgtm https://codereview.chromium.org/2856873002/diff/280001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/280001/components/history/cor... components/history/core/browser/thumbnail_database.cc:73: // only used for bitmaps of type ON_DEMAND, for clearing old Can you reword this comment a bit? It reads to me a bit circular where the type == ON_DEMAND tells you that this is used, but this existing tells you that type == ON_DEMAND.
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2856873002/diff/280001/components/history/cor... File components/history/core/browser/thumbnail_database.cc (right): https://codereview.chromium.org/2856873002/diff/280001/components/history/cor... components/history/core/browser/thumbnail_database.cc:73: // only used for bitmaps of type ON_DEMAND, for clearing old On 2017/06/19 19:31:07, brettw wrote: > Can you reword this comment a bit? It reads to me a bit circular where the type > == ON_DEMAND tells you that this is used, but this existing tells you that type > == ON_DEMAND. Done. https://codereview.chromium.org/2856873002/diff/280001/components/history/cor... components/history/core/browser/thumbnail_database.cc:609: time - base::TimeDelta::FromDays(kFaviconUpdateLastRequestedAfterDays); On 2017/06/09 23:47:23, pkotwicz wrote: > Nit: You can move the initialization of |max_time| outside the loop Done. https://codereview.chromium.org/2856873002/diff/280001/components/history/cor... File components/history/core/browser/thumbnail_database.h (right): https://codereview.chromium.org/2856873002/diff/280001/components/history/cor... components/history/core/browser/thumbnail_database.h:30: // All sooner updates are ignored. On 2017/06/09 23:48:27, pkotwicz wrote: > Nit: sooner -> earlier Done.
The CQ bit was unchecked by jkrcal@chromium.org
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2856873002/#ps340001 (title: "Peter's comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1498065654933290,
"parent_rev": "1f51373d28edbd397234c9ee7d60198f0a5e0189", "commit_rev":
"97b537bf1aba5ca6e106920faa65f140dd80df32"}
Message was sent while issue was closed.
Description was changed from ========== [Thumbnails DB] Allow setting last_requested time when accessing favicons. This CL deals with favicons that are added without the user visiting the corresponding page. The CL introduces the notion of "on-demand" favicons for them: 1) The CL reuses (and renames) an existing function in FaviconService for storing such on-demand favicons and pushes this concept further to ThumbnailDatabase. 2) The CL adds a new function to touch (i.e. update last_requested time-stamp) for "on-deman" favicons. The functionality to clear unused on-demand favicons is left for a further CL (https://codereview.chromium.org/2903573002/). Similarly, calling TouchOnDemandFavicon from LargeIconService is also left for a later CL. BUG=709471 ========== to ========== [Thumbnails DB] Allow setting last_requested time when accessing favicons. This CL deals with favicons that are added without the user visiting the corresponding page. The CL introduces the notion of "on-demand" favicons for them: 1) The CL reuses (and renames) an existing function in FaviconService for storing such on-demand favicons and pushes this concept further to ThumbnailDatabase. 2) The CL adds a new function to touch (i.e. update last_requested time-stamp) for "on-deman" favicons. The functionality to clear unused on-demand favicons is left for a further CL (https://codereview.chromium.org/2903573002/). Similarly, calling TouchOnDemandFavicon from LargeIconService is also left for a later CL. BUG=709471 Review-Url: https://codereview.chromium.org/2856873002 Cr-Commit-Position: refs/heads/master@{#481232} Committed: https://chromium.googlesource.com/chromium/src/+/97b537bf1aba5ca6e106920faa65... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:340001) as https://chromium.googlesource.com/chromium/src/+/97b537bf1aba5ca6e106920faa65... |
