Fix a DCHECK failure in AutocompleteEditModel::OnPopupDataChanged()

Fix a DCHECK failure in AutocompleteEditModel::OnPopupDataChanged()

BUG=70343
TEST=See bug reports.

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

[James Su, 2011-01-21 17:48:51]

I'm still trying to figure out a better solution that doesn't need to add the
extra parameter and covers all corner cases. This CL currently can't handle the
default match being deleted case.

[Peter Kasting, 2011-01-21 18:27:47]

I replied on the bug with some thoughts about what we should be doing here, see
if it makes sense.

[James Su, 2011-01-22 07:58:05]

New CL should handle all corner cases properly.

[Peter Kasting, 2011-01-24 23:25:52]

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:609: 
Nit: Unnecessary blank line

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:616: // |has_temporary_text_|
is true here means we have been reverted to the
Nit: How about:

  // If |has_temporary_text_| is true, then we previously had a manual selection
  // but now don't (or |destination_for_temporary_text_change| would have been
  // non-NULL).  This can happen when deleting the selected item in the popup.
  // In this case, we've already reverted the popup to the default match, so we
  // need to revert ourselves as well.

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:622: has_temporary_text_ =
false;
Let's refactor a little:

[private] void RevertTemporaryText() {
  just_deleted_text_ = false;
  has_temporary_text_ = false;
  view_->OnRevertTemporaryText();
}

Then call this function here as well as in OnEscapeKeyPressed() (which will
still also need to call popup_->ResetToDefaultMatch()).

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:299: if
(!result.empty()) {
If I'm not mistaken, we can change this conditional to be more restrictive:

    if (!result.empty() && (selected_line_ != selected_line)) {

...because if the two are equal, then we're in one of two cases:

(1) Deleting the match didn't cause any change to our results at all, so the
controller never notified anyone of any changes.  In this case, nothing has
changed so it's safe to take no further action.

(2) We deleted the match and called the various notifications and thus have
switched to the new result set's default match.  It turns out this match is the
one we want to select anyway, so since we already have the right match selected,
we can safely do nothing further.

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:300: // Set
|selected_line_| to kNoMatch to forcely select the line, even if
I think if we change the conditional as I note above, then we don't need this
change or any of the modifications in SetSelectedLine().

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:327: // reverted to the
default match.
Nit: I don't think this comment is necessary; it's clear we need to keep these
two variables in sync.

[James Su, 2011-01-25 02:24:55]

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:609: 
On 2011/01/24 23:25:52, Peter Kasting wrote:
> Nit: Unnecessary blank line

Done.

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:616: // |has_temporary_text_|
is true here means we have been reverted to the
On 2011/01/24 23:25:52, Peter Kasting wrote:
> Nit: How about:
> 
>   // If |has_temporary_text_| is true, then we previously had a manual
selection
>   // but now don't (or |destination_for_temporary_text_change| would have been
>   // non-NULL).  This can happen when deleting the selected item in the popup.
>   // In this case, we've already reverted the popup to the default match, so
we
>   // need to revert ourselves as well.

Done.

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:622: has_temporary_text_ =
false;
On 2011/01/24 23:25:52, Peter Kasting wrote:
> Let's refactor a little:
> 
> [private] void RevertTemporaryText() {
>   just_deleted_text_ = false;
>   has_temporary_text_ = false;
>   view_->OnRevertTemporaryText();
> }
> 
> Then call this function here as well as in OnEscapeKeyPressed() (which will
> still also need to call popup_->ResetToDefaultMatch()).

It's actually not possible. In OnEscapeKeyPressed(), we need to call
popup_->ResetToDefaultMatch() between has_temporary_text_ = false and
view_->OnRevertTemporaryText().

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:299: if
(!result.empty()) {
On 2011/01/24 23:25:52, Peter Kasting wrote:
> If I'm not mistaken, we can change this conditional to be more restrictive:
> 
>     if (!result.empty() && (selected_line_ != selected_line)) {
> 
> ...because if the two are equal, then we're in one of two cases:
> 
> (1) Deleting the match didn't cause any change to our results at all, so the
> controller never notified anyone of any changes.  In this case, nothing has
> changed so it's safe to take no further action.
> 
> (2) We deleted the match and called the various notifications and thus have
> switched to the new result set's default match.  It turns out this match is
the
> one we want to select anyway, so since we already have the right match
selected,
> we can safely do nothing further.

It has an issue for the deleting default match case. If we don't do the update
here, when the default match gets deleted, the new default match may be selected
as if it's reverted from a temporary text, i.e. the user text will be reverted
to the original user typed text, rather than being selected as a new temporary
text. I think we should stay in temporary text mode in this case, as the new
default match is actually selected manually.

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:300: // Set
|selected_line_| to kNoMatch to forcely select the line, even if
On 2011/01/24 23:25:52, Peter Kasting wrote:
> I think if we change the conditional as I note above, then we don't need this
> change or any of the modifications in SetSelectedLine().
See above.

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:327: // reverted to the
default match.
On 2011/01/24 23:25:52, Peter Kasting wrote:
> Nit: I don't think this comment is necessary; it's clear we need to keep these
> two variables in sync.

Done.

[Peter Kasting, 2011-01-25 02:36:20]

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:622: has_temporary_text_ =
false;
On 2011/01/25 02:24:55, James Su wrote:
> On 2011/01/24 23:25:52, Peter Kasting wrote:
> > Let's refactor a little:
> > 
> > [private] void RevertTemporaryText() {
> >   just_deleted_text_ = false;
> >   has_temporary_text_ = false;
> >   view_->OnRevertTemporaryText();
> > }
> > 
> > Then call this function here as well as in OnEscapeKeyPressed() (which will
> > still also need to call popup_->ResetToDefaultMatch()).
> 
> It's actually not possible. In OnEscapeKeyPressed(), we need to call
> popup_->ResetToDefaultMatch() between has_temporary_text_ = false and
> view_->OnRevertTemporaryText().

In that case, let's refactor and make that call a conditional part of the
refactored function.

I am very keen to have places that are doing "the same thing" actually run
through the same code.

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:299: if
(!result.empty()) {
On 2011/01/25 02:24:55, James Su wrote:
> On 2011/01/24 23:25:52, Peter Kasting wrote:
> > If I'm not mistaken, we can change this conditional to be more restrictive:
> > 
> >     if (!result.empty() && (selected_line_ != selected_line)) {
> > 
> > ...because if the two are equal, then we're in one of two cases:
> > 
> > (1) Deleting the match didn't cause any change to our results at all, so the
> > controller never notified anyone of any changes.  In this case, nothing has
> > changed so it's safe to take no further action.
> > 
> > (2) We deleted the match and called the various notifications and thus have
> > switched to the new result set's default match.  It turns out this match is
> the
> > one we want to select anyway, so since we already have the right match
> selected,
> > we can safely do nothing further.
> 
> It has an issue for the deleting default match case. If we don't do the update
> here, when the default match gets deleted, the new default match may be
selected
> as if it's reverted from a temporary text, i.e. the user text will be reverted
> to the original user typed text, rather than being selected as a new temporary
> text. I think we should stay in temporary text mode in this case, as the new
> default match is actually selected manually.

But your code has an opposite problem: If we don't have a manually selected
match already, and calling DeleteMatch() does nothing to the result set, then
the user's text is changed to the temporary text (as if we'd arrowed down and
back up).

Perhaps we should make AutocompleteController::DeleteMatch() return a bool to
indicate whether it did anything, and use that to avoid this case.  It can do
this by storing the value of |updated_latest_result_| before it calls
CommitResult().

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:300: // Set
|selected_line_| to kNoMatch to forcely select the line, even if
On 2011/01/25 02:24:55, James Su wrote:
> On 2011/01/24 23:25:52, Peter Kasting wrote:
> > I think if we change the conditional as I note above, then we don't need
this
> > change or any of the modifications in SetSelectedLine().
> See above.

OK, but I still think this can be done more cleanly.  Instead of this statement
or any of your current modifications to SetSelectedLine(), you can simply add a
third arg, "bool force_update_even_if_same_line", set it true here and false
elsewhere, and change "if (line == selected_line_)" in SetSelectedLine() to "if
(!force_update_even_if_same_line && (line == selected_line_))".  This should
work because Observe() has ensures that |selected_line_| is set correctly to the
new default match and thus we don't need special-case code in SetSelectedLine()
to get the right destination for this case.

[James Su, 2011-01-25 02:51:51]

I'll update this CL after dinner.

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:622: has_temporary_text_ =
false;
On 2011/01/25 02:36:20, Peter Kasting wrote:
> On 2011/01/25 02:24:55, James Su wrote:
> > On 2011/01/24 23:25:52, Peter Kasting wrote:
> > > Let's refactor a little:
> > > 
> > > [private] void RevertTemporaryText() {
> > >   just_deleted_text_ = false;
> > >   has_temporary_text_ = false;
> > >   view_->OnRevertTemporaryText();
> > > }
> > > 
> > > Then call this function here as well as in OnEscapeKeyPressed() (which
will
> > > still also need to call popup_->ResetToDefaultMatch()).
> > 
> > It's actually not possible. In OnEscapeKeyPressed(), we need to call
> > popup_->ResetToDefaultMatch() between has_temporary_text_ = false and
> > view_->OnRevertTemporaryText().
> 
> In that case, let's refactor and make that call a conditional part of the
> refactored function.
> 
> I am very keen to have places that are doing "the same thing" actually run
> through the same code.
Ok, I'll update it asap.

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:299: if
(!result.empty()) {
On 2011/01/25 02:36:20, Peter Kasting wrote:
> On 2011/01/25 02:24:55, James Su wrote:
> > On 2011/01/24 23:25:52, Peter Kasting wrote:
> > > If I'm not mistaken, we can change this conditional to be more
restrictive:
> > > 
> > >     if (!result.empty() && (selected_line_ != selected_line)) {
> > > 
> > > ...because if the two are equal, then we're in one of two cases:
> > > 
> > > (1) Deleting the match didn't cause any change to our results at all, so
the
> > > controller never notified anyone of any changes.  In this case, nothing
has
> > > changed so it's safe to take no further action.
> > > 
> > > (2) We deleted the match and called the various notifications and thus
have
> > > switched to the new result set's default match.  It turns out this match
is
> > the
> > > one we want to select anyway, so since we already have the right match
> > selected,
> > > we can safely do nothing further.
> > 
> > It has an issue for the deleting default match case. If we don't do the
update
> > here, when the default match gets deleted, the new default match may be
> selected
> > as if it's reverted from a temporary text, i.e. the user text will be
reverted
> > to the original user typed text, rather than being selected as a new
temporary
> > text. I think we should stay in temporary text mode in this case, as the new
> > default match is actually selected manually.
> 
> But your code has an opposite problem: If we don't have a manually selected
> match already, and calling DeleteMatch() does nothing to the result set, then
> the user's text is changed to the temporary text (as if we'd arrowed down and
> back up).
> 
> Perhaps we should make AutocompleteController::DeleteMatch() return a bool to
> indicate whether it did anything, and use that to avoid this case.  It can do
> this by storing the value of |updated_latest_result_| before it calls
> CommitResult().
Isn't that guaranteed by above if (match.deletable) block? I'll do this change
if it's possible that deleting a deletable item may fail.

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:300: // Set
|selected_line_| to kNoMatch to forcely select the line, even if
On 2011/01/25 02:36:20, Peter Kasting wrote:
> On 2011/01/25 02:24:55, James Su wrote:
> > On 2011/01/24 23:25:52, Peter Kasting wrote:
> > > I think if we change the conditional as I note above, then we don't need
> this
> > > change or any of the modifications in SetSelectedLine().
> > See above.
> 
> OK, but I still think this can be done more cleanly.  Instead of this
statement
> or any of your current modifications to SetSelectedLine(), you can simply add
a
> third arg, "bool force_update_even_if_same_line", set it true here and false
> elsewhere, and change "if (line == selected_line_)" in SetSelectedLine() to
"if
> (!force_update_even_if_same_line && (line == selected_line_))".  This should
> work because Observe() has ensures that |selected_line_| is set correctly to
the
> new default match and thus we don't need special-case code in
SetSelectedLine()
> to get the right destination for this case.
Ok I'll do it. It's actually my original approach. I'm just hesitated to add
another bool parameter to this method :)

[Peter Kasting, 2011-01-25 04:23:20]

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:299: if
(!result.empty()) {
On 2011/01/25 02:51:51, James Su wrote:
> On 2011/01/25 02:36:20, Peter Kasting wrote:
> > But your code has an opposite problem: If we don't have a manually selected
> > match already, and calling DeleteMatch() does nothing to the result set,
then
> > the user's text is changed to the temporary text (as if we'd arrowed down
and
> > back up).
> > 
> > Perhaps we should make AutocompleteController::DeleteMatch() return a bool
to
> > indicate whether it did anything, and use that to avoid this case.  It can
do
> > this by storing the value of |updated_latest_result_| before it calls
> > CommitResult().
>
> Isn't that guaranteed by above if (match.deletable) block? I'll do this change
> if it's possible that deleting a deletable item may fail.

Why don't we do this: Add a DCHECK(updated_latest_result_) into DeleteMatch()
before the controller calls CommitResult() and don't bother adding a return
value.

Semantically, providers are _supposed_ to do something to force an update when
they delete a match, but right now we don't enforce that, so that's basically
what the DCHECK asserts.

[James Su, 2011-01-25 05:30:11]

CL has been updated.

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/6340012/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:299: if
(!result.empty()) {
On 2011/01/25 04:23:20, Peter Kasting wrote:
> On 2011/01/25 02:51:51, James Su wrote:
> > On 2011/01/25 02:36:20, Peter Kasting wrote:
> > > But your code has an opposite problem: If we don't have a manually
selected
> > > match already, and calling DeleteMatch() does nothing to the result set,
> then
> > > the user's text is changed to the temporary text (as if we'd arrowed down
> and
> > > back up).
> > > 
> > > Perhaps we should make AutocompleteController::DeleteMatch() return a bool
> to
> > > indicate whether it did anything, and use that to avoid this case.  It can
> do
> > > this by storing the value of |updated_latest_result_| before it calls
> > > CommitResult().
> >
> > Isn't that guaranteed by above if (match.deletable) block? I'll do this
change
> > if it's possible that deleting a deletable item may fail.
> 
> Why don't we do this: Add a DCHECK(updated_latest_result_) into DeleteMatch()
> before the controller calls CommitResult() and don't bother adding a return
> value.
> 
> Semantically, providers are _supposed_ to do something to force an update when
> they delete a match, but right now we don't enforce that, so that's basically
> what the DCHECK asserts.
I just checked the source code, and yes it's probably necessary to add a
DCHECK(updated_latest_result_) to avoid potential problem. But the history
provider may update the match without deleting it, so in such case your previous
statement makes sense that we shouldn't forcibly select this match as temporary
text.

[Peter Kasting, 2011-01-25 21:18:24]

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right):

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:247: // Remove
old template urls to avoid interfering our tests.
Nit: What are the old URLs and how would they interfere?

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:250: for
(size_t i = 0; i < old.size(); ++i)
Nit: An iterator might be nice

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:295: //
|controller->DeleteMatch()| to see if it's actually deleted or not.
I am confused.  The entire point of the DCHECK() added to
AutocompleteController::DeleteMatch() is to document the invariant that if you
ask a match to be deleted, the provider _must_ delete it and update the result
set.  Given that, AutocompletePopupModel::Observe() must get called, and this
business with |old_url| will never be necessary.

I think you should omit everything that refers to |old_url|.

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:303: // the results have
changed. And may
Nit: If you really want to add this second piece of the comment, I'd make it a
full-line comment and join it into one sentence, since it's getting long for an
EOL comment.  Otherwise, you can just leave it out.

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.h (right):

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.h:74: // If |force| is true
then the selected line will be updated forcibly even if
Nit: No need for a linebreak before this line.

[James Su, 2011-01-25 21:47:03]

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right):

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:247: // Remove
old template urls to avoid interfering our tests.
On 2011/01/25 21:18:24, Peter Kasting wrote:
> Nit: What are the old URLs and how would they interfere?

Done.

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:250: for
(size_t i = 0; i < old.size(); ++i)
On 2011/01/25 21:18:24, Peter Kasting wrote:
> Nit: An iterator might be nice

Done.

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:295: //
|controller->DeleteMatch()| to see if it's actually deleted or not.
On 2011/01/25 21:18:24, Peter Kasting wrote:
> I am confused.  The entire point of the DCHECK() added to
> AutocompleteController::DeleteMatch() is to document the invariant that if you
> ask a match to be deleted, the provider _must_ delete it and update the result
> set.  Given that, AutocompletePopupModel::Observe() must get called, and this
> business with |old_url| will never be necessary.
> 
> I think you should omit everything that refers to |old_url|.

According to HistoryProvider::DeleteMatch(), the match will not really be
deleted if it's a What-You-Typed or Bookmark match, though the match will be
updated to be looked like it has no backing data. And in this case the latest
result will still be updated to make sure the popup can be updated accordingly.
But we probably don't want to update the edit, as the match is actually still
there.

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:303: // the results have
changed. And may
On 2011/01/25 21:18:24, Peter Kasting wrote:
> Nit: If you really want to add this second piece of the comment, I'd make it a
> full-line comment and join it into one sentence, since it's getting long for
an
> EOL comment.  Otherwise, you can just leave it out.

Done.

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.h (right):

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.h:74: // If |force| is true
then the selected line will be updated forcibly even if
On 2011/01/25 21:18:24, Peter Kasting wrote:
> Nit: No need for a linebreak before this line.

Done.

[Peter Kasting, 2011-01-25 21:56:44]

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:295: //
|controller->DeleteMatch()| to see if it's actually deleted or not.
On 2011/01/25 21:47:03, James Su wrote:
> On 2011/01/25 21:18:24, Peter Kasting wrote:
> > I am confused.  The entire point of the DCHECK() added to
> > AutocompleteController::DeleteMatch() is to document the invariant that if
you
> > ask a match to be deleted, the provider _must_ delete it and update the
result
> > set.  Given that, AutocompletePopupModel::Observe() must get called, and
this
> > business with |old_url| will never be necessary.
> > 
> > I think you should omit everything that refers to |old_url|.
> 
> According to HistoryProvider::DeleteMatch(), the match will not really be
> deleted if it's a What-You-Typed or Bookmark match, though the match will be
> updated to be looked like it has no backing data. And in this case the latest
> result will still be updated to make sure the popup can be updated
accordingly.
> But we probably don't want to update the edit, as the match is actually still
> there.

You're missing my point.  There are two possible cases where the user is trying
to delete (WLOG) the What You Typed match:

1) That match is the default and the user is deleting it without having made a
prior manual selection.
2) That match may or may not be the default and the user has made a manual
selection.

Because the result set is always updated, the controller will notify the popup
and edit of changes in both cases.  That means in both cases, after we call
DeleteMatch() here, the edit and popup will have been reset to the default
match, no temporary text.  In case (1), we want to stop there.  In case (2), we
want to force-select the same line to put us back in temporary text mode.

Thus, the check for (was_temporary_text || (selected_line != selected_line_)) is
sufficient to handle this correctly.  There's no need to store and check the
default match URL.

http://codereview.chromium.org/6340012/diff/32001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right):

http://codereview.chromium.org/6340012/diff/32001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:251: for
(std::vector<const TemplateURL*>::iterator i = builtins.begin();
Nit: Can use a const_iterator here.

We really ought to make the TemplateURLVector typedef in template_url_model.h
public and use it here too...

http://codereview.chromium.org/6340012/diff/32001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/6340012/diff/32001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:302: // This may
synchronously notify us that the results have changed. And may
Nit: Given the DCHECK in DeleteMatch(), it would probably be more accurate to
say:

    // This will synchronously notify both the edit and us that the results have
    // changed, causing both to revert to the default match.

[James Su, 2011-01-25 22:32:37]

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/6340012/diff/23001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:295: //
|controller->DeleteMatch()| to see if it's actually deleted or not.
On 2011/01/25 21:56:44, Peter Kasting wrote:
> On 2011/01/25 21:47:03, James Su wrote:
> > On 2011/01/25 21:18:24, Peter Kasting wrote:
> > > I am confused.  The entire point of the DCHECK() added to
> > > AutocompleteController::DeleteMatch() is to document the invariant that if
> you
> > > ask a match to be deleted, the provider _must_ delete it and update the
> result
> > > set.  Given that, AutocompletePopupModel::Observe() must get called, and
> this
> > > business with |old_url| will never be necessary.
> > > 
> > > I think you should omit everything that refers to |old_url|.
> > 
> > According to HistoryProvider::DeleteMatch(), the match will not really be
> > deleted if it's a What-You-Typed or Bookmark match, though the match will be
> > updated to be looked like it has no backing data. And in this case the
latest
> > result will still be updated to make sure the popup can be updated
> accordingly.
> > But we probably don't want to update the edit, as the match is actually
still
> > there.
> 
> You're missing my point.  There are two possible cases where the user is
trying
> to delete (WLOG) the What You Typed match:
> 
> 1) That match is the default and the user is deleting it without having made a
> prior manual selection.
> 2) That match may or may not be the default and the user has made a manual
> selection.
> 
> Because the result set is always updated, the controller will notify the popup
> and edit of changes in both cases.  That means in both cases, after we call
> DeleteMatch() here, the edit and popup will have been reset to the default
> match, no temporary text.  In case (1), we want to stop there.  In case (2),
we
> want to force-select the same line to put us back in temporary text mode.
> 
> Thus, the check for (was_temporary_text || (selected_line != selected_line_))
is
> sufficient to handle this correctly.  There's no need to store and check the
> default match URL.

For case (1), my point is: if the match is really deleted then the default match
is actually changed, then we should convert into temporary text mode, because
this change is made by the user manually. But anyway I don't have strong opinion
here. I'm ok to change the code, if you insist that we should stay in normal
mode for this case.

http://codereview.chromium.org/6340012/diff/32001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right):

http://codereview.chromium.org/6340012/diff/32001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:251: for
(std::vector<const TemplateURL*>::iterator i = builtins.begin();
On 2011/01/25 21:56:44, Peter Kasting wrote:
> Nit: Can use a const_iterator here.
> 
> We really ought to make the TemplateURLVector typedef in template_url_model.h
> public and use it here too...

Done.

http://codereview.chromium.org/6340012/diff/32001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/6340012/diff/32001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_popup_model.cc:302: // This may
synchronously notify us that the results have changed. And may
On 2011/01/25 21:56:44, Peter Kasting wrote:
> Nit: Given the DCHECK in DeleteMatch(), it would probably be more accurate to
> say:
> 
>     // This will synchronously notify both the edit and us that the results
have
>     // changed, causing both to revert to the default match.

Done.

[James Su, 2011-01-26 04:02:21]

Just removed the url check.

[Peter Kasting, 2011-01-26 17:36:22]

LGTM

I had a response to the URL check written up but apparently never sent it.  In
case you care, it was this:

"I thought the whole point originally was that when you weren't in temporary
text mode to begin with, you shouldn't be placed into temporary text mode
afterwards, because you haven't arrowed around.  Otherwise the text in the
address bar would change without the selected line moving or the items changing
significantly, and then hitting escape would likewise change the text back
without moving the selection.  That seems very strange."

http://codereview.chromium.org/6340012/diff/44001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right):

http://codereview.chromium.org/6340012/diff/44001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:251:
TemplateURLModel::TemplateURLVector::const_iterator i = builtins.begin();
Nit: Never pull loop index variables out of the loop scope.