Cache SearchEngineType of TemplateURL

Cache SearchEngineType of TemplateURL.

TemplateURLPrepopulateData::GetEngineType() is slow and frequently called even for prepopulated engines. This patch fixes it.

BUG=612369
Committed: https://crrev.com/da6a5018316e1a0912e606bdca9b06dc03ff567d
Cr-Commit-Position: refs/heads/master@{#399752}

[Vitaly Baranov, 2016-05-16 06:18:54]

Patchset #1 (id:1) has been deleted

[Vitaly Baranov, 2016-05-16 06:19:07]

Patchset #1 (id:20001) has been deleted

[Vitaly Baranov, 2016-05-16 06:19:23]

Patchset #1 (id:40001) has been deleted

[Vitaly Baranov, 2016-05-16 06:53:29]

Description was changed from

==========
Optimize TemplateURLPrepopulateData::GetEngineType.
==========

to

==========
Use cache to store SearchEngineType.

TemplateURLPrepopulateData::GetEngineType() is slow and frequently called even
for prepopulated engines. This patch fixes it.

R=pkasting@chromium.org
==========

[Vitaly Baranov, 2016-05-16 06:59:18]

Description was changed from

==========
Use cache to store SearchEngineType.

TemplateURLPrepopulateData::GetEngineType() is slow and frequently called even
for prepopulated engines. This patch fixes it.

R=pkasting@chromium.org
==========

to

==========
Cache SearchEngineType of TemplateURL.

TemplateURLPrepopulateData::GetEngineType() is slow and frequently called even
for prepopulated engines. This patch fixes it.

R=pkasting@chromium.org
==========

[Vitaly Baranov, 2016-05-16 06:59:55]

vitbar@yandex-team.ru changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-05-16 08:49:05]

Is there a bug for this?  Can the bug have some relevant trace or other data
explaining the improvement here?

[Vitaly Baranov, 2016-05-17 03:55:25]

Patchset #2 (id:80001) has been deleted

[Vitaly Baranov, 2016-05-17 03:55:56]

Description was changed from

==========
Cache SearchEngineType of TemplateURL.

TemplateURLPrepopulateData::GetEngineType() is slow and frequently called even
for prepopulated engines. This patch fixes it.

R=pkasting@chromium.org
==========

to

==========
Cache SearchEngineType of TemplateURL.

TemplateURLPrepopulateData::GetEngineType() is slow and frequently called even
for prepopulated engines. This patch fixes it.

BUG=612369

R=pkasting@chromium.org
==========

[Vitaly Baranov, 2016-05-17 04:00:39]

On 2016/05/16 08:49:05, Peter Kasting wrote:
> Is there a bug for this?  Can the bug have some relevant trace or other data
> explaining the improvement here?

I've created issue https://bugs.chromium.org/p/chromium/issues/detail?id=612369

[Peter Kasting, 2016-05-17 04:43:04]

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
File components/search_engines/keyword_table.cc (right):

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
components/search_engines/keyword_table.cc:393: BindURLToStatement(data, &s, 25,
0);  // "24" binds id() as the last item.
Nit: 24 -> 25

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
components/search_engines/template_url.h:647: // Returns the type of the
provided engine, or SEARCH_ENGINE_OTHER if no
"the provided engine" is meaningless since this doesn't take an engine.

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
components/search_engines/template_url.h:756: mutable SearchEngineType
engine_type_;
I think it's confusing for there to be an engine_type_ field in both TemplateURL
and TemplateURLData.  I understand what you're using them for (the "cached" vs.
the "prepopulated, if any" value), but this doesn't really match how the rest of
the TemplateURL[Data] code works.  I think instead, TemplateURLData should have
the only copy of this info.

We shouldn't reset its value in InvalidateCachedValues(), either, since that
gets called for cases that shouldn't actually affect the engine type.  Part of
the issue here is whether it's important that this not be computed except on
demand.  If a single computation is cheap enough, or we ask for engine types
frequently enough that we'll need to compute them for every TemplateURL anyway,
then instead of setting this on demand, we can set it when the computed value
would actually change, e.g. in SetURL() and at construction.

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1075: const
SearchEngineType type = SEARCH_ENGINE_UNKNOWN;
Nit: Inline into next statement

[Vitaly Baranov, 2016-06-03 14:29:01]

Patchset #2 (id:100001) has been deleted

[Vitaly Baranov, 2016-06-03 15:31:45]

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
File components/search_engines/keyword_table.cc (right):

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
components/search_engines/keyword_table.cc:393: BindURLToStatement(data, &s, 25,
0);  // "24" binds id() as the last item.
On 2016/05/17 04:43:04, Peter Kasting wrote:
> Nit: 24 -> 25

Done.

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
File components/search_engines/template_url.h (right):

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
components/search_engines/template_url.h:647: // Returns the type of the
provided engine, or SEARCH_ENGINE_OTHER if no
On 2016/05/17 04:43:04, Peter Kasting wrote:
> "the provided engine" is meaningless since this doesn't take an engine.

Done.

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
components/search_engines/template_url.h:756: mutable SearchEngineType
engine_type_;
On 2016/05/17 04:43:04, Peter Kasting wrote:
> I think it's confusing for there to be an engine_type_ field in both
TemplateURL
> and TemplateURLData.  I understand what you're using them for (the "cached"
vs.
> the "prepopulated, if any" value), but this doesn't really match how the rest
of
> the TemplateURL[Data] code works.  I think instead, TemplateURLData should
have
> the only copy of this info.
Done. And field TemplateURLData::engine_type_ is mutable now to keep
TemplateURL::GetEngineType() a constant function.

> We shouldn't reset its value in InvalidateCachedValues(), either, since that
> gets called for cases that shouldn't actually affect the engine type.
Done.

> If a single computation is cheap enough, or we ask for engine types
> frequently enough that we'll need to compute them for every TemplateURL
anyway,
> then instead of setting this on demand, we can set it when the computed value
> would actually change, e.g. in SetURL() and at construction.
A single computation costs less than a millisecond however we cannot easily
perform it in TemplateURLData::SetURL() because we need SearchTermsData for
this.

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
File components/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/1983773002/diff/60001/components/search_engin...
components/search_engines/template_url_prepopulate_data.cc:1075: const
SearchEngineType type = SEARCH_ENGINE_UNKNOWN;
On 2016/05/17 04:43:04, Peter Kasting wrote:
> Nit: Inline into next statement

Done.

[Peter Kasting, 2016-06-04 01:50:23]

The way this is designed has several problems I think are related.

* You added a setter for TemplateURLData::engine_type_, but the setter doesn't
do anything other than write to the variable; if that's what we really wanted,
we'd want to just avoid the setter/getter and access the field directly as with
most other fields in this object.

* You made this field mutable, but it's directly accessible as external state
for the class, meaning changes to it would violate logical constness.

* Use of the StoreCalculatedEngineType() method is still confusing.  The
TemplateURL caller seems to assume that this is set-once, but it seems to me
that calling SetURL() on the TemplateURLData could violate that.  It also means
users of TemplateURLData must ensure this method has been called correctly
before they access the field, or its value will be wrong, which makes the API
error-prone.

I think all of these get fixed if you move the functionality from
TemplateURL::GetEngineType() into TemplateURLData::GetEngineType(), which will
require that this method have sufficient pointers and such passed in that it can
call back into the TemplateURL to compute the engine type if need be.

In other words, from the perspective of users of TemplateURLData, the underlying
|engine_type_| member is invisible, and all access goes through the
(non-inlined) getter.  This allows TemplateURLData to safely make the underlying
member mutable.  It removes the need for an engine type setter at all; instead,
anything that might affect the engine type, e.g. TemplateURLData::SetURL(), can
just clear the stored engine type, so the next time the getter is called it will
be recomputed.

This also may remove the need to touch the keyword table; because this field is
purely a cached computed value, it's questionable whether the table "should"
store it anyway.  More importantly, the keyword table code probably won't have
the relevant info necessary to pass to GetEngineType() to be able to call it.

[Vitaly Baranov, 2016-06-05 06:12:07]

Done.

[Vitaly Baranov, 2016-06-06 17:41:21]

I've added a new unit-test to template_url_prepopulate_data_unittest.cc because
I guess it's useful to check that
TemplateURLPrepopulateData::GetEngineType(const GURL&) returns correct engine
types for every prepopulated engine.

[Peter Kasting, 2016-06-09 23:00:57]

LGTM with one major change

https://codereview.chromium.org/1983773002/diff/140001/components/search_engi...
File components/search_engines/template_url_data.h (right):

https://codereview.chromium.org/1983773002/diff/140001/components/search_engi...
components/search_engines/template_url_data.h:48: const TemplateURL*
template_url_hint = nullptr) const;
Supplying a null second arg is only used by one test, AFAICT.  Let's just change
that test to construct a TemplateURL and call through that.  Then this can be
named |template_url| and can always be non-null.  This will also simplify the
implementation noticeably.

https://codereview.chromium.org/1983773002/diff/140001/components/search_engi...
components/search_engines/template_url_data.h:135: // Mutable to allow
GetEngineType() to assign it implicitly.
Nit: This is sort of implied by the mutable keyword.  How about: "Caches the
computed engine type across successive calls to GetEngineType()."

https://codereview.chromium.org/1983773002/diff/140001/components/search_engi...
components/search_engines/template_url_data.h:137: };
NIt: While here: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1983773002/diff/140001/components/search_engi...
File components/search_engines/template_url_prepopulate_data.h (right):

https://codereview.chromium.org/1983773002/diff/140001/components/search_engi...
components/search_engines/template_url_prepopulate_data.h:52: std::vector<const
PrepopulatedEngine*> GetAllKnownPrepopulatedEngines();
Nit: I would place this right under GetPrepopulatedEngines() and rework as
follows:

  // Returns all prepopulated engines for all locales.  Used only by tests.
  std::vector<const PrepopulatedEngine*> GetAllPrepopulatedEngines();

[Vitaly Baranov, 2016-06-12 00:18:29]

Patchset #4 (id:160001) has been deleted

[Vitaly Baranov, 2016-06-12 06:59:56]

Patchset #4 (id:180001) has been deleted

[Vitaly Baranov, 2016-06-12 07:15:48]

Patchset #4 (id:200001) has been deleted

[Vitaly Baranov, 2016-06-12 07:29:16]

And I think the engine_type_ field should be moved from TemplateURLData to
TemplateURL because we cannot use TemplateURLData::GetEngineType() without a
TemplateURL currently.

https://codereview.chromium.org/1983773002/diff/140001/components/search_engi...
File components/search_engines/template_url_data.h (right):

https://codereview.chromium.org/1983773002/diff/140001/components/search_engi...
components/search_engines/template_url_data.h:48: const TemplateURL*
template_url_hint = nullptr) const;
On 2016/06/09 23:00:56, Peter Kasting wrote:
> Supplying a null second arg is only used by one test, AFAICT.  Let's just
change
> that test to construct a TemplateURL and call through that.  Then this can be
> named |template_url| and can always be non-null.  This will also simplify the
> implementation noticeably.

Done.

https://codereview.chromium.org/1983773002/diff/140001/components/search_engi...
components/search_engines/template_url_data.h:135: // Mutable to allow
GetEngineType() to assign it implicitly.
On 2016/06/09 23:00:56, Peter Kasting wrote:
> Nit: This is sort of implied by the mutable keyword.  How about: "Caches the
> computed engine type across successive calls to GetEngineType()."

Done.

https://codereview.chromium.org/1983773002/diff/140001/components/search_engi...
components/search_engines/template_url_data.h:137: };
On 2016/06/09 23:00:56, Peter Kasting wrote:
> NIt: While here: DISALLOW_COPY_AND_ASSIGN

TemplateURLData has a copy constructor.

https://codereview.chromium.org/1983773002/diff/140001/components/search_engi...
File components/search_engines/template_url_prepopulate_data.h (right):

https://codereview.chromium.org/1983773002/diff/140001/components/search_engi...
components/search_engines/template_url_prepopulate_data.h:52: std::vector<const
PrepopulatedEngine*> GetAllKnownPrepopulatedEngines();
On 2016/06/09 23:00:56, Peter Kasting wrote:
> Nit: I would place this right under GetPrepopulatedEngines() and rework as
> follows:
> 
>   // Returns all prepopulated engines for all locales.  Used only by tests.
>   std::vector<const PrepopulatedEngine*> GetAllPrepopulatedEngines();

Done.

[Vitaly Baranov, 2016-06-12 07:39:56]

Patchset #5 (id:240001) has been deleted

[Vitaly Baranov, 2016-06-12 08:07:34]

On 2016/06/12 07:29:16, Vitaly Baranov wrote:
> And I think the engine_type_ field should be moved from TemplateURLData to
> TemplateURL because we cannot use TemplateURLData::GetEngineType() without a
> TemplateURL currently.

I've done it in patch set 5. Please take a look.

[Peter Kasting, 2016-06-12 10:42:17]

LGTM

https://codereview.chromium.org/1983773002/diff/260001/components/search_engi...
File components/search_engines/template_url.cc (right):

https://codereview.chromium.org/1983773002/diff/260001/components/search_engi...
components/search_engines/template_url.cc:1346: }
Nit: Shorter:

    engine_type_ = url.is_valid() ?
        TemplateURLPrepopulateData::GetEngineType(url) : SEARCH_ENGINE_OTHER;
    DCHECK_NE(SEARCH_ENGINE_UNKNOWN, engine_type_);

[Vitaly Baranov, 2016-06-13 09:43:35]

https://codereview.chromium.org/1983773002/diff/260001/components/search_engi...
File components/search_engines/template_url.cc (right):

https://codereview.chromium.org/1983773002/diff/260001/components/search_engi...
components/search_engines/template_url.cc:1346: }
On 2016/06/12 10:42:17, Peter Kasting wrote:
> Nit: Shorter:
> 
>     engine_type_ = url.is_valid() ?
>         TemplateURLPrepopulateData::GetEngineType(url) : SEARCH_ENGINE_OTHER;
>     DCHECK_NE(SEARCH_ENGINE_UNKNOWN, engine_type_);

Done.

[Vitaly Baranov, 2016-06-13 09:44:15]

The CQ bit was checked by vitbar@yandex-team.ru

[Vitaly Baranov, 2016-06-13 09:44:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1983773002/#ps280001
(title: "Make code shorter")

[commit-bot: I haz the power, 2016-06-13 09:44:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983773002/280001

[commit-bot: I haz the power, 2016-06-13 09:46:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-13 09:46:17]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Vitaly Baranov, 2016-06-13 11:52:07]

The CQ bit was checked by vitbar@yandex-team.ru

[Vitaly Baranov, 2016-06-13 11:52:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1983773002/#ps300001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-13 11:52:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983773002/300001

[commit-bot: I haz the power, 2016-06-13 11:54:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-13 11:54:38]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[Vitaly Baranov, 2016-06-13 12:44:34]

Patchset #7 (id:300001) has been deleted

[Vitaly Baranov, 2016-06-13 20:50:53]

The CQ bit was checked by vitbar@yandex-team.ru

[Vitaly Baranov, 2016-06-13 20:50:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1983773002/#ps340001
(title: "Remove changes from desktop_search_utils.cc because this file was
removed in master")

[commit-bot: I haz the power, 2016-06-13 20:51:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983773002/340001

[commit-bot: I haz the power, 2016-06-13 20:56:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-13 20:56:31]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Vitaly Baranov, 2016-06-13 21:14:45]

Patchset #8 (id:340001) has been deleted

[Vitaly Baranov, 2016-06-13 21:14:58]

Patchset #7 (id:320001) has been deleted

[Vitaly Baranov, 2016-06-13 21:16:34]

The CQ bit was checked by vitbar@yandex-team.ru

[Vitaly Baranov, 2016-06-13 21:16:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1983773002/#ps360001
(title: "Remove changes from desktop_search_utils.cc because this file was
removed in master")

[commit-bot: I haz the power, 2016-06-13 21:17:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983773002/360001

[commit-bot: I haz the power, 2016-06-13 21:27:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-13 21:27:52]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Vitaly Baranov, 2016-06-14 03:52:44]

The CQ bit was checked by vitbar@yandex-team.ru

[Vitaly Baranov, 2016-06-14 03:52:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1983773002/#ps400001
(title: "Apply changes to popular_sites.cc")

[commit-bot: I haz the power, 2016-06-14 03:52:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983773002/400001

[commit-bot: I haz the power, 2016-06-14 03:54:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-14 03:54:48]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Vitaly Baranov, 2016-06-14 04:00:11]

Patchset #9 (id:400001) has been deleted

[Vitaly Baranov, 2016-06-14 04:03:58]

Patchset #8 (id:380001) has been deleted

[Vitaly Baranov, 2016-06-14 04:07:01]

The CQ bit was checked by vitbar@yandex-team.ru

[Vitaly Baranov, 2016-06-14 04:07:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1983773002/#ps420001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-14 04:07:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983773002/420001

[commit-bot: I haz the power, 2016-06-14 04:08:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-14 04:08:45]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Vitaly Baranov, 2016-06-14 04:27:18]

Patchset #8 (id:420001) has been deleted

[Vitaly Baranov, 2016-06-14 04:27:30]

Patchset #7 (id:360001) has been deleted

[Vitaly Baranov, 2016-06-14 04:41:16]

The CQ bit was checked by vitbar@yandex-team.ru

[Vitaly Baranov, 2016-06-14 04:41:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1983773002/#ps460001
(title: "Apply changes to popular_sites.cc")

[commit-bot: I haz the power, 2016-06-14 04:41:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983773002/460001

[commit-bot: I haz the power, 2016-06-14 04:49:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-14 04:49:03]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Vitaly Baranov, 2016-06-14 05:27:33]

vitbar@yandex-team.ru changed reviewers:
+ thestig@chromium.org, treib@chromium.org

[Vitaly Baranov, 2016-06-14 05:28:09]

Description was changed from

==========
Cache SearchEngineType of TemplateURL.

TemplateURLPrepopulateData::GetEngineType() is slow and frequently called even
for prepopulated engines. This patch fixes it.

BUG=612369

R=pkasting@chromium.org
==========

to

==========
Cache SearchEngineType of TemplateURL.

TemplateURLPrepopulateData::GetEngineType() is slow and frequently called even
for prepopulated engines. This patch fixes it.

BUG=612369
==========

[Vitaly Baranov, 2016-06-14 05:34:55]

treib@chromium.org: Please review changes in
components/ntp_tiles/popular_sites.cc

thestig@chromium.org: Please review changes in
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
                                              
chrome/browser/search/local_ntp_source.cc

[Vitaly Baranov, 2016-06-14 05:38:22]

The CQ bit was checked by vitbar@yandex-team.ru

[Vitaly Baranov, 2016-06-14 05:38:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1983773002/#ps480001
(title: "Fix typo")

[Vitaly Baranov, 2016-06-14 05:38:37]

The CQ bit was unchecked by vitbar@yandex-team.ru

[commit-bot: I haz the power, 2016-06-14 05:38:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983773002/480001

[Marc Treib, 2016-06-14 09:50:00]

local_ntp_source.cc and popular_sites.cc LGTM

[Lei Zhang, 2016-06-14 17:26:06]

On 2016/06/14 05:34:55, Vitaly Baranov wrote:
> mailto:thestig@chromium.org: Please review changes in
> chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
>                                               
> chrome/browser/search/local_ntp_source.cc

lgtm

[Vitaly Baranov, 2016-06-14 17:27:44]

The CQ bit was checked by vitbar@yandex-team.ru

[commit-bot: I haz the power, 2016-06-14 17:28:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1983773002/480001

[commit-bot: I haz the power, 2016-06-14 18:20:50]

Description was changed from

==========
Cache SearchEngineType of TemplateURL.

TemplateURLPrepopulateData::GetEngineType() is slow and frequently called even
for prepopulated engines. This patch fixes it.

BUG=612369
==========

to

==========
Cache SearchEngineType of TemplateURL.

TemplateURLPrepopulateData::GetEngineType() is slow and frequently called even
for prepopulated engines. This patch fixes it.

BUG=612369
==========

[commit-bot: I haz the power, 2016-06-14 18:20:53]

Committed patchset #9 (id:480001)

[commit-bot: I haz the power, 2016-06-14 18:20:59]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-14 18:23:44]

Description was changed from

==========
Cache SearchEngineType of TemplateURL.

TemplateURLPrepopulateData::GetEngineType() is slow and frequently called even
for prepopulated engines. This patch fixes it.

BUG=612369
==========

to

==========
Cache SearchEngineType of TemplateURL.

TemplateURLPrepopulateData::GetEngineType() is slow and frequently called even
for prepopulated engines. This patch fixes it.

BUG=612369

Committed: https://crrev.com/da6a5018316e1a0912e606bdca9b06dc03ff567d
Cr-Commit-Position: refs/heads/master@{#399752}
==========

[commit-bot: I haz the power, 2016-06-14 18:23:46]

Patchset 9 (id:??) landed as
https://crrev.com/da6a5018316e1a0912e606bdca9b06dc03ff567d
Cr-Commit-Position: refs/heads/master@{#399752}

[Alexander Yashkin, 2016-06-15 06:55:22]

a-v-y@yandex-team.ru changed reviewers:
+ a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-06-15 06:55:23]

https://codereview.chromium.org/1983773002/diff/480001/components/search_engi...
File components/search_engines/prepopulated_engines.json (left):

https://codereview.chromium.org/1983773002/diff/480001/components/search_engi...
components/search_engines/prepopulated_engines.json:33: "kCurrentDataVersion":
91
kCurrentDataVersion becomes 90 with previous version of 91?
I beleive its a bug.

[Peter Kasting, 2016-06-15 09:18:12]

https://codereview.chromium.org/1983773002/diff/480001/components/search_engi...
File components/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/1983773002/diff/480001/components/search_engi...
components/search_engines/prepopulated_engines.json:33: "kCurrentDataVersion":
90
Yeah, this is some kind of bad merge conflict resolution.  Please bump to 92.

[Vitaly Baranov, 2016-06-15 11:16:34]

On 2016/06/15 09:18:12, Peter Kasting wrote:
>
https://codereview.chromium.org/1983773002/diff/480001/components/search_engi...
> File components/search_engines/prepopulated_engines.json (right):
> 
>
https://codereview.chromium.org/1983773002/diff/480001/components/search_engi...
> components/search_engines/prepopulated_engines.json:33: "kCurrentDataVersion":
> 90
> Yeah, this is some kind of bad merge conflict resolution.  Please bump to 92.

Yeah, it was bad rebase. However why do you suggest bumping it to 92? The final
version of this CL doesn't make any changes in the database and this CL never
changes the prepopulated engines themselves. Therefore I guess I should revert
kCurrentDataVersion just to its original value, which is 91.

[Vitaly Baranov, 2016-06-15 11:18:28]

On 2016/06/15 11:16:34, Vitaly Baranov wrote:
> On 2016/06/15 09:18:12, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/1983773002/diff/480001/components/search_engi...
> > File components/search_engines/prepopulated_engines.json (right):
> > 
> >
>
https://codereview.chromium.org/1983773002/diff/480001/components/search_engi...
> > components/search_engines/prepopulated_engines.json:33:
"kCurrentDataVersion":
> > 90
> > Yeah, this is some kind of bad merge conflict resolution.  Please bump to
92.
> 
> Yeah, it was bad rebase. However why do you suggest bumping it to 92? The
final
> version of this CL doesn't make any changes in the database and this CL never
> changes the prepopulated engines themselves. Therefore I guess I should revert
> kCurrentDataVersion just to its original value, which is 91.

https://codereview.chromium.org/2065163008/