Introduce RenderView::Observer interface so that RenderView doesn't have to k...

Introduce RenderView::Observer interface so that RenderView doesn't have to know about the details of every feature.  Observers get to filter and send IPC messages, and basic notifications of frame related events.I've moved over AutoFill related classes, and also made AutoFillManager implement the new WebAutoFillClient interface.  For the rest of the classes, they implement the interface just for message filtering.

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

[Ilya Sherman, 2011-01-13 02:18:30]

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
File chrome/renderer/autofill_helper.cc (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
chrome/renderer/autofill_helper.cc:144: }
Normally, we use "autocomplete" within this file to refer to the form
autocomplete feature.  It looks like this method is password-autocomplete
specific.  Can we either move into the password autocomplete manager code, or
rename it here?

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
File chrome/renderer/autofill_helper.h (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
chrome/renderer/autofill_helper.h:75: const WebKit::WebString& value);
nit: indentation

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view...
chrome/renderer/render_view.cc:967: password_autocomplete_manager);
nit: Might be nice to have a quick comment mentioning how memory is managed for
these.

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view...
chrome/renderer/render_view.h:207: int routing_id() { return routing_id_; }
nit: please add a blank line below

[jam, 2011-01-13 02:53:43]

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
File chrome/renderer/autofill_helper.cc (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
chrome/renderer/autofill_helper.cc:144: }
On 2011/01/13 02:18:30, Ilya Sherman wrote:
> Normally, we use "autocomplete" within this file to refer to the form
> autocomplete feature.  It looks like this method is password-autocomplete
> specific.  Can we either move into the password autocomplete manager code, or
> rename it here?

agree it's misnamed, but this is from the WebKit API so it's harder to rename
(would need yet another 2-sided patch).  Given that it was like that before, and
I'm just moving it, I think it's orthogonal to this patch

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
File chrome/renderer/autofill_helper.h (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
chrome/renderer/autofill_helper.h:75: const WebKit::WebString& value);
On 2011/01/13 02:18:30, Ilya Sherman wrote:
> nit: indentation

Done.

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view...
chrome/renderer/render_view.cc:967: password_autocomplete_manager);
On 2011/01/13 02:18:30, Ilya Sherman wrote:
> nit: Might be nice to have a quick comment mentioning how memory is managed
for
> these.

There's a comment beside the Observer interface, I'd rather not duplicate it

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view...
chrome/renderer/render_view.h:207: int routing_id() { return routing_id_; }
On 2011/01/13 02:18:30, Ilya Sherman wrote:
> nit: please add a blank line below

Done.

[Ilya Sherman, 2011-01-13 18:34:49]

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
File chrome/renderer/autofill_helper.cc (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
chrome/renderer/autofill_helper.cc:144: }
On 2011/01/13 02:53:43, John Abd-El-Malek wrote:
> On 2011/01/13 02:18:30, Ilya Sherman wrote:
> > Normally, we use "autocomplete" within this file to refer to the form
> > autocomplete feature.  It looks like this method is password-autocomplete
> > specific.  Can we either move into the password autocomplete manager code,
or
> > rename it here?
> 
> agree it's misnamed, but this is from the WebKit API so it's harder to rename
> (would need yet another 2-sided patch).  Given that it was like that before,
and
> I'm just moving it, I think it's orthogonal to this patch

Can this be moved into the password autocomplete manager source file, rather
than living in autofill_helper.cc?  If not, please at least add a brief comment
(perhaps a TODO to rename the method).

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view...
chrome/renderer/render_view.cc:967: password_autocomplete_manager);
On 2011/01/13 02:53:43, John Abd-El-Malek wrote:
> On 2011/01/13 02:18:30, Ilya Sherman wrote:
> > nit: Might be nice to have a quick comment mentioning how memory is managed
> for
> > these.
> 
> There's a comment beside the Observer interface, I'd rather not duplicate it

That comment is really visible when looking at the changes from this patch, but
a month down the line it'll be harder to know to look there when reading these
lines.  Your call, but I think a (very brief) comment here would be helpful.

[jam, 2011-01-13 18:40:53]

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
File chrome/renderer/autofill_helper.cc (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
chrome/renderer/autofill_helper.cc:144: }
On 2011/01/13 18:34:50, Ilya Sherman wrote:
> On 2011/01/13 02:53:43, John Abd-El-Malek wrote:
> > On 2011/01/13 02:18:30, Ilya Sherman wrote:
> > > Normally, we use "autocomplete" within this file to refer to the form
> > > autocomplete feature.  It looks like this method is password-autocomplete
> > > specific.  Can we either move into the password autocomplete manager code,
> or
> > > rename it here?
> > 
> > agree it's misnamed, but this is from the WebKit API so it's harder to
rename
> > (would need yet another 2-sided patch).  Given that it was like that before,
> and
> > I'm just moving it, I think it's orthogonal to this patch
> 
> Can this be moved into the password autocomplete manager source file, rather
> than living in autofill_helper.cc?  If not, please at least add a brief
comment
> (perhaps a TODO to rename the method).

it can't because it's part of the WebAutoFillClient interface, the big hint
being that the function name is using WebKit style.  Again, I'm not in favor of
documenting stuff that should be obvious, otherwise we'd have to do this all
over the RenderView code etc

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view...
chrome/renderer/render_view.cc:967: password_autocomplete_manager);
On 2011/01/13 18:34:50, Ilya Sherman wrote:
> On 2011/01/13 02:53:43, John Abd-El-Malek wrote:
> > On 2011/01/13 02:18:30, Ilya Sherman wrote:
> > > nit: Might be nice to have a quick comment mentioning how memory is
managed
> > for
> > > these.
> > 
> > There's a comment beside the Observer interface, I'd rather not duplicate it
> 
> That comment is really visible when looking at the changes from this patch,
but
> a month down the line it'll be harder to know to look there when reading these
> lines.  Your call, but I think a (very brief) comment here would be helpful.

My main issue with documenting stuff multiple times is that they'll go out of
sync, which is more confusing than not having comments.  Also, I think if
someone is using an interface, they should be familiar with it.

[Ilya Sherman, 2011-01-13 22:34:51]

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
File chrome/renderer/autofill_helper.cc (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
chrome/renderer/autofill_helper.cc:144: }
On 2011/01/13 18:40:53, John Abd-El-Malek wrote:
> On 2011/01/13 18:34:50, Ilya Sherman wrote:
> > On 2011/01/13 02:53:43, John Abd-El-Malek wrote:
> > > On 2011/01/13 02:18:30, Ilya Sherman wrote:
> > > > Normally, we use "autocomplete" within this file to refer to the form
> > > > autocomplete feature.  It looks like this method is
password-autocomplete
> > > > specific.  Can we either move into the password autocomplete manager
code,
> > or
> > > > rename it here?
> > > 
> > > agree it's misnamed, but this is from the WebKit API so it's harder to
> rename
> > > (would need yet another 2-sided patch).  Given that it was like that
before,
> > and
> > > I'm just moving it, I think it's orthogonal to this patch
> > 
> > Can this be moved into the password autocomplete manager source file, rather
> > than living in autofill_helper.cc?  If not, please at least add a brief
> comment
> > (perhaps a TODO to rename the method).
> 
> it can't because it's part of the WebAutoFillClient interface, the big hint
> being that the function name is using WebKit style.

O.k.  I'm not sure that this really belongs in the WebAutoFillClient interface
in WebKit, but you're right that that's orthogonal to this cl.

> Again, I'm not in favor of documenting stuff that should be obvious,
> otherwise we'd have to do this all over the RenderView code etc

This doesn't seem obvious to me.  Based on the function's name, the obvious
reading is that all form field suggestions call into this code.  I think a brief
comment like

// TODO(isherman): Rename this function to
|didAcceptPasswordAutocompleteSuggestion()|

is appropriate (yes, you can assign the TODO to me).

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view...
chrome/renderer/render_view.cc:967: password_autocomplete_manager);
On 2011/01/13 18:40:53, John Abd-El-Malek wrote:
> On 2011/01/13 18:34:50, Ilya Sherman wrote:
> > On 2011/01/13 02:53:43, John Abd-El-Malek wrote:
> > > On 2011/01/13 02:18:30, Ilya Sherman wrote:
> > > > nit: Might be nice to have a quick comment mentioning how memory is
> managed
> > > for
> > > > these.
> > > 
> > > There's a comment beside the Observer interface, I'd rather not duplicate
it
> > 
> > That comment is really visible when looking at the changes from this patch,
> but
> > a month down the line it'll be harder to know to look there when reading
these
> > lines.  Your call, but I think a (very brief) comment here would be helpful.
> 
> My main issue with documenting stuff multiple times is that they'll go out of
> sync, which is more confusing than not having comments.  Also, I think if
> someone is using an interface, they should be familiar with it.

It's not reasonable to expect everyone reading this code to know all of the
details of the Observer interface, and the interface has a decidedly
non-standard approach to memory management.  Without a comment, the immediate
reaction is "Huh, I wonder why there's an unprotected memory allocation."  Sure,
it's not very hard to figure out what's actually going on, but it's an
unnecessary distraction.  A comment like

// Observers will be deleted when the RenderView goes away.

alleviates this; and if it goes out of sync, the code probably needs to be
updated as well.

[jam, 2011-01-13 22:52:24]

On Thu, Jan 13, 2011 at 2:34 PM, <isherman@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
> File chrome/renderer/autofill_helper.cc (right):
>
>
>
http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/autofill_he...
> chrome/renderer/autofill_helper.cc:144: }
> On 2011/01/13 18:40:53, John Abd-El-Malek wrote:
>
>> On 2011/01/13 18:34:50, Ilya Sherman wrote:
>> > On 2011/01/13 02:53:43, John Abd-El-Malek wrote:
>> > > On 2011/01/13 02:18:30, Ilya Sherman wrote:
>> > > > Normally, we use "autocomplete" within this file to refer to the
>>
> form
>
>> > > > autocomplete feature.  It looks like this method is
>>
> password-autocomplete
>
>> > > > specific.  Can we either move into the password autocomplete
>>
> manager code,
>
>> > or
>> > > > rename it here?
>> > >
>> > > agree it's misnamed, but this is from the WebKit API so it's
>>
> harder to
>
>> rename
>> > > (would need yet another 2-sided patch).  Given that it was like
>>
> that before,
>
>> > and
>> > > I'm just moving it, I think it's orthogonal to this patch
>> >
>> > Can this be moved into the password autocomplete manager source
>>
> file, rather
>
>> > than living in autofill_helper.cc?  If not, please at least add a
>>
> brief
>
>> comment
>> > (perhaps a TODO to rename the method).
>>
>
>  it can't because it's part of the WebAutoFillClient interface, the big
>>
> hint
>
>> being that the function name is using WebKit style.
>>
>
> O.k.  I'm not sure that this really belongs in the WebAutoFillClient
> interface in WebKit, but you're right that that's orthogonal to this cl.
>
>
>  Again, I'm not in favor of documenting stuff that should be obvious,
>> otherwise we'd have to do this all over the RenderView code etc
>>
>
> This doesn't seem obvious to me.  Based on the function's name, the
> obvious reading is that all form field suggestions call into this code.
> I think a brief comment like
>
> // TODO(isherman): Rename this function to
> |didAcceptPasswordAutocompleteSuggestion()|
>
> is appropriate (yes, you can assign the TODO to me).


For this comment and the above one, the goal as I understand it is to get
rid of these interfaces at all.  This is to provide the necessary
feature-independent hooks without having to touch WebKit for each feature.
 PageClickListener was the start of that effort but it seems to have been
stopped. See http://crbug.com/51644.

We can of course add TODOs, but not only do they go out of sync, but the
usual style is to create bugs to track this and have them assigned, instead
of TODOs which are ignored.  I ran into a number of old TODOs that I ran
into while refactoring the autofill code.  Some were just never done, while
others were wrong and wasted a lot of time.

>
>
>
>
http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view.cc
> File chrome/renderer/render_view.cc (right):
>
>
>
http://codereview.chromium.org/6151011/diff/10002/chrome/renderer/render_view...
> chrome/renderer/render_view.cc:967: password_autocomplete_manager);
> On 2011/01/13 18:40:53, John Abd-El-Malek wrote:
>
>> On 2011/01/13 18:34:50, Ilya Sherman wrote:
>> > On 2011/01/13 02:53:43, John Abd-El-Malek wrote:
>> > > On 2011/01/13 02:18:30, Ilya Sherman wrote:
>> > > > nit: Might be nice to have a quick comment mentioning how memory
>>
> is
>
>> managed
>> > > for
>> > > > these.
>> > >
>> > > There's a comment beside the Observer interface, I'd rather not
>>
> duplicate it
>
>> >
>> > That comment is really visible when looking at the changes from this
>>
> patch,
>
>> but
>> > a month down the line it'll be harder to know to look there when
>>
> reading these
>
>> > lines.  Your call, but I think a (very brief) comment here would be
>>
> helpful.
>
>  My main issue with documenting stuff multiple times is that they'll go
>>
> out of
>
>> sync, which is more confusing than not having comments.  Also, I think
>>
> if
>
>> someone is using an interface, they should be familiar with it.
>>
>
> It's not reasonable to expect everyone reading this code to know all of
> the details of the Observer interface, and the interface has a decidedly
> non-standard approach to memory management.  Without a comment, the
> immediate reaction is "Huh, I wonder why there's an unprotected memory
> allocation."  Sure, it's not very hard to figure out what's actually
> going on, but it's an unnecessary distraction.


hmm, I feel this is one of those areas where I feel this would be
overdocumenting.  There are a bunch of places where this is done right now,
and that will increase, and I prefer not to have to repeat the same comment
everywhere.


>  A comment like
>
> // Observers will be deleted when the RenderView goes away.
>
> alleviates this; and if it goes out of sync, the code probably needs to
> be updated as well.
>
>
> http://codereview.chromium.org/6151011/
>

[Ilya Sherman, 2011-01-13 23:20:16]

> For this comment and the above one, the goal as I understand it is to get
> rid of these interfaces at all.  This is to provide the necessary
> feature-independent hooks without having to touch WebKit for each feature.
>  PageClickListener was the start of that effort but it seems to have been
> stopped. See http://crbug.com/51644.

Right, that's the desired long-term solution.  Jay Civelli was working on that,
but is now busy with other things.

> We can of course add TODOs, but not only do they go out of sync, but the
> usual style is to create bugs to track this and have them assigned, instead
> of TODOs which are ignored.  I ran into a number of old TODOs that I ran
> into while refactoring the autofill code.  Some were just never done, while
> others were wrong and wasted a lot of time.

Bugs can be ignored and go out of sync just as easily as TODOs -- in fact a
little more easily, as they're further from the code ;)

Practically speaking, this sort of change seems better to track in a TODO rather
than a bug: It's a small style issue that has a WebKit dependency, and it's not
the sort of comment that will waste a lot of time if it goes out of sync.  That
said, feel free to file a bug or fix this directly if you prefer.


> > It's not reasonable to expect everyone reading this code to know all of
> > the details of the Observer interface, and the interface has a decidedly
> > non-standard approach to memory management.  Without a comment, the
> > immediate reaction is "Huh, I wonder why there's an unprotected memory
> > allocation."  Sure, it's not very hard to figure out what's actually
> > going on, but it's an unnecessary distraction.
> 
> 
> hmm, I feel this is one of those areas where I feel this would be
> overdocumenting.  There are a bunch of places where this is done right now,
> and that will increase, and I prefer not to have to repeat the same comment
> everywhere.

Take a step back for a moment:  Suppose you're reading through an unfamiliar bit
of Chromium code, and you see the line "Foo* f = new Foo();".
 (a) Would you wonder whether these are unsafe allocations?
 (b) Would you immediately think to check whether there is some reason these
objects' lifetimes are managed by another's, and quickly be able to find the
appropriate interface and reassure yourself?
 (c) Would you, as a reader, prefer if there were a quick comment to the effect
of "This looks wrong, but it's actually ok"?

If your answer to (c) is genuinely "no", then I guess leave the comment out -- I
doubt there's a more convincing reason to include it, and it's not really
important enough to quibble over.

Another option, of course, is to design the interface so that the memory
management is more obvious...

[jam, 2011-01-13 23:32:54]

On Thu, Jan 13, 2011 at 3:20 PM, <isherman@chromium.org> wrote:

> For this comment and the above one, the goal as I understand it is to get
>> rid of these interfaces at all.  This is to provide the necessary
>> feature-independent hooks without having to touch WebKit for each feature.
>>  PageClickListener was the start of that effort but it seems to have been
>> stopped. See http://crbug.com/51644.
>>
>
> Right, that's the desired long-term solution.  Jay Civelli was working on
> that,
> but is now busy with other things.
>
>
>  We can of course add TODOs, but not only do they go out of sync, but the
>> usual style is to create bugs to track this and have them assigned,
>> instead
>> of TODOs which are ignored.  I ran into a number of old TODOs that I ran
>> into while refactoring the autofill code.  Some were just never done,
>> while
>> others were wrong and wasted a lot of time.
>>
>
> Bugs can be ignored and go out of sync just as easily as TODOs -- in fact a
> little more easily, as they're further from the code ;)
>
> Practically speaking, this sort of change seems better to track in a TODO
> rather
> than a bug: It's a small style issue that has a WebKit dependency, and it's
> not
> the sort of comment that will waste a lot of time if it goes out of sync.
>  That
> said, feel free to file a bug or fix this directly if you prefer.


There is already a bug to take this function out, so I don't think another
one to rename it is necessary.  btw there was a TODO about renaming some of
the WebKit autofill callback names (i.e. removeAutofillSuggestions) for ~14
months, but it was never done.

>
>
>
>  > It's not reasonable to expect everyone reading this code to know all of
>> > the details of the Observer interface, and the interface has a decidedly
>> > non-standard approach to memory management.  Without a comment, the
>> > immediate reaction is "Huh, I wonder why there's an unprotected memory
>> > allocation."  Sure, it's not very hard to figure out what's actually
>> > going on, but it's an unnecessary distraction.
>>
>
>
>  hmm, I feel this is one of those areas where I feel this would be
>> overdocumenting.  There are a bunch of places where this is done right
>> now,
>> and that will increase, and I prefer not to have to repeat the same
>> comment
>> everywhere.
>>
>
> Take a step back for a moment:  Suppose you're reading through an
> unfamiliar bit
> of Chromium code, and you see the line "Foo* f = new Foo();".
>  (a) Would you wonder whether these are unsafe allocations?
>  (b) Would you immediately think to check whether there is some reason
> these
> objects' lifetimes are managed by another's, and quickly be able to find
> the
> appropriate interface and reassure yourself?
>  (c) Would you, as a reader, prefer if there were a quick comment to the
> effect
> of "This looks wrong, but it's actually ok"?
>
> If your answer to (c) is genuinely "no", then I guess leave the comment out
> -- I
> doubt there's a more convincing reason to include it, and it's not really
> important enough to quibble over.
>
> Another option, of course, is to design the interface so that the memory
> management is more obvious...


There is a lot of Chrome code that I don't understand from a quick glance.
 If I'm curious about something though, I would follow the code to figure
out what happens (i.e. your option b).  I think there's a judgement call
about how much time you want another person reading it to spend figuring out
things before commenting it.  When something is very obvious, then we don't
leave a comment because that goes against our style guide of document the
code.  When something is very tricky, then we do ask people to write
comments so that a reader can understand what's happening.  In this case, I
don't think it's tricky, because it can be figured out quickly by someone
who wants to change it.

The way the interface is done (which changed slightly right now), is that
classes which inherit from RenderView::Observer take the RenderView* at
creation and Observer's constructor registers them.  To change this so that
memory management is manual would be adding unnecessary code in my opinion.
 i.e. one would probably have to make this interface ref counted, which I
don't think it's necessary, and would involve collisions with other classes
that derive from refcounted through other parents.  The nice thing about
this is that there's no way to screw up (i.e. creating an observer and
forgetting to register it or more importantly, having an observer outlive
the renderview or vice versa leading to crashes).


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

[brettw, 2011-01-14 02:20:05]

http://codereview.chromium.org/6151011/diff/84001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6151011/diff/84001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:1005: for (size_t i = 0; i < observers_.size();
++i) {
You should probably use base/observer_list and NOTIFY_ALL_OBSERVERS. The
ObserverList makes sure nothing bad happens if one of the guys you're notifying
modifies the observer list, which is one thing we had perennial problems with in
Chrome. The macro probably won't help here since you need to return early, but I
think you can copy the code from there.

http://codereview.chromium.org/6151011/diff/84001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6151011/diff/84001/chrome/renderer/render_view...
chrome/renderer/render_view.h:181: class Observer : public
IPC::Channel::Listener,
Personally, I would split this out into a separate RenderViewObsever class and
header file. This should reduce the size of the (huge) render_view.h file, and
mean that the headers of the components that implement this don't have to
include render_view.h and all of its dependencies.

[dhollowa, 2011-01-14 16:40:47]

http://codereview.chromium.org/6151011/diff/84001/chrome/renderer/page_click_...
File chrome/renderer/page_click_tracker_unittest.cc (right):

http://codereview.chromium.org/6151011/diff/84001/chrome/renderer/page_click_...
chrome/renderer/page_click_tracker_unittest.cc:54: PageClickTracker*
page_click_tracker = new PageClickTracker(view_);
Leak?  I'm not seeing where |page_click_tracker| goes away.  A simple stack
variable would be better.

http://codereview.chromium.org/6151011/diff/84001/chrome/test/render_view_tes...
File chrome/test/render_view_test.cc (right):

http://codereview.chromium.org/6151011/diff/84001/chrome/test/render_view_tes...
chrome/test/render_view_test.cc:14: #include "chrome/renderer/autofill_helper.h"
I'm slowly growing to hate the name "Helper".  Not this CL... but any
suggestions for a better name?  "autofill_render_delegate" seems too long.  *Me:
wonders about the word "rep" for this role.  autofill_rep, password_rep, etc. 
Or maybe "filter"?

[jam, 2011-01-14 23:00:03]

did Brett's suggestions

http://codereview.chromium.org/6151011/diff/84001/chrome/renderer/page_click_...
File chrome/renderer/page_click_tracker_unittest.cc (right):

http://codereview.chromium.org/6151011/diff/84001/chrome/renderer/page_click_...
chrome/renderer/page_click_tracker_unittest.cc:54: PageClickTracker*
page_click_tracker = new PageClickTracker(view_);
On 2011/01/14 16:40:48, dhollowa wrote:
> Leak?  I'm not seeing where |page_click_tracker| goes away.  A simple stack
> variable would be better.

RenderView manages the observers and will delete them when it goes away

http://codereview.chromium.org/6151011/diff/84001/chrome/test/render_view_tes...
File chrome/test/render_view_test.cc (right):

http://codereview.chromium.org/6151011/diff/84001/chrome/test/render_view_tes...
chrome/test/render_view_test.cc:14: #include "chrome/renderer/autofill_helper.h"
On 2011/01/14 16:40:48, dhollowa wrote:
> I'm slowly growing to hate the name "Helper".  Not this CL... but any
> suggestions for a better name?  "autofill_render_delegate" seems too long. 
*Me:
> wonders about the word "rep" for this role.  autofill_rep, password_rep, etc. 
> Or maybe "filter"?

we use filter for classes that filter messages on different threads, so I'd
prefer we keep it for that only to avoid confusion.  re names, not sure off the
top of my head.

[brettw, 2011-01-14 23:06:19]

LGTM

[dhollowa, 2011-01-14 23:21:22]

http://codereview.chromium.org/6151011/diff/84001/chrome/renderer/page_click_...
File chrome/renderer/page_click_tracker_unittest.cc (right):

http://codereview.chromium.org/6151011/diff/84001/chrome/renderer/page_click_...
chrome/renderer/page_click_tracker_unittest.cc:54: PageClickTracker*
page_click_tracker = new PageClickTracker(view_);
Really?  But there is a raw "new PageClickTracker(...);" call here, and I see no
transfer of ownership anywhere.  RemoveListener doesn't delete |this| or
anything.  What am I missing?

Certainly the original code had a |view_| that clearly owned the tracker, but
not with the new code.

On 2011/01/14 23:00:03, John Abd-El-Malek wrote:
> On 2011/01/14 16:40:48, dhollowa wrote:
> > Leak?  I'm not seeing where |page_click_tracker| goes away.  A simple stack
> > variable would be better.
> 
> RenderView manages the observers and will delete them when it goes away

[jam, 2011-01-15 00:06:50]

http://codereview.chromium.org/6151011/diff/84001/chrome/renderer/page_click_...
File chrome/renderer/page_click_tracker_unittest.cc (right):

http://codereview.chromium.org/6151011/diff/84001/chrome/renderer/page_click_...
chrome/renderer/page_click_tracker_unittest.cc:54: PageClickTracker*
page_click_tracker = new PageClickTracker(view_);
check out render_view_observer.h.  The objects become owned by RenderView.

[dhollowa, 2011-01-15 18:24:23]

LGTM.  Sorry for the delay.