Safely cancel prerenders on threads other than the UI thread

Safely cancel prerenders on threads other than the UI thread.

Previously, prerendering was cancelled on the IO thread by not
doing something, and then passing a task to the UI thread
to abort the prerender.  This resulted in a race which could
result in swapping in the prerender before the task was
executed.  This fixes that.

BUG=83062
TEST=PrerenderStatusManagerTests, PrerenderBrowserTests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86082

[mmenke, 2011-05-19 15:04:57]

John:  Please review the src/content changes.  I know adding another dependency
could be considered a step backwards, but this does remove some of the
prerendering code from resource_dispatcher_host, along with all its externally
visible prerendering-related functions.

Timo, Dominic, Shishir:  Please review everything.

[mmenke, 2011-05-19 15:39:11]

I should add that I talked with Chris about making this a singleton.  Due to
concerns over the lifetime of profiles and the ResourceDispatcherHost, we
decided to make it a Singleton for now, though but may revisit the decision in
the future.

On 2011/05/19 15:04:57, Matt Menke wrote:
> John:  Please review the src/content changes.  I know adding another
dependency
> could be considered a step backwards, but this does remove some of the
> prerendering code from resource_dispatcher_host, along with all its externally
> visible prerendering-related functions.
> 
> Timo, Dominic, Shishir:  Please review everything.

[dominich, 2011-05-19 15:49:33]

This actually meshes very nicely with a CL that I've been working on that
removes Prerender from the ResourceDispatcherHost entirely. I've been struggling
with how to correctly initialize the IO part and it looks like I'll be able to
merge my changes with these and use the PrerenderTracker.

Having looked into this for my own CL, I agree that the Singleton is the right
option in this case.

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

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:285: return child_id_ >= 0;
This should be a != -1 as you never expect a value < -1. You could DCHECK that
too.

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:291: return route_id_ >= 0;
See above comment re != -1.

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:666: NOTREACHED();
Can this not be CHECK(PrerenderTracker::GetInstance()->GetFinalStatus(...)) ?

[dominich, 2011-05-19 16:07:27]

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker.cc (right):

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.cc:32: FinalStatus final_status;
Should this have a constructor that takes the PrerenderManager* and sets the
final_status to FINAL_STATUS_MAX?

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:53: bool TryCancelOnIOThread(int
child_id, int route_id,
Can you simplify the API by combining this with TryCancel? Check if we're on the
IO thread in TryCancel and do the early exit check if we are.

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:94: FinalStatus SetFinalStatus(int
child_id, int route_id,
nit: Can you move this down to the other method declarations (line 104ish)?

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:108: const std::pair<int, int>&
child_route_id_pair);
using a typedef for this std::pair might increase readability here.

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:119: base::Lock lock_;
Name this final_status_map_lock_ so it's clear what it is protecting.

[mmenke, 2011-05-19 16:40:42]

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

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:285: return child_id_ >= 0;
On 2011/05/19 15:49:33, dominic wrote:
> This should be a != -1 as you never expect a value < -1. You could DCHECK that
> too.

Done (for all the checks in this file, also updated the tracker's corresponding
DCHECKs)

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:666: NOTREACHED();
On 2011/05/19 15:49:33, dominic wrote:
> Can this not be CHECK(PrerenderTracker::GetInstance()->GetFinalStatus(...)) ?

A CHECK results in a crash in release builds, which I prefer to avoid unless
we're about to crash anyways.

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker.cc (right):

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.cc:32: FinalStatus final_status;
On 2011/05/19 16:07:27, dominic wrote:
> Should this have a constructor that takes the PrerenderManager* and sets the
> final_status to FINAL_STATUS_MAX? 

In order to do that, I'd also have to make a constructor that takes no
arguments, since std::map works by creating a new entry with that constructor,
and then copying the data over.  Creating such an extra constructor for that
seems a little crufty.

I've switched to creating it on the stack and then inserting it, to avoid the
double lookup the old code produced, though I'm open to other ideas.

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:53: bool TryCancelOnIOThread(int
child_id, int route_id,
On 2011/05/19 16:07:27, dominic wrote:
> Can you simplify the API by combining this with TryCancel? Check if we're on
the
> IO thread in TryCancel and do the early exit check if we are.

Checking if we're on the IO thread itself triggers a lock, unfortunately.

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:94: FinalStatus SetFinalStatus(int
child_id, int route_id,
On 2011/05/19 16:07:27, dominic wrote:
> nit: Can you move this down to the other method declarations (line 104ish)?

Done.

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:108: const std::pair<int, int>&
child_route_id_pair);
On 2011/05/19 16:07:27, dominic wrote:
> using a typedef for this std::pair might increase readability here.

Done.

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:119: base::Lock lock_;
On 2011/05/19 16:07:27, dominic wrote:
> Name this final_status_map_lock_ so it's clear what it is protecting.

Great idea, done.

[dominich, 2011-05-19 16:55:46]

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

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:666: NOTREACHED();
On 2011/05/19 16:40:42, Matt Menke wrote:
> On 2011/05/19 15:49:33, dominic wrote:
> > Can this not be CHECK(PrerenderTracker::GetInstance()->GetFinalStatus(...))
?
> 
> A CHECK results in a crash in release builds, which I prefer to avoid unless
> we're about to crash anyways.

I thought NOTREACHED was a CHECK rather than a DCHECK. My bad.

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker.cc (right):

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.cc:32: FinalStatus final_status;
On 2011/05/19 16:40:42, Matt Menke wrote:
> On 2011/05/19 16:07:27, dominic wrote:
> > Should this have a constructor that takes the PrerenderManager* and sets the
> > final_status to FINAL_STATUS_MAX? 
> 
> In order to do that, I'd also have to make a constructor that takes no
> arguments, since std::map works by creating a new entry with that constructor,
> and then copying the data over.  Creating such an extra constructor for that
> seems a little crufty.
> 
> I've switched to creating it on the stack and then inserting it, to avoid the
> double lookup the old code produced, though I'm open to other ideas.

I think that's only if you use operator[]. If you use:

map->insert(std::make_pair(child_route_id_pair,
RenderViewInfo(prerender_manager)));

Then you don't get the lookup or the empty constructor call. It's not exactly
performance critical, but it means the RenderViewInfo is a self-contained type
with sensible default values set at construction.

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

http://codereview.chromium.org/7038012/diff/15025/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:321: if (render_view_host_ ||
prerender_contents_.get()) {
there's no need for this prerender_contents_.get() check now.

http://codereview.chromium.org/7038012/diff/15025/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:662: DCHECK(is_cancelled);
Consider making this a CHECK if it should never be false.

http://codereview.chromium.org/7038012/diff/15025/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

http://codereview.chromium.org/7038012/diff/15025/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker_unittest.cc:31: return
(cancelled_id_pairs_.count(child_route_id_pair) > 0);
nit: Could be != instead.

[mmenke, 2011-05-19 17:09:23]

Thanks for all the great feedback.

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

http://codereview.chromium.org/7038012/diff/15025/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:321: if (render_view_host_ ||
prerender_contents_.get()) {
On 2011/05/19 16:55:47, dominic wrote:
> there's no need for this prerender_contents_.get() check now.

Good point.

http://codereview.chromium.org/7038012/diff/15025/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:662: DCHECK(is_cancelled);
On 2011/05/19 16:55:47, dominic wrote:
> Consider making this a CHECK if it should never be false.

Done.  If it ever happens, I suspect we'd crash a bit later, due to a double
delete.

http://codereview.chromium.org/7038012/diff/15025/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

http://codereview.chromium.org/7038012/diff/15025/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker_unittest.cc:31: return
(cancelled_id_pairs_.count(child_route_id_pair) > 0);
On 2011/05/19 16:55:47, dominic wrote:
> nit: Could be != instead.

Done.

[mmenke, 2011-05-19 17:19:10]

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker.cc (right):

http://codereview.chromium.org/7038012/diff/17012/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.cc:32: FinalStatus final_status;
On 2011/05/19 16:55:47, dominic wrote:
> On 2011/05/19 16:40:42, Matt Menke wrote:
> > On 2011/05/19 16:07:27, dominic wrote:
> > > Should this have a constructor that takes the PrerenderManager* and sets
the
> > > final_status to FINAL_STATUS_MAX? 
> > 
> > In order to do that, I'd also have to make a constructor that takes no
> > arguments, since std::map works by creating a new entry with that
constructor,
> > and then copying the data over.  Creating such an extra constructor for that
> > seems a little crufty.
> > 
> > I've switched to creating it on the stack and then inserting it, to avoid
the
> > double lookup the old code produced, though I'm open to other ideas.
> 
> I think that's only if you use operator[]. If you use:
> 
> map->insert(std::make_pair(child_route_id_pair,
> RenderViewInfo(prerender_manager)));
> 
> Then you don't get the lookup or the empty constructor call. It's not exactly
> performance critical, but it means the RenderViewInfo is a self-contained type
> with sensible default values set at construction.

You're right.  Done as you suggest.

[dominich, 2011-05-19 17:20:21]

LGTM

You still need jam@ to look over the ResourceDispatcherHost though.

http://codereview.chromium.org/7038012/diff/18014/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker.cc (right):

http://codereview.chromium.org/7038012/diff/18014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.cc:187: void
PrerenderTracker::AddPrerenderOnIOThreadTask(
Take these out of the class and add to an anonymous namespace at the top of the
file.

http://codereview.chromium.org/7038012/diff/18014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.cc:193: void
PrerenderTracker::RemovePrerenderOnIOThreadTask(
See above comment re AddPrerenderOnIOThreadTask.

http://codereview.chromium.org/7038012/diff/18014/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/7038012/diff/18014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:60: bool
IsPrerenderingOnIOThread(int child_id, int route_id);
Can this method be const?

http://codereview.chromium.org/7038012/diff/18014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:65: bool GetFinalStatus(int
child_id, int route_id, FinalStatus* final_status);
Can this method be const? (I'm not sure in this case as locking
final_map_status_lock_ might be a non-const op).

http://codereview.chromium.org/7038012/diff/18014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:112: // Tasks posted to the IO
Thread to call the above functions.
These don't need to be class members.

[jam, 2011-05-19 17:24:35]

On 2011/05/19 15:04:57, Matt Menke wrote:
> John:  Please review the src/content changes.  I know adding another
dependency
> could be considered a step backwards, but this does remove some of the
> prerendering code from resource_dispatcher_host, along with all its externally
> visible prerendering-related functions.

removing code from RDH is great.  adding another dependency that will have to be
removed later should be avoided.  i would prefer that we don't commit code that
we know we have to refactor later, and adds work to the existing effort to
remove all dependencies.

> 
> Timo, Dominic, Shishir:  Please review everything.

[dominich, 2011-05-19 17:26:49]

On 2011/05/19 17:24:35, John Abd-El-Malek wrote:
> On 2011/05/19 15:04:57, Matt Menke wrote:
> > John:  Please review the src/content changes.  I know adding another
> dependency
> > could be considered a step backwards, but this does remove some of the
> > prerendering code from resource_dispatcher_host, along with all its
externally
> > visible prerendering-related functions.
> 
> removing code from RDH is great.  adding another dependency that will have to
be
> removed later should be avoided.  i would prefer that we don't commit code
that
> we know we have to refactor later, and adds work to the existing effort to
> remove all dependencies.
> 

I understand your concern completely. As someone who is working on removing this
dependency, I can assure you that this CL does not add work to what I'm doing
and actually makes my work slightly easier.

[jam, 2011-05-19 17:29:56]

On Thu, May 19, 2011 at 10:26 AM, <dominich@chromium.org> wrote:

> On 2011/05/19 17:24:35, John Abd-El-Malek wrote:
>
>> On 2011/05/19 15:04:57, Matt Menke wrote:
>> > John:  Please review the src/content changes.  I know adding another
>> dependency
>> > could be considered a step backwards, but this does remove some of the
>> > prerendering code from resource_dispatcher_host, along with all its
>>
> externally
>
>> > visible prerendering-related functions.
>>
>
>  removing code from RDH is great.  adding another dependency that will have
>> to
>>
> be
>
>> removed later should be avoided.  i would prefer that we don't commit code
>>
> that
>
>> we know we have to refactor later, and adds work to the existing effort to
>> remove all dependencies.
>>
>
>
> I understand your concern completely. As someone who is working on removing
> this
> dependency, I can assure you that this CL does not add work to what I'm
> doing
> and actually makes my work slightly easier.


is there an ETA of when you expect these dependencies to be removed?

>
>
>
>
> http://codereview.chromium.org/7038012/
>

[dominich, 2011-05-19 17:33:50]

On 2011/05/19 17:29:56, John Abd-El-Malek wrote:
> On Thu, May 19, 2011 at 10:26 AM, <mailto:dominich@chromium.org> wrote:
> 
> > On 2011/05/19 17:24:35, John Abd-El-Malek wrote:
> >
> >> On 2011/05/19 15:04:57, Matt Menke wrote:
> >> > John:  Please review the src/content changes.  I know adding another
> >> dependency
> >> > could be considered a step backwards, but this does remove some of the
> >> > prerendering code from resource_dispatcher_host, along with all its
> >>
> > externally
> >
> >> > visible prerendering-related functions.
> >>
> >
> >  removing code from RDH is great.  adding another dependency that will have
> >> to
> >>
> > be
> >
> >> removed later should be avoided.  i would prefer that we don't commit code
> >>
> > that
> >
> >> we know we have to refactor later, and adds work to the existing effort to
> >> remove all dependencies.
> >>
> >
> >
> > I understand your concern completely. As someone who is working on removing
> > this
> > dependency, I can assure you that this CL does not add work to what I'm
> > doing
> > and actually makes my work slightly easier.
> 
> 
> is there an ETA of when you expect these dependencies to be removed?

I had them on the back-burner while we get Prerender ready for feature freeze,
but I can always bump up the priority. I could probably get the changes in
before the end of next week, assuming the reviewers approve of my approach.

[jam, 2011-05-19 17:36:39]

On Thu, May 19, 2011 at 10:33 AM, <dominich@chromium.org> wrote:

> On 2011/05/19 17:29:56, John Abd-El-Malek wrote:
>
>  On Thu, May 19, 2011 at 10:26 AM, <mailto:dominich@chromium.org> wrote:
>>
>
>  > On 2011/05/19 17:24:35, John Abd-El-Malek wrote:
>> >
>> >> On 2011/05/19 15:04:57, Matt Menke wrote:
>> >> > John:  Please review the src/content changes.  I know adding another
>> >> dependency
>> >> > could be considered a step backwards, but this does remove some of
>> the
>> >> > prerendering code from resource_dispatcher_host, along with all its
>> >>
>> > externally
>> >
>> >> > visible prerendering-related functions.
>> >>
>> >
>> >  removing code from RDH is great.  adding another dependency that will
>> have
>> >> to
>> >>
>> > be
>> >
>> >> removed later should be avoided.  i would prefer that we don't commit
>> code
>> >>
>> > that
>> >
>> >> we know we have to refactor later, and adds work to the existing effort
>> to
>> >> remove all dependencies.
>> >>
>> >
>> >
>> > I understand your concern completely. As someone who is working on
>> removing
>> > this
>> > dependency, I can assure you that this CL does not add work to what I'm
>> > doing
>> > and actually makes my work slightly easier.
>>
>
>
>  is there an ETA of when you expect these dependencies to be removed?
>>
>
> I had them on the back-burner while we get Prerender ready for feature
> freeze,
> but I can always bump up the priority. I could probably get the changes in
> before the end of next week, assuming the reviewers approve of my approach.


I'll give a lgtm for adding this dependency now only if the priority of the
change to remove them is high :)

>
>
>
> http://codereview.chromium.org/7038012/
>

[dominich, 2011-05-19 17:38:54]

On 2011/05/19 17:36:39, John Abd-El-Malek wrote:
> On Thu, May 19, 2011 at 10:33 AM, <mailto:dominich@chromium.org> wrote:
> 
> > On 2011/05/19 17:29:56, John Abd-El-Malek wrote:
> >
> >  On Thu, May 19, 2011 at 10:26 AM, <mailto:dominich@chromium.org> wrote:
> >>
> >
> >  > On 2011/05/19 17:24:35, John Abd-El-Malek wrote:
> >> >
> >> >> On 2011/05/19 15:04:57, Matt Menke wrote:
> >> >> > John:  Please review the src/content changes.  I know adding another
> >> >> dependency
> >> >> > could be considered a step backwards, but this does remove some of
> >> the
> >> >> > prerendering code from resource_dispatcher_host, along with all its
> >> >>
> >> > externally
> >> >
> >> >> > visible prerendering-related functions.
> >> >>
> >> >
> >> >  removing code from RDH is great.  adding another dependency that will
> >> have
> >> >> to
> >> >>
> >> > be
> >> >
> >> >> removed later should be avoided.  i would prefer that we don't commit
> >> code
> >> >>
> >> > that
> >> >
> >> >> we know we have to refactor later, and adds work to the existing effort
> >> to
> >> >> remove all dependencies.
> >> >>
> >> >
> >> >
> >> > I understand your concern completely. As someone who is working on
> >> removing
> >> > this
> >> > dependency, I can assure you that this CL does not add work to what I'm
> >> > doing
> >> > and actually makes my work slightly easier.
> >>
> >
> >
> >  is there an ETA of when you expect these dependencies to be removed?
> >>
> >
> > I had them on the back-burner while we get Prerender ready for feature
> > freeze,
> > but I can always bump up the priority. I could probably get the changes in
> > before the end of next week, assuming the reviewers approve of my approach.
> 
> 
> I'll give a lgtm for adding this dependency now only if the priority of the
> change to remove them is high :)
> 

http://crbug.com/83247

[mmenke, 2011-05-19 17:39:18]

http://codereview.chromium.org/7038012/diff/18014/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/7038012/diff/18014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:60: bool
IsPrerenderingOnIOThread(int child_id, int route_id);
On 2011/05/19 17:20:21, dominic wrote:
> Can this method be const?

Indeed it can.  Done.

http://codereview.chromium.org/7038012/diff/18014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:65: bool GetFinalStatus(int
child_id, int route_id, FinalStatus* final_status);
On 2011/05/19 17:20:21, dominic wrote:
> Can this method be const? (I'm not sure in this case as locking
> final_map_status_lock_ might be a non-const op).

I've declared it const and made the lock mutable.

http://codereview.chromium.org/7038012/diff/18014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:112: // Tasks posted to the IO
Thread to call the above functions.
On 2011/05/19 17:20:21, dominic wrote:
> These don't need to be class members.

They access private functions, so they do, unless I make the functions they call
public.  They could be friend functions, I suppose, but I don't think that helps
us any.

[jam, 2011-05-19 17:41:44]

On Thu, May 19, 2011 at 10:38 AM, <dominich@chromium.org> wrote:

> On 2011/05/19 17:36:39, John Abd-El-Malek wrote:
>
>  On Thu, May 19, 2011 at 10:33 AM, <mailto:dominich@chromium.org> wrote:
>>
>
>  > On 2011/05/19 17:29:56, John Abd-El-Malek wrote:
>> >
>> >  On Thu, May 19, 2011 at 10:26 AM, <mailto:dominich@chromium.org>
>> wrote:
>> >>
>> >
>> >  > On 2011/05/19 17:24:35, John Abd-El-Malek wrote:
>> >> >
>> >> >> On 2011/05/19 15:04:57, Matt Menke wrote:
>> >> >> > John:  Please review the src/content changes.  I know adding
>> another
>> >> >> dependency
>> >> >> > could be considered a step backwards, but this does remove some of
>> >> the
>> >> >> > prerendering code from resource_dispatcher_host, along with all
>> its
>> >> >>
>> >> > externally
>> >> >
>> >> >> > visible prerendering-related functions.
>> >> >>
>> >> >
>> >> >  removing code from RDH is great.  adding another dependency that
>> will
>> >> have
>> >> >> to
>> >> >>
>> >> > be
>> >> >
>> >> >> removed later should be avoided.  i would prefer that we don't
>> commit
>> >> code
>> >> >>
>> >> > that
>> >> >
>> >> >> we know we have to refactor later, and adds work to the existing
>> effort
>> >> to
>> >> >> remove all dependencies.
>> >> >>
>> >> >
>> >> >
>> >> > I understand your concern completely. As someone who is working on
>> >> removing
>> >> > this
>> >> > dependency, I can assure you that this CL does not add work to what
>> I'm
>> >> > doing
>> >> > and actually makes my work slightly easier.
>> >>
>> >
>> >
>> >  is there an ETA of when you expect these dependencies to be removed?
>> >>
>> >
>> > I had them on the back-burner while we get Prerender ready for feature
>> > freeze,
>> > but I can always bump up the priority. I could probably get the changes
>> in
>> > before the end of next week, assuming the reviewers approve of my
>> approach.
>>
>
>
>  I'll give a lgtm for adding this dependency now only if the priority of
>> the
>> change to remove them is high :)
>>
>
content lgtm, thanks

>
>
> http://crbug.com/83247
>
>
> http://codereview.chromium.org/7038012/
>

[commit-bot: I haz the power, 2011-05-20 14:55:04]

Can't apply patch for file content/browser/DEPS.
patching file content/browser/DEPS
Hunk #1 FAILED at 62.
1 out of 1 hunk FAILED -- saving rejects to file content/browser/DEPS.rej

[cbentzel, 2011-05-23 12:08:14]

LGTM

This was already landed, but a few small comments. I'll use this for the SSL
Client Auth cancellation, and possibly as an approach for HTTP auth.

http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker.cc (right):

http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.cc:143: FinalStatus
PrerenderTracker::SetFinalStatus(int child_id, int route_id,
I think it would be clearer if this signature changed to

bool PrerenderTracker::SetFinalStatus(int child_id, int route_id, FinalStatus
desired_final_status, FinalStatus* actual_final_status)

it would also be more symmetric with GetFinalStatus

false would be returned if the status was already set.

http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_tracker.h:68: protected:
It looks like there is no need for the protected section - no inheritance is
done.

[dominich, 2011-05-23 15:33:39]

On 2011/05/23 12:08:14, cbentzel wrote:
> LGTM
> 
> This was already landed, but a few small comments. I'll use this for the SSL
> Client Auth cancellation, and possibly as an approach for HTTP auth.
> 
>
http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_tracker.cc (right):
> 
>
http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_tracker.cc:143: FinalStatus
> PrerenderTracker::SetFinalStatus(int child_id, int route_id,
> I think it would be clearer if this signature changed to
> 
> bool PrerenderTracker::SetFinalStatus(int child_id, int route_id, FinalStatus
> desired_final_status, FinalStatus* actual_final_status)
> 
> it would also be more symmetric with GetFinalStatus
> 
> false would be returned if the status was already set.
> 
>
http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_tracker.h (right):
> 
>
http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_tracker.h:68: protected:
> It looks like there is no need for the protected section - no inheritance is
> done.

I'm touching this code so I can make these changes as part of that change if you
want.

[mmenke, 2011-05-23 15:37:45]

On 2011/05/23 15:33:39, dominic wrote:
> On 2011/05/23 12:08:14, cbentzel wrote:
> > LGTM
> > 
> > This was already landed, but a few small comments. I'll use this for the SSL
> > Client Auth cancellation, and possibly as an approach for HTTP auth.
> > 
> >
>
http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
> > File chrome/browser/prerender/prerender_tracker.cc (right):
> > 
> >
>
http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
> > chrome/browser/prerender/prerender_tracker.cc:143: FinalStatus
> > PrerenderTracker::SetFinalStatus(int child_id, int route_id,
> > I think it would be clearer if this signature changed to
> > 
> > bool PrerenderTracker::SetFinalStatus(int child_id, int route_id,
FinalStatus
> > desired_final_status, FinalStatus* actual_final_status)
> > 
> > it would also be more symmetric with GetFinalStatus
> > 
> > false would be returned if the status was already set.
> > 
> >
>
http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
> > File chrome/browser/prerender/prerender_tracker.h (right):
> > 
> >
>
http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
> > chrome/browser/prerender/prerender_tracker.h:68: protected:
> > It looks like there is no need for the protected section - no inheritance is
> > done.
> 
> I'm touching this code so I can make these changes as part of that change if
you
> want.

I'm happy to do it myself - already have a CL running through the trybots
(http://codereview.chromium.org/7060012/).  I don't think it'll conflict with
your changes, however, if incorporating it into your CL makes life easier on
you, just say so.

[dominich.google, 2011-05-23 15:39:46]

On Mon, May 23, 2011 at 8:37 AM, <mmenke@chromium.org> wrote:

> On 2011/05/23 15:33:39, dominic wrote:
>
>> On 2011/05/23 12:08:14, cbentzel wrote:
>> > LGTM
>> >
>> > This was already landed, but a few small comments. I'll use this for the
>> SSL
>> > Client Auth cancellation, and possibly as an approach for HTTP auth.
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
>
>> > File chrome/browser/prerender/prerender_tracker.cc (right):
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
>
>> > chrome/browser/prerender/prerender_tracker.cc:143: FinalStatus
>> > PrerenderTracker::SetFinalStatus(int child_id, int route_id,
>> > I think it would be clearer if this signature changed to
>> >
>> > bool PrerenderTracker::SetFinalStatus(int child_id, int route_id,
>>
> FinalStatus
>
>> > desired_final_status, FinalStatus* actual_final_status)
>> >
>> > it would also be more symmetric with GetFinalStatus
>> >
>> > false would be returned if the status was already set.
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
>
>> > File chrome/browser/prerender/prerender_tracker.h (right):
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/7038012/diff/25001/chrome/browser/prerender/pr...
>
>> > chrome/browser/prerender/prerender_tracker.h:68: protected:
>> > It looks like there is no need for the protected section - no
>> inheritance is
>> > done.
>>
>
>  I'm touching this code so I can make these changes as part of that change
>> if
>>
> you
>
>> want.
>>
>
> I'm happy to do it myself - already have a CL running through the trybots
> (http://codereview.chromium.org/7060012/).  I don't think it'll conflict
> with
> your changes, however, if incorporating it into your CL makes life easier
> on
> you, just say so.
>
>
>
Go ahead. Thanks.