Closed Bug 458579 Opened 11 years ago Closed 11 years ago

Feed tab missing in page info

Categories

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

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: elmar.ludwig, Assigned: elmar.ludwig)

References

Details

(Keywords: regression, verified1.9.1)

Attachments

(2 files, 5 obsolete files)

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
Flags: blocking-firefox3.1?
Attached patch proposed patch (obsolete) — Splinter Review
trivial fix, this avoids using the no longer defined variable "feed"
Assignee: nobody → elmar.ludwig
Status: NEW → ASSIGNED
Attachment #341802 - Flags: review?(sayrer)
Attached patch proposed patchSplinter Review
same patch as before with correct path in the diff header
Attachment #341802 - Attachment is obsolete: true
Attachment #341816 - Flags: review?(sayrer)
Attachment #341802 - Flags: review?(sayrer)
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).
Attachment #341816 - Flags: review?(sayrer) → review+
(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.
Keywords: checkin-needed
No longer blocks: 459157
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Duplicate of this bug: 459272
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
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).
(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
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
Status: RESOLVED → VERIFIED
Please relnote this for 3.1beta 1.  it's fixed in the nightlies, but needs to be mentioned in the b1 releasenotes.
Keywords: relnote
(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.)
Flags: blocking-firefox3.1+ → blocking-firefox3.1?
Keywords: relnotecheckin-needed
Target Milestone: Firefox 3.1b2 → ---
Keywords: checkin-neededrelnote
Target Milestone: --- → Firefox 3.1b2
The blocking flag was changed from + to ? as well...
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?).
Can't you serve the page through HTTP?

See: <http://developer.mozilla.org/En/HTTP_server_for_unit_tests>
Attached patch Ok got it :) (obsolete) — Splinter Review
Here's the browser chrome test, it tests for feed tabs existence and for correct listing of the feeds.
Attachment #343368 - Attachment is obsolete: true
Great!  Did you mean to ask review on it, then?
Attachment #343451 - Flags: review?(gavin.sharp)
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.
(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.
(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.
(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"?
(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.
Attached patch Using observers, etc. (obsolete) — Splinter Review
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...).
Attachment #343451 - Attachment is obsolete: true
Attachment #343852 - Flags: review?(gavin.sharp)
Attachment #343451 - Flags: review?(gavin.sharp)
Attached patch Updated to Trunk (obsolete) — Splinter Review
Nothing new, just unbitrotted.
Attachment #343852 - Attachment is obsolete: true
Attachment #345133 - Flags: review?(gavin.sharp)
Attachment #343852 - Flags: review?(gavin.sharp)
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.
Attachment #345133 - Flags: review?(gavin.sharp) → review+
(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 :(.
Attachment #345133 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [check-needed for test]
Whiteboard: [check-needed for test] → [checkin-needed for test]
Comment on attachment 345136 [details] [diff] [review]
Nits Addressed
[Checkin: Comment 26]

http://hg.mozilla.org/mozilla-central/rev/5373680e5d49
Attachment #345136 - Attachment description: [for-checkin] Nits Addressed → Nits Addressed [Checkin: Comment 26]
Keywords: checkin-needed
Whiteboard: [checkin-needed for test]
Flags: in-testsuite? → in-testsuite+
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Keywords: verified1.9.1
Keywords: fixed1.9.1
Depends on: 767896
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.