Cleanup FaviconSQLHandler::Update

Cleanup FaviconSQLHandler::Update

Bug=10896006
Test=Try bots
R=michaelbai
TBR=sky

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155491

[pkotwicz, 2012-09-06 14:12:43]

Michael can you please take a look at this CL?
The android_test try bot output for the previous revision is here:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_te...
It doesn't have any failures related to favicons. But I was wondering if you
could verify that I haven't missed anything.
I have rebased in the hope that the (unrelated) test failure will go away.

[pkotwicz, 2012-09-06 19:12:34]

To give some context http://codereview.chromium.org/10802066/ depends on this CL
and the diff http://codereview.chromium.org/10802066/ is based off of this CL.

[michaelbai, 2012-09-07 00:29:59]

http://codereview.chromium.org/10917041/diff/11001/chrome/browser/history/and...
File chrome/browser/history/android/favicon_sql_handler.cc (right):

http://codereview.chromium.org/10917041/diff/11001/chrome/browser/history/and...
chrome/browser/history/android/favicon_sql_handler.cc:63: if
(!thumbnail_db_->DeleteIconMappings(i->url))
This not right, you will delete all the mappings onto this url. Not the one
specified.

[pkotwicz, 2012-09-07 01:07:04]

http://codereview.chromium.org/10917041/diff/11001/chrome/browser/history/and...
File chrome/browser/history/android/favicon_sql_handler.cc (right):

http://codereview.chromium.org/10917041/diff/11001/chrome/browser/history/and...
chrome/browser/history/android/favicon_sql_handler.cc:63: if
(!thumbnail_db_->DeleteIconMappings(i->url))
Is it the case, that in the case of favicon_id != 0, only the favicon mappings
to favicons of type FAVICON should be deleted?

[pkotwicz, 2012-09-07 14:57:57]

I have updated the code in FaviconSQLHandler to what I think it is supposed to
do. Note that this is different from what it currently does now too.

[michaelbai, 2012-09-07 16:55:43]

http://codereview.chromium.org/10917041/diff/14003/chrome/browser/history/and...
File chrome/browser/history/android/favicon_sql_handler.cc (right):

http://codereview.chromium.org/10917041/diff/14003/chrome/browser/history/and...
chrome/browser/history/android/favicon_sql_handler.cc:58: m !=
icon_mappings.end(); ++m) {
In which case, there are multiple icon_mappings returned?

[michaelbai, 2012-09-07 18:19:54]

lgtm

Please wait for the android_tests try bot succeeding before landing it.

[commit-bot: I haz the power, 2012-09-07 19:31:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10917041/17

[commit-bot: I haz the power, 2012-09-07 21:39:39]

Change committed as 155491