ash: Remove ShelfModel id conversion functions.

ash: Remove ShelfModel id conversion functions.

ShelfID is an app_id and launch_id; conversion isn't needed.
Update some callers to check for ShelfItem presence instead.
Lots of cleanup: remove unused code and simplify elsewhere.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2870683002
Cr-Commit-Position: refs/heads/master@{#470694}
Committed: https://chromium.googlesource.com/chromium/src/+/2893fe08decf1fca698b0096e5e530eba397baa5

[msw, 2017-05-09 21:44:00]

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

[commit-bot: I haz the power, 2017-05-09 21:45:05]

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, 2017-05-09 21:50:49]

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

[commit-bot: I haz the power, 2017-05-09 21:50:50]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msw, 2017-05-09 22:37:58]

Description was changed from

==========
ash: Remove ShelfModel id conversion functions.

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

to

==========
ash: Remove ShelfModel id conversion functions.

ShelfID is an app_id and launch_id; conversion isn't needed.
Update some callers to check for ShelfItem presence instead.
Lots of cleanup: remove unused code and simplify elsewhere.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes.
R=jamescook@chromium.org
==========

[msw, 2017-05-09 22:37:59]

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

[msw, 2017-05-09 22:46:23]

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

[commit-bot: I haz the power, 2017-05-09 22:48:13]

Dry run: CQ is trying da patch.

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

[msw, 2017-05-09 23:14:43]

Hey James, please take a look; thanks! (no rush)

[commit-bot: I haz the power, 2017-05-10 00:51:25]

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

[commit-bot: I haz the power, 2017-05-10 00:51:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[James Cook, 2017-05-10 02:51:18]

Some initial comments. I only made it as far as
browser_shortcut_launcher_item_controller.cc - I'll look at the rest tomorrow.

https://codereview.chromium.org/2870683002/diff/60001/ash/shelf/shelf_model.cc
File ash/shelf/shelf_model.cc (right):

https://codereview.chromium.org/2870683002/diff/60001/ash/shelf/shelf_model.c...
ash/shelf/shelf_model.cc:46: std::string GetShelfAppIdFromArcAppId(const
std::string& arc_app_id) {
Not for this CL: This function really needs to be renamed. |arc_app_id| is not
an ARC app id.

https://codereview.chromium.org/2870683002/diff/60001/ash/shelf/shelf_model.c...
ash/shelf/shelf_model.cc:59: // TODO(khmel): Fix this Arc application id
mapping. See http://b/31703859
Aside: Is this ever going to happen? It's such a bummer we have to copy/paste
this conversion function everywhere.

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc
(right):

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:210:
app_controller_map_[shelf_app_id] = controller.get();
Is it safe to move this up? Won't that mean the if () on line 219 won't ever
execute?  Does any of the code between here and there touch app_controller_map_?

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc (right):

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:379:
controller->PinAppWithID(shelf_id1.app_id);
Just for my knowledge, any particular reason you're switching from
ShelfModel::PinAppWithID() to CLC::PinAppWithID()?

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:387:
EXPECT_NE(nullptr, controller->GetItem(shelf_id1));
nit: Can you do EXPECT_TRUE/EXPECT_FALSE on the pointers here? I think it would
be easier to read than EN/EQ vs nullptr.

Or maybe add a HasItem() function somewhere?

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:658: if
(owner()->GetItem(shelf_id) == nullptr) {
Yeah, I think HasItem() would be easier to read.

"== nullptr" makes me think of Java code.

[James Cook, 2017-05-10 15:56:34]

The rest looks good.

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:931:
shelf_ids_after.push_back(model_->items()[i].id);
optional: I think it was a little more readable pulling model_->items()[i].id
into a local

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (left):

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:98:
controller()->GetAppIDForShelfID(item().id));
Global praise: It's so awesome we don't have to do these lookups all over the
place anymore.

[msw, 2017-05-10 18:21:54]

Comments addressed, please take another look; thanks!

https://codereview.chromium.org/2870683002/diff/60001/ash/shelf/shelf_model.cc
File ash/shelf/shelf_model.cc (right):

https://codereview.chromium.org/2870683002/diff/60001/ash/shelf/shelf_model.c...
ash/shelf/shelf_model.cc:46: std::string GetShelfAppIdFromArcAppId(const
std::string& arc_app_id) {
On 2017/05/10 02:51:18, James Cook (sheriff - slow) wrote:
> Not for this CL: This function really needs to be renamed. |arc_app_id| is not
> an ARC app id.

Agreed... I might consider following up here.

https://codereview.chromium.org/2870683002/diff/60001/ash/shelf/shelf_model.c...
ash/shelf/shelf_model.cc:59: // TODO(khmel): Fix this Arc application id
mapping. See http://b/31703859
On 2017/05/10 02:51:18, James Cook (sheriff - slow) wrote:
> Aside: Is this ever going to happen? It's such a bummer we have to copy/paste
> this conversion function everywhere.

I can ping Yury (though I've been woefully slow reviewing his icon cl).
I may try to investigate and figure out a better solution here myself.

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc
(right):

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_deferred_launcher_controller.cc:210:
app_controller_map_[shelf_app_id] = controller.get();
On 2017/05/10 02:51:18, James Cook (sheriff - slow) wrote:
> Is it safe to move this up? Won't that mean the if () on line 219 won't ever
> execute?  Does any of the code between here and there touch
app_controller_map_?

Good catch, I reverted this part of the change. This is a good reminder to not
get cavalier with refactoring changes.

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc (right):

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:379:
controller->PinAppWithID(shelf_id1.app_id);
On 2017/05/10 02:51:18, James Cook (sheriff - slow) wrote:
> Just for my knowledge, any particular reason you're switching from
> ShelfModel::PinAppWithID() to CLC::PinAppWithID()?

I mostly wanted to avoid using ash::Shell's ShelfModel here (to support mash's
impending two-model pattern). I could use
controller->shelf_model()->PinAppWithID(), but it just seemed to make sense to
use ChromeLauncherController's helper function to do the same. WDYT?

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:387:
EXPECT_NE(nullptr, controller->GetItem(shelf_id1));
On 2017/05/10 02:51:18, James Cook (sheriff - slow) wrote:
> nit: Can you do EXPECT_TRUE/EXPECT_FALSE on the pointers here? I think it
would
> be easier to read than EN/EQ vs nullptr.
> 
> Or maybe add a HasItem() function somewhere?

Done changed here and elsewhere to TRUE/FALSE.

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:658: if
(owner()->GetItem(shelf_id) == nullptr) {
On 2017/05/10 02:51:18, James Cook (sheriff - slow) wrote:
> Yeah, I think HasItem() would be easier to read.
> 
> "== nullptr" makes me think of Java code.

I'm using TRUE/FALSE for test expectations, and I simplified this.

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:931:
shelf_ids_after.push_back(model_->items()[i].id);
On 2017/05/10 15:56:34, James Cook (sheriff - slow) wrote:
> optional: I think it was a little more readable pulling model_->items()[i].id
> into a local

Done.

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc (left):

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc:98:
controller()->GetAppIDForShelfID(item().id));
On 2017/05/10 15:56:34, James Cook (sheriff - slow) wrote:
> Global praise: It's so awesome we don't have to do these lookups all over the
> place anymore.

I agree!

[msw, 2017-05-10 18:21:59]

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

[commit-bot: I haz the power, 2017-05-10 18:24:37]

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, 2017-05-10 18:35:43]

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

[commit-bot: I haz the power, 2017-05-10 18:35:44]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[James Cook, 2017-05-10 19:29:35]

LGTM assuming the bot failures aren't serious

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc (right):

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_launcher_browsertest.cc:379:
controller->PinAppWithID(shelf_id1.app_id);
On 2017/05/10 18:21:53, msw wrote:
> On 2017/05/10 02:51:18, James Cook (sheriff - slow) wrote:
> > Just for my knowledge, any particular reason you're switching from
> > ShelfModel::PinAppWithID() to CLC::PinAppWithID()?
> 
> I mostly wanted to avoid using ash::Shell's ShelfModel here (to support mash's
> impending two-model pattern). I could use
> controller->shelf_model()->PinAppWithID(), but it just seemed to make sense to
> use ChromeLauncherController's helper function to do the same. WDYT?

sgtm

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2870683002/diff/60001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:658: if
(owner()->GetItem(shelf_id) == nullptr) {
On 2017/05/10 18:21:53, msw wrote:
> On 2017/05/10 02:51:18, James Cook (sheriff - slow) wrote:
> > Yeah, I think HasItem() would be easier to read.
> > 
> > "== nullptr" makes me think of Java code.
> 
> I'm using TRUE/FALSE for test expectations, and I simplified this.

Yeah, I think the TRUE/FALSE works well enough.

[msw, 2017-05-10 20:01:23]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2017-05-10 20:03:52]

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-05-10 20:38:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494446483909060,
"parent_rev": "30c518245c94361d0eb5077e278782d49b9f9cd5", "commit_rev":
"2893fe08decf1fca698b0096e5e530eba397baa5"}

[commit-bot: I haz the power, 2017-05-10 20:38:38]

Description was changed from

==========
ash: Remove ShelfModel id conversion functions.

ShelfID is an app_id and launch_id; conversion isn't needed.
Update some callers to check for ShelfItem presence instead.
Lots of cleanup: remove unused code and simplify elsewhere.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes.
R=jamescook@chromium.org
==========

to

==========
ash: Remove ShelfModel id conversion functions.

ShelfID is an app_id and launch_id; conversion isn't needed.
Update some callers to check for ShelfItem presence instead.
Lots of cleanup: remove unused code and simplify elsewhere.

BUG=557406
TEST=Automated; no Chrome OS shelf behavior changes.
R=jamescook@chromium.org

Review-Url: https://codereview.chromium.org/2870683002
Cr-Commit-Position: refs/heads/master@{#470694}
Committed:
https://chromium.googlesource.com/chromium/src/+/2893fe08decf1fca698b0096e5e5...
==========

[commit-bot: I haz the power, 2017-05-10 20:38:40]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2893fe08decf1fca698b0096e5e5...