598795 - Clean up reconnect code once we have instant access to sessionstore

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P4)

Description

[Ian Gilman [:iangilman]]

Once bug 590268 lands, we can get rid of the "delayed reconnect" code in Panorama, always loading our data immediately, as we'll know the data is going to be there right away.

Note that 590268 by itself will make a dramatic improvement in the panorama UX; this bug is more a code cleanup.

Comment 1

[Ian Gilman [:iangilman]]

Attachment: patch v1 (requires 590268, 598600)

Needs test.

Comment 2

[Ian Gilman [:iangilman]]

This blocks a blocker (bug 594644), so requesting blocking.

Comment 3

[Sean Dunn]

330	        // the code in the progress listener doesn't fire sometimes because
331	        // tab is being restored so need to catch that.
332	        setTimeout(function() {
333	          if (this && this.isShowingCachedData) {
334	            this.hideCachedData();
335	          }
336	        }, 15000);

I know you didn't write this, but is this something that needs to be improved? Should it be a new bug?

Besides that, it looks straightforward.

Comment 4

[Raymond Lee [:raymondlee]]

(In reply to comment #3)
> 330            // the code in the progress listener doesn't fire sometimes
> because
> 331            // tab is being restored so need to catch that.
> 332            setTimeout(function() {
> 333              if (this && this.isShowingCachedData) {
> 334                this.hideCachedData();
> 335              }
> 336            }, 15000);
> 
> I know you didn't write this, but is this something that needs to be improved?
> Should it be a new bug?
> 
> Besides that, it looks straightforward.

I believe they are just typos because the correct references are used in the original code.

Comment 5

[Ian Gilman [:iangilman]]

(In reply to comment #3)
> I know you didn't write this, but is this something that needs to be improved?
> Should it be a new bug?

You're referring to the 15 second timeout? That's getting removed in bug 597248.

> Besides that, it looks straightforward.

Thanks!

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 484478 [details] [diff] [review]
patch v1 (requires  590268, 598600)

I've come to the conclusion that this patch doesn't need a test, as it's just a refactoring. Ready for review.

Comment 7

[Johnathan Nightingale [:johnath]]

Blocks a blocker.

Comment 8

[Ian Gilman [:iangilman]]

Comment on attachment 484478 [details] [diff] [review]
patch v1 (requires  590268, 598600)

Load balancing review over to dietrich now that it's blocking.

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 484478 [details] [diff] [review]
patch v1 (requires  590268, 598600)

looks good, r=me

Comment 10

[Ian Gilman [:iangilman]]

Landed on panorama-central:

http://hg.mozilla.org/users/ian_iangilman.com/panorama-central/rev/c379ef042d95

Comment 11

[Ian Gilman [:iangilman]]

Attachment: patch v2

Updated for bitrot and test failures.

Comment 12

[Kevin Hanes]

Beta8 has 1 bug left on it, moving our blockers to b9

Comment 13

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/2d849e2d302e