Simplifying launcher handling by removing an unnecessary data structure and some functions

Simplifying launcher handling by removing an unnecessary data structure and some functions

Ahh. Getting rid of unnecessary complexity!

BUG=343254
TEST=covered by unit tests & tested by hand

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

[Mr4D (OOO till 08-26), 2014-03-26 21:17:48]

Please have a look!

[sky, 2014-03-26 21:59:34]

LGTM

I think you need to investigate BrowserStatusMonitor. I think some notifications
may be missed when leading to web_contents_to_app_id_ getting out of date.

https://codereview.chromium.org/213193007/diff/10001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/213193007/diff/10001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:939: // We remove
the content before checking existence with GetAppState.
Again, document why. This comment just documents code, not why order matters
here.

https://codereview.chromium.org/213193007/diff/10001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:946:
web_contents_to_app_id_[contents] = app_id;
Seems silly to have a remove 2 lines later. How about an else for the if on 949.

https://codereview.chromium.org/213193007/diff/10001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1681:
WebContentsToAppIDMap::iterator it = web_contents_to_app_id_.begin();
nit: for loop?

https://codereview.chromium.org/213193007/diff/10001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1685:
DCHECK(browser);
Are you sure this DCHECK is valid here? Seems to me there are a bunch of invalid
assummptions in BrowserStatusMonitor that could lead to improve cleanup. I
suppose that is separate from this patch though.

[Mr4D (OOO till 08-26), 2014-03-27 14:50:53]

Thanks for your review!

For BrowserStatusMonitor:

I looked at that before - many times (and even found a bug there) - but there
was nothing obvious. Today I looked again and I think I have found the culprit -
an interaction between multi profile and windowed V1 apps. I will fix that in a
separate CL.

https://codereview.chromium.org/213193007/diff/10001/chrome/browser/ui/ash/la...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/213193007/diff/10001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:939: // We remove
the content before checking existence with GetAppState.
I do not see it this way. It says that it removes the content before GetAppState
gets called which would otherwise find it as active. Sure the reader needs to
think a little bit.. I add a few more words. Done.

https://codereview.chromium.org/213193007/diff/10001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:946:
web_contents_to_app_id_[contents] = app_id;
On 2014/03/26 21:59:35, sky wrote:
> Seems silly to have a remove 2 lines later. How about an else for the if on
949.

Done.

https://codereview.chromium.org/213193007/diff/10001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1681:
WebContentsToAppIDMap::iterator it = web_contents_to_app_id_.begin();
On 2014/03/26 21:59:35, sky wrote:
> nit: for loop?

Done.

https://codereview.chromium.org/213193007/diff/10001/chrome/browser/ui/ash/la...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1685:
DCHECK(browser);
This DCHECK was added to identify an ilegal state which was the reason for this
cleanup: a page got added but never removed. I think that it is a good test to
have, but it could use a comment to point this out. - Done.

For BrowserStatusMonitor: I looked in the past over that module several times -
and will do so again.

[Mr4D (OOO till 08-26), 2014-03-27 14:50:56]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-27 14:51:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/213193007/30001

[commit-bot: I haz the power, 2014-03-27 15:10:01]

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

[commit-bot: I haz the power, 2014-03-27 15:10:01]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[Mr4D (OOO till 08-26), 2014-03-27 15:39:50]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-27 15:40:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/213193007/30001

[commit-bot: I haz the power, 2014-03-27 15:48:12]

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

[commit-bot: I haz the power, 2014-03-27 15:48:13]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Mr4D (OOO till 08-26), 2014-03-27 17:25:25]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-27 17:26:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/213193007/30001

[commit-bot: I haz the power, 2014-03-27 19:49:00]

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

[commit-bot: I haz the power, 2014-03-27 19:49:00]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Mr4D (OOO till 08-26), 2014-03-27 21:03:05]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-27 21:04:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/213193007/30001

[commit-bot: I haz the power, 2014-03-27 21:48:50]

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

[commit-bot: I haz the power, 2014-03-27 21:48:51]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Mr4D (OOO till 08-26), 2014-03-27 21:51:45]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-27 21:55:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/213193007/30001

[commit-bot: I haz the power, 2014-03-27 22:16:55]

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

[commit-bot: I haz the power, 2014-03-27 22:16:55]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Mr4D (OOO till 08-26), 2014-03-27 22:21:02]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-27 22:22:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/213193007/30001

[commit-bot: I haz the power, 2014-03-28 01:04:09]

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

[commit-bot: I haz the power, 2014-03-28 01:04:10]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Mr4D (OOO till 08-26), 2014-03-28 01:52:03]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-28 01:54:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/213193007/30001

[commit-bot: I haz the power, 2014-03-28 02:35:38]

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

[commit-bot: I haz the power, 2014-03-28 02:35:38]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Mr4D (OOO till 08-26), 2014-03-28 03:26:31]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-28 03:27:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/213193007/50001

[commit-bot: I haz the power, 2014-03-28 04:40:39]

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

[commit-bot: I haz the power, 2014-03-28 04:40:39]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Mr4D (OOO till 08-26), 2014-03-28 05:00:14]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-28 05:00:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/213193007/50001

[commit-bot: I haz the power, 2014-03-28 06:28:25]

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

[commit-bot: I haz the power, 2014-03-28 06:28:26]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Mr4D (OOO till 08-26), 2014-03-28 13:55:19]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-28 13:57:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/213193007/50001

[Mr4D (OOO till 08-26), 2014-03-28 14:52:28]

The CQ bit was unchecked by skuhne@chromium.org

[Mr4D (OOO till 08-26), 2014-03-28 17:29:53]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-28 17:32:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/213193007/70001

[commit-bot: I haz the power, 2014-03-28 20:03:26]

Change committed as 260246