Feed tab missing in page info

VERIFIED FIXED in Firefox 3.1b2

Status

()

Firefox
RSS Discovery and Preview
VERIFIED FIXED
9 years ago
5 years ago

People

(Reporter: Elmar Ludwig, Assigned: Elmar Ludwig)

Tracking

({regression, verified1.9.1})

Trunk
Firefox 3.1b2
regression, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Comment 1

9 years ago
Created attachment 341802 [details] [diff] [review]
proposed patch

trivial fix, this avoids using the no longer defined variable "feed"
Assignee: nobody → elmar.ludwig
Status: NEW → ASSIGNED
Attachment #341802 - Flags: review?(sayrer)
(Assignee)

Comment 2

9 years ago
Created attachment 341816 [details] [diff] [review]
proposed patch

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

Comment 4

9 years ago
(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
Blocks: 459157
No longer blocks: 459157
Flags: blocking-firefox3.1? → blocking-firefox3.1+

Updated

9 years ago
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
Last Resolved: 9 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).
(Assignee)

Comment 8

9 years ago
(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

9 years ago
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

Updated

9 years ago
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: relnote → checkin-needed
Target Milestone: Firefox 3.1b2 → ---
Keywords: checkin-needed → relnote
Target Milestone: --- → Firefox 3.1b2
The blocking flag was changed from + to ? as well...
Created attachment 343368 [details] [diff] [review]
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?).
Can't you serve the page through HTTP?

See: <http://developer.mozilla.org/En/HTTP_server_for_unit_tests>
Created attachment 343451 [details] [diff] [review]
Ok got it :)

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.
Created attachment 343852 [details] [diff] [review]
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...).
Attachment #343451 - Attachment is obsolete: true
Attachment #343852 - Flags: review?(gavin.sharp)
Attachment #343451 - Flags: review?(gavin.sharp)
Created attachment 345133 [details] [diff] [review]
Updated to Trunk

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+
Created attachment 345136 [details] [diff] [review]
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 :(.
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]
Keywords: relnote
Flags: in-testsuite? → in-testsuite+
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Keywords: fixed1.9.1

Updated

9 years ago
Keywords: verified1.9.1
Keywords: fixed1.9.1

Updated

5 years ago
Depends on: 767896
You need to log in before you can comment on or make changes to this bug.