Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
800183005:
Adding UMA stats for dragdrop events for both touch and mouse. (Closed)
DescriptionAdding UMA stats for ash.dragdrop events for both touch and mouse.
BUG=318948
Committed: https://crrev.com/5a2d550f7b3b9a7ff41e0bc1e7fae0607c7d8488
Cr-Commit-Position: refs/heads/master@{#312868}
Patch Set 1 #Patch Set 2 : Adding UMA stats to track usage of DragAndDrop, on Chromeos, X11 and Windows. #
Total comments: 16
Patch Set 3 : Responding to comments, deliberate error in windows client to make sure that it is caught by the tr… #Patch Set 4 : Rebased, still with deliberate error in windows client. #Patch Set 5 : All errors removed, Adding UMA stats to track usage of DragAndDrop on ChromeOS, X11, and Windows. A… #
Total comments: 8
Patch Set 6 : Changed location of drag/cancel event stat collection in X11 client #Patch Set 7 : Added move_loop_->EndMoveLoop back to DragCancel #Patch Set 8 : Fixing issue that would have resulted in overcounting cancel events. #
Total comments: 17
Patch Set 9 : Adding DragDrop.ExternalOriginDrop, leaving in logging for teating on windows. #Patch Set 10 : Fixes needed for windows version - still has logging statements #Patch Set 11 : Windows now correctly identifying external origin events. #
Total comments: 2
Patch Set 12 : Moving location where ash DragDrop drop/cancel events are logged. #
Total comments: 2
Patch Set 13 : Adding an explicit value to DragEventSource enum #
Total comments: 7
Patch Set 14 : Moving location where x11 DragDrop drop/cancel events are logged. Changed DragDrop.X to Event.DragD… #
Total comments: 2
Patch Set 15 : Adressing nit re: unnecessary else statement #
Messages
Total messages: 38 (7 generated)
caelyn@chromium.org changed reviewers: + mfomitchev@chromium.org
Draft of the change to track drag and drop usage.
https://codereview.chromium.org/800183005/diff/20001/ash/drag_drop/drag_drop_... File ash/drag_drop/drag_drop_controller.cc (right): https://codereview.chromium.org/800183005/diff/20001/ash/drag_drop/drag_drop_... ash/drag_drop/drag_drop_controller.cc:164: UMA_HISTOGRAM_COUNTS("DragDrop.Touch.Start", 1); I think a single shared UMA_HISTOGRAM_ENUMERATION histogram would make more sense here. https://codereview.chromium.org/800183005/diff/20001/ash/drag_drop/drag_drop_... ash/drag_drop/drag_drop_controller.cc:321: StartCanceledAnimation(kCancelAnimationDuration); don't we need to log a Cancel here? https://codereview.chromium.org/800183005/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/800183005/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:6068: + <owner>rbyers@chromium.org</owner> You can put me as the owner for these https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:784: StartEndMoveLoopTimer(); Wouldn't this lead to cancelling the DnD? (meaning we should log a cancel)? https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:788: move_loop_->EndMoveLoop(); And this? https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:792: if (negotiated_operation_ != ui::DragDropTypes::DRAG_NONE) { What if the negotiated operation is NONE? Don't we need to log a cancel in that case as well? Rather than doing it in a bunch of different places, perhaps we could find a way to do it in EndMoveLoop()? https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_win.cc (right): https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_win.cc:64: if (drag_operation != ui::DragDropTypes::DRAG_NONE) { If the operation is not NONE, shouldn't we log a cancel?
The CQ bit was checked by mfomitchev@chromium.org
The CQ bit was unchecked by mfomitchev@chromium.org
Responded to all comments. https://codereview.chromium.org/800183005/diff/20001/ash/drag_drop/drag_drop_... File ash/drag_drop/drag_drop_controller.cc (right): https://codereview.chromium.org/800183005/diff/20001/ash/drag_drop/drag_drop_... ash/drag_drop/drag_drop_controller.cc:164: UMA_HISTOGRAM_COUNTS("DragDrop.Touch.Start", 1); On 2015/01/08 23:30:59, mfomitchev wrote: > I think a single shared UMA_HISTOGRAM_ENUMERATION histogram would make more > sense here. I made the changes in histograms.xml and elsewhere - was that what you had in mind? https://codereview.chromium.org/800183005/diff/20001/ash/drag_drop/drag_drop_... ash/drag_drop/drag_drop_controller.cc:321: StartCanceledAnimation(kCancelAnimationDuration); On 2015/01/08 23:30:59, mfomitchev wrote: > don't we need to log a Cancel here? Done. This changes the semantics of cancel to be logged whenever a DnD event doesn't end in a drop event. https://codereview.chromium.org/800183005/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/800183005/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:6068: + <owner>rbyers@chromium.org</owner> On 2015/01/08 23:30:59, mfomitchev wrote: > You can put me as the owner for these Done. https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:784: StartEndMoveLoopTimer(); On 2015/01/08 23:30:59, mfomitchev wrote: > Wouldn't this lead to cancelling the DnD? (meaning we should log a cancel)? See comment below. https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:788: move_loop_->EndMoveLoop(); On 2015/01/08 23:30:59, mfomitchev wrote: > And this? See comment below. https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:792: if (negotiated_operation_ != ui::DragDropTypes::DRAG_NONE) { On 2015/01/08 23:30:59, mfomitchev wrote: > What if the negotiated operation is NONE? Don't we need to log a cancel in that > case as well? Rather than doing it in a bunch of different places, perhaps we > could find a way to do it in EndMoveLoop()? EndMoveLoop is not always called in circumstances where a cancel event is occurring and I am not sure it makes sense to track it there. There is already a DragCancel method (which was previously never called, as far as codesearch will tell me, and I think it was added purely to conform to aura::client::DragDropClient), which I have modified to call EndMoveLoop and to track the cancel stats. https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_win.cc (right): https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_win.cc:64: if (drag_operation != ui::DragDropTypes::DRAG_NONE) { On 2015/01/08 23:30:59, mfomitchev wrote: > If the operation is not NONE, shouldn't we log a cancel? If the drag_operation is DRAG_NONE, then we know the either things were cancelled, or the object was dropped somewhere with no on drop functionality. If the drag_operation is *not* DRAG_NONE, then we know some kind of drop operation successfully completed. At least, that is my understanding of this code. I could be wrong. I have started tracking the 'drag_operation == DRAG_NONE' case as a cancel.
https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:792: if (negotiated_operation_ != ui::DragDropTypes::DRAG_NONE) { DragCancel() is at least called from web_contents_view_aura.cc.. I am wondering what will happen if the move loop ends itself (e.g. I think it can happen on a key press: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de...). I am also wondering if DragCancel/EndMoveLoop can be called without DnD being started.. Perhaps we should be logging drop/cancel in OnMoveLoopEnded()? Seems like that could be a decent catch-all. I've also noticed methods like OnXdndDrop, OnXdndFinished, etc. Any idea when do they get called? If you haven't done this already, perhaps build the Linux version, add a bunch of log statements, and run the binary doing DnD is various different ways to understand this code better and also validate your changes. If this is non-trivial to figure out, pkotwicz may be able to explain some of this to us. I was going to ask him some questions about how this code works myself. https://codereview.chromium.org/800183005/diff/60002/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/60002/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:10: #include <utility> why are these new imports needed? https://codereview.chromium.org/800183005/diff/60002/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:811: DragCancel(); Unless there is a good reason not to, just let it fall through to EndMoveLoop()/DragCancel() at the bottom. https://codereview.chromium.org/800183005/diff/60002/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:816: move_loop_->EndMoveLoop(); DragCancel()? https://codereview.chromium.org/800183005/diff/60002/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h (right): https://codereview.chromium.org/800183005/diff/60002/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h:11: #include <utility> Doesn't seem like this should be needed.
Comments addressed. https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/20001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:792: if (negotiated_operation_ != ui::DragDropTypes::DRAG_NONE) { On 2015/01/13 17:01:25, mfomitchev wrote: > DragCancel() is at least called from web_contents_view_aura.cc.. Oops. Should have checked from the virtual method, not the override method. > > I am wondering what will happen if the move loop ends itself (e.g. I think it > can happen on a key press: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de...). > I am also wondering if DragCancel/EndMoveLoop can be called without DnD being > started.. > > Perhaps we should be logging drop/cancel in OnMoveLoopEnded()? Seems like that > could be a decent catch-all. I think that makes the most sense. I wasn't able to produce a case where it wasn't called, including ending the drag and drop operation by hitting the escape key on keyboard, which I wasn't capturing as a cancel event before. > > I've also noticed methods like OnXdndDrop, OnXdndFinished, etc. Any idea when do > they get called? OnXdndDrop either gets called from SendXdndDrop or from a method in desktop_window_tree_host_x11.cc (I am not sure how to produce the later case). OnXdndFinished gets called from OnXdndDrop or from the same dispatch method in desktop_window_tree_host_x11.cc (again, not sure how to provoke that case). Both are only ever called from a successful drop operation. > If you haven't done this already, perhaps build the Linux > version, add a bunch of log statements, and run the binary doing DnD is various > different ways to understand this code better and also validate your changes. Already done. I also left the UMA_HISTOGRAM_ENUMERATION in the DragCancel method, just in case it somehow gets called from web_contents_view_aura.cc. https://codereview.chromium.org/800183005/diff/60002/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/60002/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:10: #include <utility> On 2015/01/13 17:01:25, mfomitchev wrote: > why are these new imports needed? See reasoning from the .h file. https://codereview.chromium.org/800183005/diff/60002/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:811: DragCancel(); On 2015/01/13 17:01:25, mfomitchev wrote: > Unless there is a good reason not to, just let it fall through to > EndMoveLoop()/DragCancel() at the bottom. Done. https://codereview.chromium.org/800183005/diff/60002/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:816: move_loop_->EndMoveLoop(); On 2015/01/13 17:01:25, mfomitchev wrote: > DragCancel()? Since OnMoveLoopEnded gets called from X11WholeScreenMoveLoop::EndMoveLoop(), I am reasonably certain that we can safely let this method fall to this point. (EndMoveLoop is a pure virtual method that is overridden only in X11WholeScreenMoveLoop and desktop_drag_drop_client_aurax11_unittest.cc) https://codereview.chromium.org/800183005/diff/60002/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h (right): https://codereview.chromium.org/800183005/diff/60002/ui/views/widget/desktop_... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.h:11: #include <utility> On 2015/01/13 17:01:25, mfomitchev wrote: > Doesn't seem like this should be needed. Added because lint suggested it, removing it and all other new imports that are not necessary for compilation.
Belatedly realized a serious issue with patch 7. Could not continue to live with it.
https://codereview.chromium.org/800183005/diff/130001/ash/drag_drop/drag_drop... File ash/drag_drop/drag_drop_controller.cc (right): https://codereview.chromium.org/800183005/diff/130001/ash/drag_drop/drag_drop... ash/drag_drop/drag_drop_controller.cc:336: UMA_HISTOGRAM_ENUMERATION("DragDrop.Cancel", current_drag_event_source_, There is at least one case where DoDragCancel is called direactly, so we should probably move the UMA logging there https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:732: // EndMoveLoop will call DesktopDragDropClientAuraX11::OnMoveLoopEnded and IMHO this is not really worth explaining in a comment here. https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_win.cc (right): https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_win.cc:69: if (drag_operation != ui::DragDropTypes::DRAG_NONE) { I'd check for '==' (and swap the bodies)
I'll post some code review comments after lunch
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
Looks good, a few review comments https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:615: drag_operation = delegate->OnPerformDrop(event); If you put a histogram here, you would be able to count how often people drag something into Chrome in total (from a different app & from within Chrome) I am unsure how useful such a histogram would be. I am not sure if it is possible to determine whether the drag occurred as a result of mouse or touch. https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:663: current_drag_event_source_ = source; Chrome for Desktop Linux does not support initiating drag and drop via touch at all. We would need to change X11WholeScreenMoveLoop::DispatchEvent() and find out whether we need to block events while a touch drag is in progress. https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:706: Can you move the histogram code for whether a drag & drop operation initiated by Chrome succeeded here?
Some questions about drag and drop on windows and a response to why I support multiple types of events when only mouse is supported for desktop chrome. https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:615: drag_operation = delegate->OnPerformDrop(event); On 2015/01/14 18:31:30, pkotwicz wrote: > If you put a histogram here, you would be able to count how often people drag > something into Chrome in total (from a different app & from within Chrome) > > I am unsure how useful such a histogram would be. I am not sure if it is > possible to determine whether the drag occurred as a result of mouse or touch. The real question is whether we can do the same thing on Windows. Do you happen to know off the top of your head if we can detect drag drop events, which originate from outside of chrome, successfully dropping something in chrome? https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:663: current_drag_event_source_ = source; On 2015/01/14 18:31:30, pkotwicz wrote: > Chrome for Desktop Linux does not support initiating drag and drop via touch at > all. We would need to change X11WholeScreenMoveLoop::DispatchEvent() and find > out whether we need to block events while a touch drag is in progress. I know that chrome on linux does not support touch events; but I thought I'd add support for them anyway since the added complexity is very small and it saves having ui::DragDropTypes::DRAG_EVENT_SOURCE_MOUSE duplicated three times (currently DragDrop.Start uses source; but if I switched to using a constant for the other two, I would switch it as well). I don't have strong feelings on it, since, right now, adding back support for touch events if they do start being supported is really small. If you feel the added complexity isn't worth it, I'll take it out. https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:706: On 2015/01/14 18:31:30, pkotwicz wrote: > Can you move the histogram code for whether a drag & drop operation initiated by > Chrome succeeded here? Acknowledged. - My tests show moving it here has the same behaviour as the previous location. Change coming in the next patch.
Please add a bug number to the change list description. This is a convention that we follow (even in cases where it does not add much value like here) https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:615: drag_operation = delegate->OnPerformDrop(event); I think that DesktopDropTargetWin::OnDrop() gets called upon a successful drop regardless of whether the source of the drag is Chrome. https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:663: current_drag_event_source_ = source; Once you move all of the histograms to this method you would be able to remove |current_drag_event_source_| so I am ok with passing |source| to UMA_HISTOGRAM_ENUMERATION().
Patch has now been tested on Windows. Drop events which originated externally from the given chrome browser are now being counted separately. https://codereview.chromium.org/800183005/diff/130001/ash/drag_drop/drag_drop... File ash/drag_drop/drag_drop_controller.cc (right): https://codereview.chromium.org/800183005/diff/130001/ash/drag_drop/drag_drop... ash/drag_drop/drag_drop_controller.cc:336: UMA_HISTOGRAM_ENUMERATION("DragDrop.Cancel", current_drag_event_source_, On 2015/01/14 17:03:41, mfomitchev wrote: > There is at least one case where DoDragCancel is called direactly, so we should > probably move the UMA logging there Done. https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:663: current_drag_event_source_ = source; On 2015/01/14 20:39:55, pkotwicz wrote: > Once you move all of the histograms to this method you would be able to remove > |current_drag_event_source_| so I am ok with passing |source| to > UMA_HISTOGRAM_ENUMERATION(). Done. https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:732: // EndMoveLoop will call DesktopDragDropClientAuraX11::OnMoveLoopEnded and On 2015/01/14 17:03:41, mfomitchev wrote: > IMHO this is not really worth explaining in a comment here. Done. https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_win.cc (right): https://codereview.chromium.org/800183005/diff/130001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_win.cc:69: if (drag_operation != ui::DragDropTypes::DRAG_NONE) { On 2015/01/14 17:03:41, mfomitchev wrote: > I'd check for '==' (and swap the bodies) Done.
https://codereview.chromium.org/800183005/diff/130001/ash/drag_drop/drag_drop... File ash/drag_drop/drag_drop_controller.cc (right): https://codereview.chromium.org/800183005/diff/130001/ash/drag_drop/drag_drop... ash/drag_drop/drag_drop_controller.cc:336: UMA_HISTOGRAM_ENUMERATION("DragDrop.Cancel", current_drag_event_source_, Doesn't look like you've moved it. However look at my new comment - I think it would be better to do it that way instead. https://codereview.chromium.org/800183005/diff/190001/ash/drag_drop/drag_drop... File ash/drag_drop/drag_drop_controller.cc (right): https://codereview.chromium.org/800183005/diff/190001/ash/drag_drop/drag_drop... ash/drag_drop/drag_drop_controller.cc:228: I wonder if we should follow the pattern established for X11 and Windows, and put logging of drop/cancel here? Then we won't have to worry about missing some obscure exit condition, plus the implementation would be consistent across platforms.
Moved location of logging of drop/cancel events for ash. https://codereview.chromium.org/800183005/diff/130001/ash/drag_drop/drag_drop... File ash/drag_drop/drag_drop_controller.cc (right): https://codereview.chromium.org/800183005/diff/130001/ash/drag_drop/drag_drop... ash/drag_drop/drag_drop_controller.cc:336: UMA_HISTOGRAM_ENUMERATION("DragDrop.Cancel", current_drag_event_source_, On 2015/01/21 18:54:36, mfomitchev wrote: > Doesn't look like you've moved it. However look at my new comment - I think it > would be better to do it that way instead. Oops. I confused this with another thing and hit done by mistake. https://codereview.chromium.org/800183005/diff/190001/ash/drag_drop/drag_drop... File ash/drag_drop/drag_drop_controller.cc (right): https://codereview.chromium.org/800183005/diff/190001/ash/drag_drop/drag_drop... ash/drag_drop/drag_drop_controller.cc:228: On 2015/01/21 18:54:36, mfomitchev wrote: > I wonder if we should follow the pattern established for X11 and Windows, and > put logging of drop/cancel here? Then we won't have to worry about missing some > obscure exit condition, plus the implementation would be consistent across > platforms. I think that makes sense - particularly since this pattern is followed on both of the other platforms. I tested it locally and the semantics of the logged events is the same.
LGTM
caelyn@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: Please review my changes adding UMA stats to track drag and drop usage in chrome. It also tracks successful drop events which originate from outside of chrome.
https://codereview.chromium.org/800183005/diff/210001/ui/base/dragdrop/drag_d... File ui/base/dragdrop/drag_drop_types.h (right): https://codereview.chromium.org/800183005/diff/210001/ui/base/dragdrop/drag_d... ui/base/dragdrop/drag_drop_types.h:26: DRAG_EVENT_SOURCE_COUNT It's bad to assume enumerations start with a particular value. Use explicit values if you need them.
Added an explicit value to the DragEventSource enum. https://codereview.chromium.org/800183005/diff/210001/ui/base/dragdrop/drag_d... File ui/base/dragdrop/drag_drop_types.h (right): https://codereview.chromium.org/800183005/diff/210001/ui/base/dragdrop/drag_d... ui/base/dragdrop/drag_drop_types.h:26: DRAG_EVENT_SOURCE_COUNT On 2015/01/21 22:53:15, sky wrote: > It's bad to assume enumerations start with a particular value. Use explicit > values if you need them. I poked around the code base and saw some enums using just =0 for the first element and others that were explicit throughout. I guessed at the first option being the standard.
LGTM
caelyn@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@chromium.org: Please review changes in histograms.xml
desktop_drag_drop_client_aurax11.cc LGTM modulo one important change request https://codereview.chromium.org/800183005/diff/230001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/230001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:709: if (negotiated_operation_ == ui::DragDropTypes::DRAG_NONE) { Can you please change the "alive if statement" to be the following: if (!alive) { UMA_HISTOGRAM_ENUMERATION("DragDrop.Cancel", source, ui::DragDropTypes::DRAG_EVENT_SOURCE_COUNT); return ui::DragDropTypes::DRAG_NONE; } if (negotiated_operation_ == ui::DragDropTypes::DRAG_NONE) { ... } else { ... } drag_widget_.reset() source_provider_ = NULL; ... Can you move the UMA histograms after the "alive if". If !|alive|, accessing |negotiated_operation_| will cause a crash. |alive| is false when the drag and drop source browser window is closed during the drag and drop operation. An evil browser extension can close the browser window during a drag and drop operation. I think that this case is covered by BookmarkBarViewTest22.*
https://codereview.chromium.org/800183005/diff/230001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/800183005/diff/230001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6092: +<histogram name="DragDrop.Cancel" enum="DragDropEventSource"> This seems to be adding a new top-level prefix. Can it be nested under an existing one? For example, looks like these are specific to Ash, so how about Ash.DragDrop.Cancel and such? https://codereview.chromium.org/800183005/diff/230001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6110: + a chrome window successfuly drops. Nit: capitalize Chrome
Comments addressed. https://codereview.chromium.org/800183005/diff/230001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/800183005/diff/230001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6092: +<histogram name="DragDrop.Cancel" enum="DragDropEventSource"> On 2015/01/22 20:02:20, Alexei Svitkine wrote: > This seems to be adding a new top-level prefix. Can it be nested under an > existing one? For example, looks like these are specific to Ash, so how about > Ash.DragDrop.Cancel and such? These are not Ash specific. How about Event.DragDrop.Cancel and such? https://codereview.chromium.org/800183005/diff/230001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6110: + a chrome window successfuly drops. On 2015/01/22 20:02:20, Alexei Svitkine wrote: > Nit: capitalize Chrome Done. https://codereview.chromium.org/800183005/diff/230001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/230001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:709: if (negotiated_operation_ == ui::DragDropTypes::DRAG_NONE) { On 2015/01/22 19:00:30, pkotwicz wrote: > Can you please change the "alive if statement" to be the following: > > if (!alive) { > UMA_HISTOGRAM_ENUMERATION("DragDrop.Cancel", source, > ui::DragDropTypes::DRAG_EVENT_SOURCE_COUNT); > return ui::DragDropTypes::DRAG_NONE; > } > > if (negotiated_operation_ == ui::DragDropTypes::DRAG_NONE) { > ... > } else { > ... > } > > drag_widget_.reset() > > source_provider_ = NULL; > ... > > Can you move the UMA histograms after the "alive if". If !|alive|, accessing > |negotiated_operation_| will cause a crash. |alive| is false when the drag and > drop source browser window is closed during the drag and drop operation. An evil > browser extension can close the browser window during a drag and drop operation. > I think that this case is covered by BookmarkBarViewTest22.* I moved if (negotiated_operation == ...) section inside the if alive section, and added an else to record a cancel and return DRAG_NONE
https://codereview.chromium.org/800183005/diff/230001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/800183005/diff/230001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6092: +<histogram name="DragDrop.Cancel" enum="DragDropEventSource"> On 2015/01/22 21:13:08, caelyn wrote: > On 2015/01/22 20:02:20, Alexei Svitkine wrote: > > This seems to be adding a new top-level prefix. Can it be nested under an > > existing one? For example, looks like these are specific to Ash, so how about > > Ash.DragDrop.Cancel and such? > > These are not Ash specific. How about Event.DragDrop.Cancel and such? Event.* SGTM
lgtm
https://codereview.chromium.org/800183005/diff/250001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/250001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:726: } else { Nit: No need for the "else" if (alive) { ... return negotiated_operation_; } UMA_HISTOGRAM_ENUMERATION("Event.DragDrop.Cancel", source, ui::DragDropTypes::DRAG_EVENT_SOURCE_COUNT); return ui::DragDropTypes::DRAG_NONE; Thanks for addressing my comments!
Final nit addressed. https://codereview.chromium.org/800183005/diff/250001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right): https://codereview.chromium.org/800183005/diff/250001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:726: } else { On 2015/01/22 21:48:34, pkotwicz wrote: > Nit: No need for the "else" > > if (alive) { > ... > return negotiated_operation_; > } > UMA_HISTOGRAM_ENUMERATION("Event.DragDrop.Cancel", source, > ui::DragDropTypes::DRAG_EVENT_SOURCE_COUNT); > return ui::DragDropTypes::DRAG_NONE; > > Thanks for addressing my comments! Nit addressed. Thanks for giving them! :-)
The CQ bit was checked by caelyn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/800183005/270001
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/5a2d550f7b3b9a7ff41e0bc1e7fae0607c7d8488 Cr-Commit-Position: refs/heads/master@{#312868} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
