MD Settings: Display: Add mirroring and other fixes

MD Settings: Display: Add mirroring and other fixes

This adds proper mirroring support and includes some styling fixes.

BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3ac0d2920fd29ac0dbbc5cebdd59454eda58533c
Cr-Commit-Position: refs/heads/master@{#404883}

[stevenjb, 2016-07-07 20:51:49]

Description was changed from

==========
MD Settings: Display: Add mirroring and other fixes

This adds proper mirroring support and includes some styling fixes.

BUG=547080
==========

to

==========
MD Settings: Display: Add mirroring and other fixes

This adds proper mirroring support and includes some styling fixes.

BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2016-07-07 20:53:01]

stevenjb@chromium.org changed reviewers:
+ michaelpg@chromium.org

[michaelpg, 2016-07-11 13:37:55]

https://codereview.chromium.org/2128773004/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/display_layout.html (right):

https://codereview.chromium.org/2128773004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display_layout.html:20: in
getDivStyle_ and gitMirrorDivStyle_ */
git => get

https://codereview.chromium.org/2128773004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display_layout.html:54: <div
id="_mirror_[[item.id]]" class="display mirror"
do we consider these beginning underscores valid Polymer WebUI style?

https://codereview.chromium.org/2128773004/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/display_layout.js (right):

https://codereview.chromium.org/2128773004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display_layout.js:61: function
tryCalcVisualScale() {
var interval = setInterval(function() {
  if (self.calculateVisualScale_())
    clearInterval(interval);
}, retry);

https://codereview.chromium.org/2128773004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display_layout.js:168: var bounds
= this.getCalculatedDisplayBounds(id, false);
can you factor out the majority of this function and the above function? looks
like the only difference is the values of BORDER and OFFSET

[stevenjb, 2016-07-11 19:45:36]

https://codereview.chromium.org/2128773004/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/display_layout.html (right):

https://codereview.chromium.org/2128773004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display_layout.html:20: in
getDivStyle_ and gitMirrorDivStyle_ */
On 2016/07/11 13:37:55, michaelpg wrote:
> git => get

Done.

https://codereview.chromium.org/2128773004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display_layout.html:54: <div
id="_mirror_[[item.id]]" class="display mirror"
On 2016/07/11 13:37:55, michaelpg wrote:
> do we consider these beginning underscores valid Polymer WebUI style?

I used an initial _ to explicitly differentiate these "generated" ids from
conventional ones, for better or for worse. (I made the _mirror ids consistent
with id="_[[item.id]]" below). I also wanted to ensure the ids would not
duplicate some other id since we do not control the value of |item.id|, i.e. I
explicitly avoided the WebUI convention. That said, if this seems confusing I
could switch to something like id="[[itemid]]DisplayDiv" instead? (I would
probably wrap it in a helper function at that point).

https://codereview.chromium.org/2128773004/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/device_page/display_layout.js (right):

https://codereview.chromium.org/2128773004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display_layout.js:61: function
tryCalcVisualScale() {
On 2016/07/11 13:37:55, michaelpg wrote:
> var interval = setInterval(function() {
>   if (self.calculateVisualScale_())
>     clearInterval(interval);
> }, retry);

The problem with setInterval is that it delays before the first call and I want
the initial call to occur immediately.

https://codereview.chromium.org/2128773004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/device_page/display_layout.js:168: var bounds
= this.getCalculatedDisplayBounds(id, false);
On 2016/07/11 13:37:55, michaelpg wrote:
> can you factor out the majority of this function and the above function? looks
> like the only difference is the values of BORDER and OFFSET

Done.

[michaelpg, 2016-07-11 20:41:57]

lgtm

https://codereview.chromium.org/2128773004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2128773004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:193: * @param
{boolean=} opt_test
i'd prefer optional params default to false, e.g. opt_allowMissingBounds

if you insist on opt_test, comment that it asserts by default

[stevenjb, 2016-07-11 21:56:23]

While testing I found some issues with the drag behavior and mirroring so I also
cleaned that up.

PTAL

https://codereview.chromium.org/2128773004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2128773004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:193: * @param
{boolean=} opt_test
On 2016/07/11 20:41:56, michaelpg wrote:
> i'd prefer optional params default to false, e.g. opt_allowMissingBounds
> 
> if you insist on opt_test, comment that it asserts by default

I inverted the variable to be notest instead. We call this about 20 times so I
don't want to pass true explicitly in all of them.

[michaelpg, 2016-07-12 22:36:49]

https://codereview.chromium.org/2128773004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/drag_behavior.js (right):

https://codereview.chromium.org/2128773004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/drag_behavior.js:119:
window.removeEventListener('mouseup', this.endDragListener_);
do we risk retaining this listener on window if this.container_ is unset? then
we may call endDrag_ when this.dragEnabled is false

[stevenjb, 2016-07-12 22:41:32]

https://codereview.chromium.org/2128773004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/drag_behavior.js (right):

https://codereview.chromium.org/2128773004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/drag_behavior.js:119:
window.removeEventListener('mouseup', this.endDragListener_);
On 2016/07/12 22:36:48, michaelpg (slow) wrote:
> do we risk retaining this listener on window if this.container_ is unset? then
> we may call endDrag_ when this.dragEnabled is false

We shouldn't be unsetting this.container_ anywhere, unless I forgot+missed
someplace? If container_ is never set, addListeners_ will never set the
listener.

[michaelpg, 2016-07-12 22:43:12]

slgtm

https://codereview.chromium.org/2128773004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/drag_behavior.js (right):

https://codereview.chromium.org/2128773004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/drag_behavior.js:119:
window.removeEventListener('mouseup', this.endDragListener_);
On 2016/07/12 22:41:32, stevenjb wrote:
> On 2016/07/12 22:36:48, michaelpg (slow) wrote:
> > do we risk retaining this listener on window if this.container_ is unset?
then
> > we may call endDrag_ when this.dragEnabled is false
> 
> We shouldn't be unsetting this.container_ anywhere, unless I forgot+missed
> someplace? If container_ is never set, addListeners_ will never set the
> listener.

yeah, makes sense. it helps that it's called "initializeDrag".

[stevenjb, 2016-07-12 22:43:24]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-07-12 22:43:25]

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/2128773004/#ps60001
(title: "Fix drag + mirrored, cleanup, feedback")

[commit-bot: I haz the power, 2016-07-12 22:46:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-13 01:42:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-13 01:43:17]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 01:46:54]

Description was changed from

==========
MD Settings: Display: Add mirroring and other fixes

This adds proper mirroring support and includes some styling fixes.

BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Display: Add mirroring and other fixes

This adds proper mirroring support and includes some styling fixes.

BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3ac0d2920fd29ac0dbbc5cebdd59454eda58533c
Cr-Commit-Position: refs/heads/master@{#404883}
==========

[commit-bot: I haz the power, 2016-07-13 01:46:55]

Patchset 4 (id:??) landed as
https://crrev.com/3ac0d2920fd29ac0dbbc5cebdd59454eda58533c
Cr-Commit-Position: refs/heads/master@{#404883}