Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
481483002:
DevTools: backend part of the color picker (behind experiment). (Closed)
|
Created:
6 years, 4 months ago by pfeldman Modified:
6 years, 4 months ago CC:
chromium-reviews, vsevik, jam, yurys, paulirish+reviews_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionDevTools: backend part of the color picker (behind experiment).
NOTRY=true
BUG=404375
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290185
Patch Set 1 #
Total comments: 2
Patch Set 2 : Same using Add/RemoveMouseEventCallback. #Patch Set 3 : Same with custom Linux cursor #Patch Set 4 : #Patch Set 5 : #
Total comments: 6
Patch Set 6 : #Patch Set 7 : Make bots happy. #
Total comments: 7
Patch Set 8 : For landing. #Patch Set 9 : float... #
Total comments: 2
Messages
Total messages: 22 (0 generated)
https://codereview.chromium.org/481483002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/481483002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:865: if (!intercepting_mouse_callback_.is_null()) { Can you not use the existing AddMouseEventCallback/RemoveMouseEventCallback interface?
https://codereview.chromium.org/481483002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/481483002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:865: if (!intercepting_mouse_callback_.is_null()) { I can. Was not expecting things to be so simple. Thanks.
Please add a BUG and screenshot. Overall, this seems pretty lightweight. https://codereview.chromium.org/481483002/diff/80001/content/browser/devtools... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/481483002/diff/80001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:936: SetColorPickerEnabled(false); Why do you disable color picker here, but not below (where you get the actual color)? https://codereview.chromium.org/481483002/diff/80001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:1043: cursor_info.image_scale_factor = 1; You need to set this to 2.0 on Retina for smoothness, don't you? https://codereview.chromium.org/481483002/diff/80001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:1057: void RendererOverridesHandler::UpdateColorPickerCursorSmall(int x, int y) { This is not called from anywhere. Will it be used?
https://codereview.chromium.org/481483002/diff/80001/content/browser/devtools... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/481483002/diff/80001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:936: SetColorPickerEnabled(false); I used to disable it below, now I don't for the UX reasons. I'll remove this line. https://codereview.chromium.org/481483002/diff/80001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:1043: cursor_info.image_scale_factor = 1; Yep. Will follow up with that. https://codereview.chromium.org/481483002/diff/80001/content/browser/devtools... content/browser/devtools/renderer_overrides_handler.cc:1057: void RendererOverridesHandler::UpdateColorPickerCursorSmall(int x, int y) { Forgot to nuke this one (I used to have two independent cursor paints for Mac, Win and Linux).
Bug filed, video uploaded, comments addressed.
lgtm https://codereview.chromium.org/481483002/diff/190001/content/browser/devtool... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/481483002/diff/190001/content/browser/devtool... content/browser/devtools/renderer_overrides_handler.cc:975: float device_scale_factor = gfx::Screen::GetNativeScreen()-> It's better to get this via |host_->GetScreenInfo|, which works right in case of multiple screens. https://codereview.chromium.org/481483002/diff/190001/content/browser/devtool... content/browser/devtools/renderer_overrides_handler.cc:992: canvas->drawLine(kHotspotOffset + kHotspotRadius - 1, This is actually |kHotspotOffset + kHotspotRadius / sqrt(2)|, right? Introduce a const with right value? https://codereview.chromium.org/481483002/diff/190001/content/browser/devtool... content/browser/devtools/renderer_overrides_handler.cc:1013: // Paint outline circle. Wrong comment.
https://codereview.chromium.org/481483002/diff/190001/content/browser/devtool... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/481483002/diff/190001/content/browser/devtool... content/browser/devtools/renderer_overrides_handler.cc:975: float device_scale_factor = gfx::Screen::GetNativeScreen()-> On 2014/08/17 04:28:59, dgozman wrote: > It's better to get this via |host_->GetScreenInfo|, which works right in case of > multiple screens. Done. https://codereview.chromium.org/481483002/diff/190001/content/browser/devtool... content/browser/devtools/renderer_overrides_handler.cc:992: canvas->drawLine(kHotspotOffset + kHotspotRadius - 1, On 2014/08/17 04:28:59, dgozman wrote: > This is actually |kHotspotOffset + kHotspotRadius / sqrt(2)|, right? Introduce a > const with right value? Done. https://codereview.chromium.org/481483002/diff/190001/content/browser/devtool... content/browser/devtools/renderer_overrides_handler.cc:992: canvas->drawLine(kHotspotOffset + kHotspotRadius - 1, On 2014/08/17 04:28:59, dgozman wrote: > This is actually |kHotspotOffset + kHotspotRadius / sqrt(2)|, right? Introduce a > const with right value? Done. https://codereview.chromium.org/481483002/diff/190001/content/browser/devtool... content/browser/devtools/renderer_overrides_handler.cc:1013: // Paint outline circle. On 2014/08/17 04:28:59, dgozman wrote: > Wrong comment. Done.
The CQ bit was checked by pfeldman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pfeldman@chromium.org/481483002/210001
The CQ bit was unchecked by pfeldman@chromium.org
The CQ bit was checked by pfeldman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pfeldman@chromium.org/481483002/230001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pfeldman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pfeldman@chromium.org/481483002/230001
Message was sent while issue was closed.
Committed patchset #9 (230001) as 290185
Message was sent while issue was closed.
FYI https://codereview.chromium.org/481483002/diff/230001/content/browser/devtool... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/481483002/diff/230001/content/browser/devtool... content/browser/devtools/renderer_overrides_handler.cc:194: if (color_picker_enabled_) there is "&& host" check above before accessing host. might be an error here.
Message was sent while issue was closed.
https://codereview.chromium.org/481483002/diff/230001/content/browser/devtool... File content/browser/devtools/renderer_overrides_handler.cc (right): https://codereview.chromium.org/481483002/diff/230001/content/browser/devtool... content/browser/devtools/renderer_overrides_handler.cc:194: if (color_picker_enabled_) This should not happen, but I agree I should follow up with the check.
Message was sent while issue was closed.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
