Chromium Code Reviews
Help | Chromium Project | Sign in
(671)

Issue 10915180: Make DownloadHistory observe manager, items (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 10 months ago by benjhayden_chromium
Modified:
1 year, 7 months ago
CC:
chromium-reviews, asanka, mihaip-chromium-reviews_chromium.org, jam, browser-components-watch_chromium.org, joi+watch-content_chromium.org, Aaron Boodman, darin-cc_chromium.org, rdsmith+dwatch_chromium.org
Visibility:
Public.

Description

Make DownloadHistory observe manager, items

Rip out half of DownloadManagerDelegate.
Make DownloadManager create persisted DownloadItems one at a time and return them to DownloadHistory.
Move DownloadPersistentStoreInfo from content to chrome.
Kill DownloadDatabase::CheckThread(). (Leftover from 85408.)
Change DownloadDatabase::RemoveDownloads() to take an explicit set of db_handles.
Merge DownloadDatabase::UpdateDownload[Path]().

After this CL, I'll send out another one to remove the usage of CancelableRequest from the downloads-specific HistoryService APIs.

BUG=154309
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168670

Patch Set 1 #

Patch Set 2 : . #

Total comments: 36

Patch Set 3 : . #

Patch Set 4 : . #

Patch Set 5 : . #

Total comments: 29

Patch Set 6 : . #

Patch Set 7 : . #

Patch Set 8 : . #

Patch Set 9 : . #

Patch Set 10 : . #

Patch Set 11 : . #

Patch Set 12 : . #

Patch Set 13 : . #

Patch Set 14 : . #

Patch Set 15 : . #

Total comments: 30

Patch Set 16 : . #

Total comments: 28

Patch Set 17 : . #

Patch Set 18 : @r158560 #

Patch Set 19 : @r158560 #

Patch Set 20 : @r158560 #

Patch Set 21 : @r158560 #

Total comments: 7

Patch Set 22 : @r158560 #

Patch Set 23 : @r158560 #

Patch Set 24 : @r159248 #

Total comments: 5

Patch Set 25 : @r159425 #

Patch Set 26 : @r159474 #

Patch Set 27 : @r161344 #

Patch Set 28 : @r161344 #

Patch Set 29 : @r161344 #

Patch Set 30 : @r163544 #

Patch Set 31 : @r163634 #

Patch Set 32 : @r164337 #

Patch Set 33 : @r164337 #

Patch Set 34 : @r164337 #

Patch Set 35 : @r164555 #

Patch Set 36 : @r164555 #

Patch Set 37 : @r164626 #

Patch Set 38 : @r164910 #

Patch Set 39 : @r164910 #

Patch Set 40 : @r165352 #

Patch Set 41 : @r165669 #

Patch Set 42 : @r165669 #

Patch Set 43 : @r165669 #

Total comments: 24

Patch Set 44 : @r165938 #

Patch Set 45 : @r165938 #

Patch Set 46 : @r166227 #

Patch Set 47 : @r166227 #

Patch Set 48 : @r166419 #

Total comments: 6

Patch Set 49 : @r166491 #

Patch Set 50 : @r166491 #

Total comments: 8

Patch Set 51 : @r166491 #

Patch Set 52 : @r166680 #

Patch Set 53 : @r166680 #

Patch Set 54 : @r166680 #

Total comments: 23

Patch Set 55 : @r166680 #

Total comments: 37

Patch Set 56 : @r167172 #

Patch Set 57 : @r167172 #

Patch Set 58 : @r167172 #

Patch Set 59 : @r167172 #

Patch Set 60 : @r167172 #

Patch Set 61 : @r167394 #

Total comments: 4

Patch Set 62 : @r167394 #

Patch Set 63 : @r167666 #

Patch Set 64 : @r167666 #

Patch Set 65 : @r167927 #

Patch Set 66 : @r168179 #

Patch Set 67 : @r168573 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+2033 lines, -1549 lines) Lint Patch
M chrome/browser/download/chrome_download_manager_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 7 chunks +9 lines, -24 lines 0 comments 0 errors Download
M chrome/browser/download/chrome_download_manager_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 7 chunks +33 lines, -65 lines 0 comments ? errors Download
M chrome/browser/download/chrome_download_manager_delegate_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 1 chunk +4 lines, -0 lines 0 comments ? errors Download
M chrome/browser/download/download_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 6 chunks +52 lines, -86 lines 0 comments ? errors Download
M chrome/browser/download/download_history.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 1 chunk +117 lines, -55 lines 0 comments ? errors Download
M chrome/browser/download/download_history.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 1 chunk +385 lines, -110 lines 0 comments ? errors Download
A chrome/browser/download/download_history_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 1 chunk +741 lines, -0 lines 2 comments ? errors Download
M chrome/browser/download/download_service.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 3 chunks +9 lines, -0 lines 0 comments ? errors Download
M chrome/browser/download/download_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 4 chunks +22 lines, -0 lines 0 comments ? errors Download
M chrome/browser/download/download_status_updater_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 1 chunk +8 lines, -8 lines 0 comments ? errors Download
M chrome/browser/download/save_page_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 16 chunks +191 lines, -220 lines 0 comments 1 errors Download
M chrome/browser/extensions/api/downloads/downloads_api.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 1 chunk +0 lines, -3 lines 0 comments ? errors Download
M chrome/browser/extensions/api/downloads/downloads_api_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 9 chunks +20 lines, -22 lines 0 comments ? errors Download
M chrome/browser/history/download_database.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 4 chunks +14 lines, -23 lines 0 comments ? errors Download
M chrome/browser/history/download_database.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 10 chunks +30 lines, -107 lines 0 comments 0 errors Download
A chrome/browser/history/download_row.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 1 chunk +71 lines, -0 lines 0 comments ? errors Download
A + chrome/browser/history/download_row.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 3 chunks +8 lines, -8 lines 0 comments ? errors Download
M chrome/browser/history/history.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 4 chunks +25 lines, -36 lines 0 comments ? errors Download
M chrome/browser/history/history.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 4 chunks +6 lines, -24 lines 0 comments 0 errors Download
M chrome/browser/history/history_backend.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 2 chunks +5 lines, -12 lines 0 comments ? errors Download
M chrome/browser/history/history_backend.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 3 chunks +33 lines, -21 lines 0 comments ? errors Download
M chrome/browser/history/history_marshaling.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 1 chunk +1 line, -1 line 0 comments ? errors Download
M chrome/browser/history/history_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 5 chunks +8 lines, -77 lines 0 comments ? errors Download
M chrome/browser/ui/webui/downloads_dom_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 3 chunks +6 lines, -5 lines 0 comments ? errors Download
M chrome/browser/ui/webui/downloads_dom_handler_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 3 chunks +11 lines, -17 lines 0 comments ? errors Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 1 chunk +2 lines, -0 lines 0 comments ? errors Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 1 chunk +1 line, -0 lines 0 comments ? errors Download
M content/browser/download/download_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 3 chunks +0 lines, -11 lines 0 comments ? errors Download
M content/browser/download/download_item_factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 3 chunks +10 lines, -2 lines 0 comments ? errors Download
M content/browser/download/download_item_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 6 chunks +9 lines, -15 lines 0 comments ? errors Download
M content/browser/download/download_item_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 26 chunks +61 lines, -92 lines 0 comments ? errors Download
M content/browser/download/download_item_impl_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 1 chunk +3 lines, -3 lines 0 comments 0 errors Download
M content/browser/download/download_item_impl_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 1 chunk +1 line, -4 lines 0 comments ? errors Download
M content/browser/download/download_item_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 4 chunks +3 lines, -25 lines 0 comments ? errors Download
M content/browser/download/download_manager_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 4 chunks +11 lines, -21 lines 0 comments ? errors Download
M content/browser/download/download_manager_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 15 chunks +65 lines, -196 lines 0 comments ? errors Download
M content/browser/download/download_manager_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 9 chunks +33 lines, -54 lines 0 comments ? errors Download
M content/browser/download/download_stats.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +0 lines, -4 lines 0 comments ? errors Download
M content/browser/download/download_stats.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +0 lines, -8 lines 0 comments ? errors Download
M content/browser/download/save_package.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 1 chunk +0 lines, -1 line 0 comments ? errors Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 2 chunks +0 lines, -3 lines 0 comments ? errors Download
M content/public/browser/download_item.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 5 chunks +0 lines, -9 lines 0 comments ? errors Download
M content/public/browser/download_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 2 chunks +10 lines, -10 lines 0 comments 0 errors Download
M content/public/browser/download_manager_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 1 chunk +0 lines, -27 lines 0 comments ? errors Download
D content/public/browser/download_persistent_store_info.h View 1 chunk +0 lines, -74 lines 0 comments 0 errors Download
D content/public/browser/download_persistent_store_info.cc View 1 chunk +0 lines, -43 lines 0 comments ? errors Download
M content/public/test/mock_download_item.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 3 chunks +2 lines, -4 lines 0 comments ? errors Download
M content/public/test/mock_download_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +10 lines, -2 lines 0 comments ? errors Download
M content/shell/shell_download_manager_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 2 chunks +0 lines, -2 lines 0 comments 0 errors Download
M content/shell/shell_download_manager_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 2 chunks +1 line, -8 lines 0 comments ? errors Download
M net/base/net_log_event_type_list.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 2 chunks +2 lines, -7 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 55
benjhayden_chromium
PTAL Moved from http://codereview.chromium.org/10665049/ in order to get the automated CC line, which is only ...
1 year, 10 months ago (2012-09-10 19:08:15 UTC) #1
rdsmith
On 2012/09/10 19:08:15, benjhayden_chromium wrote: > PTAL > Moved from http://codereview.chromium.org/10665049/ in order to get ...
1 year, 10 months ago (2012-09-11 17:40:08 UTC) #2
rdsmith
Next round of comments. This is getting exciting :-). One high level question ('cause I'm ...
1 year, 10 months ago (2012-09-11 18:36:52 UTC) #3
benjhayden_chromium
PTAL, but ignore SavePackage until I figure out what to do. Maybe we can talk ...
1 year, 10 months ago (2012-09-13 15:18:16 UTC) #4
rdsmith
Looks good except for save package and the comments below. I'd recommend pulling Brett in ...
1 year, 10 months ago (2012-09-13 19:53:19 UTC) #5
benjhayden_chromium
PTAL I decided to keep CancelableRequestConsumer in DownloadHistory and not simplify our corner of HistoryService ...
1 year, 9 months ago (2012-09-21 20:45:46 UTC) #6
rdsmith
I haven't reviewed SavePackage interaction yet. When you have a chance, could you give me ...
1 year, 9 months ago (2012-09-24 18:03:24 UTC) #7
asanka
http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/chrome_download_manager_delegate.h File chrome/browser/download/chrome_download_manager_delegate.h (right): http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/chrome_download_manager_delegate.h#newcode61 chrome/browser/download/chrome_download_manager_delegate.h:61: void AddHistoryObserver(DownloadHistory::Observer* observer); Nit: Document these? http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/download_history.cc File chrome/browser/download/download_history.cc ...
1 year, 9 months ago (2012-09-24 18:39:43 UTC) #8
benjhayden_chromium
Feel free to either PTAL or wait for me to fix some broken tests. http://codereview.chromium.org/10915180/diff/28007/chrome/browser/download/chrome_download_manager_delegate.h ...
1 year, 9 months ago (2012-09-24 20:12:11 UTC) #9
rdsmith
SavePackage interaction review. I'd like it if you could do testing (manual, or ideally, automatic) ...
1 year, 9 months ago (2012-09-25 18:09:20 UTC) #10
benjhayden_chromium
Still working on tests. http://codereview.chromium.org/10915180/diff/37062/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): http://codereview.chromium.org/10915180/diff/37062/content/browser/download/download_manager_impl.cc#newcode530 content/browser/download/download_manager_impl.cc:530: download_item->AddObserver(observer); On 2012/09/25 18:09:20, rdsmith ...
1 year, 9 months ago (2012-09-25 20:22:18 UTC) #11
rdsmith
http://codereview.chromium.org/10915180/diff/37062/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): http://codereview.chromium.org/10915180/diff/37062/content/browser/download/download_manager_impl.cc#newcode538 content/browser/download/download_manager_impl.cc:538: OnDownloadTargetDetermined( On 2012/09/25 20:22:18, benjhayden_chromium wrote: > On 2012/09/25 ...
1 year, 9 months ago (2012-09-26 15:56:12 UTC) #12
benjhayden_chromium
On 2012/09/26 15:56:12, rdsmith wrote: > http://codereview.chromium.org/10915180/diff/37062/content/browser/download/download_manager_impl.cc > File content/browser/download/download_manager_impl.cc (right): > > http://codereview.chromium.org/10915180/diff/37062/content/browser/download/download_manager_impl.cc#newcode538 > ...
1 year, 9 months ago (2012-09-28 13:51:13 UTC) #13
benjhayden_chromium
ShowDownloadInBrowser appears to have been the only thing that CreateSavePackageItem needed to do. I think ...
1 year, 9 months ago (2012-09-28 17:00:06 UTC) #14
asanka
http://codereview.chromium.org/10915180/diff/55002/content/browser/download/download_item_impl.cc File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/10915180/diff/55002/content/browser/download/download_item_impl.cc#newcode892 content/browser/download/download_item_impl.cc:892: bound_net_log_.AddEvent( EndEvent() http://codereview.chromium.org/10915180/diff/55002/content/browser/download/download_item_impl.cc#newcode1030 content/browser/download/download_item_impl.cc:1030: UpdateObservers(); Given that with this ...
1 year, 9 months ago (2012-09-28 19:02:30 UTC) #15
benjhayden_chromium
sky, csilv, creis, jochen: PTAL Still working some kinks out of the changes to */download/*, ...
1 year, 9 months ago (2012-10-01 14:03:36 UTC) #16
benjhayden_chromium
http://codereview.chromium.org/10915180/diff/55002/content/browser/download/download_item_impl.cc File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/10915180/diff/55002/content/browser/download/download_item_impl.cc#newcode892 content/browser/download/download_item_impl.cc:892: bound_net_log_.AddEvent( On 2012/09/28 19:02:30, asanka wrote: > EndEvent() Can ...
1 year, 9 months ago (2012-10-01 14:10:03 UTC) #17
asanka
http://codereview.chromium.org/10915180/diff/55002/content/browser/download/download_item_impl.cc File content/browser/download/download_item_impl.cc (right): http://codereview.chromium.org/10915180/diff/55002/content/browser/download/download_item_impl.cc#newcode892 content/browser/download/download_item_impl.cc:892: bound_net_log_.AddEvent( On 2012/10/01 14:10:03, benjhayden_chromium wrote: > On 2012/09/28 ...
1 year, 9 months ago (2012-10-01 15:59:36 UTC) #18
rdsmith
The save package interface looks good. I'm holding off on doing other review until you ...
1 year, 9 months ago (2012-10-01 17:19:29 UTC) #19
csilv
/webui/ lgtm
1 year, 9 months ago (2012-10-01 17:29:52 UTC) #20
Charlie Reis (OOO)
content/content_browser.gypi LGTM. nit: Can you add a BUG= line to the CL description? There's a ...
1 year, 9 months ago (2012-10-01 21:01:01 UTC) #21
jochen
content/shell lgtm
1 year, 9 months ago (2012-10-09 20:58:19 UTC) #22
csilv
ui/webui/downloads_dom_handler_browsertest.cc lgtm
1 year, 9 months ago (2012-10-09 21:19:00 UTC) #23
benjhayden_chromium
If you've already LG'd, then you do not need to look again. Randy, Asanka, PTAL.
1 year, 8 months ago (2012-10-30 20:25:05 UTC) #24
rdsmith
On 2012/10/30 20:25:05, benjhayden_chromium wrote: > If you've already LG'd, then you do not need ...
1 year, 8 months ago (2012-10-30 22:26:21 UTC) #25
benjhayden_chromium
PTAL while I look at the tests. http://codereview.chromium.org/10915180/diff/15001/content/browser/download/download_manager_impl.cc File content/browser/download/download_manager_impl.cc (right): http://codereview.chromium.org/10915180/diff/15001/content/browser/download/download_manager_impl.cc#newcode619 content/browser/download/download_manager_impl.cc:619: // If ...
1 year, 8 months ago (2012-11-02 17:21:36 UTC) #26
asanka
http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/download_history.cc File chrome/browser/download/download_history.cc (right): http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/download_history.cc#newcode269 chrome/browser/download/download_history.cc:269: void DownloadHistory::ItemAdded(int32 download_id, int64 db_handle) { If CreateDownload() failed, ...
1 year, 8 months ago (2012-11-02 19:56:57 UTC) #27
rdsmith
I didn't see an answer to my earlier question as to whether you were satisfied ...
1 year, 8 months ago (2012-11-02 23:31:24 UTC) #28
benjhayden_chromium
Yes, I just went through all the tests changed in this CL and convinced myself ...
1 year, 8 months ago (2012-11-06 20:01:13 UTC) #29
benjhayden_chromium
Test done. PTAL at that if you haven't already LG'd. Thanks! http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/download_history_unittest.cc File chrome/browser/download/download_history_unittest.cc (right): ...
1 year, 8 months ago (2012-11-07 16:39:55 UTC) #30
rdsmith
I haven't reviewed the tests yet, but I'm feeling a bit fried, so I'll toss ...
1 year, 8 months ago (2012-11-07 21:10:29 UTC) #31
benjhayden_chromium
PTAL if you haven't already LG'd. http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/chrome_download_manager_delegate.h File chrome/browser/download/chrome_download_manager_delegate.h (right): http://codereview.chromium.org/10915180/diff/101003/chrome/browser/download/chrome_download_manager_delegate.h#newcode63 chrome/browser/download/chrome_download_manager_delegate.h:63: void RemoveHistoryObserver(DownloadHistory::Observer* observer); ...
1 year, 8 months ago (2012-11-08 18:57:03 UTC) #32
benjhayden_chromium
Scott asked that I use an adapter class between HistoryService and DownloadHistory instead of mocking ...
1 year, 8 months ago (2012-11-08 19:11:06 UTC) #33
benjhayden_chromium
On 2012/11/08 19:11:06, benjhayden_chromium wrote: > Scott asked that I use an adapter class between ...
1 year, 8 months ago (2012-11-08 19:56:36 UTC) #34
sky
https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/download_database.cc File chrome/browser/history/download_database.cc (right): https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/download_database.cc#newcode257 chrome/browser/history/download_database.cc:257: void DownloadDatabase::RemoveDownloads(const std::set<DownloadID>& handles) { If this is going ...
1 year, 8 months ago (2012-11-08 20:55:17 UTC) #35
benjhayden_chromium
http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/history.h File chrome/browser/history/history.h (right): http://codereview.chromium.org/10915180/diff/101003/chrome/browser/history/history.h#newcode455 chrome/browser/history/history.h:455: virtual Handle CreateDownload( On 2012/11/07 21:10:29, rdsmith wrote: > ...
1 year, 8 months ago (2012-11-08 21:40:03 UTC) #36
asanka
http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/chrome_download_manager_delegate_unittest.cc File chrome/browser/download/chrome_download_manager_delegate_unittest.cc (right): http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/chrome_download_manager_delegate_unittest.cc#newcode307 chrome/browser/download/chrome_download_manager_delegate_unittest.cc:307: .WillRepeatedly(Return()); Nit: indent http://codereview.chromium.org/10915180/diff/136064/chrome/browser/download/chrome_download_manager_delegate_unittest.cc#newcode341 chrome/browser/download/chrome_download_manager_delegate_unittest.cc:341: .WillRepeatedly(Return()); These expectations are ...
1 year, 8 months ago (2012-11-08 22:45:53 UTC) #37
sky
https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/download_database.cc File chrome/browser/history/download_database.cc (right): https://codereview.chromium.org/10915180/diff/136064/chrome/browser/history/download_database.cc#newcode257 chrome/browser/history/download_database.cc:257: void DownloadDatabase::RemoveDownloads(const std::set<DownloadID>& handles) { On 2012/11/08 21:40:03, benjhayden_chromium ...
1 year, 8 months ago (2012-11-09 16:45:50 UTC) #38
rdsmith
Could you skim over the logging statements you've added in this CL and just confirm ...
1 year, 8 months ago (2012-11-09 21:36:39 UTC) #39
benjhayden_chromium
All the log statements that I added are VLOG(20), so they should only bother folks ...
1 year, 8 months ago (2012-11-12 18:44:16 UTC) #40
rdsmith
LGTM.q http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/save_page_browsertest.cc File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/10915180/diff/128069/chrome/browser/download/save_page_browsertest.cc#newcode145 chrome/browser/download/save_page_browsertest.cc:145: if (info.path != expected_path) { On 2012/11/12 18:44:16, ...
1 year, 8 months ago (2012-11-12 19:24:30 UTC) #41
benjhayden_chromium
http://codereview.chromium.org/10915180/diff/128069/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right): http://codereview.chromium.org/10915180/diff/128069/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc#newcode954 chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:954: EXPECT_FALSE(download_item->GetTargetFilePath().empty()); On 2012/11/12 19:24:30, rdsmith wrote: > On 2012/11/12 ...
1 year, 8 months ago (2012-11-12 19:59:53 UTC) #42
asanka
LGTM
1 year, 8 months ago (2012-11-12 22:51:13 UTC) #43
sky
https://codereview.chromium.org/10915180/diff/136085/chrome/browser/history/download_database.cc File chrome/browser/history/download_database.cc (right): https://codereview.chromium.org/10915180/diff/136085/chrome/browser/history/download_database.cc#newcode26 chrome/browser/history/download_database.cc:26: const int64 DownloadDatabase::kUninitializedHandle = -1; nit: for static we ...
1 year, 8 months ago (2012-11-13 20:45:51 UTC) #44
benjhayden_chromium
https://codereview.chromium.org/10915180/diff/136085/chrome/browser/history/download_database.cc File chrome/browser/history/download_database.cc (right): https://codereview.chromium.org/10915180/diff/136085/chrome/browser/history/download_database.cc#newcode26 chrome/browser/history/download_database.cc:26: const int64 DownloadDatabase::kUninitializedHandle = -1; On 2012/11/13 20:45:52, sky ...
1 year, 8 months ago (2012-11-13 21:24:13 UTC) #45
sky
LGTM
1 year, 8 months ago (2012-11-13 22:05:14 UTC) #46
benjhayden_chromium
Thanks everybody! I'll make a final self-review, run the trybots, and commit tomorrow.
1 year, 8 months ago (2012-11-13 22:18:32 UTC) #47
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10915180/130145
1 year, 8 months ago (2012-11-14 18:01:00 UTC) #48
I haz the power (commit-bot)
Retried try job too often for step(s) browser_tests
1 year, 8 months ago (2012-11-14 20:56:04 UTC) #49
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10915180/172001
1 year, 7 months ago (2012-11-19 21:23:44 UTC) #50
I haz the power (commit-bot)
Retried try job too often for step(s) interactive_ui_tests
1 year, 7 months ago (2012-11-19 22:27:34 UTC) #51
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10915180/172001
1 year, 7 months ago (2012-11-19 22:30:00 UTC) #52
I haz the power (commit-bot)
Change committed as 168670
1 year, 7 months ago (2012-11-20 00:52:14 UTC) #53
rdsmith
https://chromiumcodereview.appspot.com/10915180/diff/172001/chrome/browser/download/download_history_unittest.cc File chrome/browser/download/download_history_unittest.cc (right): https://chromiumcodereview.appspot.com/10915180/diff/172001/chrome/browser/download/download_history_unittest.cc#newcode351 chrome/browser/download/download_history_unittest.cc:351: EXPECT_CALL(item(index), GetFullPath()).WillRepeatedly(ReturnRef(path)); Apologies for not catching this on the ...
1 year, 7 months ago (2012-12-04 21:48:32 UTC) #54
rdsmith
1 year, 7 months ago (2012-12-05 13:00:29 UTC) #55
Message was sent while issue was closed.
https://codereview.chromium.org/10915180/diff/172001/chrome/browser/download/...
File chrome/browser/download/download_history_unittest.cc (right):

https://codereview.chromium.org/10915180/diff/172001/chrome/browser/download/...
chrome/browser/download/download_history_unittest.cc:351:
EXPECT_CALL(item(index), GetFullPath()).WillRepeatedly(ReturnRef(path));
On 2012/12/04 21:48:32, rdsmith wrote:
> Apologies for not catching this on the first time through, but I *think* this
is
> a bug.  Aren't you storing a ref to an automatic variable, which means you're
> effectively storing a pointer to a random location on the stack after the
> function returns?

Whoops, I see; the argument to the function is a reference, and you're relying
on the caller to keep the reference from going out of scope while item(index) is
around.  I'm inclined to think that should take a comment, but I'll put it in as
part of my current CL.  Sorry to bug you.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1357:84bb59c808bb