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

Issue 7605003: --downloads-new-ui completely disables the download shelf. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years ago by benjhayden_chromium
Modified:
4 years ago
CC:
chromium-reviews
Visibility:
Public.

Description

--downloads-new-ui completely disables the download shelf. In the future, it will also enable the experimental new ui. Also fix a bug where the temporary blank page might not have been automatically closed. BUG=89922 TEST= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97079

Patch Set 1 #

Total comments: 4

Patch Set 2 : extract_actions #

Total comments: 6

Patch Set 3 : --downloads-new-ui +cros #

Patch Set 4 : " #

Total comments: 4

Patch Set 5 : nits #

Total comments: 14

Patch Set 6 : nits #

Patch Set 7 : ifs #

Patch Set 8 : " #

Unified diffs Side-by-side diffs Delta from patch set Stats (+56 lines, -31 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 1 chunk +9 lines, -1 line 0 comments Download
M chrome/browser/ui/browser.cc View 1 2 3 4 5 6 7 2 chunks +36 lines, -30 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/tools/chromeactions.txt View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
Project "None" does not have a commit queue.

Messages

Total messages: 20 (0 generated)
benjhayden_chromium
Ready for review.
4 years ago (2011-08-09 18:37:58 UTC) #1
ahendrickson
LGTM, with one nit. http://codereview.chromium.org/7605003/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7605003/diff/1/chrome/browser/ui/browser.cc#newcode3405 chrome/browser/ui/browser.cc:3405: // Don't show the animation ...
4 years ago (2011-08-09 19:19:28 UTC) #2
benjhayden_chromium
http://codereview.chromium.org/7605003/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7605003/diff/1/chrome/browser/ui/browser.cc#newcode3405 chrome/browser/ui/browser.cc:3405: // Don't show the animation for "Save file" downloads. ...
4 years ago (2011-08-09 19:30:46 UTC) #3
asanka (On leave)
http://codereview.chromium.org/7605003/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): http://codereview.chromium.org/7605003/diff/1/chrome/browser/about_flags.cc#newcode369 chrome/browser/about_flags.cc:369: "disable-download-shelf", // FLAGS:RECORD_UMA Did you run extract_actions.py?
4 years ago (2011-08-09 19:47:16 UTC) #4
benjhayden_chromium
http://codereview.chromium.org/7605003/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): http://codereview.chromium.org/7605003/diff/1/chrome/browser/about_flags.cc#newcode369 chrome/browser/about_flags.cc:369: "disable-download-shelf", // FLAGS:RECORD_UMA On 2011/08/09 19:47:16, asanka wrote: > ...
4 years ago (2011-08-09 20:11:30 UTC) #5
cbentzel
Is this the flag you want? Should we instead introduce a flag which is --enable-new-downloads-ui ...
4 years ago (2011-08-10 11:54:54 UTC) #6
benjhayden_chromium
On 2011/08/10 11:54:54, cbentzel wrote: > Is this the flag you want? Should we instead ...
4 years ago (2011-08-10 14:07:02 UTC) #7
cbentzel
It's fine to preemptively add a flag and about:flags which doesn't have support. It lets ...
4 years ago (2011-08-10 14:13:36 UTC) #8
cbentzel
In other words, I think it's easier to just have one flag. I don't think ...
4 years ago (2011-08-10 14:14:21 UTC) #9
benjhayden_chromium
http://codereview.chromium.org/7605003/diff/1007/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): http://codereview.chromium.org/7605003/diff/1007/chrome/browser/about_flags.cc#newcode372 chrome/browser/about_flags.cc:372: kOsAll - kOsCrOS, On 2011/08/10 11:54:54, cbentzel wrote: > ...
4 years ago (2011-08-10 17:09:17 UTC) #10
cbentzel
http://codereview.chromium.org/7605003/diff/1014/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7605003/diff/1014/chrome/browser/ui/browser.cc#newcode3382 chrome/browser/ui/browser.cc:3382: if (!CommandLine::ForCurrentProcess()->HasSwitch( I'd rather wrap this in a helper ...
4 years ago (2011-08-11 20:08:29 UTC) #11
cbentzel
And other than the nits, LGTM You may need to find an OWNER to look ...
4 years ago (2011-08-11 20:21:45 UTC) #12
benjhayden_chromium
I didn't see any OWNERS files in chrome{,/app,/browser,/common,/tools}/. Can you suggest another reviewer? http://codereview.chromium.org/7605003/diff/1014/chrome/browser/ui/browser.cc File ...
4 years ago (2011-08-15 14:57:15 UTC) #13
cbentzel
+pkasting for chrome/browser/ui/OWNERS I trimmed the review list to active reviewers http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): ...
4 years ago (2011-08-15 18:28:26 UTC) #14
Peter Kasting
LGTM http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resources.grd#newcode4237 chrome/app/generated_resources.grd:4237: Show an experimental new UI instead of the ...
4 years ago (2011-08-15 18:34:02 UTC) #15
benjhayden_chromium
http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resources.grd#newcode4233 chrome/app/generated_resources.grd:4233: <message name="IDS_FLAGS_DOWNLOADS_NEW_UI_NAME" desc="Title for the flag to replace the ...
4 years ago (2011-08-15 19:15:35 UTC) #16
Peter Kasting
http://codereview.chromium.org/7605003/diff/11001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7605003/diff/11001/chrome/browser/ui/browser.cc#newcode3402 chrome/browser/ui/browser.cc:3402: if (download->total_bytes() > 0) { On 2011/08/15 19:15:36, b ...
4 years ago (2011-08-15 19:19:13 UTC) #17
cbentzel
LGTM
4 years ago (2011-08-15 19:33:56 UTC) #18
benjhayden_chromium
http://codereview.chromium.org/7605003/diff/11001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7605003/diff/11001/chrome/browser/ui/browser.cc#newcode3402 chrome/browser/ui/browser.cc:3402: if (download->total_bytes() > 0) { On 2011/08/15 19:19:13, Peter ...
4 years ago (2011-08-15 19:51:42 UTC) #19
commit-bot: I haz the power
4 years ago (2011-08-17 01:47:09 UTC) #20
Change committed as 97079
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 020f378