Chromium Code Reviews
Help | Chromium Project | Sign in
(1)

Issue 2776543002: Create a unified UIElement interface for Widget, View and Window.

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by thanhph
Modified:
2 days, 19 hours ago
Reviewers:
sadrul
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+938 lines, -445 lines) Patch
M ash/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +9 lines, -0 lines 0 comments Download
M ash/devtools/ash_devtools_css_agent.h View 1 2 3 chunks +5 lines, -6 lines 0 comments Download
M ash/devtools/ash_devtools_css_agent.cc View 1 2 3 chunks +7 lines, -55 lines 0 comments Download
M ash/devtools/ash_devtools_dom_agent.h View 1 2 3 4 5 6 3 chunks +32 lines, -87 lines 0 comments Download
M ash/devtools/ash_devtools_dom_agent.cc View 1 2 3 4 5 6 7 7 chunks +144 lines, -295 lines 0 comments Download
M ash/devtools/ash_devtools_unittest.cc View 1 2 3 chunks +23 lines, -2 lines 0 comments Download
A ash/devtools/ui_element.h View 1 2 3 4 5 6 7 1 chunk +76 lines, -0 lines 0 comments Download
A ash/devtools/ui_element.cc View 1 2 3 4 5 6 7 1 chunk +84 lines, -0 lines 0 comments Download
A ash/devtools/ui_element_delegate.h View 1 2 3 4 5 1 chunk +36 lines, -0 lines 0 comments Download
A ash/devtools/view_element.h View 1 2 3 4 1 chunk +53 lines, -0 lines 0 comments Download
A ash/devtools/view_element.cc View 1 2 3 4 5 6 1 chunk +125 lines, -0 lines 0 comments Download
A ash/devtools/widget_element.h View 1 2 3 4 1 chunk +58 lines, -0 lines 0 comments Download
A ash/devtools/widget_element.cc View 1 2 3 4 5 1 chunk +101 lines, -0 lines 0 comments Download
A ash/devtools/window_element.h View 1 2 3 4 1 chunk +55 lines, -0 lines 0 comments Download
A ash/devtools/window_element.cc View 1 2 3 4 5 1 chunk +130 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 95 (88 generated)
Fady Samuel
https://codereview.chromium.org/2776543002/diff/40001/ash/common/devtools/ash_devtools_dom_agent.h File ash/common/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2776543002/diff/40001/ash/common/devtools/ash_devtools_dom_agent.h#newcode9 ash/common/devtools/ash_devtools_dom_agent.h:9: #include "ash/common/devtools/ui_element.h" Get rid of this. Include in the ...
1 month ago (2017-03-24 22:36:21 UTC) #7
thanhph
Hi Sadrul, Can you review my CL? Thanks, Thanh.
2 weeks, 3 days ago (2017-04-11 21:29:30 UTC) #48
sadrul
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_element.cc ...
1 week, 2 days ago (2017-04-19 18:21:39 UTC) #60
thanhph
Thanks Sadrul. Please look at my new patch. https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_element.cc File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_element.cc#newcode14 ash/devtools/ui_element.cc:14: UIElement::UIElement() ...
4 days, 23 hours ago (2017-04-24 15:56:50 UTC) #74
thanhph
https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_element.cc File ash/devtools/ui_element.cc (right): https://codereview.chromium.org/2776543002/diff/420001/ash/devtools/ui_element.cc#newcode16 ash/devtools/ui_element.cc:16: UIElement::~UIElement() {} On 2017/04/24 15:56:50, thanhph wrote: > On ...
4 days, 22 hours ago (2017-04-24 16:51:28 UTC) #77
sadrul
https://codereview.chromium.org/2776543002/diff/520001/ash/devtools/ui_element_delegate.h File ash/devtools/ui_element_delegate.h (right): https://codereview.chromium.org/2776543002/diff/520001/ash/devtools/ui_element_delegate.h#newcode26 ash/devtools/ui_element_delegate.h:26: virtual std::vector<UIElement*>::iterator OnUIElementRemoved(int node_id); This is pretty weird. Why ...
4 days, 20 hours ago (2017-04-24 18:35:22 UTC) #80
thanhph
3 days, 1 hour ago (2017-04-26 13:50:38 UTC) #89
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46