|
|
Created:
4 years, 11 months ago by oshima Modified:
4 years, 10 months ago CC:
blink-reviews, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, gavinp+loader_chromium.org, jam, Nate Chapin, kinuko+watch, loading-reviews_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, nona+watch_chromium.org, shuchen+watch_chromium.org, James Su, tyoshino+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[UseZoomForDSF] Support drag&drop
* Scale drag/drop events to Viewport in RenderViewHostImpl.
* Scale the drag offset to Window/DIP in RenderViewImpl.
* Use the screen info's scale factor to tag the drag image.
* Make sure that the target image size passed to clampedImageScale is in DIP.
BUG=485650
TEST=manual. I'll add a unit test after https://codereview.chromium.org/1586923002/ is landed.
Committed: https://crrev.com/b331323047cf6c2c3fb288f45573f2897a5168da
Cr-Commit-Position: refs/heads/master@{#371950}
Patch Set 1 : #Patch Set 2 : #Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 2
Patch Set 5 : added comment #
Total comments: 1
Patch Set 6 : #Patch Set 7 : guest view support #
Messages
Total messages: 59 (32 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Generate drag image at the right device scale BUG= patch from issue 1598593002 at patchset 60001 (http://crrev.com/1598593002#ps60001) ========== to ========== Generate drag image at the right device scale BUG=485650 ==========
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1605143003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1605143003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1605143003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1605143003/100001
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Patchset #1 (id:100001) has been deleted
oshima@chromium.org changed reviewers: + dcheng@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1605143003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1605143003/120001
Description was changed from ========== Generate drag image at the right device scale BUG=485650 ========== to ========== [UseZoomForDSF] Support drag&drop * Scale drag/drop events to Viewport in RenderViewHostImpl. * Scale the drag offset to Window/DIP in RenderViewImpl. * Use the screen info's scale factor to tag the drag image. BUG=485650 ==========
Description was changed from ========== [UseZoomForDSF] Support drag&drop * Scale drag/drop events to Viewport in RenderViewHostImpl. * Scale the drag offset to Window/DIP in RenderViewImpl. * Use the screen info's scale factor to tag the drag image. BUG=485650 ========== to ========== [UseZoomForDSF] Support drag&drop * Scale drag/drop events to Viewport in RenderViewHostImpl. * Scale the drag offset to Window/DIP in RenderViewImpl. * Use the screen info's scale factor to tag the drag image. * Make sure that the target image size passed to clampedImageScale is in DIP. BUG=485650 ==========
Description was changed from ========== [UseZoomForDSF] Support drag&drop * Scale drag/drop events to Viewport in RenderViewHostImpl. * Scale the drag offset to Window/DIP in RenderViewImpl. * Use the screen info's scale factor to tag the drag image. * Make sure that the target image size passed to clampedImageScale is in DIP. BUG=485650 ========== to ========== [UseZoomForDSF] Support drag&drop * Scale drag/drop events to Viewport in RenderViewHostImpl. * Scale the drag offset to Window/DIP in RenderViewImpl. * Use the screen info's scale factor to tag the drag image. * Make sure that the target image size passed to clampedImageScale is in DIP. BUG=485650 TEST=manual. I'll add a unit test after https://codereview.chromium.org/1586923002/ is landed. ==========
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1605143003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1605143003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1605143003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1605143003/160001
https://codereview.chromium.org/1605143003/diff/160001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1605143003/diff/160001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:364: gfx::Point convertDipToViewport(const gfx::Point& point); This should be named ConvertDipToViewport() https://codereview.chromium.org/1605143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1605143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrame.cpp:135: float screenDeviceScaleFactor = m_localFrame->page()->chromeClient().screenInfo().deviceScaleFactor; When should code use FrameHost::deviceScaleFactor()? The difference to me isn't very clear.
https://codereview.chromium.org/1605143003/diff/160001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1605143003/diff/160001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:364: gfx::Point convertDipToViewport(const gfx::Point& point); On 2016/01/20 22:48:11, dcheng wrote: > This should be named ConvertDipToViewport() Done. https://codereview.chromium.org/1605143003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/1605143003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrame.cpp:135: float screenDeviceScaleFactor = m_localFrame->page()->chromeClient().screenInfo().deviceScaleFactor; On 2016/01/20 22:48:11, dcheng wrote: > When should code use FrameHost::deviceScaleFactor()? The difference to me isn't > very clear. The code above (that uses FrameHost::deviceScaleFactor) is needed for existing (old) scheme. It will be removed when all platforms are migrated to the new scheme.
ping?
https://codereview.chromium.org/1605143003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/DragController.cpp (right): https://codereview.chromium.org/1605143003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/DragController.cpp:893: dragImage = dragImageForImage(element, image, dragOrigin, imageRect.location(), imageSizeInDIP, dragLocation); I don't really understand what transformations are needed and when. Is there better documentation about this? Why are some things scaled to viewport, but others to dips?
uploaded new patch with the comment. PTAL. https://codereview.chromium.org/1605143003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/DragController.cpp (right): https://codereview.chromium.org/1605143003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/DragController.cpp:893: dragImage = dragImageForImage(element, image, dragOrigin, imageRect.location(), imageSizeInDIP, dragLocation); On 2016/01/22 17:37:41, dcheng wrote: > I don't really understand what transformations are needed and when. Is there > better documentation about this? Why are some things scaled to viewport, but > others to dips? This is because dragImageForImage clips the image in screen coordinates in DIP, which means that when it's drawn on high DPI display it becomes blurry. I'm not sure if that's intentional or it was left unimplemented. I can work on a fix (but in a separate CL) unless there is a reason not to so. Please let me know.
> This is because dragImageForImage clips the image in screen coordinates in DIP, > which means that when it's drawn on high DPI display it becomes blurry. > I'm not sure if that's intentional or it was left unimplemented. I can work on a fix > (but in a separate CL) unless there is a reason not to so. > > Please let me know. How hard/how much code would it be to fix it in this CL? It's really a bit odd that one part of the rect is transformed. https://codereview.chromium.org/1605143003/diff/200001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1605143003/diff/200001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:358: // Convers the dip to viewport coordinates. Nit: Converts a point from DIPs to viewport coordinates or just remove it (the comment is pretty obvious from the function signature) or link to explanation of difference between DIPs and viewport coordinates.
On 2016/01/26 18:52:15, dcheng wrote: > > This is because dragImageForImage clips the image in screen coordinates in > DIP, > > which means that when it's drawn on high DPI display it becomes blurry. > > I'm not sure if that's intentional or it was left unimplemented. I can work on > a fix > > (but in a separate CL) unless there is a reason not to so. > > > > Please let me know. > > How hard/how much code would it be to fix it in this CL? It's really a bit odd > that one part of the rect is transformed. It'll change the behavior, while this will no, and I do not want to mix two. Is it ok to create a patch on top of this? I can wait this until you approve that CL. > > https://codereview.chromium.org/1605143003/diff/200001/content/browser/render... > File content/browser/renderer_host/render_view_host_impl.h (right): > > https://codereview.chromium.org/1605143003/diff/200001/content/browser/render... > content/browser/renderer_host/render_view_host_impl.h:358: // Convers the dip to > viewport coordinates. > Nit: > Converts a point from DIPs to viewport coordinates > or > just remove it (the comment is pretty obvious from the function signature) > or > link to explanation of difference between DIPs and viewport coordinates. Removed.
On 2016/01/26 at 20:10:43, oshima wrote: > On 2016/01/26 18:52:15, dcheng wrote: > > > This is because dragImageForImage clips the image in screen coordinates in > > DIP, > > > which means that when it's drawn on high DPI display it becomes blurry. > > > I'm not sure if that's intentional or it was left unimplemented. I can work on > > a fix > > > (but in a separate CL) unless there is a reason not to so. > > > > > > Please let me know. > > > > How hard/how much code would it be to fix it in this CL? It's really a bit odd > > that one part of the rect is transformed. > > It'll change the behavior, while this will no, and I do not want to mix two. Is it ok to create a patch on top of this? > I can wait this until you approve that CL. Sounds good. Thanks! > > > > > https://codereview.chromium.org/1605143003/diff/200001/content/browser/render... > > File content/browser/renderer_host/render_view_host_impl.h (right): > > > > https://codereview.chromium.org/1605143003/diff/200001/content/browser/render... > > content/browser/renderer_host/render_view_host_impl.h:358: // Convers the dip to > > viewport coordinates. > > Nit: > > Converts a point from DIPs to viewport coordinates > > or > > just remove it (the comment is pretty obvious from the function signature) > > or > > link to explanation of difference between DIPs and viewport coordinates. > > Removed.
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1605143003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1605143003/220001
The CQ bit was unchecked by oshima@chromium.org
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1605143003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1605143003/240001
The CQ bit was unchecked by oshima@chromium.org
Patchset #7 (id:240001) has been deleted
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1605143003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1605143003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by oshima@chromium.org
The CQ bit was unchecked by oshima@chromium.org
oshima@chromium.org changed reviewers: + sievers@chromium.org
+sievers@ for content/ OWNER
lgtm. though seems a bit unfortunate that the distinction now bleeds into more places rather than staying in the ui and platform layers.
On 2016/01/27 23:11:05, sievers wrote: > lgtm. though seems a bit unfortunate that the distinction now bleeds into more > places rather than staying in the ui and platform layers. Is it possible to use WebInputEvent for drag & drop instead of having separate IPC message? Just an idea for future improvement because the conversion for WebInputEvent happens in ui / platform layer.
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1605143003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1605143003/260001
Message was sent while issue was closed.
Description was changed from ========== [UseZoomForDSF] Support drag&drop * Scale drag/drop events to Viewport in RenderViewHostImpl. * Scale the drag offset to Window/DIP in RenderViewImpl. * Use the screen info's scale factor to tag the drag image. * Make sure that the target image size passed to clampedImageScale is in DIP. BUG=485650 TEST=manual. I'll add a unit test after https://codereview.chromium.org/1586923002/ is landed. ========== to ========== [UseZoomForDSF] Support drag&drop * Scale drag/drop events to Viewport in RenderViewHostImpl. * Scale the drag offset to Window/DIP in RenderViewImpl. * Use the screen info's scale factor to tag the drag image. * Make sure that the target image size passed to clampedImageScale is in DIP. BUG=485650 TEST=manual. I'll add a unit test after https://codereview.chromium.org/1586923002/ is landed. ==========
Message was sent while issue was closed.
Committed patchset #7 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [UseZoomForDSF] Support drag&drop * Scale drag/drop events to Viewport in RenderViewHostImpl. * Scale the drag offset to Window/DIP in RenderViewImpl. * Use the screen info's scale factor to tag the drag image. * Make sure that the target image size passed to clampedImageScale is in DIP. BUG=485650 TEST=manual. I'll add a unit test after https://codereview.chromium.org/1586923002/ is landed. ========== to ========== [UseZoomForDSF] Support drag&drop * Scale drag/drop events to Viewport in RenderViewHostImpl. * Scale the drag offset to Window/DIP in RenderViewImpl. * Use the screen info's scale factor to tag the drag image. * Make sure that the target image size passed to clampedImageScale is in DIP. BUG=485650 TEST=manual. I'll add a unit test after https://codereview.chromium.org/1586923002/ is landed. Committed: https://crrev.com/b331323047cf6c2c3fb288f45573f2897a5168da Cr-Commit-Position: refs/heads/master@{#371950} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b331323047cf6c2c3fb288f45573f2897a5168da Cr-Commit-Position: refs/heads/master@{#371950} |