Make GetURLForDebugging return a const GURL

Make GetURLForDebugging return a const GURL.

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

[tony, 2009-10-23 01:03:27]

I'm a bit worried that this might have a negative perf impact.  Thoughts?

[sky, 2009-10-23 16:57:27]

I don't know how much this'll impact performance, you'll have to measure and
find out. If you can, use const std::string& everywhere to cut down on
additional allocations.

Two points worth mentioning:
. Jim recently told me he thought SetActiveURL was taking a fair amount of
performance. This would make that worse. I haven't heard the latest from Jim
though as to whether SetActiveURL really was eating performance.

. Should we maintain a separate field in the crash report for the network
related url? Because you're not unsetting the active url, and it's likely we'll
stop unsetting the active url in the message handler as we do know, it'll be
hard to tell where the active url came from. If we have two fields for the two
different urls it might make it a bit easier to diagnose crashes.

I've added Jim and Justin to the cc list. Justin is looking at landing the
change that stops unsetting the active URL:
http://codereview.chromium.org/307020 .

http://codereview.chromium.org/326003/diff/1/2
File chrome/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/326003/diff/1/2#newcode321
Line 321:
child_process_logging::SetActiveURL(request_info.peer->GetURLForDebugging());
Any change we could make GetURLForDebugging return a const std::string& ?

http://codereview.chromium.org/326003/diff/1/7
File webkit/glue/media/buffered_data_source.h (right):

http://codereview.chromium.org/326003/diff/1/7#newcode120
Line 120: GURL GetURLForDebugging() { return url_; }
Could this be const GURL& and const method?

[tony, 2009-10-23 17:34:20]

Ah, I had considered not scoping the active url as well.  Let's try
that for now and see how it goes.  If we want more data, we can
revisit this patch, but I think not scoping the active_url will get us
most of the way there.

You're right, my concern was with false positives (e.g., iframe loads
that wouldn't get set properly), but maybe that's rare enough.

On Fri, Oct 23, 2009 at 9:57 AM,  <sky@chromium.org> wrote:
> I don't know how much this'll impact performance, you'll have to measure and
> find out. If you can, use const std::string& everywhere to cut down on
> additional allocations.
>
> Two points worth mentioning:
> . Jim recently told me he thought SetActiveURL was taking a fair amount of
> performance. This would make that worse. I haven't heard the latest from Jim
> though as to whether SetActiveURL really was eating performance.
>
> . Should we maintain a separate field in the crash report for the network
> related url? Because you're not unsetting the active url, and it's likely
> we'll
> stop unsetting the active url in the message handler as we do know, it'll be
> hard to tell where the active url came from. If we have two fields for the
> two
> different urls it might make it a bit easier to diagnose crashes.
>
> I've added Jim and Justin to the cc list. Justin is looking at landing the
> change that stops unsetting the active URL:
> http://codereview.chromium.org/307020 .
>
>
> http://codereview.chromium.org/326003/diff/1/2
> File chrome/common/resource_dispatcher.cc (right):
>
> http://codereview.chromium.org/326003/diff/1/2#newcode321
> Line 321:
> child_process_logging::SetActiveURL(request_info.peer->GetURLForDebugging());
> Any change we could make GetURLForDebugging return a const std::string&
> ?
>
> http://codereview.chromium.org/326003/diff/1/7
> File webkit/glue/media/buffered_data_source.h (right):
>
> http://codereview.chromium.org/326003/diff/1/7#newcode120
> Line 120: GURL GetURLForDebugging() { return url_; }
> Could this be const GURL& and const method?
>
> http://codereview.chromium.org/326003
>

[tony, 2009-10-28 19:40:09]

since http://codereview.chromium.org/307020 went in, I'm morphing this change
just to return a URL (style cleanup).

[sky, 2009-10-28 19:46:34]

http://codereview.chromium.org/326003/diff/4001/4003
File chrome/common/resource_dispatcher_unittest.cc (right):

http://codereview.chromium.org/326003/diff/4001/4003#newcode262
Line 262: virtual const GURL GetURLForDebugging() const {
I don't think the const return type is buying us anything here. If we could make
it const GURL& that would be awesome, but it doesn't look like we can so I would
nuke the constness.

[tony, 2009-10-28 20:21:49]

On 2009/10/28 19:46:34, sky wrote:
> http://codereview.chromium.org/326003/diff/4001/4003
> File chrome/common/resource_dispatcher_unittest.cc (right):
> 
> http://codereview.chromium.org/326003/diff/4001/4003#newcode262
> Line 262: virtual const GURL GetURLForDebugging() const {
> I don't think the const return type is buying us anything here. If we could
make
> it const GURL& that would be awesome, but it doesn't look like we can so I
would
> nuke the constness.

Done.  Yeah, I originally was trying to return a const reference but forgot to
remove the const.

[sky, 2009-10-28 20:39:48]

LGTM