RSS feed support (part 1)...

RSS feed support (part 1)

Part 1 is RSS feed auto-discovery. 

This will parse the web page header to find the feeds in the document and notify
the browser to display the RSS icon in the toolbar. You can click on the icon,
but it will just navigate to the first feed on the page, which (unless it has
been designed to be browser friendly) will just dump XML as text on the user.

For this reason I have disabled the code that makes the RSS icon appear and
intend to enable it when we have a good landing page to display the XML.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=11640

[Finnur, 2009-03-12 00:03:48]

This changelist has been lingering in my local tree while we have been putting
all our effort in the effort stabilize the trunk and I'm getting so tired of
fixing the bitrot as people move things around. I think this is also a good time
to checkpoint since the first part of this effort is working (detecting RSS
feeds on the page).

This changelist is the culmination of a patch from an external contributor
(Jorat), our existing code that we took out and new code from me.

[Finnur, 2009-03-12 00:10:48]

> On Wed, Mar 11, 2009 at 16:43, <jorat1346@gmail.com> wrote:
> A question: Why you have implemented the rss icon as a 
> derivate from ssl icon? This limits you to display only one 
> feed at a time. Do you plan to implement some new stuff so we
> can access to all available feeds?

Yes. There is a UI proposal to have an icon and a chevron menu for other feeds.

> When I first did my code, I was doing the exact same thing as
> you. Forever, I changed my mind to make it as a dropdown menu.
> This way, the dropdown menu will display all available feeds
> for a given web page (This page give me 5 feeds from which I 
> can chose and select any of those).

> If you haven't try my code in its full version, I have made
> a design page with few ss that show how it feels and looks:
> http://sites.google.com/site/mychromium/atom-rss-feed-detector-style

> Also, one thing that I loved from my solution is that when 
> your mouse was over the rss icon, it was turning like a button.
> This way, I think it's more easy to understand that you can click
> on it and there will be something that will happen :P  The ssl 
> icon should have the same feels and looks in my mind.

I did try the full version of your code and I didn't like the fact that the RSS
icon behaved differently than the SSL icon. Since we haven't made a decision to
turn the SLL icon into a button that changes state when you hover over it I
decided consistency was better. It should be easy to convert both buttons if we
need to.

> For everything else, well, gj!

Well... thanks. :)

[Evan Martin, 2009-03-12 00:32:45]

Higher level:
is "common" the best directory for this?
what happened to this being covered by extensions?

http://codereview.chromium.org/43109/diff/32/43
File chrome/browser/tab_contents/web_contents.cc (right):

http://codereview.chromium.org/43109/diff/32/43#newcode815
Line 815: GetSiteInstance(),
Is this the correct site instance?  What if you get this message async after
you've swapped site instances?

http://codereview.chromium.org/43109/diff/32/53
File chrome/browser/toolbar_model.h (right):

http://codereview.chromium.org/43109/diff/32/53#newcode11
Line 11: #include "chrome/common/feed.h"
Can you forward-declare FeedList here and elsewhere to avoid header
proliferation?

http://codereview.chromium.org/43109/diff/32/47
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/43109/diff/32/47#newcode393
Line 393: location_y + 1,
Why +1 here?

http://codereview.chromium.org/43109/diff/32/47#newcode531
Line 531: void LocationBarView::SetRssIconVisibility(FeedList* feeds) {
Should this be a const ref&?

http://codereview.chromium.org/43109/diff/32/54
File chrome/common/feed.h (right):

http://codereview.chromium.org/43109/diff/32/54#newcode16
Line 16: std::wstring type;
This needs comments -- particularly "type".

http://codereview.chromium.org/43109/diff/32/54#newcode28
Line 28: std::vector<FeedItem>& List() {
lowercase "list()" for an accessor.  should be conster too:

const std::vector<FeedItem>& list() const {

http://codereview.chromium.org/43109/diff/32/55
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/43109/diff/32/55#newcode54
Line 54: // The list of available feed.
grammar: "feeds"

http://codereview.chromium.org/43109/diff/32/55#newcode549
Line 549: if (!ReadParam(m, iter, &arraysize))
Do we worry about overflows if this is too large?

http://codereview.chromium.org/43109/diff/32/55#newcode565
Line 565: static void Log(const param_type& p, std::wstring* l) {
one-letter variable names make the style guide cry.

http://codereview.chromium.org/43109/diff/32/35
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/43109/diff/32/35#newcode133
Line 133: // Return the URL to the WebSyndicationFormats document for the frame.
If
WebSyndicationFormats sounds like a vestigial term?

[Jo, 2009-03-12 00:50:48]

I will have loved to get comments like those, héhé! :D

http://codereview.chromium.org/43109/diff/32/43
File chrome/browser/tab_contents/web_contents.cc (right):

http://codereview.chromium.org/43109/diff/32/43#newcode815
Line 815: GetSiteInstance(),
On 2009/03/12 00:32:45, Evan Martin wrote:
> Is this the correct site instance?  What if you get this message async after
> you've swapped site instances?

I guess. I did the same thing as in the function above
(WebContents::UpdateTitle) and I take it as "working". Can you maybe explain
more? If finnur understand what you want to mean, just leave it.

http://codereview.chromium.org/43109/diff/32/53
File chrome/browser/toolbar_model.h (right):

http://codereview.chromium.org/43109/diff/32/53#newcode11
Line 11: #include "chrome/common/feed.h"
On 2009/03/12 00:32:45, Evan Martin wrote:
> Can you forward-declare FeedList here and elsewhere to avoid header
> proliferation?

Is it possible to use scoped_refptr<> with a local declaration of a
class/struct?

[Finnur, 2009-03-12 21:34:23]

We are still waiting on word about the traits Read/Write functions, but in the
meantime - here is the rest.

High level comments first:

> is "common" the best directory for this?

Probably not. I moved the .h file to webkit\glue, since it is used in webkit,
assuming dependencies from chrome to webkit are ok, but the reverse is not. This
is now similar to how autofill_form.h was included.

> what happened to this being covered by extensions?

This is the first I hear of it. This effort is one of the most requested feature
from our users, is one of my goals for this quarter and nobody has notified me
that this is on hold. 

I think it is worth pursuing in the absence of an extension system, even though
it will get replaced by an extension later on. It is easy enough to delete from
the product.

http://codereview.chromium.org/43109/diff/32/43
File chrome/browser/tab_contents/web_contents.cc (right):

http://codereview.chromium.org/43109/diff/32/43#newcode815
Line 815: GetSiteInstance(),
That is a question for Charlie Reis, since he was the one who implemented the
site instances and added the GetSiteInstance() call to UpdateTitle. I don't
recall problems due to this.

As Jo mentioned, he modeled this function after the UpdateTitle function so it
should work the same way...

http://codereview.chromium.org/43109/diff/32/53
File chrome/browser/toolbar_model.h (right):

http://codereview.chromium.org/43109/diff/32/53#newcode11
Line 11: #include "chrome/common/feed.h"
Nope. As Jo points out, that doesn't compile.

On 2009/03/12 00:50:48, Jo wrote:
> On 2009/03/12 00:32:45, Evan Martin wrote:
> > Can you forward-declare FeedList here and elsewhere to avoid header
> > proliferation?
> 
> Is it possible to use scoped_refptr<> with a local declaration of a
> class/struct?
> 
>

http://codereview.chromium.org/43109/diff/32/47
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/43109/diff/32/47#newcode393
Line 393: location_y + 1,
Because my png was differently sized (smaller). I have taken it out and made the
dimensions of the png match the other png's that we display in the toolbar.
Thanks!

On 2009/03/12 00:32:45, Evan Martin wrote:
> Why +1 here?

http://codereview.chromium.org/43109/diff/32/47#newcode531
Line 531: void LocationBarView::SetRssIconVisibility(FeedList* feeds) {
On 2009/03/12 00:32:45, Evan Martin wrote:
> Should this be a const ref&?

No. The pointer can be NULL.

http://codereview.chromium.org/43109/diff/32/54
File chrome/common/feed.h (right):

http://codereview.chromium.org/43109/diff/32/54#newcode16
Line 16: std::wstring type;
On 2009/03/12 00:32:45, Evan Martin wrote:
> This needs comments -- particularly "type".

Done.

http://codereview.chromium.org/43109/diff/32/54#newcode28
Line 28: std::vector<FeedItem>& List() {
On 2009/03/12 00:32:45, Evan Martin wrote:
> lowercase "list()" for an accessor.  should be conster too:
> 
> const std::vector<FeedItem>& list() const {

Done.

http://codereview.chromium.org/43109/diff/32/55
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/43109/diff/32/55#newcode54
Line 54: // The list of available feed.
On 2009/03/12 00:32:45, Evan Martin wrote:
> grammar: "feeds"

Done.

http://codereview.chromium.org/43109/diff/32/55#newcode549
Line 549: if (!ReadParam(m, iter, &arraysize))
This is a general problem with our traits functions, I guess. But I added a
maximum of 50, just in case.

On 2009/03/12 00:32:45, Evan Martin wrote:
> Do we worry about overflows if this is too large?

http://codereview.chromium.org/43109/diff/32/55#newcode565
Line 565: static void Log(const param_type& p, std::wstring* l) {
On 2009/03/12 00:32:45, Evan Martin wrote:
> one-letter variable names make the style guide cry.

Done.

http://codereview.chromium.org/43109/diff/32/35
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/43109/diff/32/35#newcode133
Line 133: // Return the URL to the WebSyndicationFormats document for the frame.
If
On 2009/03/12 00:32:45, Evan Martin wrote:
> WebSyndicationFormats sounds like a vestigial term?

Done.

[Evan Martin, 2009-03-12 22:46:08]

+carlos

http://codereview.chromium.org/43109/diff/1042/2023
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/43109/diff/1042/2023#newcode552
Line 552: if (arraysize > FeedList::kMaxFeeds) {
Carlos: this is the code I was talking about.  Is it Finnur's responsibility to
defend against bad renderers?  What is the right thing to do?

[cpu_(ooo_6.6-7.5), 2009-03-12 23:14:57]

http://codereview.chromium.org/43109/diff/1042/2023
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/43109/diff/1042/2023#newcode556
Line 556: 
Unfortunately the design at this layer does not let us discriminate different
kind of errors. The code in line 552 is acceptable. The important case to cover
is that the renderer could not get is in a position of running out of memory for
example in line 565.

[Evan Martin, 2009-03-12 23:26:03]

LGTM; comment nits follow

http://codereview.chromium.org/43109/diff/1042/2011
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/43109/diff/1042/2011#newcode78
Line 78: INVALIDATE_LOAD = 8,       // The loading state has changed
can add a period here while you're at it *shrug*

http://codereview.chromium.org/43109/diff/1042/2016
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/43109/diff/1042/2016#newcode1051
Line 1051: std::wstring text = L"Subscribe to this feed";
Do we need to translate this?  Or should this be a TODO?

http://codereview.chromium.org/43109/diff/1042/2002
File webkit/glue/feed.h (right):

http://codereview.chromium.org/43109/diff/1042/2002#newcode17
Line 17: // The feed type, for example: "application/rss+xml".
Could you note that this isn't guaranteed to be set?

http://codereview.chromium.org/43109/diff/1042/2002#newcode25
Line 25: static const size_t kMaxFeeds = 50;
comment on this?

http://codereview.chromium.org/43109/diff/1042/2002#newcode29
Line 29: void Add(FeedItem &item) {
const?

[Finnur, 2009-03-12 23:45:48]

All done and currently building. Will probably check this in tomorrow morning.
Thanks!

http://codereview.chromium.org/43109/diff/1042/2016
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/43109/diff/1042/2016#newcode1051
Line 1051: std::wstring text = L"Subscribe to this feed";
Added a TODO.

On 2009/03/12 23:26:03, Evan Martin wrote:
> Do we need to translate this?  Or should this be a TODO?

http://codereview.chromium.org/43109/diff/1042/2002
File webkit/glue/feed.h (right):

http://codereview.chromium.org/43109/diff/1042/2002#newcode17
Line 17: // The feed type, for example: "application/rss+xml".
On 2009/03/12 23:26:03, Evan Martin wrote:
> Could you note that this isn't guaranteed to be set?

Done.

http://codereview.chromium.org/43109/diff/1042/2002#newcode25
Line 25: static const size_t kMaxFeeds = 50;
On 2009/03/12 23:26:03, Evan Martin wrote:
> comment on this?

Done.

http://codereview.chromium.org/43109/diff/1042/2002#newcode29
Line 29: void Add(FeedItem &item) {
On 2009/03/12 23:26:03, Evan Martin wrote:
> const?

Done.