New issue
Advanced search Search tips

Issue 712475 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Can we remove chrome://fallback-icon code

Project Member Reported by pkotw...@chromium.org, Apr 18 2017

Issue description

I don't think that Chrome uses the chrome://fallback-icon content::URLDataSource

I am wondering whether we can delete the code from Chromium (or whether there is a plan to use it soon)
 
Cc: mastiz@chromium.org pkotw...@chromium.org hua...@chromium.org
CCing huangs@ and mastiz@ who likely know best about future plans

Comment 2 by hua...@chromium.org, Apr 18 2017

Cc: rogerm@chromium.org
I wrote the code but haven't touched it for ages. A potential application is in bookmarks and history -- but realistically nobody (including me) cares enough to push this forward. Also I'm not aware of any existing uses.

The icons also depend on dominant color extraction code that +rogerm@ wrote. I don't know the status of that code (Removable? Reusable elsewhere?) 

So I'm in favor of ripping this out. Let me know if you want any help.

Comment 3 by mastiz@chromium.org, Apr 18 2017

It's not obvious to me whether it's dead code, in particular around https://cs.chromium.org/chromium/src/chrome/renderer/searchbox/searchbox.cc?l=69&rcl=db9cef9964f34067223f3d95f8a0206a3d07254b, but huangs' optimism suggests so.

If it is indeed dead, +1 to removing it. I'm not aware of future plans that would depend on it.


Comment 4 by hua...@chromium.org, Apr 18 2017

I wrote the CL with the line you cited in https://codereview.chromium.org/1017853002 .

We were going to replace NTP tiles (screenshots) with large icons, and if missing, render fallback with dominant color. The project got shelved for desktop, but got green light for Clank. Meanwhile, Clank uses its own Java fallback icon generator (RoundedIconGenerator), and doesn't use chrome://fallback-icon.
Project Member

Comment 5 by bugdroid1@chromium.org, May 26 2017

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

commit 59f676347cd7bb0ec16a9cae0e1257f6548dc3b4
Author: pkotwicz <pkotwicz@chromium.org>
Date: Fri May 26 19:28:05 2017

Delete unused chrome://fallback-icon/ handling

BUG= 712475 
R=huangs,jkrcal,pkasting
TBR=treib for removal of stray includes in chrome/browser/search/

Review-Url: https://codereview.chromium.org/2883053003
Cr-Commit-Position: refs/heads/master@{#475087}

[modify] https://crrev.com/59f676347cd7bb0ec16a9cae0e1257f6548dc3b4/chrome/browser/BUILD.gn
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/chrome/browser/favicon/chrome_fallback_icon_client.cc
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/chrome/browser/favicon/chrome_fallback_icon_client.h
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/chrome/browser/favicon/chrome_fallback_icon_client_factory.cc
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/chrome/browser/favicon/chrome_fallback_icon_client_factory.h
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/chrome/browser/favicon/chrome_fallback_icon_client_unittest.cc
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/chrome/browser/favicon/fallback_icon_service_factory.cc
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/chrome/browser/favicon/fallback_icon_service_factory.h
[modify] https://crrev.com/59f676347cd7bb0ec16a9cae0e1257f6548dc3b4/chrome/browser/search/instant_service.cc
[modify] https://crrev.com/59f676347cd7bb0ec16a9cae0e1257f6548dc3b4/chrome/browser/ui/BUILD.gn
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/chrome/browser/ui/webui/fallback_icon_source.cc
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/chrome/browser/ui/webui/fallback_icon_source.h
[modify] https://crrev.com/59f676347cd7bb0ec16a9cae0e1257f6548dc3b4/chrome/common/url_constants.cc
[modify] https://crrev.com/59f676347cd7bb0ec16a9cae0e1257f6548dc3b4/chrome/renderer/searchbox/searchbox.cc
[modify] https://crrev.com/59f676347cd7bb0ec16a9cae0e1257f6548dc3b4/chrome/test/BUILD.gn
[modify] https://crrev.com/59f676347cd7bb0ec16a9cae0e1257f6548dc3b4/chrome/test/base/testing_profile.cc
[modify] https://crrev.com/59f676347cd7bb0ec16a9cae0e1257f6548dc3b4/components/favicon/core/BUILD.gn
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/components/favicon/core/fallback_icon_client.h
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/components/favicon/core/fallback_icon_service.cc
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/components/favicon/core/fallback_icon_service.h
[modify] https://crrev.com/59f676347cd7bb0ec16a9cae0e1257f6548dc3b4/components/favicon_base/BUILD.gn
[modify] https://crrev.com/59f676347cd7bb0ec16a9cae0e1257f6548dc3b4/components/favicon_base/fallback_icon_style.cc
[modify] https://crrev.com/59f676347cd7bb0ec16a9cae0e1257f6548dc3b4/components/favicon_base/fallback_icon_style.h
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/components/favicon_base/fallback_icon_url_parser.cc
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/components/favicon_base/fallback_icon_url_parser.h
[delete] https://crrev.com/b7a527720b277b3ec4af030be6c32e288f0b18c6/components/favicon_base/fallback_icon_url_parser_unittest.cc

Owner: pkotw...@chromium.org
Status: Fixed (was: Untriaged)

Sign in to add a comment