Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2776543002:
Create a unified UIElement interface for Widget, View and Window. (Closed)
DescriptionCreate a unified UIElement interface for Widget, View and Window.
The unified interface will enable css_agent to set/access many
properties of widget, view and window without the need of knowing the
actual object type. A UIElement tree where each node can be either
view, widget and window will be kept in sync with tree structures of
window, widget and view objects.
BUG=700024
Review-Url: https://codereview.chromium.org/2776543002
Cr-Original-Commit-Position: refs/heads/master@{#473315}
Committed: https://chromium.googlesource.com/chromium/src/+/bdcdb17a4fa7ffea34afed460b776cc2e5032855
Review-Url: https://codereview.chromium.org/2776543002
Cr-Commit-Position: refs/heads/master@{#473407}
Committed: https://chromium.googlesource.com/chromium/src/+/27d1ff58a6ae0788d6f9d776d3c593d853e919aa
Patch Set 1 : . #
Total comments: 1
Patch Set 2 : rebase #
Total comments: 15
Patch Set 3 : rebase #Patch Set 4 : . #Patch Set 5 : . #
Total comments: 6
Patch Set 6 : . #Patch Set 7 : . #Patch Set 8 : nits #
Total comments: 38
Patch Set 9 : . #
Total comments: 2
Patch Set 10 : . #Patch Set 11 : . #Patch Set 12 : nits #
Total comments: 22
Patch Set 13 : create node_id in ui_element.cc #Patch Set 14 : . #Patch Set 15 : . #Patch Set 16 : . #Patch Set 17 : nits #
Total comments: 48
Patch Set 18 : address comments. #
Total comments: 12
Patch Set 19 : address comments #Patch Set 20 : Remove visibility check for window and view elements. #
Total comments: 22
Patch Set 21 : Address comments. #
Total comments: 1
Patch Set 22 : . #Dependent Patchsets: Messages
Total messages: 204 (173 generated)
Description was changed from ========== WIP: DevTools BUG=700024 ========== to ========== WIP: DevTools BUG=700024 ==========
The CQ bit was checked by thanhph@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2776543002/diff/40001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2776543002/diff/40001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_dom_agent.h:9: #include "ash/common/devtools/ui_element.h" Get rid of this. Include in the cc. Forward declare here.
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:220001) has been deleted
Patchset #3 (id:240001) has been deleted
Patchset #3 (id:260001) has been deleted
Patchset #3 (id:280001) has been deleted
The CQ bit was checked by thanhph@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
Description was changed from ========== WIP: DevTools BUG=700024 ========== to ========== Create a unified UIElement interface for Widget, View and Window. BUG=700024 ==========
thanhph@chromium.org changed reviewers: - fsamuel@chromium.org
The CQ bit was checked by thanhph@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: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by thanhph@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:300001) has been deleted
Patchset #2 (id:320001) has been deleted
Patchset #2 (id:340001) has been deleted
Patchset #2 (id:360001) has been deleted
Patchset #2 (id:380001) has been deleted
The CQ bit was checked by thanhph@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...
Description was changed from ========== Create a unified UIElement interface for Widget, View and Window. BUG=700024 ========== to ========== Create a unified UIElement interface for Widget, View and Window. The unified interface will enable dom_agent to set/access many properties of widget, view and window without the need of knowing the actual object type. A UIElement tree where each node can be either view, widget and window will be kept in sync with tree structures of window, widget and view objects. BUG=700024 ==========
The CQ bit was unchecked by thanhph@chromium.org
The CQ bit was checked by thanhph@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...
Description was changed from ========== Create a unified UIElement interface for Widget, View and Window. The unified interface will enable dom_agent to set/access many properties of widget, view and window without the need of knowing the actual object type. A UIElement tree where each node can be either view, widget and window will be kept in sync with tree structures of window, widget and view objects. BUG=700024 ========== to ========== Create a unified UIElement interface for Widget, View and Window. The unified interface will enable css_agent to set/access many properties of widget, view and window without the need of knowing the actual object type. A UIElement tree where each node can be either view, widget and window will be kept in sync with tree structures of window, widget and view objects. BUG=700024 ==========
Description was changed from ========== Create a unified UIElement interface for Widget, View and Window. The unified interface will enable css_agent to set/access many properties of widget, view and window without the need of knowing the actual object type. A UIElement tree where each node can be either view, widget and window will be kept in sync with tree structures of window, widget and view objects. BUG=700024 ========== to ========== Create a unified UIElement interface for Widget, View and Window. The unified interface will enable css_agent to set/access many properties of widget, view and window without the need of knowing the actual object type. A UIElement tree where each node can be either view, widget and window will be kept in sync with tree structures of window, widget and view objects. BUG=700024 ==========
thanhph@chromium.org changed reviewers: + sadrul@chromium.org
Hi Sadrul, Can you review my CL? Thanks, Thanh.
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 thanhph@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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #2 (id:400001) has been deleted
The CQ bit was checked by thanhph@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.
Can you also merge with the changes in https://codereview.chromium.org/2821213002 (already landed in tree)? Thanks! https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_elemen... File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:14: UIElement::UIElement() {} When a new UIElement is created, can this generate and set the globally unique ID? Instead of the caller setting the id using SetNodeId()? https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:16: UIElement::~UIElement() {} Should this destroy all its children too during destruction? https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_element.h File ash/devtools/ui_element.h (right): https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:27: UIElement(); Move this into a protected ctor. Change the ctor to take the type and delegate as input. https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/view_elem... File ash/devtools/view_element.cc (right): https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/view_elem... ash/devtools/view_element.cc:17: this->SetUIElementDelegate(ui_element_delegate); Remove |this->| from the code. https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/view_elem... ash/devtools/view_element.cc:27: this->SetNodeId(node_id); As far as I can see, none of the code actually uses this ctor to create the ViewElement object. I think that means none of the ViewElement objects have a valid node id set? Same issue for WindowElement and WidgetElement. https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/view_elem... ash/devtools/view_element.cc:74: this->~ViewElement(); 'delete this' https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/window_el... File ash/devtools/window_element.cc (right): https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/window_el... ash/devtools/window_element.cc:38: int GetNodeIdFromWindow(devtools::UIElement* ui_element, WmWindow* window) { It doesn't look like we use this function?
The CQ bit was checked by thanhph@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.
The CQ bit was checked by thanhph@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.
The CQ bit was checked by thanhph@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 thanhph@chromium.org to run a CQ dry run
Patchset #5 (id:480001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Sadrul. Please look at my new patch. https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_elemen... File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:14: UIElement::UIElement() {} On 2017/04/19 18:21:39, sadrul wrote: > When a new UIElement is created, can this generate and set the globally unique > ID? Instead of the caller setting the id using SetNodeId()? No it can't at the moment but this can be addressed in this bug: https://bugs.chromium.org/p/chromium/issues/detail?id=713389 https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:16: UIElement::~UIElement() {} On 2017/04/19 18:21:39, sadrul wrote: > Should this destroy all its children too during destruction? I'm able to move RemoveChild() to UIElement.cc but dom_agent will still call RemoveChild() so that it can receive the return iterator from std::vector<UIElement*>::iterator OnUIElementRemoved(). Calling RemoveChild() directly from ~UIElement() will crash at the moment since the element is already clean up in RemoveChild() and won't be able to return iterator from destructor. https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_element.h File ash/devtools/ui_element.h (right): https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:27: UIElement(); On 2017/04/19 18:21:39, sadrul wrote: > Move this into a protected ctor. > > Change the ctor to take the type and delegate as input. Done. I also added parent as an input. https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/view_elem... File ash/devtools/view_element.cc (right): https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/view_elem... ash/devtools/view_element.cc:17: this->SetUIElementDelegate(ui_element_delegate); On 2017/04/19 18:21:39, sadrul wrote: > Remove |this->| from the code. Done. https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/view_elem... ash/devtools/view_element.cc:27: this->SetNodeId(node_id); On 2017/04/19 18:21:39, sadrul wrote: > As far as I can see, none of the code actually uses this ctor to create the > ViewElement object. I think that means none of the ViewElement objects have a > valid node id set? > > Same issue for WindowElement and WidgetElement. The node id is set in dom_agent for view, window and widget. I showed you shortly after. https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/view_elem... ash/devtools/view_element.cc:74: this->~ViewElement(); On 2017/04/19 18:21:39, sadrul wrote: > 'delete this' Done. https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/window_el... File ash/devtools/window_element.cc (right): https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/window_el... ash/devtools/window_element.cc:38: int GetNodeIdFromWindow(devtools::UIElement* ui_element, WmWindow* window) { On 2017/04/19 18:21:39, sadrul wrote: > It doesn't look like we use this function? I moved it to unittest.
The CQ bit was checked by thanhph@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/2776543002/diff/420001/ash/devtools/ui_elemen... File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:16: UIElement::~UIElement() {} On 2017/04/24 15:56:50, thanhph wrote: > On 2017/04/19 18:21:39, sadrul wrote: > > Should this destroy all its children too during destruction? > > I'm able to move RemoveChild() to UIElement.cc but dom_agent will still call > RemoveChild() so that it can receive the return iterator from > std::vector<UIElement*>::iterator OnUIElementRemoved(). > > Calling RemoveChild() directly from ~UIElement() will crash at the moment since > the element is already clean up in RemoveChild() and won't be able to return > iterator from destructor. Im able to get the destructor to work now. Please review my new patch. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2776543002/diff/520001/ash/devtools/ui_elemen... File ash/devtools/ui_element_delegate.h (right): https://codereview.chromium.org/2776543002/diff/520001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:26: virtual std::vector<UIElement*>::iterator OnUIElementRemoved(int node_id); This is pretty weird. Why not just 'void OnUIElementRemoved(UIElement*)' (or OnUIElementWillRemove)? https://codereview.chromium.org/2776543002/diff/520001/ash/devtools/view_elem... File ash/devtools/view_element.cc (right): https://codereview.chromium.org/2776543002/diff/520001/ash/devtools/view_elem... ash/devtools/view_element.cc:56: new ViewElement(view, GetUIElementDelegate(), GetParent()), This means there's a second ViewElement being created for |view|, right? (since 'this' also is for |view|) We need to avoid that, right? ... although, maybe in the OnUIElementRemoved() call above, |this| is destroyed, and so we will still end up with a single ViewElement corresponding to |view|? But, in that case, the GetUIElementDelegate() etc. calls in this line is causing a use-after-free. https://codereview.chromium.org/2776543002/diff/520001/ash/devtools/view_elem... ash/devtools/view_element.cc:58: } When stacking/ordering changes, instead of 'remove' + 'add', can we just introduce a OnUIElementReordered() or something like that to delegate? That would simplify this quite a lot.
The CQ bit was checked by thanhph@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.
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Patchset #7 (id:540001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:500001) has been deleted
Closing Chrome doesn't crash anymore during normal user case test. Please review my new patch. Thanks! https://codereview.chromium.org/2776543002/diff/520001/ash/devtools/ui_elemen... File ash/devtools/ui_element_delegate.h (right): https://codereview.chromium.org/2776543002/diff/520001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:26: virtual std::vector<UIElement*>::iterator OnUIElementRemoved(int node_id); On 2017/04/24 18:35:21, sadrul wrote: > This is pretty weird. Why not just 'void OnUIElementRemoved(UIElement*)' (or > OnUIElementWillRemove)? Done. I changed it to bool OnUIElementRemoved(UIElement*). The function returns false if the current window is highlighted. https://codereview.chromium.org/2776543002/diff/520001/ash/devtools/view_elem... File ash/devtools/view_element.cc (right): https://codereview.chromium.org/2776543002/diff/520001/ash/devtools/view_elem... ash/devtools/view_element.cc:56: new ViewElement(view, GetUIElementDelegate(), GetParent()), On 2017/04/24 18:35:21, sadrul wrote: > This means there's a second ViewElement being created for |view|, right? (since > 'this' also is for |view|) We need to avoid that, right? > > ... although, maybe in the OnUIElementRemoved() call above, |this| is destroyed, > and so we will still end up with a single ViewElement corresponding to |view|? > But, in that case, the GetUIElementDelegate() etc. calls in this line is causing > a use-after-free. This was the cause. I move Destroy() out of OnUIElementRemoved(). Thanks! https://codereview.chromium.org/2776543002/diff/520001/ash/devtools/view_elem... ash/devtools/view_element.cc:58: } On 2017/04/24 18:35:22, sadrul wrote: > When stacking/ordering changes, instead of 'remove' + 'add', can we just > introduce a OnUIElementReordered() or something like that to delegate? That > would simplify this quite a lot. I just call OnUIElementRemoved() and follow OnUIElementAdded() since these 2 functions are already implemented. The function RemoveChild() keeps track of the prev_sibling_location instead of leaving it for OnUIElementRemoved() like previous patches.
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 thanhph@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.
The CQ bit was checked by thanhph@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...
A lot of the comments from widget_element and window_element would apply to view_element too. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:140: parent_ui_element->GetChildren().push_back(child_ui_element); This should instead be: parent->AddChild(child_ui_element); https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_element.h File ash/devtools/ui_element.h (right): https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:33: UIElementDelegate* GetUIElementDelegate(); UIElementDelegate* delegate() { return ui_element_delegate_; } https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:34: void SetUIElementDelegate(UIElementDelegate* ui_element_delegate); This should no longer be needed? https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:37: void SetType(UIElementType type); This should no longer be needed. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:39: std::vector<UIElement*>& GetChildren(); this should return a const-ref. (i.e. the callers should not be able to mutate the returned list of children) https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:40: void SetChildren(std::vector<UIElement*>& children); SetChildren() seems a bit odd ... Can this be AddChild + RemoveChild instead? https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:50: virtual std::pair<aura::Window*, gfx::Rect> GetNodeWindowAndBounds() = 0; Document what the return value should be (e.g. what should be the coordinate space of gfx::Rect?) https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:66: UIElementType type_; Make this a const, so that it never changes after creation. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... File ash/devtools/ui_element_delegate.h (right): https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:6: #define ASH_COMMON_DEVTOOLS_UI_ELEMENT_DELEGATE_H_ Make sure to update these https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:25: std::vector<UIElement*>::iterator prev_siblings); This should be: virtual void OnUIElementAdded(UIElement* parent, UIElement* child, int index) = 0; or virtual void OnUIElementAdded(UIElement* parent, UIElement* child, UIElement* prev_sibling) = 0; https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:28: virtual void OnUIElementBoundsChanged(int node_id); Instead of getting node-id, can these callbacks get the UIElement* instead? https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/widget_el... File ash/devtools/widget_element.cc (right): https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/widget_el... ash/devtools/widget_element.cc:17: if (!widget_->HasRemovalsObserver(this)) There's no reason for having this check. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/widget_el... ash/devtools/widget_element.cc:22: if (widget_ && widget_->HasRemovalsObserver(this)) You are never resetting |widget_|. So this check should not be needed. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/widget_el... ash/devtools/widget_element.cc:35: GetChildren()[0]->Destroy(); Can you explain this? e.g. why use GetChildren()[0] here? https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/widget_el... ash/devtools/widget_element.cc:42: if (widget == widget_) This should be a DCHECK()? https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/widget_el... ash/devtools/widget_element.cc:51: if (widget_) { Why do you need these null-checks? https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/window_el... File ash/devtools/window_element.cc (right): https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/window_el... ash/devtools/window_element.cc:53: if (window_ == window) { This should be a DCHECK, right? https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/window_el... ash/devtools/window_element.cc:64: prev_sibling_position); Without doing Removed + Added, can we introduce UIElementDelegate::OnUIElementReordered(UIElement* element, int new_index), or something like that? I think that would simplify this. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/window_el... ash/devtools/window_element.cc:72: if (window_ == window) ditto
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Not sure if this is ready for review yet, but I just took a look, and have a comment: https://codereview.chromium.org/2776543002/diff/620001/ash/devtools/ui_element.h File ash/devtools/ui_element.h (right): https://codereview.chromium.org/2776543002/diff/620001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:45: std::vector<UIElement*>::iterator ChildIndexFromParent(); These ... seem unnecessarily complicated. The tree-related API here should be pretty simple. For example: const std::vector<UIElement*>& GetChildren() const; void AddChild(UIElement* child, UIElement* before = nullptr); void RemoveChild(UIElement* child); void ReorderChild(UIElement* child, int new_index); When it's necessary to find the index of a child element, the callers can do: const auto& children = parent->GetChildren(); auto iter = std::find(children.begin(), children.end(), child);
The CQ bit was checked by sadrul@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thanhph@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 thanhph@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 #12 (id:680001) has been deleted
The CQ bit was checked by thanhph@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 #12 (id:700001) has been deleted
The CQ bit was checked by thanhph@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 #10 (id:640001) has been deleted
Thanks Sadrul. I clean up the code based on your comments. Thanh. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:140: parent_ui_element->GetChildren().push_back(child_ui_element); On 2017/05/01 16:22:26, sadrul wrote: > This should instead be: parent->AddChild(child_ui_element); Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_element.h File ash/devtools/ui_element.h (right): https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:33: UIElementDelegate* GetUIElementDelegate(); On 2017/05/01 16:22:27, sadrul wrote: > UIElementDelegate* delegate() { return ui_element_delegate_; } Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:34: void SetUIElementDelegate(UIElementDelegate* ui_element_delegate); On 2017/05/01 16:22:26, sadrul wrote: > This should no longer be needed? Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:37: void SetType(UIElementType type); On 2017/05/01 16:22:27, sadrul wrote: > This should no longer be needed. Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:39: std::vector<UIElement*>& GetChildren(); On 2017/05/01 16:22:27, sadrul wrote: > this should return a const-ref. (i.e. the callers should not be able to mutate > the returned list of children) Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:40: void SetChildren(std::vector<UIElement*>& children); On 2017/05/01 16:22:27, sadrul wrote: > SetChildren() seems a bit odd ... Can this be AddChild + RemoveChild instead? Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:50: virtual std::pair<aura::Window*, gfx::Rect> GetNodeWindowAndBounds() = 0; On 2017/05/01 16:22:26, sadrul wrote: > Document what the return value should be (e.g. what should be the coordinate > space of gfx::Rect?) Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:66: UIElementType type_; On 2017/05/01 16:22:27, sadrul wrote: > Make this a const, so that it never changes after creation. Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... File ash/devtools/ui_element_delegate.h (right): https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:6: #define ASH_COMMON_DEVTOOLS_UI_ELEMENT_DELEGATE_H_ On 2017/05/01 16:22:27, sadrul wrote: > Make sure to update these Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:25: std::vector<UIElement*>::iterator prev_siblings); On 2017/05/01 16:22:27, sadrul wrote: > This should be: > > virtual void OnUIElementAdded(UIElement* parent, > UIElement* child, > int index) = 0; > or > virtual void OnUIElementAdded(UIElement* parent, > UIElement* child, > UIElement* prev_sibling) = 0; Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:28: virtual void OnUIElementBoundsChanged(int node_id); On 2017/05/01 16:22:27, sadrul wrote: > Instead of getting node-id, can these callbacks get the UIElement* instead? Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/widget_el... File ash/devtools/widget_element.cc (right): https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/widget_el... ash/devtools/widget_element.cc:17: if (!widget_->HasRemovalsObserver(this)) On 2017/05/01 16:22:27, sadrul wrote: > There's no reason for having this check. Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/widget_el... ash/devtools/widget_element.cc:22: if (widget_ && widget_->HasRemovalsObserver(this)) On 2017/05/01 16:22:27, sadrul wrote: > You are never resetting |widget_|. So this check should not be needed. Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/widget_el... ash/devtools/widget_element.cc:35: GetChildren()[0]->Destroy(); On 2017/05/01 16:22:27, sadrul wrote: > Can you explain this? e.g. why use GetChildren()[0] here? Widget has only 1 child which is the root view, a.k.a, GetChildren()[0]. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/widget_el... ash/devtools/widget_element.cc:42: if (widget == widget_) On 2017/05/01 16:22:27, sadrul wrote: > This should be a DCHECK()? Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/widget_el... ash/devtools/widget_element.cc:51: if (widget_) { On 2017/05/01 16:22:27, sadrul wrote: > Why do you need these null-checks? I removed them. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/window_el... File ash/devtools/window_element.cc (right): https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/window_el... ash/devtools/window_element.cc:53: if (window_ == window) { On 2017/05/01 16:22:27, sadrul wrote: > This should be a DCHECK, right? Done. Only the changed window is called back here. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/window_el... ash/devtools/window_element.cc:64: prev_sibling_position); On 2017/05/01 16:22:27, sadrul wrote: > Without doing Removed + Added, can we introduce > UIElementDelegate::OnUIElementReordered(UIElement* element, int new_index), or > something like that? I think that would simplify this. Done. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/window_el... ash/devtools/window_element.cc:72: if (window_ == window) On 2017/05/01 16:22:27, sadrul wrote: > ditto Done. https://codereview.chromium.org/2776543002/diff/620001/ash/devtools/ui_element.h File ash/devtools/ui_element.h (right): https://codereview.chromium.org/2776543002/diff/620001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:45: std::vector<UIElement*>::iterator ChildIndexFromParent(); On 2017/05/03 03:02:06, sadrul wrote: > These ... seem unnecessarily complicated. > > The tree-related API here should be pretty simple. For example: > > const std::vector<UIElement*>& GetChildren() const; > void AddChild(UIElement* child, UIElement* before = nullptr); > void RemoveChild(UIElement* child); > void ReorderChild(UIElement* child, int new_index); > > When it's necessary to find the index of a child element, the callers can do: > > const auto& children = parent->GetChildren(); > auto iter = std::find(children.begin(), children.end(), child); Removed, thanks!
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 thanhph@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.
I looked at UIElement and DOMAgent mostly, since I think we need to clean up the interface first. https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:143: if (parent->GetChildren().empty()) Use {} https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:148: } Why do you need to AddChild() here at all? You are getting this notification from UIElement that the child has already been added. Why add it again? https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:150: BuildTreeForUIElement(child)); Do you need |prev_sibling| as the input to find prev_node_id? You can get that just from |parent| and |child|, right? For example: const auto& children = parent->GetChildren(); const auto iter = std::find(children.rbegin(), children.rend(), child); if (iter != children.rend()) ++iter; int prev_node_id = (iter == children.rend()) ? 0 : *iter->GetNodeId(); https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:155: UIElement* prev_sibling) { Same comment about not needing |prev_sibling| https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:339: node_id_to_ui_element_[ui_element->GetNodeId()] = 0; Use erase https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:340: for (auto* child_element : ui_element->GetChildren()) Add {} https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:396: if (node_id_to_ui_element_.count(node_id)) Add {} https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:57: children_.push_back(child); {} here too https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:73: return nullptr; I think this can be simpler: new_index = std::min(children_.size(), new_index); auto iter = children_.begin() + new_index; children_.insert(iter, child); return iter == children_.begin() ? nullptr : *std::prev(iter); Although, see my comments in dom-agent. I don't think you actually need to return anything from this function. https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_element.h File ash/devtools/ui_element.h (right): https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:29: UIElement* GetParent() const; Since these are simple accessors, these should use unix_style_names() (and inlined). For example: int node_id() const { return node_id_; } UIElementDelegate* delegate() { return delegate_; } UIElementType type() const { return type_; } std::string GetTypeName() const; UIElement* parent() { return parent_; } const std::vector<UIElement*>& children() const { return children_; } https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:35: void AddChild(UIElement* child, UIElement* before = nullptr); Document that |child| is inserted in front of |before|. If |before| is null, it is inserted at the end.
The CQ bit was checked by thanhph@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thanhph@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/05/05 20:19:34, sadrul wrote: > I looked at UIElement and DOMAgent mostly, since I think we need to clean up the > interface first. > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > File ash/devtools/ash_devtools_dom_agent.cc (right): > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:143: if (parent->GetChildren().empty()) > Use {} > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:148: } > Why do you need to AddChild() here at all? You are getting this notification > from UIElement that the child has already been added. Why add it again? > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:150: BuildTreeForUIElement(child)); > Do you need |prev_sibling| as the input to find prev_node_id? You can get that > just from |parent| and |child|, right? For example: > > const auto& children = parent->GetChildren(); > const auto iter = std::find(children.rbegin(), children.rend(), child); > if (iter != children.rend()) > ++iter; > int prev_node_id = (iter == children.rend()) ? 0 : *iter->GetNodeId(); > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:155: UIElement* prev_sibling) { > Same comment about not needing |prev_sibling| > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:339: > node_id_to_ui_element_[ui_element->GetNodeId()] = 0; > Use erase > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:340: for (auto* child_element : > ui_element->GetChildren()) > Add {} > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:396: if > (node_id_to_ui_element_.count(node_id)) > Add {} > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... > File ash/devtools/ui_element.cc (right): > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... > ash/devtools/ui_element.cc:57: children_.push_back(child); > {} here too > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... > ash/devtools/ui_element.cc:73: return nullptr; > I think this can be simpler: > > new_index = std::min(children_.size(), new_index); > auto iter = children_.begin() + new_index; > children_.insert(iter, child); > return iter == children_.begin() ? nullptr : *std::prev(iter); > > Although, see my comments in dom-agent. I don't think you actually need to > return anything from this function. > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_element.h > File ash/devtools/ui_element.h (right): > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... > ash/devtools/ui_element.h:29: UIElement* GetParent() const; > Since these are simple accessors, these should use unix_style_names() (and > inlined). For example: > > int node_id() const { return node_id_; } > UIElementDelegate* delegate() { return delegate_; } > UIElementType type() const { return type_; } > > std::string GetTypeName() const; > > UIElement* parent() { return parent_; } > const std::vector<UIElement*>& children() const { return children_; } > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... > ash/devtools/ui_element.h:35: void AddChild(UIElement* child, UIElement* before > = nullptr); > Document that |child| is inserted in front of |before|. If |before| is null, it > is inserted at the end.
On 2017/05/05 20:19:34, sadrul wrote: > I looked at UIElement and DOMAgent mostly, since I think we need to clean up the > interface first. > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > File ash/devtools/ash_devtools_dom_agent.cc (right): > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:143: if (parent->GetChildren().empty()) > Use {} > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:148: } > Why do you need to AddChild() here at all? You are getting this notification > from UIElement that the child has already been added. Why add it again? > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:150: BuildTreeForUIElement(child)); > Do you need |prev_sibling| as the input to find prev_node_id? You can get that > just from |parent| and |child|, right? For example: > > const auto& children = parent->GetChildren(); > const auto iter = std::find(children.rbegin(), children.rend(), child); > if (iter != children.rend()) > ++iter; > int prev_node_id = (iter == children.rend()) ? 0 : *iter->GetNodeId(); > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:155: UIElement* prev_sibling) { > Same comment about not needing |prev_sibling| > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:339: > node_id_to_ui_element_[ui_element->GetNodeId()] = 0; > Use erase > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:340: for (auto* child_element : > ui_element->GetChildren()) > Add {} > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... > ash/devtools/ash_devtools_dom_agent.cc:396: if > (node_id_to_ui_element_.count(node_id)) > Add {} > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... > File ash/devtools/ui_element.cc (right): > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... > ash/devtools/ui_element.cc:57: children_.push_back(child); > {} here too > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... > ash/devtools/ui_element.cc:73: return nullptr; > I think this can be simpler: > > new_index = std::min(children_.size(), new_index); > auto iter = children_.begin() + new_index; > children_.insert(iter, child); > return iter == children_.begin() ? nullptr : *std::prev(iter); > > Although, see my comments in dom-agent. I don't think you actually need to > return anything from this function. > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_element.h > File ash/devtools/ui_element.h (right): > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... > ash/devtools/ui_element.h:29: UIElement* GetParent() const; > Since these are simple accessors, these should use unix_style_names() (and > inlined). For example: > > int node_id() const { return node_id_; } > UIElementDelegate* delegate() { return delegate_; } > UIElementType type() const { return type_; } > > std::string GetTypeName() const; > > UIElement* parent() { return parent_; } > const std::vector<UIElement*>& children() const { return children_; } > > https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... > ash/devtools/ui_element.h:35: void AddChild(UIElement* child, UIElement* before > = nullptr); > Document that |child| is inserted in front of |before|. If |before| is null, it > is inserted at the end.
The CQ bit was checked by thanhph@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 thanhph@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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:143: if (parent->GetChildren().empty()) On 2017/05/05 20:19:33, sadrul wrote: > Use {} I moved this part to ui_element.cc https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:148: } On 2017/05/05 20:19:33, sadrul wrote: > Why do you need to AddChild() here at all? You are getting this notification > from UIElement that the child has already been added. Why add it again? Child wasn't added till here as I explained. I moved this part to ui_element.cc as we discussed though. https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:150: BuildTreeForUIElement(child)); On 2017/05/05 20:19:33, sadrul wrote: > Do you need |prev_sibling| as the input to find prev_node_id? You can get that > just from |parent| and |child|, right? For example: > > const auto& children = parent->GetChildren(); > const auto iter = std::find(children.rbegin(), children.rend(), child); > if (iter != children.rend()) > ++iter; > int prev_node_id = (iter == children.rend()) ? 0 : *iter->GetNodeId(); Done, I alter the code a bit. Thanks! https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:155: UIElement* prev_sibling) { On 2017/05/05 20:19:33, sadrul wrote: > Same comment about not needing |prev_sibling| Done. https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:339: node_id_to_ui_element_[ui_element->GetNodeId()] = 0; On 2017/05/05 20:19:33, sadrul wrote: > Use erase Removed this part since this only affects DOM, not ui_element. https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:340: for (auto* child_element : ui_element->GetChildren()) On 2017/05/05 20:19:33, sadrul wrote: > Add {} Done. https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:396: if (node_id_to_ui_element_.count(node_id)) On 2017/05/05 20:19:33, sadrul wrote: > Add {} Done. https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:57: children_.push_back(child); On 2017/05/05 20:19:33, sadrul wrote: > {} here too Done. https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:73: return nullptr; On 2017/05/05 20:19:33, sadrul wrote: > I think this can be simpler: > > new_index = std::min(children_.size(), new_index); > auto iter = children_.begin() + new_index; > children_.insert(iter, child); > return iter == children_.begin() ? nullptr : *std::prev(iter); > > Although, see my comments in dom-agent. I don't think you actually need to > return anything from this function. That's correct. I removed the return part. https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_element.h File ash/devtools/ui_element.h (right): https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:29: UIElement* GetParent() const; On 2017/05/05 20:19:34, sadrul wrote: > Since these are simple accessors, these should use unix_style_names() (and > inlined). For example: > > int node_id() const { return node_id_; } > UIElementDelegate* delegate() { return delegate_; } > UIElementType type() const { return type_; } > > std::string GetTypeName() const; > > UIElement* parent() { return parent_; } > const std::vector<UIElement*>& children() const { return children_; } Done, thanks! https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:35: void AddChild(UIElement* child, UIElement* before = nullptr); On 2017/05/05 20:19:33, sadrul wrote: > Document that |child| is inserted in front of |before|. If |before| is null, it > is inserted at the end. Done.
Patchset #17 (id:840001) has been deleted
The CQ bit was checked by thanhph@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 #17 (id:860001) has been deleted
The CQ bit was checked by thanhph@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.
https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.h:34: public AshDevToolsDOMAgentObserver { DOMAgent itself does not need to be its own observer. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:47: auto iter = std::find(children_.begin(), children_.end(), before); Can you add a DCHECK(iter != children_.end()) here? https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:59: } You should call delegate_->OnUIElementRemoved(child); from here. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:64: children_.insert(iter, child); delegate_->OnUIElementReordered(child, new_index) here https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_element.h File ash/devtools/ui_element.h (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:27: void SetNodeId(int node_id); SetNodeId() should no longer be needed, right? https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:69: int node_id_; const int node_id_; https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... File ash/devtools/ui_element_delegate.h (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:22: ~UIElementDelegate(){}; Make the dtor virtual You may need to move the bodies into the .cc file. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:25: virtual void OnUIElementAdded(UIElement* parent, UIElement* child); These should be pure virtuals (i.e. = 0;) https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:34: virtual void RemoveNodeIdMap(UIElement* ui_element); AddNodeIdMap() And RemoveNodeIdMap() should not be in this interface. UIElementDelegate should only have the methods necessary for a UIElement to talk back to the DOMAgent. These methods related to the id-map are internal implementation details of the DOMAgent, and so does not need to be in the UIElementDelegate interface. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:43: virtual bool IsHighlightingWindow(aura::Window* window); This feels a little bit weird here. We may be able to move it out of this in the future, but for now, this is probably OK. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... File ash/devtools/view_element.cc (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... ash/devtools/view_element.cc:14: int FindSibling(views::View* view) { Call this FindIndexInParent() or something like that. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... ash/devtools/view_element.cc:23: DCHECK_GE(view_index, 0); Use View::GetIndexOf() instead. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... ash/devtools/view_element.cc:49: } You can avoid the for-loop here: auto iter = std::find_if(children().begin(), children().end(), [view](UIElement* child) { return view == UIElement::GetBackingElement<...>(child); }); DCHECK(iter != children().end()); UIElement* child_element = *iter; if (!delegate()->OnUIElementRemoved(child_element)) return; RemoveChild(child_element); child_element->Destroy(); https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... ash/devtools/view_element.cc:53: DCHECK(parent == view_); DCHECK_EQ (here and elsewhere) https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... ash/devtools/view_element.cc:59: DCHECK(view == view_); This is wrong, right? Because according to the comment (and code) [1], |view_| would be |parent| (i.e. the parent is the observed view). The following code, therefore, is also incorrect. [1] https://cs.chromium.org/chromium/src/ui/views/view_observer.h?type=cs&sq=pack... https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... ash/devtools/view_element.cc:66: DCHECK(view_ == view); Use DCHECK_EQ https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/widget_el... File ash/devtools/widget_element.cc (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/widget_el... ash/devtools/widget_element.cc:26: delegate()->OnUIElementRemoved(children()[0])) { It'd be good to verify the assumption on children() here. For example: if (view != widget->GetRootView()) return; DCHECK_EQ(1u, children().size()); UIElement* child = children()[0]; delegate()->OnUIElementRemoved(child); RemoveChild(child); child->Destroy(); https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/widget_el... ash/devtools/widget_element.cc:34: DCHECK(widget == widget_); DCHECK_EQ https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/widget_el... ash/devtools/widget_element.cc:55: if (visible != widget_->IsVisible()) { early return. i.e.: if (visible == widget_->IsVisible()) return; if (visible) ... https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/window_el... File ash/devtools/window_element.cc (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/window_el... ash/devtools/window_element.cc:14: int FindSibling(aura::Window* window) { Call this: int GetIndexOfChildInParent(aura::Window* window) https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/window_el... ash/devtools/window_element.cc:28: if (window_) |window_| should be non-null. You should not need to null-check here. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/window_el... ash/devtools/window_element.cc:60: this->parent()->RemoveChild(this); Don't need |this->| Why do you need to RemoveChild() here? Just calling ReorderChild() should work? https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/window_el... ash/devtools/window_element.cc:62: delegate()->OnUIElementReordered(this->parent(), this); UIElement::ReorderChild() should call OnUIElementReordered https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/window_el... ash/devtools/window_element.cc:89: if (visible != window_->IsVisible()) { early return
The CQ bit was checked by thanhph@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 #18 (id:900001) has been deleted
The CQ bit was checked by thanhph@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...
Thanks Sadrul. I address your comments below. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.h:34: public AshDevToolsDOMAgentObserver { On 2017/05/09 04:58:06, sadrul wrote: > DOMAgent itself does not need to be its own observer. Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:47: auto iter = std::find(children_.begin(), children_.end(), before); On 2017/05/09 04:58:06, sadrul wrote: > Can you add a DCHECK(iter != children_.end()) here? Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:59: } On 2017/05/09 04:58:06, sadrul wrote: > You should call delegate_->OnUIElementRemoved(child); from here. Done. To get this to work, I need UIElement::RemoveChild() to return false or true to destroy element from derived classes WindowElement and ViewElement. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:64: children_.insert(iter, child); On 2017/05/09 04:58:06, sadrul wrote: > delegate_->OnUIElementReordered(child, new_index) here Done, this interfaces instead takes delegate()->OnUIElementReordered(child->parent(), child); Removing |child| from |children_| happens before moving |child| to other index in |children_|. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_element.h File ash/devtools/ui_element.h (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:27: void SetNodeId(int node_id); On 2017/05/09 04:58:06, sadrul wrote: > SetNodeId() should no longer be needed, right? Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element.h:69: int node_id_; On 2017/05/09 04:58:06, sadrul wrote: > const int node_id_; Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... File ash/devtools/ui_element_delegate.h (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:22: ~UIElementDelegate(){}; On 2017/05/09 04:58:07, sadrul wrote: > Make the dtor virtual > > You may need to move the bodies into the .cc file. Added. The dtor code runs ok here. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:25: virtual void OnUIElementAdded(UIElement* parent, UIElement* child); On 2017/05/09 04:58:07, sadrul wrote: > These should be pure virtuals (i.e. = 0;) Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:34: virtual void RemoveNodeIdMap(UIElement* ui_element); On 2017/05/09 04:58:07, sadrul wrote: > AddNodeIdMap() And RemoveNodeIdMap() should not be in this interface. > UIElementDelegate should only have the methods necessary for a UIElement to talk > back to the DOMAgent. These methods related to the id-map are internal > implementation details of the DOMAgent, and so does not need to be in the > UIElementDelegate interface. Done. When ui_element is being constructed, the constructor will call delegate()->OnUIElementAdded(0, this) which will initialize dom map. void AshDevToolsDOMAgent::OnUIElementAdded(UIElement* parent, UIElement* child) { if (!parent) { node_id_to_ui_element_[child->node_id()] = child; return; } .... } Similarly, OnUIElementRemoved() will erase the map based on node_id. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ui_elemen... ash/devtools/ui_element_delegate.h:43: virtual bool IsHighlightingWindow(aura::Window* window); On 2017/05/09 04:58:07, sadrul wrote: > This feels a little bit weird here. We may be able to move it out of this in the > future, but for now, this is probably OK. Acknowledged. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... File ash/devtools/view_element.cc (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... ash/devtools/view_element.cc:14: int FindSibling(views::View* view) { On 2017/05/09 04:58:07, sadrul wrote: > Call this FindIndexInParent() or something like that. Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... ash/devtools/view_element.cc:23: DCHECK_GE(view_index, 0); On 2017/05/09 04:58:07, sadrul wrote: > Use View::GetIndexOf() instead. cool, I removed FindSibling(). https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... ash/devtools/view_element.cc:49: } On 2017/05/09 04:58:07, sadrul wrote: > You can avoid the for-loop here: > > auto iter = std::find_if(children().begin(), children().end(), > [view](UIElement* child) { > return view == UIElement::GetBackingElement<...>(child); > }); > DCHECK(iter != children().end()); > UIElement* child_element = *iter; > if (!delegate()->OnUIElementRemoved(child_element)) > return; > RemoveChild(child_element); > child_element->Destroy(); Nice, done! https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... ash/devtools/view_element.cc:53: DCHECK(parent == view_); On 2017/05/09 04:58:07, sadrul wrote: > DCHECK_EQ (here and elsewhere) Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... ash/devtools/view_element.cc:59: DCHECK(view == view_); On 2017/05/09 04:58:07, sadrul wrote: > This is wrong, right? Because according to the comment (and code) [1], |view_| > would be |parent| (i.e. the parent is the observed view). The following code, > therefore, is also incorrect. > > [1] > https://cs.chromium.org/chromium/src/ui/views/view_observer.h?type=cs&sq=pack... Good catch, unittests havent covered this so I added a case in AshDevtoolsTest.ViewRearranged which hits here. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/view_elem... ash/devtools/view_element.cc:66: DCHECK(view_ == view); On 2017/05/09 04:58:07, sadrul wrote: > Use DCHECK_EQ Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/widget_el... File ash/devtools/widget_element.cc (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/widget_el... ash/devtools/widget_element.cc:26: delegate()->OnUIElementRemoved(children()[0])) { On 2017/05/09 04:58:07, sadrul wrote: > It'd be good to verify the assumption on children() here. For example: > if (view != widget->GetRootView()) > return; > DCHECK_EQ(1u, children().size()); > UIElement* child = children()[0]; > delegate()->OnUIElementRemoved(child); > RemoveChild(child); > child->Destroy(); Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/widget_el... ash/devtools/widget_element.cc:34: DCHECK(widget == widget_); On 2017/05/09 04:58:07, sadrul wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/widget_el... ash/devtools/widget_element.cc:55: if (visible != widget_->IsVisible()) { On 2017/05/09 04:58:07, sadrul wrote: > early return. i.e.: > > if (visible == widget_->IsVisible()) > return; > if (visible) > ... Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/window_el... File ash/devtools/window_element.cc (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/window_el... ash/devtools/window_element.cc:14: int FindSibling(aura::Window* window) { On 2017/05/09 04:58:07, sadrul wrote: > Call this: int GetIndexOfChildInParent(aura::Window* window) Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/window_el... ash/devtools/window_element.cc:28: if (window_) On 2017/05/09 04:58:07, sadrul wrote: > |window_| should be non-null. You should not need to null-check here. For root ui_element tree, I assume associated window of the root is null so I can't remove this, i.e., in AshDevToolsDOMAgent::BuildInitialTree(), I have this line window_element_root_ = new WindowElement(nullptr, this, nullptr); https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/window_el... ash/devtools/window_element.cc:60: this->parent()->RemoveChild(this); On 2017/05/09 04:58:07, sadrul wrote: > Don't need |this->| > > Why do you need to RemoveChild() here? Just calling ReorderChild() should work? Done, I put this line in ReorderChild. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/window_el... ash/devtools/window_element.cc:62: delegate()->OnUIElementReordered(this->parent(), this); On 2017/05/09 04:58:07, sadrul wrote: > UIElement::ReorderChild() should call OnUIElementReordered Done. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/window_el... ash/devtools/window_element.cc:89: if (visible != window_->IsVisible()) { On 2017/05/09 04:58:07, sadrul wrote: > early return Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Looking really good! Some pretty small changes. https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:212: window_element_root_ = new WindowElement(nullptr, this, nullptr); Can you add a TODO here that this should not be a WindowElement (we should create a different kind of UIElement to be the root of the entire system) https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ui_elemen... File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:30: default: Remove default: https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:53: if (delegate()->OnUIElementRemoved(child)) { You don't need to check the return value of OnUIElementRemoved(). You can just look for the child in |children_|, and if it's not there, you wouldn't just do anything, right? https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:84: if (child) You should not need this null check. https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/window_el... File ash/devtools/window_element.cc (right): https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/window_el... ash/devtools/window_element.cc:48: AddChild(new WindowElement(params.target, delegate(), this), Can you check if |params.target| is the highlight window? i.e.: if (window_ == params.new_parent && window_ == params.receiver) { if (delegate()->IsHighlightingWindow(params.target)) return; AddChild(....); } This way, we make sure that we do not create any UIElement for the highlight window at all. That should simplify OnUIElementRemoved() and OnUIElementReordered(), where you would not need to check for IsHighlightingWindow(). https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/window_el... ash/devtools/window_element.cc:57: this->parent()->ReorderChild(this, GetIndexOfChildInParent(window)); Don't need this->
The CQ bit was checked by thanhph@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 #19 (id:940001) has been deleted
The CQ bit was checked by thanhph@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 thanhph@chromium.org to run a CQ dry run
Patchset #19 (id:960001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Sadrul. I clean up a few more things based on your feedback. https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:212: window_element_root_ = new WindowElement(nullptr, this, nullptr); On 2017/05/10 03:03:37, sadrul wrote: > Can you add a TODO here that this should not be a WindowElement (we should > create a different kind of UIElement to be the root of the entire system) Done. https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ui_elemen... File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:30: default: On 2017/05/10 03:03:38, sadrul wrote: > Remove default: Done. https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:53: if (delegate()->OnUIElementRemoved(child)) { On 2017/05/10 03:03:38, sadrul wrote: > You don't need to check the return value of OnUIElementRemoved(). You can just > look for the child in |children_|, and if it's not there, you wouldn't just do > anything, right? Done in a slightly different way. I can't access the |children_| of dom tree here but I can reuse the IsHighlightedWindow interface to check if window is highlighted. https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ui_elemen... ash/devtools/ui_element.cc:84: if (child) On 2017/05/10 03:03:37, sadrul wrote: > You should not need this null check. Done. https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/window_el... File ash/devtools/window_element.cc (right): https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/window_el... ash/devtools/window_element.cc:48: AddChild(new WindowElement(params.target, delegate(), this), On 2017/05/10 03:03:38, sadrul wrote: > Can you check if |params.target| is the highlight window? i.e.: > > if (window_ == params.new_parent && window_ == params.receiver) { > if (delegate()->IsHighlightingWindow(params.target)) > return; > AddChild(....); > } > > This way, we make sure that we do not create any UIElement for the highlight > window at all. That should simplify OnUIElementRemoved() and > OnUIElementReordered(), where you would not need to check for > IsHighlightingWindow(). Nice. I'm also able to turn bool UIElement::RemoveChild() to void function. https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/window_el... ash/devtools/window_element.cc:57: this->parent()->ReorderChild(this, GetIndexOfChildInParent(window)); On 2017/05/10 03:03:38, sadrul wrote: > Don't need this-> Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by thanhph@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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Last set of nits. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devt... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devt... ash/devtools/ash_devtools_dom_agent.cc:49: std::unique_ptr<Array<std::string>> attributes = Array<std::string>::create(); Leave a TODO here that this should move into UIElement, i.e. we would introduce a UIElement::GetAttributes(), which the various implementations override to do the right thing. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devt... ash/devtools/ash_devtools_dom_agent.cc:163: (iter == children.end() - 1) ? 0 : (*std::next(iter))->node_id(); You are doing next() for prev_node_id, you should use std::prev(), right? auto iter = ...; DCHECK(iter != children.end()); int prev_node_id = iter == children.begin() ? 0 : (*std::prev(iter))->node_id(); https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devt... ash/devtools/ash_devtools_dom_agent.cc:374: : std::make_pair<aura::Window*, gfx::Rect>(nullptr, gfx::Rect()); You can probably just do std::make_pair<aura::Window*, gfx::Rect>() https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devt... File ash/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devt... ash/devtools/ash_devtools_dom_agent.h:27: virtual void OnNodeBoundsChanged(int node_id) = 0; In a follow up CL, the input to this function should just be a UIElement*, so that the CSS agent does not need to use GetElementFromNodeId() to get the node. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... ash/devtools/ui_element.cc:47: if (iter != children_.end()) Can this be a DCHECK()? i.e. DCHECK(iter != children_.end()) https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... File ash/devtools/ui_element.h (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... ash/devtools/ui_element.h:34: // is inserted at the end. Add a comment that the parent takes ownership of the child added. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... ash/devtools/ui_element.h:37: // Remove |child| out of vector |children_|. Make a note that |child| is not destroyed when removed using RemoveChild(), and the caller is responsible for destroying |child|. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... ash/devtools/ui_element.h:43: virtual void Destroy() = 0; All the overrides just do 'delete this'. Can the callers just do 'delete element' directly instead? https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... File ash/devtools/ui_element_delegate.h (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... ash/devtools/ui_element_delegate.h:24: // |parent| adds |child| in DOM tree. You can remove these comments. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/window_e... File ash/devtools/window_element.cc (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/window_e... ash/devtools/window_element.cc:40: if (delegate()->IsHighlightingWindow(window_)) This would never be true, right? Because we never create a WindowElement for the highlight-window. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/window_e... ash/devtools/window_element.cc:61: if (delegate()->IsHighlightingWindow(window)) You should not need this check either. See above.
The CQ bit was checked by thanhph@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...
Thanks Sadrul. Please review my new patch. Thanh. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devt... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devt... ash/devtools/ash_devtools_dom_agent.cc:49: std::unique_ptr<Array<std::string>> attributes = Array<std::string>::create(); On 2017/05/17 21:54:13, sadrul wrote: > Leave a TODO here that this should move into UIElement, i.e. we would introduce > a UIElement::GetAttributes(), which the various implementations override to do > the right thing. Done. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devt... ash/devtools/ash_devtools_dom_agent.cc:163: (iter == children.end() - 1) ? 0 : (*std::next(iter))->node_id(); On 2017/05/17 21:54:13, sadrul wrote: > You are doing next() for prev_node_id, you should use std::prev(), right? > > auto iter = ...; > DCHECK(iter != children.end()); > int prev_node_id = iter == children.begin() ? 0 : > (*std::prev(iter))->node_id(); This should be std::next() because this UIElement is already inserted in its parent's children and std::next() of this UIElement refers to a UIElement, says, with node id N. We want to insert a new DOM child before a corresponding DOM node id N in DOM parent's children. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devt... ash/devtools/ash_devtools_dom_agent.cc:374: : std::make_pair<aura::Window*, gfx::Rect>(nullptr, gfx::Rect()); On 2017/05/17 21:54:13, sadrul wrote: > You can probably just do std::make_pair<aura::Window*, gfx::Rect>() I think the std::make_pair template hasn't got implemented since I got "error: no matching function for call to 'make_pair'... candidate function template not viable: requires 2 arguments, but 0 were provided". Leaving the code as it is seems ok. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devt... File ash/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devt... ash/devtools/ash_devtools_dom_agent.h:27: virtual void OnNodeBoundsChanged(int node_id) = 0; On 2017/05/17 21:54:13, sadrul wrote: > In a follow up CL, the input to this function should just be a UIElement*, so > that the CSS agent does not need to use GetElementFromNodeId() to get the node. Done. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... ash/devtools/ui_element.cc:47: if (iter != children_.end()) On 2017/05/17 21:54:13, sadrul wrote: > Can this be a DCHECK()? i.e. DCHECK(iter != children_.end()) Done. Also changed to use DCHECK in ReOrder. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... File ash/devtools/ui_element.h (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... ash/devtools/ui_element.h:34: // is inserted at the end. On 2017/05/17 21:54:13, sadrul wrote: > Add a comment that the parent takes ownership of the child added. Done. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... ash/devtools/ui_element.h:37: // Remove |child| out of vector |children_|. On 2017/05/17 21:54:13, sadrul wrote: > Make a note that |child| is not destroyed when removed using RemoveChild(), and > the caller is responsible for destroying |child|. Done. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... ash/devtools/ui_element.h:43: virtual void Destroy() = 0; On 2017/05/17 21:54:13, sadrul wrote: > All the overrides just do 'delete this'. Can the callers just do 'delete > element' directly instead? Yes, done. I move ~UIElement() back to public method to enable this. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... File ash/devtools/ui_element_delegate.h (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ui_eleme... ash/devtools/ui_element_delegate.h:24: // |parent| adds |child| in DOM tree. On 2017/05/17 21:54:14, sadrul wrote: > You can remove these comments. Done. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/window_e... File ash/devtools/window_element.cc (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/window_e... ash/devtools/window_element.cc:40: if (delegate()->IsHighlightingWindow(window_)) On 2017/05/17 21:54:14, sadrul wrote: > This would never be true, right? Because we never create a WindowElement for the > highlight-window. It's true, done! https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/window_e... ash/devtools/window_element.cc:61: if (delegate()->IsHighlightingWindow(window)) On 2017/05/17 21:54:14, sadrul wrote: > You should not need this check either. See above. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
LGTM!!
The CQ bit was checked by thanhph@chromium.org
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": 1020001, "attempt_start_ts": 1495222576037480,
"parent_rev": "b04bf69ea958add88d418420802b2188cb099217", "commit_rev":
"bdcdb17a4fa7ffea34afed460b776cc2e5032855"}
Message was sent while issue was closed.
Description was changed from ========== Create a unified UIElement interface for Widget, View and Window. The unified interface will enable css_agent to set/access many properties of widget, view and window without the need of knowing the actual object type. A UIElement tree where each node can be either view, widget and window will be kept in sync with tree structures of window, widget and view objects. BUG=700024 ========== to ========== Create a unified UIElement interface for Widget, View and Window. The unified interface will enable css_agent to set/access many properties of widget, view and window without the need of knowing the actual object type. A UIElement tree where each node can be either view, widget and window will be kept in sync with tree structures of window, widget and view objects. BUG=700024 Review-Url: https://codereview.chromium.org/2776543002 Cr-Commit-Position: refs/heads/master@{#473315} Committed: https://chromium.googlesource.com/chromium/src/+/bdcdb17a4fa7ffea34afed460b77... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:1020001) as https://chromium.googlesource.com/chromium/src/+/bdcdb17a4fa7ffea34afed460b77...
Message was sent while issue was closed.
A revert of this CL (patchset #21 id:1020001) has been created in https://codereview.chromium.org/2899503002/ by afakhry@chromium.org. The reason for reverting is: Fails to compile on ChromiumOS x86-generic Compile with error: [903/7505] CXX obj/ash/ash/ui_element.o FAILED: obj/ash/ash/ui_element.o /b/c/goma_client/gomacc i686-pc-linux-gnu-g++ -B/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9536.0.0+target_toolchain/usr/x86_64-pc-linux-gnu/i686-pc-linux-gnu/binutils-bin/2.25.51-gold -MMD -MF obj/ash/ash/ui_element.o.d -DASH_IMPLEMENTATION -DV8_DEPRECATION_WARNINGS -DUSE_UDEV -DUSE_ASH=1 -DUSE_AURA=1 -DUSE_NSS_CERTS=1 -DUSE_OZONE=1 -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DFIELDTRIAL_TESTING_ENABLED -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -DOS_CHROMEOS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DUSE_EGL -DTOOLKIT_VIEWS=1 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -DUCHAR_TYPE=uint16_t -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DSK_HAS_PNG_LIBRARY -DSK_HAS_WEBP_LIBRARY -DSK_HAS_JPEG_LIBRARY -DSK_SUPPORT_GPU=1 -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DHAVE_PTHREAD -DMESA_EGL_NO_X11_HEADERS -I../.. -Igen -I../../third_party/libwebp -I../../third_party/khronos -I../../gpu -I../../third_party/ced/src -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -I../../skia/config -I../../skia/ext -I../../third_party/skia/include/c -I../../third_party/skia/include/config -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/encode -I../../third_party/skia/include/images -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pdf -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../third_party/skia/third_party/vulkan -I../../third_party/skia/include/gpu -I../../third_party/skia/src/gpu -I../../third_party/skia/src/sksl -I/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9536.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/include/dbus-1.0 -I/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9536.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/lib/dbus-1.0/include -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/protobuf/src -I../../third_party/libwebm/source -I../../third_party/boringssl/src/include -I/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9536.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/include/nss -I/b/c/b/ChromiumOS_x86_generic_Compile/.cros_cache/chrome-sdk/tarballs/x86-generic+9536.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz/usr/include/nspr -I../../third_party/WebKit -Igen/third_party/WebKit -I../../v8/include -Igen/v8/include -I../../third_party/qcms/src -Igen -I../../third_party/mesa/src/include -fno-strict-aliasing -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -funwind-tables -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin -m32 -msse2 -mfpmath=sse -mmmx -pthread -Wall -Werror -Wno-unused-local-typedefs -Wno-maybe-uninitialized -Wno-missing-field-initializers -Wno-unused-parameter -O2 -fno-ident -fdata-sections -ffunction-sections -fno-omit-frame-pointer -g2 -gsplit-dwarf --sysroot=../../../.cros_cache/chrome-sdk/tarballs/x86-generic+9536.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz -fvisibility=hidden -std=gnu++11 -Wno-narrowing -fno-rtti -fno-exceptions -fvisibility-inlines-hidden -march=i686 -pipe -march=i686 -pipe -pipe -march=i686 -mfpmath=sse -mmmx -msse -msse2 -msse3 -D__google_stl_debug_vector=1 -c ../../ash/devtools/ui_element.cc -o obj/ash/ash/ui_element.o ../../ash/devtools/ui_element.cc: In member function 'std::string ash::devtools::UIElement::GetTypeName() const': ../../ash/devtools/ui_element.cc:37:1: error: control reaches end of non-void function [-Werror=return-type] } ^ cc1plus.elf: all warnings being treated as errors.
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) confirmed this CL at revision 473315 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
It's unfortunate that we do not have a bot in the CQ that would catch this. https://codereview.chromium.org/2776543002/diff/1020001/ash/devtools/ui_eleme... File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/1020001/ash/devtools/ui_eleme... ash/devtools/ui_element.cc:36: } I guess you need to add the following after the switch: switch (type_) { ... } NOTREACHED(); return std::string();
Message was sent while issue was closed.
Description was changed from ========== Create a unified UIElement interface for Widget, View and Window. The unified interface will enable css_agent to set/access many properties of widget, view and window without the need of knowing the actual object type. A UIElement tree where each node can be either view, widget and window will be kept in sync with tree structures of window, widget and view objects. BUG=700024 Review-Url: https://codereview.chromium.org/2776543002 Cr-Commit-Position: refs/heads/master@{#473315} Committed: https://chromium.googlesource.com/chromium/src/+/bdcdb17a4fa7ffea34afed460b77... ========== to ========== Create a unified UIElement interface for Widget, View and Window. The unified interface will enable css_agent to set/access many properties of widget, view and window without the need of knowing the actual object type. A UIElement tree where each node can be either view, widget and window will be kept in sync with tree structures of window, widget and view objects. BUG=700024 Review-Url: https://codereview.chromium.org/2776543002 Cr-Commit-Position: refs/heads/master@{#473315} Committed: https://chromium.googlesource.com/chromium/src/+/bdcdb17a4fa7ffea34afed460b77... ==========
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
On 2017/05/19 21:53:40, sadrul wrote: > It's unfortunate that we do not have a bot in the CQ that would catch this. > > https://codereview.chromium.org/2776543002/diff/1020001/ash/devtools/ui_eleme... > File ash/devtools/ui_element.cc (right): > > https://codereview.chromium.org/2776543002/diff/1020001/ash/devtools/ui_eleme... > ash/devtools/ui_element.cc:36: } > I guess you need to add the following after the switch: > > switch (type_) { > ... > } > NOTREACHED(); > return std::string(); Thanks Sadrul. I'll commit this new patch. Thanh.
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 thanhph@chromium.org
The CQ bit was checked by thanhph@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/2776543002/#ps1040001 (title: ".")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sadrul@chromium.org
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": 1040001, "attempt_start_ts": 1495240152105970,
"parent_rev": "24d9a7bc5e72878b84bfb94dc0e3cdc4b489b579", "commit_rev":
"27d1ff58a6ae0788d6f9d776d3c593d853e919aa"}
Message was sent while issue was closed.
Description was changed from ========== Create a unified UIElement interface for Widget, View and Window. The unified interface will enable css_agent to set/access many properties of widget, view and window without the need of knowing the actual object type. A UIElement tree where each node can be either view, widget and window will be kept in sync with tree structures of window, widget and view objects. BUG=700024 Review-Url: https://codereview.chromium.org/2776543002 Cr-Commit-Position: refs/heads/master@{#473315} Committed: https://chromium.googlesource.com/chromium/src/+/bdcdb17a4fa7ffea34afed460b77... ========== to ========== Create a unified UIElement interface for Widget, View and Window. The unified interface will enable css_agent to set/access many properties of widget, view and window without the need of knowing the actual object type. A UIElement tree where each node can be either view, widget and window will be kept in sync with tree structures of window, widget and view objects. BUG=700024 Review-Url: https://codereview.chromium.org/2776543002 Cr-Original-Commit-Position: refs/heads/master@{#473315} Committed: https://chromium.googlesource.com/chromium/src/+/bdcdb17a4fa7ffea34afed460b77... Review-Url: https://codereview.chromium.org/2776543002 Cr-Commit-Position: refs/heads/master@{#473407} Committed: https://chromium.googlesource.com/chromium/src/+/27d1ff58a6ae0788d6f9d776d3c5... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:1040001) as https://chromium.googlesource.com/chromium/src/+/27d1ff58a6ae0788d6f9d776d3c5... |
