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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 8 months ago by benjhayden_chromium
Modified:
2 years, 8 months ago
CC:
chromium-reviews_chromium.org
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) Lint Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments ? errors Download
M chrome/browser/about_flags.cc View 1 2 3 4 1 chunk +9 lines, -1 line 0 comments 1 errors Download
M chrome/browser/ui/browser.cc View 1 2 3 4 5 6 7 2 chunks +36 lines, -30 lines 0 comments ? errors Download
M chrome/common/chrome_switches.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments ? errors Download
M chrome/common/chrome_switches.cc View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments ? errors Download
M chrome/tools/chromeactions.txt View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 20
benjhayden_chromium
Ready for review.
2 years, 8 months ago #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 ...
2 years, 8 months ago #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. ...
2 years, 8 months ago #3
asanka
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?
2 years, 8 months ago #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: > ...
2 years, 8 months ago #5
cbentzel
Is this the flag you want? Should we instead introduce a flag which is --enable-new-downloads-ui ...
2 years, 8 months ago #6
benjhayden_chromium
On 2011/08/10 11:54:54, cbentzel wrote: > Is this the flag you want? Should we instead ...
2 years, 8 months ago #7
cbentzel
It's fine to preemptively add a flag and about:flags which doesn't have support. It lets ...
2 years, 8 months ago #8
cbentzel
In other words, I think it's easier to just have one flag. I don't think ...
2 years, 8 months ago #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: > ...
2 years, 8 months ago #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 ...
2 years, 8 months ago #11
cbentzel
And other than the nits, LGTM You may need to find an OWNER to look ...
2 years, 8 months ago #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 ...
2 years, 8 months ago #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): ...
2 years, 8 months ago #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 ...
2 years, 8 months ago #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 ...
2 years, 8 months ago #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 ...
2 years, 8 months ago #17
cbentzel
LGTM
2 years, 8 months ago #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 ...
2 years, 8 months ago #19
I haz the power (commit-bot)
2 years, 8 months ago #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 1280:2d3e6564b7b6