Last Comment Bug 735139 - Improve browser_pageInfo.js a little
: Improve browser_pageInfo.js a little
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Page Info Window (show other bugs)
: Trunk
: All All
: P4 minor (vote)
: Firefox 13
Assigned To: Serge Gautherie (:sgautherie)
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks: 735143
  Show dependency treegraph
 
Reported: 2012-03-12 20:07 PDT by Serge Gautherie (:sgautherie)
Modified: 2012-03-13 12:26 PDT (History)
1 user (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(Av1) Improve browser_pageInfo.js a little [Checked in: See comment 3+5] (2.31 KB, patch)
2012-03-12 20:55 PDT, Serge Gautherie (:sgautherie)
dao+bmo: review+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2012-03-12 20:07:26 PDT

    
Comment 1 Serge Gautherie (:sgautherie) 2012-03-12 20:55:27 PDT
Created attachment 605294 [details] [diff] [review]
(Av1) Improve browser_pageInfo.js a little
[Checked in: See comment 3+5]
Comment 2 Dão Gottwald [:dao] 2012-03-13 04:17:41 PDT
Comment on attachment 605294 [details] [diff] [review]
(Av1) Improve browser_pageInfo.js a little
[Checked in: See comment 3+5]

>-    ok(feedRowsNum == 3, "Number of feeds listed: " +
>-                         feedRowsNum + ", should be 3");
>-
>+    is(feedRowsNum, 3, "3 feeds listed");

is(feedRowsNum, 3, "Number of feeds listed");

>     for (var i = 0; i < feedRowsNum; i++) {
>       let feedItem = feedListbox.getItemAtIndex(i);
>-      ok(feedItem.getAttribute("name") == (i+1), 
>-         "Name given: " + feedItem.getAttribute("name") + ", should be " + (i+1));
>+      is(feedItem.getAttribute("name"), i + 1,
>+         "Feed name " + (i + 1) + "/" + feedRowsNum);

is(feedItem.getAttribute("name"), i + 1, "Feed name");

r+ with these changes
Comment 3 Serge Gautherie (:sgautherie) 2012-03-13 10:00:48 PDT
Comment on attachment 605294 [details] [diff] [review]
(Av1) Improve browser_pageInfo.js a little
[Checked in: See comment 3+5]

Av1, with comment 2 suggestion(s).


(In reply to Dão Gottwald [:dao] from comment #2)

Ftr,

> >+    is(feedRowsNum, 3, "3 feeds listed");
> 
> is(feedRowsNum, 3, "Number of feeds listed");

My text change was per bug 521263 comment 26, but I'm fine retaining the initial text.

> >-      ok(feedItem.getAttribute("name") == (i+1), 
> >-         "Name given: " + feedItem.getAttribute("name") + ", should be " + (i+1));
> >+      is(feedItem.getAttribute("name"), i + 1,
> >+         "Feed name " + (i + 1) + "/" + feedRowsNum);
> 
> is(feedItem.getAttribute("name"), i + 1, "Feed name");

I'm not totally fine with having no reference to which name is checked (especially without hint at how many they should be) and getting 3 times the same text, but as you wish.
Comment 4 Dão Gottwald [:dao] 2012-03-13 10:04:16 PDT
is() will spit out the expected value in case of a failure, which seems sufficient here.
Comment 5 Serge Gautherie (:sgautherie) 2012-03-13 10:04:54 PDT
Comment on attachment 605294 [details] [diff] [review]
(Av1) Improve browser_pageInfo.js a little
[Checked in: See comment 3+5]

https://hg.mozilla.org/mozilla-central/rev/cf4978c2e32c
Comment 6 Serge Gautherie (:sgautherie) 2012-03-13 12:21:28 PDT
https://tbpl.mozilla.org/php/getParsedLog.php?id=10037139&tree=Firefox&full=1
Rev3 Fedora 12x64 mozilla-central debug test mochitest-other on 2012-03-13 10:17:57 PDT for push cf4978c2e32c
{
TEST-PASS | [...] | Feed tab
TEST-PASS | [...] | Feed list
TEST-PASS | [...] | Number of feeds listed
TEST-PASS | [...] | Feed name
TEST-PASS | [...] | Feed name
TEST-PASS | [...] | Feed name
}

V.Fixed

Note You need to log in before you can comment on or make changes to this bug.