Fix the Crash in the launcher's start page (a better approach).

Fix the Crash in the launcher's start page (a better approach).

In Chrome OS launcher's start page, we recreate the app tile views
if we need to display different numbers of apps compared with the
previous time (e.g., install a new app or uninstall an existing app).
We need to make sure the number of the displayed apps is properly
updated. Otherwise, it might cause browser crash because of retrieving
a stale pointer in StartPageView::StartPageTilesContainer::GetTileItemView().
Also refactor related functions.

This is a better approach of https://codereview.chromium.org/2605463003/.

BUG=675150
Review-Url: https://codereview.chromium.org/2609843004
Cr-Commit-Position: refs/heads/master@{#441733}
Committed: https://chromium.googlesource.com/chromium/src/+/a1260e2951edbb3f8c0645827a3bb42a0ec7b926

[xdai1, 2017-01-03 23:37:35]

xdai@chromium.org changed reviewers:
+ xiyuan@chromium.org

[xdai1, 2017-01-03 23:37:36]

xiyuan@, this is the better approach as we discussed offline. Please take a look
at it, thanks for your review!

[xiyuan, 2017-01-04 00:12:09]

lgtm

https://codereview.chromium.org/2609843004/diff/1/ui/app_list/views/start_pag...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/2609843004/diff/1/ui/app_list/views/start_pag...
ui/app_list/views/start_page_view.cc:139: void UpdateSelectedIndex(int
old_selected, int new_selected) override;
nit: I don't have a strong preference of whether to keep overridden method
private. But we should probably keep them together. That is, either move the
remaining ones here or put those two back.

[xdai1, 2017-01-04 00:30:21]

https://codereview.chromium.org/2609843004/diff/1/ui/app_list/views/start_pag...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/2609843004/diff/1/ui/app_list/views/start_pag...
ui/app_list/views/start_page_view.cc:139: void UpdateSelectedIndex(int
old_selected, int new_selected) override;
On 2017/01/04 00:12:09, xiyuan wrote:
> nit: I don't have a strong preference of whether to keep overridden method
> private. But we should probably keep them together. That is, either move the
> remaining ones here or put those two back.

Just curious, why should we keep the overridden methods together?

[xiyuan, 2017-01-04 00:36:27]

https://codereview.chromium.org/2609843004/diff/1/ui/app_list/views/start_pag...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/2609843004/diff/1/ui/app_list/views/start_pag...
ui/app_list/views/start_page_view.cc:139: void UpdateSelectedIndex(int
old_selected, int new_selected) override;
On 2017/01/04 00:30:20, xdai1 wrote:
> On 2017/01/04 00:12:09, xiyuan wrote:
> > nit: I don't have a strong preference of whether to keep overridden method
> > private. But we should probably keep them together. That is, either move the
> > remaining ones here or put those two back.
> 
> Just curious, why should we keep the overridden methods together?

Think that would make it easier to read. And style guide says to keep similar
declarations together.

https://google.github.io/styleguide/cppguide.html#Declaration_Order

[xdai1, 2017-01-04 23:18:41]

xiyuan@, I've addressed your comment. Thanks for your review!

https://codereview.chromium.org/2609843004/diff/1/ui/app_list/views/start_pag...
File ui/app_list/views/start_page_view.cc (right):

https://codereview.chromium.org/2609843004/diff/1/ui/app_list/views/start_pag...
ui/app_list/views/start_page_view.cc:139: void UpdateSelectedIndex(int
old_selected, int new_selected) override;
On 2017/01/04 00:36:27, xiyuan wrote:
> On 2017/01/04 00:30:20, xdai1 wrote:
> > On 2017/01/04 00:12:09, xiyuan wrote:
> > > nit: I don't have a strong preference of whether to keep overridden method
> > > private. But we should probably keep them together. That is, either move
the
> > > remaining ones here or put those two back.
> > 
> > Just curious, why should we keep the overridden methods together?
> 
> Think that would make it easier to read. And style guide says to keep similar
> declarations together.
> 
> https://google.github.io/styleguide/cppguide.html#Declaration_Order

Done. Thanks for the reference!

[xiyuan, 2017-01-04 23:26:10]

still lgtm

[xdai1, 2017-01-05 18:53:33]

The CQ bit was checked by xdai@chromium.org

[xdai1, 2017-01-05 18:53:34]

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

[commit-bot: I haz the power, 2017-01-05 18:54:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 19:37:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483642413674050,
"parent_rev": "91ce10c665667d62c7f523334f8f127c51ffdef4", "commit_rev":
"a1260e2951edbb3f8c0645827a3bb42a0ec7b926"}

[commit-bot: I haz the power, 2017-01-05 19:37:41]

Description was changed from

==========
Fix the Crash in the launcher's start page (a better approach).

In Chrome OS launcher's start page, we recreate the app tile views
if we need to display different numbers of apps compared with the
previous time (e.g., install a new app or uninstall an existing app).
We need to make sure the number of the displayed apps is properly
updated. Otherwise, it might cause browser crash because of retrieving
a stale pointer in StartPageView::StartPageTilesContainer::GetTileItemView().
Also refactor related functions.

This is a better approach of https://codereview.chromium.org/2605463003/.

BUG=675150
==========

to

==========
Fix the Crash in the launcher's start page (a better approach).

In Chrome OS launcher's start page, we recreate the app tile views
if we need to display different numbers of apps compared with the
previous time (e.g., install a new app or uninstall an existing app).
We need to make sure the number of the displayed apps is properly
updated. Otherwise, it might cause browser crash because of retrieving
a stale pointer in StartPageView::StartPageTilesContainer::GetTileItemView().
Also refactor related functions.

This is a better approach of https://codereview.chromium.org/2605463003/.

BUG=675150

Review-Url: https://codereview.chromium.org/2609843004
Cr-Commit-Position: refs/heads/master@{#441733}
Committed:
https://chromium.googlesource.com/chromium/src/+/a1260e2951edbb3f8c0645827a3b...
==========

[commit-bot: I haz the power, 2017-01-05 19:37:42]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/a1260e2951edbb3f8c0645827a3b...