Closed
Bug 637232
Opened 14 years ago
Closed 14 years ago
Update browser_tabview_firstrun_pref.js to test the existing behavior
Categories
(Firefox Graveyard :: Panorama, defect, P4)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: raymondlee, Assigned: raymondlee)
References
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
Removed the test since it is not included in the MakeFile.in.
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Updated•14 years ago
|
Priority: -- → P4
Assignee | ||
Updated•14 years ago
|
Attachment #515574 -
Flags: review?(ian)
Comment 2•14 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 3•14 years ago
|
||
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+
Comment 4•14 years ago
|
||
(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•14 years ago
|
Summary: Remove browser_tabview_firstrun_pref.js test → Update browser_tabview_firstrun_pref.js to test the existing behavior
Assignee | ||
Comment 5•14 years ago
|
||
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 6•14 years ago
|
||
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 | ||
Updated•14 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•14 years ago
|
Attachment #517457 -
Flags: review?(ian)
Assignee | ||
Comment 8•14 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•14 years ago
|
||
Attachment #517457 -
Attachment is obsolete: true
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #517481 -
Attachment is obsolete: true
Assignee | ||
Comment 11•14 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 12•14 years ago
|
||
Comment on attachment 517482 [details] [diff] [review]
v3
tests don't need approval.
Attachment #517482 -
Flags: approval2.0?
Assignee | ||
Comment 13•14 years ago
|
||
Attachment #517482 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #517998 -
Attachment is patch: true
Attachment #517998 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 14•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•