Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1915513002:
Fixed invalid parameter used by most visited thumbnail. (Closed)
|
Created:
4 years, 8 months ago by Alexander Yashkin Modified:
4 years, 7 months ago Reviewers:
Marc Treib CC:
fserb, huangs, chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixed invalid parameter used by most visited thumbnail.
After https://codereview.chromium.org/1260113002 most visited sites
thumbnails on non Google newtab page are displayed incorrectly.
I beleive that the cause of this is rename of parameter "thumbnailUrl"
to "thumbnailUrls" in searchbox_extension.cc. I tried to
implement simple fix for it.
BUG=596285
R=fserb@chromium.org, huangs@chromium.org
Committed: https://crrev.com/e18fcb44299b1d344a0d25a418e76e37596ccb26
Cr-Commit-Position: refs/heads/master@{#392401}
Patch Set 1 #Patch Set 2 : Added check for thumbnailUrls.length #Messages
Total messages: 22 (9 generated)
On 2016/04/22 15:04:08, a-v-y wrote: PTAL fserb, huangs
Description was changed from ========== Fixed invalid parameter used by most visited thumbnail. After https://codereview.chromium.org/1260113002 most visited sites thumbnails on non Google newtab page are displayed incorrectly. I beleive that the cause of this is rename of parameter "thumbnailUrl" to "thumbnailUrls" in searchbox_extension.cc. I tried to implement simple fix for it. BUG=605939 R=fserb@chromium.org, huangs@chromium.org ========== to ========== Fixed invalid parameter used by most visited thumbnail. After https://codereview.chromium.org/1260113002 most visited sites thumbnails on non Google newtab page are displayed incorrectly. I beleive that the cause of this is rename of parameter "thumbnailUrl" to "thumbnailUrls" in searchbox_extension.cc. I tried to implement simple fix for it. BUG=605939 R=fserb@chromium.org, huangs@chromium.org ==========
a-v-y@yandex-team.ru changed reviewers: + jered@chromium.org
On 2016/04/28 08:20:42, a-v-y wrote: > On 2016/04/22 15:04:08, a-v-y wrote: > > PTAL fserb, huangs PTAL Jered
jered@chromium.org changed reviewers: + treib@chromium.org - fserb@chromium.org, huangs@chromium.org, jered@chromium.org
On 2016/05/04 07:46:16, a-v-y wrote: > On 2016/04/28 08:20:42, a-v-y wrote: > > On 2016/04/22 15:04:08, a-v-y wrote: > > > > PTAL fserb, huangs > > PTAL Jered I believe the fix looks correct. I will forward the review to Marc who's active in chromium and may know more about what's going on in this code.
On 2016/05/04 13:12:53, Jered wrote: > On 2016/05/04 07:46:16, a-v-y wrote: > > On 2016/04/28 08:20:42, a-v-y wrote: > > > On 2016/04/22 15:04:08, a-v-y wrote: > > > > > > PTAL fserb, huangs > > > > PTAL Jered > > I believe the fix looks correct. I will forward the review to Marc who's active > in chromium and may know more about what's going on in this code. Thanks for looking into this! Kinda worrying that nobody noticed for 9 months... I guess that's because it won't actually happen for all other search engines, only for those that implement a custom NTP. Bing does that, but the Bing NTP doesn't show thumbnails at all. I wasn't even aware that any other search engine had a custom NTP. Since your bug has been merged into the other one, please update the CL description to mention that one. Then LGTM :)
Description was changed from ========== Fixed invalid parameter used by most visited thumbnail. After https://codereview.chromium.org/1260113002 most visited sites thumbnails on non Google newtab page are displayed incorrectly. I beleive that the cause of this is rename of parameter "thumbnailUrl" to "thumbnailUrls" in searchbox_extension.cc. I tried to implement simple fix for it. BUG=605939 R=fserb@chromium.org, huangs@chromium.org ========== to ========== Fixed invalid parameter used by most visited thumbnail. After https://codereview.chromium.org/1260113002 most visited sites thumbnails on non Google newtab page are displayed incorrectly. I beleive that the cause of this is rename of parameter "thumbnailUrl" to "thumbnailUrls" in searchbox_extension.cc. I tried to implement simple fix for it. BUG=596285 R=fserb@chromium.org, huangs@chromium.org ==========
On 2016/05/04 14:28:17, Marc Treib wrote: > On 2016/05/04 13:12:53, Jered wrote: > > On 2016/05/04 07:46:16, a-v-y wrote: > > > On 2016/04/28 08:20:42, a-v-y wrote: > > > > On 2016/04/22 15:04:08, a-v-y wrote: > > > > > > > > PTAL fserb, huangs > > > > > > PTAL Jered > > > > I believe the fix looks correct. I will forward the review to Marc who's > active > > in chromium and may know more about what's going on in this code. > > Thanks for looking into this! Kinda worrying that nobody noticed for 9 months... > I guess that's because it won't actually happen for all other search engines, > only for those that implement a custom NTP. Bing does that, but the Bing NTP > doesn't show thumbnails at all. I wasn't even aware that any other search engine > had a custom NTP. > > Since your bug has been merged into the other one, please update the CL > description to mention that one. Then LGTM :) Thanks for quick response. CL description updated. Actually we only started to use chrome NTP recently, since December of 2015. https://codereview.chromium.org/1524723002
The CQ bit was checked by a-v-y@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1915513002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1915513002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/05/04 14:44:58, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Hah, turns out I don't actually own this folder (yet). Jered, can you approve?
On 2016/05/04 15:07:48, Marc Treib wrote: > On 2016/05/04 14:44:58, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > Hah, turns out I don't actually own this folder (yet). Jered, can you approve? Update: I am now a proud owner of the local_ntp folder. You should be able to submit this now!
The CQ bit was checked by a-v-y@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1915513002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1915513002/20001
Message was sent while issue was closed.
Description was changed from ========== Fixed invalid parameter used by most visited thumbnail. After https://codereview.chromium.org/1260113002 most visited sites thumbnails on non Google newtab page are displayed incorrectly. I beleive that the cause of this is rename of parameter "thumbnailUrl" to "thumbnailUrls" in searchbox_extension.cc. I tried to implement simple fix for it. BUG=596285 R=fserb@chromium.org, huangs@chromium.org ========== to ========== Fixed invalid parameter used by most visited thumbnail. After https://codereview.chromium.org/1260113002 most visited sites thumbnails on non Google newtab page are displayed incorrectly. I beleive that the cause of this is rename of parameter "thumbnailUrl" to "thumbnailUrls" in searchbox_extension.cc. I tried to implement simple fix for it. BUG=596285 R=fserb@chromium.org, huangs@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fixed invalid parameter used by most visited thumbnail. After https://codereview.chromium.org/1260113002 most visited sites thumbnails on non Google newtab page are displayed incorrectly. I beleive that the cause of this is rename of parameter "thumbnailUrl" to "thumbnailUrls" in searchbox_extension.cc. I tried to implement simple fix for it. BUG=596285 R=fserb@chromium.org, huangs@chromium.org ========== to ========== Fixed invalid parameter used by most visited thumbnail. After https://codereview.chromium.org/1260113002 most visited sites thumbnails on non Google newtab page are displayed incorrectly. I beleive that the cause of this is rename of parameter "thumbnailUrl" to "thumbnailUrls" in searchbox_extension.cc. I tried to implement simple fix for it. BUG=596285 R=fserb@chromium.org, huangs@chromium.org Committed: https://crrev.com/e18fcb44299b1d344a0d25a418e76e37596ccb26 Cr-Commit-Position: refs/heads/master@{#392401} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e18fcb44299b1d344a0d25a418e76e37596ccb26 Cr-Commit-Position: refs/heads/master@{#392401} |
