458579 - Feed tab missing in page info

Status: VERIFIED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Elmar Ludwig]

Since the 2008-09-07-02 nightly build, the RSS/ATOM feed list is missing from the page info dialog. This is a regression from the checkin for bug 413915. Fix is trivial, going to attach in a minute.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080929020532 Minefield/3.1b1pre ID:20080929020532

Comment 1

[Elmar Ludwig]

Attachment: proposed patch

trivial fix, this avoids using the no longer defined variable "feed"

Comment 2

[Elmar Ludwig]

Attachment: proposed patch

same patch as before with correct path in the diff header

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 341816 [details] [diff] [review]
proposed patch

Any chance I could convince you to write a browser chrome test for this? :)

Something similar to browser/base/content/test/browser_autodiscovery.js would work (see also 
http://developer.mozilla.org/en/Browser_chrome_tests if you're not familiar with them).

Comment 4

[Elmar Ludwig]

(In reply to comment #3)
> (From update of attachment 341816 [details] [diff] [review])
> Any chance I could convince you to write a browser chrome test for this? :)

Sorry, but I never did a chrome test before and really don't have time to do this at the moment... :-(

Marking patch for checkin once the tree reopens.

Comment 6

[(no longer active)]

Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/57a08a2acc51>.  Marked in-testsuite? for tracking the development of the browser chrome test requested in comment 3.

Comment 7

[Nochum Sossonko [:Natch]]

I can make the test, but 1)Should I just test for the feed tabs existence? 2)The test you mentioned isn't there (did you mean browser_discovery.js).

Comment 8

[Elmar Ludwig]

(In reply to comment #7)
> 2)The test you mentioned isn't there (did you mean browser_discovery.js).

The test file Gavin mentioned was renamed to browser_discovery.js recently:

http://hg.mozilla.org/mozilla-central/rev/0e52c4677cb4

Comment 9

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081013 Minefield/3.1b2pre

Comment 10

[Tony Chung [:tchung]]

Please relnote this for 3.1beta 1.  it's fixed in the nightlies, but needs to be mentioned in the b1 releasenotes.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> I can make the test, but 1)Should I just test for the feed tabs existence?

That would be better than the current state of no tests at all, sure :)

Even better would be to test the functionality as well (check that all feeds are listed correctly, maybe even test that the "Subscribe" button works, etc.)

Comment 12

[(no longer active)]

The blocking flag was changed from + to ? as well...

Comment 13

[Nochum Sossonko [:Natch]]

Attachment: doesn't work (although it should) :-(

chrome:// urls aren't supported in the page info dialog (afaict), I was driving myself nuts, till I just put the file in a chrome url and realized that page info doesn't work properly with chrome urls... this should do it though otherwise, any ideas (I could use a legit website, but which one would that be?).

Comment 14

[(no longer active)]

Can't you serve the page through HTTP?

See: <http://developer.mozilla.org/En/HTTP_server_for_unit_tests>

Comment 15

[Nochum Sossonko [:Natch]]

Attachment: Ok got it :)

Here's the browser chrome test, it tests for feed tabs existence and for correct listing of the feeds.

Comment 16

[(no longer active)]

Great!  Did you mean to ask review on it, then?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 343451 [details] [diff] [review]
Ok got it :)

Thanks for doing this!

>diff --git a/browser/base/content/test/browser_feed_tab.js b/browser/base/content

>+  function handleLoad() {
>+
>+    pageInfo = window.openDialog("chrome://browser/content/pageinfo/pageInfo.xul",
>+                                     "_blank",
>+                                     "chrome,toolbar,dialog=no,resizable",
>+                                     { doc: content.document, initialTab: null });

Using BrowserPageInfo() would result in more complete test coverage.

>+    setTimeout(handlePageInfo, 1000);

Can you not use an onload listener set on the window ("pageInfo")?

>+  function handlePageInfo() {

>+    var tests = [{ check: feedRowsNum == 3,
>+                   message: "Number of feeds listed: " +
>+                            feedRowsNum + ", should be 3" }];
>+    
>+    
>+    var feedItems = [];
>+    for (var i = 0; i < feedRowsNum; i++) {
>+      feedItems.push(feedListbox.getItemAtIndex(i));
>+      tests.push({ check: feedItems[i].getAttribute("name") == (i+1),
>+                   message: "Name given: " + feedItems[i].getAttribute("name") +
>+                            ", should be " + (i+1) });
>+    }
>+  
>+    for (var i = 0; i < tests.length; i++) {
>+      ok(tests[i].check, tests[i].message);
>+    }

Building a "tests" array and then running them seperately is kind of unnecessary, I would just have a single loop and just do the tests directly.

Comment 18

[Nochum Sossonko [:Natch]]

(In reply to comment #17)
> >+  function handleLoad() {
> >+
> >+    pageInfo = window.openDialog("chrome://browser/content/pageinfo/pageInfo.xul",
> >+                                     "_blank",
> >+                                     "chrome,toolbar,dialog=no,resizable",
> >+                                     { doc: content.document, initialTab: null });
> 
> Using BrowserPageInfo() would result in more complete test coverage.
How would I get the window after that to test/close it? I tried with nsIWindowMediator's getMostRecentWindow, but it doesn't work (I *think* b/c it needs time to init or we).
> >+    setTimeout(handlePageInfo, 1000);
> 
> Can you not use an onload listener set on the window ("pageInfo")?
Won't do as the feeds get loaded in after that event fires and then I can't check the number of feeds or the order in which they are listed.

> Building a "tests" array and then running them seperately is kind of
> unnecessary, I would just have a single loop and just do the tests directly.
Yeah, I'll redo that as soon as I get the answer's for the other two.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #18)
> > Using BrowserPageInfo() would result in more complete test coverage.
> How would I get the window after that to test/close it?

We should just change BrowserPageInfo/toOpenDialogByTypeAndUrl to return the window.

> > >+    setTimeout(handlePageInfo, 1000);
> > 
> > Can you not use an onload listener set on the window ("pageInfo")?
> Won't do as the feeds get loaded in after that event fires and then I can't
> check the number of feeds or the order in which they are listed.

Hrm, yeah, that's sucky. The timeout isn't really an option, though, because they frequently cause failures on the (slow) unit test machines. What we could do is have initFeedTab fire an observer notification similar to http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/applications.js#986 and use that in the test.

Comment 20

[Nochum Sossonko [:Natch]]

(In reply to comment #19)
Alright, I'll make those changes, though should I just put the notification in onLoadPageInfo (to make it more general) and then call it something like "page-info-dialog-loaded"?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #20)
> Alright, I'll make those changes, though should I just put the notification in
> onLoadPageInfo (to make it more general) and then call it something like
> "page-info-dialog-loaded"?

Sounds good.

Comment 22

[Nochum Sossonko [:Natch]]

Attachment: Using observers, etc.

Ok this implements everything mentioned previously. Heading on a flight now, thanks for the review. (I added the return in BrowserPageInfo even though I probably could have gotten the window from the observer...).

Comment 23

[Nochum Sossonko [:Natch]]

Attachment: Updated to Trunk

Nothing new, just unbitrotted.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 345133 [details] [diff] [review]
Updated to Trunk

>diff --git a/browser/base/content/test/browser_feed_tab.js b/browser/base/content/test/browser_feed_tab.js

>+  function handlePageInfo() {

>+    var feedItems = [];
>+    for (var i = 0; i < feedRowsNum; i++) {
>+      feedItems.push(feedListbox.getItemAtIndex(i));
>+      ok(feedItems[i].getAttribute("name") == (i+1), "Name given: " + 
>+                                                     feedItems[i].getAttribute("name") +
>+                                                     ", should be " + (i+1));
>+    }

Again, it seems kind of silly to build an array here, since you're not actually even using it. Just use something like |let feedItem = feedListbox.getItemAtIndex(i);| and get rid of feedItems. Could rewrap to put the second argument on its own line and avoid wrapping it far off to the right like that.

Comment 25

[Nochum Sossonko [:Natch]]

Attachment: Nits Addressed [Checkin: Comment 26]

(In reply to comment #24)

> Again, it seems kind of silly to build an array here, since you're not actually
> even using it. Just use something like |let feedItem =
> feedListbox.getItemAtIndex(i);| and get rid of feedItems. Could rewrap to put
> the second argument on its own line and avoid wrapping it far off to the right
> like that.

Yeah, stupid me forgot to change that array as well :(.

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 345136 [details] [diff] [review]
Nits Addressed
[Checkin: Comment 26]

http://hg.mozilla.org/mozilla-central/rev/5373680e5d49