Initial checkin of the HTML downloads UI. It's not yet complete (lacking...

Initial checkin of the HTML downloads UI. It's not yet complete (lacking interface polish). But is enough for us to begin removing the native UI.ojan, please review downloads.htmlpaul, please review everything else

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

[Paul Godavari, 2009-02-06 00:16:27]

http://codereview.chromium.org/20110/diff/1/7
File chrome/browser/dom_ui/dom_ui.cc (right):

http://codereview.chromium.org/20110/diff/1/7#newcode136
Line 136: static_cast<const StringValue*>(list_member);
Nit: indent 2 more spaces.

http://codereview.chromium.org/20110/diff/1/7#newcode139
Line 139: return _wtoi(wstring_value.c_str());
Using StringToInt (base/string_util.h) is more cross-platformy.

http://codereview.chromium.org/20110/diff/1/7#newcode154
Line 154: static_cast<const StringValue*>(list_member);
Nit: indent 2 more spaces.

http://codereview.chromium.org/20110/diff/1/9
File chrome/browser/dom_ui/downloads_ui.cc (right):

http://codereview.chromium.org/20110/diff/1/9#newcode45
Line 45: SkBitmap* icon = im->LookupIcon(UTF8ToWide(path), IconLoader::NORMAL);
I'm not sure what the proper string conversions should be, so maybe ask Brett to
look at that?

http://codereview.chromium.org/20110/diff/1/9#newcode200
Line 200: // Get the id for the download.
I think it would be useful to expand the comment to explain what we're doing
with the 'id' calculation (i.e. latest first ordering).

http://codereview.chromium.org/20110/diff/1/9#newcode359
Line 359: void DownloadsDOMHandler::ClearDownloadItemObservers() {
Maybe add 'download_items_.clear();' to the end of this function and call it
ClearDownloadItems()?

http://codereview.chromium.org/20110/diff/1/9#newcode368
Line 368: if (id != NULL) {
'id < 0' (i.e. shouldn't be a NULL check on an int)?

http://codereview.chromium.org/20110/diff/1/11
File chrome/browser/dom_ui/downloads_ui.h (right):

http://codereview.chromium.org/20110/diff/1/11#newcode5
Line 5: #ifndef CHROME_BROWSER_DOM_UI_DOWNLOADS_UI_H__
Nit: As per the style guide, only one trailing underscore.

http://codereview.chromium.org/20110/diff/1/11#newcode19
Line 19: class FileIconSource : public ChromeURLDataManager::DataSource {
Even though this is a small class, it's probably worth putting in separate files
in case the others want to use it.

http://codereview.chromium.org/20110/diff/1/11#newcode21
Line 21: explicit FileIconSource();
I think this class will need a destructor that cancels any in progress icon
requests (see ~DownloadTabView).

http://codereview.chromium.org/20110/diff/1/11#newcode35
Line 35: SkBitmap* icon);
Nit: indent 2 more spaces.

http://codereview.chromium.org/20110/diff/1/11#newcode68
Line 68: explicit DownloadsDOMHandler(DOMUI* dom_ui_, DownloadManager* dlm);
Nit: looks weird to have a function parameter with a trailing underscore.

[ojan, 2009-02-06 02:38:13]

http://codereview.chromium.org/20110/diff/1/5
File chrome/browser/resources/downloads.html (right):

http://codereview.chromium.org/20110/diff/1/5#newcode81
Line 81: /**
Do we have a plan of any kind to make it so that we can share script/css across
html pages? We should figure this out. At quick glance, there's 50 lines right
here that should be shared with history.

I don't think it should block this CL. But we should put it on the radar (i.e.
file a bug).

http://codereview.chromium.org/20110/diff/1/5#newcode140
Line 140: this.summary_ = document.getElementById('downloads-summary');
These two lines, use $?

http://codereview.chromium.org/20110/diff/1/5#newcode203
Line 203: Downloads.prototype.clear = function() {
Can this whole function be:
for (var id in this.downloads_) {
  this.downloads_[id].clear();
  this.remove(id);
}

http://codereview.chromium.org/20110/diff/1/5#newcode225
Line 225: this.node.className = 'download';
Can you create a createElementWithClassName helper function? Would make this
code a lot more readable.

http://codereview.chromium.org/20110/diff/1/5#newcode270
Line 270: localStrings.getString('control_showinfolder');
Add a createLink(onclick, html) helper function?

http://codereview.chromium.org/20110/diff/1/5#newcode296
Line 296: this.dangerSave_.onclick = bind(this.saveDangerous_, this);
Add a createButton(onclick, value) helper function?

http://codereview.chromium.org/20110/diff/1/5#newcode321
Line 321: this.received_ = download.received_;
download.received? (that underscore is probably a typo?)

http://codereview.chromium.org/20110/diff/1/5#newcode330
Line 330: 
Maybe have:
isComplete = this.state_ == "COMPLETE";
isInProgress = this.state_ == "IN_PROGRESS";

And use those. Would make this code easier to follow.

http://codereview.chromium.org/20110/diff/1/5#newcode337
Line 337: this.nodeFileLink_.style.display = 'none';
This line is unnecessary with the line two below that does the same thing.

http://codereview.chromium.org/20110/diff/1/5#newcode342
Line 342: (this.state_ == "COMPLETE") ? 'inline' : 'none';
Add a showInline helper function for readability sake?

function showInline(node, isShow) {
  node.style.display = isShow ? 'inline' : 'none';
}

http://codereview.chromium.org/20110/diff/1/5#newcode375
Line 375: }
Why not just:
this.node_.innerHTML = '';

Also, why do we need to remove the children at all if we're about to remove the
node itself?

http://codereview.chromium.org/20110/diff/1/5#newcode394
Line 394: return "";
Would be nice to move the strings into an object so there's only one place that
needs to be matched up with downloads_ui.cc instead of strings everywhere.

Download.States = {
  IN_PROGRESS = "IN_PROGRESS",
  CANCELLED = "CANCELLED",
  ...
}

Also, when would we ever hit the default case. Seems like we should throw an
error there.

http://codereview.chromium.org/20110/diff/1/5#newcode404
Line 404: chrome.send("drag", [this.id_.toString()]);
Do you actually need to toString the id here and below?

http://codereview.chromium.org/20110/diff/1/5#newcode482
Line 482: }
downloadUpdated(results) does this.

[Glen Murphy, 2009-02-13 00:57:47]

Done, with the following exception notes:

On 2009/02/06 02:38:13, paul wrote:
> http://codereview.chromium.org/20110/diff/1/9#newcode45
> Line 45: SkBitmap* icon = im->LookupIcon(UTF8ToWide(path),
IconLoader::NORMAL);
> I'm not sure what the proper string conversions should be, so maybe ask Brett
to
> look at that?

Brett OKed this.


On 2009/02/06 02:38:13, ojan wrote:
> Do we have a plan of any kind to make it so that we can share script/css
across
> html pages? We should figure this out. At quick glance, there's 50 lines right
> here that should be shared with history.

It's being done elsewhere.

> Can you create a createElementWithClassName helper function? Would make this
> code a lot more readable.
> Add a createLink(onclick, html) helper function?
> Add a createButton(onclick, value) helper function?

Done, but I think it makes the code look worse. I imagine arv will be having at
it in short order, though, so I'm happy either way.

> Maybe have:
> isComplete = this.state_ == "COMPLETE";
> isInProgress = this.state_ == "IN_PROGRESS";
> And use those. Would make this code easier to follow.

Didn't do this. The Downloads.State stuff made it look better, and I didn't like
the addition of state bools.

[ojan, 2009-02-13 01:48:31]

LGTM

[Paul Godavari, 2009-02-17 22:23:18]

LGTM, with one nit.

http://codereview.chromium.org/20110/diff/54/1058
File chrome/browser/dom_ui/dom_ui.cc (right):

http://codereview.chromium.org/20110/diff/54/1058#newcode147
Line 147: return NULL;
Nit: I think this should return some default value here (like the empty string
in the method below) rather than returning NULL for an int value. Or you could
change the method signature to be:
bool Extract(const Value* in_value, int* out_int)
to better indicate success / failure.