Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2739523002:
Ash devtools: Add support for changing visibility. (Closed)
DescriptionAsh devtools: Add support for changing visibility.
Adding visibility property will enable a window/widget/view to be shown
or hidden by simply changing the value 1 (visible) to 0 (hide) in the
element CSS style.
BUG=697175
Review-Url: https://codereview.chromium.org/2739523002
Cr-Commit-Position: refs/heads/master@{#457745}
Committed: https://chromium.googlesource.com/chromium/src/+/6f8c23bd5124fa128de72da19d6b1f50e22d52cc
Patch Set 1 : Add visibility property. #Patch Set 2 : . #
Total comments: 11
Patch Set 3 : Address comments. #
Total comments: 8
Patch Set 4 : . #
Total comments: 5
Patch Set 5 : Add unittest. Fix nits. #
Messages
Total messages: 49 (37 generated)
Description was changed from ========== Ash devtools: Add support for changing visibility. BUG=697175 ========== to ========== Ash devtools: Add support for changing visibility. Adding visibility property will enable a window/widget/view to be shown or hidden by simply changing the value 1 (visible) to 0 (hide) in the element CSS style. BUG=697175 ==========
Description was changed from ========== Ash devtools: Add support for changing visibility. Adding visibility property will enable a window/widget/view to be shown or hidden by simply changing the value 1 (visible) to 0 (hide) in the element CSS style. BUG=697175 ========== to ========== Ash devtools: Add support for changing visibility. Adding visibility property will enable a window/widget/view to be shown or hidden by simply changing the value 1 (visible) to 0 (hide) in the element CSS style. BUG=697175 ==========
The CQ bit was checked by thanhph@google.com 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:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by thanhph@google.com 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@google.com 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...
thanhph@google.com changed reviewers: + sadrul@chromium.org
Hi Sadrul, Please review my cl. Thanks, Thanh.
Patchset #1 (id:40001) has been deleted
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@google.com 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.
varkha@chromium.org changed reviewers: + varkha@chromium.org
Quick drive-by. https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.cc (right): https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:48: const bool& visible) { Here and below. No need to pass simple types by reference. https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:97: "No node id " + std::to_string(node_id) + " found"); Maybe format as "Node with id=<node_id> not found" (here and below). Also this could be a utility method to avoid duplication. https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.h (right): https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.h:46: bool GetNodeIdProperties(int node_id, gfx::Rect* bounds, bool* visible); GetPropertiesForNodeId https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.h:47: bool UpdateObjectProperties(int node_id, [Set|Update]PropertiesForNodeId https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.h:53: bool* visible); What was the reason to make this into a class method? Is it using |this| anywhere?
Sorry, lost track of this. https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.cc (right): https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:97: "No node id " + std::to_string(node_id) + " found"); On 2017/03/08 15:23:59, varkha wrote: > Maybe format as "Node with id=<node_id> not found" (here and below). Also this > could be a utility method to avoid duplication. +1 re utility method to generate the error message.
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Thanks Valery. I added a new patch. Thanh. https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.cc (right): https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:48: const bool& visible) { On 2017/03/08 15:23:59, varkha wrote: > Here and below. No need to pass simple types by reference. Done. https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:97: "No node id " + std::to_string(node_id) + " found"); On 2017/03/08 15:38:59, sadrul wrote: > On 2017/03/08 15:23:59, varkha wrote: > > Maybe format as "Node with id=<node_id> not found" (here and below). Also this > > could be a utility method to avoid duplication. > > +1 re utility method to generate the error message. Done, thanks! A helper function is added in anon namespace. ui::devtools::protocol::Response NodeNotFoundError(int node_id) { return ui::devtools::protocol::Response::Error( "Node with id=" + std::to_string(node_id) + " not found"); } https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.h (right): https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.h:46: bool GetNodeIdProperties(int node_id, gfx::Rect* bounds, bool* visible); On 2017/03/08 15:23:59, varkha wrote: > GetPropertiesForNodeId Done. https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.h:47: bool UpdateObjectProperties(int node_id, On 2017/03/08 15:23:59, varkha wrote: > [Set|Update]PropertiesForNodeId Done, I use SetPropertiesForNodeId(). Thanks! https://codereview.chromium.org/2739523002/diff/80001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.h:53: bool* visible); On 2017/03/08 15:23:59, varkha wrote: > What was the reason to make this into a class method? Is it using |this| > anywhere? Yes it uses |this->dom_agent_| to retrieve either view, widget or window. Passing these 3 objects as a input parameter would be a bit ugly. Sardul and I have discussed to refactor this in a next CL which will create a unified interface for view, widget or window. This refactor will need to change CCSAgent or DomAgent. https://bugs.chromium.org/p/chromium/issues/detail?id=700024
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/2739523002/diff/100001/ash/common/devtools/as... File ash/common/devtools/ash_devtools_css_agent.cc (right): https://codereview.chromium.org/2739523002/diff/100001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.cc:100: NodeNotFoundError(node_id); return NodeNotFoundError(node_id); https://codereview.chromium.org/2739523002/diff/100001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.cc:123: NodeNotFoundError(node_id); return (applies same comment applies to code below) https://codereview.chromium.org/2739523002/diff/100001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.cc:224: Response AshDevToolsCSSAgent::ParseProperties(const std::string& style_text, Is there a reason this function can't remain as an independent function in the anonymous namespace above? (this doesn't actually seem to use |dom_agent_| or anything?) https://codereview.chromium.org/2739523002/diff/100001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.cc:232: if (!base::StringToInt(tokens.at(i + 1), &value)) Is visibility sent as an int too?
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/2739523002/diff/100001/ash/common/devtools/as... File ash/common/devtools/ash_devtools_css_agent.cc (right): https://codereview.chromium.org/2739523002/diff/100001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.cc:100: NodeNotFoundError(node_id); On 2017/03/09 18:11:33, sadrul wrote: > return NodeNotFoundError(node_id); oops, thanks! https://codereview.chromium.org/2739523002/diff/100001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.cc:123: NodeNotFoundError(node_id); On 2017/03/09 18:11:33, sadrul wrote: > return > > (applies same comment applies to code below) Done. https://codereview.chromium.org/2739523002/diff/100001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.cc:224: Response AshDevToolsCSSAgent::ParseProperties(const std::string& style_text, On 2017/03/09 18:11:33, sadrul wrote: > Is there a reason this function can't remain as an independent function in the > anonymous namespace above? (this doesn't actually seem to use |dom_agent_| or > anything?) My bad, I was using |dom_agent_| here to reduce code duplication before we discussed the new interface. I move ParseProperties() back to anon namespace. https://codereview.chromium.org/2739523002/diff/100001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.cc:232: if (!base::StringToInt(tokens.at(i + 1), &value)) On 2017/03/09 18:11:33, sadrul wrote: > Is visibility sent as an int too? Yes, 0 for non-visible and 1 is visible. We could also parse the word "true" or "false" but this is probably more costly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good! Can you add a test in ash_devtools_unittest.cc?
https://codereview.chromium.org/2739523002/diff/120001/ash/common/devtools/as... File ash/common/devtools/ash_devtools_css_agent.cc (right): https://codereview.chromium.org/2739523002/diff/120001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.cc:58: const gfx::Rect& bounds, Not sure if const is necessary for simple types like bool here. https://codereview.chromium.org/2739523002/diff/120001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.cc:181: bool visible; Can you please set initial value? https://codereview.chromium.org/2739523002/diff/120001/ash/common/devtools/as... File ash/common/devtools/ash_devtools_css_agent.h (right): https://codereview.chromium.org/2739523002/diff/120001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.h:49: const bool visible); Not sure if const is necessary for simple types passed as input.
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 #5 (id:140001) 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 Valery and Sadrul. Please review my new patch. I added a visibility attribute in existing unit tests. Thanh. https://codereview.chromium.org/2739523002/diff/120001/ash/common/devtools/as... File ash/common/devtools/ash_devtools_css_agent.cc (right): https://codereview.chromium.org/2739523002/diff/120001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.cc:58: const gfx::Rect& bounds, On 2017/03/10 20:17:58, varkha (OOO Mar 15) wrote: > Not sure if const is necessary for simple types like bool here. Done. I removed it here. https://codereview.chromium.org/2739523002/diff/120001/ash/common/devtools/as... ash/common/devtools/ash_devtools_css_agent.cc:181: bool visible; On 2017/03/10 20:17:58, varkha wrote: > Can you please set initial value? I set |visible| to false here and above, thanks! |bounds| has its default constructor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Cool! 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": 160001, "attempt_start_ts": 1489755207616290,
"parent_rev": "5d465e529f51519165cf401267cbf17888750899", "commit_rev":
"6f8c23bd5124fa128de72da19d6b1f50e22d52cc"}
Message was sent while issue was closed.
Description was changed from ========== Ash devtools: Add support for changing visibility. Adding visibility property will enable a window/widget/view to be shown or hidden by simply changing the value 1 (visible) to 0 (hide) in the element CSS style. BUG=697175 ========== to ========== Ash devtools: Add support for changing visibility. Adding visibility property will enable a window/widget/view to be shown or hidden by simply changing the value 1 (visible) to 0 (hide) in the element CSS style. BUG=697175 Review-Url: https://codereview.chromium.org/2739523002 Cr-Commit-Position: refs/heads/master@{#457745} Committed: https://chromium.googlesource.com/chromium/src/+/6f8c23bd5124fa128de72da19d6b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://chromium.googlesource.com/chromium/src/+/6f8c23bd5124fa128de72da19d6b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
