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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 9689007: Downloads: First pass at a cleanup to match webdev style. (Closed)

Created:
8 years, 9 months ago by James Hawkins
Modified:
8 years, 9 months ago
CC:
chromium-reviews, asanka, arv (Not doing code reviews), Randy Smith (Not in Mondays)
Visibility:
Public.

Description

Downloads: First pass at a cleanup to match webdev style. BUG=none TEST=none R=csilv,dbeam Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126201

Patch Set 1 #

Patch Set 2 : Added css. #

Total comments: 29

Patch Set 3 : Review fixes. #

Patch Set 4 : Review fixes 2. #

Total comments: 11

Patch Set 5 : One more. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+139 lines, -1005 lines) Patch
M chrome/browser/browser_resources.grd View 1 chunk +3 lines, -2 lines 0 comments Download
D chrome/browser/resources/downloads.html View 1 chunk +0 lines, -187 lines 0 comments Download
D chrome/browser/resources/downloads.js View 1 chunk +0 lines, -695 lines 0 comments Download
A + chrome/browser/resources/downloads/downloads.css View 1 2 3 6 chunks +13 lines, -51 lines 0 comments Download
A chrome/browser/resources/downloads/downloads.html View 1 2 3 4 1 chunk +43 lines, -0 lines 0 comments Download
A + chrome/browser/resources/downloads/downloads.js View 1 2 18 chunks +79 lines, -70 lines 0 comments Download
M chrome/browser/ui/webui/downloads_ui.cc View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
James Hawkins
8 years, 9 months ago (2012-03-12 04:15:38 UTC) #1
Dan Beam
mainly nits https://chromiumcodereview.appspot.com/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css File chrome/browser/resources/downloads/downloads.css (left): https://chromiumcodereview.appspot.com/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css#oldcode157 chrome/browser/resources/downloads/downloads.css:157: </style> wtf? https://chromiumcodereview.appspot.com/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css File chrome/browser/resources/downloads/downloads.css (right): https://chromiumcodereview.appspot.com/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css#newcode4 ...
8 years, 9 months ago (2012-03-12 08:38:48 UTC) #2
Randy Smith (Not in Mondays)
I'd like Ben's eyes to pass over this before it goes in ...
8 years, 9 months ago (2012-03-12 13:33:59 UTC) #3
benjhayden
LGTM Thanks! https://chromiumcodereview.appspot.com/9689007/diff/5001/chrome/browser/resources/downloads/downloads.js File chrome/browser/resources/downloads/downloads.js (right): https://chromiumcodereview.appspot.com/9689007/diff/5001/chrome/browser/resources/downloads/downloads.js#newcode346 chrome/browser/resources/downloads/downloads.js:346: DANGEROUS_CONTENT: 'DANGEROUS_CONTENT' FYI http://codereview.chromium.org/9621007/ That CL might ...
8 years, 9 months ago (2012-03-12 14:14:35 UTC) #4
csilv
http://codereview.chromium.org/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css File chrome/browser/resources/downloads/downloads.css (right): http://codereview.chromium.org/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css#newcode110 chrome/browser/resources/downloads/downloads.css:110: background: url('../../../app/theme/download_progress_background32.png'); change to: chrome://theme/IDR_DOWNLOAD_PROGRESS_BACKGROUND_32 http://codereview.chromium.org/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css#newcode114 chrome/browser/resources/downloads/downloads.css:114: background: url('../../../app/theme/download_progress_foreground32.png'); ...
8 years, 9 months ago (2012-03-12 18:16:50 UTC) #5
James Hawkins
https://chromiumcodereview.appspot.com/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css File chrome/browser/resources/downloads/downloads.css (left): https://chromiumcodereview.appspot.com/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css#oldcode157 chrome/browser/resources/downloads/downloads.css:157: </style> On 2012/03/12 08:38:48, Dan Beam wrote: > wtf? ...
8 years, 9 months ago (2012-03-12 18:33:45 UTC) #6
James Hawkins
http://codereview.chromium.org/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css File chrome/browser/resources/downloads/downloads.css (right): http://codereview.chromium.org/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css#newcode110 chrome/browser/resources/downloads/downloads.css:110: background: url('../../../app/theme/download_progress_background32.png'); On 2012/03/12 18:16:50, csilv wrote: > change ...
8 years, 9 months ago (2012-03-12 19:51:54 UTC) #7
csilv
lgtm
8 years, 9 months ago (2012-03-12 19:58:01 UTC) #8
Dan Beam
https://chromiumcodereview.appspot.com/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css File chrome/browser/resources/downloads/downloads.css (left): https://chromiumcodereview.appspot.com/9689007/diff/5001/chrome/browser/resources/downloads/downloads.css#oldcode157 chrome/browser/resources/downloads/downloads.css:157: </style> On 2012/03/12 18:33:45, James Hawkins wrote: > On ...
8 years, 9 months ago (2012-03-12 20:11:18 UTC) #9
Dan Beam
lgtm w/nits
8 years, 9 months ago (2012-03-12 20:12:11 UTC) #10
James Hawkins
https://chromiumcodereview.appspot.com/9689007/diff/11001/chrome/browser/resources/downloads/downloads.html File chrome/browser/resources/downloads/downloads.html (right): https://chromiumcodereview.appspot.com/9689007/diff/11001/chrome/browser/resources/downloads/downloads.html#newcode4 chrome/browser/resources/downloads/downloads.html:4: <meta charset="utf-8"> On 2012/03/12 20:11:18, Dan Beam wrote: > ...
8 years, 9 months ago (2012-03-12 20:14:41 UTC) #11
Dan Beam
8 years, 9 months ago (2012-03-12 20:17:04 UTC) #12 
https://chromiumcodereview.appspot.com/9689007/diff/11001/chrome/browser/reso...
File chrome/browser/resources/downloads/downloads.js (right):

https://chromiumcodereview.appspot.com/9689007/diff/11001/chrome/browser/reso...
chrome/browser/resources/downloads/downloads.js:213: 'div', 'download' +
(download.otr ? ' otr' : ''));
On 2012/03/12 20:14:41, James Hawkins wrote:
> On 2012/03/12 20:11:18, Dan Beam wrote:
> > nit: this might be easier to read as download-otr
> 
> .download is a class in itself so .download.otr is className = "download otr";

ah, I missed the space

Powered by Google App Engine
This is Rietveld 408576698