Make app list search result mixer more resilient to reordering.

Make app list search result mixer more resilient to reordering.

To add more result display types to the app list search results, we plan to
have a large pool of results which will be filtered by display type.

As a displayed result may retain its display index but have a different model
index, it is necessary to handle reordering of the items in an in-place way to
avoid icon flickering.

The current implementation only avoids flickering when the result is in the
same model index which is unsuitable for our purposes.

BUG=349727
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282299

[Matt Giuca, 2014-07-04 08:21:22]

Nice work! I only have a very large handful of comments.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/mixer.cc (right):

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:69: // Publishes the given
|new_results| to |ui_results|. Reuse existing ones to
I'm a little skeptical of the name |new_results|, because it could be
misconstrued to mean "add |new_results| to the existing set of results, and
don't remove existing ones", whereas I think you mean to replace all results in
|ui_results| with the new ones.

I'm fine with keeping the name, but please give a more detailed description
here:
"Publishes the given |new_results| to |ui_results|, deleting any existing
results that are not in |new_results|. ..."

Also s/Reuse/Reuses/.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:71: void Publish(const SortedResults&
new_results,
I think there should be unit tests for this function alone. It is sufficiently
complicated (it probably was before, but now it certainly is).

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:72: AppListModel::SearchResults*
ui_results) {
It might be nice to give a high-level overview of the algorithm at the top:

// 1. Transform both the |new_results| and |ui_results| lists into unordered
maps from result ID to item.
// 2. Update the existing items with the items from |new_results|, deleting any
items that are not in |new_results|.
// 3. Convert the updated item map back into an ordered results list, using the
order of the items in |new_results|.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:76: for (size_t i = 0; i <
new_results.size(); ++i)
Use an iterator.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:87: // meaningful indexes.
Does this mean observers will see a "remove" of a result and an "add" of the
same result, if one is present in both the old and new lists?

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:100: ChromeSearchResult* new_result =
result_it->second;
Can I suggest (the original code could have used this too) a separate function:
UpdateResult(const SortData& new_result, AppListModel::SearchResult* ui_result)
that updates a single result as appropriate.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:105:
new_results_map.erase(result_it);
Is there a reason you need to erase results from new_results_map? You never
refer to them again, so may as well just let them be deleted when
new_results_map goes out of scope.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:116: // Insert results in the order
of |new_results|.
Before this comment, say:
// Now, ui_results_map contains all of the updated results that were common to
both |ui_results| and |new_results|, but none of the newly added results.

(When I first read this, I was confused and thought you had forgotten about the
newly added results, until I saw that you were getting them directly from
|new_results| in the loop below.)

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:124: :
new_result->Duplicate().release());
Should you be calling NotifyItemsChanged here? The original code does it.

[calamity, 2014-07-08 09:47:21]

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/mixer.cc (right):

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:69: // Publishes the given
|new_results| to |ui_results|. Reuse existing ones to
On 2014/07/04 08:21:21, Matt Giuca wrote:
> I'm a little skeptical of the name |new_results|, because it could be
> misconstrued to mean "add |new_results| to the existing set of results, and
> don't remove existing ones", whereas I think you mean to replace all results
in
> |ui_results| with the new ones.
> 
> I'm fine with keeping the name, but please give a more detailed description
> here:
> "Publishes the given |new_results| to |ui_results|, deleting any existing
> results that are not in |new_results|. ..."
> 
> Also s/Reuse/Reuses/.

Done. Also changed the second sentence to better describe the problem.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:71: void Publish(const SortedResults&
new_results,
On 2014/07/04 08:21:21, Matt Giuca wrote:
> I think there should be unit tests for this function alone. It is sufficiently
> complicated (it probably was before, but now it certainly is).

Done.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:72: AppListModel::SearchResults*
ui_results) {
On 2014/07/04 08:21:22, Matt Giuca wrote:
> It might be nice to give a high-level overview of the algorithm at the top:
> 
> // 1. Transform both the |new_results| and |ui_results| lists into unordered
> maps from result ID to item.
> // 2. Update the existing items with the items from |new_results|, deleting
any
> items that are not in |new_results|.
> // 3. Convert the updated item map back into an ordered results list, using
the
> order of the items in |new_results|.

Done. Changed for new algorithm.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:76: for (size_t i = 0; i <
new_results.size(); ++i)
On 2014/07/04 08:21:21, Matt Giuca wrote:
> Use an iterator.

Done.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:87: // meaningful indexes.
On 2014/07/04 08:21:22, Matt Giuca wrote:
> Does this mean observers will see a "remove" of a result and an "add" of the
> same result, if one is present in both the old and new lists?

Yes. The alternative is to use a Move(), but the old code did a DeleteAt() then
AddAt() so I think this is okay (there's actually a PostTask that batches these
events in search_result_list_view.cc). 

Although, there should really be an AddAll() in list model so that I can just
send 1 notification. I can add this in a follow-up CL.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:100: ChromeSearchResult* new_result =
result_it->second;
On 2014/07/04 08:21:22, Matt Giuca wrote:
> Can I suggest (the original code could have used this too) a separate
function:
> UpdateResult(const SortData& new_result, AppListModel::SearchResult*
ui_result)
> that updates a single result as appropriate.

Done.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:116: // Insert results in the order
of |new_results|.
On 2014/07/04 08:21:21, Matt Giuca wrote:
> Before this comment, say:
> // Now, ui_results_map contains all of the updated results that were common
to
> both |ui_results| and |new_results|, but none of the newly added results.
> 
> (When I first read this, I was confused and thought you had forgotten about
the
> newly added results, until I saw that you were getting them directly from
> |new_results| in the loop below.)

Done.

https://codereview.chromium.org/369693004/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/mixer.cc:124: :
new_result->Duplicate().release());
On 2014/07/04 08:21:21, Matt Giuca wrote:
> Should you be calling NotifyItemsChanged here? The original code does it.

Not necessary as the add and remove are used to notify the observers of a
change. Changed SearchResultListView to reflect this.

[calamity, 2014-07-08 09:47:29]

The CQ bit was checked by calamity@chromium.org

[calamity, 2014-07-08 09:47:31]

The CQ bit was unchecked by calamity@chromium.org

[Matt Giuca, 2014-07-09 01:30:55]

https://codereview.chromium.org/369693004/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/mixer.cc (right):

https://codereview.chromium.org/369693004/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/mixer.cc:182: // The following algorithm is
used:
nit: Move this comment inside the function braces (since it is the
implementation details, not the public interface).

https://codereview.chromium.org/369693004/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/mixer_unittest.cc (right):

https://codereview.chromium.org/369693004/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/mixer_unittest.cc:263: new_results[1] =
Mixer::SortData(result1.get(), 1.0f);
I think this is the "new one", correct? (Because result1 was not used in the
previous test, so it is "new".) I think it's kind of unclear... a casual reading
of this is like "huh? you added a new one in the fourth place, not the second!"

I think you should add a result5 and use that in place of result1 here, so it's
clear that it is a totally new result.

https://codereview.chromium.org/369693004/diff/100001/ui/app_list/views/searc...
File ui/app_list/views/search_result_list_view.cc (right):

https://codereview.chromium.org/369693004/diff/100001/ui/app_list/views/searc...
ui/app_list/views/search_result_list_view.cc:264: NOTREACHED();
Did this become unreachable, or was it always? I don't see the relevance of this
change.

[calamity, 2014-07-09 03:03:49]

Rebase got mashed into the patchset =( Only 2 small chunks so hopefully it
doesn't cause any confusion.

https://codereview.chromium.org/369693004/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/mixer.cc (right):

https://codereview.chromium.org/369693004/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/mixer.cc:182: // The following algorithm is
used:
On 2014/07/09 01:30:54, Matt Giuca wrote:
> nit: Move this comment inside the function braces (since it is the
> implementation details, not the public interface).

Done.

https://codereview.chromium.org/369693004/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/mixer_unittest.cc (right):

https://codereview.chromium.org/369693004/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/mixer_unittest.cc:263: new_results[1] =
Mixer::SortData(result1.get(), 1.0f);
On 2014/07/09 01:30:54, Matt Giuca wrote:
> I think this is the "new one", correct? (Because result1 was not used in the
> previous test, so it is "new".) I think it's kind of unclear... a casual
reading
> of this is like "huh? you added a new one in the fourth place, not the
second!"
> 
> I think you should add a result5 and use that in place of result1 here, so
it's
> clear that it is a totally new result.

Done.

https://codereview.chromium.org/369693004/diff/100001/ui/app_list/views/searc...
File ui/app_list/views/search_result_list_view.cc (right):

https://codereview.chromium.org/369693004/diff/100001/ui/app_list/views/searc...
ui/app_list/views/search_result_list_view.cc:264: NOTREACHED();
On 2014/07/09 01:30:55, Matt Giuca wrote:
> Did this become unreachable, or was it always? I don't see the relevance of
this
> change.

It's unreachable because NotifyItemsChanged is no longer called.

[Matt Giuca, 2014-07-09 03:15:53]

lgtm

[calamity, 2014-07-09 08:04:57]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-07-09 08:06:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/369693004/160001

[commit-bot: I haz the power, 2014-07-09 09:59:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26352)

[commit-bot: I haz the power, 2014-07-09 10:05:18]

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

[commit-bot: I haz the power, 2014-07-09 10:05:19]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[calamity, 2014-07-10 04:53:33]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-07-10 04:55:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/369693004/200001

[commit-bot: I haz the power, 2014-07-10 10:56:30]

Change committed as 282299