Use ChromeLauncherControllerImpl::GetItem; cleanup.

Use ChromeLauncherControllerImpl::GetItem; cleanup.

Use GetItem in more places, reduce some index use.
Be more consistent about checking for negative indices.
Change GetItem to return a pointer and handle null.
Update id_to_item_controller_map_ for loops.

BUG=557406
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org

Committed: https://crrev.com/48f434bf27b2b671d04eb833a9bc208453e517b3
Cr-Commit-Position: refs/heads/master@{#436659}

[msw, 2016-12-05 21:06:18]

Description was changed from

==========
Use ChromeLauncherControllerImpl::GetItem; cleanup.

BUG=557406
==========

to

==========
Use ChromeLauncherControllerImpl::GetItem; cleanup.

Use GetItem in more places, reduce some index use.
Be more consistent about checking for negative indices.
Update id_to_item_controller_map_ for loops.

BUG=557406
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org
==========

[msw, 2016-12-05 21:06:18]

msw@chromium.org changed reviewers:
+ jamescook@chromium.org

[msw, 2016-12-05 21:07:48]

Hey James, please take a look; thanks!

[msw, 2016-12-05 21:07:51]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 21:08:27]

Dry run: 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, 2016-12-05 21:46:29]

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

[commit-bot: I haz the power, 2016-12-05 21:46:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-12-05 22:19:36]

On 2016/12/05 21:07:48, msw wrote:
> Hey James, please take a look; thanks!

Please hold, I'll look at fixing the tests first...

[msw, 2016-12-06 00:16:04]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 00:16:33]

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

[msw, 2016-12-06 00:19:18]

Tests should pass... please take a look; thanks!

[James Cook, 2016-12-06 00:27:01]

LGTM with an idea for ShelfModel.  Nice patch!

https://codereview.chromium.org/2551243002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2551243002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:315:
model_->Set(model_->ItemIndexByID(id), item);
ShelfModel::Set() has a DCHECK that the index is in range, which is good. What
do you think about also adding an early return if the index is bad, just in case
there's some bad code in production?

[msw, 2016-12-06 00:30:41]

https://codereview.chromium.org/2551243002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2551243002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:315:
model_->Set(model_->ItemIndexByID(id), item);
On 2016/12/06 00:27:01, James Cook wrote:
> ShelfModel::Set() has a DCHECK that the index is in range, which is good. What
> do you think about also adding an early return if the index is bad, just in
case
> there's some bad code in production?

Bad code would have already crashed in GetItem, if the index is invalid.
GetItem could return a ptr, using null for ids without an item; WDYT?

[commit-bot: I haz the power, 2016-12-06 00:48:31]

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

[commit-bot: I haz the power, 2016-12-06 00:48:32]

Dry run: This issue passed the CQ dry run.

[msw, 2016-12-06 01:54:17]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 01:54:57]

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

[msw, 2016-12-06 01:56:22]

https://codereview.chromium.org/2551243002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right):

https://codereview.chromium.org/2551243002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:315:
model_->Set(model_->ItemIndexByID(id), item);
On 2016/12/06 00:30:40, msw wrote:
> On 2016/12/06 00:27:01, James Cook wrote:
> > ShelfModel::Set() has a DCHECK that the index is in range, which is good.
What
> > do you think about also adding an early return if the index is bad, just in
> case
> > there's some bad code in production?
> 
> Bad code would have already crashed in GetItem, if the index is invalid.
> GetItem could return a ptr, using null for ids without an item; WDYT?

It just occurred to me that you meant an early return in ShelfModel::Set.
I uploaded a patch set which does that and changes GetItem to return a ptr.
Let me know what you think; I can easily change it up as you see fit.

[commit-bot: I haz the power, 2016-12-06 02:26:38]

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

[commit-bot: I haz the power, 2016-12-06 02:26:39]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-12-06 05:54:22]

still lgtm

Sorry for the confusion. All I meant was adding a safe return inside
ShelfModel::Set().

I'm fine with returning a pointer as long as the returned object stays const,
like you have it.

One other thought, not necessarily for this CL. Could it help to use the
null-object pattern (define a NullShelfItem and start using that for invalid
items)? We could have ShelfItem::is_null() to test for it.

https://codereview.chromium.org/2551243002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_model.cc (right):

https://codereview.chromium.org/2551243002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_model.cc:94: if (index < 0 || index >= item_count())
Yeah, this is all that I meant.

optional: You could also remove the DCHECK line and put a NOTREACHED inside the
if().

[msw, 2016-12-06 18:02:23]

On 2016/12/06 05:54:22, James Cook wrote:
> I'm fine with returning a pointer as long as the returned object stays const,
> like you have it.
> 
> One other thought, not necessarily for this CL. Could it help to use the
> null-object pattern (define a NullShelfItem and start using that for invalid
> items)? We could have ShelfItem::is_null() to test for it.

That could work; we also have shelfmodel::ItemByID which returns a const
iterator into the list of items, or the end() for not found. I might consolidate
on ptr/null or const-ref/is_null in the future.

https://codereview.chromium.org/2551243002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_model.cc (right):

https://codereview.chromium.org/2551243002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_model.cc:94: if (index < 0 || index >= item_count())
On 2016/12/06 05:54:22, James Cook wrote:
> Yeah, this is all that I meant.
> 
> optional: You could also remove the DCHECK line and put a NOTREACHED inside
the
> if().

Done.

[msw, 2016-12-06 18:02:30]

The CQ bit was checked by msw@chromium.org

[msw, 2016-12-06 18:02:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2551243002/#ps100001
(title: "Address comment.")

[commit-bot: I haz the power, 2016-12-06 18:03:04]

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

[msw, 2016-12-06 18:03:43]

Description was changed from

==========
Use ChromeLauncherControllerImpl::GetItem; cleanup.

Use GetItem in more places, reduce some index use.
Be more consistent about checking for negative indices.
Update id_to_item_controller_map_ for loops.

BUG=557406
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org
==========

to

==========
Use ChromeLauncherControllerImpl::GetItem; cleanup.

Use GetItem in more places, reduce some index use.
Be more consistent about checking for negative indices.
Change GetItem to return a pointer and handle null.
Update id_to_item_controller_map_ for loops.

BUG=557406
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-12-06 18:45:53]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481047350603190,
"parent_rev": "ac492cd5e4a2f941ce69dcf2b3f6c0af5e15c030", "commit_rev":
"dbd4e4590f167f28427048c59570187d06f82afd"}

[commit-bot: I haz the power, 2016-12-06 18:46:27]

Description was changed from

==========
Use ChromeLauncherControllerImpl::GetItem; cleanup.

Use GetItem in more places, reduce some index use.
Be more consistent about checking for negative indices.
Change GetItem to return a pointer and handle null.
Update id_to_item_controller_map_ for loops.

BUG=557406
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org
==========

to

==========
Use ChromeLauncherControllerImpl::GetItem; cleanup.

Use GetItem in more places, reduce some index use.
Be more consistent about checking for negative indices.
Change GetItem to return a pointer and handle null.
Update id_to_item_controller_map_ for loops.

BUG=557406
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-12-06 18:46:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-06 18:48:22]

Description was changed from

==========
Use ChromeLauncherControllerImpl::GetItem; cleanup.

Use GetItem in more places, reduce some index use.
Be more consistent about checking for negative indices.
Change GetItem to return a pointer and handle null.
Update id_to_item_controller_map_ for loops.

BUG=557406
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org
==========

to

==========
Use ChromeLauncherControllerImpl::GetItem; cleanup.

Use GetItem in more places, reduce some index use.
Be more consistent about checking for negative indices.
Change GetItem to return a pointer and handle null.
Update id_to_item_controller_map_ for loops.

BUG=557406
TEST=No Chrome OS shelf behavior changes.
R=jamescook@chromium.org

Committed: https://crrev.com/48f434bf27b2b671d04eb833a9bc208453e517b3
Cr-Commit-Position: refs/heads/master@{#436659}
==========

[commit-bot: I haz the power, 2016-12-06 18:48:23]

Patchset 6 (id:??) landed as
https://crrev.com/48f434bf27b2b671d04eb833a9bc208453e517b3
Cr-Commit-Position: refs/heads/master@{#436659}