Update browser_tabview_firstrun_pref.js to test the existing behavior

RESOLVED FIXED

Status

Firefox Graveyard
Panorama
P4
normal
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: raymondlee, Assigned: raymondlee)

Tracking

Bug Flags:
in-testsuite +

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 515574 [details] [diff] [review]
v1

Removed the test since it is not included in the MakeFile.in.
Assignee: nobody → raymond
Status: NEW → ASSIGNED

Updated

7 years ago
Priority: -- → P4
(Assignee)

Updated

7 years ago
Attachment #515574 - Flags: review?(ian)

Comment 2

7 years ago
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?
(Assignee)

Updated

7 years ago
Summary: Remove browser_tabview_firstrun_pref.js test → Update browser_tabview_firstrun_pref.js to test the existing behavior
(Assignee)

Comment 5

7 years ago
Created attachment 517371 [details] [diff] [review]
v2

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+
(Assignee)

Comment 7

7 years ago
Created attachment 517457 [details] [diff] [review]
v2

Minor update
Attachment #517371 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #517457 - Attachment is patch: true
Attachment #517457 - Attachment mime type: application/octet-stream → text/plain
Attachment #517457 - Flags: review?(ian)
(Assignee)

Updated

7 years ago
Attachment #517457 - Flags: review?(ian)
(Assignee)

Comment 8

7 years ago
(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.
(Assignee)

Comment 9

7 years ago
Created attachment 517481 [details] [diff] [review]
v3
Attachment #517457 - Attachment is obsolete: true
(Assignee)

Comment 10

7 years ago
Created attachment 517482 [details] [diff] [review]
v3
Attachment #517481 - Attachment is obsolete: true
(Assignee)

Comment 11

7 years ago
Comment on attachment 517482 [details] [diff] [review]
v3

Passed try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=a240367d2f61
Attachment #517482 - Flags: approval2.0?
Comment on attachment 517482 [details] [diff] [review]
v3

tests don't need approval.
Attachment #517482 - Flags: approval2.0?
(Assignee)

Comment 13

7 years ago
Created attachment 517998 [details] [diff] [review]
Patch for check-in
Attachment #517482 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #517998 - Attachment is patch: true
Attachment #517998 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
No longer blocks: 585689
Blocks: 603789

Comment 14

7 years ago
http://hg.mozilla.org/mozilla-central/rev/7b66721b7e63
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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.