New issue
Advanced search Search tips

Issue 860507 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Unify icon selection logic

Project Member Reported by na...@chromium.org, Jul 5

Issue description

Background Fetch introduced a new way to select icons (design doc here: https://docs.google.com/document/d/1rEhpXios2wkeePHm6NfHzplESecn0-zQ2XBVjP7ykOk/edit).

However, now that we have udpated our spec and code to use ImageResource instead of IconDefinition to represent icons, we should reuse the Web Manifest's icon selection code here: https://cs.chromium.org/chromium/src/content/browser/manifest/manifest_icon_selector.cc.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 10

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

commit 62db8de384c4e960d8c5cc0d8282385f4a3a98a5
Author: Mugdha Lakhani <nator@chromium.org>
Date: Tue Jul 10 16:00:48 2018

[Background Fetch] Move ManifestIconSelector to the blink layer.

Background Fetch introduced a new way to select icons (Bug:  813564 )

However, now that we have udpated our spec and code to use ImageResource
instead of IconDefinition to represent icons, we should reuse the Web Manifest's
icon selection code here.

This is the first step towards that. Since the background fetch code that picks an
icon for the UI lives in blink, here we move the icon selector code to blink, so it
can also be used there.

TBR=gogerald@chromium.com

Bug:  860507 
Change-Id: I50ef7f0e7b79a65765590ce7f9164a74512cc8ec
Reviewed-on: https://chromium-review.googlesource.com/1127167
Commit-Queue: Mugdha Lakhani <nator@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573753}
[modify] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/chrome/browser/android/shortcut_helper.cc
[modify] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc
[modify] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/chrome/browser/installable/installable_manager.cc
[modify] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/components/payments/content/installable_payment_app_crawler.cc
[modify] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/content/browser/BUILD.gn
[modify] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/content/browser/manifest/manifest_icon_downloader.cc
[modify] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/content/browser/payments/payment_app_info_fetcher.cc
[modify] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/content/browser/payments/payment_instrument_icon_fetcher.cc
[modify] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/content/public/browser/BUILD.gn
[modify] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/content/test/BUILD.gn
[modify] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/third_party/blink/common/BUILD.gn
[rename] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/third_party/blink/common/manifest/manifest_icon_selector.cc
[rename] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/third_party/blink/common/manifest/manifest_icon_selector_unittest.cc
[modify] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/third_party/blink/public/common/BUILD.gn
[rename] https://crrev.com/62db8de384c4e960d8c5cc0d8282385f4a3a98a5/third_party/blink/public/common/manifest/manifest_icon_selector.h

Project Member

Comment 2 by bugdroid1@chromium.org, Jul 16

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

commit 5cea57e17e8ed793a48048e59a4f35e13e5dae6d
Author: Mugdha Lakhani <nator@chromium.org>
Date: Mon Jul 16 14:51:39 2018

Constructor to create a KURL from a GURL.

We now have utilities in blink/common which operate GURLs. To be able to call these utilities
and use the returned GURL value in Blink, we should be able to convert it to KURL.

Bug:  860507 
Change-Id: I8175a432db5a1d1ab16770b7c1326042a5bcb306
Reviewed-on: https://chromium-review.googlesource.com/1131117
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Mugdha Lakhani <nator@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575259}
[modify] https://crrev.com/5cea57e17e8ed793a48048e59a4f35e13e5dae6d/third_party/blink/renderer/platform/weborigin/kurl.cc
[modify] https://crrev.com/5cea57e17e8ed793a48048e59a4f35e13e5dae6d/third_party/blink/renderer/platform/weborigin/kurl.h

Project Member

Comment 3 by bugdroid1@chromium.org, Jul 23

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

commit d4467892e8983b4ea42cf29c626acc8e55444a97
Author: Mugdha Lakhani <nator@chromium.org>
Date: Mon Jul 23 11:24:28 2018

[Background Fetch] Update icon selection logic to use ManifestIconSelector code.

Background Fetch introduced a new way to select icons (Bug:  813564 )

However, now that we have udpated our spec and code to use ImageResource
instead of IconDefinition to represent icons, we should reuse the Web Manifest's
icon selection code here.

This is the second step towards that.

Bug:  860507 
Change-Id: I1e8795f8768610e22002a9a87b0c7bf8c33f9d0b
Reviewed-on: https://chromium-review.googlesource.com/1128086
Commit-Queue: Mugdha Lakhani <nator@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577148}
[modify] https://crrev.com/d4467892e8983b4ea42cf29c626acc8e55444a97/content/common/background_fetch/background_fetch_struct_traits_unittest.cc
[modify] https://crrev.com/d4467892e8983b4ea42cf29c626acc8e55444a97/third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader.cc
[modify] https://crrev.com/d4467892e8983b4ea42cf29c626acc8e55444a97/third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader.h
[modify] https://crrev.com/d4467892e8983b4ea42cf29c626acc8e55444a97/third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader_test.cc
[modify] https://crrev.com/d4467892e8983b4ea42cf29c626acc8e55444a97/third_party/blink/renderer/modules/manifest/image_resource_type_converters.cc
[modify] https://crrev.com/d4467892e8983b4ea42cf29c626acc8e55444a97/third_party/blink/renderer/modules/manifest/image_resource_type_converters.h
[modify] https://crrev.com/d4467892e8983b4ea42cf29c626acc8e55444a97/third_party/blink/renderer/modules/manifest/image_resource_type_converters_test.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 24

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

commit 6b0f6110a957f7db89a58e35c8d503811401da65
Author: Mugdha Lakhani <nator@chromium.org>
Date: Tue Jul 24 10:42:31 2018

[BackgroundFetch] Weed out instances of IconDefinition

from the codebase, now that we're using Manifest::ImageResource instead.

Bug:  860507 
Change-Id: Ie8ac76ebe4e4ed1ab8a6b6a090dbf05b28d17f5c
TBR: peter
Reviewed-on: https://chromium-review.googlesource.com/1146656
Commit-Queue: Mugdha Lakhani <nator@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577480}
[modify] https://crrev.com/6b0f6110a957f7db89a58e35c8d503811401da65/content/common/background_fetch/background_fetch_types.typemap

Status: Fixed (was: Started)

Sign in to add a comment