MD Settings: Add display layout
This correctly sizes and positions displays, but does not support
dragging yet.
BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
For trivial change to display_info_provider_chromeos.cc:
TBR=asargent@chromium.org
Committed: https://crrev.com/587e5f5dd32f3ce79ff084e60ff6c0eaebbfec3c
Cr-Commit-Position: refs/heads/master@{#401989}
Description was changed from ========== MD Settings: Add display layout This correctly sizes and positions ...
4 years, 6 months ago
(2016-06-20 21:26:18 UTC)
#1
Description was changed from
==========
MD Settings: Add display layout
This correctly sizes and positions displays, but does not support
dragging yet.
BUG=547080
==========
to
==========
MD Settings: Add display layout
This correctly sizes and positions displays, but does not support
dragging yet.
BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========
4 years, 6 months ago
(2016-06-20 23:32:30 UTC)
#3
stevenjb
Description was changed from ========== MD Settings: Add display layout This correctly sizes and positions ...
4 years, 6 months ago
(2016-06-21 19:12:34 UTC)
#4
Description was changed from
==========
MD Settings: Add display layout
This correctly sizes and positions displays, but does not support
dragging yet.
BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========
to
==========
MD Settings: Add display layout
This correctly sizes and positions displays, but does not support
dragging yet.
BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
For trivial change to display_info_provider_chromeos.cc:
TBR=asargent@chromium.org
==========
stevenjb
The CQ bit was checked by stevenjb@chromium.org to run a CQ dry run
4 years, 6 months ago
(2016-06-21 19:12:53 UTC)
#5
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/204327)
4 years, 6 months ago
(2016-06-21 19:21:53 UTC)
#8
https://codereview.chromium.org/2084673003/diff/60001/chrome/browser/resources/settings/device_page/display_layout.html File chrome/browser/resources/settings/device_page/display_layout.html (right): https://codereview.chromium.org/2084673003/diff/60001/chrome/browser/resources/settings/device_page/display_layout.html#newcode3 chrome/browser/resources/settings/device_page/display_layout.html:3: <!-- DO NOT SUBMIT: Fix Page Title. Missing tags? ...
4 years, 6 months ago
(2016-06-23 19:11:02 UTC)
#13
https://codereview.chromium.org/2084673003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display_layout.html (right):
https://codereview.chromium.org/2084673003/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.html:3: <!-- DO NOT
SUBMIT: Fix Page Title. Missing tags? http://go/optional-html -->
On 2016/06/23 04:29:34, michaelpg wrote:
> On 2016/06/22 23:38:52, stevenjb wrote:
> > On 2016/06/22 21:57:57, michaelpg wrote:
> > > ?
> >
> > Sigh. Stupid editor. One of these days I will figure out how this happens.
> > Removed.
>
> Maybe Google's emacs config triggers this?
>
> https://goto.google.com/yecgl
Good theory but I can't find anywhere in emacs that directly triggers 'genhtml'.
Sigh.
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display_layout.html (right):
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.html:9: /* Use
relative position with no offset so that the diplay divs (children),
On 2016/06/23 04:29:34, michaelpg wrote:
> all indent inside <style> is off by one
Done.
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.html:9: /* Use
relative position with no offset so that the diplay divs (children),
On 2016/06/23 04:29:34, michaelpg wrote:
> display
Done.
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.html:31: <div
id="displayArea" on-iron-resize="calculateDisplayArea_">
On 2016/06/23 04:29:34, michaelpg wrote:
> calculateVisualScale_? (this should show a console warning btw)
Doh. Didn't test resize.
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display_layout.js (right):
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.js:104: {left:
Number.MAX_VALUE, right: 0, top: Number.MAX_VALUE, bottom: 0};
On 2016/06/23 04:29:35, michaelpg wrote:
> Normally the origin (0, 0) is the top-left, and the bottom is greater than the
> top.
The intention is that these will be immediately overwritten. I will initialize
this to the first display instead to make this more clear.
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.js:121: // largest
display.
On 2016/06/23 04:29:34, michaelpg wrote:
> display(s) -- e.g., a landscape display and a portrait display could give you
> maxWidth and maxHeight from different displays
>
> (so the code is right, but the comment isn't quite)
Done.
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.js:151: /** @const
{number} */ var BORDER = 2;
On 2016/06/23 04:29:35, michaelpg wrote:
> why 2, when the border with on .display is 1?
It has a box shadow of width 2. Added a comment.
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.js:153: var height
= Math.round(bounds.height * this.visualScale) - BORDER * 2;
On 2016/06/23 04:29:35, michaelpg wrote:
> I think using box-sizing: border-box on these divs can remove the need to
> account for the border manually.
Alas, your newfangled CSS property doesn't appear to play nicely with
box-shadow. Good thought though.
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.js:156:
this.visualOffset_.left + Math.round(bounds.left * this.visualScale);
On 2016/06/23 04:29:34, michaelpg wrote:
> why round the 2nd term but not the offset?
Good catch. Fixed.
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.js:159: var style =
'height:' + height + 'px;' + 'width:' + width + 'px;' +
On 2016/06/23 04:29:35, michaelpg wrote:
> i think this is a great use case for es6 template strings if closure will
allow
> it:
>
> return `height: ${height}px; width: ${width}px; ...`;
Cute. Closure is fine with it. Done.
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.js:175: * @param
{!{model: !{index: number}, target: !PaperButtonElement}} e
On 2016/06/23 04:29:35, michaelpg wrote:
> meh, remove unused target property info
Done.
https://codereview.chromium.org/2084673003/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/display_layout.js (right):
https://codereview.chromium.org/2084673003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/display_layout.js:204: var
parentDisplay = this.displayMap_[layout.parentId];
On 2016/06/23 04:29:35, michaelpg wrote:
> layout.parentId can be '', making parentDisplay undefined, making line 206
> throw. Unless (layout.parentId == '') always implies (display.isPrimary) but
it
> doesn't sound like the primary display always has to be the root.
>
> If I'm right, add a test case :-)
The primary display does have to be the root, and there is a test case :) (I had
to fix the fake model to avoid just that problem).
I'll add an assert to document that.
https://codereview.chromium.org/2084673003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/display_layout.js:206: if
(parentDisplay.id in this.boundsMap_)
On 2016/06/23 04:29:35, michaelpg wrote:
> it's a little weird that calcDisplayBounds_ both returns and caches. Maybe
don't
> return the result, and do:
>
> if (!(parentDisplay.id in this.boundsMap_))
> this.calcDisplayBounds_(parentDisplay);
> var parentBounds = this.boundsmap_[parentDisplay.id];
>
> which IMO is simpler all around.
Meh. Done.
https://codereview.chromium.org/2084673003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/display_layout.js:209:
parentBounds = this.calcDisplayBounds_(parentDisplay);
On 2016/06/23 04:29:35, michaelpg wrote:
> Does this mean calcDisplayBounds_ could be called twice for a parent -- once
> here, then later, once in the remainder of the updateDisplays loop? I think
yes,
> unless the display list is ordered like a tree (but then this else condition
> would never occur)
Err, yes. I swear I checked the cache at the top of this function at one point,
must have gotten lost. Added.
michaelpg
lgtm, but I do recommend using reduce() https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resources/settings/device_page/display_layout.js File chrome/browser/resources/settings/device_page/display_layout.js (right): https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resources/settings/device_page/display_layout.js#newcode104 chrome/browser/resources/settings/device_page/display_layout.js:104: {left: Number.MAX_VALUE, ...
4 years, 6 months ago
(2016-06-24 20:25:11 UTC)
#14
lgtm, but I do recommend using reduce()
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display_layout.js (right):
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.js:104: {left:
Number.MAX_VALUE, right: 0, top: Number.MAX_VALUE, bottom: 0};
On 2016/06/23 19:11:01, stevenjb wrote:
> On 2016/06/23 04:29:35, michaelpg wrote:
> > Normally the origin (0, 0) is the top-left, and the bottom is greater than
the
> > top.
>
> The intention is that these will be immediately overwritten. I will initialize
> this to the first display instead to make this more clear.
If you use Array.prototype.reduce you wouldn't have to explicitly initialize
these (that's how blink unions all the rects in GetBoundingClientRect, btw)
https://codereview.chromium.org/2084673003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/display_layout.html (right):
https://codereview.chromium.org/2084673003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/display_layout.html:9: /* Use
relative position with no offset so that display divs(children),
nit: space before (children)
4 years, 6 months ago
(2016-06-24 20:59:51 UTC)
#15
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display_layout.js (right):
https://codereview.chromium.org/2084673003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.js:104: {left:
Number.MAX_VALUE, right: 0, top: Number.MAX_VALUE, bottom: 0};
On 2016/06/24 20:25:10, michaelpg wrote:
> On 2016/06/23 19:11:01, stevenjb wrote:
> > On 2016/06/23 04:29:35, michaelpg wrote:
> > > Normally the origin (0, 0) is the top-left, and the bottom is greater than
> the
> > > top.
> >
> > The intention is that these will be immediately overwritten. I will
initialize
> > this to the first display instead to make this more clear.
>
> If you use Array.prototype.reduce you wouldn't have to explicitly initialize
> these (that's how blink unions all the rects in GetBoundingClientRect, btw)
Interesting. I can see how that would be useful, but here I am also:
* converting left/right/width/height to left/right/top/bottom
* calculating the max width and height of any individual bounds
I could do it in a couple of reduce() functions but they would be not trivial
and I think ultimately a bit harder to read than what I ended up with.
https://codereview.chromium.org/2084673003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/display_layout.html (right):
https://codereview.chromium.org/2084673003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/display_layout.html:9: /* Use
relative position with no offset so that display divs(children),
On 2016/06/24 20:25:10, michaelpg wrote:
> nit: space before (children)
Done.
stevenjb
The CQ bit was checked by stevenjb@chromium.org
4 years, 6 months ago
(2016-06-24 20:59:58 UTC)
#16
Description was changed from ========== MD Settings: Add display layout This correctly sizes and positions ...
4 years, 6 months ago
(2016-06-24 22:00:24 UTC)
#19
Message was sent while issue was closed.
Description was changed from
==========
MD Settings: Add display layout
This correctly sizes and positions displays, but does not support
dragging yet.
BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
For trivial change to display_info_provider_chromeos.cc:
TBR=asargent@chromium.org
==========
to
==========
MD Settings: Add display layout
This correctly sizes and positions displays, but does not support
dragging yet.
BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
For trivial change to display_info_provider_chromeos.cc:
TBR=asargent@chromium.org
==========
commit-bot: I haz the power
Committed patchset #8 (id:140001)
4 years, 6 months ago
(2016-06-24 22:00:27 UTC)
#20
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
commit-bot: I haz the power
Description was changed from ========== MD Settings: Add display layout This correctly sizes and positions ...
4 years, 6 months ago
(2016-06-24 22:03:46 UTC)
#21
Message was sent while issue was closed.
Description was changed from
==========
MD Settings: Add display layout
This correctly sizes and positions displays, but does not support
dragging yet.
BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
For trivial change to display_info_provider_chromeos.cc:
TBR=asargent@chromium.org
==========
to
==========
MD Settings: Add display layout
This correctly sizes and positions displays, but does not support
dragging yet.
BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
For trivial change to display_info_provider_chromeos.cc:
TBR=asargent@chromium.org
Committed: https://crrev.com/587e5f5dd32f3ce79ff084e60ff6c0eaebbfec3c
Cr-Commit-Position: refs/heads/master@{#401989}
==========
commit-bot: I haz the power
Patchset 8 (id:??) landed as https://crrev.com/587e5f5dd32f3ce79ff084e60ff6c0eaebbfec3c Cr-Commit-Position: refs/heads/master@{#401989}
4 years, 6 months ago
(2016-06-24 22:03:48 UTC)
#22
Issue 2084673003: MD Settings: Add display layout
(Closed)
Created 4 years, 6 months ago by stevenjb
Modified 4 years, 6 months ago
Reviewers: michaelpg
Base URL: https://chromium.googlesource.com/chromium/src.git@issue_547080_display_settings6
Comments: 50