Closed Bug 637232 Opened 9 years ago Closed 9 years ago

Update browser_tabview_firstrun_pref.js to test the existing behavior

Categories

(Firefox Graveyard :: Panorama, defect, P4)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: raymondlee, Assigned: raymondlee)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Attached patch v1 (obsolete) — Splinter Review
Removed the test since it is not included in the MakeFile.in.
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Priority: -- → P4
Attachment #515574 - Flags: review?(ian)
Comment on attachment 515574 [details] [diff] [review]
v1

Flagging Mitcho for review as Ian is out part of this week as well.
Attachment #515574 - Flags: feedback?(mitcho)
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).
Attachment #515574 - Flags: feedback+
(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?
Summary: Remove browser_tabview_firstrun_pref.js test → Update browser_tabview_firstrun_pref.js to test the existing behavior
Attached patch v2 (obsolete) — Splinter Review
Updated the patch.
Attachment #515574 - Attachment is obsolete: true
Attachment #515574 - Flags: review?(ian)
Attachment #515574 - Flags: feedback?(mitcho)
Attachment #517371 - Flags: review?(ian)
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
Attachment #517371 - Flags: review?(ian) → review+
Attached patch v2 (obsolete) — Splinter Review
Minor update
Attachment #517371 - Attachment is obsolete: true
Attachment #517457 - Attachment is patch: true
Attachment #517457 - Attachment mime type: application/octet-stream → text/plain
Attachment #517457 - Flags: review?(ian)
Attachment #517457 - Flags: review?(ian)
(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.
Attached patch v3 (obsolete) — Splinter Review
Attachment #517457 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
Attachment #517481 - Attachment is obsolete: true
Comment on attachment 517482 [details] [diff] [review]
v3

tests don't need approval.
Attachment #517482 - Flags: approval2.0?
Attachment #517482 - Attachment is obsolete: true
Attachment #517998 - Attachment is patch: true
Attachment #517998 - Attachment mime type: application/octet-stream → text/plain
Keywords: checkin-needed
No longer blocks: 585689
http://hg.mozilla.org/mozilla-central/rev/7b66721b7e63
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.