Skip to content

Commit

Permalink
arc: Prevent overwriting of the Chrome Play Store icon.
Browse files Browse the repository at this point in the history
This implement correct access to ARC icon from notification manager and
from ARC kiosk handler. Incorrect access to the Play Store icon caused
request to extract this app icon from ARC side and which finally
overwrote Chrome Play Store icon, which was not expected.

TEST=Manually, Ensure that Play Store notification icon (in
     notification settings) correctly loaded with Chrome version of
     Play Store. Also checked arc.app icon cache to confirm that
     Android version of Play Store icon did not exist there.
BUG=719118

Review-Url: https://codereview.chromium.org/2865813002
Cr-Commit-Position: refs/heads/master@{#470058}
  • Loading branch information
khmel authored and Commit bot committed May 8, 2017
1 parent d50d846 commit 832d8b7
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 31 deletions.
6 changes: 4 additions & 2 deletions chrome/browser/chromeos/app_mode/arc/arc_kiosk_app_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,11 @@ void ArcKioskAppService::RequestNameAndIconUpdate() {
return;
app_icon_ = base::MakeUnique<ArcAppIcon>(profile_, app_id_,
app_list::kGridIconDimension, this);
app_icon_->LoadForScaleFactor(ui::GetSupportedScaleFactor(
app_icon_->image_skia().GetRepresentation(ui::GetSupportedScaleFactor(
display::Screen::GetScreen()->GetPrimaryDisplay().device_scale_factor()));
// Name and icon are updated when icon is loaded in OnIconUpdated()
// Apply default image now and in case icon is updated then OnIconUpdated()
// will be called additionally.
OnIconUpdated(app_icon_.get());
}

void ArcKioskAppService::PreconditionsChanged() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,12 @@ ArcApplicationNotifierSourceChromeOS::GetNotifierList(Profile* profile) {
kArcAppIconSizeInDp,
// The life time of icon must shorter than |this|.
this));
icon->LoadForScaleFactor(
icon->image_skia().GetRepresentation(
ui::GetSupportedScaleFactor(display::Screen::GetScreen()
->GetPrimaryDisplay()
.device_scale_factor()));
// Apply icon now to set the default image.
OnIconUpdated(icon.get());

// Add notifiers.
package_to_app_ids_.insert(std::make_pair(app->package_name, app_id));
Expand All @@ -73,7 +75,7 @@ ArcApplicationNotifierSourceChromeOS::GetNotifierList(Profile* profile) {
std::unique_ptr<message_center::Notifier> notifier(
new message_center::Notifier(notifier_id, base::UTF8ToUTF16(app->name),
app->notifications_enabled));
notifier->icon = icon->image();
notifier->icon = gfx::Image(icon->image_skia());
icons_.push_back(std::move(icon));
results.push_back(std::move(notifier));
}
Expand Down Expand Up @@ -117,7 +119,7 @@ void ArcApplicationNotifierSourceChromeOS::OnIconUpdated(ArcAppIcon* icon) {
observer_->OnIconImageUpdated(
message_center::NotifierId(message_center::NotifierId::ARC_APPLICATION,
icon->app_id()),
icon->image());
gfx::Image(icon->image_skia()));
}

void ArcApplicationNotifierSourceChromeOS::StopObserving() {
Expand Down
10 changes: 6 additions & 4 deletions chrome/browser/ui/app_list/arc/arc_app_icon.cc
Original file line number Diff line number Diff line change
Expand Up @@ -258,10 +258,14 @@ ArcAppIcon::~ArcAppIcon() {
}

void ArcAppIcon::LoadForScaleFactor(ui::ScaleFactor scale_factor) {
ArcAppListPrefs* prefs = ArcAppListPrefs::Get(context_);
// We provide Play Store icon from Chrome resources and it is not expected
// that we have external load request.
DCHECK_NE(app_id_, arc::kPlayStoreAppId);

const ArcAppListPrefs* const prefs = ArcAppListPrefs::Get(context_);
DCHECK(prefs);

base::FilePath path = prefs->GetIconPath(app_id_, scale_factor);
const base::FilePath path = prefs->GetIconPath(app_id_, scale_factor);
if (path.empty())
return;

Expand Down Expand Up @@ -362,8 +366,6 @@ void ArcAppIcon::Update(const gfx::ImageSkia* image) {
}
}

image_ = gfx::Image(image_skia_);

observer_->OnIconUpdated(this);
}

Expand Down
42 changes: 20 additions & 22 deletions chrome/browser/ui/app_list/arc/arc_app_icon.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,26 +45,8 @@ class ArcAppIcon {
~ArcAppIcon();

const std::string& app_id() const { return app_id_; }
gfx::Image image() const { return image_; }
const gfx::ImageSkia& image_skia() const { return image_skia_; }

// Icon loading is performed in several steps. It is initiated by
// LoadImageForScaleFactor request that specifies a required scale factor.
// ArcAppListPrefs is used to resolve a path to resource. Content of file is
// asynchronously read in context of browser file thread. On successful read,
// an icon data is decoded to an image in the special utility process.
// DecodeRequest is used to interact with the utility process, and each
// active request is stored at |decode_requests_| vector. When decoding is
// complete, results are returned in context of UI thread, and corresponding
// request is removed from |decode_requests_|. In case of some requests are
// not completed by the time of deleting this icon, they are automatically
// canceled.
// In case of the icon file is not available this requests ArcAppListPrefs to
// install required resource from ARC side. ArcAppListPrefs notifies UI items
// that new icon is available and corresponding item should invoke
// LoadImageForScaleFactor again.
void LoadForScaleFactor(ui::ScaleFactor scale_factor);

// Disables async safe decoding requests when unit tests are executed. This is
// done to avoid two problems. Problems come because icons are decoded at a
// separate process created by ImageDecoder. ImageDecoder has 5 seconds delay
Expand All @@ -78,10 +60,30 @@ class ArcAppIcon {
static void DisableSafeDecodingForTesting();

private:
friend class ArcAppIconLoader;
friend class ArcAppModelBuilder;

class Source;
class DecodeRequest;
struct ReadResult;

// Icon loading is performed in several steps. It is initiated by
// LoadImageForScaleFactor request that specifies a required scale factor.
// ArcAppListPrefs is used to resolve a path to resource. Content of file is
// asynchronously read in context of browser file thread. On successful read,
// an icon data is decoded to an image in the special utility process.
// DecodeRequest is used to interact with the utility process, and each
// active request is stored at |decode_requests_| vector. When decoding is
// complete, results are returned in context of UI thread, and corresponding
// request is removed from |decode_requests_|. In case of some requests are
// not completed by the time of deleting this icon, they are automatically
// canceled.
// In case of the icon file is not available this requests ArcAppListPrefs to
// install required resource from ARC side. ArcAppListPrefs notifies UI items
// that new icon is available and corresponding item should invoke
// LoadImageForScaleFactor again.
void LoadForScaleFactor(ui::ScaleFactor scale_factor);

void MaybeRequestIcon(ui::ScaleFactor scale_factor);
static std::unique_ptr<ArcAppIcon::ReadResult> ReadOnFileThread(
ui::ScaleFactor scale_factor,
Expand All @@ -99,10 +101,6 @@ class ArcAppIcon {
Source* source_ = nullptr; // Owned by ImageSkia storage.
gfx::ImageSkia image_skia_;

// The image wrapper around |image_skia_|.
// Note: this is reset each time a new representation is loaded.
gfx::Image image_;

// Contains pending image decode requests.
std::vector<std::unique_ptr<DecodeRequest>> decode_requests_;

Expand Down

0 comments on commit 832d8b7

Please sign in to comment.