Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2522103002:
MD Settings: Display: Fix runtime error (Closed)
|
Created:
4 years, 1 month ago by stevenjb Modified:
4 years ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Display: Fix runtime error
Do not attempt to find a parent display when there is only one.
BUG=654582
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/7bd96109dbda7d002e40000915bdc673f0d7e868
Cr-Commit-Position: refs/heads/master@{#434247}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move test to display_layout.js #
Total comments: 6
Messages
Total messages: 18 (6 generated)
Description was changed from ========== MD Settings: Display: Fix runtime error Do not attempt to find a parent display when there is only one. BUG=654582 ========== to ========== MD Settings: Display: Fix runtime error Do not attempt to find a parent display when there is only one. BUG=654582 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2522103002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/layout_behavior.js (right): https://codereview.chromium.org/2522103002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/layout_behavior.js:84: updateDisplayBounds(id, newBounds) { I am confused by the mixing of ES6 and non-ES6 syntax for defining methods. |LayoutBehavior| is not a proper ES6 class. Can we simply be consistent with the rest of this file, and use ES6 method syntax only in proper ES6 classes? updateDisplayBounds: function(id, newBounds) { ... }, https://codereview.chromium.org/2522103002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/layout_behavior.js:86: if (this.displays.length < 2) { Where is |this.displays| defined? Did not find it in this file. Is it assuming that elements that implement the behavior have a |displays| member var? Can we infer that information without creating such implicit contracts between this behavior and the classes that inherit from it? Perhaps the following condition is equivalent? this.displayBoundsMap_.size < 2
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2522103002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/layout_behavior.js (right): https://codereview.chromium.org/2522103002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/layout_behavior.js:84: updateDisplayBounds(id, newBounds) { On 2016/11/23 01:48:51, dpapad wrote: > I am confused by the mixing of ES6 and non-ES6 syntax for defining methods. > |LayoutBehavior| is not a proper ES6 class. Can we simply be consistent with the > rest of this file, and use ES6 method syntax only in proper ES6 classes? > > updateDisplayBounds: function(id, newBounds) { > ... > }, Oversight on my part. I wish we could enforce this. Fixed. https://codereview.chromium.org/2522103002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/layout_behavior.js:86: if (this.displays.length < 2) { On 2016/11/23 01:48:51, dpapad wrote: > Where is |this.displays| defined? Did not find it in this file. Is it assuming > that elements that implement the behavior have a |displays| member var? Can we > infer that information without creating such implicit contracts between this > behavior and the classes that inherit from it? > > Perhaps the following condition is equivalent? > > this.displayBoundsMap_.size < 2 Ah, you're right, I should actually move this check up to display_layout.js (which calls updateDisplayBounds). We don't actually need to update dragBounds or dragLayoutId with just one display. Fixed, thanks.
LGTM https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/display_layout.js (right): https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/display_layout.js:227: div.style.left = '' + left + 'px'; Unrelated drive-by question: What is the point of using '' here? Why not just? div.style.left = left + 'px'; Also this file is not consistent with itself, since it makes use of ES6 template literals like `${left}px` in some places, but not here. https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/layout_behavior.js (right): https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:88: var closestId = this.findClosest_(id, newBounds); Nit (optional): var closestId = assert(this.findClosest_(id, newBounds));
https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/display_layout.js (right): https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/display_layout.js:227: div.style.left = '' + left + 'px'; On 2016/11/23 18:18:43, dpapad wrote: > Unrelated drive-by question: What is the point of using '' here? Why not just? > div.style.left = left + 'px'; > > Also this file is not consistent with itself, since it makes use of ES6 template > literals like `${left}px` in some places, but not here. Some of this code is new, some of it was copy/pasted from options. I expect the '' + ... is a C++ ish thing where string << number can be a valid string concatenation but number << string is not. When I am in here next I'll try to remember to do a pass to make everything use ES6 template literals. https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/layout_behavior.js (right): https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:88: var closestId = this.findClosest_(id, newBounds); On 2016/11/23 18:18:43, dpapad wrote: > Nit (optional): > var closestId = assert(this.findClosest_(id, newBounds)); Personal nit: I'm not a huge fan of wrapping essential logic inside of an assert outside of tests. Partly because in C++ if you accidentally do that with a DCHECK the logic is never run in production code, but also because I find it harder to read. We rarely do that in Chrome C++ code, although I realize this may be a common pattern in JS.
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/display_layout.js (right): https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/display_layout.js:227: div.style.left = '' + left + 'px'; On 2016/11/23 at 18:44:32, stevenjb wrote: > On 2016/11/23 18:18:43, dpapad wrote: > > Unrelated drive-by question: What is the point of using '' here? Why not just? > > div.style.left = left + 'px'; > > > > Also this file is not consistent with itself, since it makes use of ES6 template > > literals like `${left}px` in some places, but not here. > > Some of this code is new, some of it was copy/pasted from options. > > I expect the '' + ... is a C++ ish thing where string << number can be a valid string concatenation but number << string is not. > > When I am in here next I'll try to remember to do a pass to make everything use ES6 template literals. I suggest holding off ES6-ifying this file, because we actually might need to go the opposite direction for the purposes of vulcaniz-ation. There are known issues, for example template string literals `${foo}` are not handled yet by vulcanize.
Removing string literals shouldn't be a big deal, so please file an issue and I can strip those out if we need to. Other ES6 things in common use like 'let' would be more concerning. Since none of the cros specific UI will ever be shown on iOS, we have been pretty liberal with using ES6 elements that Chrome desktop supports. On Wed, Nov 23, 2016 at 11:51 AM, dpapad@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > > https://codereview.chromium.org/2522103002/diff/40001/ > chrome/browser/resources/settings/device_page/display_layout.js > File chrome/browser/resources/settings/device_page/display_layout.js > (right): > > https://codereview.chromium.org/2522103002/diff/40001/ > chrome/browser/resources/settings/device_page/display_layout.js#newcode227 > chrome/browser/resources/settings/device_page/display_layout.js:227: > div.style.left = '' + left + 'px'; > On 2016/11/23 at 18:44:32, stevenjb wrote: > > On 2016/11/23 18:18:43, dpapad wrote: > > > Unrelated drive-by question: What is the point of using '' here? Why > not just? > > > div.style.left = left + 'px'; > > > > > > Also this file is not consistent with itself, since it makes use of > ES6 template > > > literals like `${left}px` in some places, but not here. > > > > Some of this code is new, some of it was copy/pasted from options. > > > > I expect the '' + ... is a C++ ish thing where string << number can be > a valid string concatenation but number << string is not. > > > > When I am in here next I'll try to remember to do a pass to make > everything use ES6 template literals. > > I suggest holding off ES6-ifying this file, because we actually might > need to go the opposite direction for the purposes of vulcaniz-ation. > There are known issues, for example template string literals `${foo}` > are not handled yet by vulcanize. > > https://codereview.chromium.org/2522103002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/layout_behavior.js (right): https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/layout_behavior.js:88: var closestId = this.findClosest_(id, newBounds); On 2016/11/23 18:44:32, stevenjb wrote: > On 2016/11/23 18:18:43, dpapad wrote: > > Nit (optional): > > var closestId = assert(this.findClosest_(id, newBounds)); > > Personal nit: I'm not a huge fan of wrapping essential logic inside of an assert > outside of tests. Partly because in C++ if you accidentally do that with a > DCHECK the logic is never run in production code, but also because I find it > harder to read. We rarely do that in Chrome C++ code, although I realize this > may be a common pattern in JS. JS != C++
On 2016/11/23 20:46:56, Dan Beam wrote: > https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/device_page/layout_behavior.js (right): > > https://codereview.chromium.org/2522103002/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/device_page/layout_behavior.js:88: var > closestId = this.findClosest_(id, newBounds); > On 2016/11/23 18:44:32, stevenjb wrote: > > On 2016/11/23 18:18:43, dpapad wrote: > > > Nit (optional): > > > var closestId = assert(this.findClosest_(id, newBounds)); > > > > Personal nit: I'm not a huge fan of wrapping essential logic inside of an > assert > > outside of tests. Partly because in C++ if you accidentally do that with a > > DCHECK the logic is never run in production code, but also because I find it > > harder to read. We rarely do that in Chrome C++ code, although I realize this > > may be a common pattern in JS. > > JS != C++ Really? :P
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479926677705880,
"parent_rev": "bb10e3a10748faa6084b34033889ae83abd3900d", "commit_rev":
"4365c1bf915cac406537e6a0494891f8b9dd704e"}
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Display: Fix runtime error Do not attempt to find a parent display when there is only one. BUG=654582 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Display: Fix runtime error Do not attempt to find a parent display when there is only one. BUG=654582 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/7bd96109dbda7d002e40000915bdc673f0d7e868 Cr-Commit-Position: refs/heads/master@{#434247} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7bd96109dbda7d002e40000915bdc673f0d7e868 Cr-Commit-Position: refs/heads/master@{#434247} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
