Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529553002:
Hook up views/widgets to AshDevToolsDOMAgent (Closed)
|
Created:
4 years ago by Sarmad Hashmi Modified:
4 years ago Reviewers:
sadrul CC:
chromium-reviews, kalyank, sadrul, pfeldman, devtools-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHook up views/widgets to AshDevToolsDOMAgent
- This CL adds AshDevToolsDOMAgent as an observer to all widgets and views.
The WidgetRemovalObserver is specifically to remove the RootView subtree,
while the ViewObserver is to update any other descendant views.
AddViewTree was created so that whenever any |view| is added, it can
be pushed to the frontend.
- Add tests for when views are inserted, removed, rearranged,
removed & inserted.
- Add test to check if a widget owned by a native widget is represented
properly in the DOM.
BUG=648701
Committed: https://crrev.com/b4ce166c6eb7bf65dedbdb6947e23fbf2992e251
Cr-Commit-Position: refs/heads/master@{#434863}
Patch Set 1 #
Total comments: 6
Patch Set 2 : sadruls comments and merge test CL with this #Patch Set 3 : sadruls comments and merge test CL with this #
Depends on Patchset: Messages
Total messages: 26 (18 generated)
The CQ bit was checked by mhashmi@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...
mhashmi@chromium.org changed reviewers: + sadrul@chromium.org
PTAL sadrul@. Just hooking up views/widget observers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It'd be make sense to move the relevant tests (in https://codereview.chromium.org/2528563002/) into this CL as well. https://codereview.chromium.org/2529553002/diff/1/ash/common/devtools/ash_dev... File ash/common/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2529553002/diff/1/ash/common/devtools/ash_dev... ash/common/devtools/ash_devtools_dom_agent.cc:78: DCHECK(view_index >= 0); DCHECK_GE https://codereview.chromium.org/2529553002/diff/1/ash/common/devtools/ash_dev... ash/common/devtools/ash_devtools_dom_agent.cc:222: if (!widget->HasRemovalsObserver(this)) Do we need to check here? If this ever returns true, that would mean the DCHECK above in line 217 would trip, right? https://codereview.chromium.org/2529553002/diff/1/ash/common/devtools/ash_dev... ash/common/devtools/ash_devtools_dom_agent.cc:237: if (!view->HasObserver(this)) ditto
The CQ bit was checked by mhashmi@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...
Merged the tests CL with this one. https://codereview.chromium.org/2529553002/diff/1/ash/common/devtools/ash_dev... File ash/common/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2529553002/diff/1/ash/common/devtools/ash_dev... ash/common/devtools/ash_devtools_dom_agent.cc:78: DCHECK(view_index >= 0); On 2016/11/25 18:27:48, sadrul wrote: > DCHECK_GE Done. https://codereview.chromium.org/2529553002/diff/1/ash/common/devtools/ash_dev... ash/common/devtools/ash_devtools_dom_agent.cc:222: if (!widget->HasRemovalsObserver(this)) On 2016/11/25 18:27:48, sadrul wrote: > Do we need to check here? If this ever returns true, that would mean the DCHECK > above in line 217 would trip, right? Not necessarily. If this returns true, then that means |remove_observer| was false when calling the remove methods. This is so that when methods like OnWindowStackingChanged or OnChildViewReordered are called, we don't remove and add the observer in the callbacks (which would result in endless calls to the observer). So, widget->HasRemovalsObserver(this) or (view|window)->HasObserver(this) would return true if we ever have a case where the node is being removed and added within the same callback (observer method). https://codereview.chromium.org/2529553002/diff/1/ash/common/devtools/ash_dev... ash/common/devtools/ash_devtools_dom_agent.cc:237: if (!view->HasObserver(this)) On 2016/11/25 18:27:48, sadrul wrote: > ditto Same as above.
Description was changed from ========== Hook up views/widgets to AshDevToolsDOMAgent This CL adds AshDevToolsDOMAgent as an observer to all widgets and views. The WidgetRemovalObserver is specifically to remove the RootView subtree, while the ViewObserver is to update any other descendant views. AddViewTree was created so that whenever any |view| is added, it can be pushed to the frontend. BUG=648701 ========== to ========== Hook up views/widgets to AshDevToolsDOMAgent - This CL adds AshDevToolsDOMAgent as an observer to all widgets and views. The WidgetRemovalObserver is specifically to remove the RootView subtree, while the ViewObserver is to update any other descendant views. AddViewTree was created so that whenever any |view| is added, it can be pushed to the frontend. - Add tests for when views are inserted, removed, rearranged, removed & inserted. - Add test to check if a widget owned by a native widget is represented properly in the DOM. BUG=648701 ==========
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 mhashmi@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.
lgtm
The CQ bit was checked by mhashmi@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": 40001, "attempt_start_ts": 1480388006346420,
"parent_rev": "9f6d61d8f05b8b0062b4e27152e44786ea91b7e7", "commit_rev":
"881e780f83d9f16292f6472f65beabfaeebfe7d3"}
Message was sent while issue was closed.
Description was changed from ========== Hook up views/widgets to AshDevToolsDOMAgent - This CL adds AshDevToolsDOMAgent as an observer to all widgets and views. The WidgetRemovalObserver is specifically to remove the RootView subtree, while the ViewObserver is to update any other descendant views. AddViewTree was created so that whenever any |view| is added, it can be pushed to the frontend. - Add tests for when views are inserted, removed, rearranged, removed & inserted. - Add test to check if a widget owned by a native widget is represented properly in the DOM. BUG=648701 ========== to ========== Hook up views/widgets to AshDevToolsDOMAgent - This CL adds AshDevToolsDOMAgent as an observer to all widgets and views. The WidgetRemovalObserver is specifically to remove the RootView subtree, while the ViewObserver is to update any other descendant views. AddViewTree was created so that whenever any |view| is added, it can be pushed to the frontend. - Add tests for when views are inserted, removed, rearranged, removed & inserted. - Add test to check if a widget owned by a native widget is represented properly in the DOM. BUG=648701 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Hook up views/widgets to AshDevToolsDOMAgent - This CL adds AshDevToolsDOMAgent as an observer to all widgets and views. The WidgetRemovalObserver is specifically to remove the RootView subtree, while the ViewObserver is to update any other descendant views. AddViewTree was created so that whenever any |view| is added, it can be pushed to the frontend. - Add tests for when views are inserted, removed, rearranged, removed & inserted. - Add test to check if a widget owned by a native widget is represented properly in the DOM. BUG=648701 ========== to ========== Hook up views/widgets to AshDevToolsDOMAgent - This CL adds AshDevToolsDOMAgent as an observer to all widgets and views. The WidgetRemovalObserver is specifically to remove the RootView subtree, while the ViewObserver is to update any other descendant views. AddViewTree was created so that whenever any |view| is added, it can be pushed to the frontend. - Add tests for when views are inserted, removed, rearranged, removed & inserted. - Add test to check if a widget owned by a native widget is represented properly in the DOM. BUG=648701 Committed: https://crrev.com/b4ce166c6eb7bf65dedbdb6947e23fbf2992e251 Cr-Commit-Position: refs/heads/master@{#434863} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b4ce166c6eb7bf65dedbdb6947e23fbf2992e251 Cr-Commit-Position: refs/heads/master@{#434863}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2532243003/ by battre@chromium.org. The reason for reverting is: Reverting due to a memory leak. See http://crbug.com/648701#c26. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
