Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698593002:
(Android) Blocking multiple scheduled downloads for the same URL (Closed)
|
Created:
3 years, 10 months ago by marcin Modified:
3 years, 9 months ago CC:
chromium-reviews, asanka, cbentzel+watch_chromium.org, ntp-dev+reviews_chromium.org, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, net-reviews_chromium.org, chili+watch_chromium.org, noyau+watch_chromium.org, agrieve+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description(Android) Blocking multiple scheduled downloads for the same URL
Currently Chrome Chrome doesn't block multiple scheduled downloads
for the same URL.
Patch is adding adequate checks for
* Download icon and
* Download page later button and
* Suggestions
Additionally showDownloadingToast contains call for DownloadUtils function to make it more clear, that code there is the same.
User experiences (displaying Toast after clicking icon, disabling button after clicking, etc.) are not changed.
BUG=693218
Patch Set 1 : Patch #
Total comments: 7
Messages
Total messages: 36 (14 generated)
Patchset #1 (id:1) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== X X X X X Revert "X" This reverts commit 630e69db90aed6761e7be2bc9d278d196c8bcc19. X X Revert "X" This reverts commit 78c0128f22382a79b331976ee84da7dc36787064. X X X X X X X Merge branch 'master' of https://chromium.googlesource.com/chromium/src into x2 X BUG= ========== to ========== (Android) Blocking multiple scheduled downloads for the same URL Currently Chrome Chrome doesn't block multiple scheduled downloads for the same URL. Patch is adding checks for *Download icon and *"Download page later" button BUG=693218 ==========
Description was changed from ========== (Android) Blocking multiple scheduled downloads for the same URL Currently Chrome Chrome doesn't block multiple scheduled downloads for the same URL. Patch is adding checks for *Download icon and *"Download page later" button BUG=693218 ========== to ========== (Android) Blocking multiple scheduled downloads for the same URL Currently Chrome Chrome doesn't block multiple scheduled downloads for the same URL. Patch is adding checks for * Download icon and * Download page later button BUG=693218 ==========
Description was changed from ========== (Android) Blocking multiple scheduled downloads for the same URL Currently Chrome Chrome doesn't block multiple scheduled downloads for the same URL. Patch is adding checks for * Download icon and * Download page later button BUG=693218 ========== to ========== (Android) Blocking multiple scheduled downloads for the same URL Currently Chrome Chrome doesn't block multiple scheduled downloads for the same URL. Patch is adding checks for * Download icon and * Download page later button Additionally showDownloadingToast contains call for DownloadUtils function to make it more clear, that code there is the same. BUG=693218 ==========
Description was changed from ========== (Android) Blocking multiple scheduled downloads for the same URL Currently Chrome Chrome doesn't block multiple scheduled downloads for the same URL. Patch is adding checks for * Download icon and * Download page later button Additionally showDownloadingToast contains call for DownloadUtils function to make it more clear, that code there is the same. BUG=693218 ========== to ========== (Android) Blocking multiple scheduled downloads for the same URL Currently Chrome Chrome doesn't block multiple scheduled downloads for the same URL. Patch is adding adequate checks for * Download icon and * Download page later button Additionally showDownloadingToast contains call for DownloadUtils function to make it more clear, that code there is the same. User experiences (displaying Toast after clicking icon, disabling button after clicking, etc.) are not changed. BUG=693218 ==========
marcin@mwiacek.com changed reviewers: + cbentzel@chromium.org, dimich@chromium.org, peconn@chromium.org, qinmin@chromium.org
Description was changed from ========== (Android) Blocking multiple scheduled downloads for the same URL Currently Chrome Chrome doesn't block multiple scheduled downloads for the same URL. Patch is adding adequate checks for * Download icon and * Download page later button Additionally showDownloadingToast contains call for DownloadUtils function to make it more clear, that code there is the same. User experiences (displaying Toast after clicking icon, disabling button after clicking, etc.) are not changed. BUG=693218 ========== to ========== (Android) Blocking multiple scheduled downloads for the same URL Currently Chrome Chrome doesn't block multiple scheduled downloads for the same URL. Patch is adding adequate checks for * Download icon and * Download page later button and * Suggestions Additionally showDownloadingToast contains call for DownloadUtils function to make it more clear, that code there is the same. User experiences (displaying Toast after clicking icon, disabling button after clicking, etc.) are not changed. BUG=693218 ==========
Patchset #1 (id:80001) has been deleted
Thanks for a patch! There are couple of notes: https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:193: .getRequestsInQueue(new Callback<SavePageRequest[]>() { This is a SQLLite operation, with disk IO. It can take seconds on a heavy-loaded low-end device. The chances are, the tab is already killed/navigated away when it is completed. Also, the UX is not good - if this is originated from UI, the user has a feeling his request is ignored. Instead, the user should be prevented from clickign at the buttons at the first place (by disabling controls). See how the download buttons are disabled when the page is Incognito etc (isAllowedToDownloadPage() function below this one) https://codereview.chromium.org/2698593002/diff/100001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2698593002/diff/100001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:301: auto request_coordinator_continuation = []( I think this will be a suboptimal UX - there will be a blue (active) button, which would do nothing when clicked. At least today it'll do something, even if it is to create a duplicate download. I think we should instead disable the button from the start (if the download with the same URL is already active) and make it say "Downloading" - exactly the same as it looks when pressed for the first time... Also, this (creating auto lambda variable) is not a typical style of doing this in Chrome, usually the better way is to mimic the style already in the code around.
https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:193: .getRequestsInQueue(new Callback<SavePageRequest[]>() { On 2017/02/17 02:40:59, Dmitry Titov wrote: > This is a SQLLite operation, with disk IO. It can take seconds on a heavy-loaded > low-end device. The chances are, the tab is already killed/navigated away when > it is completed. On heavy-loaded low-end devices people the most often don't download many pages at all. > Also, the UX is not good - if this is originated from UI, the user has a feeling > his request is ignored. Instead, the user should be prevented from clickign at > the buttons at the first place (by disabling controls). See how the download > buttons are disabled when the page is Incognito etc (isAllowedToDownloadPage() > function below this one) When user will be prevented from clicking, it can make feeling, that this page (type) cannot be downloaded by Chrome. https://codereview.chromium.org/2698593002/diff/100001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/2698593002/diff/100001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:301: auto request_coordinator_continuation = []( On 2017/02/17 02:40:59, Dmitry Titov wrote: > I think this will be a suboptimal UX - there will be a blue (active) button, > which would do nothing when clicked. At least today it'll do something, even if > it is to create a duplicate download. Button will do something -> will change to Downloading after pressing. > I think we should instead disable the button from the start (if the download > with the same URL is already active) and make it say "Downloading" - exactly the > same as it looks when pressed for the first time... It means, that you will need to make check on the tab opening. On low-end heavy-loaded devices... > Also, this (creating auto lambda variable) is not a typical style of doing this > in Chrome, usually the better way is to mimic the style already in the code > around. I just used style from some Download/Offline files around. I don't see anything wrong with this.
https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:193: .getRequestsInQueue(new Callback<SavePageRequest[]>() { On 2017/02/17 08:02:32, marcin wrote: > On 2017/02/17 02:40:59, Dmitry Titov wrote: > > This is a SQLLite operation, with disk IO. It can take seconds on a > heavy-loaded > > low-end device. The chances are, the tab is already killed/navigated away when > > it is completed. > > On heavy-loaded low-end devices people the most often don't download many pages > at all. > > > Also, the UX is not good - if this is originated from UI, the user has a > feeling > > his request is ignored. Instead, the user should be prevented from clickign at > > the buttons at the first place (by disabling controls). See how the download > > buttons are disabled when the page is Incognito etc (isAllowedToDownloadPage() > > function below this one) > > When user will be prevented from clicking, it can make feeling, that this page > (type) > cannot be downloaded by Chrome. maybe I will do toast in case, when user try the same Url for the same time. WDYT?
marcin@mwiacek.com changed reviewers: + mariakhomenko@chromium.org
peconn@chromium.org changed reviewers: + dgn@chromium.org
+dgn - do you mind taking a look at this? It looks OK to me, but given Dmitry's concerns with UX on his side, it could do with another pair of eyes.
On 2017/02/21 11:20:49, PEConn wrote: > +dgn - do you mind taking a look at this? It looks OK to me, but given Dmitry's > concerns with UX on his side, it could do with another pair of eyes. I have some nits, but since the patch might change, I'll hold //c/b/suggestions review until after it looks good to Downloads OWNERS Also, question: Will it handle resuming download queries that might be paused or in an error state?
https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:193: .getRequestsInQueue(new Callback<SavePageRequest[]>() { On 2017/02/17 08:02:32, marcin wrote: > On 2017/02/17 02:40:59, Dmitry Titov wrote: > > This is a SQLLite operation, with disk IO. It can take seconds on a > heavy-loaded > > low-end device. The chances are, the tab is already killed/navigated away when > > it is completed. > > On heavy-loaded low-end devices people the most often don't download many pages > at all. > Backing this this assumption with numbers would be nice. I'd think low-end devices and data-conscious go often together, so these users would try to avoid their phone loading pages from the web, which will also be slower than loading locally.
> Also, question: Will it handle resuming download queries that might be paused or > in an error state? Nothing is done with them. Like described - it's just doesn't allow for opening many downloads for the same URL.
https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java (right): https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:193: .getRequestsInQueue(new Callback<SavePageRequest[]>() { On 2017/02/21 13:51:14, dgn wrote: > On 2017/02/17 08:02:32, marcin wrote: > > On 2017/02/17 02:40:59, Dmitry Titov wrote: > > > This is a SQLLite operation, with disk IO. It can take seconds on a > > heavy-loaded > > > low-end device. The chances are, the tab is already killed/navigated away > when > > > it is completed. > > > > On heavy-loaded low-end devices people the most often don't download many > pages > > at all. > > > Backing this this assumption with numbers would be nice. I'd think low-end > devices and data-conscious go often together, so these users would try to avoid > their phone loading pages from the web, which will also be slower than loading > locally. Patch allows for saving device resources (no multiple downloads) and saving user data connection. Cost is one (?) SQL operation, which shouldn't be very expensive on device, where not too many multiple downloads are done (in my opinion - low-end, where it's even more important not too have useless things). If you think, that it's better to open many connections, I will have to close this topic, because it doesn't have sense.
On 2017/02/21 18:24:31, marcin wrote: > https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java > (right): > > https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:193: > .getRequestsInQueue(new Callback<SavePageRequest[]>() { > On 2017/02/21 13:51:14, dgn wrote: > > On 2017/02/17 08:02:32, marcin wrote: > > > On 2017/02/17 02:40:59, Dmitry Titov wrote: > > > > This is a SQLLite operation, with disk IO. It can take seconds on a > > > heavy-loaded > > > > low-end device. The chances are, the tab is already killed/navigated away > > when > > > > it is completed. > > > > > > On heavy-loaded low-end devices people the most often don't download many > > pages > > > at all. > > > > > Backing this this assumption with numbers would be nice. I'd think low-end > > devices and data-conscious go often together, so these users would try to > avoid > > their phone loading pages from the web, which will also be slower than loading > > locally. > > Patch allows for saving device resources (no multiple downloads) and saving user > data connection. > > Cost is one (?) SQL operation, which shouldn't be very expensive on device, > where > not too many multiple downloads are done (in my opinion - low-end, where it's > even more important not too have useless things). > > If you think, that it's better to open many connections, I will have to close > this > topic, because it doesn't have sense. I don't think the concern was the cost of SQL operation, the concern here was that because of the time that it can take, there can be a timing condition when objects will stop exist and pointers go invalid while this async operation is taking place. SO the callback may be invoked after the tab is either on another page or closed. Also, the main comment here is about the proposed solution. Instead of silently dropping the operation, it would be better to prevent the user from initiating it in the first place, for example by showing 'download' button in disabled form, or even bringing a UI that says similar download is already going on. Otherwise, it has a high risk of looking like broken functionality for the user, if they see 'Downloading...' balloon but nothing happens after that (since they may not realize similar URL is already in the queue).
On 2017/02/21 18:34:03, Dmitry Titov wrote: > On 2017/02/21 18:24:31, marcin wrote: > > > https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java > > (right): > > > > > https://codereview.chromium.org/2698593002/diff/100001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/download/DownloadUtils.java:193: > > .getRequestsInQueue(new Callback<SavePageRequest[]>() { > > On 2017/02/21 13:51:14, dgn wrote: > > > On 2017/02/17 08:02:32, marcin wrote: > > > > On 2017/02/17 02:40:59, Dmitry Titov wrote: > > > > > This is a SQLLite operation, with disk IO. It can take seconds on a > > > > heavy-loaded > > > > > low-end device. The chances are, the tab is already killed/navigated > away > > > when > > > > > it is completed. > > > > > > > > On heavy-loaded low-end devices people the most often don't download many > > > pages > > > > at all. > > > > > > > Backing this this assumption with numbers would be nice. I'd think low-end > > > devices and data-conscious go often together, so these users would try to > > avoid > > > their phone loading pages from the web, which will also be slower than > loading > > > locally. > > > > Patch allows for saving device resources (no multiple downloads) and saving > user > > data connection. > > > > Cost is one (?) SQL operation, which shouldn't be very expensive on device, > > where > > not too many multiple downloads are done (in my opinion - low-end, where it's > > even more important not too have useless things). > > > > If you think, that it's better to open many connections, I will have to close > > this > > topic, because it doesn't have sense. > > I don't think the concern was the cost of SQL operation, the concern here was > that because of the time that it can take, > there can be a timing condition when objects will stop exist and pointers go > invalid while this async operation is > taking place. SO the callback may be invoked after the tab is either on another > page or closed. If you will do checks for example on tab opening, it can be also time consuming and probably will have to be done using similar callback. > Also, the main comment here is about the proposed solution. Instead of silently > dropping the operation, it would be better > to prevent the user from initiating it in the first place, for example by > showing 'download' button in disabled form, > or even bringing a UI that says similar download is already going on. Otherwise, > it has a high risk of looking like > broken functionality for the user, if they see 'Downloading...' balloon but > nothing happens after that > (since they may not realize similar URL is already in the queue). Once again - if you will do checks for example on tab opening, it can be also time consuming and probably will have to be done using similar callback. Disabling control is not good: 1. open tab when download is scheduled 2. cancel download You have to reload tab to enable controls I assume, that user knows, what he/she download. "Download already scheduled" is nice idea, but do we really want to add new string? Let's vote.
Sent mail offlist, but curious about where my help is being requested. Is it for /c/b/n/net_error_tab_helper? There's been active discussion and I'll wait for that to settle down before reviewing. Unless I'm supposed to be part of that discussion :)
johnmarienettal@gmail.com changed reviewers: + johnmarienettal@gmail.com
On 2017/02/23 11:29:37, cbentzel wrote: > Sent mail offlist, but curious about where my help is being requested. Is it for > /c/b/n/net_error_tab_helper? > > There's been active discussion and I'll wait for that to settle down before > reviewing. Unless I'm supposed to be part of that discussion :) AFAIR, you were suggested during submit as reviewer. Please also take voice in the discussion, as we see, there is no strong "NO" and no strong "YES" now.
Hi all, Any proposals here better than proposed patch ? If not, can be LGTM it?
First of all, thanks for filing the bug, the issue you found is a real one and will have to be fixed! The patch will likely need to be updated though, I've mentioned it in my first comment with quite strongly, sorry if it didn't go over as such. I've updated the bug https://bugs.chromium.org/p/chromium/issues/detail?id=693218 What we need here is a better UX designed, here is why we think so: - the Download button in main menu, when it detects the existing page with the same URL, shows infobar asking user if they want to download another copy or cancel. - the blue button on Dino page goes disabled once clicked, partially to prevent clicking again - but if the user refreshes the page (resulting again in Dino page), the button is enabled again. - The Download button that is shown when user looks at Dino page is enabled all the time. These behaviors are inconsistent, so adding a check to more places and just ignoring user action would be adding to a 'quirky' behavior rather then fixing it. We will work on a design doc that would propose a consistent UI treatment of these situations (likely using combination of showing the infobar in more places, and disabling buttons). The reason we need that design doc and not just vote in the codereview log is that our UI designers should chime in, to keep UI consistent with other pieces and behaviors. Once we have this done, you are more than welcome to update the CL to this design doc - all the necessary communication will be on the bug you filed. We'll also add you to the design doc as well so you could chime in. Stay tuned!
On 2017/03/01 19:47:35, Dmitry Titov wrote: > First of all, thanks for filing the bug, the issue you found is a real one and > will have to be fixed! > > The patch will likely need to be updated though, I've mentioned it in my first > comment with quite strongly, sorry if it didn't go over as such. > > I've updated the bug > https://bugs.chromium.org/p/chromium/issues/detail?id=693218 > What we need here is a better UX designed, here is why we think so: > > - the Download button in main menu, when it detects the existing page with the > same URL, shows infobar asking user if they want to download another copy or > cancel. > - the blue button on Dino page goes disabled once clicked, partially to prevent > clicking again - but if the user refreshes the page (resulting again in Dino > page), the button is enabled again. > - The Download button that is shown when user looks at Dino page is enabled all > the time. > > These behaviors are inconsistent, so adding a check to more places and just > ignoring user action would be adding to a 'quirky' behavior rather then fixing > it. > > We will work on a design doc that would propose a consistent UI treatment of > these situations (likely using combination of showing the infobar in more > places, and disabling buttons). The reason we need that design doc and not just > vote in the codereview log is that our UI designers should chime in, to keep UI > consistent with other pieces and behaviors. Once we have this done, you are more > than welcome to update the CL to this design doc - all the necessary > communication will be on the bug you filed. We'll also add you to the design doc > as well so you could chime in. Stay tuned! LGTM, thank you!
On 2017/03/01 23:41:33, marcin wrote: > On 2017/03/01 19:47:35, Dmitry Titov wrote: > > First of all, thanks for filing the bug, the issue you found is a real one and > > will have to be fixed! > > > > The patch will likely need to be updated though, I've mentioned it in my first > > comment with quite strongly, sorry if it didn't go over as such. > > > > I've updated the bug > > https://bugs.chromium.org/p/chromium/issues/detail?id=693218 > > What we need here is a better UX designed, here is why we think so: > > > > - the Download button in main menu, when it detects the existing page with the > > same URL, shows infobar asking user if they want to download another copy or > > cancel. > > - the blue button on Dino page goes disabled once clicked, partially to > prevent > > clicking again - but if the user refreshes the page (resulting again in Dino > > page), the button is enabled again. > > - The Download button that is shown when user looks at Dino page is enabled > all > > the time. > > > > These behaviors are inconsistent, so adding a check to more places and just > > ignoring user action would be adding to a 'quirky' behavior rather then fixing > > it. > > > > We will work on a design doc that would propose a consistent UI treatment of > > these situations (likely using combination of showing the infobar in more > > places, and disabling buttons). The reason we need that design doc and not > just > > vote in the codereview log is that our UI designers should chime in, to keep > UI > > consistent with other pieces and behaviors. Once we have this done, you are > more > > than welcome to update the CL to this design doc - all the necessary > > communication will be on the bug you filed. We'll also add you to the design > doc > > as well so you could chime in. Stay tuned! > > LGTM, thank you! Is this CL going to be closed out, or do you expect to create a new one? [My OCD is kicking in for keeping my code review dashboard clean]
On 2017/03/03 16:07:50, cbentzel wrote: > On 2017/03/01 23:41:33, marcin wrote: > > On 2017/03/01 19:47:35, Dmitry Titov wrote: > > > First of all, thanks for filing the bug, the issue you found is a real one > and > > > will have to be fixed! > > > > > > The patch will likely need to be updated though, I've mentioned it in my > first > > > comment with quite strongly, sorry if it didn't go over as such. > > > > > > I've updated the bug > > > https://bugs.chromium.org/p/chromium/issues/detail?id=693218 > > > What we need here is a better UX designed, here is why we think so: > > > > > > - the Download button in main menu, when it detects the existing page with > the > > > same URL, shows infobar asking user if they want to download another copy or > > > cancel. > > > - the blue button on Dino page goes disabled once clicked, partially to > > prevent > > > clicking again - but if the user refreshes the page (resulting again in Dino > > > page), the button is enabled again. > > > - The Download button that is shown when user looks at Dino page is enabled > > all > > > the time. > > > > > > These behaviors are inconsistent, so adding a check to more places and just > > > ignoring user action would be adding to a 'quirky' behavior rather then > fixing > > > it. > > > > > > We will work on a design doc that would propose a consistent UI treatment of > > > these situations (likely using combination of showing the infobar in more > > > places, and disabling buttons). The reason we need that design doc and not > > just > > > vote in the codereview log is that our UI designers should chime in, to keep > > UI > > > consistent with other pieces and behaviors. Once we have this done, you are > > more > > > than welcome to update the CL to this design doc - all the necessary > > > communication will be on the bug you filed. We'll also add you to the design > > doc > > > as well so you could chime in. Stay tuned! > > > > LGTM, thank you! > > Is this CL going to be closed out, or do you expect to create a new one? > > [My OCD is kicking in for keeping my code review dashboard clean] We could close this one and when we have a design doc (in about a week or two) - create a new one. I'd let marcin@ close it...
Message was sent while issue was closed.
See https://codereview.chromium.org/2698593002/ for the alias of the guy and context. On Fri, Mar 3, 2017 at 2:07 PM <dimich@chromium.org> wrote: > On 2017/03/03 16:07:50, cbentzel wrote: > > On 2017/03/01 23:41:33, marcin wrote: > > > On 2017/03/01 19:47:35, Dmitry Titov wrote: > > > > First of all, thanks for filing the bug, the issue you found is a > real one > > and > > > > will have to be fixed! > > > > > > > > The patch will likely need to be updated though, I've mentioned it > in my > > first > > > > comment with quite strongly, sorry if it didn't go over as such. > > > > > > > > I've updated the bug > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=693218 > > > > What we need here is a better UX designed, here is why we think so: > > > > > > > > - the Download button in main menu, when it detects the existing > page with > > the > > > > same URL, shows infobar asking user if they want to download another > copy > or > > > > cancel. > > > > - the blue button on Dino page goes disabled once clicked, partially > to > > > prevent > > > > clicking again - but if the user refreshes the page (resulting again > in > Dino > > > > page), the button is enabled again. > > > > - The Download button that is shown when user looks at Dino page is > enabled > > > all > > > > the time. > > > > > > > > These behaviors are inconsistent, so adding a check to more places > and > just > > > > ignoring user action would be adding to a 'quirky' behavior rather > then > > fixing > > > > it. > > > > > > > > We will work on a design doc that would propose a consistent UI > treatment > of > > > > these situations (likely using combination of showing the infobar in > more > > > > places, and disabling buttons). The reason we need that design doc > and not > > > just > > > > vote in the codereview log is that our UI designers should chime in, > to > keep > > > UI > > > > consistent with other pieces and behaviors. Once we have this done, > you > are > > > more > > > > than welcome to update the CL to this design doc - all the necessary > > > > communication will be on the bug you filed. We'll also add you to the > design > > > doc > > > > as well so you could chime in. Stay tuned! > > > > > > LGTM, thank you! > > > > Is this CL going to be closed out, or do you expect to create a new one? > > > > [My OCD is kicking in for keeping my code review dashboard clean] > > We could close this one and when we have a design doc (in about a week or > two) - > create a new one. > I'd let marcin@ close it... > > https://codereview.chromium.org/2698593002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Sorry for spam, wrong thread :) We are following up on sharing a design doc with Marcin :) On Tue, Mar 21, 2017 at 2:52 PM Dmitry Titov <dimich@chromium.org> wrote: > See https://codereview.chromium.org/2698593002/ for the alias of the guy > and context. > > On Fri, Mar 3, 2017 at 2:07 PM <dimich@chromium.org> wrote: > > On 2017/03/03 16:07:50, cbentzel wrote: > > On 2017/03/01 23:41:33, marcin wrote: > > > On 2017/03/01 19:47:35, Dmitry Titov wrote: > > > > First of all, thanks for filing the bug, the issue you found is a > real one > > and > > > > will have to be fixed! > > > > > > > > The patch will likely need to be updated though, I've mentioned it > in my > > first > > > > comment with quite strongly, sorry if it didn't go over as such. > > > > > > > > I've updated the bug > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=693218 > > > > What we need here is a better UX designed, here is why we think so: > > > > > > > > - the Download button in main menu, when it detects the existing > page with > > the > > > > same URL, shows infobar asking user if they want to download another > copy > or > > > > cancel. > > > > - the blue button on Dino page goes disabled once clicked, partially > to > > > prevent > > > > clicking again - but if the user refreshes the page (resulting again > in > Dino > > > > page), the button is enabled again. > > > > - The Download button that is shown when user looks at Dino page is > enabled > > > all > > > > the time. > > > > > > > > These behaviors are inconsistent, so adding a check to more places > and > just > > > > ignoring user action would be adding to a 'quirky' behavior rather > then > > fixing > > > > it. > > > > > > > > We will work on a design doc that would propose a consistent UI > treatment > of > > > > these situations (likely using combination of showing the infobar in > more > > > > places, and disabling buttons). The reason we need that design doc > and not > > > just > > > > vote in the codereview log is that our UI designers should chime in, > to > keep > > > UI > > > > consistent with other pieces and behaviors. Once we have this done, > you > are > > > more > > > > than welcome to update the CL to this design doc - all the necessary > > > > communication will be on the bug you filed. We'll also add you to the > design > > > doc > > > > as well so you could chime in. Stay tuned! > > > > > > LGTM, thank you! > > > > Is this CL going to be closed out, or do you expect to create a new one? > > > > [My OCD is kicking in for keeping my code review dashboard clean] > > We could close this one and when we have a design doc (in about a week or > two) - > create a new one. > I'd let marcin@ close it... > > https://codereview.chromium.org/2698593002/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
