Consolidate all NavigationController::LoadURL and family functions

Consolidate all NavigationController::LoadURL and family functions

Added public LoadUrlExtraParams struct to hold all extra parameters to the
family of LoadURL functions, were extra means not in the definition of LoadURL.

Added LoadURLWithExtraParams and removed all other load functions except
LoadURL. Updated all call sites to use the new method.

Removed NavigationControllerWebView.

BUG=


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

[boliu, 2012-08-02 20:30:14]

Just an FYI that I am trying to consolidate all the LoadURL functions. Patch Set
1 is NOT ready for review yet.

[boliu, 2012-08-02 22:41:17]

Patch set 3 ready for review.

[mnaganov (inactive), 2012-08-03 09:20:04]

http://codereview.chromium.org/10830144/diff/11002/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10830144/diff/11002/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:587:
DCHECK(params.url.SchemeIs(chrome::kHttpScheme) ||
We need to bail out for safety (as the original code did). DCHECKs are removed
in release builds, so this code will pass through.

http://codereview.chromium.org/10830144/diff/11002/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:591:
DCHECK(params.url.SchemeIs(chrome::kDataScheme));
Same here

[boliu, 2012-08-03 15:55:42]

http://codereview.chromium.org/10830144/diff/11002/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10830144/diff/11002/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:587:
DCHECK(params.url.SchemeIs(chrome::kHttpScheme) ||
On 2012/08/03 09:20:04, Mikhail Naganov (Chromium) wrote:
> We need to bail out for safety (as the original code did). DCHECKs are removed
> in release builds, so this code will pass through.

Done.

http://codereview.chromium.org/10830144/diff/11002/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:591:
DCHECK(params.url.SchemeIs(chrome::kDataScheme));
On 2012/08/03 09:20:04, Mikhail Naganov (Chromium) wrote:
> Same here

Done.

[gone, 2012-08-03 20:17:37]

https://chromiumcodereview.appspot.com/10830144/diff/2003/chrome/browser/inst...
File chrome/browser/instant/instant_loader.cc (right):

https://chromiumcodereview.appspot.com/10830144/diff/2003/chrome/browser/inst...
chrome/browser/instant/instant_loader.cc:761:
load_url_params.should_inherit_user_agent_override = false;
Seems like this should automatically be set to false whenever the
override_user_agent boolean is set.  Change the direct setting to a function
that does this?

https://chromiumcodereview.appspot.com/10830144/diff/2003/content/public/brow...
File content/public/browser/navigation_controller.h (right):

https://chromiumcodereview.appspot.com/10830144/diff/2003/content/public/brow...
content/public/browser/navigation_controller.h:112: // Used in
LOAD_TYPE_BROWSER_INITIATED_HTTP_POST loads only. Carrys the
nit: Carrys -> Carries

https://chromiumcodereview.appspot.com/10830144/diff/2003/content/public/brow...
content/public/browser/navigation_controller.h:241: virtual void
LoadURLWithParams(LoadURLParams& params) = 0;
Feels like the LoadURLParams struct should only carry "extra" parameters that
aren't sent in the LoadURL call (i.e. not the url, referrer, type, or
extra_headers).  It'd probably save a few copies...

[boliu, 2012-08-03 20:46:26]

http://codereview.chromium.org/10830144/diff/2003/chrome/browser/instant/inst...
File chrome/browser/instant/instant_loader.cc (right):

http://codereview.chromium.org/10830144/diff/2003/chrome/browser/instant/inst...
chrome/browser/instant/instant_loader.cc:761:
load_url_params.should_inherit_user_agent_override = false;
I'm against doing this kind of hidden "magic" logic in a simple value struct.
The struct is simply holding parameter values. If patterns like this emerge
throughout the code base, then we could add simple helper methods that make the
intention clear.

But I think in the case of user agent override, it should really be a tri-state
variable: override to true, override to false, inherit. I'll make this change
next.

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
File content/public/browser/navigation_controller.h (right):

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:241: virtual void
LoadURLWithParams(LoadURLParams& params) = 0;
I was debating about this. And previous Patch Set 4 does exactly what you
describe here.

Problem is that all the parameters except |url| are "extra". There are code
refactored in this CL that just passes in empty strings for referrer and
extra_headers. I guess PageTransition is "required", but it has a default value
so I didn't require it in the construction of LoadULRParams.

I can make this LoadURL(GURL& url, LoadURLParams& params) to save one copy.

Charlie, do you have any thoughts on this?

[Charlie Reis, 2012-08-03 21:06:29]

On 2012/08/03 20:46:26, boliu wrote:
> Charlie, do you have any thoughts on this?

Looking...

[Charlie Reis, 2012-08-03 21:28:35]

While there's more complexity now from having to create a params struct, I do
like having fewer entry points to LoadURL.  Seems worthwhile.

http://codereview.chromium.org/10830144/diff/2003/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator.cc (left):

http://codereview.chromium.org/10830144/diff/2003/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:227: params->is_renderer_initiated);
You've lost params->is_renderer_initiated in this case.

http://codereview.chromium.org/10830144/diff/2003/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10830144/diff/2003/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl.cc:589: NOTREACHED() <<
"Http post load must use http(s) scheme.";
This isn't fixed.  NOTREACHED() is still debug-only.  We have to return here
(and below) after the NOTREACHED, as we used to.

http://codereview.chromium.org/10830144/diff/2003/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl.cc:606:
if(params.should_inherit_user_agent_override) {
nit: Need a space after if and no braces.

http://codereview.chromium.org/10830144/diff/2003/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

http://codereview.chromium.org/10830144/diff/2003/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl_unittest.cc:233:
NavigationController::LoadURLParams load_params(GURL("data://dataurl"));
nit: "data:text/html,dataurl"
(There's no :// in data URLs.)

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
File content/public/browser/navigation_controller.h (right):

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:51: enum LoadURLType {
enums and structs belong above the static CreateNavigationEntry function, I
believe.

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:52: // For loads that does not
fall into any types below.
nit: do not

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:57: // Used in Android
WebView.postUrl implementation.
I don't think we should talk about Android WebView uses in the public API
documentation.  Maybe we can mention it somewhere else?

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:110: GURL virtual_url;
virtual_url has a different meaning in NavigationEntry (used outside of
LOAD_TYPE_DATA), which makes this confusing.  Please use either
virtual_url_for_data_url or history_url_for_data_url.

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:240: // using |extra_params|.
nit: |params|

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:241: virtual void
LoadURLWithParams(LoadURLParams& params) = 0;
On 2012/08/03 20:46:27, boliu wrote:
> I was debating about this. And previous Patch Set 4 does exactly what you
> describe here.
> 
> Problem is that all the parameters except |url| are "extra". There are code
> refactored in this CL that just passes in empty strings for referrer and
> extra_headers. I guess PageTransition is "required", but it has a default
value
> so I didn't require it in the construction of LoadULRParams.
> 
> I can make this LoadURL(GURL& url, LoadURLParams& params) to save one copy.
> 
> Charlie, do you have any thoughts on this?

I'm ok with it as is.

[boliu, 2012-08-03 22:42:08]

http://codereview.chromium.org/10830144/diff/2003/chrome/browser/ui/browser_n...
File chrome/browser/ui/browser_navigator.cc (left):

http://codereview.chromium.org/10830144/diff/2003/chrome/browser/ui/browser_n...
chrome/browser/ui/browser_navigator.cc:227: params->is_renderer_initiated);
Very good catch! Fixed.

http://codereview.chromium.org/10830144/diff/2003/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10830144/diff/2003/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl.cc:589: NOTREACHED() <<
"Http post load must use http(s) scheme.";
On 2012/08/03 21:28:36, creis wrote:
> This isn't fixed.  NOTREACHED() is still debug-only.  We have to return here
> (and below) after the NOTREACHED, as we used to.

Done.

http://codereview.chromium.org/10830144/diff/2003/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl.cc:606:
if(params.should_inherit_user_agent_override) {
Code block removed.

http://codereview.chromium.org/10830144/diff/2003/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

http://codereview.chromium.org/10830144/diff/2003/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl_unittest.cc:233:
NavigationController::LoadURLParams load_params(GURL("data://dataurl"));
On 2012/08/03 21:28:36, creis wrote:
> nit: "data:text/html,dataurl"
> (There's no :// in data URLs.)

Done.

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
File content/public/browser/navigation_controller.h (right):

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:51: enum LoadURLType {
Done. Placed new UserAgentOverrideOption enum here as well.

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:52: // For loads that does not
fall into any types below.
On 2012/08/03 21:28:36, creis wrote:
> nit: do not

Done.

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:57: // Used in Android
WebView.postUrl implementation.
Removed mentions of webview usage. I originally added it to avoid confusing
chromium developers using this api wondering about what load type to use. The
android code using this has not been upstreamed yet.

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:110: GURL virtual_url;
Renamed to virtual_url_for_data_url.

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:112: // Used in
LOAD_TYPE_BROWSER_INITIATED_HTTP_POST loads only. Carrys the
On 2012/08/03 20:17:37, dfalcantara wrote:
> nit: Carrys -> Carries

Done.

http://codereview.chromium.org/10830144/diff/2003/content/public/browser/navi...
content/public/browser/navigation_controller.h:240: // using |extra_params|.
On 2012/08/03 21:28:36, creis wrote:
> nit: |params|

Done.

http://codereview.chromium.org/10830144/diff/5004/content/public/browser/navi...
File content/public/browser/navigation_controller.h (right):

http://codereview.chromium.org/10830144/diff/5004/content/public/browser/navi...
content/public/browser/navigation_controller.h:120: base::RefCountedMemory*
browser_initiated_post_data;
I thought about commenting about ownership of this, but don't know what the
general chromium style is. Currently the data is copied out in the LoadURL call
and never used again even though NavigationEntry holds a ref-counted reference
to this for a short time afterwards. So technically there is no harm in the
caller modifying it after the LoadURL call, but probably should not be
encouraged.

Should I say something like
"ownership transferred to NavigationController" or
"do not modify after calling LoadURL"?

[Charlie Reis, 2012-08-03 23:12:47]

LGTM with nits, but you'll need owner's approval for content/public changes.

http://codereview.chromium.org/10830144/diff/5004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/10830144/diff/5004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:380:
content::NavigationController::UA_OVERRIDE_TRUE :
The indent here is wrong.  Should be 4 spaces for 379 and 8 for 380-381.

http://codereview.chromium.org/10830144/diff/5004/content/browser/android/con...
File content/browser/android/content_view_core_impl.cc (right):

http://codereview.chromium.org/10830144/diff/5004/content/browser/android/con...
content/browser/android/content_view_core_impl.cc:423:
content::NavigationController::UA_OVERRIDE_TRUE :
Too much indent here.

http://codereview.chromium.org/10830144/diff/5004/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

http://codereview.chromium.org/10830144/diff/5004/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl_unittest.cc:202:
load_params.override_user_agent ? true : false,
This is a bit awkward to read (three types of equality checking in one line),
and it depends on operator precedence.  Maybe split a version of this expression
out into a should_override bool?

http://codereview.chromium.org/10830144/diff/5004/content/public/browser/navi...
File content/public/browser/navigation_controller.h (right):

http://codereview.chromium.org/10830144/diff/5004/content/public/browser/navi...
content/public/browser/navigation_controller.h:120: base::RefCountedMemory*
browser_initiated_post_data;
On 2012/08/03 22:42:08, boliu wrote:
> I thought about commenting about ownership of this, but don't know what the
> general chromium style is. Currently the data is copied out in the LoadURL
call
> and never used again even though NavigationEntry holds a ref-counted reference
> to this for a short time afterwards. So technically there is no harm in the
> caller modifying it after the LoadURL call, but probably should not be
> encouraged.
> 
> Should I say something like
> "ownership transferred to NavigationController" or
> "do not modify after calling LoadURL"?

I'd recommend the "ownership transferred to NavigationController" approach.

[Charlie Reis, 2012-08-03 23:13:34]

On 2012/08/03 23:12:47, creis wrote:
> LGTM with nits, but you'll need owner's approval for content/public changes.

For that, I might recommend brettw@, who knows NavigationController well.

[boliu, 2012-08-03 23:31:39]

http://codereview.chromium.org/10830144/diff/5004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/10830144/diff/5004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:380:
content::NavigationController::UA_OVERRIDE_TRUE :
On 2012/08/03 23:12:48, creis wrote:
> The indent here is wrong.  Should be 4 spaces for 379 and 8 for 380-381.

Done.

http://codereview.chromium.org/10830144/diff/5004/content/browser/android/con...
File content/browser/android/content_view_core_impl.cc (right):

http://codereview.chromium.org/10830144/diff/5004/content/browser/android/con...
content/browser/android/content_view_core_impl.cc:423:
content::NavigationController::UA_OVERRIDE_TRUE :
On 2012/08/03 23:12:48, creis wrote:
> Too much indent here.

Done.

http://codereview.chromium.org/10830144/diff/5004/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

http://codereview.chromium.org/10830144/diff/5004/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl_unittest.cc:202:
load_params.override_user_agent ? true : false,
On 2012/08/03 23:12:48, creis wrote:
> This is a bit awkward to read (three types of equality checking in one line),
> and it depends on operator precedence.  Maybe split a version of this
expression
> out into a should_override bool?

Done.

http://codereview.chromium.org/10830144/diff/5004/content/public/browser/navi...
File content/public/browser/navigation_controller.h (right):

http://codereview.chromium.org/10830144/diff/5004/content/public/browser/navi...
content/public/browser/navigation_controller.h:120: base::RefCountedMemory*
browser_initiated_post_data;
On 2012/08/03 23:12:48, creis wrote:
> On 2012/08/03 22:42:08, boliu wrote:
> > I thought about commenting about ownership of this, but don't know what the
> > general chromium style is. Currently the data is copied out in the LoadURL
> call
> > and never used again even though NavigationEntry holds a ref-counted
reference
> > to this for a short time afterwards. So technically there is no harm in the
> > caller modifying it after the LoadURL call, but probably should not be
> > encouraged.
> > 
> > Should I say something like
> > "ownership transferred to NavigationController" or
> > "do not modify after calling LoadURL"?
> 
> I'd recommend the "ownership transferred to NavigationController" approach.

Done.

[gone, 2012-08-04 00:57:05]

LGTM, but I'd like to play with an Android build with the "final" version before
attempting to land it to make sure everything is working as intended...

On 2012/08/03 23:31:39, boliu wrote:
>
http://codereview.chromium.org/10830144/diff/5004/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_contents.cc (right):
> 
>
http://codereview.chromium.org/10830144/diff/5004/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_contents.cc:380:
> content::NavigationController::UA_OVERRIDE_TRUE :
> On 2012/08/03 23:12:48, creis wrote:
> > The indent here is wrong.  Should be 4 spaces for 379 and 8 for 380-381.
> 
> Done.
> 
>
http://codereview.chromium.org/10830144/diff/5004/content/browser/android/con...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
http://codereview.chromium.org/10830144/diff/5004/content/browser/android/con...
> content/browser/android/content_view_core_impl.cc:423:
> content::NavigationController::UA_OVERRIDE_TRUE :
> On 2012/08/03 23:12:48, creis wrote:
> > Too much indent here.
> 
> Done.
> 
>
http://codereview.chromium.org/10830144/diff/5004/content/browser/web_content...
> File content/browser/web_contents/navigation_controller_impl_unittest.cc
> (right):
> 
>
http://codereview.chromium.org/10830144/diff/5004/content/browser/web_content...
> content/browser/web_contents/navigation_controller_impl_unittest.cc:202:
> load_params.override_user_agent ? true : false,
> On 2012/08/03 23:12:48, creis wrote:
> > This is a bit awkward to read (three types of equality checking in one
line),
> > and it depends on operator precedence.  Maybe split a version of this
> expression
> > out into a should_override bool?
> 
> Done.
> 
>
http://codereview.chromium.org/10830144/diff/5004/content/public/browser/navi...
> File content/public/browser/navigation_controller.h (right):
> 
>
http://codereview.chromium.org/10830144/diff/5004/content/public/browser/navi...
> content/public/browser/navigation_controller.h:120: base::RefCountedMemory*
> browser_initiated_post_data;
> On 2012/08/03 23:12:48, creis wrote:
> > On 2012/08/03 22:42:08, boliu wrote:
> > > I thought about commenting about ownership of this, but don't know what
the
> > > general chromium style is. Currently the data is copied out in the LoadURL
> > call
> > > and never used again even though NavigationEntry holds a ref-counted
> reference
> > > to this for a short time afterwards. So technically there is no harm in
the
> > > caller modifying it after the LoadURL call, but probably should not be
> > > encouraged.
> > > 
> > > Should I say something like
> > > "ownership transferred to NavigationController" or
> > > "do not modify after calling LoadURL"?
> > 
> > I'd recommend the "ownership transferred to NavigationController" approach.
> 
> Done.

[boliu, 2012-08-08 15:25:47]

Ping! Hi Brett, can you take a look at this?

[brettw, 2012-08-10 22:25:17]

Sorry for the delay. This seems like a nice improvement, I have mostly style
nits.

http://codereview.chromium.org/10830144/diff/15/content/public/browser/naviga...
File content/public/browser/navigation_controller.h (right):

http://codereview.chromium.org/10830144/diff/15/content/public/browser/naviga...
content/public/browser/navigation_controller.h:11: #include
"base/memory/ref_counted_memory.h"
You should be able to remove this and forward declare when you move the
constructor and destructor to the .cc file like I suggested (then include this
from only the .cc file)

http://codereview.chromium.org/10830144/diff/15/content/public/browser/naviga...
content/public/browser/navigation_controller.h:121: base::RefCountedMemory*
browser_initiated_post_data;
It seems like the memory management would be clearer here if this were a
scoped_refptr<RefCountedMemory>.

http://codereview.chromium.org/10830144/diff/15/content/public/browser/naviga...
content/public/browser/navigation_controller.h:123: LoadURLParams(const GURL&
url)
This should be explicit.

I don't think we want this to be inline or this code will get duplicated all
over. Can you make a .cc file for this?

Also, can you add a non-inline destructor for the same reason?

http://codereview.chromium.org/10830144/diff/15/content/public/browser/naviga...
content/public/browser/navigation_controller.h:247: virtual void
LoadURLWithParams(LoadURLParams& params) = 0;
Shouldn't this be a const ref?

[boliu, 2012-08-11 00:53:35]

http://codereview.chromium.org/10830144/diff/15/content/public/browser/naviga...
File content/public/browser/navigation_controller.h (right):

http://codereview.chromium.org/10830144/diff/15/content/public/browser/naviga...
content/public/browser/navigation_controller.h:11: #include
"base/memory/ref_counted_memory.h"
Removed ref_counted_memory.h but added ref_counted.h for adding scoped_refptr
below.

On 2012/08/10 22:25:18, brettw wrote:
> You should be able to remove this and forward declare when you move the
> constructor and destructor to the .cc file like I suggested (then include this
> from only the .cc file)

http://codereview.chromium.org/10830144/diff/15/content/public/browser/naviga...
content/public/browser/navigation_controller.h:121: base::RefCountedMemory*
browser_initiated_post_data;
On 2012/08/10 22:25:18, brettw wrote:
> It seems like the memory management would be clearer here if this were a
> scoped_refptr<RefCountedMemory>.

Done.

http://codereview.chromium.org/10830144/diff/15/content/public/browser/naviga...
content/public/browser/navigation_controller.h:123: LoadURLParams(const GURL&
url)
On 2012/08/10 22:25:18, brettw wrote:
> This should be explicit.
> 
> I don't think we want this to be inline or this code will get duplicated all
> over. Can you make a .cc file for this?
> 
> Also, can you add a non-inline destructor for the same reason?

Done.

New cc file is
content/browser/web_contents/navigation_controller_load_url_params.cc. Is this
appropriately named?

http://codereview.chromium.org/10830144/diff/15/content/public/browser/naviga...
content/public/browser/navigation_controller.h:247: virtual void
LoadURLWithParams(LoadURLParams& params) = 0;
On 2012/08/10 22:25:18, brettw wrote:
> Shouldn't this be a const ref?

Done.

[brettw, 2012-08-11 17:32:31]

I'd make the new .cc file match the .h file. If we add more non-pure-virtual
members in NavigationController they would go there as well.

[boliu, 2012-08-13 14:31:02]

Done.

[brettw, 2012-08-13 17:48:35]

The navigation_controller.cc file should be in the same dir as the .h (the
public one). You can see there are a smattering of other .cc files in the
directory similar to the one you're adding.

LGTM with that moved.

[boliu, 2012-08-13 18:00:15]

Done. Thanks for the review. Waiting for trybots before cq.

[boliu, 2012-08-13 18:27:18]

One more change in Patch Set 12 to fix linux_clang trybot:
Moved CONTENT_EXPORT after struct in declaration of
NavigationController::LoadURLParams

[commit-bot: I haz the power, 2012-08-13 20:03:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/10830144/1013

[commit-bot: I haz the power, 2012-08-13 21:25:49]

Try job failure for 10830144-1013 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[boliu, 2012-08-13 22:19:15]

Hi reviewers, PTAL.

win debug trybot failed and I think it is due to the compiler trying to add a
copy constructor to NavigationController::LoadURLParams. I fixed it by adding
DISALLOW_COPY_AND_ASSIGN in a private section.

Is this an appropriate fix? The other option is to include RefCountedMemory
header file instead of forward declaring it.

[boliu, 2012-08-13 23:16:14]

On advice from akalin and awong from irc, I implemented the copy constructor and
assignment operator for LoadURLParams instead of using DISALLOW_COPY_AND_ASSIGN.
I think there is no reason why LoadURLParams shouldn't be copyable.

I had to make the url field in LoadURLParams non-const to implement the
assignment operator.

PTAL

[brettw, 2012-08-13 23:18:27]

LGTM rubberstamp (didn't really look)

[commit-bot: I haz the power, 2012-08-13 23:22:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/10830144/3023

[commit-bot: I haz the power, 2012-08-13 23:22:40]

Failed to apply patch for
content/browser/web_contents/navigation_controller_impl.h:
While running patch -p1 --forward --force;
patching file content/browser/web_contents/navigation_controller_impl.h
Hunk #2 succeeded at 23 (offset -1 lines).
Hunk #3 succeeded at 56 (offset -2 lines).
Hunk #4 FAILED at 86.
1 out of 4 hunks FAILED -- saving rejects to file
content/browser/web_contents/navigation_controller_impl.h.rej

[commit-bot: I haz the power, 2012-08-13 23:37:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/10830144/11017

[commit-bot: I haz the power, 2012-08-14 00:49:48]

Try job failure for 10830144-11017 (retry) on mac_rel for step
"content_unittests".
It's a second try, previously, steps "media_unittests, content_unittests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-14 17:16:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/10830144/11017

[commit-bot: I haz the power, 2012-08-14 19:17:49]

Change committed as 151534