Eliminate sending NOTIFICATION_HISTORY_KEYWORD_SEARCH_TERM_UPDATED

Eliminate sending NOTIFICATION_HISTORY_KEYWORD_SEARCH_TERM_UPDATED

Added HistoryKeywordSearchTermUpdated inside HistoryBackendObserver
and HistoryServiceObserver.

Will be removed notification and its detail from chrome_notification_types.

BUG=373321
Committed: https://crrev.com/895a5b1f31ca56353f8a2ec1fcb323cc5d8dfdae
Cr-Commit-Position: refs/heads/master@{#311027}

[Abhishek, 2014-12-23 16:02:33]

abhishek.a21@samsung.com changed reviewers:
+ sdefresne@chromium.org

[Abhishek, 2014-12-24 07:06:23]

PTAL!

[sdefresne, 2014-12-24 08:52:43]

+naiem.shaik: FYI so that you don't duplicate effort

[sdefresne, 2014-12-24 09:03:43]

Thank you. Some comments, but already looking pretty good.

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.cc:1646: if (type ==
chrome::NOTIFICATION_HISTORY_URLS_DELETED) {
DCHECK_EQ(type, chrome::NOTIFICATION_HISTORY_URLS_DELETED);

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.cc:1646: if (type ==
chrome::NOTIFICATION_HISTORY_URLS_DELETED) {
DCHECK_EQ(type, chrome::NOTIFICATION_HISTORY_URLS_DELETED);

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
File chrome/browser/android/provider/chrome_browser_provider.h (right):

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.h:188: void
OnSearchTermUpdated();
style: comment this method

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.h:191: virtual void
OnURLVisited(HistoryService* history_service,
style: method should only have one of "override", "virtual", "final". Please
remove the "virtual" keyword for all method also marked as "override", keeping
just the "override" keyword (also applicable to other methods and files in that
change list).

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.h:196: 
nit: remove empty line

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend.h:146: virtual void
NotifyHistoryKeywordSearchTermUpdated(
style: please comment this method

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.h:618: void
NotifyHistoryKeywordSearchTermUpdated(const history::URLRow& row,
style: please comment this method

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/i...
File chrome/browser/history/in_memory_history_backend.h (right):

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/i...
chrome/browser/history/in_memory_history_backend.h:44: struct
KeywordSearchUpdatedDetails;
KeywordSearchUpdatedDetails is not used anywhere, please remove:
- this forward-declaration,
- type declaration and implementation in
chrome/browser/history/history_notifications.{cc,h}

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/i...
chrome/browser/history/in_memory_history_backend.h:85: 
nit: remove empty line

https://codereview.chromium.org/808123003/diff/20001/components/history/core/...
File components/history/core/browser/history_service_observer.h (right):

https://codereview.chromium.org/808123003/diff/20001/components/history/core/...
components/history/core/browser/history_service_observer.h:52: virtual void
HistoryKeywordSearchTermUpdated(HistoryService* history_service,
style: please comment this method.

[Abhishek, 2014-12-24 19:39:41]

All comments are incorporated in patch set #4.
PTAL!

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.cc:1646: if (type ==
chrome::NOTIFICATION_HISTORY_URLS_DELETED) {
On 2014/12/24 09:03:43, sdefresne wrote:
> DCHECK_EQ(type, chrome::NOTIFICATION_HISTORY_URLS_DELETED);

Done.

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
File chrome/browser/android/provider/chrome_browser_provider.h (right):

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.h:188: void
OnSearchTermUpdated();
On 2014/12/24 09:03:43, sdefresne wrote:
> style: comment this method

Done.

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.h:191: virtual void
OnURLVisited(HistoryService* history_service,
On 2014/12/24 09:03:43, sdefresne wrote:
> style: method should only have one of "override", "virtual", "final". Please
> remove the "virtual" keyword for all method also marked as "override", keeping
> just the "override" keyword (also applicable to other methods and files in
that
> change list).

Done.

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/android/p...
chrome/browser/android/provider/chrome_browser_provider.h:196: 
On 2014/12/24 09:03:43, sdefresne wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_backend.h:146: virtual void
NotifyHistoryKeywordSearchTermUpdated(
On 2014/12/24 09:03:43, sdefresne wrote:
> style: please comment this method

Done.

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/h...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/h...
chrome/browser/history/history_service.h:618: void
NotifyHistoryKeywordSearchTermUpdated(const history::URLRow& row,
On 2014/12/24 09:03:43, sdefresne wrote:
> style: please comment this method

Done.

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/i...
File chrome/browser/history/in_memory_history_backend.h (right):

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/i...
chrome/browser/history/in_memory_history_backend.h:44: struct
KeywordSearchUpdatedDetails;
On 2014/12/24 09:03:43, sdefresne wrote:
> KeywordSearchUpdatedDetails is not used anywhere, please remove:
> - this forward-declaration,
> - type declaration and implementation in
> chrome/browser/history/history_notifications.{cc,h}

Done.

https://codereview.chromium.org/808123003/diff/20001/chrome/browser/history/i...
chrome/browser/history/in_memory_history_backend.h:85: 
On 2014/12/24 09:03:43, sdefresne wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/808123003/diff/20001/components/history/core/...
File components/history/core/browser/history_service_observer.h (right):

https://codereview.chromium.org/808123003/diff/20001/components/history/core/...
components/history/core/browser/history_service_observer.h:52: virtual void
HistoryKeywordSearchTermUpdated(HistoryService* history_service,
On 2014/12/24 09:03:43, sdefresne wrote:
> style: please comment this method.

Done.

[sdefresne, 2014-12-29 09:53:26]

lgtm with nits

https://codereview.chromium.org/808123003/diff/60001/chrome/browser/history/h...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/808123003/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_backend.h:149: virtual void
NotifyHistoryKeywordSearchTermUpdated(
nit: rename to NotifyKeyedSearchTermUpdated (we are in History code, so no need
to repeat ourselves).

https://codereview.chromium.org/808123003/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_backend.h:149: virtual void
NotifyHistoryKeywordSearchTermUpdated(
nit: rename to NotifyKeyedSearchTermUpdated (we are in History code, so no need
to repeat ourselves).

https://codereview.chromium.org/808123003/diff/60001/chrome/browser/history/h...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/808123003/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_service.h:619: // using omni box. |row| contains
the URL information for search |term|.
nit: s/omni box/omnibox/

https://codereview.chromium.org/808123003/diff/60001/components/history/core/...
File components/history/core/browser/history_service_observer.h (right):

https://codereview.chromium.org/808123003/diff/60001/components/history/core/...
components/history/core/browser/history_service_observer.h:56: virtual void
HistoryKeywordSearchTermUpdated(HistoryService* history_service,
nit: rename to OnKeywordSearchTermUpdated() (it is obvious it is coming from
History due to the first parameter).

[sdefresne, 2014-12-29 13:29:29]

https://codereview.chromium.org/808123003/diff/80001/chrome/browser/history/i...
File chrome/browser/history/in_memory_history_backend.cc (right):

https://codereview.chromium.org/808123003/diff/80001/chrome/browser/history/i...
chrome/browser/history/in_memory_history_backend.cc:146: void
InMemoryHistoryBackend::OnKeywordSearchTermUpdated(
I'd suggest removing this method and inlining the code into the other
InMemoryHistoryBackend::OnKeywordSearchTermUpdated():

void InMemoryHistoryBackend::OnKeywordSearchTermUpdated(
    HistoryService* history_service,
    const URLRow& row,
    KeywordID keyword_id,
    const base::string16& term) {
  DCHECK(row.id());
  db_->InsertOrUpdateURLRowByID(row);
  db_->SetKeywordSearchTermsForURL(row.id(), keyword_id, term);
}

[Abhishek, 2014-12-29 14:12:39]

abhishek.a21@samsung.com changed reviewers:
+ newt@chromium.org

[Abhishek, 2014-12-29 14:12:40]

Updated all the comments in patch set #6.

@newt:
PTAL!

https://codereview.chromium.org/808123003/diff/60001/chrome/browser/history/h...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/808123003/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_backend.h:149: virtual void
NotifyHistoryKeywordSearchTermUpdated(
On 2014/12/29 09:53:26, sdefresne wrote:
> nit: rename to NotifyKeyedSearchTermUpdated (we are in History code, so no
need
> to repeat ourselves).

Done.

https://codereview.chromium.org/808123003/diff/60001/chrome/browser/history/h...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/808123003/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_service.h:619: // using omni box. |row| contains
the URL information for search |term|.
On 2014/12/29 09:53:26, sdefresne wrote:
> nit: s/omni box/omnibox/

Done.

https://codereview.chromium.org/808123003/diff/60001/components/history/core/...
File components/history/core/browser/history_service_observer.h (right):

https://codereview.chromium.org/808123003/diff/60001/components/history/core/...
components/history/core/browser/history_service_observer.h:56: virtual void
HistoryKeywordSearchTermUpdated(HistoryService* history_service,
On 2014/12/29 09:53:26, sdefresne wrote:
> nit: rename to OnKeywordSearchTermUpdated() (it is obvious it is coming from
> History due to the first parameter).

Done.

https://codereview.chromium.org/808123003/diff/80001/chrome/browser/history/i...
File chrome/browser/history/in_memory_history_backend.cc (right):

https://codereview.chromium.org/808123003/diff/80001/chrome/browser/history/i...
chrome/browser/history/in_memory_history_backend.cc:146: void
InMemoryHistoryBackend::OnKeywordSearchTermUpdated(
I also felt the same because its creating confusion having 2 function with same
name 1 in public and other one is private. Also 1st is calling to 2nd.

Thanks for the suggestion :)
Done

[sdefresne, 2014-12-29 14:20:50]

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

[sdefresne, 2014-12-29 14:20:50]

still lgtm

+sky for OWNERS of chrome/browser/chrome_notification_types.h

[newt (away), 2014-12-30 16:00:56]

https://codereview.chromium.org/808123003/diff/100001/chrome/browser/android/...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://codereview.chromium.org/808123003/diff/100001/chrome/browser/android/...
chrome/browser/android/provider/chrome_browser_provider.cc:1626: void
ChromeBrowserProvider::OnSearchTermUpdated() {
I'd inline this method into OnKeywordSearchTermUpdated() below.

https://codereview.chromium.org/808123003/diff/100001/chrome/browser/history/...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/808123003/diff/100001/chrome/browser/history/...
chrome/browser/history/history_backend.h:146: // Notify HistoryService that some
keyword has be searched in omni box. The
s/omni box/omnibox

https://codereview.chromium.org/808123003/diff/100001/components/history/core...
File components/history/core/browser/history_service_observer.h (right):

https://codereview.chromium.org/808123003/diff/100001/components/history/core...
components/history/core/browser/history_service_observer.h:52: // Called when
any term searched in omni box.
s/omni box/omnibox

[Abhishek, 2014-12-30 16:20:53]

All comments are incorporated in #7.
PTAL!

https://codereview.chromium.org/808123003/diff/100001/chrome/browser/android/...
File chrome/browser/android/provider/chrome_browser_provider.cc (right):

https://codereview.chromium.org/808123003/diff/100001/chrome/browser/android/...
chrome/browser/android/provider/chrome_browser_provider.cc:1626: void
ChromeBrowserProvider::OnSearchTermUpdated() {
On 2014/12/30 16:00:56, newt wrote:
> I'd inline this method into OnKeywordSearchTermUpdated() below.

Done.

https://codereview.chromium.org/808123003/diff/100001/chrome/browser/history/...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/808123003/diff/100001/chrome/browser/history/...
chrome/browser/history/history_backend.h:146: // Notify HistoryService that some
keyword has be searched in omni box. The
On 2014/12/30 16:00:56, newt wrote:
> s/omni box/omnibox

Done.

https://codereview.chromium.org/808123003/diff/100001/components/history/core...
File components/history/core/browser/history_service_observer.h (right):

https://codereview.chromium.org/808123003/diff/100001/components/history/core...
components/history/core/browser/history_service_observer.h:52: // Called when
any term searched in omni box.
On 2014/12/30 16:00:56, newt wrote:
> s/omni box/omnibox

Done.

[newt (away), 2014-12-30 16:24:09]

chrome/browser/android lgtm

[nshaik, 2014-12-30 18:56:35]

naiem.shaik@gmail.com changed reviewers:
+ naiem.shaik@gmail.com

[nshaik, 2014-12-30 18:56:35]

https://codereview.chromium.org/808123003/diff/60001/chrome/browser/history/h...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/808123003/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_backend.h:146: // Notify HistoryService that some
keyword has be searched in omni box. The
nit: has "to" be searched

https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
chrome/browser/history/history_backend.h:146: // Notify HistoryService that some
keyword has be searched in omnibox. The
nit: has "to" be

https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
chrome/browser/history/history_service.h:618: // Notify all
HistoryServicerObservs registered that keyword have be searched
nit: HistoryServiceObservers

https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
chrome/browser/history/history_service.h:618: // Notify all
HistoryServicerObservs registered that keyword have be searched
nit:keyword has "to" be searched

https://codereview.chromium.org/808123003/diff/120001/components/history/core...
File components/history/core/browser/history_service_observer.h (right):

https://codereview.chromium.org/808123003/diff/120001/components/history/core...
components/history/core/browser/history_service_observer.h:52: // Called when
any term searched in omnibox.
nit: term "is" searched

[Abhishek, 2014-12-31 05:57:22]

@nshaik
Nice catch !
All comments are incorporated in #9.
PTAL !

https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
chrome/browser/history/history_backend.h:146: // Notify HistoryService that some
keyword has be searched in omnibox. The
On 2014/12/30 18:56:35, nshaik wrote:
> nit: has "to" be

Done.

https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
chrome/browser/history/history_service.h:618: // Notify all
HistoryServicerObservs registered that keyword have be searched
On 2014/12/30 18:56:35, nshaik wrote:
> nit: HistoryServiceObservers

Done.

https://codereview.chromium.org/808123003/diff/120001/components/history/core...
File components/history/core/browser/history_service_observer.h (right):

https://codereview.chromium.org/808123003/diff/120001/components/history/core...
components/history/core/browser/history_service_observer.h:52: // Called when
any term searched in omnibox.
On 2014/12/30 18:56:35, nshaik wrote:
> nit: term "is" searched

Done.

[nshaik, 2015-01-05 17:31:38]

On 2014/12/31 05:57:22, Abhishek wrote:
> @nshaik
> Nice catch !
> All comments are incorporated in #9.
> PTAL !
> 
>
https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
> File chrome/browser/history/history_backend.h (right):
> 
>
https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
> chrome/browser/history/history_backend.h:146: // Notify HistoryService that
some
> keyword has be searched in omnibox. The
> On 2014/12/30 18:56:35, nshaik wrote:
> > nit: has "to" be
> 
> Done.
> 
>
https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
> File chrome/browser/history/history_service.h (right):
> 
>
https://codereview.chromium.org/808123003/diff/120001/chrome/browser/history/...
> chrome/browser/history/history_service.h:618: // Notify all
> HistoryServicerObservs registered that keyword have be searched
> On 2014/12/30 18:56:35, nshaik wrote:
> > nit: HistoryServiceObservers
> 
> Done.
> 
>
https://codereview.chromium.org/808123003/diff/120001/components/history/core...
> File components/history/core/browser/history_service_observer.h (right):
> 
>
https://codereview.chromium.org/808123003/diff/120001/components/history/core...
> components/history/core/browser/history_service_observer.h:52: // Called when
> any term searched in omnibox.
> On 2014/12/30 18:56:35, nshaik wrote:
> > nit: term "is" searched
> 
> Done.

lgtm

[sky, 2015-01-05 18:40:07]

LGTM

https://codereview.chromium.org/808123003/diff/160001/chrome/browser/history/...
File chrome/browser/history/history_notifications.h (right):

https://codereview.chromium.org/808123003/diff/160001/chrome/browser/history/...
chrome/browser/history/history_notifications.h:14: #include
"components/history/core/browser/keyword_id.h"
I think you can remove this now.

https://codereview.chromium.org/808123003/diff/160001/components/history/core...
File components/history/core/browser/history_service_observer.h (right):

https://codereview.chromium.org/808123003/diff/160001/components/history/core...
components/history/core/browser/history_service_observer.h:52: // Called when
any term is searched in omnibox.
This isn't necessary correct. It should be "Sent when a keyword search term is
updated."

[Abhishek, 2015-01-05 19:00:43]

All comments are incorporated in #10.
PTAL!

https://codereview.chromium.org/808123003/diff/160001/chrome/browser/history/...
File chrome/browser/history/history_notifications.h (right):

https://codereview.chromium.org/808123003/diff/160001/chrome/browser/history/...
chrome/browser/history/history_notifications.h:14: #include
"components/history/core/browser/keyword_id.h"
On 2015/01/05 18:40:07, sky wrote:
> I think you can remove this now.

Done.

https://codereview.chromium.org/808123003/diff/160001/components/history/core...
File components/history/core/browser/history_service_observer.h (right):

https://codereview.chromium.org/808123003/diff/160001/components/history/core...
components/history/core/browser/history_service_observer.h:52: // Called when
any term is searched in omnibox.
On 2015/01/05 18:40:07, sky wrote:
> This isn't necessary correct. It should be "Sent when a keyword search term is
> updated."

Done.

[sky, 2015-01-05 19:03:31]

SLGTM

https://codereview.chromium.org/808123003/diff/180001/chrome/browser/history/...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/808123003/diff/180001/chrome/browser/history/...
chrome/browser/history/history_service.h:619: // searched using omnibox. |row|
contains the URL information for search
'keyword has been searched using omnibox' -> 'a keyword search term has been
updated'

[Abhishek, 2015-01-05 19:07:41]

The CQ bit was checked by abhishek.a21@samsung.com

[commit-bot: I haz the power, 2015-01-05 19:08:37]

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

[Abhishek, 2015-01-05 19:08:40]

Updated comment.
Thanks !

https://codereview.chromium.org/808123003/diff/180001/chrome/browser/history/...
File chrome/browser/history/history_service.h (right):

https://codereview.chromium.org/808123003/diff/180001/chrome/browser/history/...
chrome/browser/history/history_service.h:619: // searched using omnibox. |row|
contains the URL information for search
On 2015/01/05 19:03:30, sky wrote:
> 'keyword has been searched using omnibox' -> 'a keyword search term has been
> updated'

Done.

[commit-bot: I haz the power, 2015-01-05 19:31:42]

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

[commit-bot: I haz the power, 2015-01-05 19:31:43]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Abhishek, 2015-01-12 08:27:32]

The CQ bit was checked by abhishek.a21@samsung.com

[commit-bot: I haz the power, 2015-01-12 08:28:04]

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

[commit-bot: I haz the power, 2015-01-12 09:24:56]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-01-12 09:25:58]

Patchset 13 (id:??) landed as
https://crrev.com/895a5b1f31ca56353f8a2ec1fcb323cc5d8dfdae
Cr-Commit-Position: refs/heads/master@{#311027}