Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2456193003:
NTPResourceCache don't add substitution for colorSectionBorder twice (Closed)
DescriptionNTPResourceCache don't add substitution for colorSectionBorder twice
Found through static analysis by PVS-Studio!
Also remove .thumbnail-wrapper style, which referred to colorSectionBorder
but wasn't used.
BUG=660198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/c6ddf87591b0227947d16d415d3401aabb26ebef
Cr-Commit-Position: refs/heads/master@{#428917}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Do the safe thing: keep the second entry instead #Patch Set 3 : remove unused .thumbnail-wrapper style #
Messages
Total messages: 24 (11 generated)
Description was changed from ========== NTPResourceCache don't add substitution for colorSectionBorder twice Found through static analysis by PVS-Studio! BUG=660198 ========== to ========== NTPResourceCache don't add substitution for colorSectionBorder twice Found through static analysis by PVS-Studio! BUG=660198 ==========
hans@chromium.org changed reviewers: + dschuyler@chromium.org, estade@chromium.org
Please take a look.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2456193003/diff/1/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (left): https://codereview.chromium.org/2456193003/diff/1/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:583: SkColorToRGBComponents(color_section_border); The first addition was as a string, this one as components. Is it clear the first one is the correct one?
https://codereview.chromium.org/2456193003/diff/1/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (left): https://codereview.chromium.org/2456193003/diff/1/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:583: SkColorToRGBComponents(color_section_border); On 2016/10/27 22:29:10, Peter Kasting wrote: > The first addition was as a string, this one as components. Is it clear the > first one is the correct one? Hmm, you're right. I was assuming an RGBA string and components would be functionally equivalent, but I don't really know that of course. The more defensive thing would be to remove the first addition. I'd like to hear what dschuyler thinks.
https://codereview.chromium.org/2456193003/diff/1/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (left): https://codereview.chromium.org/2456193003/diff/1/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:583: SkColorToRGBComponents(color_section_border); On 2016/10/27 22:44:38, hans wrote: > On 2016/10/27 22:29:10, Peter Kasting wrote: > > The first addition was as a string, this one as components. Is it clear the > > first one is the correct one? > > Hmm, you're right. I was assuming an RGBA string and components would be > functionally equivalent, but I don't really know that of course. The more > defensive thing would be to remove the first addition. > > I'd like to hear what dschuyler thinks. I think that I didn't notice that there were two places that color_selection_border was being set previously. My apologies. https://codereview.chromium.org/1220793010/diff/490001/chrome/browser/ui/webu... When I ported the code to 'the new way', it brought that weird aspect forward. IMO, the safer thing to do would be to keep the latter entry. That (I believe) is the one that is currently active - it will have been stomping the former entry.
https://codereview.chromium.org/2456193003/diff/1/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (left): https://codereview.chromium.org/2456193003/diff/1/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:583: SkColorToRGBComponents(color_section_border); On 2016/10/27 23:07:34, dschuyler wrote: > On 2016/10/27 22:44:38, hans wrote: > > On 2016/10/27 22:29:10, Peter Kasting wrote: > > > The first addition was as a string, this one as components. Is it clear the > > > first one is the correct one? > > > > Hmm, you're right. I was assuming an RGBA string and components would be > > functionally equivalent, but I don't really know that of course. The more > > defensive thing would be to remove the first addition. > > > > I'd like to hear what dschuyler thinks. > > I think that I didn't notice that there were two > places that color_selection_border was being set previously. My apologies. > > https://codereview.chromium.org/1220793010/diff/490001/chrome/browser/ui/webu... > > When I ported the code to 'the new way', it brought that weird aspect forward. > IMO, the safer thing to do would be to keep the latter entry. That (I > believe) is the one that is currently active - it will have been > stomping the former entry. Sounds good to me. New patch uploaded.
On 2016/10/27 23:13:07, hans wrote: > https://codereview.chromium.org/2456193003/diff/1/chrome/browser/ui/webui/ntp... > File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (left): > > https://codereview.chromium.org/2456193003/diff/1/chrome/browser/ui/webui/ntp... > chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:583: > SkColorToRGBComponents(color_section_border); > On 2016/10/27 23:07:34, dschuyler wrote: > > On 2016/10/27 22:44:38, hans wrote: > > > On 2016/10/27 22:29:10, Peter Kasting wrote: > > > > The first addition was as a string, this one as components. Is it clear > the > > > > first one is the correct one? > > > > > > Hmm, you're right. I was assuming an RGBA string and components would be > > > functionally equivalent, but I don't really know that of course. The more > > > defensive thing would be to remove the first addition. > > > > > > I'd like to hear what dschuyler thinks. > > > > I think that I didn't notice that there were two > > places that color_selection_border was being set previously. My apologies. > > > > > https://codereview.chromium.org/1220793010/diff/490001/chrome/browser/ui/webu... > > > > When I ported the code to 'the new way', it brought that weird aspect forward. > > IMO, the safer thing to do would be to keep the latter entry. That (I > > believe) is the one that is currently active - it will have been > > stomping the former entry. > > Sounds good to me. New patch uploaded. The change looks good, though I have a request (not required). Please remove the style .thumbnail-wrapper (see link below) that has a reference to colorSectionBorder, since it was getting the wrong format output. Since it's not being used anywhere that I can find, I'm suggesting removing it. (So it's incorrect, but since it's not used it's not having an impact). https://cs.chromium.org/chromium/src/chrome/browser/resources/ntp4/new_tab_th... Looking in that file also confirms that SkColorToRGBComponents (the latter one) is the one we want to keep, so patch#2 is an improvement over patch#1.
Description was changed from ========== NTPResourceCache don't add substitution for colorSectionBorder twice Found through static analysis by PVS-Studio! BUG=660198 ========== to ========== NTPResourceCache don't add substitution for colorSectionBorder twice Found through static analysis by PVS-Studio! BUG=660198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/10/28 23:05:54, dschuyler wrote: > The change looks good, though I have a request (not required). Please remove > the style .thumbnail-wrapper (see link below) that has a reference to > colorSectionBorder, since it was getting the wrong format output. Since > it's not being used anywhere that I can find, I'm suggesting removing it. > (So it's incorrect, but since it's not used it's not having an impact). > https://cs.chromium.org/chromium/src/chrome/browser/resources/ntp4/new_tab_th... > > Looking in that file also confirms that SkColorToRGBComponents (the latter > one) is the one we want to keep, so patch#2 is an improvement over patch#1. Done. Please take another look.
lgtm
hans@chromium.org changed reviewers: + tommycli@chromium.org
I now see estade is marked "emeritus" in the OWNERS file. tommycli: maybe you can stamp this?
thakis@chromium.org changed reviewers: + thakis@chromium.org
lgtm stamp mention the css removal in the cl description though
Description was changed from ========== NTPResourceCache don't add substitution for colorSectionBorder twice Found through static analysis by PVS-Studio! BUG=660198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== NTPResourceCache don't add substitution for colorSectionBorder twice Found through static analysis by PVS-Studio! Also remove .thumbnail-wrapper style, which refered to colorSectionBorder but wasn't used. BUG=660198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== NTPResourceCache don't add substitution for colorSectionBorder twice Found through static analysis by PVS-Studio! Also remove .thumbnail-wrapper style, which refered to colorSectionBorder but wasn't used. BUG=660198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== NTPResourceCache don't add substitution for colorSectionBorder twice Found through static analysis by PVS-Studio! Also remove .thumbnail-wrapper style, which referred to colorSectionBorder but wasn't used. BUG=660198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hans@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== NTPResourceCache don't add substitution for colorSectionBorder twice Found through static analysis by PVS-Studio! Also remove .thumbnail-wrapper style, which referred to colorSectionBorder but wasn't used. BUG=660198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== NTPResourceCache don't add substitution for colorSectionBorder twice Found through static analysis by PVS-Studio! Also remove .thumbnail-wrapper style, which referred to colorSectionBorder but wasn't used. BUG=660198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== NTPResourceCache don't add substitution for colorSectionBorder twice Found through static analysis by PVS-Studio! Also remove .thumbnail-wrapper style, which referred to colorSectionBorder but wasn't used. BUG=660198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== NTPResourceCache don't add substitution for colorSectionBorder twice Found through static analysis by PVS-Studio! Also remove .thumbnail-wrapper style, which referred to colorSectionBorder but wasn't used. BUG=660198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/c6ddf87591b0227947d16d415d3401aabb26ebef Cr-Commit-Position: refs/heads/master@{#428917} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c6ddf87591b0227947d16d415d3401aabb26ebef Cr-Commit-Position: refs/heads/master@{#428917} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
