Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(19)

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

Created:
8 months ago by thanhph
Modified:
6 months 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 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 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+908 lines, -477 lines) Patch
M ash/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 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 4 5 6 7 8 9 10 11 4 chunks +12 lines, -54 lines 0 comments Download
M ash/devtools/ash_devtools_dom_agent.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +30 lines, -89 lines 0 comments Download
M ash/devtools/ash_devtools_dom_agent.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 8 chunks +186 lines, -326 lines 0 comments Download
M ash/devtools/ash_devtools_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +40 lines, -2 lines 0 comments Download
A ash/devtools/ui_element.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +78 lines, -0 lines 0 comments Download
A ash/devtools/ui_element.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +80 lines, -0 lines 0 comments Download
A ash/devtools/ui_element_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +44 lines, -0 lines 0 comments Download
A ash/devtools/view_element.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +52 lines, -0 lines 0 comments Download
A ash/devtools/view_element.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +88 lines, -0 lines 0 comments Download
A ash/devtools/widget_element.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +57 lines, -0 lines 0 comments Download
A ash/devtools/widget_element.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +73 lines, -0 lines 0 comments Download
A ash/devtools/window_element.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +54 lines, -0 lines 0 comments Download
A ash/devtools/window_element.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +100 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 204 (173 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 ...
7 months, 4 weeks ago (2017-03-24 22:36:21 UTC) #7
thanhph
Hi Sadrul, Can you review my CL? Thanks, Thanh.
7 months, 1 week 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 ...
7 months 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() ...
6 months, 4 weeks 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 ...
6 months, 4 weeks 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 ...
6 months, 4 weeks ago (2017-04-24 18:35:22 UTC) #80
thanhph
Closing Chrome doesn't crash anymore during normal user case test. Please review my new patch. ...
6 months, 3 weeks ago (2017-04-26 13:50:38 UTC) #89
sadrul
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_devtools_dom_agent.cc ...
6 months, 3 weeks ago (2017-05-01 16:22:27 UTC) #98
sadrul
Not sure if this is ready for review yet, but I just took a look, ...
6 months, 2 weeks ago (2017-05-03 03:02:07 UTC) #101
thanhph
Thanks Sadrul. I clean up the code based on your comments. Thanh. https://codereview.chromium.org/2776543002/diff/600001/ash/devtools/ash_devtools_dom_agent.cc File ash/devtools/ash_devtools_dom_agent.cc ...
6 months, 2 weeks ago (2017-05-03 21:58:11 UTC) #117
sadrul
I looked at UIElement and DOMAgent mostly, since I think we need to clean up ...
6 months, 2 weeks ago (2017-05-05 20:19:34 UTC) #124
MTBlife43va
On 2017/05/05 20:19:34, sadrul wrote: > I looked at UIElement and DOMAgent mostly, since I ...
6 months, 2 weeks ago (2017-05-08 00:07:31 UTC) #133
MTBlife43va
On 2017/05/05 20:19:34, sadrul wrote: > I looked at UIElement and DOMAgent mostly, since I ...
6 months, 2 weeks ago (2017-05-08 00:07:33 UTC) #134
thanhph
https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devtools_dom_agent.cc File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2776543002/diff/740001/ash/devtools/ash_devtools_dom_agent.cc#newcode143 ash/devtools/ash_devtools_dom_agent.cc:143: if (parent->GetChildren().empty()) On 2017/05/05 20:19:33, sadrul wrote: > Use ...
6 months, 2 weeks ago (2017-05-08 17:03:31 UTC) #141
sadrul
https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ash_devtools_dom_agent.h File ash/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ash_devtools_dom_agent.h#newcode34 ash/devtools/ash_devtools_dom_agent.h:34: public AshDevToolsDOMAgentObserver { DOMAgent itself does not need to ...
6 months, 2 weeks ago (2017-05-09 04:58:07 UTC) #150
thanhph
Thanks Sadrul. I address your comments below. https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ash_devtools_dom_agent.h File ash/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2776543002/diff/880001/ash/devtools/ash_devtools_dom_agent.h#newcode34 ash/devtools/ash_devtools_dom_agent.h:34: public AshDevToolsDOMAgentObserver ...
6 months, 1 week ago (2017-05-09 20:52:47 UTC) #156
sadrul
Looking really good! Some pretty small changes. https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ash_devtools_dom_agent.cc File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ash_devtools_dom_agent.cc#newcode212 ash/devtools/ash_devtools_dom_agent.cc:212: window_element_root_ = ...
6 months, 1 week ago (2017-05-10 03:03:38 UTC) #159
thanhph
Thanks Sadrul. I clean up a few more things based on your feedback. https://codereview.chromium.org/2776543002/diff/920001/ash/devtools/ash_devtools_dom_agent.cc File ...
6 months, 1 week ago (2017-05-10 17:21:49 UTC) #168
sadrul
Last set of nits. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devtools_dom_agent.cc File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devtools_dom_agent.cc#newcode49 ash/devtools/ash_devtools_dom_agent.cc:49: std::unique_ptr<Array<std::string>> attributes = Array<std::string>::create(); Leave ...
6 months ago (2017-05-17 21:54:14 UTC) #175
thanhph
Thanks Sadrul. Please review my new patch. Thanh. https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devtools_dom_agent.cc File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2776543002/diff/1000001/ash/devtools/ash_devtools_dom_agent.cc#newcode49 ash/devtools/ash_devtools_dom_agent.cc:49: std::unique_ptr<Array<std::string>> ...
6 months ago (2017-05-18 14:28:16 UTC) #178
sadrul
LGTM!!
6 months ago (2017-05-19 19:31:40 UTC) #181
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2776543002/1020001
6 months ago (2017-05-19 19:37:37 UTC) #183
commit-bot: I haz the power
Committed patchset #21 (id:1020001) as https://chromium.googlesource.com/chromium/src/+/bdcdb17a4fa7ffea34afed460b776cc2e5032855
6 months ago (2017-05-19 21:07:28 UTC) #186
afakhry
A revert of this CL (patchset #21 id:1020001) has been created in https://codereview.chromium.org/2899503002/ by afakhry@chromium.org. ...
6 months ago (2017-05-19 21:34:49 UTC) #187
findit-for-me
Findit (https://goo.gl/kROfz5) confirmed this CL at revision 473315 as the culprit for failures in the ...
6 months ago (2017-05-19 21:40:24 UTC) #188
sadrul
It's unfortunate that we do not have a bot in the CQ that would catch ...
6 months ago (2017-05-19 21:53:40 UTC) #189
thanhph
On 2017/05/19 21:53:40, sadrul wrote: > It's unfortunate that we do not have a bot ...
6 months ago (2017-05-19 22:20:52 UTC) #192
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2776543002/1040001
6 months ago (2017-05-19 22:22:30 UTC) #197
commit-bot: I haz the power
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_android_rel_ng/builds/298678)
6 months ago (2017-05-19 23:40:24 UTC) #199
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2776543002/1040001
6 months ago (2017-05-20 00:29:37 UTC) #201
commit-bot: I haz the power
6 months ago (2017-05-20 04:09:45 UTC) #204
Message was sent while issue was closed.
Committed patchset #22 (id:1040001) as
https://chromium.googlesource.com/chromium/src/+/27d1ff58a6ae0788d6f9d776d3c5...

Powered by Google App Engine
This is Rietveld efc10ee0f