Defer creating ChromeVox node menus to make menus open more quickly.

Defer creating ChromeVox node menus to make menus open more quickly.

BUG=none
TESTED=open panel menu and/or open headings list on large wikipedia page
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[dmazzoni, 2016-10-24 19:54:20]

Description was changed from

==========
Defer creating ChromeVox node menus to make menus open more quickly.

BUG=none
TESTED=open panel menu and/or open headings list on large wikipedia page
==========

to

==========
Defer creating ChromeVox node menus to make menus open more quickly.

BUG=none
TESTED=open panel menu and/or open headings list on large wikipedia page
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dmazzoni, 2016-10-24 19:55:02]

dmazzoni@chromium.org changed reviewers:
+ dtseng@chromium.org

[David Tseng, 2016-10-24 22:19:11]

https://codereview.chromium.org/2443103004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_menu.js
(right):

https://codereview.chromium.org/2443103004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_menu.js:244:
}).bind(this), 500);
It would be great if defer could populate the menus when it gets activated for
the first time. I'm not sure what happens now if one arrows rapidly across all
top-level menus.

https://codereview.chromium.org/2443103004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_menu.js:259: 
js doc

[dmazzoni, 2016-10-27 18:56:36]

https://codereview.chromium.org/2443103004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_menu.js
(right):

https://codereview.chromium.org/2443103004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_menu.js:244:
}).bind(this), 500);
On 2016/10/24 22:19:11, David Tseng wrote:
> It would be great if defer could populate the menus when it gets activated for
> the first time. I'm not sure what happens now if one arrows rapidly across all
> top-level menus.

I tried that, but that would actually be more janky.

I cleaned up the code a bit more and now there's no
need for an arbitrary timeout of 500 ms. Instead, all
of the menus other than the one you select do the
search in "chunks" of about 100 nodes before calling
setTimeout.

The result is that if you open the headings list
directly, for example, it pauses briefly to populate the menu
and then opens directly to it, giving you accurate
feedback (for example Table Of Contents Heading 2
5 of 17).

If you open the menus in general without choosing a
PanelNodeMenu, those menus all populate asynchronously,
so if you are very fast and navigate to one while it's
still populating you might get an incomplete result.
However, this is unlikely since even for a large page
it only takes a second or two to populate all five.
That one or two seconds makes a big difference in
the perceived time to open the menu in general, though,
which is why I think this change is worth it.

https://codereview.chromium.org/2443103004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_menu.js:259: 
On 2016/10/24 22:19:11, David Tseng wrote:
> js doc

Done.

[David Tseng, 2016-10-28 21:11:34]

https://codereview.chromium.org/2443103004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_menu.js
(right):

https://codereview.chromium.org/2443103004/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_menu.js:269:
if (!this.node_)
Does this happen? Maybe you should store the range after making this change to
pick up on ancestors.

[dmazzoni, 2016-10-28 21:28:06]

https://codereview.chromium.org/2443103004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_menu.js
(right):

https://codereview.chromium.org/2443103004/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_menu.js:269:
if (!this.node_)
On 2016/10/28 21:11:34, David Tseng wrote:
> Does this happen? Maybe you should store the range after making this change to
> pick up on ancestors.

I didn't observe it happening, but the Closure compiler said
that the input was nullable. Should we fix it higher up in
the call chain and make this take a {!AutomationNode}?

[David Tseng, 2016-10-28 23:09:15]

https://codereview.chromium.org/2443103004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_menu.js
(right):

https://codereview.chromium.org/2443103004/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_menu.js:269:
if (!this.node_)
On 2016/10/28 21:28:06, dmazzoni wrote:
> On 2016/10/28 21:11:34, David Tseng wrote:
> > Does this happen? Maybe you should store the range after making this change
to
> > pick up on ancestors.
> 
> I didn't observe it happening, but the Closure compiler said
> that the input was nullable. Should we fix it higher up in
> the call chain and make this take a {!AutomationNode}?

Yes; I guess the node could in theory be detached at any time. At the very
least, you need to call finish() in all of the return points here.

[dmazzoni, 2016-11-01 22:17:16]

Good call about calling finish() from all codepaths.
Fixed, PTAL.

[David Tseng, 2016-11-01 22:20:18]

lgtm

[dmazzoni, 2016-11-02 06:36:43]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-11-02 06:36:56]

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-11-02 07:51:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-02 07:53:31]

Description was changed from

==========
Defer creating ChromeVox node menus to make menus open more quickly.

BUG=none
TESTED=open panel menu and/or open headings list on large wikipedia page
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Defer creating ChromeVox node menus to make menus open more quickly.

BUG=none
TESTED=open panel menu and/or open headings list on large wikipedia page
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[commit-bot: I haz the power, 2016-11-02 07:53:32]

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