New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 767767 link

Starred by 1 user
Status: Fixed
Owner:
dominickn@chromium.org
Closed: Oct 2017
Cc:
pkotw...@chromium.org
█ mcgreevy@chromium.org
dominickn@chromium.org
█ bgalbs@google.com
pcovell@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Task



Sign in to add a comment

Consolidate installable icon size handling in InstallableManager

Project Member Reported by dominickn@chromium.org, Sep 22 2017 
Currently, the icon sizes required for add to homescreen / app banners / add to desktop are specified as parameters to the InstallableManager. This doesn't make much sense:

 - the icon sizes are constants per platform
 - it means InstallableManager doesn't ever know what icon sizes its fetching, creating a knowledge dependency
 - it requires a large amount of member variables with the icon sizes passed around the different classes.

The size calculations should be centralised in InstallableManager. Some amount of platform-specific bits will be required as the sizes on Android must be fetched over the JNI (specified in dips on the Java side).
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 26 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/87175111b60a633822e6630c0fd55e83605a0dd6

commit 87175111b60a633822e6630c0fd55e83605a0dd6
Author: Dominick Ng <dominickn@chromium.org>
Date: Tue Sep 26 07:40:12 2017

Consolidate installable icon size handling in InstallableManager.

The required icon sizes for adding a site to homescreen or desktop are
constants per platform. However, for legacy reasons, these sizes are
currently passed as parameters to InstallableManager::GetData. This
requires every caller of InstallableManager to know about icon sizes,
which isn't really necessary since every caller uses the same sizes on
desktop and on Android.

This CL consolidates all installable icon size specifications in
InstallableManager. Custom icon sizes can no longer be requested.

BUG= 767767 

Change-Id: Iee36e14854b3dd80d597e0c0d9370b6ebea46911
Reviewed-on: https://chromium-review.googlesource.com/678695
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504308}
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/android/banners/app_banner_manager_android.cc
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/android/banners/app_banner_manager_android.h
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/android/webapk/webapk_update_data_fetcher.cc
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/android/webapps/add_to_homescreen_manager.cc
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/banners/app_banner_manager.cc
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/banners/app_banner_manager.h
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/installable/installable_manager.cc
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/installable/installable_manager.h
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/installable/installable_manager_browsertest.cc
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/installable/installable_params.h
[modify] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/browser/installable/installable_task_queue_unittest.cc
[add] https://crrev.com/87175111b60a633822e6630c0fd55e83605a0dd6/chrome/test/data/banners/manifest_too_small_icon.json

Comment 2 by dominickn@chromium.org, Oct 9 2017

Status: Fixed (was: Started)
►

Sign in to add a comment