Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2596553003:
Implements Touch calibrator controller and a skeleton for touch calibrator view. (Closed)
|
Created:
4 years ago by malaykeshav Modified:
3 years, 12 months ago CC:
chromium-reviews, davemoore+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplements Touch calibrator controller and a skeleton for touch calibrator view.
Implements TouchCalibratorController class that is responsible for
collecting the touch calibration associated data from the user. It uses
TouchCalibratorView class to present an interface the user can interact
with.
Also includes unittests for TouhcCalibratorController.
Design Doc=go/cros-touch-calibration
BUG=667921, 634166
COMPONENT=Touch Calibration, ChromeOS, Unittests
Committed: https://crrev.com/9fe4c53f0a4a1d6a983e34ce0d971740b989d8d1
Cr-Commit-Position: refs/heads/master@{#440502}
Patch Set 1 : Implements Touch calibrator controller and a skeleton for touch calibrator view. #
Total comments: 44
Patch Set 2 : Resolved Comments #
Total comments: 10
Patch Set 3 : Resolved comments #
Dependent Patchsets: Messages
Total messages: 46 (28 generated)
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
malaykeshav@chromium.org changed reviewers: + stevenjb@chromium.org, xiyuan@chromium.org
PTAL
Shouldn't this code be in src/ui/ ? We are actively trying to reduce the chromeos specific code in src/chrome/.
On 2016/12/21 at 18:04:49, stevenjb wrote: > Shouldn't this code be in src/ui/ ? We are actively trying to reduce the chromeos specific code in src/chrome/. I was trying to keep the overscan calibrator and the touch calibrator together. I think we can keep the touch_calibrator_controller with overscan calibrator while the touch_calibrator_view can be moved to src/ui to keep things consistent.
On 2016/12/22 00:38:38, malaykeshav wrote: > On 2016/12/21 at 18:04:49, stevenjb wrote: > > Shouldn't this code be in src/ui/ ? We are actively trying to reduce the > chromeos specific code in src/chrome/. > > I was trying to keep the overscan calibrator and the touch calibrator together. > I think we can keep the touch_calibrator_controller with > overscan calibrator while the touch_calibrator_view can be moved to src/ui to > keep things consistent. OK, we should move overscan_calibrator also. Really we should do that first, but since there is apparently time pressure here we can move both of them in a follow-up, but please file an issue to make sure that we do so.
https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc (right): https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:21: const auto kTouchIntervalThreshold = base::TimeDelta::FromMilliseconds(200); If this is used in a test it should really be a static member defined in the header. Also, this is not a good use of auto, explicitly declare the type here. https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:45: if (GetDisplayPointStateIndex(state) + 1) nit: This is confusing, just use if (GetDisplayPointStateIndex(state) >= 0) https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:48: return true; nit: Test this first (simpler test) https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:87: display, display.id() == target_display_.id()); nit: Define a local var for display.id() == target_display_.id() to document what the argument is. https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:92: // ->SetForCalibration(true); We generally avoid committing commented out code like this, instead just: TODO(malaykeshav): Call TouchTransformerController::SetForCalibration() https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:105: // ->SetForCalibration(false); ditto https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:127: LOG(ERROR) << "Received touch event"; VLOG or remove https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:148: int state_index = GetDisplayPointStateIndex(calibrator_view_state); We also call this in CanTouch, which is only called from here, it would probably be better/simpler just to inline CanTouch(). https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:151: if (state_index + 1) { state_index >= 0 https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:155: std::make_pair(display_point, touch->location()); {} https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.h (right): https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.h:42: // WindowTreeHostManager::Observer: nit: Style should be consistent with line 38. I prefer just '// classname', e.g. '// ui::EventHandler' and '// WindowTreeHostManager::Observer'. https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc (right): https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc:22: namespace chromeos { nit: blank line https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc:24: const auto kTouchIntervalThreshold = base::TimeDelta::FromMilliseconds(200); See other comment. https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc:35: int64_t target_display_id = display_id_list[1]; Why [1]? 1 should probably be a defined constant, along with "500x500,500x500" above. https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc:48: EXPECT_EQ(ctrl->touch_calibrator_views_.size(), 2UL); 2 should probably also be a constant. https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:14: namespace chromeos { blank line https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:17: const char kWidgetName[] = "TouchCalibratorOverlay"; constexpr https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:54: } blank line https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:55: // views::View override: Elim. unnecessary comments, these should just be in the header. https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h (right): https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:64: State GetState() { return state_; } state() https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:67: void NextState(); AdvanceToNextState() https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:87: State state_ = UNKNOWN; blank line
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Resolved comments. PTAL https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc (right): https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:21: const auto kTouchIntervalThreshold = base::TimeDelta::FromMilliseconds(200); On 2016/12/22 at 01:14:20, stevenjb wrote: > If this is used in a test it should really be a static member defined in the header. > Also, this is not a good use of auto, explicitly declare the type here. Done. https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:45: if (GetDisplayPointStateIndex(state) + 1) On 2016/12/22 at 01:14:20, stevenjb wrote: > nit: This is confusing, just use if (GetDisplayPointStateIndex(state) >= 0) Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:48: return true; On 2016/12/22 at 01:14:20, stevenjb wrote: > nit: Test this first (simpler test) Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:87: display, display.id() == target_display_.id()); On 2016/12/22 at 01:14:20, stevenjb wrote: > nit: Define a local var for display.id() == target_display_.id() to document what the argument is. Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:92: // ->SetForCalibration(true); On 2016/12/22 at 01:14:20, stevenjb wrote: > We generally avoid committing commented out code like this, instead just: > > TODO(malaykeshav): Call TouchTransformerController::SetForCalibration() Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:105: // ->SetForCalibration(false); On 2016/12/22 at 01:14:20, stevenjb wrote: > ditto Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:127: LOG(ERROR) << "Received touch event"; On 2016/12/22 at 01:14:20, stevenjb wrote: > VLOG or remove Remnant from when I was testing. Forgot to remove. Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:148: int state_index = GetDisplayPointStateIndex(calibrator_view_state); On 2016/12/22 at 01:14:20, stevenjb wrote: > We also call this in CanTouch, which is only called from here, it would probably be better/simpler just to inline CanTouch(). Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:151: if (state_index + 1) { On 2016/12/22 at 01:14:20, stevenjb wrote: > state_index >= 0 Removed. Not required. https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:155: std::make_pair(display_point, touch->location()); On 2016/12/22 at 01:14:20, stevenjb wrote: > {} done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.h (right): https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.h:42: // WindowTreeHostManager::Observer: On 2016/12/22 at 01:14:21, stevenjb wrote: > nit: Style should be consistent with line 38. I prefer just '// classname', e.g. '// ui::EventHandler' and '// WindowTreeHostManager::Observer'. Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc (right): https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc:22: namespace chromeos { On 2016/12/22 at 01:14:21, stevenjb wrote: > nit: blank line Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc:24: const auto kTouchIntervalThreshold = base::TimeDelta::FromMilliseconds(200); On 2016/12/22 at 01:14:21, stevenjb wrote: > See other comment. Removed. https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc:35: int64_t target_display_id = display_id_list[1]; On 2016/12/22 at 01:14:21, stevenjb wrote: > Why [1]? 1 should probably be a defined constant, along with "500x500,500x500" above. Initialized a constant instead of rvalue. Through out the codebase I have seen the use of UpdateDisplay() with a string literal as above. https://cs.chromium.org/search/?q=%22UpdateDisplay(%5C%22%22+f:test&sq=packag... Have added a comment on what spec "500x500,500x500". Will change it to a constant if required. https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller_unittest.cc:48: EXPECT_EQ(ctrl->touch_calibrator_views_.size(), 2UL); On 2016/12/22 at 01:14:21, stevenjb wrote: > 2 should probably also be a constant. Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:14: namespace chromeos { On 2016/12/22 at 01:14:21, stevenjb wrote: > blank line Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:17: const char kWidgetName[] = "TouchCalibratorOverlay"; On 2016/12/22 at 01:14:21, stevenjb wrote: > constexpr Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:54: } On 2016/12/22 at 01:14:21, stevenjb wrote: > blank line Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:55: // views::View override: On 2016/12/22 at 01:14:21, stevenjb wrote: > Elim. unnecessary comments, these should just be in the header. Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h (right): https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:64: State GetState() { return state_; } On 2016/12/22 at 01:14:21, stevenjb wrote: > state() Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:67: void NextState(); On 2016/12/22 at 01:14:21, stevenjb wrote: > AdvanceToNextState() Done https://codereview.chromium.org/2596553003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:87: State state_ = UNKNOWN; On 2016/12/22 at 01:14:21, stevenjb wrote: > blank line Done
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xiyuan@chromium.org changed reviewers: + sadrul@chromium.org
High level comment: Directly access to Shell instance and using pre target handler is discouraged in new chrome code. Ash is moving towards mash where it runs in its own process. We are migrating old code that is doing this and probably should not add more. The code probably should live in ash with the TouchTransformerController. +sadrul for his input.
On 2016/12/22 18:10:12, xiyuan wrote: > High level comment: > > Directly access to Shell instance and using pre target handler is discouraged in > new chrome code. Ash is moving towards mash where it runs in its own process. We > are migrating old code that is doing this and probably should not add more. > > The code probably should live in ash with the TouchTransformerController. > > +sadrul for his input. I agree. It doesn't look like this new calibration-controller has any dependency on other parts of chrome anyway, so it should be easy to move this into ash. Ideally, this would be a completely separate module that does not have ash dependency either (so, in //ui/, for example). That would make it possible to easily put this into a separate mojo/mus app, so that it is loaded only when necessary (see //ash/touch_hud/mus, for example). But even if we don't do that right now, we need to at least keep this out of //chrome.
On 2016/12/22 18:23:00, sadrul wrote: > On 2016/12/22 18:10:12, xiyuan wrote: > > High level comment: > > > > Directly access to Shell instance and using pre target handler is discouraged > in > > new chrome code. Ash is moving towards mash where it runs in its own process. > We > > are migrating old code that is doing this and probably should not add more. > > > > The code probably should live in ash with the TouchTransformerController. > > > > +sadrul for his input. > > I agree. It doesn't look like this new calibration-controller has any dependency > on other parts of chrome anyway, so it should be easy to move this into ash. > > Ideally, this would be a completely separate module that does not have ash > dependency either (so, in //ui/, for example). That would make it possible to > easily put this into a separate mojo/mus app, so that it is loaded only when > necessary (see //ash/touch_hud/mus, for example). But even if we don't do that > right now, we need to at least keep this out of //chrome. I made the comment earlier that this code belongs in src/ui or src/ash. It was pointed out that it should reside with the touch calibration code, which also should be moved out of src/chrome, so we agreed to move both of them in a follow-up CL. See crbug.com/676513.
lgtm https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc (right): https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:53: bool is_target_display = display.id() == target_display_.id(); nit: In the constructor the parameter is 'is_primary_view', these should be consistent. https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:126: return; nit: move early return and comment into 'default' case above.
On 2016/12/22 18:28:33, stevenjb wrote: > On 2016/12/22 18:23:00, sadrul wrote: > > On 2016/12/22 18:10:12, xiyuan wrote: > > > High level comment: > > > > > > Directly access to Shell instance and using pre target handler is > discouraged > > in > > > new chrome code. Ash is moving towards mash where it runs in its own > process. > > We > > > are migrating old code that is doing this and probably should not add more. > > > > > > The code probably should live in ash with the TouchTransformerController. > > > > > > +sadrul for his input. > > > > I agree. It doesn't look like this new calibration-controller has any > dependency > > on other parts of chrome anyway, so it should be easy to move this into ash. > > > > Ideally, this would be a completely separate module that does not have ash > > dependency either (so, in //ui/, for example). That would make it possible to > > easily put this into a separate mojo/mus app, so that it is loaded only when > > necessary (see //ash/touch_hud/mus, for example). But even if we don't do that > > right now, we need to at least keep this out of //chrome. > > I made the comment earlier that this code belongs in src/ui or src/ash. It was > pointed out that it should reside with the touch calibration code, which also > should be moved out of src/chrome, so we agreed to move both of them in a > follow-up CL. See crbug.com/676513. Sounds like the existing code for overscan calibrator already has no dependency on chrome. So, can this move happen first before this goes in? Landing this in the wrong place, then doing a move is unnecessary churn that seems easy to avoid in this case. Thinking about this a little bit more: //ui/display/manager may be the perfect place for this.
Apparently there is some time pressure on this feature. In principal I agree that the overscan calibrator should be moved first and this added after, but if we decide to put it in src/ui instead of src/ash (which sounds reasonable to me) it looks like we will need to set up a delegate to WindowTreeHostManager, which will take a bit of time. I would rather let this feature move forward while oshima and the folks working on mus+ash figure out the best way to move this at their leisure. On Thu, Dec 22, 2016 at 10:32 AM, <sadrul@chromium.org> wrote: > On 2016/12/22 18:28:33, stevenjb wrote: > > On 2016/12/22 18:23:00, sadrul wrote: > > > On 2016/12/22 18:10:12, xiyuan wrote: > > > > High level comment: > > > > > > > > Directly access to Shell instance and using pre target handler is > > discouraged > > > in > > > > new chrome code. Ash is moving towards mash where it runs in its own > > process. > > > We > > > > are migrating old code that is doing this and probably should not add > more. > > > > > > > > The code probably should live in ash with the > TouchTransformerController. > > > > > > > > +sadrul for his input. > > > > > > I agree. It doesn't look like this new calibration-controller has any > > dependency > > > on other parts of chrome anyway, so it should be easy to move this > into ash. > > > > > > Ideally, this would be a completely separate module that does not have > ash > > > dependency either (so, in //ui/, for example). That would make it > possible > to > > > easily put this into a separate mojo/mus app, so that it is loaded > only when > > > necessary (see //ash/touch_hud/mus, for example). But even if we don't > do > that > > > right now, we need to at least keep this out of //chrome. > > > > I made the comment earlier that this code belongs in src/ui or src/ash. > It was > > pointed out that it should reside with the touch calibration code, which > also > > should be moved out of src/chrome, so we agreed to move both of them in a > > follow-up CL. See crbug.com/676513. > > Sounds like the existing code for overscan calibrator already has no > dependency > on chrome. So, can this move happen first before this goes in? Landing > this in > the wrong place, then doing a move is unnecessary churn that seems easy to > avoid > in this case. > > Thinking about this a little bit more: //ui/display/manager may be the > perfect > place for this. > > https://codereview.chromium.org/2596553003/ > -- 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.
lgtm Okay. I am fine to get the feature moving if there is a time pressure. Not ideal but... https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:46: widget_->Show(); nit: move Show() after SetcontentsView and SetBounds ? https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h (right): https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:64: State state() { return state_; } nit: move accessor to the last of decl https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:91: } // namespace chromeos nit: insert an empty line before
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc (right): https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:53: bool is_target_display = display.id() == target_display_.id(); On 2016/12/22 at 18:28:41, stevenjb wrote: > nit: In the constructor the parameter is 'is_primary_view', these should be consistent. done https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:126: return; On 2016/12/22 at 18:28:41, stevenjb wrote: > nit: move early return and comment into 'default' case above. done https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:46: widget_->Show(); On 2016/12/22 at 19:23:30, xiyuan wrote: > nit: move Show() after SetcontentsView and SetBounds ? done https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h (right): https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:64: State state() { return state_; } On 2016/12/22 at 19:23:30, xiyuan wrote: > nit: move accessor to the last of decl done https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:91: } // namespace chromeos On 2016/12/22 at 19:23:31, xiyuan wrote: > nit: insert an empty line before done
https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc (right): https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:53: bool is_target_display = display.id() == target_display_.id(); On 2016/12/22 at 18:28:41, stevenjb wrote: > nit: In the constructor the parameter is 'is_primary_view', these should be consistent. done https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_controller.cc:126: return; On 2016/12/22 at 18:28:41, stevenjb wrote: > nit: move early return and comment into 'default' case above. done https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc (right): https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.cc:46: widget_->Show(); On 2016/12/22 at 19:23:30, xiyuan wrote: > nit: move Show() after SetcontentsView and SetBounds ? done https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h (right): https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:64: State state() { return state_; } On 2016/12/22 at 19:23:30, xiyuan wrote: > nit: move accessor to the last of decl done https://codereview.chromium.org/2596553003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/display/touch_calibrator/touch_calibrator_view.h:91: } // namespace chromeos On 2016/12/22 at 19:23:31, xiyuan wrote: > nit: insert an empty line before done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by malaykeshav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2596553003/#ps120001 (title: "Resolved comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482441105892020,
"parent_rev": "4407e78062f5be5d749b9b750f58cabaf58e3b65", "commit_rev":
"b500a7eeea732608cff50c0400f491053d1b910d"}
Message was sent while issue was closed.
Description was changed from ========== Implements Touch calibrator controller and a skeleton for touch calibrator view. Implements TouchCalibratorController class that is responsible for collecting the touch calibration associated data from the user. It uses TouchCalibratorView class to present an interface the user can interact with. Also includes unittests for TouhcCalibratorController. Design Doc=go/cros-touch-calibration BUG=667921 COMPONENT=Touch Calibration, ChromeOS, Unittests ========== to ========== Implements Touch calibrator controller and a skeleton for touch calibrator view. Implements TouchCalibratorController class that is responsible for collecting the touch calibration associated data from the user. It uses TouchCalibratorView class to present an interface the user can interact with. Also includes unittests for TouhcCalibratorController. Design Doc=go/cros-touch-calibration BUG=667921 COMPONENT=Touch Calibration, ChromeOS, Unittests Review-Url: https://codereview.chromium.org/2596553003 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Implements Touch calibrator controller and a skeleton for touch calibrator view. Implements TouchCalibratorController class that is responsible for collecting the touch calibration associated data from the user. It uses TouchCalibratorView class to present an interface the user can interact with. Also includes unittests for TouhcCalibratorController. Design Doc=go/cros-touch-calibration BUG=667921 COMPONENT=Touch Calibration, ChromeOS, Unittests Review-Url: https://codereview.chromium.org/2596553003 ========== to ========== Implements Touch calibrator controller and a skeleton for touch calibrator view. Implements TouchCalibratorController class that is responsible for collecting the touch calibration associated data from the user. It uses TouchCalibratorView class to present an interface the user can interact with. Also includes unittests for TouhcCalibratorController. Design Doc=go/cros-touch-calibration BUG=667921 COMPONENT=Touch Calibration, ChromeOS, Unittests Committed: https://crrev.com/9fe4c53f0a4a1d6a983e34ce0d971740b989d8d1 Cr-Commit-Position: refs/heads/master@{#440502} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9fe4c53f0a4a1d6a983e34ce0d971740b989d8d1 Cr-Commit-Position: refs/heads/master@{#440502}
Message was sent while issue was closed.
Description was changed from ========== Implements Touch calibrator controller and a skeleton for touch calibrator view. Implements TouchCalibratorController class that is responsible for collecting the touch calibration associated data from the user. It uses TouchCalibratorView class to present an interface the user can interact with. Also includes unittests for TouhcCalibratorController. Design Doc=go/cros-touch-calibration BUG=667921 COMPONENT=Touch Calibration, ChromeOS, Unittests Committed: https://crrev.com/9fe4c53f0a4a1d6a983e34ce0d971740b989d8d1 Cr-Commit-Position: refs/heads/master@{#440502} ========== to ========== Implements Touch calibrator controller and a skeleton for touch calibrator view. Implements TouchCalibratorController class that is responsible for collecting the touch calibration associated data from the user. It uses TouchCalibratorView class to present an interface the user can interact with. Also includes unittests for TouhcCalibratorController. Design Doc=go/cros-touch-calibration BUG=667921,634166 COMPONENT=Touch Calibration, ChromeOS, Unittests Committed: https://crrev.com/9fe4c53f0a4a1d6a983e34ce0d971740b989d8d1 Cr-Commit-Position: refs/heads/master@{#440502} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
