Stop ChromeVox Next page loading sounds when page finishes loading.

Stop ChromeVox Next page loading sounds when page finishes loading.

The underlying issue was that the chrome.tabs.onUpdated event sometimes
doesn't fire when a page finishes loading, so this change adds code to
poll it. In addition, it distinguishes between a page finishing loading,
and cancelling the page load - like when you switch from a loading tab
to a tab that's already loaded.

BUG=557913
Committed: https://crrev.com/6ce5cc796fbb7d67377df79e4a2e814da1fa59f6
Cr-Commit-Position: refs/heads/master@{#361766}

[dmazzoni, 2015-11-18 19:44:59]

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

[Peter Lundblad, 2015-11-19 13:04:42]

lgtm

Is ti feasable to write some test for the state handling?

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

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:199:
if (this.pageLoadTabID_)
When will this end up being false? Would that be a sign of a bug?

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:212:
}
Clear the tab id as well to have an inconsistent state.

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:218:
isPageLoading_: function() {
I think this nname is confusing. It doesn't reflect whether the page is loading,
but only if we are still polling or not.

[David Tseng, 2015-11-19 17:04:08]

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

[David Tseng, 2015-11-19 17:04:09]

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

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:93:
if (tab.status == 'loading' && !this.isPageLoading_()) {
This line is very confusing as it reads tab loading and page is not loading?

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:97:
cvox.ChromeVox.earcons.playEarcon(cvox.Earcon.PAGE_CANCEL_LOADING);
What does the cancel earcon mean here?

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:127:
cvox.ChromeVox.earcons.playEarcon(cvox.Earcon.PAGE_START_LOADING);
One issue I think I saw was that you could have multiple earcons playing
simultaneously. Wanted to note that here as a reminder. Can file a bug as well.

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:208:
cancelPageLoadTimer_: function() {
I still don't see what would cancel the earcon when we close the tab while the
page is still loading?

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:219:
return !!this.pageLoadIntervalID_;
nit: != null

[David Tseng, 2015-11-19 17:10:35]

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

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:208:
cancelPageLoadTimer_: function() {
On 2015/11/19 17:04:09, David Tseng wrote:
> I still don't see what would cancel the earcon when we close the tab while the
> page is still loading?

Also, what happens if I switch windows (with alt+tab)? Shouldn't we be doing
something inside of onFocusChanged?

[dmazzoni, 2015-11-25 21:13:19]

PTAL. All feedback addressed but could use one more review pass.

Tests would be nice but the code really doesn't lend itself to being tested
right now - it'd require refactoring out the chrome.tabs and chrome.windows APIs
to make them mockable.

If it's okay I'd like to land this and move on for now. The main reason is I
don't think we're incurring a lot of technical debt with this source file - it's
not as clean as it could be but it's pretty self-contained. If this was more
core to the ChromeVox Next logic it'd probably be better to refactor and add
tests now.

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

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:93:
if (tab.status == 'loading' && !this.isPageLoading_()) {
On 2015/11/19 17:04:09, David Tseng wrote:
> This line is very confusing as it reads tab loading and page is not loading?

Changed to !this.isPlayingPageLoadingSound_().

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:97:
cvox.ChromeVox.earcons.playEarcon(cvox.Earcon.PAGE_CANCEL_LOADING);
On 2015/11/19 17:04:09, David Tseng wrote:
> What does the cancel earcon mean here?

I guess this was confusing. I changed it to:

cvox.ChromeVox.earcons.cancelEarcon(cvox.Earcon.PAGE_START_LOADING);

Classic earcons does nothing on cancelEarcon, but Next earcons can cancel a
long-running earcon when this is called.

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:127:
cvox.ChromeVox.earcons.playEarcon(cvox.Earcon.PAGE_START_LOADING);
On 2015/11/19 17:04:09, David Tseng wrote:
> One issue I think I saw was that you could have multiple earcons playing
> simultaneously. Wanted to note that here as a reminder. Can file a bug as
well.

Yes, thanks. This patch fixes this in earcon_engine.js by having startProgress()
cancel a previous progress sound and then restart it.

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:199:
if (this.pageLoadTabID_)
On 2015/11/19 13:04:42, Peter Lundblad wrote:
> When will this end up being false? Would that be a sign of a bug?

Shouldn't happen, but we get a compiler warning because you can't pass null to
onUpdated.

Seems safer to check than to typecast. If you disagree I can fix it in a
follow-up.

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:208:
cancelPageLoadTimer_: function() {
On 2015/11/19 17:04:09, David Tseng wrote:
> I still don't see what would cancel the earcon when we close the tab while the
> page is still loading?

Yes, good catch. If you switch to any other tab it's caught by onActivated on
the next tab. But if you closed all tabs it's possible the earcon could still be
playing so I added code to handle that in onRemoved.

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:208:
cancelPageLoadTimer_: function() {
On 2015/11/19 17:10:35, David Tseng wrote:
> On 2015/11/19 17:04:09, David Tseng wrote:
> > I still don't see what would cancel the earcon when we close the tab while
the
> > page is still loading?
> 
> Also, what happens if I switch windows (with alt+tab)? Shouldn't we be doing
> something inside of onFocusChanged?

Needed there too. Thanks.

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:212:
}
On 2015/11/19 13:04:42, Peter Lundblad wrote:
> Clear the tab id as well to have an inconsistent state.

Done.

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:218:
isPageLoading_: function() {
On 2015/11/19 13:04:42, Peter Lundblad wrote:
> I think this nname is confusing. It doesn't reflect whether the page is
loading,
> but only if we are still polling or not.

Done.

https://codereview.chromium.org/1455223003/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox/background/tabs_api_handler.js:219:
return !!this.pageLoadIntervalID_;
On 2015/11/19 17:04:09, David Tseng wrote:
> nit: != null

Done.

[David Tseng, 2015-11-25 23:33:09]

lgtm



[dmazzoni, 2015-11-25 23:36:07]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-11-25 23:36:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
plundblad@chromium.org
Link to the patchset: https://codereview.chromium.org/1455223003/#ps20001
(title: "Fixed focus changing and removing all tabs, addressed other feedback")

[commit-bot: I haz the power, 2015-11-25 23:40:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455223003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455223003/20001

[commit-bot: I haz the power, 2015-11-26 00:44:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-26 00:49:22]

Description was changed from

==========
Stop ChromeVox Next page loading sounds when page finishes loading.

The underlying issue was that the chrome.tabs.onUpdated event sometimes
doesn't fire when a page finishes loading, so this change adds code to
poll it. In addition, it distinguishes between a page finishing loading,
and cancelling the page load - like when you switch from a loading tab
to a tab that's already loaded.

BUG=557913
==========

to

==========
Stop ChromeVox Next page loading sounds when page finishes loading.

The underlying issue was that the chrome.tabs.onUpdated event sometimes
doesn't fire when a page finishes loading, so this change adds code to
poll it. In addition, it distinguishes between a page finishing loading,
and cancelling the page load - like when you switch from a loading tab
to a tab that's already loaded.

BUG=557913

Committed: https://crrev.com/6ce5cc796fbb7d67377df79e4a2e814da1fa59f6
Cr-Commit-Position: refs/heads/master@{#361766}
==========

[commit-bot: I haz the power, 2015-11-26 00:49:23]

Patchset 2 (id:??) landed as
https://crrev.com/6ce5cc796fbb7d67377df79e4a2e814da1fa59f6
Cr-Commit-Position: refs/heads/master@{#361766}