Clean-up RemoveSuggestion and related codes

Clean-up RemoveSuggestion and related codes

This patch is to clean-up PaswordAutofillManager::RemoveSuggestion
and related codes which is not supported.

BUG=551392
Committed: https://crrev.com/37a9b18f893843bebf55b85c8f541cdd8517c932
Cr-Commit-Position: refs/heads/master@{#358541}

[msu.koo, 2015-11-05 06:19:07]

msu.koo@samsung.com changed reviewers:
+ vabr@chromium.org

[msu.koo, 2015-11-05 06:19:08]

I made a patch for 551392. In this patch I also clean-up the related interface
for RemoveSuggestion.

Please take a look :)

[vabr (Chromium), 2015-11-05 08:39:11]

Thanks for the patch, but this does not lgtm. The bug report is about removing
PasswordAutofillManager::RemoveSuggestion, which should have handled removing
passwords via Shift+Delete. Most of the code you are removing is removing the
non-password autofill version, which removes autocomplete suggestions on
Shift+Delete. The latter is a legitimate feature and needs to keep working.

Also, some general tips for uploading CLs:
 * Please chose reviewers based on OWNERS files rather than on who filed the bug
report.
 * Please do not include commented-out lines of code. Running 'git cl format'
and 'git cl lint' might also be a good idea, although you may have done this
already.
 * When you start working on a bug, post a comment notifying others that you
work on it (or assign it to you if the tracker lets you), otherwise work could
end up duplicated.

Thanks,
Vaclav

[msu.koo, 2015-11-05 23:55:27]

On 2015/11/05 08:39:11, vabr (Chromium) wrote:
> Thanks for the patch, but this does not lgtm. The bug report is about removing
> PasswordAutofillManager::RemoveSuggestion, which should have handled removing
> passwords via Shift+Delete. Most of the code you are removing is removing the
> non-password autofill version, which removes autocomplete suggestions on
> Shift+Delete. The latter is a legitimate feature and needs to keep working.
> 
> Also, some general tips for uploading CLs:
>  * Please chose reviewers based on OWNERS files rather than on who filed the
bug
> report.
>  * Please do not include commented-out lines of code. Running 'git cl format'
> and 'git cl lint' might also be a good idea, although you may have done this
> already.
>  * When you start working on a bug, post a comment notifying others that you
> work on it (or assign it to you if the tracker lets you), otherwise work could
> end up duplicated.
> 
> Thanks,
> Vaclav

Thank you for your comments and tips. I'll address this patch as you guided.

[msu.koo, 2015-11-05 23:55:29]

On 2015/11/05 08:39:11, vabr (Chromium) wrote:
> Thanks for the patch, but this does not lgtm. The bug report is about removing
> PasswordAutofillManager::RemoveSuggestion, which should have handled removing
> passwords via Shift+Delete. Most of the code you are removing is removing the
> non-password autofill version, which removes autocomplete suggestions on
> Shift+Delete. The latter is a legitimate feature and needs to keep working.
> 
> Also, some general tips for uploading CLs:
>  * Please chose reviewers based on OWNERS files rather than on who filed the
bug
> report.
>  * Please do not include commented-out lines of code. Running 'git cl format'
> and 'git cl lint' might also be a good idea, although you may have done this
> already.
>  * When you start working on a bug, post a comment notifying others that you
> work on it (or assign it to you if the tracker lets you), otherwise work could
> end up duplicated.
> 
> Thanks,
> Vaclav

Thank you for your comments and tips. I'll address this patch as you guided.

[msu.koo, 2015-11-06 01:19:59]

msu.koo@samsung.com changed reviewers:
+ vasilii@chromium.org

[msu.koo, 2015-11-06 01:19:59]

Patch uploaded. I checked the call stack to
PasswordAutofillManager::RemoveSuggestion, but there is no special treatment
only for password removing case.
IMHO, it's natural not to introduce special treatment not to call this function,
but just remove NOTIMPLEMENTED and make RemoveSuggestion just return "false" as
it is.
Please take a look and let me know if you have any comments.
Thanks :)

[vabr (Chromium), 2015-11-06 10:57:40]

OK, LGTM.

An alternative to consider would be to change
AutofillPopupDelegate::RemoveSuggestion from a pure virtual method to a virtual
with a default "return false;" implementation. But I do not see any advantages
of that to your current patch, so please go ahead with what you have, once you
address my comment below.

Thanks for your contribution!
Vaclav

https://codereview.chromium.org/1432603007/diff/20001/components/password_man...
File components/password_manager/core/browser/password_autofill_manager.cc
(right):

https://codereview.chromium.org/1432603007/diff/20001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.cc:261:
return false;
Please add a comment referencing http://crbug.com/329038#c15 and stating that
removing password suggestions is not allowed. For example:

// Password suggestions cannot be deleted this way. See
http://crbug.com/329038#c15.

[msu.koo, 2015-11-08 00:14:37]

On 2015/11/06 10:57:40, vabr (Chromium) wrote:
> OK, LGTM.
> 
> An alternative to consider would be to change
> AutofillPopupDelegate::RemoveSuggestion from a pure virtual method to a
virtual
> with a default "return false;" implementation. But I do not see any advantages
> of that to your current patch, so please go ahead with what you have, once you
> address my comment below.
> 
> Thanks for your contribution!
> Vaclav
> 
>
https://codereview.chromium.org/1432603007/diff/20001/components/password_man...
> File components/password_manager/core/browser/password_autofill_manager.cc
> (right):
> 
>
https://codereview.chromium.org/1432603007/diff/20001/components/password_man...
> components/password_manager/core/browser/password_autofill_manager.cc:261:
> return false;
> Please add a comment referencing http://crbug.com/329038#c15 and stating that
> removing password suggestions is not allowed. For example:
> 
> // Password suggestions cannot be deleted this way. See
> http://crbug.com/329038#c15.

Thank you for your review :)
your comments are reflected.

[msu.koo, 2015-11-08 00:14:51]

The CQ bit was checked by msu.koo@samsung.com

[msu.koo, 2015-11-08 00:14:51]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1432603007/#ps40001
(title: "comment reflected")

[commit-bot: I haz the power, 2015-11-08 00:14:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432603007/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432603007/40001

[commit-bot: I haz the power, 2015-11-08 01:00:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-08 01:01:21]

Patchset 3 (id:??) landed as
https://crrev.com/37a9b18f893843bebf55b85c8f541cdd8517c932
Cr-Commit-Position: refs/heads/master@{#358541}