Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2548103002:
Make bounds editable through the CSS sidepanel (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. |
DescriptionMake bounds editable through the CSS sidepanel
This CL adds the feature which allows users to edit
bounds (i.e: height, width, x, y) of any window/widget/view in Ash by
simply updating them in the CSS sidepanel.
- In order to make fields editable, all CSSStyle and CSSProperty objects
sent require a range field, which in our case is all 0s
because we don't have any stylesheets.
- Added setStyleTexts which is the command called by the frontend whenever
a CSS property is updated. This is sent to the CSSAgent in the form of
text. Example: "height: 1;"
- The text received in setStyleTexts is parsed by the CSSAgent, and
put into a bounds object. Any bounds properties not included in the
|style_text| must default to the original property value for that window,
widget or view.
- The updated bounds are sent back to the frontend and the bounds for the
object are updated on the main thread.
- A lot of error checking must happen as well. This CL checks for the
following cases: invalid node id, node id valid but not found, unsupported
property and invalid value for a given property.
BUG=648701
Committed: https://crrev.com/d35614ff6bf165d7aec31b2e9bb45cadecdce21e
Cr-Commit-Position: refs/heads/master@{#437072}
Patch Set 1 #Patch Set 2 : Make bounds editable through the CSS sidepanel #
Total comments: 16
Patch Set 3 : sadruls comments #Patch Set 4 : sadruls comments #
Total comments: 4
Patch Set 5 : sadruls comments #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 39 (31 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@!
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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.cc (right): https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:29: .setEndColumn(0) What do these mean? https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:66: std::string property = tokens.at(i); Use const & to avoid extra string copy https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:77: else if (property == kY) Use {} for all, or none. https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:132: auto edit = edits->get(i); auto&? https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:151: base::Unretained(this), node_id, updated_bounds)); Why post-task? https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:207: window->SetBounds(bounds); return too from here, right? https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:211: widget->SetBounds(bounds); ditto https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.h (right): https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.h:48: bool GetBoundsForNodeId(int node_id, gfx::Rect& bounds); Make this a gfx::Rect* The styleguide requires that the out-params are pointers, and not refs.
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...
https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.cc (right): https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:29: .setEndColumn(0) On 2016/12/06 20:56:30, sadrul (OOO) wrote: > What do these mean? Added comment. https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:66: std::string property = tokens.at(i); On 2016/12/06 20:56:30, sadrul (OOO) wrote: > Use const & to avoid extra string copy Done. https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:77: else if (property == kY) On 2016/12/06 20:56:30, sadrul (OOO) wrote: > Use {} for all, or none. Removed {} https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:132: auto edit = edits->get(i); On 2016/12/06 20:56:30, sadrul (OOO) wrote: > auto&? Done. https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:151: base::Unretained(this), node_id, updated_bounds)); On 2016/12/06 20:56:30, sadrul (OOO) wrote: > Why post-task? As discussed, removed. https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:207: window->SetBounds(bounds); On 2016/12/06 20:56:30, sadrul (OOO) wrote: > return too from here, right? Done. https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:211: widget->SetBounds(bounds); On 2016/12/06 20:56:30, sadrul (OOO) wrote: > ditto Done. https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.h (right): https://codereview.chromium.org/2548103002/diff/20001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.h:48: bool GetBoundsForNodeId(int node_id, gfx::Rect& bounds); On 2016/12/06 20:56:30, sadrul (OOO) wrote: > Make this a gfx::Rect* > > The styleguide requires that the out-params are pointers, and not refs. Done.
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 https://codereview.chromium.org/2548103002/diff/60001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.cc (right): https://codereview.chromium.org/2548103002/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:137: gfx::Rect updated_bounds = gfx::Rect(); Don't need the = gfx::Rect() part. The default ctor will take care of initializing it. https://codereview.chromium.org/2548103002/diff/60001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.h (right): https://codereview.chromium.org/2548103002/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.h:20: AshDevToolsCSSAgent(AshDevToolsDOMAgent* dom_agent); Add back explicit
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.
The CQ bit was checked by mhashmi@chromium.org to run a CQ dry run
https://codereview.chromium.org/2548103002/diff/60001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.cc (right): https://codereview.chromium.org/2548103002/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.cc:137: gfx::Rect updated_bounds = gfx::Rect(); On 2016/12/07 17:42:24, sadrul wrote: > Don't need the = gfx::Rect() part. The default ctor will take care of > initializing it. Done. https://codereview.chromium.org/2548103002/diff/60001/ash/common/devtools/ash... File ash/common/devtools/ash_devtools_css_agent.h (right): https://codereview.chromium.org/2548103002/diff/60001/ash/common/devtools/ash... ash/common/devtools/ash_devtools_css_agent.h:20: AshDevToolsCSSAgent(AshDevToolsDOMAgent* dom_agent); On 2016/12/07 17:42:24, sadrul wrote: > Add back explicit Done.
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 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.
The CQ bit was checked by mhashmi@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/2548103002/#ps80001 (title: "sadruls comments")
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": 80001, "attempt_start_ts": 1481147037065780,
"parent_rev": "a28a710f394e066d85f2eb991b5ac2176e88b4fb", "commit_rev":
"de71498cba9d403fa5b159017bccb4ab49c32032"}
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Make bounds editable through the CSS sidepanel This CL adds the feature which allows users to edit bounds (i.e: height, width, x, y) of any window/widget/view in Ash by simply updating them in the CSS sidepanel. - In order to make fields editable, all CSSStyle and CSSProperty objects sent require a range field, which in our case is all 0s because we don't have any stylesheets. - Added setStyleTexts which is the command called by the frontend whenever a CSS property is updated. This is sent to the CSSAgent in the form of text. Example: "height: 1;" - The text received in setStyleTexts is parsed by the CSSAgent, and put into a bounds object. Any bounds properties not included in the |style_text| must default to the original property value for that window, widget or view. - The updated bounds are sent back to the frontend and the bounds for the object are updated on the main thread. - A lot of error checking must happen as well. This CL checks for the following cases: invalid node id, node id valid but not found, unsupported property and invalid value for a given property. BUG=648701 ========== to ========== Make bounds editable through the CSS sidepanel This CL adds the feature which allows users to edit bounds (i.e: height, width, x, y) of any window/widget/view in Ash by simply updating them in the CSS sidepanel. - In order to make fields editable, all CSSStyle and CSSProperty objects sent require a range field, which in our case is all 0s because we don't have any stylesheets. - Added setStyleTexts which is the command called by the frontend whenever a CSS property is updated. This is sent to the CSSAgent in the form of text. Example: "height: 1;" - The text received in setStyleTexts is parsed by the CSSAgent, and put into a bounds object. Any bounds properties not included in the |style_text| must default to the original property value for that window, widget or view. - The updated bounds are sent back to the frontend and the bounds for the object are updated on the main thread. - A lot of error checking must happen as well. This CL checks for the following cases: invalid node id, node id valid but not found, unsupported property and invalid value for a given property. BUG=648701 Committed: https://crrev.com/d35614ff6bf165d7aec31b2e9bb45cadecdce21e Cr-Commit-Position: refs/heads/master@{#437072} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d35614ff6bf165d7aec31b2e9bb45cadecdce21e Cr-Commit-Position: refs/heads/master@{#437072} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
