Make lists in the passwords section aware of the scroll target.

Make lists in the passwords section aware of the scroll target.

BUG=654738
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b89d026ba8fbda3d8372bac6838a9cac6fa9f13b
Cr-Commit-Position: refs/heads/master@{#427079}

[hcarmona, 2016-04-22 19:13:27]

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

[hcarmona, 2016-04-22 19:13:28]

PTAL and give some feedback.

This is much faster. Load time for the list of saved passwords went from around
10 seconds down to around 1 for 606 passwords. (inaccurate stopwatch
measurement)

But there's a weird visual artifact that all the passwords are squished
together, then expand to fill the page...

[michaelpg, 2016-04-25 07:06:49]

On 2016/04/22 19:13:28, Hector Carmona wrote:
> PTAL and give some feedback.
> 
> This is much faster. Load time for the list of saved passwords went from
around
> 10 seconds down to around 1 for 606 passwords. (inaccurate stopwatch
> measurement)
> 
> But there's a weird visual artifact that all the passwords are squished
> together, then expand to fill the page...

i'd rather look at this at my desktop -- since i'm in MTV tomorrow, i'll look
into it Tuesday when i'm back in SFO

[hcarmona, 2016-05-31 20:52:45]

Description was changed from

==========
Make lists in the passwords section aware of the scroll target.

BUG=546836
==========

to

==========
Make lists in the passwords section aware of the scroll target.

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

[hcarmona, 2016-05-31 20:54:46]

Patchset #2 (id:20001) has been deleted

[michaelpg, 2016-06-14 01:02:34]

So when we discussed this offline, it turned out that using iron-list in the
full page and setting the scroll target doesn't actually result in virtualizing
the items -- they're all rendered. Has anyone made progress here? It's something
we need to address for other sections (e.g. Languages).

[hcarmona, 2016-07-22 21:06:35]

Description was changed from

==========
Make lists in the passwords section aware of the scroll target.

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

to

==========
Make lists in the passwords section aware of the scroll target.

BUG=546836
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

[hcarmona, 2016-07-22 21:06:38]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

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

Dry run: 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-22 21:07:13]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-22 21:09:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 21:09:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[hcarmona, 2016-10-12 14:20:32]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 14:20:44]

Dry run: 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-10-12 14:20:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 14:20:46]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[hcarmona, 2016-10-12 14:21:57]

Description was changed from

==========
Make lists in the passwords section aware of the scroll target.

BUG=546836
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Make lists in the passwords section aware of the scroll target.

BUG=546836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[hcarmona, 2016-10-12 14:22:07]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 14:22:15]

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

[hcarmona, 2016-10-12 14:25:26]

Description was changed from

==========
Make lists in the passwords section aware of the scroll target.

BUG=546836
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Make lists in the passwords section aware of the scroll target.

BUG=654738
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[hcarmona, 2016-10-12 14:28:07]

I've updated this since we last talked about this (forever ago).

No more defaults, and I found the correct scroll target.

This is now using virtual scrolling like we expect.

Also updated the issue.

[commit-bot: I haz the power, 2016-10-12 14:31:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 14:31:18]

Dry run: Exceeded global retry quota

[michaelpg, 2016-10-17 18:59:54]

is a data-bound Polymer variable the best approach? a settings-global variable
would probably be simpler, and the scroll target *is* a global, constant
element.

https://codereview.chromium.org/1914653002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/advanced_page/advanced_page.js (right):

https://codereview.chromium.org/1914653002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/advanced_page/advanced_page.js:24: * @type
{Element}
HTMLElement?

https://codereview.chromium.org/1914653002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/advanced_page/advanced_page.js:26:
scrollTarget: HTMLElement,
shouldn't this just be Object?

[hcarmona, 2016-10-18 19:33:19]

PTAL, I've updated this to have a global scroll target.

https://codereview.chromium.org/1914653002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/advanced_page/advanced_page.js (right):

https://codereview.chromium.org/1914653002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/advanced_page/advanced_page.js:24: * @type
{Element}
On 2016/10/17 18:59:54, michaelpg wrote:
> HTMLElement?

Done.

https://codereview.chromium.org/1914653002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/advanced_page/advanced_page.js:26:
scrollTarget: HTMLElement,
On 2016/10/17 18:59:54, michaelpg wrote:
> shouldn't this just be Object?

Done.

[hcarmona, 2016-10-18 20:42:36]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 20:43:29]

Dry run: 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-10-18 21:51:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 21:51:07]

Dry run: Exceeded global retry quota

[michaelpg, 2016-10-18 21:58:00]

sorry for the wall of text

https://codereview.chromium.org/1914653002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/global_scroll_target_behavior.js (right):

https://codereview.chromium.org/1914653002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/global_scroll_target_behavior.js:33:
globalData_: Object,
i'd suggest:

globalData_: {
  type: Object,
  value: global.settingsScrollTargetData,
}

https://codereview.chromium.org/1914653002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/global_scroll_target_behavior.js:43:
this.globalData_.scrollTarget = value;
This doesn't actually propagate. It may work, but don't do it.

Yes, all elements with this behavior, have a |this.globalData_| that references
the same object. The value will always be correct once it's set.

But: How does one element "know" when this.globalData_ has been changed by
another element? Even if we did:

  this.set('globalData_.scrollTarget', value);

Polymer is only notified about *one* path change, for that *one* element, and
recomputes scrollTarget for that one element. I believe for all other elements,
scrollTarget would not be recomputed.

(By the same logic, updates to scrollTarget are not magically propagated
either.)

So even though the shared object has changed, whatever uses that object (e.g.
iron-list) isn't notified of the change, so their observers won't fire, the list
won't resize, etc etc.

*If* we call setGlobalScrollTarget() on the first instantiated element with this
behavior, all other elements will read that value at creation time, so all is
well. *If* we never call the function again, so scrollTarget never changes,
everything will appear to work.

The first assumption could break depending on initialization order; if we load a
subpage at startup it would only find the empty object. The second, IDK, maybe
would break tests.

Possible fix:
  - call setGlobalScrollTarget in settings-ui ready()
  - make this.scrollTarget readOnly, and not computed, just undefined
  - in this behavior's attached(), do a setTimeout callback which sets the
readOnly property:
      // Once initialization is finished, settings-ui will have set the scroll
target.
      this._setScrollTarget(assert(this.globalData_.scrollTarget))

In that fix, you could replace this.globalData_.scrollTarget with a simple
variable you set here:
  (function() {
    var scrollTarget;
    var MyBehavior = {
      // the scrollTarget variable exists in the closure now
    };
   })();

If we could come up with a non-async way, that would be cool, but I'm not sure
how.

[hcarmona, 2016-10-19 18:15:29]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 18:15:46]

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

[hcarmona, 2016-10-19 18:53:34]

https://codereview.chromium.org/1914653002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/global_scroll_target_behavior.js (right):

https://codereview.chromium.org/1914653002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/global_scroll_target_behavior.js:33:
globalData_: Object,
On 2016/10/18 21:58:00, michaelpg wrote:
> i'd suggest:
> 
> globalData_: {
>   type: Object,
>   value: global.settingsScrollTargetData,
> }

Acknowledged.

https://codereview.chromium.org/1914653002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/global_scroll_target_behavior.js:43:
this.globalData_.scrollTarget = value;
------------------------------------------------------------------------

On 2016/10/18 21:58:00, michaelpg wrote:
> This doesn't actually propagate. It may work, but don't do it.
>
> Yes, all elements with this behavior, have a |this.globalData_| that
references
> the same object. The value will always be correct once it's set.
> 
> But: How does one element "know" when this.globalData_ has been changed by
> another element? Even if we did:
> 
>   this.set('globalData_.scrollTarget', value);
> 
> Polymer is only notified about *one* path change, for that *one* element, and
> recomputes scrollTarget for that one element. I believe for all other
elements,
> scrollTarget would not be recomputed.
> 
> (By the same logic, updates to scrollTarget are not magically propagated
> either.)
> 
> So even though the shared object has changed, whatever uses that object (e.g.
> iron-list) isn't notified of the change, so their observers won't fire, the
list
> won't resize, etc etc.
>

Arg! Ok, didn't realize that this was working by coincidence instead of
by design. Thanks for the thorough explanation.

> *If* we call setGlobalScrollTarget() on the first instantiated element with
this
> behavior, all other elements will read that value at creation time, so all is
> well. *If* we never call the function again, so scrollTarget never changes,
> everything will appear to work.
> 

Is there a reason we would want to be able to change the scroll target
after it's been set?

> The first assumption could break depending on initialization order; if we load
a
> subpage at startup it would only find the empty object. The second, IDK, maybe
> would break tests.
>

Agreed, having our code depend on the order of things is bad.

> Possible fix:
>   - call setGlobalScrollTarget in settings-ui ready()

I had issues when I was first trying this out and set it in |ready|.

>   - make this.scrollTarget readOnly, and not computed, just undefined

Done.

>   - in this behavior's attached(), do a setTimeout callback which sets the
> readOnly property:

Not a fan of timeouts, they can be flaky. How does a promise look?

>       // Once initialization is finished, settings-ui will have set the scroll
> target.
>       this._setScrollTarget(assert(this.globalData_.scrollTarget))
> 
> In that fix, you could replace this.globalData_.scrollTarget with a simple
> variable you set here:
>   (function() {
>     var scrollTarget;
>     var MyBehavior = {
>       // the scrollTarget variable exists in the closure now
>     };
>    })();
>

Wrapping this in a function didn't work for me. I get MyBehavior is
undefined when trying to use it. :-/

> If we could come up with a non-async way, that would be cool, but I'm not sure
> how.

[michaelpg, 2016-10-19 19:50:09]

https://codereview.chromium.org/1914653002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/global_scroll_target_behavior.js (right):

https://codereview.chromium.org/1914653002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/global_scroll_target_behavior.js:43:
this.globalData_.scrollTarget = value;
On 2016/10/19 18:53:34, Hector Carmona wrote:
> ------------------------------------------------------------------------
> 
> On 2016/10/18 21:58:00, michaelpg wrote:
> > This doesn't actually propagate. It may work, but don't do it.
> >
> > Yes, all elements with this behavior, have a |this.globalData_| that
> references
> > the same object. The value will always be correct once it's set.
> > 
> > But: How does one element "know" when this.globalData_ has been changed by
> > another element? Even if we did:
> > 
> >   this.set('globalData_.scrollTarget', value);
> > 
> > Polymer is only notified about *one* path change, for that *one* element,
and
> > recomputes scrollTarget for that one element. I believe for all other
> elements,
> > scrollTarget would not be recomputed.
> > 
> > (By the same logic, updates to scrollTarget are not magically propagated
> > either.)
> > 
> > So even though the shared object has changed, whatever uses that object
(e.g.
> > iron-list) isn't notified of the change, so their observers won't fire, the
> list
> > won't resize, etc etc.
> >
> 
> Arg! Ok, didn't realize that this was working by coincidence instead of
> by design. Thanks for the thorough explanation.
> 
> > *If* we call setGlobalScrollTarget() on the first instantiated element with
> this
> > behavior, all other elements will read that value at creation time, so all
is
> > well. *If* we never call the function again, so scrollTarget never changes,
> > everything will appear to work.
> > 
> 
> Is there a reason we would want to be able to change the scroll target
> after it's been set?
> 

No, but we should make that assumption explicit (which the new patch does)

> > The first assumption could break depending on initialization order; if we
load
> a
> > subpage at startup it would only find the empty object. The second, IDK,
maybe
> > would break tests.
> >
> 
> Agreed, having our code depend on the order of things is bad.
> 
> > Possible fix:
> >   - call setGlobalScrollTarget in settings-ui ready()
> 
> I had issues when I was first trying this out and set it in |ready|.
> 
> >   - make this.scrollTarget readOnly, and not computed, just undefined
> 
> Done.
> 
> >   - in this behavior's attached(), do a setTimeout callback which sets the
> > readOnly property:
> 
> Not a fan of timeouts, they can be flaky. How does a promise look?

Cool, that probably would make testing easier too.

> 
> >       // Once initialization is finished, settings-ui will have set the
scroll
> > target.
> >       this._setScrollTarget(assert(this.globalData_.scrollTarget))
> > 
> > In that fix, you could replace this.globalData_.scrollTarget with a simple
> > variable you set here:
> >   (function() {
> >     var scrollTarget;
> >     var MyBehavior = {
> >       // the scrollTarget variable exists in the closure now
> >     };
> >    })();
> >
> 
> Wrapping this in a function didn't work for me. I get MyBehavior is
> undefined when trying to use it. :-/

Yeah, my bad, we have to export MyBehavior. cr.define would work. With
a closure, settings-ui doesn't even need to have the behavior -- just
expose the function it needs to call. Would make the behavior simpler.
rough sketch:

cr.define('settings', function() {
  var scrollTargetResolver = new PromiseResolver();
  var GlobalScrollTargetBehavior = {
    properties: /* same as patch... */

    attached: function() {
      scrollTargetResolver.promise.then(
        this._setScrollTarget.bind(this));  // shortcut if you like
    },
  };
  var setGlobalScrollTarget = function(scrollTarget) {
    scrollTargetResolver.resolve(scrollTarget);
  };
  return {
    GlobalScrollTargetBehavior: GlobalScrollTargetBehavior,
    setGlobalScrollTarget: setGlobalScrollTarget,
  };
});

> 
> > If we could come up with a non-async way, that would be cool, but I'm not
sure
> > how.

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/global_scroll_target_behavior.js (right):

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/global_scroll_target_behavior.js:7: * of a
global scroll target.
I'd add a little more instruction here, e.g. that the effect is elements w/ this
behavior get a |scrollTarget| property which is set just after initialization.

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/global_scroll_target_behavior.js:17:
global.settingsScrollTargetData = {};
where does |global| actually come from? dunno why we use both global and window
for the same thing.

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/global_scroll_target_behavior.js:19:
global.settingsScrollTargetData.targetPromise = new Promise(function(accept) {
Would our PromiseResolver be more convenient?

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/global_scroll_target_behavior.js:32: notify:
true,
Shouldn't need notify: true; as we're not 2-way binding (setting passwords
section's scrollTarget shouldn't propagate *up* to settings-ui)

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/global_scroll_target_behavior.js:38: *
Reference to the global data. This allows Polymer style data binding.
comment outdated

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/global_scroll_target_behavior.js:40: *  
targetPromise: (Promise<HTMLElement>|undefined),
make a typedef instead of copying/pasting

[commit-bot: I haz the power, 2016-10-19 20:13:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 20:13:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hcarmona, 2016-10-19 21:55:53]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 21:56:37]

Dry run: 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-10-19 22:00:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 22:00:08]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hcarmona, 2016-10-20 14:24:14]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 14:24:36]

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

[hcarmona, 2016-10-20 14:27:44]

https://codereview.chromium.org/1914653002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/global_scroll_target_behavior.js (right):

https://codereview.chromium.org/1914653002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/global_scroll_target_behavior.js:43:
this.globalData_.scrollTarget = value;
On 2016/10/19 19:50:08, michaelpg wrote:
> On 2016/10/19 18:53:34, Hector Carmona wrote:
> > ------------------------------------------------------------------------
> > 
> > On 2016/10/18 21:58:00, michaelpg wrote:
> > > This doesn't actually propagate. It may work, but don't do it.
> > >
> > > Yes, all elements with this behavior, have a |this.globalData_| that
> > references
> > > the same object. The value will always be correct once it's set.
> > > 
> > > But: How does one element "know" when this.globalData_ has been changed by
> > > another element? Even if we did:
> > > 
> > >   this.set('globalData_.scrollTarget', value);
> > > 
> > > Polymer is only notified about *one* path change, for that *one* element,
> and
> > > recomputes scrollTarget for that one element. I believe for all other
> > elements,
> > > scrollTarget would not be recomputed.
> > > 
> > > (By the same logic, updates to scrollTarget are not magically propagated
> > > either.)
> > > 
> > > So even though the shared object has changed, whatever uses that object
> (e.g.
> > > iron-list) isn't notified of the change, so their observers won't fire,
the
> > list
> > > won't resize, etc etc.
> > >
> > 
> > Arg! Ok, didn't realize that this was working by coincidence instead of
> > by design. Thanks for the thorough explanation.
> > 
> > > *If* we call setGlobalScrollTarget() on the first instantiated element
with
> > this
> > > behavior, all other elements will read that value at creation time, so all
> is
> > > well. *If* we never call the function again, so scrollTarget never
changes,
> > > everything will appear to work.
> > > 
> > 
> > Is there a reason we would want to be able to change the scroll target
> > after it's been set?
> > 
> 
> No, but we should make that assumption explicit (which the new patch does)
> 
> > > The first assumption could break depending on initialization order; if we
> load
> > a
> > > subpage at startup it would only find the empty object. The second, IDK,
> maybe
> > > would break tests.
> > >
> > 
> > Agreed, having our code depend on the order of things is bad.
> > 
> > > Possible fix:
> > >   - call setGlobalScrollTarget in settings-ui ready()
> > 
> > I had issues when I was first trying this out and set it in |ready|.
> > 
> > >   - make this.scrollTarget readOnly, and not computed, just undefined
> > 
> > Done.
> > 
> > >   - in this behavior's attached(), do a setTimeout callback which sets the
> > > readOnly property:
> > 
> > Not a fan of timeouts, they can be flaky. How does a promise look?
> 
> Cool, that probably would make testing easier too.
> 
> > 
> > >       // Once initialization is finished, settings-ui will have set the
> scroll
> > > target.
> > >       this._setScrollTarget(assert(this.globalData_.scrollTarget))
> > > 
> > > In that fix, you could replace this.globalData_.scrollTarget with a simple
> > > variable you set here:
> > >   (function() {
> > >     var scrollTarget;
> > >     var MyBehavior = {
> > >       // the scrollTarget variable exists in the closure now
> > >     };
> > >    })();
> > >
> > 
> > Wrapping this in a function didn't work for me. I get MyBehavior is
> > undefined when trying to use it. :-/
> 
> Yeah, my bad, we have to export MyBehavior. cr.define would work. With
> a closure, settings-ui doesn't even need to have the behavior -- just
> expose the function it needs to call. Would make the behavior simpler.
> rough sketch:
> 
> cr.define('settings', function() {
>   var scrollTargetResolver = new PromiseResolver();
>   var GlobalScrollTargetBehavior = {
>     properties: /* same as patch... */
> 
>     attached: function() {
>       scrollTargetResolver.promise.then(
>         this._setScrollTarget.bind(this));  // shortcut if you like
>     },
>   };
>   var setGlobalScrollTarget = function(scrollTarget) {
>     scrollTargetResolver.resolve(scrollTarget);
>   };
>   return {
>     GlobalScrollTargetBehavior: GlobalScrollTargetBehavior,
>     setGlobalScrollTarget: setGlobalScrollTarget,
>   };
> });
> 
> > 
> > > If we could come up with a non-async way, that would be cool, but I'm not
> sure
> > > how.
> 

Done.

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/global_scroll_target_behavior.js (right):

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/global_scroll_target_behavior.js:7: * of a
global scroll target.
On 2016/10/19 19:50:09, michaelpg wrote:
> I'd add a little more instruction here, e.g. that the effect is elements w/
this
> behavior get a |scrollTarget| property which is set just after initialization.

Done.

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/global_scroll_target_behavior.js:17:
global.settingsScrollTargetData = {};
On 2016/10/19 19:50:09, michaelpg wrote:
> where does |global| actually come from? dunno why we use both global and
window
> for the same thing.

|global| is defined here:
https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr.js?rcl=0&l=10

No longer global, since wrapped in a function.

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/global_scroll_target_behavior.js:19:
global.settingsScrollTargetData.targetPromise = new Promise(function(accept) {
On 2016/10/19 19:50:09, michaelpg wrote:
> Would our PromiseResolver be more convenient?

Yes

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/global_scroll_target_behavior.js:32: notify:
true,
On 2016/10/19 19:50:09, michaelpg wrote:
> Shouldn't need notify: true; as we're not 2-way binding (setting passwords
> section's scrollTarget shouldn't propagate *up* to settings-ui)

Done.

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/global_scroll_target_behavior.js:38: *
Reference to the global data. This allows Polymer style data binding.
On 2016/10/19 19:50:08, michaelpg wrote:
> comment outdated

Acknowledged.

https://codereview.chromium.org/1914653002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/global_scroll_target_behavior.js:40: *  
targetPromise: (Promise<HTMLElement>|undefined),
On 2016/10/19 19:50:09, michaelpg wrote:
> make a typedef instead of copying/pasting

Acknowledged.

[commit-bot: I haz the power, 2016-10-20 15:22:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 15:22:13]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-10-22 05:01:20]

lgtm!

I like how the PromiseResolver means:
* we don't need an actual variable for the global scroll target
* setGlobalScrollTarget is guaranteed to no-op after the first time
* all instances of the behavior eventually get scrollTarget set, whether they're
created before or after setGlobalScrollTarget gets called

[hcarmona, 2016-10-24 13:59:06]

The CQ bit was checked by hcarmona@chromium.org

[commit-bot: I haz the power, 2016-10-24 13:59:24]

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-10-24 15:58:01]

Description was changed from

==========
Make lists in the passwords section aware of the scroll target.

BUG=654738
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Make lists in the passwords section aware of the scroll target.

BUG=654738
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-24 15:58:02]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-10-24 16:19:23]

Description was changed from

==========
Make lists in the passwords section aware of the scroll target.

BUG=654738
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Make lists in the passwords section aware of the scroll target.

BUG=654738
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b89d026ba8fbda3d8372bac6838a9cac6fa9f13b
Cr-Commit-Position: refs/heads/master@{#427079}
==========

[commit-bot: I haz the power, 2016-10-24 16:19:24]

Patchset 7 (id:??) landed as
https://crrev.com/b89d026ba8fbda3d8372bac6838a9cac6fa9f13b
Cr-Commit-Position: refs/heads/master@{#427079}