Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2090953007:
MD Settings: Display: Reparent children of dragged displays (Closed)
|
Created:
4 years, 6 months ago by stevenjb Modified:
4 years, 5 months ago Reviewers:
michaelpg CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@issue_547080_display_settings8b_collide Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Display: Reparent children of dragged displays
BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/30de137be92a0b6eb565fa1882fd6126ff98cc5b
Cr-Commit-Position: refs/heads/master@{#404023}
Patch Set 1 #Patch Set 2 : . #
Total comments: 17
Patch Set 3 : Feedback #Patch Set 4 : Rebase #Patch Set 5 : Rebase #Messages
Total messages: 16 (5 generated)
Description was changed from ========== MD Settings: Display: Reparent children of dragged displays BUG=547080 ========== to ========== MD Settings: Display: Reparent children of dragged displays BUG=547080 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + michaelpg@chromium.org
Part 3/3 Last but not least, this handles the children of dragged displays, which need to be re-parented to ensure that they have a valid non overlapping layout.
I'm not gonna pretend I grokked all of this, but nothing jumped out as wrong. Sorry of some of the nits were dupes of the previous CL. https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/layout_behavior.js (right): https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:161: // Re-parent the dragged display nit: period https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:204: * Re-parent all entries in |orphanIds| and any children. nit: indicative ("Re-parents") here & below https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:205: * @param {Array<string>} orphanIds The list of ids affected by the move. nit: !Array<string> https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:215: if (orphans.indexOf(o) == -1) !orphans.includes(o) also, consider using a Set if order doesn't matter https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:237: var layout = this.displayLayoutMap_.get(orphanId); opt nit: var layout = assert(...); https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:254: // Move from the nearest corner to the desired locaiton and get the delta. location https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:643: * Return the position of the corner of the div closest to |pos|. nits: Return => Returns, position => bounds, pos => somethingelse
https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/layout_behavior.js (right): https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:161: // Re-parent the dragged display On 2016/06/29 20:16:50, michaelpg wrote: > nit: period Done. https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:204: * Re-parent all entries in |orphanIds| and any children. On 2016/06/29 20:16:51, michaelpg wrote: > nit: indicative ("Re-parents") here & below Done. https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:205: * @param {Array<string>} orphanIds The list of ids affected by the move. On 2016/06/29 20:16:51, michaelpg wrote: > nit: !Array<string> Done. https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:215: if (orphans.indexOf(o) == -1) On 2016/06/29 20:16:51, michaelpg wrote: > !orphans.includes(o) > > also, consider using a Set if order doesn't matter Done. (Order matters, see findChildren). https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:237: var layout = this.displayLayoutMap_.get(orphanId); On 2016/06/29 20:16:51, michaelpg wrote: > opt nit: var layout = assert(...); I'm not a fan of that pattern outside of tests. https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:254: // Move from the nearest corner to the desired locaiton and get the delta. On 2016/06/29 20:16:51, michaelpg wrote: > location Done. https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:643: * Return the position of the corner of the div closest to |pos|. On 2016/06/29 20:16:51, michaelpg wrote: > nits: Return => Returns, position => bounds, pos => somethingelse Rewrote.
https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/layout_behavior.js (right): https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:237: var layout = this.displayLayoutMap_.get(orphanId); On 2016/06/29 23:25:30, stevenjb wrote: > On 2016/06/29 20:16:51, michaelpg wrote: > > opt nit: var layout = assert(...); > I'm not a fan of that pattern outside of tests. why?
https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/layout_behavior.js (right): https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:237: var layout = this.displayLayoutMap_.get(orphanId); On 2016/06/29 23:35:10, Dan Beam wrote: > On 2016/06/29 23:25:30, stevenjb wrote: > > On 2016/06/29 20:16:51, michaelpg wrote: > > > opt nit: var layout = assert(...); > > I'm not a fan of that pattern outside of tests. > > why? In C++ I've seen bugs where someone puts code with side effects in a CHECK then later someone changes that toa DCHECK. I realize we don't have that issue with assert, but even so I avoid putting code with side effects inside asserts.
lgtm are there command line args for faking displays? are those documented somewhere, and if not, could you briefly list them in the bug? https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/layout_behavior.js (right): https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:237: var layout = this.displayLayoutMap_.get(orphanId); On 2016/06/29 23:38:33, stevenjb wrote: > On 2016/06/29 23:35:10, Dan Beam wrote: > > On 2016/06/29 23:25:30, stevenjb wrote: > > > On 2016/06/29 20:16:51, michaelpg wrote: > > > > opt nit: var layout = assert(...); > > > I'm not a fan of that pattern outside of tests. > > > > why? > > In C++ I've seen bugs where someone puts code with side effects in a CHECK then > later someone changes that toa DCHECK. I realize we don't have that issue with > assert, but even so I avoid putting code with side effects inside asserts. Acknowledged.
On 2016/07/02 00:53:27, michaelpg wrote: > lgtm > > are there command line args for faking displays? are those > documented somewhere, and if not, could you briefly list them > in the bug? > > https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/device_page/layout_behavior.js (right): > > https://codereview.chromium.org/2090953007/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/device_page/layout_behavior.js:237: var layout > = this.displayLayoutMap_.get(orphanId); > On 2016/06/29 23:38:33, stevenjb wrote: > > On 2016/06/29 23:35:10, Dan Beam wrote: > > > On 2016/06/29 23:25:30, stevenjb wrote: > > > > On 2016/06/29 20:16:51, michaelpg wrote: > > > > > opt nit: var layout = assert(...); > > > > I'm not a fan of that pattern outside of tests. > > > > > > why? > > > > In C++ I've seen bugs where someone puts code with side effects in a CHECK > then > > later someone changes that toa DCHECK. I realize we don't have that issue with > > assert, but even so I avoid putting code with side effects inside asserts. > > Acknowledged. Added the following to the issue: Note: to test these changes on linux, you can run: chrome --login-manager --ash-use-first-display-as-internal --ash-host-window-bounds=1600x800,800+0-800x600,1600+0-800x600,2400+0-800x600
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2090953007/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Display: Reparent children of dragged displays BUG=547080 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Display: Reparent children of dragged displays BUG=547080 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/30de137be92a0b6eb565fa1882fd6126ff98cc5b Cr-Commit-Position: refs/heads/master@{#404023} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/30de137be92a0b6eb565fa1882fd6126ff98cc5b Cr-Commit-Position: refs/heads/master@{#404023} |
