Fixed problems with pin/unpin preferences changing for not running applications & icon order chang…

Fixed problems with pin/unpin preferences changing for not running applications & icon order changes upon sync operations

There are multiple problems fixed with this patch:
1. When a running and pinned app (Windowed V1, V2) gets unpinned via sync operation (or multi user switching), the icon will disappear and the sytem gets into an unstable state (-> Crash later on).
2. When a running app (Windowed V1, V2) gets pinned via a sync operation (or multi user switching), the icon will be shown, but get into an unsecure state which can lead to crashes.
3. There are several cases, where the chrome browser icon does change locations due to sync / multi user switching operations. In this case the icon can shift by one spot to the right.
4. There were some problems with the alternate shelf layout.

That all said: With M32 we should possibly come up with a new way of storing running V1/V2 applications since the running applications are not stored by the preferences and can therefore flip around when switching between users AND windowed V1 apps currently have the same "weight" as an application launcher - which means that they will "float around" in the shelf between sync / switch operations.

My visual test to see that everything was working was as follows:

In Multi session mode, have one user with the following applications pinned:
Chrome Shop, VNC, Angry Birds, gmail, calendar, chrome
and another user with two other applications and chrome.

Switch between the two and everything should be fine.
Run VNC (V2 app). Switch to user 2 and see that it is unpinned at the end of the list. Switch back and see it sliding back to where it belongs.
Then do the same for a windowed V1 app (gmail). Note that the gmail item will not float to the end of the list in user 2 - which is because of the already mentioned weights.

NOTE: This CL got reverted due to an ASAN failure which was fixed by another fix I did over the day waiting to land this thing. So folded this in.


BUG=294435, 294231, 294227, 295965
TEST=unit tests and visual (see above)
R=oshima@chromium.org

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

[Mr4D (OOO till 08-26), 2013-09-19 19:10:15]

Please have a look!

[Mr4D (OOO till 08-26), 2013-09-19 19:43:49]

Added Oshima since James does not feel well. Oshima, can you please have a look?

[oshima, 2013-09-19 21:42:10]

Man, this is complicated!

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:445: } else {
can you expland if else like
if (... == TYPE_APP) {
} else if (controller->locked()) {
  UnpinRunningAppInternal(model_->ItemIndexByID(id));
} else {
  LauncherItemClosed(id);
}

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:742: int
app_list_found = 0;
maybe app_list_index_found, or found_app_list_index? also size_t for index.

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:766: app_list_found
= 1;
why this is 1 not i?

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1353: // calls
|PersistPinnedState()| - otherwise all kinds of problems can arise.
add reference to bug? (assuming one of bugs listed is for this)

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1472:
model_->item_count() - 1);
indent

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1686: it !=
pinned_apps_pref->end(); ++it) {
since you need index anyway, using size_t index would be better than using
iterator.

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:471: bool
IsIncognito(content::WebContents* web_contents) const;
can web_content be const?

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:484: void
GetListOfPinnedAppsAndBrowser(std::vector<std::string>* pinned_apps);
Just return std::vector<std::string>
(See http://en.wikipedia.org/wiki/Return_value_optimization)

[oshima, 2013-09-19 21:43:25]

and...

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:225:
return items.Pass();
can you just inline them?

[Mr4D (OOO till 08-26), 2013-09-19 22:31:50]

Addressed! Please have another look!

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:445: } else {
Ahhh! Good one! case combined!

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:742: int
app_list_found = 0;
On 2013/09/19 21:42:11, oshima wrote:
> maybe app_list_index_found, or found_app_list_index? also size_t for index.

Done.

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:766: app_list_found
= 1;
Because all we do is to subtract 1 if the app list item is before the chrome
item. This has to change once Harry's CL lands. It's on my list, will prepare
that once it has landed.

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1353: // calls
|PersistPinnedState()| - otherwise all kinds of problems can arise.
No it is not. This was meant to be a helper comment for the next poor guy
debugging the code. I am a bit worried what happens if someone changes a
callback handler in an edge case. An "observer" suddenly call Pin/Unpin/Move/
which will cause the sync function to fire. Finding that will take a while. 
As an example:
  There is a special MoveItemWithoutPinnedStateChangeNotification function which
takes care of this for move.
I thought already to add a special member variable in the sync code and a DCHECK
at the end of the function to see if the sync function was called. But if I do
that - it would possibly be better to add the suggested ignore flag (then we are
100% sure).

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1472:
model_->item_count() - 1);
On 2013/09/19 21:42:11, oshima wrote:
> indent

Done.

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1686: it !=
pinned_apps_pref->end(); ++it) {
On 2013/09/19 21:42:11, oshima wrote:
> since you need index anyway, using size_t index would be better than using
> iterator.

Done.

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.h (right):

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:471: bool
IsIncognito(content::WebContents* web_contents) const;
On 2013/09/19 21:42:11, oshima wrote:
> can web_content be const?

Done.

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.h:484: void
GetListOfPinnedAppsAndBrowser(std::vector<std::string>* pinned_apps);
On 2013/09/19 21:42:11, oshima wrote:
> Just return std::vector<std::string>
> (See http://en.wikipedia.org/wiki/Return_value_optimization)

Done.

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/23580008/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:225:
return items.Pass();
On 2013/09/19 21:43:25, oshima wrote:
> can you just inline them?

Done.

[oshima, 2013-09-19 23:34:35]

lgtm

[commit-bot: I haz the power, 2013-09-20 00:21:09]

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

[commit-bot: I haz the power, 2013-09-20 01:56:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-20 03:10:12]

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

[commit-bot: I haz the power, 2013-09-20 06:56:26]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-20 14:00:19]

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

[commit-bot: I haz the power, 2013-09-20 18:56:24]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-20 19:17:13]

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

[commit-bot: I haz the power, 2013-09-20 22:20:25]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-20 22:52:44]

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

[Mr4D (OOO till 08-26), 2013-09-20 23:09:23]

Committed patchset #4 manually as r224514 (presubmit successful).

[commit-bot: I haz the power, 2013-09-21 01:31:37]

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

[commit-bot: I haz the power, 2013-09-21 04:11:26]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-21 04:12:31]

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

[commit-bot: I haz the power, 2013-09-21 06:37:28]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-09-21 13:43:57]

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

[commit-bot: I haz the power, 2013-09-21 16:20:50]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-09-21 16:21:37]

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

[commit-bot: I haz the power, 2013-09-21 16:59:44]

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

[Mr4D (OOO till 08-26), 2013-09-21 23:09:54]

Committed patchset #6 manually as r224611 (presubmit successful).