Commit a9130219 authored by Alexey Baskakov's avatar Alexey Baskakov Committed by Commit Bot

dpwa: Fix user_display_mode field migration.

Background:
WebAppMigrationManager uses BookmarkAppRegistrar::GetAppUserDisplayMode()
getter to obtain user_display_mode field.

user_display_mode is a user-controllable field which we sync across all
user's devices. user_display_mode shouldn't depend on anything else
(like manifest display mode or any other web app field)

The call path:
BookmarkAppRegistrar::GetAppUserDisplayMode() ->
extensions::GetLaunchContainer() ->
extensions::GetLaunchType() ->
extensions::GetLaunchTypePrefValue().

The problem:

While extensions::GetLaunchTypePrefValue() returns a raw launch_type
that we are really interested in, extensions::GetLaunchType() resets
launch_type to "run as a tab" if the bookmark app is not locally
installed (Background: we sync bookmark apps as non-locally installed
on all OSes except ChromeOS):

LaunchType GetLaunchType(const ExtensionPrefs* prefs,
                         const Extension* extension) {
 ...
  // Force hosted apps that are not locally installed to open in tabs.
  if (extension->is_hosted_app() &&
      !BookmarkAppIsLocallyInstalled(prefs, extension)) {
    result = LAUNCH_TYPE_REGULAR;
  } else ...
}

This is the place where we were loosing information. If local migration
happens first on a device with non-locally installed app, this
preference gets uploaded to the sync server as the source of truth. As
a result, all secondary installs treat this "run as a tab" value as a
user choice.

Solution: Our ideal raw GetAppUserDisplayModeForMigration() getter
must not "Force hosted apps that are not locally installed to open
in tabs". We need to convert a raw value from prefs
extensions::GetLaunchTypePrefValue() using a switch statement with
all cases covered.

This is a P0 bug. This CL will be merged into M85 branch ASAP.

TBR=mgiuca@chromium.org

Bug: 1125020
Change-Id: Ie68aa8a86a2a052456fa66349bbd86758aaa56db
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2392168
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: default avatarAlexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804537}
parent da8cca0e
...@@ -190,6 +190,37 @@ DisplayMode BookmarkAppRegistrar::GetAppUserDisplayMode( ...@@ -190,6 +190,37 @@ DisplayMode BookmarkAppRegistrar::GetAppUserDisplayMode(
} }
} }
DisplayMode BookmarkAppRegistrar::GetAppUserDisplayModeForMigration(
const web_app::AppId& app_id) const {
const Extension* extension = GetBookmarkAppDchecked(app_id);
if (!extension)
return DisplayMode::kStandalone;
LaunchType launch_type = LAUNCH_TYPE_WINDOW;
int launch_type_value = GetLaunchTypePrefValue(
extensions::ExtensionPrefs::Get(profile()), app_id);
if (launch_type_value >= LAUNCH_TYPE_FIRST &&
launch_type_value < NUM_LAUNCH_TYPES) {
launch_type = static_cast<LaunchType>(launch_type_value);
}
DisplayMode user_display_mode = DisplayMode::kStandalone;
switch (launch_type) {
case LAUNCH_TYPE_PINNED:
case LAUNCH_TYPE_REGULAR:
user_display_mode = DisplayMode::kBrowser;
break;
case LAUNCH_TYPE_FULLSCREEN:
case LAUNCH_TYPE_WINDOW:
case LAUNCH_TYPE_INVALID:
case NUM_LAUNCH_TYPES:
user_display_mode = DisplayMode::kStandalone;
break;
}
return user_display_mode;
}
std::vector<DisplayMode> BookmarkAppRegistrar::GetAppDisplayModeOverride( std::vector<DisplayMode> BookmarkAppRegistrar::GetAppDisplayModeOverride(
const web_app::AppId& app_id) const { const web_app::AppId& app_id) const {
NOTIMPLEMENTED(); NOTIMPLEMENTED();
......
...@@ -74,6 +74,11 @@ class BookmarkAppRegistrar : public web_app::AppRegistrar, ...@@ -74,6 +74,11 @@ class BookmarkAppRegistrar : public web_app::AppRegistrar,
syncer::StringOrdinal GetUserLaunchOrdinal( syncer::StringOrdinal GetUserLaunchOrdinal(
const web_app::AppId& app_id) const; const web_app::AppId& app_id) const;
// This is the same as GetAppUserDisplayMode above except it doesn't take
// BookmarkAppIsLocallyInstalled() flag into consideration.
web_app::DisplayMode GetAppUserDisplayModeForMigration(
const web_app::AppId& app_id) const;
// ExtensionRegistryObserver: // ExtensionRegistryObserver:
// OnExtensionInstalled is not handled here. // OnExtensionInstalled is not handled here.
// AppRegistrar::NotifyWebAppInstalled is triggered by // AppRegistrar::NotifyWebAppInstalled is triggered by
......
...@@ -227,7 +227,7 @@ std::unique_ptr<WebApp> WebAppMigrationManager::MigrateBookmarkApp( ...@@ -227,7 +227,7 @@ std::unique_ptr<WebApp> WebAppMigrationManager::MigrateBookmarkApp(
web_app->SetDisplayMode(bookmark_app_registrar_.GetAppDisplayMode(app_id)); web_app->SetDisplayMode(bookmark_app_registrar_.GetAppDisplayMode(app_id));
DisplayMode user_display_mode = DisplayMode user_display_mode =
bookmark_app_registrar_.GetAppUserDisplayMode(app_id); bookmark_app_registrar_.GetAppUserDisplayModeForMigration(app_id);
if (user_display_mode != DisplayMode::kUndefined) if (user_display_mode != DisplayMode::kUndefined)
web_app->SetUserDisplayMode(user_display_mode); web_app->SetUserDisplayMode(user_display_mode);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment