Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(371)

Issue 12732005: Most visited thumbnails and favicons need id-based urls (Closed)

Created:
7 years, 9 months ago by dhollowa
Modified:
7 years, 9 months ago
Reviewers:
palmer, sreeram, Evan Stade
CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, melevin, sreeram, chromium-apps-reviews_chromium.org, dominich, Aaron Boodman, David Black, samarth+watch_chromium.org, darin-cc_chromium.org, estade+watch_chromium.org, Jered, pedrosimonetti+watch_chromium.org
Visibility:
Public.

Description

Most visited thumbnails and favicons need id-based urls Extends ThumbnailSouce and FaviconSource to serve requests of the form: chrome-search://favicon/<id> chrome-search://thumb/<id> BUG=180371 TEST=Manual, look at NTP Most Visited on 1993 R=sreeram@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187480

Patch Set 1 #

Patch Set 2 : Adds proper handling of ThumbnailSource #

Total comments: 39

Patch Set 3 : Address estade's comments. #

Patch Set 4 : Addresses Sreeram's comments, culls IDs/URLs. #

Patch Set 5 : Renaming ids and address Chris' comments. #

Patch Set 6 : Rebase #

Patch Set 7 : Better string conversion. #

Patch Set 8 : Rebase with Sreeram's MV clicks change #

Total comments: 6

Patch Set 9 : Address estade's comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+486 lines, -158 lines) Patch
M chrome/browser/instant/instant_controller.h View 1 2 3 4 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/instant/instant_controller.cc View 1 2 3 4 5 6 7 5 chunks +36 lines, -7 lines 0 comments Download
M chrome/browser/instant/instant_io_context.h View 1 2 3 4 3 chunks +29 lines, -0 lines 0 comments Download
M chrome/browser/instant/instant_io_context.cc View 1 2 3 4 4 chunks +63 lines, -2 lines 0 comments Download
M chrome/browser/instant/instant_page.h View 1 2 3 4 3 chunks +5 lines, -5 lines 0 comments Download
M chrome/browser/instant/instant_page.cc View 1 2 3 4 3 chunks +9 lines, -9 lines 0 comments Download
M chrome/browser/instant/instant_service.h View 1 2 3 4 5 6 7 8 5 chunks +45 lines, -0 lines 0 comments Download
M chrome/browser/instant/instant_service.cc View 1 2 3 4 5 6 7 8 6 chunks +164 lines, -9 lines 0 comments Download
M chrome/browser/ui/webui/favicon_source.cc View 1 2 3 4 5 6 7 8 3 chunks +14 lines, -4 lines 0 comments Download
M chrome/browser/ui/webui/ntp/thumbnail_source.h View 1 2 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/ntp/thumbnail_source.cc View 1 2 3 4 5 6 7 5 chunks +24 lines, -4 lines 0 comments Download
M chrome/common/instant_types.h View 1 2 3 4 1 chunk +9 lines, -4 lines 0 comments Download
M chrome/common/render_messages.h View 1 2 3 4 2 chunks +9 lines, -8 lines 0 comments Download
M chrome/renderer/resources/extensions/searchbox_api.js View 1 2 3 4 5 6 7 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/renderer/searchbox/searchbox.h View 1 2 3 4 5 6 7 4 chunks +9 lines, -20 lines 0 comments Download
M chrome/renderer/searchbox/searchbox.cc View 1 2 3 4 5 6 7 6 chunks +13 lines, -53 lines 0 comments Download
M chrome/renderer/searchbox/searchbox_extension.cc View 1 2 3 4 5 6 7 4 chunks +43 lines, -26 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
dhollowa
This is not quite ready for full review. Sreeram, posting so you can advise on ...
7 years, 9 months ago (2013-03-09 00:03:10 UTC) #1
dhollowa
Sreeram, the ThumbnailSource is working on both UI and IO threads now. PTAL.
7 years, 9 months ago (2013-03-09 01:43:17 UTC) #2
dhollowa
+palmer -> chrome/common/render_messages.h +estade -> chrome/browser/ui/webui/...
7 years, 9 months ago (2013-03-11 15:19:38 UTC) #3
dhollowa
On 2013/03/11 15:19:38, dhollowa wrote: > +palmer -> chrome/common/render_messages.h > +estade -> chrome/browser/ui/webui/...
7 years, 9 months ago (2013-03-11 16:10:29 UTC) #4
sreeram
First glance looks okay; will review in more detail shortly. Main comment is that the ...
7 years, 9 months ago (2013-03-11 17:18:18 UTC) #5
Evan Stade
https://codereview.chromium.org/12732005/diff/3001/chrome/browser/instant/instant_service.h File chrome/browser/instant/instant_service.h (right): https://codereview.chromium.org/12732005/diff/3001/chrome/browser/instant/instant_service.h#newcode52 chrome/browser/instant/instant_service.h:52: // Returns true if the |url| is known, and ...
7 years, 9 months ago (2013-03-11 19:47:07 UTC) #6
dhollowa
https://codereview.chromium.org/12732005/diff/3001/chrome/browser/instant/instant_service.h File chrome/browser/instant/instant_service.h (right): https://codereview.chromium.org/12732005/diff/3001/chrome/browser/instant/instant_service.h#newcode52 chrome/browser/instant/instant_service.h:52: // Returns true if the |url| is known, and ...
7 years, 9 months ago (2013-03-11 20:05:23 UTC) #7
palmer
https://codereview.chromium.org/12732005/diff/3001/chrome/browser/instant/instant_controller.cc File chrome/browser/instant/instant_controller.cc (right): https://codereview.chromium.org/12732005/diff/3001/chrome/browser/instant/instant_controller.cc#newcode200 chrome/browser/instant/instant_controller.cc:200: bool GetURLForRestrictedId(Profile* profile, uint64 restricted_id, GURL* url) { |profile| ...
7 years, 9 months ago (2013-03-11 20:42:31 UTC) #8
Evan Stade
https://codereview.chromium.org/12732005/diff/3001/chrome/browser/instant/instant_service.h File chrome/browser/instant/instant_service.h (right): https://codereview.chromium.org/12732005/diff/3001/chrome/browser/instant/instant_service.h#newcode48 chrome/browser/instant/instant_service.h:48: // If |url| is known the exiting restricted ID ...
7 years, 9 months ago (2013-03-11 21:01:27 UTC) #9
dhollowa
On 2013/03/11 17:18:18, sreeram wrote: > First glance looks okay; will review in more detail ...
7 years, 9 months ago (2013-03-11 21:29:27 UTC) #10
sreeram
instant and searchbox lgtm
7 years, 9 months ago (2013-03-11 22:39:12 UTC) #11
dhollowa
https://codereview.chromium.org/12732005/diff/3001/chrome/browser/instant/instant_controller.cc File chrome/browser/instant/instant_controller.cc (right): https://codereview.chromium.org/12732005/diff/3001/chrome/browser/instant/instant_controller.cc#newcode200 chrome/browser/instant/instant_controller.cc:200: bool GetURLForRestrictedId(Profile* profile, uint64 restricted_id, GURL* url) { On ...
7 years, 9 months ago (2013-03-11 23:27:59 UTC) #12
palmer
Personally I would make the performance improvement before committing. But not doing so is not ...
7 years, 9 months ago (2013-03-11 23:47:17 UTC) #13
dhollowa
On 2013/03/11 23:47:17, Chris P. wrote: > Personally I would make the performance improvement before ...
7 years, 9 months ago (2013-03-11 23:56:45 UTC) #14
gideonwald
Evan, your approval here would likely let us make the Dev cut. Any chance you ...
7 years, 9 months ago (2013-03-11 23:59:05 UTC) #15
Evan Stade
lgtm https://codereview.chromium.org/12732005/diff/21005/chrome/browser/instant/instant_service.h File chrome/browser/instant/instant_service.h (right): https://codereview.chromium.org/12732005/diff/21005/chrome/browser/instant/instant_service.h#newcode81 chrome/browser/instant/instant_service.h:81: // IDs if |all_history| is true. |deleted_ids| is ...
7 years, 9 months ago (2013-03-12 01:30:23 UTC) #16
dhollowa
Thanks for the fast turn-around Evan. Much appreciated. https://codereview.chromium.org/12732005/diff/21005/chrome/browser/instant/instant_service.h File chrome/browser/instant/instant_service.h (right): https://codereview.chromium.org/12732005/diff/21005/chrome/browser/instant/instant_service.h#newcode81 chrome/browser/instant/instant_service.h:81: // ...
7 years, 9 months ago (2013-03-12 01:48:07 UTC) #17
dhollowa
7 years, 9 months ago (2013-03-12 03:56:15 UTC) #18
Message was sent while issue was closed.
Committed patchset #9 manually as r187480.

Powered by Google App Engine
This is Rietveld 408576698