Add a referrer class to be used in the browser process whenever passing around referrsr

Add a referrer class to be used in the browser process whenever passing around referrers

BUG=10502
TEST=content_unittest:ReferrerTest.*


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

[jochen (gone - plz use gerrit), 2011-11-22 19:16:15]

plz review

[darin (slow to review), 2011-11-22 19:20:59]

http://codereview.chromium.org/8635017/diff/1/content/browser/tab_contents/re...
File content/browser/tab_contents/referrer.h (right):

http://codereview.chromium.org/8635017/diff/1/content/browser/tab_contents/re...
content/browser/tab_contents/referrer.h:27: const GURL& referrer() const {
return referrer_; }
nit: since the class is named Referrer, how about naming this method url()?  and
similarly drop the referrer_ prefix on policy().

http://codereview.chromium.org/8635017/diff/1/content/browser/tab_contents/re...
content/browser/tab_contents/referrer.h:30: bool operator!=(const Referrer&
other) const {
nit: we usually don't provide operator overloads.  however, if you are going to
do this, then you should at least implement != in terms of ==, so that you are
not duplicating code.

 operator!=(...) {
   return !(a == b);
 }

also, i think it good c++ style to make operator== and operator!= be top-level
functions.  that way, you support type coercion for either the LHS or RHS of the
== operator.

[jochen (gone - plz use gerrit), 2011-11-22 19:54:47]

http://codereview.chromium.org/8635017/diff/1/content/browser/tab_contents/re...
File content/browser/tab_contents/referrer.h (right):

http://codereview.chromium.org/8635017/diff/1/content/browser/tab_contents/re...
content/browser/tab_contents/referrer.h:27: const GURL& referrer() const {
return referrer_; }
On 2011/11/22 19:20:59, darin wrote:
> nit: since the class is named Referrer, how about naming this method url()? 
and
> similarly drop the referrer_ prefix on policy().

Done.

http://codereview.chromium.org/8635017/diff/1/content/browser/tab_contents/re...
content/browser/tab_contents/referrer.h:30: bool operator!=(const Referrer&
other) const {
I'll drop this, since I'm not sure it's really needed anywhere

[brettw, 2011-11-22 21:14:01]

http://codereview.chromium.org/8635017/diff/2002/content/browser/tab_contents...
File content/browser/tab_contents/referrer.h (right):

http://codereview.chromium.org/8635017/diff/2002/content/browser/tab_contents...
content/browser/tab_contents/referrer.h:36: CONTENT_EXPORT std::ostream&
operator<<(std::ostream& out,
We don't usually write these. Is it really necessary?

I guess I was expecting this to be in content/common and be sent over IPC where
we have GURLs sent as the referrer today. Is that undesirable for some reason?

[jochen (gone - plz use gerrit), 2011-11-22 21:26:06]

http://codereview.chromium.org/8635017/diff/2002/content/browser/tab_contents...
File content/browser/tab_contents/referrer.h (right):

http://codereview.chromium.org/8635017/diff/2002/content/browser/tab_contents...
content/browser/tab_contents/referrer.h:36: CONTENT_EXPORT std::ostream&
operator<<(std::ostream& out,
On 2011/11/22 21:14:01, brettw wrote:
> We don't usually write these. Is it really necessary?
> 
> I guess I was expecting this to be in content/common and be sent over IPC
where
> we have GURLs sent as the referrer today. Is that undesirable for some reason?

Done.

[jochen (gone - plz use gerrit), 2011-11-23 19:30:32]

Is the CL ok like this?

If yes, I'd like to submit it and start converting all referrer-referring code
:)

[brettw, 2011-11-24 20:23:31]

lgtm

[jochen (gone - plz use gerrit), 2011-11-24 20:26:59]

John, can you rubber stamp the content/public part?

[jam, 2011-11-24 21:17:35]

http://codereview.chromium.org/8635017/diff/10001/content/common/referrer.cc
File content/common/referrer.cc (right):

http://codereview.chromium.org/8635017/diff/10001/content/common/referrer.cc#...
content/common/referrer.cc:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
nit: we put files holding constructor/destructor of classes/structs in public
alongside those files

http://codereview.chromium.org/8635017/diff/10001/content/public/common/refer...
File content/public/common/referrer.h (right):

http://codereview.chromium.org/8635017/diff/10001/content/public/common/refer...
content/public/common/referrer.h:5: #ifndef
CONTENT_BROWSER_TAB_CONTENTS_REFERRER_H_
CONTENT_PUBLIC_COMMON_REFERRER_H_, presubmit would have caught this :)

http://codereview.chromium.org/8635017/diff/10001/content/public/common/refer...
content/public/common/referrer.h:19: class CONTENT_EXPORT Referrer {
this seems like it should just be a struct (perhaps with const members if you so
insist)?

[jochen (gone - plz use gerrit), 2011-11-24 23:02:39]

http://codereview.chromium.org/8635017/diff/10001/content/common/referrer.cc
File content/common/referrer.cc (right):

http://codereview.chromium.org/8635017/diff/10001/content/common/referrer.cc#...
content/common/referrer.cc:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
On 2011/11/24 21:17:35, John Abd-El-Malek wrote:
> nit: we put files holding constructor/destructor of classes/structs in public
> alongside those files

Done.

http://codereview.chromium.org/8635017/diff/10001/content/public/common/refer...
File content/public/common/referrer.h (right):

http://codereview.chromium.org/8635017/diff/10001/content/public/common/refer...
content/public/common/referrer.h:5: #ifndef
CONTENT_BROWSER_TAB_CONTENTS_REFERRER_H_
On 2011/11/24 21:17:35, John Abd-El-Malek wrote:
> CONTENT_PUBLIC_COMMON_REFERRER_H_, presubmit would have caught this :)

actually, it doesn't... strange :-/

http://codereview.chromium.org/8635017/diff/10001/content/public/common/refer...
content/public/common/referrer.h:19: class CONTENT_EXPORT Referrer {
On 2011/11/24 21:17:35, John Abd-El-Malek wrote:
> this seems like it should just be a struct (perhaps with const members if you
so
> insist)?

Done.

[jam, 2011-11-24 23:06:41]

lgtm

is the cc file needed? if you can get away with it (which I think you can,
putting the constructor in the header since it's a simple structure), that'd be
preferable since the fewer cc files in the public directory the better.

http://codereview.chromium.org/8635017/diff/19003/content/public/common/refer...
File content/public/common/referrer.h (right):

http://codereview.chromium.org/8635017/diff/19003/content/public/common/refer...
content/public/common/referrer.h:9: #include "base/basictypes.h"
nit: needed?

[commit-bot: I haz the power, 2011-11-24 23:11:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/8635017/20005

[commit-bot: I haz the power, 2011-11-25 00:10:25]

Change committed as 111566