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

Issue 6320007: Updates filebrowser list when download starts/ends. (Closed)

Created:
9 years, 11 months ago by altimofeev
Modified:
9 years, 7 months ago
Reviewers:
xiyuan, achuithb
CC:
chromium-reviews
Visibility:
Public.

Description

Updates filebrowser list when download starts/ends. BUG=chromium-os:9344, chromium-os:10605 TEST=manual Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71785

Patch Set 1 #

Patch Set 2 : avoid recursion #

Patch Set 3 : fixing unittests #

Total comments: 2

Patch Set 4 : nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -2 lines) Patch
M chrome/browser/dom_ui/filebrowse_ui.cc View 1 2 3 4 chunks +16 lines, -2 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
altimofeev
9 years, 11 months ago (2011-01-18 14:21:17 UTC) #1
xiyuan
LGTM
9 years, 11 months ago (2011-01-18 17:27:23 UTC) #2
achuithb
http://codereview.chromium.org/6320007/diff/5001/chrome/browser/dom_ui/filebrowse_ui.cc File chrome/browser/dom_ui/filebrowse_ui.cc (right): http://codereview.chromium.org/6320007/diff/5001/chrome/browser/dom_ui/filebrowse_ui.cc#newcode717 chrome/browser/dom_ui/filebrowse_ui.cc:717: void FilebrowseHandler::GetDownloads() { Maybe you could move this function ...
9 years, 11 months ago (2011-01-18 17:53:44 UTC) #3
altimofeev
9 years, 11 months ago (2011-01-19 10:29:34 UTC) #4
http://codereview.chromium.org/6320007/diff/5001/chrome/browser/dom_ui/filebr...
File chrome/browser/dom_ui/filebrowse_ui.cc (right):

http://codereview.chromium.org/6320007/diff/5001/chrome/browser/dom_ui/filebr...
chrome/browser/dom_ui/filebrowse_ui.cc:717: void
FilebrowseHandler::GetDownloads() {
On 2011/01/18 17:53:44, achuith.bhandarkar wrote:
> Maybe you could move this function to after the definition of ModelChanged?
That
> would make the diff smaller and have this function closer to where it's used
for
> readability.
> 
> Also GetDownloads may perhaps be too generic a name? Maybe have a more
> descriptive name like UpdateDownloadList or something along those lines?
> 
> LGTM otherwise.

Done.

Powered by Google App Engine
This is Rietveld 408576698