Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7605003:
--downloads-new-ui completely disables the download shelf. (Closed)
|
Created:
9 years, 4 months ago by benjhayden Modified:
9 years, 4 months 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 : " #
Messages
Total messages: 20 (0 generated)
Ready for review.
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#ne... chrome/browser/ui/browser.cc:3405: // Don't show the animation for "Save file" downloads. Nit: the sense of this comment needs to be flipped.
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#ne... chrome/browser/ui/browser.cc:3405: // Don't show the animation for "Save file" downloads. On 2011/08/09 19:19:28, ahendrickson wrote: > Nit: the sense of this comment needs to be flipped. Can you clarify? I thought I was careful to avoid changing intentional functionality. It still doesn't animate for savefile downloads.
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#n... chrome/browser/about_flags.cc:369: "disable-download-shelf", // FLAGS:RECORD_UMA Did you run extract_actions.py?
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#n... chrome/browser/about_flags.cc:369: "disable-download-shelf", // FLAGS:RECORD_UMA On 2011/08/09 19:47:16, asanka wrote: > Did you run extract_actions.py? Good catch!
Is this the flag you want? Should we instead introduce a flag which is --enable-new-downloads-ui or something which also disables the shelf? I think that's the way this will head. 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.c... chrome/browser/about_flags.cc:372: kOsAll - kOsCrOS, Nit: Maybe it's just me, but it feels weird to do this instead of (kOsAll & ~kOsCros) http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:3401: if (download->total_bytes() > 0) { This is different behavior from before - now the two lines at the end of the function will be called in this case. Is that OK? http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:3404: ExtensionService::IsDownloadFromMiniGallery(download->GetURL())) { Same concern here.
On 2011/08/10 11:54:54, cbentzel wrote: > Is this the flag you want? Should we instead introduce a flag which is > --enable-new-downloads-ui or something which also disables the shelf? I think > that's the way this will head. We don't have the new ui yet, so that flag would be a misnomer. When we do have the new ui, sure, that flag sgtm. Until we have the new ui and it gains enough traction to be bundled with chrome, we can ask beta testers to both install this extension and set --disable-download-shelf. Hm, maybe this flag should disable the cros dl panel, too? > > 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.c... > chrome/browser/about_flags.cc:372: kOsAll - kOsCrOS, > Nit: Maybe it's just me, but it feels weird to do this instead of > > (kOsAll & ~kOsCros) > > http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc > File chrome/browser/ui/browser.cc (right): > > http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc... > chrome/browser/ui/browser.cc:3401: if (download->total_bytes() > 0) { > This is different behavior from before - now the two lines at the end of the > function will be called in this case. Is that OK? > > http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc... > chrome/browser/ui/browser.cc:3404: > ExtensionService::IsDownloadFromMiniGallery(download->GetURL())) { > Same concern here.
It's fine to preemptively add a flag and about:flags which doesn't have support. It lets you develop in trunk without impacting other users. On Wed, Aug 10, 2011 at 10:07 AM, <benjhayden@chromium.org> wrote: > On 2011/08/10 11:54:54, cbentzel wrote: >> >> Is this the flag you want? Should we instead introduce a flag which is >> --enable-new-downloads-ui or something which also disables the shelf? I >> think >> that's the way this will head. > > We don't have the new ui yet, so that flag would be a misnomer. When we do > have > the new ui, sure, that flag sgtm. Until we have the new ui and it gains > enough > traction to be bundled with chrome, we can ask beta testers to both install > this > extension and set --disable-download-shelf. > > Hm, maybe this flag should disable the cros dl panel, too? > > > >> >> 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.c... >> >> chrome/browser/about_flags.cc:372: kOsAll - kOsCrOS, >> Nit: Maybe it's just me, but it feels weird to do this instead of > >> (kOsAll & ~kOsCros) > >> >> http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc >> File chrome/browser/ui/browser.cc (right): > > > http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc... >> >> chrome/browser/ui/browser.cc:3401: if (download->total_bytes() > 0) { >> This is different behavior from before - now the two lines at the end of >> the >> function will be called in this case. Is that OK? > > > http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc... >> >> chrome/browser/ui/browser.cc:3404: >> ExtensionService::IsDownloadFromMiniGallery(download->GetURL())) { >> Same concern here. > > > > http://codereview.chromium.org/7605003/ >
In other words, I think it's easier to just have one flag. I don't think we'll want to support both disabling the existing UI and not enabling the new UI when it is in place. On Wed, Aug 10, 2011 at 10:13 AM, Chris Bentzel <cbentzel@chromium.org> wrote: > It's fine to preemptively add a flag and about:flags which doesn't > have support. It lets you develop in trunk without impacting other > users. > > On Wed, Aug 10, 2011 at 10:07 AM, <benjhayden@chromium.org> wrote: >> On 2011/08/10 11:54:54, cbentzel wrote: >>> >>> Is this the flag you want? Should we instead introduce a flag which is >>> --enable-new-downloads-ui or something which also disables the shelf? I >>> think >>> that's the way this will head. >> >> We don't have the new ui yet, so that flag would be a misnomer. When we do >> have >> the new ui, sure, that flag sgtm. Until we have the new ui and it gains >> enough >> traction to be bundled with chrome, we can ask beta testers to both install >> this >> extension and set --disable-download-shelf. >> >> Hm, maybe this flag should disable the cros dl panel, too? >> >> >> >>> >>> 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.c... >>> >>> chrome/browser/about_flags.cc:372: kOsAll - kOsCrOS, >>> Nit: Maybe it's just me, but it feels weird to do this instead of >> >>> (kOsAll & ~kOsCros) >> >>> >>> http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc >>> File chrome/browser/ui/browser.cc (right): >> >> >> http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc... >>> >>> chrome/browser/ui/browser.cc:3401: if (download->total_bytes() > 0) { >>> This is different behavior from before - now the two lines at the end of >>> the >>> function will be called in this case. Is that OK? >> >> >> http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc... >>> >>> chrome/browser/ui/browser.cc:3404: >>> ExtensionService::IsDownloadFromMiniGallery(download->GetURL())) { >>> Same concern here. >> >> >> >> http://codereview.chromium.org/7605003/ >> >
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.c... chrome/browser/about_flags.cc:372: kOsAll - kOsCrOS, On 2011/08/10 11:54:54, cbentzel wrote: > Nit: Maybe it's just me, but it feels weird to do this instead of > > (kOsAll & ~kOsCros) > Even better, kOsAll. If we're reimplementing the UI as an extension, then we can be consistent even across cros. http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:3401: if (download->total_bytes() > 0) { On 2011/08/10 11:54:54, cbentzel wrote: > This is different behavior from before - now the two lines at the end of the > function will be called in this case. Is that OK? Right, that's the bug fix part of this CL. It seems to me like we want to close the blank temporary tab even for savepages and non-theme extensions. I quickly tested this to check that it doesn't close non-blank tabs, but we'll see what the browser_tests say. http://codereview.chromium.org/7605003/diff/1007/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:3404: ExtensionService::IsDownloadFromMiniGallery(download->GetURL())) { On 2011/08/10 11:54:54, cbentzel wrote: > Same concern here. Same answer.
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... chrome/browser/ui/browser.cc:3382: if (!CommandLine::ForCurrentProcess()->HasSwitch( I'd rather wrap this in a helper function such as bool DisplayDownloadShelf() which hides the command line itself. http://codereview.chromium.org/7605003/diff/1014/chrome/common/chrome_switches.h File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/7605003/diff/1014/chrome/common/chrome_switche... chrome/common/chrome_switches.h:82: extern const char kDisableIPPooling[]; Unclear what order this should be - could you not do it as part of this CL?
And other than the nits, LGTM You may need to find an OWNER to look at this as well.
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 chrome/browser/ui/browser.cc (right):
http://codereview.chromium.org/7605003/diff/1014/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:3382: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
On 2011/08/11 20:08:29, cbentzel wrote:
> I'd rather wrap this in a helper function such as bool DisplayDownloadShelf()
> which hides the command line itself.
Done.
http://codereview.chromium.org/7605003/diff/1014/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (right):
http://codereview.chromium.org/7605003/diff/1014/chrome/common/chrome_switche...
chrome/common/chrome_switches.h:82: extern const char kDisableIPPooling[];
On 2011/08/11 20:08:29, cbentzel wrote:
> Unclear what order this should be - could you not do it as part of this CL?
Done.
+pkasting for chrome/browser/ui/OWNERS I trimmed the review list to active reviewers http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resour... chrome/app/generated_resources.grd:4233: <message name="IDS_FLAGS_DOWNLOADS_NEW_UI_NAME" desc="Title for the flag to replace the download shelf with an experimental new UI."> Nit: I recommend replacing with "Name of the 'New Downloads UI' lab." to be more consistent with the rest of this file. http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resour... chrome/app/generated_resources.grd:4236: <message name="IDS_FLAGS_DOWNLOADS_NEW_UI_DESCRIPTION" desc="Description for the flag to replace the download shelf with an experimental new UI."> Nit: I recommend replacing with "Description of the 'New Downloads UI' lab." to be more consistent with the rest of this file. 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.c... chrome/browser/ui/browser.cc:3365: bool DisplayOldDownloadsUI() { Nit: extra newlines here. http://codereview.chromium.org/7605003/diff/11001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3397: // GetDownloadShelf creates the download shelf if it was not yet created. I haven't looked at what happens if there isn't a DownloadItemModel/shelf. Do downloads still proceed? Do they expect a DownloadItem?
LGTM http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resour... chrome/app/generated_resources.grd:4237: Show an experimental new UI instead of the download shelf. Nit: Show -> Shows, for consistency with elsewhere 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.c... chrome/browser/ui/browser.cc:3402: if (download->total_bytes() > 0) { Nit: If you're going to change the early-return style of the old code, collapse these conditionals together (like you did above).
http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resour... chrome/app/generated_resources.grd:4233: <message name="IDS_FLAGS_DOWNLOADS_NEW_UI_NAME" desc="Title for the flag to replace the download shelf with an experimental new UI."> On 2011/08/15 18:28:26, cbentzel wrote: > Nit: I recommend replacing with > > "Name of the 'New Downloads UI' lab." > > to be more consistent with the rest of this file. Done. http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resour... chrome/app/generated_resources.grd:4236: <message name="IDS_FLAGS_DOWNLOADS_NEW_UI_DESCRIPTION" desc="Description for the flag to replace the download shelf with an experimental new UI."> On 2011/08/15 18:28:26, cbentzel wrote: > Nit: I recommend replacing with > > "Description of the 'New Downloads UI' lab." > > to be more consistent with the rest of this file. Done. http://codereview.chromium.org/7605003/diff/11001/chrome/app/generated_resour... chrome/app/generated_resources.grd:4237: Show an experimental new UI instead of the download shelf. On 2011/08/15 18:34:02, Peter Kasting wrote: > Nit: Show -> Shows, for consistency with elsewhere Done. 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.c... chrome/browser/ui/browser.cc:3365: bool DisplayOldDownloadsUI() { On 2011/08/15 18:28:26, cbentzel wrote: > Nit: extra newlines here. Done. http://codereview.chromium.org/7605003/diff/11001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3397: // GetDownloadShelf creates the download shelf if it was not yet created. On 2011/08/15 18:28:26, cbentzel wrote: > I haven't looked at what happens if there isn't a DownloadItemModel/shelf. Do > downloads still proceed? Do they expect a DownloadItem? Downloads still proceed. I haven't run all the tests with --downloads-new-ui, but I ran ./out/Debug/chrome, turned the flag on, downloaded a safe file and a dangerous file, and both showed up in chrome://downloads correctly, with the danger confirmation dialog. http://codereview.chromium.org/7605003/diff/11001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:3402: if (download->total_bytes() > 0) { On 2011/08/15 18:34:02, Peter Kasting wrote: > Nit: If you're going to change the early-return style of the old code, collapse > these conditionals together (like you did above). Unfortunately, I can't merge these conditionals without significantly changing functionality. There are significant differences between ExtensionService::IsDownloadFrom{,Mini}Gallery(). The only commonality is download->is_extension_install(). I added a TODO to talk to the original authors about merging them, but the new UI will be a cross-platform extension, so I wonder how much we should worry about merging these branches.
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.c... chrome/browser/ui/browser.cc:3402: if (download->total_bytes() > 0) { On 2011/08/15 19:15:36, b s h wrote: > On 2011/08/15 18:34:02, Peter Kasting wrote: > > Nit: If you're going to change the early-return style of the old code, > collapse > > these conditionals together (like you did above). > > Unfortunately, I can't merge these conditionals without significantly changing > functionality. There are significant differences between > ExtensionService::IsDownloadFrom{,Mini}Gallery(). You misunderstand me. I don't mean to merge the ChromeOS and non-ChromeOS cases. I mean merge the successive conditionals right here: if (...) { if (...) { ... if (...) { ... -> ... if ((...) && (...) && (...)) { ...
LGTM
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.c... chrome/browser/ui/browser.cc:3402: if (download->total_bytes() > 0) { On 2011/08/15 19:19:13, Peter Kasting wrote: > On 2011/08/15 19:15:36, b s h wrote: > > On 2011/08/15 18:34:02, Peter Kasting wrote: > > > Nit: If you're going to change the early-return style of the old code, > > collapse > > > these conditionals together (like you did above). > > > > Unfortunately, I can't merge these conditionals without significantly changing > > functionality. There are significant differences between > > ExtensionService::IsDownloadFrom{,Mini}Gallery(). > > You misunderstand me. I don't mean to merge the ChromeOS and non-ChromeOS > cases. I mean merge the successive conditionals right here: > > if (...) { > if (...) { > ... > if (...) { > ... > > -> > > ... > if ((...) && (...) && (...)) { > ... Done.
Change committed as 97079 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
