Implement Web Page Feed Detection

RESOLVED FIXED in seamonkey2.0a1

Status

defect
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: bugs_mozilla_2q1889, Assigned: Callek)

Tracking

(Depends on 1 bug)

Trunk
seamonkey2.0a1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 13 obsolete attachments)

61.81 KB, image/png
Details
89.69 KB, patch
Details | Diff | Splinter Review
2.86 KB, image/png
Details
34.05 KB, patch
Callek
: review+
Callek
: superreview+
Details | Diff | Splinter Review
Reporter

Description

15 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040316
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040316

Isn't there an easy and simple way to use the bookmark manager as an RSS feed
reader? There are already update notifications implemented. To make it a simple
reader, one could add the following features:

1.) clicking on the bookmark button loads the url associated with the earliest
new headline.
2.) clicking on the bookmark (or via context menu) displays an internally
generated html page representing the RSS feed data and providing the
corresponding links to the full texts.
3.) Tooltips for the bookmark buttons could contain new headlines.

Would be very neat. I don't like to use an additional RSS reader.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 2

15 years ago
See also bug 244078.
Reporter

Comment 3

15 years ago

*** This bug has been marked as a duplicate of 244078 ***
Status: UNCONFIRMED → RESOLVED
Closed: 15 years ago
Resolution: --- → DUPLICATE
Product: Browser → Seamonkey

Comment 4

14 years ago
This bug was filed against the Mozilla Application Suite (Seamonkey).  Why was
it duped against a Firefox bug?

On a tangent, is porting Firefox's Live Bookmarks planned?

Comment 5

14 years ago
from duped bug:

Mail&News should have the RSS Reader from Thunderbird (as described in bug
255834) but the Navigator should have the RSS abbilities from Firefox (Live
Bookmarks, see bug 244078) too one of the greatest things in Firefox that i miss
in SeaMonkey.
Severity: enhancement → normal
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
Summary: RSS-feed integration with bookmarks → port Firefox Live Bookmarks / RSS reader to Seamonkey (RSS-feed integration with bookmarks)
Target Milestone: --- → Future

Comment 6

14 years ago
*** Bug 310176 has been marked as a duplicate of this bug. ***

Updated

14 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: port Firefox Live Bookmarks / RSS reader to Seamonkey (RSS-feed integration with bookmarks) → port Firefox Live Bookmarks / RSS reader to SeaMonkey (RSS-feed integration with bookmarks)

Comment 7

14 years ago
Reassigning as per Bug #32644
Assignee: p_ch → nobody

Comment 8

13 years ago
*** Bug 337615 has been marked as a duplicate of this bug. ***
The bug I reported 337615 Also aplies to the latest version of Firefox (not a Nightly).

While Seamonkey does not have a RSS button if you click a RSS link and go to some they appear correctly and some do not.
Assignee

Comment 10

12 years ago
I'm working on this right now, will tie this in with Bug 255834 when I get further along.

Blocks: 255834
Status: NEW → ASSIGNED
Target Milestone: Future → seamonkey2.0alpha
Assignee

Comment 11

12 years ago
Posted patch WIP patch (Feed discovery) (obsolete) — Splinter Review
figured I'd post my current effort here so far (its majority a direct FF port so far)

note: the livemark-item.png is a direct copy of the FF winstripe one for now
Assignee

Comment 12

12 years ago
Assignee

Comment 13

12 years ago
Posted patch Part 1: Feed Discovery (obsolete) — Splinter Review
Attachment #285190 - Attachment is obsolete: true
Assignee

Comment 14

12 years ago
Comment on attachment 285323 [details] [diff] [review]
Part 1: Feed Discovery

Notes:

* This only does the Discovery and UI Exposing of the feed names (subscribe to feed menu items)
* the file at suite/themes/*/communicator/icons/livemark-item.png is a direct copy of the one at browser/themes/winstripe/browser/livemark-item.png
* the function openUILink is not defined for us (yet), which makes the UI of this patch, basically a no-op when selected.
* I moved the icon handling code out of the tabbrowser binding along with this.
* majority of this here is close to an exact lift from FF code

This should all be ok to land on its own pending review, however I can supply Part 2's patch as a combination with this part if its easier for the reviewer.
Attachment #285323 - Flags: superreview?(neil)
Attachment #285323 - Flags: review?(neil)

Comment 15

12 years ago
> the function openUILink is not defined for us (yet), which makes the UI of
> this patch, basically a no-op when selected.

This would be Bug 370698
Depends on: 370698

Comment 16

12 years ago
(In reply to comment #14)
> * the file at suite/themes/*/communicator/icons/livemark-item.png is a direct
> copy of the one at browser/themes/winstripe/browser/livemark-item.png

I think the icons themselves are good, at least for the default theme, maybe actually even for Modern (as the orange color is standardized along with the feed icon look). I's prefer to have it named "feed[s].png" though, at it isn't per se livemark-specific, but feed-specific.

> * the function openUILink is not defined for us (yet), which makes the UI of
> this patch, basically a no-op when selected.

Do we need this function now that the global utilityOverlay from toolkit provides openLink() to us?

Comment 17

12 years ago
> Do we need this function now that the global utilityOverlay from toolkit
> provides openLink() to us?

In Bug 384139 you moved openURL() to mozilla/toolkit/content/contentAreaUtils.js but that function uses a different pref and treats links as external. openUILink() and it's relatives treats links as internal.
Assignee

Comment 18

12 years ago
>>I's prefer to have it named "feed[s].png" though, at it isn't
per se livemark-specific, but feed-specific.<<

That sounds doable, but the livemark-item has other UI stuff in it than *just* the url-bar RSS item, I'm not sure that the differentiation is really needed here in terms of splitting out other things, is it?
Assignee

Updated

12 years ago
Assignee: nobody → bugspam.Callek
Status: ASSIGNED → NEW
Assignee

Comment 19

12 years ago
Posted patch WIP Part 2Splinter Review
This actually works for when you load a feed in the url-bar directly, loads the "about:feeds" type page you get in Firefox. (Subscribe button does nothing yet [i think])

still to do for this "Part"
* Modern Theme
* Attach a zip of binary files
* implement openUILink
* go over code with a comb to ensure we are doing what we should for SeaMonkey

Notes: most of it is near-direct ports from FF impl still, I had to ressurrect some #ifdef MOZ_PLACES stuff from their end.

Posting WIP so people can scan with comments if they wish
Assignee

Updated

12 years ago
Depends on: 401820
Posted image Smaller feed icon (obsolete) —
Smaller feed icon, to fit in with our other 16x16 icons.
Intended locations: suite/themes/*/communicator/icons/feed.png
Attachment #287109 - Flags: superreview?(jag)
Attachment #287109 - Flags: review?(bugspam.Callek)
This is just the back end - nsBrowserStatusHandler.js would need to have an onFeedAvailable method added to update the UI.
Attachment #287210 - Flags: superreview?(jag)
Attachment #287210 - Flags: review?(bugspam.Callek)
You might find the changes to onLinkAdded easier to review with -w.
Assignee

Comment 24

12 years ago
Posted patch Part 1: Feed Discovery (v2) (obsolete) — Splinter Review
Neil asked on irc to produce a version of this without ripping out icon handling from the tabbrowser, so here we go.

(Also changed from previous version is the feed-item.png [was: livemarks-item.png] now resides in navigator/ rather than communicator/)
Attachment #285323 - Attachment is obsolete: true
Attachment #287370 - Flags: superreview?(neil)
Attachment #287370 - Flags: review?(neil)
Attachment #285323 - Flags: superreview?(neil)
Attachment #285323 - Flags: review?(neil)
Assignee

Comment 25

12 years ago
Comment on attachment 287370 [details] [diff] [review]
Part 1: Feed Discovery (v2)

if (relVal == "feed" || relVal == "alternate")

erm should have an '{' at the end of that (thats what I get for submitting patch *as* I go to double check and test it)
Comment on attachment 287210 [details] [diff] [review]
Use tabbrowser to discover feeds

>Index: suite/browser/tabbrowser.xml
>===================================================================
>+                case "text/xml":
>+                case "application/rdf+xml":
>+                case "application/xml":
>+                  isFeed = /(^|\s)rss($|\s)/i.test(title);

Where did |title| come from?

Updated

12 years ago
Attachment #287109 - Flags: superreview?(jag) → superreview+
Intended locations: suite/themes/*/navigator/btn1/feeds.png
Attachment #287102 - Attachment is obsolete: true
This builds on my previous patches. It doesn't completely replace Callek's port but I thought I'd give someone else the chance to implement the URLbar icon.

* Fixes link toolbar CSS
* Maintains list of feeds in window.XULBrowserWindow.feeds
* Enables and populates feed submenu on bookmarks menu
Attachment #287109 - Attachment is obsolete: true
Attachment #287210 - Attachment is obsolete: true
Attachment #287211 - Attachment is obsolete: true
Attachment #287698 - Flags: superreview?(jag)
Attachment #287698 - Flags: review?(bugspam.Callek)
Attachment #287109 - Flags: review?(bugspam.Callek)
Attachment #287210 - Flags: superreview?(jag)
Attachment #287210 - Flags: review?(bugspam.Callek)

Comment 29

12 years ago
(In reply to comment #27)
> Intended locations: suite/themes/*/navigator/btn1/feeds.png

The icons and file name look good, but I dislike the btn1/ subdir for its strange name... Can't we put it in icons/ and later go and put reworked icons for the other icons there in icons/linkToolbar.png or such (in a later step along with finishing up the default theme icon rework)?
Comment on attachment 287370 [details] [diff] [review]
Part 1: Feed Discovery (v2)

>+    XULBrowserWindow.asyncUpdateUI();
Why not just update feeds directly?

>+const DOMLinkHandler = {
Why a separate object?

>+  handleEvent: function (event) {
Why a separate method?

>+    var iconAdded = false;
Not used.

>+    var feedAdded = false;
>+    var relStrings = rel.split(/\s+/);
>+    var rels = {}
>+    for (let i = 0; i < relStrings.length; i++)
>+      rels[relStrings[i]] = true;
>+
>+    for (let relVal in rels) {
>+      // We only care about "feed" and "alternate" here
>+      // for now other rels handled elsewhere
>+      if (relVal == "feed" || relVal == "alternate")
>+        if (feedAdded)
>+          break;
>+        if (!rels.feed && rels.alternate && rels.stylesheet)
>+          break;
This seems overly complex when all you are looking for is either the word "feed" or the word "alternate" except for the words "alternate stylesheet".

>+        var feed = { title: link.title, href: link.href, type: link.type };
>+        if (isValidFeed(feed, link.ownerDocument.nodePrincipal, rels.feed)) {
Why not just pass the link element? Or what is the use case for isValidFeed?

>+          FeedHandler.addFeed(feed, link.ownerDocument);
Again, why not just pass the link element?

>+  onFeedButtonClick: function(event) {
>+    event.stopPropagation();
What does this solve?

>+      // XXX hack -- menu opening depends on setting of an "open"
>+      // attribute, and the menu refuses to open if that attribute is
>+      // set (because it thinks it's already open).
I don't think this has been true since Mozilla 1.7, and it doesn't match with the next return false below.

>+    if (feeds.length == 1) {
>+      var feedButton = document.getElementById("feed-button");
>+      if (feedButton)
>+        feedButton.setAttribute("feed", feeds[0].href);
>+      return false;
>+    }
This is a strange thing to do in a popupshowing handler...

>+   * @param   href
>+   *          The feed to subscribe to. May be null
Always null, as far as I can see...

>+    urlSecurityCheck(gBrowser.contentPrincipal, href,
>+                     Components.interfaces.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);
Haven't you already done this check?

>+    var feedURI = makeURI(href, document.characterSet);
Wrong characterSet, probably.

>+    // Use the feed scheme so X-Moz-Is-Feed will be set
>+    // The value doesn't matter
>+    if (/^https?/.test(feedURI.scheme))
>+      href = "feed:" + href;
So, why only for http(s)?

>+  loadFeed: function(href, event) {
>+    var feeds = gBrowser.selectedBrowser.feeds;
>+    try {
>+      openUILink(href, event, false, true, false, null);
>+    }
>+    finally {
>+      // We might default to a livebookmarks modal dialog, 
>+      // so reset that if the user happens to click it again
>+      gBrowser.selectedBrowser.feeds = feeds;
In what circumstances is this necessary?

>+    var feeds = [];
>+    if (browserForLink.feeds != null)
>+      feeds = browserForLink.feeds;
>+    
>+    feeds.push(feed);
>+    browserForLink.feeds = feeds;
Again, this is long-winded.

>+    if (browserForLink == gBrowser || browserForLink == gBrowser.mCurrentBrowser) {
>+      var feedButton = document.getElementById("feed-button");
>+      if (feedButton) {
>+        feedButton.setAttribute("feeds", "true");
>+        feedButton.setAttribute("tooltiptext", 
>+                                gNavigatorBundle.getString("feedHasFeedsNew"));
>+      }
>+    }
Doesn't this make the pageshow handler unnecessary?

>+            <button type="menu"
>+                    style="-moz-user-focus: none"
>+                    class="plain"
Eww. I'd prefer an image with a click handler that shows a popup.

>+                    chromedir="&locale.dir;"
We don't use chromedir yet.

>+    // See bug 358202, when tabs are switched during a drag operation,
We don't switch tabs during a drag operation, do we?

>+  asyncUpdateUI : function()
>+  {
>+    FeedHandler.updateFeeds();
>   },
Why not just call updateFeeds directly?

>+// Closes all popups that are ancestors of the node.
>+function closeMenus(node)
>+{
>+  if ("tagName" in node) {
Not the right way to check this.

>+#feed-button, #feed-button > .button-box,
>+#feed-button:hover:active > .button-box {
>+  padding: 0px;
>+  margin: 0px;
>+  border: 0px; 
>+  background-color: transparent;
>+}
It hardly seems worth setting type="plain". Sigh.

>+#feed-button {
>+  -moz-binding: url("chrome://global/content/bindings/button.xml#menu");
You'd probably be better off with a custom binding.

>+  -moz-appearance: none; 
Modern doesn't set this anyway. Also you have trailing whitespace.
Attachment #287370 - Flags: superreview?(neil)
Attachment #287370 - Flags: review?(neil)
Attachment #287370 - Flags: review-
Comment on attachment 287698 [details] [diff] [review]
Link toolbar and tabbrowser feed detection

>+                  isFeed = /(^|\s)rss($|\s)/i.test(title);
Oops, this should be event.originalTarget.title of course.
Assignee

Comment 32

12 years ago
(In reply to comment #30)
> (From update of attachment 287370 [details] [diff] [review])
> >+    XULBrowserWindow.asyncUpdateUI();
> Why not just update feeds directly?

umm, FF + Places cruft I missed; will fix.
 
> >+const DOMLinkHandler = {
> Why a separate object?
> 

To make group the functions together (logically) it easier and saner to add-in "DOMLinkRemoved" handling later; (Though I could do this *all* in a followup bug which would also consolidate our DOMLink* handlers at once (see FF version of that in Bug 396203)

> >+  handleEvent: function (event) {
> Why a separate method?

[same as DOMLinkHandler answer]

> This seems overly complex when all you are looking for is either the word
> "feed" or the word "alternate" except for the words "alternate stylesheet".
> 

Fairly said; will consolidate.

> >+        var feed = { title: link.title, href: link.href, type: link.type };
> >+        if (isValidFeed(feed, link.ownerDocument.nodePrincipal, rels.feed)) {
> Why not just pass the link element? Or what is the use case for isValidFeed?

I don't see a need to have the overhead of passing a whole link element here, this just makes complete sense to me (and I can utilize "isValidFeed" easily when I get the MailNews portion of feed handling done; where the user may wish to manually enter a url, etc.)

> >+  onFeedButtonClick: function(event) {
> >+    event.stopPropagation();
> What does this solve?

not entirely sure (was part of the FF portion of this code) I can investigate more if it is important (otherwise I'm inclined to keep it in).

> >+      // XXX hack -- menu opening depends on setting of an "open"
> >+      // attribute, and the menu refuses to open if that attribute is
> >+      // set (because it thinks it's already open).
> I don't think this has been true since Mozilla 1.7, and it doesn't match with
> the next return false below.

I wasn't sure, and since it is *still* part of the FF code, I didn't want to risk it, but the return false makes sense with the feeds == null above it anyway.  But if you are sure I can drop this logic.

> >+    if (feeds.length == 1) {
> >+      var feedButton = document.getElementById("feed-button");
> >+      if (feedButton)
> >+        feedButton.setAttribute("feed", feeds[0].href);
> >+      return false;
> >+    }
> This is a strange thing to do in a popupshowing handler...

...why? given _what action_ is calling the popupshowing here.

> >+   * @param   href
> >+   *          The feed to subscribe to. May be null
> Always null, as far as I can see...

Not necessarily null from extensions; and I don't personally see the harm in keeping FF API parity here (for ease of extension porting)

> >+    urlSecurityCheck(gBrowser.contentPrincipal, href,
> >+                     Components.interfaces.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);
> Haven't you already done this check?

for listing the feed yes; but if href is specified and in the (slimmer) chance the stuff the security check looks at changes why not check it again?

> >+    var feedURI = makeURI(href, document.characterSet);
> Wrong characterSet, probably.

ummm why... http://mxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIProtocolHandler.idl#81

> >+    // Use the feed scheme so X-Moz-Is-Feed will be set
> >+    // The value doesn't matter
> >+    if (/^https?/.test(feedURI.scheme))
> >+      href = "feed:" + href;
> So, why only for http(s)?

why does anything else make sense here? (what other protocols would a _feed_ be expected from?)

> >+  loadFeed: function(href, event) {
> >+    var feeds = gBrowser.selectedBrowser.feeds;
> >+    try {
> >+      openUILink(href, event, false, true, false, null);
> >+    }
> >+    finally {
> >+      // We might default to a livebookmarks modal dialog, 
> >+      // so reset that if the user happens to click it again
> >+      gBrowser.selectedBrowser.feeds = feeds;
> In what circumstances is this necessary?

I suppose none right now, can probably investigate the specifics here when I do implement livebookmarks themselves.

> >+    var feeds = [];
> >+    if (browserForLink.feeds != null)
> >+      feeds = browserForLink.feeds;
> >+    
> >+    feeds.push(feed);
> >+    browserForLink.feeds = feeds;
> Again, this is long-winded.

makes perfect sense to me, got a better idea?

> >+    if (browserForLink == gBrowser || browserForLink == gBrowser.mCurrentBrowser) {
> >+      var feedButton = document.getElementById("feed-button");
> >+      if (feedButton) {
> >+        feedButton.setAttribute("feeds", "true");
> >+        feedButton.setAttribute("tooltiptext", 
> >+                                gNavigatorBundle.getString("feedHasFeedsNew"));
> >+      }
> >+    }
> Doesn't this make the pageshow handler unnecessary?

pageshow is also called with bfcache "forward"; where this method is only called on actual new pageloads (aiui).
 

> >+                    chromedir="&locale.dir;"
> We don't use chromedir yet.

fair enough, removing.

> >+    // See bug 358202, when tabs are switched during a drag operation,
> We don't switch tabs during a drag operation, do we?

Erm yes; for some reason I was still thinking we did.

> >+// Closes all popups that are ancestors of the node.
> >+function closeMenus(node)
> >+{
> >+  if ("tagName" in node) {
> Not the right way to check this.

directly ported from Firefox code, so "what is the right way to check this" then?

> >+#feed-button {
> >+  -moz-binding: url("chrome://global/content/bindings/button.xml#menu");
> You'd probably be better off with a custom binding.

...what use does a custom binding give me here?

> >+  -moz-appearance: none; 
> Modern doesn't set this anyway. Also you have trailing whitespace.

fair enough, removed.

...Please answer my questions so I can get an updated patch your way.

Comment 33

12 years ago
(In reply to comment #30)
> >+                    chromedir="&locale.dir;"
> We don't use chromedir yet.

but, once again, we should. And every place we leave it out makes it more difficult to finally make us not suck on RTL. The way we are going atm with leaving those out probably will need us to deny any RTL languages from being accepted for SeaMonkey L10n, as their old SeaMonkey 1.x way of providing a replacemant theme won't work with the way we are doing L10n in 2.0
(In reply to comment #32)
> > >+const DOMLinkHandler = {
> > Why a separate object?
> > >+  handleEvent: function (event) {
> > Why a separate method?
> To make group the functions together (logically) it easier and saner to add-in
> "DOMLinkRemoved" handling later; (Though I could do this *all* in a followup
> bug which would also consolidate our DOMLink* handlers at once (see FF version
> of that in Bug 396203)
Ah, well now I know where that crufty-looking code comes from... I'm don't think this is the correct way to consolidate; if they are consolidated then they should be added to the tabbrowser as per my patches (!).

> > >+        var feed = { title: link.title, href: link.href, type: link.type };
> > >+        if (isValidFeed(feed, link.ownerDocument.nodePrincipal, rels.feed)) {
> > Why not just pass the link element? Or what is the use case for isValidFeed?
> I don't see a need to have the overhead of passing a whole link element here,
As distinct from the overhead of making a partial clone of the element?
> this just makes complete sense to me (and I can utilize "isValidFeed" easily
> when I get the MailNews portion of feed handling done; where the user may wish
> to manually enter a url, etc.)
I don't see how isValidFeed is useful for manual feed entry.

> > >+  onFeedButtonClick: function(event) {
> > >+    event.stopPropagation();
> > What does this solve?
> not entirely sure (was part of the FF portion of this code) I can investigate
> more if it is important (otherwise I'm inclined to keep it in).
I'm not inclined to keep code that has no effect ;-)

> > >+      // XXX hack -- menu opening depends on setting of an "open"
> > >+      // attribute, and the menu refuses to open if that attribute is
> > >+      // set (because it thinks it's already open).
> > I don't think this has been true since Mozilla 1.7, and it doesn't match with
> > the next return false below.
> I wasn't sure, and since it is *still* part of the FF code, I didn't want to
> risk it, but the return false makes sense with the feeds == null above it
> anyway.  But if you are sure I can drop this logic.
Yes, I'm sure.

> > >+    if (feeds.length == 1) {
> > >+      var feedButton = document.getElementById("feed-button");
> > >+      if (feedButton)
> > >+        feedButton.setAttribute("feed", feeds[0].href);
> > >+      return false;
> > >+    }
> > This is a strange thing to do in a popupshowing handler...
> ...why? given _what action_ is calling the popupshowing here.
As I see it
* User clicks on feed icon
* Popup code tries to open popup
* Popup handler sets feed attribute
* Click handler sees feed attribute
In other words, the popup handler seems to be using internal knowledge of the order of popup processing and click event handling.

> > >+   * @param   href
> > >+   *          The feed to subscribe to. May be null
> > Always null, as far as I can see...
> Not necessarily null from extensions; and I don't personally see the harm in
> keeping FF API parity here (for ease of extension porting)
OK, I guess that makes sense.

> > >+    urlSecurityCheck(gBrowser.contentPrincipal, href,
> > >+                     Components.interfaces.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);
> > Haven't you already done this check?
> for listing the feed yes; but if href is specified and in the (slimmer) chance
> the stuff the security check looks at changes why not check it again?
If the href is specified then we're just guessing that we need to check against the current page, which seems odd, but I guess extensions need this.

> > >+    var feedURI = makeURI(href, document.characterSet);
> > Wrong characterSet, probably.
> ummm why...
document is the XUL document

> > >+    // Use the feed scheme so X-Moz-Is-Feed will be set
> > >+    // The value doesn't matter
> > >+    if (/^https?/.test(feedURI.scheme))
> > >+      href = "feed:" + href;
> > So, why only for http(s)?
> why does anything else make sense here? (what other protocols would a _feed_ be
> expected from?)
Surely if you only expect http(s) feeds you should filter them out earlier?

> > >+  loadFeed: function(href, event) {
> > >+    var feeds = gBrowser.selectedBrowser.feeds;
> > >+    try {
> > >+      openUILink(href, event, false, true, false, null);
> > >+    }
> > >+    finally {
> > >+      // We might default to a livebookmarks modal dialog, 
> > >+      // so reset that if the user happens to click it again
> > >+      gBrowser.selectedBrowser.feeds = feeds;
> > In what circumstances is this necessary?
> I suppose none right now, can probably investigate the specifics here when I do
> implement livebookmarks themselves.

> 
> > >+    var feeds = [];
> > >+    if (browserForLink.feeds != null)
> > >+      feeds = browserForLink.feeds;
> > >+    
> > >+    feeds.push(feed);
> > >+    browserForLink.feeds = feeds;
> > Again, this is long-winded.
> makes perfect sense to me, got a better idea?
One possibility, but there are others:
if (!browserForLink.feeds)
  browserForLink.feeds = [feed];
else
  browserForLink.feeds.push(feed);

> > >+    if (browserForLink == gBrowser || browserForLink == gBrowser.mCurrentBrowser) {
> > >+      var feedButton = document.getElementById("feed-button");
> > >+      if (feedButton) {
> > >+        feedButton.setAttribute("feeds", "true");
> > >+        feedButton.setAttribute("tooltiptext", 
> > >+                                gNavigatorBundle.getString("feedHasFeedsNew"));
> > >+      }
> > >+    }
> > Doesn't this make the pageshow handler unnecessary?
> pageshow is also called with bfcache "forward"; where this method is only
> called on actual new pageloads (aiui).
nsDocument::OnPageShow resends all the DOMLinkAdded events.

> > >+// Closes all popups that are ancestors of the node.
> > >+function closeMenus(node)
> > >+{
> > >+  if ("tagName" in node) {
> > Not the right way to check this.
> directly ported from Firefox code, so "what is the right way to check this"
> then?
The right way is to stop when you reach a known node (null if you must).
I also notice that you use recursion but I'd prefer a loop.

> > >+#feed-button {
> > >+  -moz-binding: url("chrome://global/content/bindings/button.xml#menu");
> > You'd probably be better off with a custom binding.
> ...what use does a custom binding give me here?
It saves you from having to use CSS to turn off all the borders and such.

(In reply to comment #33)
> (In reply to comment #30)
> > >+                    chromedir="&locale.dir;"
> > We don't use chromedir yet.
> but, once again, we should. And every place we leave it out
0% of suite/ uses chromedir, so I'm just being consistent.

Comment 35

12 years ago
Callek asked me over #IRC to add a comment here so that he doesn't forget:

[04:27:19]<Ratty> Callek: "This Page..." The trend is to use an actual ellipse unicode glyph.
04:30:36]<Callek> Ratty: can you do me a favor and comment in the bug about the elipse thing, and I'll be sure to add it in my patch ;-)

See Bug 390282 and Bug 373623 . But watch out for 255051 .

Comment 36

12 years ago
mFeeds: [],

Ugh, that should so be a browser property (does destroy() ring a bell?).
(In reply to comment #36)
> mFeeds: [],
> 
> Ugh, that should so be a browser property (does destroy() ring a bell?).
> 
Which patch?
Assignee

Comment 39

12 years ago
Posted patch Part 1: Feed Discovery (v3) (obsolete) — Splinter Review
heres my updated patch, I don't believe I missed anything (other than things we talked about)

One thing not mentioned between us is that the addFeed and updateFeeds are both needed, because in updateFeeds I learned is needed in pageshow even though the DOMLinkAdded gets called, this is to ensure (at the least) that we activate the menu-items in the bookmarks menu as well.
Attachment #287370 - Attachment is obsolete: true
Attachment #290956 - Flags: superreview?(neil)
Attachment #290956 - Flags: review?(neil)
Assignee

Comment 40

12 years ago
Comment on attachment 287698 [details] [diff] [review]
Link toolbar and tabbrowser feed detection

clearing my own review request here.

Neil this does seem to work ok, though I would prefer to morph this into my own patch as a followup (bug) if that is ok.  And try to reuse some of my own code once my own discovery stuff is done.

If you really want an "official" review of this particular patch, re-request and I'll give it
Attachment #287698 - Flags: review?(bugspam.Callek)
Comment on attachment 287698 [details] [diff] [review]
Link toolbar and tabbrowser feed detection

If you prefer, you can review this in two halves, a) the link toolbar enhancements b) the use of tabbrowser to discover and cache the list of feeds.
Attachment #287698 - Flags: review?(bugspam.Callek)
Assignee

Comment 42

12 years ago
Comment on attachment 290956 [details] [diff] [review]
Part 1: Feed Discovery (v3)

umm, just realized my patch broke the url-bar "dropdown" on SeaMonkey somehow, will have to investigate.  Neil I'd still like a review of the rest of it if you can manage.
Attachment #290956 - Flags: review-
Assignee

Updated

12 years ago
Depends on: 406396
Assignee

Comment 43

12 years ago
Comment on attachment 290956 [details] [diff] [review]
Part 1: Feed Discovery (v3)

fix for new issue filed as Bug 406396
Attachment #290956 - Flags: review-
Comment on attachment 290956 [details] [diff] [review]
Part 1: Feed Discovery (v3)

>+    
>+    FeedHandler.updateFeeds();
Erroneous blank line.

>+  gBrowser.addEventListener("pagehide", FeedHandler.onPageHide, false);
I don't think this is the best time to clear the feeds, but then again I think that should be the tabbrowser's job.

>+const DOMLinkHandler = {
Why (assuming we still keep it) isn't this part of FeedHandler?

>+    var rel = link.rel && link.rel.toLowerCase();
link.rel is always a string, so it's safe to lower case.

>+      if (isValidFeed(feed, link.ownerDocument.nodePrincipal, rels.feed)) {
>+        FeedHandler.addFeed(feed, link.ownerDocument);
>+        return;
Useless return.

>+    if (feeds.length == 1) {
>+      var feedButton = document.getElementById("feed-button");
>+      if (feedButton)
>+        feedButton.setAttribute("feed", feeds[0].href);
>+      return false;
>+    }
If anything this is back to front. Rather than cancelling the popup if there is only one feed, a mouse down on the feed image should open the popup if there are two or more feeds.

>+      var feedInfo = feeds[i];
Nit: you called this a "feed" earlier...

>+    if (!href)
This is always true, unless you can point to an existing caller.

>+    urlSecurityCheck(gBrowser.contentPrincipal, href,
>+                     Components.interfaces.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);
Didn't you already check this once? What's going to change?

>+    var feedURI = makeURI(href, gBrowser.contentDocument.characterSet);
Don't make a URI just to check a scheme, there are other methods for that.

>+  loadFeed: function(href, event) {
Who else is using this API?

>+      openUILink(href, event, false, true, false, null);
Just use openAsExternal for now.

>+      // We might default to a livebookmarks modal dialog, 
>+      // so reset that if the user happens to click it again
>+      gBrowser.selectedBrowser.feeds = feeds;
Do you know what would clear this?

>+      if (feedButton) {
>+        feedButton.setAttribute("feeds", "true");
>+        feedButton.setAttribute("tooltiptext", 
>+                                gNavigatorBundle.getString("feedHasFeedsNew"));
>+      }
Doesn't addFeed already do this?

>+        if (feedButton)
>+          feedButton.removeAttribute("feed");
Can't you assume that the feedButton always exists?

>+        this._feedMenuitem.setAttribute("feed", feeds[0].href);
Shouldn't you set the label to "Subscribe to 'News'..." (or whatever)?

>+  addFeed: function(feed, targetDoc) {
>+    if (!feed)
>+      return;
How can this happen?

>+    var browserForLink = getBrowserForDocument(targetDoc);
Depending on how well the popup notification box works, we might be removing this function. As you know I'd prefer tabbrowser to cache the feeds anyway.

>+<!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd" >
>+%globalDTD;
Unused?

>+                    onclick="return FeedHandler.onFeedButtonClick(event);">
I'm still not keen on this dual use. It's not even obvious that it does something immediately if you only have the one feed. 

>+        <menu id="subscribeToPageMenupopup"
>+              label="&subscribeToPageMenupopup.label;"
Except it's not a menupopup, is it? Also, an accesskey would be nice.

>+    FeedHandler.updateFeeds();
It looks as if the pageshow call is used to set up the menu(item), while this one resets the menu(item) back to default. It seems a bit silly to use the same function for two different things. Why not use addFeed to set up the menu(item) as well as the icon?

>+    /* Execute the node's oncommand.
>+     *
>+     * XXX: we should use node.oncommand(event) once bug 246720 is fixed.
>+     */
>+    var fn = new Function("event", node.getAttribute("oncommand"));
>+    fn.call(node, event);
Nit: the right way to execute an oncommand is either to call node.doCommand() or to create and dispatch a command event, although of course that won't open anything in a tab, because you're no longer passing the mouse event to your command handler (and why is it expecting a mouse event anyway?). The right way to handle this is not to have a generic middle click handler, but write onclick="return FeedHandler.subscribeToFeedMiddleClick(event);"

>+// Closes all popups that are ancestors of the node.
>+function closeMenus(node)
>+{
>+  var currNode = node;
>+  do {
>+    if ("tagName" in currNode &&
>+        currNode.namespaceURI == "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+        && (currNode.tagName == "menupopup" || currNode.tagName == "popup"))
>+      currNode.hidePopup();
>+
>+  } while (currNode = currNode.parentNode);
>+}
I think the version in BookmarksMenu.js might be simpler.

>+      const titleRegex = /(^|\s)rss($|\s)/i;
>+      aIsFeed = ((type == "text/xml" || type == "application/rdf+xml" ||
>+                  type == "application/xml") && titleRegex.test(aData.title));
Apparently regexes now have to be evaluted on each use :-( Please inline it.

>\ No newline at end of file
Nit: oops!

>+# RSS Pretty Print
Discovery, no?

>+feedNoFeeds=Page has no feeds
when do you see this?

>+#feed-button, #feed-button > .button-box,
>+#feed-button:hover:active > .button-box {
>+  padding: 0px;
>+  margin: 0px;
>+  border: 0px; 
>+  background-color: transparent;
>+}
>+
>+#feed-button > hbox > .button-menu-dropmarker,
>+#feed-button > hbox > .button-text {
>+  display: none;
>+}
>+
>+#feed-button {
>+  -moz-binding: url("chrome://global/content/bindings/button.xml#menu");
You shouldn't need this, because you're using type="menu". That said, the big problem with this binding is all the extra styles you need to turn off things you aren't using...

>+  -moz-appearance: none;
>+  min-width: 0px; 
>+  margin-right: 1px !important;
>+}

>+#feed-button[feeds] {
>+  -moz-image-region: rect(0px, 36px, 18px, 18px);
>+  list-style-image: url("chrome://navigator/skin/icons/feed-item.png");
Nit: Please put the image first and the region second.

>+#feed-button[chromedir="rtl"][feeds] {
We're not using this, are we?

>+#urlbar-icons {
>+  height: 18px;
>+}
This is nasty, because it makes the whole URLbar taller...

I also notice that the feed button doesn't look right in Modern.

Updated

12 years ago
Attachment #290956 - Flags: superreview?(neil)
Attachment #290956 - Flags: superreview-
Attachment #290956 - Flags: review?(neil)
Attachment #290956 - Flags: review-
Note that the UI is subtly different from Callek's patch, mainly because I didn't want to write UI without someone arguing the case for it first!
Attachment #287698 - Attachment is obsolete: true
Attachment #292077 - Flags: superreview?(jag)
Attachment #292077 - Flags: review?(bugspam.Callek)
Attachment #287698 - Flags: superreview?(jag)
Attachment #287698 - Flags: review?(bugspam.Callek)
Assignee

Comment 46

12 years ago
This iterates attachment 292077 [details] [diff] [review], which was Neil's patch (so sr? is jag)

If this suites everyone, I will file a follow up bug for a "one-click" solution one sites that have only one feed.

And another follow up to investigate if we need some API-Parity to Firefox here (as my attempt started out).

[Note: even with reviews, this probably should not land until "Part 2" is done]
Attachment #290956 - Attachment is obsolete: true
Attachment #292077 - Attachment is obsolete: true
Attachment #297312 - Flags: superreview?
Attachment #297312 - Flags: review?
Attachment #292077 - Flags: superreview?(jag)
Attachment #292077 - Flags: review?(bugspam.Callek)
Assignee

Updated

12 years ago
Attachment #297312 - Flags: superreview?(jag)
Attachment #297312 - Flags: superreview?
Attachment #297312 - Flags: review?(neil)
Attachment #297312 - Flags: review?
Comment on attachment 297312 [details] [diff] [review]
v2: Link toolbar, tabbrowser feed detection, feed menu and icon

r=me excluding the feed button changes that are working around a version of toolbar customisation that might not actually land...
Attachment #297312 - Flags: review?(neil) → review+
Comment on attachment 297312 [details] [diff] [review]
v2: Link toolbar, tabbrowser feed detection, feed menu and icon

Neil:

+        urlSecurityCheck(element.nodePrincipal, element.href, Components.interfaces.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);

Why is our order of the principal and the url different from Firefox's?

I guess I missed that when I reviewed the patch on bug 394471.

suite/browser/pageinfo/feeds.js#132 uses Firefox's order with our |urlSecurityCheck(...)|.
Ah, I see, it's because checkLoadURI(WithPrincipal) use that order.

Maybe we can convince the Firefox folks to change theirs?
*checkLoadURIWithPrincipal() uses*
Comment on attachment 297312 [details] [diff] [review]
v2: Link toolbar, tabbrowser feed detection, feed menu and icon

>Index: suite/browser/linkToolbarHandler.js
>===================================================================

>+    case "alternate":
>+      if (!element.type)
>+        return "alternate";
>+
>+      var type = element.type.toLowerCase().replace(/^\s+|\s*(?:;.*)?$/g, "");
>+      switch (type) {
>+        case "application/rss+xml":
>+        case "application/atom+xml":
>+          break;
>+
>+        case "text/xml":
>+        case "application/rdf+xml":
>+        case "application/xml":
>+          if (/\brss\b/i.test(element.title))
>+            break;
>+
>+        default:
>+          return "alternate";
>+      }
>+      // fall through
>+    case "feed":
>+      try {
>+        urlSecurityCheck(element.nodePrincipal, element.href, Components.interfaces.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);
>+        return "feed";
>+      } catch (e) {}
>+      // fall through

This code's logic is (nearly*) the same as the logic in recognizeFeedFromLink in suite/browser/pageinfo/feeds.js and tabbrowser's |onLinkAdded|. Can (some of) it be refactored and shared?

* Except that you're checking for "?:;.*" and the logic in feeds.js is checking for ";.*". Is the difference intentional?

     
>Index: suite/browser/nsBrowserStatusHandler.js
>===================================================================

>@@ -73,6 +74,7 @@
>     this.statusTextField = document.getElementById("statusbar-display");
>     this.isImage         = document.getElementById("isImage");
>     this.securityButton  = document.getElementById("security-button");
>+    this.feedsMenu       = document.getElementById("feedsMenu");
> 
>     // Initialize the security button's state and tooltip text
>     const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;

You're clearing feedsButton below without initializing it up here. I would say replace the |var feedsButton = ...| code below with |this.feedsButton| and just initialize it once up here.

>@@ -92,6 +94,8 @@
>     this.statusTextField = null;
>     this.isImage         = null;
>     this.securityButton  = null;
>+    this.feedsButton     = null;
>+    this.feedsMenu       = null;
>   },

---


>+  onFeedAvailable : function(aLink)
>+  {
>+    this.feeds.push(aLink);
>+    this.feedsMenu.removeAttribute("disabled");
>+    var feedsButton = document.getElementById("feedsButton");
>+    if (feedsButton)
>+      feedsButton.hidden = false;
>+  },

There's no need to null-check feedsButton. And of course you should just use |this.feedButton| with the change made for my previous comment.


>@@ -343,6 +372,12 @@
>         this.urlBar.value = userTypedValue;
>         SetPageProxyState("invalid", null);
>       }
>+
>+      this.feedsMenu.setAttribute("disabled", "true");
>+      var feedsButton = document.getElementById("feedsButton");
>+      if (feedsButton)
>+        feedsButton.hidden = true;
>+      this.feeds = [];

Same here.


>Index: suite/browser/tabbrowser.xml
>===================================================================

>             mBrowser: aBrowser,
>             mBlank: aStartsBlank,
>             mIcon: "",
>+            mFeeds: [],

Instead of keeping track of the feeds here and in BrowserStatusHandler, could you add a feeds array field to <browser> and have both places use that?


>@@ -857,68 +868,97 @@

... *snip*

>             for (var i = 0; i < this.browsers.length; i++) {
>+              if (this.browsers[i].contentDocument != event.originalTarget.ownerDocument)
>+                continue;
>+              
>+              // Verify that the load of this href is legal.
>+              // We check first with the security manager
>+              const nsIScriptSecurityManager =
>+                Components.interfaces.nsIScriptSecurityManager;
>+
>+              const secMan =
>+                Components.classes["@mozilla.org/scriptsecuritymanager;1"]
>+                          .getService(nsIScriptSecurityManager);
>+
>+              // Get the IOService so we can make URIs
>+              const ioService =
>+                Components.classes["@mozilla.org/network/io-service;1"]
>+                          .getService(Components.interfaces.nsIIOService);
>+
>+              const targetDoc = event.target.ownerDocument;

Why are you doing this work inside the loop? Oh, except you're not really doing it more than once. We |continue| for the ones that the event wasn't for, so we never get here, and once we've found the browser there's no need to check the others so we return, so this code never gets executed more than once. This is the second time I had to work that out. Maybe add a comment?


>Index: suite/locales/en-US/chrome/browser/navigator.dtd
>===================================================================

>+<!ENTITY feedsMenu.label "Subscribe to This Page">
>+<!ENTITY feedsMenu.accesskey "S">

What's the rule again on capitalizing "to"? We seem to do both "To" and "to" in various places.
Attachment #297312 - Flags: superreview?(jag) → superreview-

Comment 52

12 years ago
(In reply to Comment 51)
> There's no need to null-check feedsButton. And of course you should just use
> |this.feedButton| with the change made for my previous comment.

This was presupposed on the customizable toolbars patch landing in it's current form. The way Firefox does customizable toolbars means that the feeds button may not always be in the DOM. However see comment 47
Ah, right.
(In reply to comment #48)
>Why is our order of the principal and the url different from Firefox's?
Because it didn't occur to me to copy their code?

>suite/browser/pageinfo/feeds.js#132 uses Firefox's order with our
>|urlSecurityCheck(...)|.
Oops...

(In reply to comment #49)
>Maybe we can convince the Firefox folks to change theirs?
Well, having different APIs will surely confuse extensions...

(In reply to comment #51)
>(From update of attachment 297312 [details] [diff] [review])
>>Index: suite/browser/linkToolbarHandler.js
>This code's logic is (nearly*) the same as the logic in recognizeFeedFromLink
>in suite/browser/pageinfo/feeds.js and tabbrowser's |onLinkAdded|. Can (some
>of) it be refactored and shared?
Sharing between page info and tabbrowser would be tricky as they're separate windows, but page info and link toolbar could share it in contentAreaUtils.

>* Except that you're checking for "?:;.*" and the logic in feeds.js is checking
>for ";.*". Is the difference intentional?
Inside ()s, the ?: is the non-capturing syntax.

>>+            mFeeds: [],
>Instead of keeping track of the feeds here and in BrowserStatusHandler, could
>you add a feeds array field to <browser> and have both places use that?
We don't own <browser>s.

>This is the second time I had to work that out.
Callek made this change; I thought it possibly looked simpler; we were wrong!

>>+<!ENTITY feedsMenu.label "Subscribe to This Page">
>>+<!ENTITY feedsMenu.accesskey "S">
>What's the rule again on capitalizing "to"?
Don't captialise bit words.

> We seem to do both "To" and "to" in various places.
Then that's probably a bug.
>Inside ()s, the ?: is the non-capturing syntax.
Doh! I knew that!

>>Instead of keeping track of the feeds here and in BrowserStatusHandler, could
>>you add a feeds array field to <browser> and have both places use that?
>We don't own <browser>s.
Bah! So can we make this proposal to the toolkit owner? Surely they could benefit from it too.

>>This is the second time I had to work that out.
>Callek made this change; I thought it possibly looked simpler; we were wrong!
Just add a comment explaining that it's ok 'coz the code only runs once.
Assignee

Comment 56

12 years ago
porting Neil's r+

isValidFeed's api is being copied from Firefox, I figure if we have a shared API for this, we might as well use the existing one (for extension compat, if needed)

While I was in pageinfo/ I chose to enable the subscribe buttons that were commented out there, easier to do it now.

Other nits (as clarified on irc) are addressed.
Attachment #297312 - Attachment is obsolete: true
Attachment #298230 - Flags: superreview?
Attachment #298230 - Flags: review+
Assignee

Updated

12 years ago
Attachment #298230 - Flags: superreview? → superreview?(jag)
Assignee

Comment 57

12 years ago
Comment on attachment 298230 [details] [diff] [review]
v3: Link toolbar, tabbrowser feed detection, feed menu and icon

after realizing I accidentally left in code to hide the feeds tab, that db48x explicitly left out, I figured I might as well r? him...

That said, I'm inclined to have it hide that tab, as if a page doesn't have feeds, I personally don't see the value in offering a "feeds" tab, _that is completely empty_, for that page's info.

(db48x: feel free to review other parts, but I'm r?-ing you for just the ok/not-ok on keeping the hide feeds tab in)
Attachment #298230 - Flags: review?(db48x)
Not sure whether I like this better or not:

  relAttribute = relAttribute.toLowerCase();
  switch(relAttribute) {

    //...

    case "alternate":
    case "feed":
      var feed = { title: element.title, href: element.href,
                   type: element.type || "" };
      if (isValidFeed(feed, element.nodePrincipal, relAttribute == "feed")) {
        return "feed";
      }

      if (relAttribute == "alternate") {
        return "alternate";
      }
      // fall through
    case "prefetch":
      return null;

If you don't want to use that, here are a few nits:

There's no point in having || "" after element.type, you bail early on !element.type. And in the "feed" case you only need the href, so let's not bother passing in the type and title. Minor nit: missing space between ) and { on the isValidFeed() lines.

More (maybe) in a bit.
Slightly better:

    case "feed":
      var relAttrIsFeed = true; // better name?
    case "alternate":
      var feed = { title: element.title, href: element.href,
                   type: element.type || "" };
      if (isValidFeed(feed, element.nodePrincipal, relAttrIsFeed)) {
        return "feed";
      }

      if (!relAttrIsFeed) {
        return "alternate";
      }
      // fall through
    case "prefetch":
      return null;

And to handle "alternate XXX", do something like:

  for (var i = 0; i < linkElement.relValues.length; i++) {
    var rel = linkElement.relValues[i];
    if (linkElement.relValues.length > 1 && rel == "alternate")
      continue; // skip "alternate" when we have "alternate XXX"

    var linkType = LinkToolbarHandler.getLinkType(rel);
    if (linkType)
      // etc.

Other than that your patch looks good. Test these suggested changes, attach a new patch, maybe have Neil give it a quick look, and request sr from me.
Comment on attachment 298230 [details] [diff] [review]
v3: Link toolbar, tabbrowser feed detection, feed menu and icon

I'm ok with hiding the tab, though another possibility would be to explicitly say that there are no feeds. Leaving the tab empty is obviously the worst of the three.

>+    if (rels.feed || (link.type && rels.alternate && !rels.stylesheet)) {
>+      var feed = { title: link.title, href: link.href, type: link.type };
>+      if (isValidFeed(feed, gDocument.nodePrincipal, rels.feed)) {
>+        var type = feed.type;
>+        if (type in feedTypes)
>+          type = feedTypes[type];
>+        else
>+          type = feedTypes["application/rss+xml"];
>+        addRow(feed.title, type, feed.href);

It's just a nit, but I'd ommit the feed variable entirely, and just use link.whatever in it's place.

r=db48x
Attachment #298230 - Flags: review?(db48x) → review+
(In reply to comment #60)
>(From update of attachment 298230 [details] [diff] [review])
>I'd omit the feed variable entirely, and just use link.whatever in it's place.
IsValidFeed modifies its first argument. Sucky, isn't it?
Comment on attachment 298230 [details] [diff] [review]
v3: Link toolbar, tabbrowser feed detection, feed menu and icon

>+      if (isValidFeed(feed, element.nodePrincipal, false)){
>+        return "feed";
>+      } else {
>+        return "alternate";
>+      }
Use ?:

>+      if (isValidFeed(feed, element.nodePrincipal, true)){
>+        return "feed";
>+      }
Possibly use ?: here instead of falling through.
If not, no {}s please.

>+<!ENTITY feedsMenu.label "Subscribe to This Page">
>+<!ENTITY feedsMenu.accesskey "S">
This only makes sense for a menuitem (when you only have one feed).

>+    var type = aData.type && aData.type.toLowerCase();
Type should never be empty here, should it?

>+    aIsFeed = (type == "application/rss+xml" ||
>+               type == "application/atom+xml");
>+
>+    if (!aIsFeed) {
>+      // really slimy: general XML types with magic letters in the title
>+      const titleRegex = /(^|\s)rss($|\s)/i;
>+      aIsFeed = ((type == "text/xml" || type == "application/rdf+xml" ||
>+                  type == "application/xml") && titleRegex.test(aData.title));
>+    }
Please use the switch test as used in tabbrowser and "original" link toolbar.

>+  if (type)
>+    aData.type = type;
Grrr, I need to ask someone toolkitty about this...

>+    if (rels.feed || (link.type && rels.alternate && !rels.stylesheet)) {
Can we use the regexp tests here please?

>+  openTopWin(listbox.selectedItem.getAttribute("feedURL"), null);
Don't need to explicitly specify a null opener here.
Assignee

Comment 63

12 years ago
Porting Neil (and db48x's) review+

This patch addresses jag's review comments, and Neil's latest nits...

(I thought I had attached this days ago, I guess not)
Attachment #298230 - Attachment is obsolete: true
Attachment #300601 - Flags: superreview?(jag)
Attachment #300601 - Flags: review+
Attachment #298230 - Flags: superreview?(jag)

Updated

12 years ago
Attachment #300601 - Flags: superreview?(jag) → superreview+
Comment on attachment 300601 [details] [diff] [review]
v4: Link toolbar, tabbrowser feed detection, feed menu and icon

>   switch (relAttribute.toLowerCase()) {
...
>+    case "feed":
>+      var isFeed = true;
>+      //Fall Through
>+    case "alternate":
Eww...

>+      var feed = { title: element.title, href: element.href,
>+                    type: element.type || "" };
Don't need the || "" - element.type never returns null.
Comment on attachment 300601 [details] [diff] [review]
v4: Link toolbar, tabbrowser feed detection, feed menu and icon

>+    var type = aData.type && aData.type.toLowerCase();
>+    type = type.replace(/^\s+|\s*(?:;.*)?$/g, "");
aData.type is never null either, so you can just write
var type = aData.type.toLowerCase().replace(/^\s+|\s*(?:;.*)?$/g, "");
Comment on attachment 300601 [details] [diff] [review]
v4: Link toolbar, tabbrowser feed detection, feed menu and icon

>   switch (relAttribute.toLowerCase()) {
...
>+    case "feed":
>+      var isFeed = true;
>+      //Fall Through
>+    case "alternate":
var isFeed = false;
switch (relAttribute.toLowerCase()) {
...
  case "feed":
    isFeed = true;
    // fall through

(note also that fall through is in lower case and has a space after //)
Assignee

Comment 67

12 years ago
address review nits, ready for checkin

needs a cvs add [binary] of attachment 287696 [details] in two locations
suite/themes/modern/navigator/btn1/feeds.png
suite/themes/classic/navigator/btn1/feeds.png

r+=db48x, r+=Neil, sr+=jag
Attachment #300601 - Attachment is obsolete: true
Attachment #300877 - Flags: superreview+
Attachment #300877 - Flags: review+
Assignee

Comment 68

12 years ago
Comment on attachment 300877 [details] [diff] [review]
v4.1: Link toolbar, tabbrowser feed detection, feed menu and icon

requesting approval 1.9b3, this is entirely suite/ only.
Attachment #300877 - Flags: approval1.9b3?
Comment on attachment 300877 [details] [diff] [review]
v4.1: Link toolbar, tabbrowser feed detection, feed menu and icon

a=beltzner, this is NPOTB for Firefox, but thanks for asking!
Attachment #300877 - Flags: approval1.9b3? → approval1.9b3+
Assignee

Updated

12 years ago
Keywords: checkin-needed
Whiteboard: see c#67-69 for checkin-needed
Attachment #300877 - Flags: approval1.9b3+
Checking in suite/browser/linkToolbarHandler.js;
/cvsroot/mozilla/suite/browser/linkToolbarHandler.js,v  <--  linkToolbarHandler.js
new revision: 1.9; previous revision: 1.8
done
Checking in suite/browser/linkToolbarOverlay.js;
/cvsroot/mozilla/suite/browser/linkToolbarOverlay.js,v  <--  linkToolbarOverlay.js
new revision: 1.17; previous revision: 1.16
done
Checking in suite/browser/linkToolbarOverlay.xul;
/cvsroot/mozilla/suite/browser/linkToolbarOverlay.xul,v  <--  linkToolbarOverlay.xul
new revision: 1.18; previous revision: 1.17
done
Checking in suite/browser/navigator.xul;
/cvsroot/mozilla/suite/browser/navigator.xul,v  <--  navigator.xul
new revision: 1.457; previous revision: 1.456
done
Checking in suite/browser/navigatorOverlay.xul;
/cvsroot/mozilla/suite/browser/navigatorOverlay.xul,v  <--  navigatorOverlay.xul
new revision: 1.333; previous revision: 1.332
done
Checking in suite/browser/nsBrowserStatusHandler.js;
/cvsroot/mozilla/suite/browser/nsBrowserStatusHandler.js,v  <--  nsBrowserStatusHandler.js
new revision: 1.75; previous revision: 1.74
done
Checking in suite/browser/tabbrowser.xml;
/cvsroot/mozilla/suite/browser/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.193; previous revision: 1.192
done
Checking in suite/browser/pageinfo/feeds.js;
/cvsroot/mozilla/suite/browser/pageinfo/feeds.js,v  <--  feeds.js
new revision: 1.2; previous revision: 1.1
done
Checking in suite/browser/pageinfo/feeds.xml;
/cvsroot/mozilla/suite/browser/pageinfo/feeds.xml,v  <--  feeds.xml
new revision: 1.2; previous revision: 1.1
done
Checking in suite/browser/pageinfo/pageInfo.xul;
/cvsroot/mozilla/suite/browser/pageinfo/pageInfo.xul,v  <--  pageInfo.xul
new revision: 1.5; previous revision: 1.4
done
Checking in suite/common/utilityOverlay.js;
/cvsroot/mozilla/suite/common/utilityOverlay.js,v  <--  utilityOverlay.js
new revision: 1.110; previous revision: 1.109
done
Checking in suite/locales/en-US/chrome/browser/linkToolbar.dtd;
/cvsroot/mozilla/suite/locales/en-US/chrome/browser/linkToolbar.dtd,v  <--  linkToolbar.dtd
new revision: 1.6; previous revision: 1.5
done
Checking in suite/locales/en-US/chrome/browser/navigator.dtd;
/cvsroot/mozilla/suite/locales/en-US/chrome/browser/navigator.dtd,v  <--  navigator.dtd
new revision: 1.184; previous revision: 1.183
done
Checking in suite/themes/classic/jar.mn;
/cvsroot/mozilla/suite/themes/classic/jar.mn,v  <--  jar.mn
new revision: 1.173; previous revision: 1.172
done
Checking in suite/themes/classic/navigator/linkToolbar.css;
/cvsroot/mozilla/suite/themes/classic/navigator/linkToolbar.css,v  <--  linkToolbar.css
new revision: 1.5; previous revision: 1.4
done
Checking in suite/themes/classic/navigator/navigator.css;
/cvsroot/mozilla/suite/themes/classic/navigator/navigator.css,v  <--  navigator.css
new revision: 1.42; previous revision: 1.41
done
RCS file: /cvsroot/mozilla/suite/themes/classic/navigator/btn1/feeds.png,v
done
Checking in suite/themes/classic/navigator/btn1/feeds.png;
/cvsroot/mozilla/suite/themes/classic/navigator/btn1/feeds.png,v  <--  feeds.png
initial revision: 1.1
done
Checking in suite/themes/modern/jar.mn;
/cvsroot/mozilla/suite/themes/modern/jar.mn,v  <--  jar.mn
new revision: 1.182; previous revision: 1.181
done
Checking in suite/themes/modern/navigator/linkToolbar.css;
/cvsroot/mozilla/suite/themes/modern/navigator/linkToolbar.css,v  <--  linkToolbar.css
new revision: 1.5; previous revision: 1.4
done
Checking in suite/themes/modern/navigator/navigator.css;
/cvsroot/mozilla/suite/themes/modern/navigator/navigator.css,v  <--  navigator.css
new revision: 1.100; previous revision: 1.99
done
RCS file: /cvsroot/mozilla/suite/themes/modern/navigator/btn1/feeds.png,v
done
Checking in suite/themes/modern/navigator/btn1/feeds.png;
/cvsroot/mozilla/suite/themes/modern/navigator/btn1/feeds.png,v  <--  feeds.png
initial revision: 1.1
done
Status: NEW → RESOLVED
Closed: 15 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: see c#67-69 for checkin-needed

Comment 71

12 years ago
Callek, given the size this bug has now already, could you split off the other parts into separate bugs? That way, the FIXED here is also correct ;-)
Assignee

Comment 72

12 years ago
Kairo, no problem.

To everyone CC-ed here, sorry for the "Bug hijack".

This "Livemarks" itself is now Bug 415373, the "Feed Preview" part is Bug 415372
Blocks: 415372, 415373
Summary: port Firefox Live Bookmarks / RSS reader to SeaMonkey (RSS-feed integration with bookmarks) → Implement Web Page Feed Detection

Updated

12 years ago
Depends on: 416136

Updated

12 years ago
Depends on: 416193
Assignee

Updated

12 years ago
Depends on: 416457

Updated

12 years ago
Depends on: 417309
Assignee

Updated

11 years ago
Depends on: 450205

Updated

11 years ago
Blocks: 454072
Assignee

Updated

11 years ago
Blocks: 465258

Updated

10 years ago
Depends on: 484371

Updated

9 years ago
Component: Bookmarks → Feed Discovery and Preview
QA Contact: bookmarks → feed.handling
You need to log in before you can comment on or make changes to this bug.