Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902063004:
View shows wrong bound when it is highlighted. (Closed)
DescriptionView shows wrong bound when it is highlighted.
Hovering mouse on the DOM node on views highlights wrong area on
screen.
BUG=726161
Review-Url: https://codereview.chromium.org/2902063004
Cr-Commit-Position: refs/heads/master@{#475213}
Committed: https://chromium.googlesource.com/chromium/src/+/46d59d4b14841bdc82c24ad95d2c446fd508fdb4
Patch Set 1 #Patch Set 2 : . #
Total comments: 4
Patch Set 3 : use GetElementFromNodeId to get ui_element. #
Total comments: 3
Patch Set 4 : . #
Messages
Total messages: 30 (22 generated)
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 ========== View shows wrong bound when it is highlighted. Hovering mouse on the DOM node on views highlights wrong area. BUG=726161 ========== to ========== View shows wrong bound when it is highlighted. Hovering mouse on the DOM node on views highlights wrong area on screen. BUG=726161 ==========
thanhph@chromium.org changed reviewers: + sadrul@chromium.org
Hi Sadrul, I used wrong view bound coordinate so here's the fix. Thanks, Thanh.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you add a test?
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...
Ok, I added a few EXPECT_EQ for (Widget|View|Window)Element in WindowWidgetViewHighlight test. Thanh.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2902063004/diff/20001/ash/devtools/ash_devtoo... File ash/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2902063004/diff/20001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_dom_agent.h:38: std::unordered_map<int, UIElement*>& node_id_to_ui_element() { Make it const. https://codereview.chromium.org/2902063004/diff/20001/ash/devtools/ash_devtoo... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2902063004/diff/20001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:642: dom_agent()->node_id_to_ui_element()[window_node->getNodeId()]); Can you just use dom_agent()->GetElementFromNodeId() here?
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.
Thanks Sadrul. Please review my new patch. Thanh. https://codereview.chromium.org/2902063004/diff/20001/ash/devtools/ash_devtoo... File ash/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2902063004/diff/20001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_dom_agent.h:38: std::unordered_map<int, UIElement*>& node_id_to_ui_element() { On 2017/05/25 19:07:28, sadrul wrote: > Make it const. I removed this function. https://codereview.chromium.org/2902063004/diff/20001/ash/devtools/ash_devtoo... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2902063004/diff/20001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:642: dom_agent()->node_id_to_ui_element()[window_node->getNodeId()]); On 2017/05/25 19:07:28, sadrul wrote: > Can you just use dom_agent()->GetElementFromNodeId() here? Done.
lgtm after the following changes in the test. https://codereview.chromium.org/2902063004/diff/40001/ash/devtools/ash_devtoo... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2902063004/diff/40001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:641: static_cast<devtools::WindowElement*>( You don't need to use a WindowElement* here. Just: devtools::UIElement* element = dom_agent()->GetElementFromNodeId(window_node->getNodeId()); ASSERT_EQ(devtools::UIElementType::WINDOW, element->type()); EXPECT_EQ(element->GetNodeWindowAndBounds().first, window.get()); EXPECT_EQ(element->GetNodeWindowAndBounds().second, window->GetBoundsInScreen()); https://codereview.chromium.org/2902063004/diff/40001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:654: dom_agent()->GetElementFromNodeId(widget_node->getNodeId())); Don't need the static cast. Just use UIElement. https://codereview.chromium.org/2902063004/diff/40001/ash/devtools/ash_devtoo... ash/devtools/ash_devtools_unittest.cc:665: devtools::ViewElement* view_element = static_cast<devtools::ViewElement*>( ditto
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
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/2902063004/#ps60001 (title: ".")
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": 60001, "attempt_start_ts": 1495842316236140,
"parent_rev": "833aa96095e1f827ffc05af8bf545a094ffa6e35", "commit_rev":
"46d59d4b14841bdc82c24ad95d2c446fd508fdb4"}
Message was sent while issue was closed.
Description was changed from ========== View shows wrong bound when it is highlighted. Hovering mouse on the DOM node on views highlights wrong area on screen. BUG=726161 ========== to ========== View shows wrong bound when it is highlighted. Hovering mouse on the DOM node on views highlights wrong area on screen. BUG=726161 Review-Url: https://codereview.chromium.org/2902063004 Cr-Commit-Position: refs/heads/master@{#475213} Committed: https://chromium.googlesource.com/chromium/src/+/46d59d4b14841bdc82c24ad95d2c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/46d59d4b14841bdc82c24ad95d2c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
