Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2786833003:
NTP thumbnails: Always pass singular "thumbnailUrl" to the iframe (Closed)
|
Created:
3 years, 8 months ago by Marc Treib Modified:
3 years, 8 months ago Reviewers:
sfiera CC:
chromium-reviews, extensions-reviews_chromium.org, skanuj+watch_chromium.org, melevin+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, chromium-apps-reviews_chromium.org, kmadhusu+watch_chromium.org, Jered, mastiz Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNTP thumbnails: Always pass singular "thumbnailUrl" to the iframe
rather than the plural "thumbnailUrls".
This allows us to get rid of a whole bunch of ugly JS code.
The remote NTP has passed "thumbnailUrl" (with a "?fb=" param for the
remote fallback thumbnail) for ML suggestions for a long time, but
TopSites suggestions used "thumbnailUrls" (but with only ever a single
value in the array).
With NTPTilesInInstantService enabled, before this CL all suggestions
used the plural version, but never with more than two entries. Now it's
always the singular version, with an "?fb=" param if appropriate.
I've checked that local NTP, remote NTP, and third-party NTPs still work.
BUG=703165
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2786833003
Cr-Commit-Position: refs/heads/master@{#461401}
Committed: https://chromium.googlesource.com/chromium/src/+/92005c86d523ba4d37d4c2ff60eb05b385bb5c38
Patch Set 1 #Patch Set 2 : . #
Total comments: 4
Messages
Total messages: 23 (13 generated)
Description was changed from ========== NTP thumbnails: Always pass singular "thumbnailUrl" to the iframe rather than the plural "thumbnailUrls". This allows us to get rid of a whole bunch of ugly JS code. The remote NTP has passed "thumbnailUrl" (with a "?fb=" param for the remote fallback thumbnail) for ML suggestions for a long time, but TopSites suggestions used "thumbnailUrls" (but with only ever a single value in the array). With NTPTilesInInstantService enabled, before this CL all suggestions used the plural version, but never with more than two entries. Now it's always the singular version, with an "?fb=" param if appropriate. BUG=703165 ========== to ========== NTP thumbnails: Always pass singular "thumbnailUrl" to the iframe rather than the plural "thumbnailUrls". This allows us to get rid of a whole bunch of ugly JS code. The remote NTP has passed "thumbnailUrl" (with a "?fb=" param for the remote fallback thumbnail) for ML suggestions for a long time, but TopSites suggestions used "thumbnailUrls" (but with only ever a single value in the array). With NTPTilesInInstantService enabled, before this CL all suggestions used the plural version, but never with more than two entries. Now it's always the singular version, with an "?fb=" param if appropriate. BUG=703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== NTP thumbnails: Always pass singular "thumbnailUrl" to the iframe rather than the plural "thumbnailUrls". This allows us to get rid of a whole bunch of ugly JS code. The remote NTP has passed "thumbnailUrl" (with a "?fb=" param for the remote fallback thumbnail) for ML suggestions for a long time, but TopSites suggestions used "thumbnailUrls" (but with only ever a single value in the array). With NTPTilesInInstantService enabled, before this CL all suggestions used the plural version, but never with more than two entries. Now it's always the singular version, with an "?fb=" param if appropriate. BUG=703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== NTP thumbnails: Always pass singular "thumbnailUrl" to the iframe rather than the plural "thumbnailUrls". This allows us to get rid of a whole bunch of ugly JS code. The remote NTP has passed "thumbnailUrl" (with a "?fb=" param for the remote fallback thumbnail) for ML suggestions for a long time, but TopSites suggestions used "thumbnailUrls" (but with only ever a single value in the array). With NTPTilesInInstantService enabled, before this CL all suggestions used the plural version, but never with more than two entries. Now it's always the singular version, with an "?fb=" param if appropriate. I've checked that local NTP, remote NTP, and third-party NTPs still work. BUG=703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
treib@chromium.org changed reviewers: + sfiera@chromium.org
The NTP just keeps on giving... I ran into this inconsistency while trying to (re-)add thumbnail type metrics. sfiera: Please review. mastiz: FYI. Remember how I said we don't cache remote thumbnails? Turns out we *could*, without too much trouble.
Have you checked that the histograms still work right? https://codereview.chromium.org/2786833003/diff/20001/chrome/renderer/searchb... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/2786833003/diff/20001/chrome/renderer/searchb... chrome/renderer/searchbox/searchbox_extension.cc:114: base::StringPrintf("chrome-search://thumb2/%s?fb=%s", No url-escaping?
Good call - no, I haven't checked the histograms yet. Will do and report back! https://codereview.chromium.org/2786833003/diff/20001/chrome/renderer/searchb... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/2786833003/diff/20001/chrome/renderer/searchb... chrome/renderer/searchbox/searchbox_extension.cc:114: base::StringPrintf("chrome-search://thumb2/%s?fb=%s", On 2017/03/30 13:40:58, sfiera wrote: > No url-escaping? Nope :-/ Example of what these URLs look like (from the current live remote NTP): chrome-search://thumb2/https://codereview.chromium.org/?fb=https://www.google.com/webpagethumbnail?c=63&d=https://codereview.chromium.org/&r=4&s=148:94&a=<someID> The code that handles these is here: https://cs.chromium.org/chromium/src/chrome/browser/search/thumbnail_source.c... And the corresponding code for the remote NTP: newtab/design.js:202
https://codereview.chromium.org/2786833003/diff/20001/chrome/renderer/searchb... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/2786833003/diff/20001/chrome/renderer/searchb... chrome/renderer/searchbox/searchbox_extension.cc:114: base::StringPrintf("chrome-search://thumb2/%s?fb=%s", On 2017/03/30 13:51:03, Marc Treib wrote: > On 2017/03/30 13:40:58, sfiera wrote: > > No url-escaping? > > Nope :-/ > > Example of what these URLs look like (from the current live remote NTP): > chrome-search://thumb2/https://codereview.chromium.org/?fb=https://www.google.com/webpagethumbnail?c=63&d=https://codereview.chromium.org/&r=4&s=148:94&a=<someID> > > The code that handles these is here: > https://cs.chromium.org/chromium/src/chrome/browser/search/thumbnail_source.c... > > And the corresponding code for the remote NTP: newtab/design.js:202 Ah, so any tile with ?fb= in its URL is probably already broken…
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2786833003/diff/20001/chrome/renderer/searchb... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/2786833003/diff/20001/chrome/renderer/searchb... chrome/renderer/searchbox/searchbox_extension.cc:114: base::StringPrintf("chrome-search://thumb2/%s?fb=%s", On 2017/03/30 13:55:19, sfiera wrote: > On 2017/03/30 13:51:03, Marc Treib wrote: > > On 2017/03/30 13:40:58, sfiera wrote: > > > No url-escaping? > > > > Nope :-/ > > > > Example of what these URLs look like (from the current live remote NTP): > > > chrome-search://thumb2/https://codereview.chromium.org/?fb=https://www.google.com/webpagethumbnail?c=63&d=https://codereview.chromium.org/&r=4&s=148:94&a=<someID> > > > > The code that handles these is here: > > > https://cs.chromium.org/chromium/src/chrome/browser/search/thumbnail_source.c... > > > > And the corresponding code for the remote NTP: newtab/design.js:202 > > Ah, so any tile with ?fb= in its URL is probably already broken… Argh, yes - I didn't even notice. Though I think we strip params from TopSites/ML URLs, so it shouldn't actually happen in practice. Still, yes, this is broken, like so many things...
Verified: NewTabPage.SuggestionsImpression.* still work.
Ping!
Oh, I thought I said LGTM earlier.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by treib@chromium.org
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491213973208760,
"parent_rev": "c696a96c4a0ae7de998a9d30bc2c27a5080e7b19", "commit_rev":
"92005c86d523ba4d37d4c2ff60eb05b385bb5c38"}
Message was sent while issue was closed.
Description was changed from ========== NTP thumbnails: Always pass singular "thumbnailUrl" to the iframe rather than the plural "thumbnailUrls". This allows us to get rid of a whole bunch of ugly JS code. The remote NTP has passed "thumbnailUrl" (with a "?fb=" param for the remote fallback thumbnail) for ML suggestions for a long time, but TopSites suggestions used "thumbnailUrls" (but with only ever a single value in the array). With NTPTilesInInstantService enabled, before this CL all suggestions used the plural version, but never with more than two entries. Now it's always the singular version, with an "?fb=" param if appropriate. I've checked that local NTP, remote NTP, and third-party NTPs still work. BUG=703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== NTP thumbnails: Always pass singular "thumbnailUrl" to the iframe rather than the plural "thumbnailUrls". This allows us to get rid of a whole bunch of ugly JS code. The remote NTP has passed "thumbnailUrl" (with a "?fb=" param for the remote fallback thumbnail) for ML suggestions for a long time, but TopSites suggestions used "thumbnailUrls" (but with only ever a single value in the array). With NTPTilesInInstantService enabled, before this CL all suggestions used the plural version, but never with more than two entries. Now it's always the singular version, with an "?fb=" param if appropriate. I've checked that local NTP, remote NTP, and third-party NTPs still work. BUG=703165 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2786833003 Cr-Commit-Position: refs/heads/master@{#461401} Committed: https://chromium.googlesource.com/chromium/src/+/92005c86d523ba4d37d4c2ff60eb... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/92005c86d523ba4d37d4c2ff60eb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
