637232 - Update browser_tabview_firstrun_pref.js to test the existing behavior

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P4)

Description

[Raymond Lee [:raymondlee]]

The test/tabview/browser_tabview_firstrun_pref.js test file isn't included in the Makefile.in. If we don't run this test, it would be better to remove it.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Removed the test since it is not included in the MakeFile.in.

Comment 2

[Kevin Hanes]

Comment on attachment 515574 [details] [diff] [review]
v1

Flagging Mitcho for review as Ian is out part of this week as well.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 515574 [details] [diff] [review]
v1

I'd say this looks good as the pref is now tested in the test from bug 626791. We can re-enable this test once Panorama is not that hidden anymore (given the heuristic from bug 626791 will be disabled).

Comment 4

[Ian Gilman [:iangilman]]

(In reply to comment #3)
> Comment on attachment 515574 [details] [diff] [review]
> v1
> 
> I'd say this looks good as the pref is now tested in the test from bug 626791.
> We can re-enable this test once Panorama is not that hidden anymore (given the
> heuristic from bug 626791 will be disabled).

The bug 626791 test doesn't seem to test UI.reset, which this test did do. Instead of removing this test, can we cut it down so it just tests the existing behavior, and re-enable it?

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v2

Updated the patch.

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 517371 [details] [diff] [review]
v2

Thanks!

>-  todo_is(orphanTabCount, 1, "There should also be an orphaned tab");
>-  
>+  is(orphanTabCount, 0, "There should also be an orphaned tab");

Please update the label. 

>+  // open tabview doesn't count as first use experience so setting it manually
>+  win.TabView.firstUseExperienced = true;
>   ok(experienced(), "we're now experienced");

Seems like it would be cleaner to have this outside of checkFirstRun; you could put it right above the newWindowWithTabView call for checkNotFirstRun.

R+ with those tweaks

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v2

Minor update

Comment 8

[Raymond Lee [:raymondlee]]

(In reply to comment #6)
> Comment on attachment 517371 [details] [diff] [review]
> v2
> 
> Thanks!
> 
> >-  todo_is(orphanTabCount, 1, "There should also be an orphaned tab");
> >-  
> >+  is(orphanTabCount, 0, "There should also be an orphaned tab");
> 
> Please update the label. 

Fixed

> 
> >+  // open tabview doesn't count as first use experience so setting it manually
> >+  win.TabView.firstUseExperienced = true;
> >   ok(experienced(), "we're now experienced");
> 
> Seems like it would be cleaner to have this outside of checkFirstRun; you could
> put it right above the newWindowWithTabView call for checkNotFirstRun.
> 

Fixed

> R+ with those tweaks

Sent it to try and waiting for the results.

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: v3

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v3

Comment 11

[Raymond Lee [:raymondlee]]

Comment on attachment 517482 [details] [diff] [review]
v3

Passed try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=a240367d2f61

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 517482 [details] [diff] [review]
v3

tests don't need approval.

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7b66721b7e63