Moving LoadFromMemoryCacheDetails to content/public/browser and making it a struct.

Moving LoadFromMemoryCacheDetails to content/public/browser and making it a struct.


BUG=None
TEST=None


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

[Shishir, 2012-06-18 22:25:14]

Sending out this CL as discussed over mail. Please take a look.

[jam, 2012-06-18 23:29:01]

http://codereview.chromium.org/10582007/diff/1/content/browser/ssl/ssl_manage...
File content/browser/ssl/ssl_manager.cc (right):

http://codereview.chromium.org/10582007/diff/1/content/browser/ssl/ssl_manage...
content/browser/ssl/ssl_manager.cc:10: #include
"content/public/browser/load_from_memory_cache_details.h"
nit: order (also below which isn't your fault:) )

http://codereview.chromium.org/10582007/diff/1/content/public/browser/load_fr...
File content/public/browser/load_from_memory_cache_details.h (right):

http://codereview.chromium.org/10582007/diff/1/content/public/browser/load_fr...
content/public/browser/load_from_memory_cache_details.h:18:
LoadFromMemoryCacheDetails(const GURL& url,
nit: is this constructor still needed? we try to avoid cc files in
content/public unless needed to avoid clang errors. or if you just want to keep
it, can we inline this?

[Shishir, 2012-06-18 23:53:46]

http://codereview.chromium.org/10582007/diff/1/content/browser/ssl/ssl_manage...
File content/browser/ssl/ssl_manager.cc (right):

http://codereview.chromium.org/10582007/diff/1/content/browser/ssl/ssl_manage...
content/browser/ssl/ssl_manager.cc:10: #include
"content/public/browser/load_from_memory_cache_details.h"
On 2012/06/18 23:29:01, John Abd-El-Malek wrote:
> nit: order (also below which isn't your fault:) )

Done.

http://codereview.chromium.org/10582007/diff/1/content/public/browser/load_fr...
File content/public/browser/load_from_memory_cache_details.h (right):

http://codereview.chromium.org/10582007/diff/1/content/public/browser/load_fr...
content/public/browser/load_from_memory_cache_details.h:18:
LoadFromMemoryCacheDetails(const GURL& url,
On 2012/06/18 23:29:01, John Abd-El-Malek wrote:
> nit: is this constructor still needed? we try to avoid cc files in
> content/public unless needed to avoid clang errors. or if you just want to
keep
> it, can we inline this?

I had tried to inline it, but I get the - Complex struct with inline constructor
- clang error.

In my past code reviews, reviewers have asked me to keep a constructor for
structs  as they have said that the default values of the fields may cause bugs
if not initialized properly. I can get rid of the constructor if you think that
its not an issue.

[jam, 2012-06-19 00:04:57]

http://codereview.chromium.org/10582007/diff/1/content/public/browser/load_fr...
File content/public/browser/load_from_memory_cache_details.h (right):

http://codereview.chromium.org/10582007/diff/1/content/public/browser/load_fr...
content/public/browser/load_from_memory_cache_details.h:18:
LoadFromMemoryCacheDetails(const GURL& url,
On 2012/06/18 23:53:46, Shishir wrote:
> On 2012/06/18 23:29:01, John Abd-El-Malek wrote:
> > nit: is this constructor still needed? we try to avoid cc files in
> > content/public unless needed to avoid clang errors. or if you just want to
> keep
> > it, can we inline this?
> 
> I had tried to inline it, but I get the - Complex struct with inline
constructor
> - clang error.
> 
> In my past code reviews, reviewers have asked me to keep a constructor for
> structs  as they have said that the default values of the fields may cause
bugs
> if not initialized properly. I can get rid of the constructor if you think
that
> its not an issue.

I don't see how this constructor helps with that issue, since it's not a default
constructor?

Yeah I'd prefer to get rid of it so we can avoid a cc file (we try hard to avoid
cc files in content/public)

[Shishir, 2012-06-19 00:17:01]

On 2012/06/19 00:04:57, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/10582007/diff/1/content/public/browser/load_fr...
> File content/public/browser/load_from_memory_cache_details.h (right):
> 
>
http://codereview.chromium.org/10582007/diff/1/content/public/browser/load_fr...
> content/public/browser/load_from_memory_cache_details.h:18:
> LoadFromMemoryCacheDetails(const GURL& url,
> On 2012/06/18 23:53:46, Shishir wrote:
> > On 2012/06/18 23:29:01, John Abd-El-Malek wrote:
> > > nit: is this constructor still needed? we try to avoid cc files in
> > > content/public unless needed to avoid clang errors. or if you just want to
> > keep
> > > it, can we inline this?
> > 
> > I had tried to inline it, but I get the - Complex struct with inline
> constructor
> > - clang error.
> > 
> > In my past code reviews, reviewers have asked me to keep a constructor for
> > structs  as they have said that the default values of the fields may cause
> bugs
> > if not initialized properly. I can get rid of the constructor if you think
> that
> > its not an issue.
> 
> I don't see how this constructor helps with that issue, since it's not a
default
> constructor?
> 
> Yeah I'd prefer to get rid of it so we can avoid a cc file (we try hard to
avoid
> cc files in content/public)

I meant that by forcing a constructor with all params, the caller would be
forced to set all the fields.

I tried removing the constructor but run into the same clang issue:
../../content/public/browser/load_from_memory_cache_details.h:17:1: error:
[chromium-style] Complex class/struct needs an explicit out-of-line constructor.

So looks like we might have to keep the .cc file.

[jam, 2012-06-19 00:24:31]

lgtm

[commit-bot: I haz the power, 2012-06-19 00:51:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10582007/8001

[commit-bot: I haz the power, 2012-06-19 07:37:29]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is 142922, job name
was 10582007-8001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2012-06-19 22:01:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10582007/8001

[commit-bot: I haz the power, 2012-06-20 01:02:02]

Change committed as 143113