Extend HistoryService with support for favicons from Google servers

Extend HistoryService with support for favicons from Google servers

We plan to extend LargeIconService with a feature that will, in certain
cases like server-side suggestions, fetch favicons from Google servers,
that is, for pages that haven't been visited locally. Design Doc
in go/favicon-pe-google-server.

An API extension in HistoryService/backend is needed because we want
to be very conservative and support an atomic verify-empty-and-write:
the remotely fetched data should not overwrite data in the local
database in any case (e.g. the local database could have changed
during the request to Google servers).

These icons should also be marked initially expired such that, if the
user visits the page in the future, new favicons will be fetched and
written to the database (overriding previously fetched icons, which
could theoretically differ, e.g. if the site serves different
favicons depending on agent info).

BUG=644102
Review-Url: https://codereview.chromium.org/2750313002
Cr-Commit-Position: refs/heads/master@{#458359}
Committed: https://chromium.googlesource.com/chromium/src/+/5d0381faf29ba02bec52363bb62c0237cfd4a314

[mastiz, 2017-03-16 09:33:16]

Description was changed from

==========
Extend HistoryService with support for favicons from Google servers

We plan to extend LargeIconService with a feature that will, in certain
cases like server-side suggestions, fetch favicons from Google servers,
that is, for pages that haven't been visited locally.

An API extension in HistoryService/backend is needed because we want
to be very conservative and support an atomic verify-empty-and-write:
the remotely fetched data should not overwrite data in the local
database in any case (e.g. the local database could have changed
during the request to Google servers).

These icons should also be marked initially expired such that, if the
user visits the page in the future, new favicons will be fetched and
written to the database (overriding previously fetched icons, which
could theoretically differ, e.g. if the site serves different
favicons depending on agent info).

BUG=644102
==========

to

==========
Extend HistoryService with support for favicons from Google servers

We plan to extend LargeIconService with a feature that will, in certain
cases like server-side suggestions, fetch favicons from Google servers,
that is, for pages that haven't been visited locally. Design Doc
in go/favicon-pe-google-server.

An API extension in HistoryService/backend is needed because we want
to be very conservative and support an atomic verify-empty-and-write:
the remotely fetched data should not overwrite data in the local
database in any case (e.g. the local database could have changed
during the request to Google servers).

These icons should also be marked initially expired such that, if the
user visits the page in the future, new favicons will be fetched and
written to the database (overriding previously fetched icons, which
could theoretically differ, e.g. if the site serves different
favicons depending on agent info).

BUG=644102
==========

[mastiz, 2017-03-16 09:35:40]

mastiz@chromium.org changed reviewers:
+ sky@chromium.org

[mastiz, 2017-03-16 09:35:40]

sky@: I believe you should have sufficient context information about what we're
trying to do, let me know otherwise (or take a look at the linked design doc).

I split this change from the actual code that uses it, see dependent patch if
you want more details, thx.

[sky, 2017-03-16 16:13:25]

sky@chromium.org changed reviewers:
+ brettw@chromium.org
- sky@chromium.org

[sky, 2017-03-16 16:14:04]

sky->brettw (I have too much going on and am handing most history reviews to
Brett these days).

[mastiz, 2017-03-17 14:10:07]

On 2017/03/16 16:14:04, sky wrote:
> sky->brettw (I have too much going on and am handing most history reviews to
> Brett these days).

Friendly ping for brettw@, thanks!

[mastiz, 2017-03-17 14:10:16]

On 2017/03/16 16:14:04, sky wrote:
> sky->brettw (I have too much going on and am handing most history reviews to
> Brett these days).

Friendly ping for brettw@, thanks!

[mastiz, 2017-03-17 14:10:21]

On 2017/03/16 16:14:04, sky wrote:
> sky->brettw (I have too much going on and am handing most history reviews to
> Brett these days).

Friendly ping for brettw@, thanks!

[mastiz, 2017-03-17 14:10:27]

On 2017/03/16 16:14:04, sky wrote:
> sky->brettw (I have too much going on and am handing most history reviews to
> Brett these days).

Friendly ping for brettw@, thanks!

[mastiz, 2017-03-20 08:39:15]

brettw@: friendly ping, thanks! (btw, apologies for the accidental repeated
messages earlier)

[brettw, 2017-03-20 17:30:04]

Sorry, I'm super backed up and you're not even the person waiting the longest :(
I should get back to you this afternoon.

[brettw, 2017-03-20 21:37:31]

lgtm

https://codereview.chromium.org/2750313002/diff/1/components/history/core/bro...
File components/history/core/browser/history_service.cc (right):

https://codereview.chromium.org/2750313002/diff/1/components/history/core/bro...
components/history/core/browser/history_service.cc:628: void
HistoryService::SetExpiredFaviconsIfNoneKnown(
This name is hard to make sense of without understanding the details of this
change. How about "SetLastResortFavicons"? Then the whole behavior of setting
the expired bit can be an implementation detail of how a "last resort" favicon
is defined.

[mastiz, 2017-03-21 08:27:43]

On 2017/03/20 21:37:31, brettw (plz ping after 24h) wrote:
> lgtm
> 
>
https://codereview.chromium.org/2750313002/diff/1/components/history/core/bro...
> File components/history/core/browser/history_service.cc (right):
> 
>
https://codereview.chromium.org/2750313002/diff/1/components/history/core/bro...
> components/history/core/browser/history_service.cc:628: void
> HistoryService::SetExpiredFaviconsIfNoneKnown(
> This name is hard to make sense of without understanding the details of this
> change. How about "SetLastResortFavicons"? Then the whole behavior of setting
> the expired bit can be an implementation detail of how a "last resort" favicon
> is defined.

Done, thanks!

[mastiz, 2017-03-21 08:28:39]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2017-03-21 08:28:39]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2750313002/#ps20001
(title: "Rename function.")

[commit-bot: I haz the power, 2017-03-21 08:28:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 09:47:16]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490084919360190,
"parent_rev": "f06a6214de1edd9f5fa1e3e3ed48adadfd80943b", "commit_rev":
"5d0381faf29ba02bec52363bb62c0237cfd4a314"}

[commit-bot: I haz the power, 2017-03-21 09:48:00]

Description was changed from

==========
Extend HistoryService with support for favicons from Google servers

We plan to extend LargeIconService with a feature that will, in certain
cases like server-side suggestions, fetch favicons from Google servers,
that is, for pages that haven't been visited locally. Design Doc
in go/favicon-pe-google-server.

An API extension in HistoryService/backend is needed because we want
to be very conservative and support an atomic verify-empty-and-write:
the remotely fetched data should not overwrite data in the local
database in any case (e.g. the local database could have changed
during the request to Google servers).

These icons should also be marked initially expired such that, if the
user visits the page in the future, new favicons will be fetched and
written to the database (overriding previously fetched icons, which
could theoretically differ, e.g. if the site serves different
favicons depending on agent info).

BUG=644102
==========

to

==========
Extend HistoryService with support for favicons from Google servers

We plan to extend LargeIconService with a feature that will, in certain
cases like server-side suggestions, fetch favicons from Google servers,
that is, for pages that haven't been visited locally. Design Doc
in go/favicon-pe-google-server.

An API extension in HistoryService/backend is needed because we want
to be very conservative and support an atomic verify-empty-and-write:
the remotely fetched data should not overwrite data in the local
database in any case (e.g. the local database could have changed
during the request to Google servers).

These icons should also be marked initially expired such that, if the
user visits the page in the future, new favicons will be fetched and
written to the database (overriding previously fetched icons, which
could theoretically differ, e.g. if the site serves different
favicons depending on agent info).

BUG=644102

Review-Url: https://codereview.chromium.org/2750313002
Cr-Commit-Position: refs/heads/master@{#458359}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d0381faf29ba02bec52363bb62c...
==========

[commit-bot: I haz the power, 2017-03-21 09:48:01]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5d0381faf29ba02bec52363bb62c...