Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
258793004:
Dragging shouldn't block interstitial page. (Closed)
|
Created:
6 years, 8 months ago by mmal Modified:
6 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDragging shouldn't block interstitial page.
Interstitial page should send DragMsg_SourceSystemDragEnded message
even if it is not supporting dragging. For similar issue see
http://crbug.com/157134.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267237
Patch Set 1 #
Total comments: 2
Patch Set 2 : Changed NOTREACHED to DVLOG. #Messages
Total messages: 31 (0 generated)
dcheng@ is the drag-n-drop expert, so I'll defer to him for review.
https://codereview.chromium.org/258793004/diff/1/content/browser/frame_host/i... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/258793004/diff/1/content/browser/frame_host/i... content/browser/frame_host/interstitial_page_impl.cc:876: interstitial_page_->render_view_host_->DragSourceSystemDragEnded(); Should this be calling web_contents_->SystemDragEnded() instead?
On 2014/04/25 18:25:10, dcheng wrote: > https://codereview.chromium.org/258793004/diff/1/content/browser/frame_host/i... > File content/browser/frame_host/interstitial_page_impl.cc (right): > > https://codereview.chromium.org/258793004/diff/1/content/browser/frame_host/i... > content/browser/frame_host/interstitial_page_impl.cc:876: > interstitial_page_->render_view_host_->DragSourceSystemDragEnded(); > Should this be calling web_contents_->SystemDragEnded() instead? No, it's original page web_contents but drag is stared for interstitial.
OK, lgtm then for dnd.
The CQ bit was checked by mmaliszkiewicz@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mmaliszkiewicz@opera.com/258793004/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
Rubberstamp LGTM.
The CQ bit was checked by mmaliszkiewicz@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mmaliszkiewicz@opera.com/258793004/1
The CQ bit was unchecked by creis@chromium.org
Not LGTM. See comment below. https://codereview.chromium.org/258793004/diff/1/content/browser/frame_host/i... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/258793004/diff/1/content/browser/frame_host/i... content/browser/frame_host/interstitial_page_impl.cc:877: NOTREACHED() << "InterstitialPage does not support dragging yet."; Wait, this isn't ok to land. NOTREACHED implies we can't get here, but obviously we can. This shouldn't be here.
Also, as always I'd like to see a test to prevent regression, but I'll defer to dcheng about whether that's tricky to write for this case. In general, please always put consideration into writing a test, and mention why it wasn't added in cases that it's not needed or feasible.
Drag and drop tests have always been tricky to write. If you're feeling particularly brave, jam@ mentioned that we might be able to use the same approach as the tab dragging tests. I haven't looked into this to see how viable it is, but I keep meaning to do it Real Soon Now.
On 2014/04/28 18:10:24, dcheng wrote: > Drag and drop tests have always been tricky to write. If you're feeling > particularly brave, jam@ mentioned that we might be able to use the same > approach as the tab dragging tests. I haven't looked into this to see how viable > it is, but I keep meaning to do it Real Soon Now. Ok, hopefully we can get to where those tests are easier to write soon. For this CL, we should either remove this NOTREACHED or change it to a VLOG/DVLOG if there's more work to be done. Don't worry about the other NOTREACHEDs in the class, since many of those are probably buggy as well.
lgtm
The CQ bit was unchecked by mmaliszkiewicz@opera.com
The CQ bit was checked by mmaliszkiewicz@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mmaliszkiewicz@opera.com/258793004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium
The CQ bit was checked by mmaliszkiewicz@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mmaliszkiewicz@opera.com/258793004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mmaliszkiewicz@opera.com/258793004/20001
Message was sent while issue was closed.
Change committed as 267237 |
