Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2118223004:
ash: Refactor touch hud drawing to ash/touch_hud as a separate component. (Closed)
Descriptionash: Refactor touch hud drawing to ash/touch_hud as a separate component.
BUG=588311
TEST=ash_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel
Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25
Committed: https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55
Committed: https://crrev.com/791f5272dac166ce531ffc441ad721c57256c2cf
Cr-Original-Original-Commit-Position: refs/heads/master@{#405296}
Cr-Original-Commit-Position: refs/heads/master@{#405506}
Cr-Commit-Position: refs/heads/master@{#405575}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Use unique_ptr; add descriptions #
Total comments: 4
Patch Set 3 : Move ui/views/controls/touch to ash/touch_hud. #Patch Set 4 : Fix dependency issue. #Patch Set 5 : Fix a comment. #
Total comments: 6
Patch Set 6 : Pass parent_widget in ctor; dependency #
Total comments: 24
Patch Set 7 : Remove extra dependency and includes; DCHECK; etc #
Total comments: 3
Patch Set 8 : Delete MakeUnique<>. #
Total comments: 2
Patch Set 9 : const-ref; try ASH_EXPORT #Patch Set 10 : Add ASH_TOUCH_HUD_EXPORT #Patch Set 11 : Add gyp file #
Total comments: 3
Patch Set 12 : GYP syntax #Patch Set 13 : Add dependency #Patch Set 14 : Add dependency #
Messages
Total messages: 70 (32 generated)
riajiang@chromium.org changed reviewers: + sadrul@chromium.org, sky@chromium.org
https://codereview.chromium.org/2118223004/diff/1/ash/touch/touch_hud_project... File ash/touch/touch_hud_projection.h (right): https://codereview.chromium.org/2118223004/diff/1/ash/touch/touch_hud_project... ash/touch/touch_hud_projection.h:38: views::TouchHudDrawer* touch_hud_drawer_ = nullptr; unique_ptr https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... File ui/views/controls/touch/touch_hud_drawer.cc (right): https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... ui/views/controls/touch/touch_hud_drawer.cc:72: void Remove() { nit: Remove() is rather confusing given this functions ends up deleting this. I recommend calling this Delete or Destroy. https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... File ui/views/controls/touch/touch_hud_drawer.h (right): https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... ui/views/controls/touch/touch_hud_drawer.h:5: #ifndef UI_VIEWS_CONTROLS_TOUCH_TOUCH_HUD_DRAWER_H_ This code is specific to ash, and not something I see more places needing. Move to ash/common/touch. https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... ui/views/controls/touch/touch_hud_drawer.h:25: class VIEWS_EXPORT TouchHudDrawer { Add description. https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... ui/views/controls/touch/touch_hud_drawer.h:33: void HandleTouchEvent(const ui::LocatedEvent* event, Widget* parent_widget); Add description. In general please add comments for classes, member functions and members.
https://codereview.chromium.org/2118223004/diff/1/ash/touch/touch_hud_project... File ash/touch/touch_hud_projection.h (right): https://codereview.chromium.org/2118223004/diff/1/ash/touch/touch_hud_project... ash/touch/touch_hud_projection.h:38: views::TouchHudDrawer* touch_hud_drawer_ = nullptr; On 2016/07/06 16:31:30, sky wrote: > unique_ptr Done. https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... File ui/views/controls/touch/touch_hud_drawer.cc (right): https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... ui/views/controls/touch/touch_hud_drawer.cc:72: void Remove() { On 2016/07/06 16:31:30, sky wrote: > nit: Remove() is rather confusing given this functions ends up deleting this. I > recommend calling this Delete or Destroy. Done. https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... File ui/views/controls/touch/touch_hud_drawer.h (right): https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... ui/views/controls/touch/touch_hud_drawer.h:5: #ifndef UI_VIEWS_CONTROLS_TOUCH_TOUCH_HUD_DRAWER_H_ On 2016/07/06 16:31:30, sky wrote: > This code is specific to ash, and not something I see more places needing. Move > to ash/common/touch. The touch hud app would be under mash/ and needs to use this. Where would be the best place to put it? https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... ui/views/controls/touch/touch_hud_drawer.h:25: class VIEWS_EXPORT TouchHudDrawer { On 2016/07/06 16:31:30, sky wrote: > Add description. Done. https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... ui/views/controls/touch/touch_hud_drawer.h:33: void HandleTouchEvent(const ui::LocatedEvent* event, Widget* parent_widget); On 2016/07/06 16:31:30, sky wrote: > Add description. In general please add comments for classes, member functions > and members. Done.
https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... File ui/views/controls/touch/touch_hud_drawer.h (right): https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... ui/views/controls/touch/touch_hud_drawer.h:5: #ifndef UI_VIEWS_CONTROLS_TOUCH_TOUCH_HUD_DRAWER_H_ On 2016/07/06 20:30:43, riajiang wrote: > On 2016/07/06 16:31:30, sky wrote: > > This code is specific to ash, and not something I see more places needing. > Move > > to ash/common/touch. > > The touch hud app would be under mash/ and needs to use this. Where would be the > best place to put it? Ah, I see now. How about ash/touch_hud, with it's own gn/gyp file that both places use?
https://codereview.chromium.org/2118223004/diff/20001/ui/views/controls/touch... File ui/views/controls/touch/touch_hud_drawer.h (right): https://codereview.chromium.org/2118223004/diff/20001/ui/views/controls/touch... ui/views/controls/touch/touch_hud_drawer.h:27: class VIEWS_EXPORT TouchHudDrawer { Can I also suggest naming this TouchHudRenderer? Drawer makes me think of a place I put my clothes:) https://codereview.chromium.org/2118223004/diff/20001/ui/views/controls/touch... ui/views/controls/touch/touch_hud_drawer.h:29: static std::unique_ptr<TouchHudDrawer> Get(); Why do you need a Get() that uses the constructor when the constructor is public? Also, if you do really need this function it should be after the constructor/destructor (see style guide) and I would name it Create() as that is what it is doing. But again, it seems like callers can just create directly.
On 2016/07/07 00:00:42, sky wrote: > https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... > File ui/views/controls/touch/touch_hud_drawer.h (right): > > https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... > ui/views/controls/touch/touch_hud_drawer.h:5: #ifndef > UI_VIEWS_CONTROLS_TOUCH_TOUCH_HUD_DRAWER_H_ > On 2016/07/06 20:30:43, riajiang wrote: > > On 2016/07/06 16:31:30, sky wrote: > > > This code is specific to ash, and not something I see more places needing. > > Move > > > to ash/common/touch. > > > > The touch hud app would be under mash/ and needs to use this. Where would be > the > > best place to put it? > > Ah, I see now. How about ash/touch_hud, with it's own gn/gyp file that both > places use? Interesting. This sounds like a good idea. Would it make sense to put the app in //ash/touch_hud/mus/ too, instead of in //mash/touch_hud/?
SGTM too. Now that we're putting code under ash we should likely consider moving the remaining parts of mash into ash. On Fri, Jul 8, 2016 at 8:54 AM, <sadrul@chromium.org> wrote: > On 2016/07/07 00:00:42, sky wrote: >> > https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... >> File ui/views/controls/touch/touch_hud_drawer.h (right): >> >> > https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... >> ui/views/controls/touch/touch_hud_drawer.h:5: #ifndef >> UI_VIEWS_CONTROLS_TOUCH_TOUCH_HUD_DRAWER_H_ >> On 2016/07/06 20:30:43, riajiang wrote: >> > On 2016/07/06 16:31:30, sky wrote: >> > > This code is specific to ash, and not something I see more places >> > > needing. >> > Move >> > > to ash/common/touch. >> > >> > The touch hud app would be under mash/ and needs to use this. Where >> > would be >> the >> > best place to put it? >> >> Ah, I see now. How about ash/touch_hud, with it's own gn/gyp file that >> both >> places use? > > Interesting. This sounds like a good idea. Would it make sense to put the > app in > //ash/touch_hud/mus/ too, instead of in //mash/touch_hud/? > > https://codereview.chromium.org/2118223004/ -- 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.
https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... File ui/views/controls/touch/touch_hud_drawer.h (right): https://codereview.chromium.org/2118223004/diff/1/ui/views/controls/touch/tou... ui/views/controls/touch/touch_hud_drawer.h:5: #ifndef UI_VIEWS_CONTROLS_TOUCH_TOUCH_HUD_DRAWER_H_ On 2016/07/07 00:00:42, sky wrote: > On 2016/07/06 20:30:43, riajiang wrote: > > On 2016/07/06 16:31:30, sky wrote: > > > This code is specific to ash, and not something I see more places needing. > > Move > > > to ash/common/touch. > > > > The touch hud app would be under mash/ and needs to use this. Where would be > the > > best place to put it? > > Ah, I see now. How about ash/touch_hud, with it's own gn/gyp file that both > places use? Done. https://codereview.chromium.org/2118223004/diff/20001/ui/views/controls/touch... File ui/views/controls/touch/touch_hud_drawer.h (right): https://codereview.chromium.org/2118223004/diff/20001/ui/views/controls/touch... ui/views/controls/touch/touch_hud_drawer.h:27: class VIEWS_EXPORT TouchHudDrawer { On 2016/07/07 00:04:38, sky wrote: > Can I also suggest naming this TouchHudRenderer? Drawer makes me think of a > place I put my clothes:) Done :) https://codereview.chromium.org/2118223004/diff/20001/ui/views/controls/touch... ui/views/controls/touch/touch_hud_drawer.h:29: static std::unique_ptr<TouchHudDrawer> Get(); On 2016/07/07 00:04:38, sky wrote: > Why do you need a Get() that uses the constructor when the constructor is > public? Also, if you do really need this function it should be after the > constructor/destructor (see style guide) and I would name it Create() as that is > what it is doing. But again, it seems like callers can just create directly. Changed it back. I was trying to convert it to a unique_ptr in Get/Create. Now moved it to when I need to own this as a unique_ptr (touch_hud_projection)
Description was changed from ========== ash: Refactor touch hud drawing to ui/views. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). ==========
riajiang@chromium.org changed reviewers: + jamescook@chromium.org
+jamescook, since sky is OOO
https://codereview.chromium.org/2118223004/diff/80001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2118223004/diff/80001/ash/BUILD.gn#newcode27 ash/BUILD.gn:27: "//ash/touch_hud", This shouldn't need to be a public-dependency. https://codereview.chromium.org/2118223004/diff/80001/ash/touch/touch_hud_pro... File ash/touch/touch_hud_projection.cc (right): https://codereview.chromium.org/2118223004/diff/80001/ash/touch/touch_hud_pro... ash/touch/touch_hud_projection.cc:18: touch_hud_renderer_ = base::WrapUnique(new TouchHudRenderer()); Use base::MakeUnique instead https://codereview.chromium.org/2118223004/diff/80001/ash/touch_hud/touch_hud... File ash/touch_hud/touch_hud_renderer.h (right): https://codereview.chromium.org/2118223004/diff/80001/ash/touch_hud/touch_hud... ash/touch_hud/touch_hud_renderer.h:36: views::Widget* parent_widget); Instead of sending the Widget here, can the Widget be sent along in the ctor instead? Presumably, all the views created by this renderer should be in the same widget?
https://codereview.chromium.org/2118223004/diff/80001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2118223004/diff/80001/ash/BUILD.gn#newcode27 ash/BUILD.gn:27: "//ash/touch_hud", On 2016/07/11 18:41:05, sadrul wrote: > This shouldn't need to be a public-dependency. Done. https://codereview.chromium.org/2118223004/diff/80001/ash/touch/touch_hud_pro... File ash/touch/touch_hud_projection.cc (right): https://codereview.chromium.org/2118223004/diff/80001/ash/touch/touch_hud_pro... ash/touch/touch_hud_projection.cc:18: touch_hud_renderer_ = base::WrapUnique(new TouchHudRenderer()); On 2016/07/11 18:41:05, sadrul wrote: > Use base::MakeUnique instead Done. https://codereview.chromium.org/2118223004/diff/80001/ash/touch_hud/touch_hud... File ash/touch_hud/touch_hud_renderer.h (right): https://codereview.chromium.org/2118223004/diff/80001/ash/touch_hud/touch_hud... ash/touch_hud/touch_hud_renderer.h:36: views::Widget* parent_widget); On 2016/07/11 18:41:05, sadrul wrote: > Instead of sending the Widget here, can the Widget be sent along in the ctor > instead? Presumably, all the views created by this renderer should be in the > same widget? Changed to set parent_widget in the ctor.
Overall approach seems fine. Please update the CL description to have BUG= and TEST= lines. Thanks. https://codereview.chromium.org/2118223004/diff/100001/ash/touch/touch_hud_pr... File ash/touch/touch_hud_projection.cc (right): https://codereview.chromium.org/2118223004/diff/100001/ash/touch/touch_hud_pr... ash/touch/touch_hud_projection.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. nit: don't change copyright date in modified file, only use the current year if it is a new file https://codereview.chromium.org/2118223004/diff/100001/ash/touch/touch_hud_pr... ash/touch/touch_hud_projection.cc:18: touch_hud_renderer_ = base::MakeUnique<TouchHudRenderer>(widget()); nit: can you do this in the initializer list, like this? : TouchObserverHUD(initial_root), touch_hud_renderer_(new TouchHudRenderer(widget()) {} https://codereview.chromium.org/2118223004/diff/100001/ash/touch/touch_hud_pr... File ash/touch/touch_hud_projection.h (right): https://codereview.chromium.org/2118223004/diff/100001/ash/touch/touch_hud_pr... ash/touch/touch_hud_projection.h:36: // TouchHudRenderer draws out the touch points nit: comments end in a period https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/BUILD.gn File ash/touch_hud/BUILD.gn (right): https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/BUILD.gn... ash/touch_hud/BUILD.gn:22: deps += [ "//chromeos" ] Is this needed? I don't see any #includes of code from //chromeos https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... File ash/touch_hud/touch_hud_renderer.cc (right): https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. per earlier comment, either this file is considered new and should be 2016, or the old file should be considered new and should be 2016, but not both (and neither would be OK too). https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.cc:19: const int kPointRadius = 40; Just curious, why is this different? https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.cc:70: void Destroy() { delete this; } Just curious, any reason to have a Destroy() method rather than just calling delete? It doesn't do any cleanup. https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... File ash/touch_hud/touch_hud_renderer.h (right): https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.h:9: #include <memory> Is this used? https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.h:28: TouchHudRenderer(views::Widget* parent_widget); explicit https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.h:43: // a map of touch ids to TouchPointView nit: "A map" and end with period. Also, the int can either be a touch ID or a pointer ID. (sadrul, are these id sequences guaranteed to not overlap? I don't know how that works.) https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.h:47: }; Thanks for documenting the class and all the functions/members. It makes it a lot easier for me to read.
https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... File ash/touch_hud/touch_hud_renderer.cc (right): https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.cc:70: void Destroy() { delete this; } On 2016/07/12 17:22:07, James Cook wrote: > Just curious, any reason to have a Destroy() method rather than just calling > delete? It doesn't do any cleanup. This is probably so we can destroy this thing from below (~line 159 newcode) without changing the ownership. https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.cc:148: id = event->AsPointerEvent()->pointer_id(); Do else, and DCHECK that event->IsPointerEvent(). Also, DCHECK that PointerEvent::pointer_details().pointer_type == POINTER_TYPE_TOUCH https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... File ash/touch_hud/touch_hud_renderer.h (right): https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.h:43: // a map of touch ids to TouchPointView On 2016/07/12 17:22:07, James Cook wrote: > nit: "A map" and end with period. Also, the int can either be a touch ID or a > pointer ID. (sadrul, are these id sequences guaranteed to not overlap? I don't > know how that works.) Pretty much guaranteed to not overlap, yeah.
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests ==========
https://codereview.chromium.org/2118223004/diff/100001/ash/touch/touch_hud_pr... File ash/touch/touch_hud_projection.cc (right): https://codereview.chromium.org/2118223004/diff/100001/ash/touch/touch_hud_pr... ash/touch/touch_hud_projection.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/07/12 17:22:07, James Cook wrote: > nit: don't change copyright date in modified file, only use the current year if > it is a new file Done. https://codereview.chromium.org/2118223004/diff/100001/ash/touch/touch_hud_pr... ash/touch/touch_hud_projection.cc:18: touch_hud_renderer_ = base::MakeUnique<TouchHudRenderer>(widget()); On 2016/07/12 17:22:07, James Cook wrote: > nit: can you do this in the initializer list, like this? > > : TouchObserverHUD(initial_root), > touch_hud_renderer_(new TouchHudRenderer(widget()) {} Done. https://codereview.chromium.org/2118223004/diff/100001/ash/touch/touch_hud_pr... File ash/touch/touch_hud_projection.h (right): https://codereview.chromium.org/2118223004/diff/100001/ash/touch/touch_hud_pr... ash/touch/touch_hud_projection.h:36: // TouchHudRenderer draws out the touch points On 2016/07/12 17:22:07, James Cook wrote: > nit: comments end in a period Done. https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/BUILD.gn File ash/touch_hud/BUILD.gn (right): https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/BUILD.gn... ash/touch_hud/BUILD.gn:22: deps += [ "//chromeos" ] On 2016/07/12 17:22:07, James Cook wrote: > Is this needed? I don't see any #includes of code from //chromeos It's not... Deleted. https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... File ash/touch_hud/touch_hud_renderer.cc (right): https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/07/12 17:22:07, James Cook wrote: > per earlier comment, either this file is considered new and should be 2016, or > the old file should be considered new and should be 2016, but not both (and > neither would be OK too). Changed both files back to 2013. https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.cc:19: const int kPointRadius = 40; On 2016/07/12 17:22:07, James Cook wrote: > Just curious, why is this different? I was testing the touch-hud app on pixel and the old touch point size looks smaller than its actual size in mash mode so I made this change. But I just double checked the new point size in non-mash mode and it's now too big for the current touch-hud we have. I changed it back to 20 for now and will check why it looks small in mash mode in the touch-hud app cl. https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.cc:148: id = event->AsPointerEvent()->pointer_id(); On 2016/07/12 17:48:10, sadrul wrote: > Do else, and DCHECK that event->IsPointerEvent(). Also, DCHECK that > PointerEvent::pointer_details().pointer_type == POINTER_TYPE_TOUCH Done. https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... File ash/touch_hud/touch_hud_renderer.h (right): https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.h:9: #include <memory> On 2016/07/12 17:22:07, James Cook wrote: > Is this used? Deleted. https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.h:28: TouchHudRenderer(views::Widget* parent_widget); On 2016/07/12 17:22:07, James Cook wrote: > explicit Done. https://codereview.chromium.org/2118223004/diff/100001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.h:43: // a map of touch ids to TouchPointView On 2016/07/12 17:22:07, James Cook wrote: > nit: "A map" and end with period. Also, the int can either be a touch ID or a > pointer ID. (sadrul, are these id sequences guaranteed to not overlap? I don't > know how that works.) Done.
LGTM https://codereview.chromium.org/2118223004/diff/120001/ash/touch/touch_hud_pr... File ash/touch/touch_hud_projection.cc (right): https://codereview.chromium.org/2118223004/diff/120001/ash/touch/touch_hud_pr... ash/touch/touch_hud_projection.cc:18: touch_hud_renderer_(base::MakeUnique<TouchHudRenderer>(widget())) {} nit: I don't think you need MakeUnique<> here.
https://codereview.chromium.org/2118223004/diff/120001/ash/touch/touch_hud_pr... File ash/touch/touch_hud_projection.cc (right): https://codereview.chromium.org/2118223004/diff/120001/ash/touch/touch_hud_pr... ash/touch/touch_hud_projection.cc:18: touch_hud_renderer_(base::MakeUnique<TouchHudRenderer>(widget())) {} On 2016/07/12 22:46:02, James Cook wrote: > nit: I don't think you need MakeUnique<> here. Deleted. I thought we have to explicitly convert pointer to unique_ptr
The CQ bit was checked by riajiang@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...
lgtm https://codereview.chromium.org/2118223004/diff/120001/ash/touch/touch_hud_pr... File ash/touch/touch_hud_projection.cc (right): https://codereview.chromium.org/2118223004/diff/120001/ash/touch/touch_hud_pr... ash/touch/touch_hud_projection.cc:18: touch_hud_renderer_(base::MakeUnique<TouchHudRenderer>(widget())) {} On 2016/07/13 00:12:11, riajiang wrote: > On 2016/07/12 22:46:02, James Cook wrote: > > nit: I don't think you need MakeUnique<> here. > > Deleted. I thought we have to explicitly convert pointer to unique_ptr We do, except in the unique_ptr<> ctor, which takes a raw pointer. https://codereview.chromium.org/2118223004/diff/140001/ash/touch_hud/touch_hu... File ash/touch_hud/touch_hud_renderer.h (right): https://codereview.chromium.org/2118223004/diff/140001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.h:34: void HandleTouchEvent(const ui::LocatedEvent* event); Send in a const-ref instead
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Also added ASH_TOUCH_HUD_EXPORT and gyp for ash/touch_hud. PTAL. Thanks https://codereview.chromium.org/2118223004/diff/140001/ash/touch_hud/touch_hu... File ash/touch_hud/touch_hud_renderer.h (right): https://codereview.chromium.org/2118223004/diff/140001/ash/touch_hud/touch_hu... ash/touch_hud/touch_hud_renderer.h:34: void HandleTouchEvent(const ui::LocatedEvent* event); On 2016/07/13 00:31:48, sadrul wrote: > Send in a const-ref instead Done.
still lgtm
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2118223004/#ps200001 (title: "Add gyp file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Cr-Commit-Position: refs/heads/master@{#405296} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Cr-Commit-Position: refs/heads/master@{#405296}
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
reverting this https://codereview.chromium.org/2118223004/diff/200001/ash/ash_touch_hud.gyp File ash/ash_touch_hud.gyp (right): https://codereview.chromium.org/2118223004/diff/200001/ash/ash_touch_hud.gyp#... ash/ash_touch_hud.gyp:26: }, ], whoops
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2145223002/ by dbeam@chromium.org. The reason for reverting is: Invalid GYP, broken Win x64 builder: https://build.chromium.org/p/chromium/builders/Win%20x64/builds/2391.
Message was sent while issue was closed.
https://codereview.chromium.org/2118223004/diff/200001/ash/ash_touch_hud.gyp File ash/ash_touch_hud.gyp (right): https://codereview.chromium.org/2118223004/diff/200001/ash/ash_touch_hud.gyp#... ash/ash_touch_hud.gyp:26: }, On 2016/07/13 22:21:35, Dan Beam wrote: > ], > > whoops Added, sorry. Should I just upload the new patch to this cl?
Message was sent while issue was closed.
https://codereview.chromium.org/2118223004/diff/200001/ash/ash_touch_hud.gyp File ash/ash_touch_hud.gyp (right): https://codereview.chromium.org/2118223004/diff/200001/ash/ash_touch_hud.gyp#... ash/ash_touch_hud.gyp:26: }, On 2016/07/13 22:28:44, riajiang wrote: > On 2016/07/13 22:21:35, Dan Beam wrote: > > ], > > > > whoops > > Added, sorry. Should I just upload the new patch to this cl? yeah, that'd work
Message was sent while issue was closed.
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Cr-Commit-Position: refs/heads/master@{#405296} ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Cr-Commit-Position: refs/heads/master@{#405296} ==========
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2118223004/#ps220001 (title: "GYP syntax")
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 dbeam@chromium.org
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Cr-Commit-Position: refs/heads/master@{#405296} ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg,master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Cr-Commit-Position: refs/heads/master@{#405296} ==========
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg,master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Cr-Commit-Position: refs/heads/master@{#405296} ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Cr-Commit-Position: refs/heads/master@{#405296} ==========
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
from the extra try bots I added: FAILED: ash_touch_hud.dll ash_touch_hud.dll.lib ash_touch_hud.dll.pdb E:\b\depot_tools\python276_bin\python.exe gyp-win-tool link-with-manifests environment.x86 True ash_touch_hud.dll "E:\b\depot_tools\python276_bin\python.exe gyp-win-tool link-wrapper environment.x86 False link.exe /nologo /IMPLIB:ash_touch_hud.dll.lib /DLL /OUT:ash_touch_hud.dll @ash_touch_hud.dll.rsp" 2 mt.exe rc.exe "obj\ash\ash_touch_hud.ash_touch_hud.dll.intermediate.manifest" obj\ash\ash_touch_hud.ash_touch_hud.dll.generated.manifest ash_touch_hud.touch_hud_renderer.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: void __thiscall ui::Layer::SetFillsBoundsOpaquely(bool)" (__imp_?SetFillsBoundsOpaquely@Layer@ui@@QAEX_N@Z) referenced in function "public: __thiscall ash::TouchPointView::TouchPointView(class views::Widget *)" (??0TouchPointView@ash@@QAE@PAVWidget@views@@@Z) ash_touch_hud.touch_hud_renderer.obj : error LNK2019: unresolved external symbol "__declspec(dllimport) public: class ui::Layer * __thiscall ui::LayerOwner::layer(void)" (__imp_?layer@LayerOwner@ui@@QAEPAVLayer@2@XZ) referenced in function "public: __thiscall ash::TouchPointView::TouchPointView(class views::Widget *)" (??0TouchPointView@ash@@QAE@PAVWidget@views@@@Z) ash_touch_hud.dll : fatal error LNK1120: 2 unresolved externals
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_gyp_dbg on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2118223004/#ps240001 (title: "Add dependency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Cr-Commit-Position: refs/heads/master@{#405296} ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Cr-Commit-Position: refs/heads/master@{#405296} ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Cr-Commit-Position: refs/heads/master@{#405296} ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Committed: https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55 Cr-Original-Commit-Position: refs/heads/master@{#405296} Cr-Commit-Position: refs/heads/master@{#405506} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55 Cr-Commit-Position: refs/heads/master@{#405506}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2153493002/ by riajiang@chromium.org. The reason for reverting is: Missing dependency, broke build.
Message was sent while issue was closed.
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Committed: https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55 Cr-Original-Commit-Position: refs/heads/master@{#405296} Cr-Commit-Position: refs/heads/master@{#405506} ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Committed: https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55 Cr-Original-Commit-Position: refs/heads/master@{#405296} Cr-Commit-Position: refs/heads/master@{#405506} ==========
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Committed: https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55 Cr-Original-Commit-Position: refs/heads/master@{#405296} Cr-Commit-Position: refs/heads/master@{#405506} ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud as a separate component. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Committed: https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55 Cr-Original-Commit-Position: refs/heads/master@{#405296} Cr-Commit-Position: refs/heads/master@{#405506} ==========
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2118223004/#ps260001 (title: "Add dependency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud as a separate component. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Committed: https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55 Cr-Original-Commit-Position: refs/heads/master@{#405296} Cr-Commit-Position: refs/heads/master@{#405506} ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud as a separate component. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Committed: https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55 Cr-Original-Commit-Position: refs/heads/master@{#405296} Cr-Commit-Position: refs/heads/master@{#405506} ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud as a separate component. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Committed: https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55 Cr-Original-Commit-Position: refs/heads/master@{#405296} Cr-Commit-Position: refs/heads/master@{#405506} ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud as a separate component. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Committed: https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55 Committed: https://crrev.com/791f5272dac166ce531ffc441ad721c57256c2cf Cr-Original-Original-Commit-Position: refs/heads/master@{#405296} Cr-Original-Commit-Position: refs/heads/master@{#405506} Cr-Commit-Position: refs/heads/master@{#405575} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/791f5272dac166ce531ffc441ad721c57256c2cf Cr-Commit-Position: refs/heads/master@{#405575}
Message was sent while issue was closed.
Description was changed from ========== ash: Refactor touch hud drawing to ash/touch_hud as a separate component. This is part of the touch hud app for mustash (https://codereview.chromium.org/2092343002/). BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Committed: https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55 Committed: https://crrev.com/791f5272dac166ce531ffc441ad721c57256c2cf Cr-Original-Original-Commit-Position: refs/heads/master@{#405296} Cr-Original-Commit-Position: refs/heads/master@{#405506} Cr-Commit-Position: refs/heads/master@{#405575} ========== to ========== ash: Refactor touch hud drawing to ash/touch_hud as a separate component. BUG=588311 TEST=ash_unittests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win8_chromium_gyp_dbg;master.tryserver.chromium.win:win8_chromium_gyp_rel Committed: https://crrev.com/98caac0fdcd637a95eb27e72c9a4c0a487e72d25 Committed: https://crrev.com/fd4123fd2d8efaa85d0b2f1550ca6e70ddc7cb55 Committed: https://crrev.com/791f5272dac166ce531ffc441ad721c57256c2cf Cr-Original-Original-Commit-Position: refs/heads/master@{#405296} Cr-Original-Commit-Position: refs/heads/master@{#405506} Cr-Commit-Position: refs/heads/master@{#405575} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
