Index: chrome/browser/ui/webui/favicon_source.cc |
diff --git a/chrome/browser/ui/webui/favicon_source.cc b/chrome/browser/ui/webui/favicon_source.cc |
index 8ecedfbd5dfe03c668431458efc1ae3d7bd20189..4ba7aa25fd2e9cc88f2f0ccab29ea6f8efc4d29f 100644 |
--- a/chrome/browser/ui/webui/favicon_source.cc |
+++ b/chrome/browser/ui/webui/favicon_source.cc |
@@ -180,7 +180,12 @@ void FaviconSource::SendDefaultResponse( |
void FaviconSource::SendDefaultResponse(const IconRequest& icon_request) { |
int favicon_index; |
int resource_id; |
- switch (icon_request.size_in_dip) { |
+ bool desired_size_not_found = false; |
+ |
+ int desired_size_in_pixel = |
+ std::ceil(icon_request.size_in_dip * icon_request.device_scale_factor); |
Peter Kasting
2016/11/26 06:22:30
Why ceil, and not round?
minggang
2016/11/28 02:53:07
1 pixel higher resolution is always better if the
Peter Kasting
2016/11/28 03:42:42
I'm not any more sure that code is correct than th
|
+ |
+ switch (desired_size_in_pixel) { |
case 64: |
favicon_index = SIZE_64; |
resource_id = IDR_DEFAULT_FAVICON_64; |
@@ -189,21 +194,33 @@ void FaviconSource::SendDefaultResponse(const IconRequest& icon_request) { |
favicon_index = SIZE_32; |
resource_id = IDR_DEFAULT_FAVICON_32; |
break; |
- default: |
+ case 16: |
favicon_index = SIZE_16; |
resource_id = IDR_DEFAULT_FAVICON; |
break; |
+ default: |
Peter Kasting
2016/11/26 06:22:30
What other values are falling into here? 0? If s
minggang
2016/11/28 02:53:07
To assign a value to |resource_id| here is not pro
Peter Kasting
2016/11/28 03:42:42
You didn't answer my question: what values are fal
|
+ desired_size_not_found = true; |
Peter Kasting
2016/11/26 06:22:30
It's very confusing that we set a temp here, and t
minggang
2016/11/28 02:53:07
Please see the comment above.
On 2016/11/26 06:22:
|
+ break; |
} |
- base::RefCountedMemory* default_favicon = |
- default_favicons_[favicon_index].get(); |
- if (!default_favicon) { |
+ base::RefCountedMemory* default_favicon = nullptr; |
+ |
+ if (!desired_size_not_found) { |
Peter Kasting
2016/11/26 06:22:30
This is super-confusing to read, because you have
minggang
2016/11/28 02:53:07
Yes, |desired_size_not_found| with the if...else..
Peter Kasting
2016/11/28 03:42:43
Aim to leave the code in the best state possible,
|
+ default_favicon = default_favicons_[favicon_index].get(); |
+ |
+ if (!default_favicon) { |
+ default_favicon = |
+ ResourceBundle::GetSharedInstance().LoadDataResourceBytes( |
+ resource_id); |
+ default_favicons_[favicon_index] = default_favicon; |
+ } |
+ } else { |
ui::ScaleFactor resource_scale_factor = |
ui::GetSupportedScaleFactor(icon_request.device_scale_factor); |
+ |
default_favicon = |
ResourceBundle::GetSharedInstance().LoadDataResourceBytesForScale( |
- resource_id, resource_scale_factor); |
- default_favicons_[favicon_index] = default_favicon; |
+ IDR_DEFAULT_FAVICON, resource_scale_factor); |
Peter Kasting
2016/11/26 06:22:30
We go to the trouble of caching the favicons above
|
} |
icon_request.callback.Run(default_favicon); |