Skip to content

Commit 651a2d0

Browse files
khmelCommit bot
authored andcommitted
[Merge M58] arc: Handle position conflict in app list items.
It is not guaranteed that sync items have unique positions. This may cause crash in case web store app has conflicting pos with some other app. TEST=Unit test added BUG=692802 NOTRY=true NOPRESUBMIT=true TBR=stevenjb@chromium.org Review-Url: https://codereview.chromium.org/2732633003 Cr-Commit-Position: refs/heads/master@{#455195} (cherry picked from commit 11f9ee6) Review-Url: https://codereview.chromium.org/2740603002 Cr-Commit-Position: refs/branch-heads/3029@{#51} Cr-Branched-From: 939b32e-refs/heads/master@{#454471}
1 parent c1080fb commit 651a2d0

4 files changed

Lines changed: 178 additions & 21 deletions

File tree

chrome/browser/ui/app_list/app_list_syncable_service.cc

Lines changed: 42 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,6 @@ namespace app_list {
5454

5555
namespace {
5656

57-
const char kOemFolderId[] = "ddb1da55-d478-4243-8642-56d3041f0263";
58-
5957
const char kNameKey[] = "name";
6058
const char kParentIdKey[] = "parent_id";
6159
const char kPositionKey[] = "position";
@@ -288,6 +286,10 @@ class AppListSyncableService::ModelObserver : public AppListModelObserver {
288286

289287
// AppListSyncableService
290288

289+
// static
290+
const char AppListSyncableService::kOemFolderId[] =
291+
"ddb1da55-d478-4243-8642-56d3041f0263";
292+
291293
// static
292294
void AppListSyncableService::RegisterProfilePrefs(
293295
user_prefs::PrefRegistrySyncable* registry) {
@@ -1143,18 +1145,21 @@ std::string AppListSyncableService::FindOrCreateOemFolder() {
11431145
if (!oem_folder) {
11441146
std::unique_ptr<AppListFolderItem> new_folder(new AppListFolderItem(
11451147
kOemFolderId, AppListFolderItem::FOLDER_TYPE_OEM));
1146-
oem_folder =
1147-
static_cast<AppListFolderItem*>(model_->AddItem(std::move(new_folder)));
11481148
SyncItem* oem_sync_item = FindSyncItem(kOemFolderId);
1149+
syncer::StringOrdinal oem_position;
11491150
if (oem_sync_item) {
1151+
DCHECK(oem_sync_item->item_ordinal.IsValid());
11501152
VLOG(1) << "Creating OEM folder from existing sync item: "
11511153
<< oem_sync_item->item_ordinal.ToDebugString();
1152-
model_->SetItemPosition(oem_folder, oem_sync_item->item_ordinal);
1154+
oem_position = oem_sync_item->item_ordinal;
11531155
} else {
1154-
model_->SetItemPosition(oem_folder, GetOemFolderPos());
1156+
oem_position = GetOemFolderPos();
11551157
// Do not create a sync item for the OEM folder here, do it in
11561158
// ResolveFolderPositions() when the item position is finalized.
11571159
}
1160+
oem_folder =
1161+
static_cast<AppListFolderItem*>(model_->AddItem(std::move(new_folder)));
1162+
model_->SetItemPosition(oem_folder, oem_position);
11581163
}
11591164
model_->SetItemName(oem_folder, oem_folder_name_);
11601165
return oem_folder->id();
@@ -1181,24 +1186,40 @@ syncer::StringOrdinal AppListSyncableService::GetOemFolderPos() {
11811186
// stable. TODO(stevenjb): consider explicitly setting the OEM folder location
11821187
// along with the name in ServicesCustomizationDocument::SetOemFolderName().
11831188
AppListItemList* item_list = model_->top_level_item_list();
1184-
if (item_list->item_count() == 0)
1185-
return syncer::StringOrdinal();
1189+
if (!item_list->item_count()) {
1190+
LOG(ERROR) << "No top level item was found. "
1191+
<< "Placing OEM folder at the beginning.";
1192+
return syncer::StringOrdinal::CreateInitialOrdinal();
1193+
}
11861194

1187-
size_t oem_index = 0;
1188-
for (; oem_index < item_list->item_count() - 1; ++oem_index) {
1189-
AppListItem* cur_item = item_list->item_at(oem_index);
1190-
if (cur_item->id() == extensions::kWebStoreAppId)
1191-
break;
1195+
size_t web_store_app_index;
1196+
if (!item_list->FindItemIndex(extensions::kWebStoreAppId,
1197+
&web_store_app_index)) {
1198+
LOG(ERROR) << "Web store position is not found it top items. "
1199+
<< "Placing OEM folder at the end.";
1200+
return item_list->item_at(item_list->item_count() - 1)
1201+
->position()
1202+
.CreateAfter();
11921203
}
1193-
syncer::StringOrdinal oem_ordinal;
1194-
AppListItem* prev = item_list->item_at(oem_index);
1195-
if (oem_index + 1 < item_list->item_count()) {
1196-
AppListItem* next = item_list->item_at(oem_index + 1);
1197-
oem_ordinal = prev->position().CreateBetween(next->position());
1198-
} else {
1199-
oem_ordinal = prev->position().CreateAfter();
1204+
1205+
// Skip items with the same position.
1206+
const AppListItem* web_store_app_item =
1207+
item_list->item_at(web_store_app_index);
1208+
for (size_t j = web_store_app_index + 1; j < item_list->item_count(); ++j) {
1209+
const AppListItem* next_item = item_list->item_at(j);
1210+
DCHECK(next_item->position().IsValid());
1211+
if (!next_item->position().Equals(web_store_app_item->position())) {
1212+
const syncer::StringOrdinal oem_ordinal =
1213+
web_store_app_item->position().CreateBetween(next_item->position());
1214+
VLOG(1) << "Placing OEM Folder at: " << j
1215+
<< " position: " << oem_ordinal.ToDebugString();
1216+
return oem_ordinal;
1217+
}
12001218
}
1201-
VLOG(1) << "Placing OEM Folder at: " << oem_index
1219+
1220+
const syncer::StringOrdinal oem_ordinal =
1221+
web_store_app_item->position().CreateAfter();
1222+
VLOG(1) << "Placing OEM Folder at: " << item_list->item_count()
12021223
<< " position: " << oem_ordinal.ToDebugString();
12031224
return oem_ordinal;
12041225
}

chrome/browser/ui/app_list/app_list_syncable_service.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@ class AppListSyncableService : public syncer::SyncableService,
8585

8686
~AppListSyncableService() override;
8787

88+
static const char kOemFolderId[];
89+
8890
// Registers prefs to support local storage.
8991
static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
9092

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
// Copyright 2017 The Chromium Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
#include "chrome/browser/ui/app_list/app_list_syncable_service.h"
6+
#include "base/files/scoped_temp_dir.h"
7+
#include "chrome/browser/extensions/extension_service.h"
8+
#include "chrome/browser/profiles/profile_manager.h"
9+
#include "chrome/browser/ui/app_list/app_list_test_util.h"
10+
#include "chrome/test/base/testing_browser_process.h"
11+
#include "chrome/test/base/testing_profile.h"
12+
#include "components/crx_file/id_util.h"
13+
#include "extensions/browser/extension_system.h"
14+
#include "extensions/common/constants.h"
15+
#include "ui/app_list/app_list_item.h"
16+
#include "ui/app_list/app_list_model.h"
17+
18+
namespace {
19+
20+
scoped_refptr<extensions::Extension> MakeApp(
21+
const std::string& name,
22+
const std::string& id,
23+
extensions::Extension::InitFromValueFlags flags) {
24+
std::string err;
25+
base::DictionaryValue value;
26+
value.SetString("name", name);
27+
value.SetString("version", "0.0");
28+
value.SetString("app.launch.web_url", "http://google.com");
29+
scoped_refptr<extensions::Extension> app = extensions::Extension::Create(
30+
base::FilePath(), extensions::Manifest::INTERNAL, value, flags, id, &err);
31+
EXPECT_EQ(err, "");
32+
return app;
33+
}
34+
35+
// Creates next by natural sort ordering application id. Application id has to
36+
// have 32 chars each in range 'a' to 'p' inclusively.
37+
std::string CreateNextAppId(const std::string& app_id) {
38+
DCHECK(crx_file::id_util::IdIsValid(app_id));
39+
std::string next_app_id = app_id;
40+
size_t index = next_app_id.length() - 1;
41+
while (index > 0 && next_app_id[index] == 'p')
42+
next_app_id[index--] = 'a';
43+
DCHECK(next_app_id[index] != 'p');
44+
next_app_id[index]++;
45+
DCHECK(crx_file::id_util::IdIsValid(next_app_id));
46+
return next_app_id;
47+
}
48+
49+
} // namespace
50+
51+
class AppListSyncableServiceTest : public AppListTestBase {
52+
public:
53+
AppListSyncableServiceTest() = default;
54+
~AppListSyncableServiceTest() override = default;
55+
56+
void SetUp() override {
57+
AppListTestBase::SetUp();
58+
59+
// Make sure we have a Profile Manager.
60+
DCHECK(temp_dir_.CreateUniqueTempDir());
61+
TestingBrowserProcess::GetGlobal()->SetProfileManager(
62+
new ProfileManagerWithoutInit(temp_dir_.GetPath()));
63+
64+
extensions::ExtensionSystem* extension_system =
65+
extensions::ExtensionSystem::Get(profile_.get());
66+
DCHECK(extension_system);
67+
app_list_syncable_service_.reset(
68+
new app_list::AppListSyncableService(profile_.get(), extension_system));
69+
}
70+
71+
void TearDown() override { app_list_syncable_service_.reset(); }
72+
73+
app_list::AppListModel* model() {
74+
return app_list_syncable_service_->GetModel();
75+
}
76+
77+
private:
78+
base::ScopedTempDir temp_dir_;
79+
std::unique_ptr<app_list::AppListSyncableService> app_list_syncable_service_;
80+
81+
DISALLOW_COPY_AND_ASSIGN(AppListSyncableServiceTest);
82+
};
83+
84+
TEST_F(AppListSyncableServiceTest, OEMFolderForConflictingPos) {
85+
// Create a "web store" app.
86+
const std::string web_store_app_id(extensions::kWebStoreAppId);
87+
scoped_refptr<extensions::Extension> store =
88+
MakeApp("webstore", web_store_app_id,
89+
extensions::Extension::WAS_INSTALLED_BY_DEFAULT);
90+
service_->AddExtension(store.get());
91+
92+
// Create some app. Note its id should be greater than web store app id in
93+
// order to move app in case of conflicting pos after web store app.
94+
const std::string some_app_id = CreateNextAppId(extensions::kWebStoreAppId);
95+
scoped_refptr<extensions::Extension> some_app =
96+
MakeApp("some_app", some_app_id,
97+
extensions ::Extension::WAS_INSTALLED_BY_DEFAULT);
98+
service_->AddExtension(some_app.get());
99+
100+
app_list::AppListItem* web_store_item = model()->FindItem(web_store_app_id);
101+
ASSERT_TRUE(web_store_item);
102+
app_list::AppListItem* some_app_item = model()->FindItem(some_app_id);
103+
ASSERT_TRUE(some_app_item);
104+
105+
// Simulate position conflict.
106+
model()->SetItemPosition(web_store_item, some_app_item->position());
107+
108+
// Install an OEM app. It must be placed by default after web store app but in
109+
// case of app of the same position should be shifted next.
110+
const std::string oem_app_id = CreateNextAppId(some_app_id);
111+
scoped_refptr<extensions::Extension> oem_app = MakeApp(
112+
"oem_app", oem_app_id, extensions::Extension::WAS_INSTALLED_BY_OEM);
113+
service_->AddExtension(oem_app.get());
114+
115+
size_t web_store_app_index;
116+
size_t some_app_index;
117+
size_t oem_app_index;
118+
size_t oem_folder_index;
119+
EXPECT_TRUE(model()->top_level_item_list()->FindItemIndex(
120+
web_store_app_id, &web_store_app_index));
121+
EXPECT_TRUE(model()->top_level_item_list()->FindItemIndex(some_app_id,
122+
&some_app_index));
123+
// OEM item is not top level element.
124+
EXPECT_FALSE(model()->top_level_item_list()->FindItemIndex(oem_app_id,
125+
&oem_app_index));
126+
// But OEM folder is.
127+
EXPECT_TRUE(model()->top_level_item_list()->FindItemIndex(
128+
app_list::AppListSyncableService::kOemFolderId, &oem_folder_index));
129+
130+
// Ensure right item sequence.
131+
EXPECT_EQ(some_app_index, web_store_app_index + 1);
132+
EXPECT_EQ(oem_folder_index, web_store_app_index + 2);
133+
}

chrome/test/BUILD.gn

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4914,6 +4914,7 @@ test("unit_tests") {
49144914
"../browser/apps/drive/drive_app_mapping_unittest.cc",
49154915
"../browser/ui/app_list/app_context_menu_unittest.cc",
49164916
"../browser/ui/app_list/app_list_service_unittest.cc",
4917+
"../browser/ui/app_list/app_list_syncable_service_unittest.cc",
49174918
"../browser/ui/app_list/app_list_test_util.cc",
49184919
"../browser/ui/app_list/app_list_test_util.h",
49194920
"../browser/ui/app_list/arc/arc_app_test.cc",

0 commit comments

Comments
 (0)