Cleanup: Stop creating RenderViewObservers from chrome/ in RenderViewer.

Cleanup: Stop creating RenderViewObservers from chrome/ in RenderView.

BUG=76795
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80818

[Lei Zhang, 2011-03-29 01:52:54]

This is a rough draft. If you feel this is going in the right direction, then
I'll clean it up.

My primary goal in this CL is to modify RenderView's constructor to stop
instantiating classes that implement RenderViewObserver in the chrome/
directory.

[jam, 2011-03-29 18:31:59]

thanks for starting on this.  i have a few rough ideas for how i was thinking of
doing this, so maybe a quick chat would be good to sync up

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
chrome/renderer/render_thread.cc:10: #include <set>
nit: needed?

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
chrome/renderer/render_thread.cc:48: #include
"chrome/renderer/render_view_wrapper.h"
I'm actually working on removing all the chrome dependencies from RenderThread,
so that I can move it to content (a lot of it is not chrome specific, and will
be needed there, so I don't want to duplicate it).  so my thinking was we would
call through the embedder api (i.e. ContentRendererClient) to inform the
embedder that a RenderView has been created.  then Chrome can add its list of
observers

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
File chrome/renderer/render_view_browsertest.cc (right):

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
chrome/renderer/render_view_browsertest.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: does this need to be moved now?  i'm planning on putting a temporary
override for deps checking of test files until we figure out what the testing
situation will look like.  because we will want to have tests like this for the
content layer

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
File chrome/renderer/render_view_wrapper.h (right):

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
chrome/renderer/render_view_wrapper.h:22: class RenderViewWrapper : public
RenderViewObserver {
it seems a little confusing to call this RenderViewWrapper, since it's just
another observer, and doesn't actually contain a RenderView as a member (i.e.
wrapping it).

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
chrome/renderer/render_view_wrapper.h:33: SearchBox* searchbox() const;
The interface of RenderViewObserver allows the implementor to delete themselves
at any time. So it would seem unfortunate to have weak pointers to RVO's, since
that will eventually lead to using deleted objects.  I realize that a few of the
RVO implementations will need to be accessed given a RV.  I wonder if we can
have some templatized helper that these objects can use to automatically
register/unregister themself on creation and then have a getter based on a
RenderView?

As for the tests, I think we just need to restructure them a bit so that we
don't need to keep the PrintWebViewHelper and other objects as a weak pointer.

http://codereview.chromium.org/6750018/diff/7024/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/6750018/diff/7024/content/renderer/render_view...
content/renderer/render_view.cc:2213: RequestTextChecking(text, document_tag_,
completion));
http://codereview.chromium.org/6764001/ removes the spellchecking functions from
RV completely.  that way, the content layer and the embedder api don't have to
know anything about it

[Lei Zhang, 2011-03-30 22:05:43]

I tried to implement your ideas in patch set 4. I didn't move all the chrome
RVOs out of RVs. There's still a couple left in there.

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
chrome/renderer/render_thread.cc:10: #include <set>
On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
> nit: needed?

I was following the linter, but it's a moot point since I reverted all changes
to render_thread.cc.

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
chrome/renderer/render_thread.cc:48: #include
"chrome/renderer/render_view_wrapper.h"
On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
> I'm actually working on removing all the chrome dependencies from
RenderThread,
> so that I can move it to content (a lot of it is not chrome specific, and will
> be needed there, so I don't want to duplicate it).  so my thinking was we
would
> call through the embedder api (i.e. ContentRendererClient) to inform the
> embedder that a RenderView has been created.  then Chrome can add its list of
> observers

Ok. I did that in the next patchset.

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
File chrome/renderer/render_view_browsertest.cc (right):

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
chrome/renderer/render_view_browsertest.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
> nit: does this need to be moved now?  i'm planning on putting a temporary
> override for deps checking of test files until we figure out what the testing
> situation will look like.  because we will want to have tests like this for
the
> content layer

No, reverted.

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
File chrome/renderer/render_view_wrapper.h (right):

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
chrome/renderer/render_view_wrapper.h:22: class RenderViewWrapper : public
RenderViewObserver {
On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
> it seems a little confusing to call this RenderViewWrapper, since it's just
> another observer, and doesn't actually contain a RenderView as a member (i.e.
> wrapping it).

It was a poorly chosen name, but I had to name it something. In any case, this
is gone now.

http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
chrome/renderer/render_view_wrapper.h:33: SearchBox* searchbox() const;
On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
> The interface of RenderViewObserver allows the implementor to delete
themselves
> at any time. So it would seem unfortunate to have weak pointers to RVO's,
since
> that will eventually lead to using deleted objects.  I realize that a few of
the
> RVO implementations will need to be accessed given a RV.  I wonder if we can
> have some templatized helper that these objects can use to automatically
> register/unregister themself on creation and then have a getter based on a
> RenderView?

I don't have a templatized helper, but I added a |type_| string member to RVOs,
and for the ones we care about, we can set it to a unique per class value. I
also added RenderView::GetObserver(std::string type) to retrieve the RVO we care
about for a given RV.

> As for the tests, I think we just need to restructure them a bit so that we
> don't need to keep the PrintWebViewHelper and other objects as a weak pointer.

Ya, I got rid of the weak pointers because I added the method above.

http://codereview.chromium.org/6750018/diff/7024/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/6750018/diff/7024/content/renderer/render_view...
content/renderer/render_view.cc:2213: RequestTextChecking(text, document_tag_,
completion));
On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
> http://codereview.chromium.org/6764001/ removes the spellchecking functions
from
> RV completely.  that way, the content layer and the embedder api don't have to
> know anything about it

Right, but we still have the spellcheck_provider_ to move out.

[jam, 2011-03-31 00:10:28]

Getting RVOs by name and then casting feels like a poor man's RTTI.
 Combined with registering only some of the observers (as for some of them,
there are multiple instances per RV).  I really think we want to avoid an
approach like that.

On Wed, Mar 30, 2011 at 3:05 PM, <thestig@chromium.org> wrote:

> I tried to implement your ideas in patch set 4. I didn't move all the
> chrome
> RVOs out of RVs. There's still a couple left in there.
>
>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
> File chrome/renderer/render_thread.cc (right):
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
> chrome/renderer/render_thread.cc:10: #include <set>
> On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
>
>> nit: needed?
>>
>
> I was following the linter, but it's a moot point since I reverted all
> changes to render_thread.cc.
>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
> chrome/renderer/render_thread.cc:48: #include
> "chrome/renderer/render_view_wrapper.h"
> On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
>
>> I'm actually working on removing all the chrome dependencies from
>>
> RenderThread,
>
>> so that I can move it to content (a lot of it is not chrome specific,
>>
> and will
>
>> be needed there, so I don't want to duplicate it).  so my thinking was
>>
> we would
>
>> call through the embedder api (i.e. ContentRendererClient) to inform
>>
> the
>
>> embedder that a RenderView has been created.  then Chrome can add its
>>
> list of
>
>> observers
>>
>
> Ok. I did that in the next patchset.
>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
> File chrome/renderer/render_view_browsertest.cc (right):
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
> chrome/renderer/render_view_browsertest.cc:1: // Copyright (c) 2011 The
> Chromium Authors. All rights reserved.
> On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
>
>> nit: does this need to be moved now?  i'm planning on putting a
>>
> temporary
>
>> override for deps checking of test files until we figure out what the
>>
> testing
>
>> situation will look like.  because we will want to have tests like
>>
> this for the
>
>> content layer
>>
>
> No, reverted.
>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
> File chrome/renderer/render_view_wrapper.h (right):
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
> chrome/renderer/render_view_wrapper.h:22: class RenderViewWrapper :
> public RenderViewObserver {
> On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
>
>> it seems a little confusing to call this RenderViewWrapper, since it's
>>
> just
>
>> another observer, and doesn't actually contain a RenderView as a
>>
> member (i.e.
>
>> wrapping it).
>>
>
> It was a poorly chosen name, but I had to name it something. In any
> case, this is gone now.
>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
> chrome/renderer/render_view_wrapper.h:33: SearchBox* searchbox() const;
> On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
>
>> The interface of RenderViewObserver allows the implementor to delete
>>
> themselves
>
>> at any time. So it would seem unfortunate to have weak pointers to
>>
> RVO's, since
>
>> that will eventually lead to using deleted objects.  I realize that a
>>
> few of the
>
>> RVO implementations will need to be accessed given a RV.  I wonder if
>>
> we can
>
>> have some templatized helper that these objects can use to
>>
> automatically
>
>> register/unregister themself on creation and then have a getter based
>>
> on a
>
>> RenderView?
>>
>
> I don't have a templatized helper, but I added a |type_| string member
> to RVOs, and for the ones we care about, we can set it to a unique per
> class value. I also added RenderView::GetObserver(std::string type) to
> retrieve the RVO we care about for a given RV.
>
>
>  As for the tests, I think we just need to restructure them a bit so
>>
> that we
>
>> don't need to keep the PrintWebViewHelper and other objects as a weak
>>
> pointer.
>
> Ya, I got rid of the weak pointers because I added the method above.
>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/content/renderer/render_view.cc
> File content/renderer/render_view.cc (right):
>
>
>
http://codereview.chromium.org/6750018/diff/7024/content/renderer/render_view...
> content/renderer/render_view.cc:2213: RequestTextChecking(text,
> document_tag_, completion));
> On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
>
>> http://codereview.chromium.org/6764001/ removes the spellchecking
>>
> functions from
>
>> RV completely.  that way, the content layer and the embedder api don't
>>
> have to
>
>> know anything about it
>>
>
> Right, but we still have the spellcheck_provider_ to move out.
>
>
> http://codereview.chromium.org/6750018/
>

[Lei Zhang, 2011-03-31 00:22:25]

Definitely a poor man's RTTI. Assuming I can take care of that, how do you feel
about the rest of the CL?

Do we ever have a case where we have multiple RVOs of the same type for a given
RV, and those RVOs need to be accessed? For all the RVOs that we need to access,
currently we access them via weak pointers in a RV - thus only one RVO of a
given type for a RV.

On 2011/03/31 00:10:28, John Abd-El-Malek wrote:
> Getting RVOs by name and then casting feels like a poor man's RTTI.
>  Combined with registering only some of the observers (as for some of them,
> there are multiple instances per RV).  I really think we want to avoid an
> approach like that.
> 
> On Wed, Mar 30, 2011 at 3:05 PM, <mailto:thestig@chromium.org> wrote:
> 
> > I tried to implement your ideas in patch set 4. I didn't move all the
> > chrome
> > RVOs out of RVs. There's still a couple left in there.
> >
> >
> >
> >
> >
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
> > File chrome/renderer/render_thread.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
> > chrome/renderer/render_thread.cc:10: #include <set>
> > On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
> >
> >> nit: needed?
> >>
> >
> > I was following the linter, but it's a moot point since I reverted all
> > changes to render_thread.cc.
> >
> >
> >
> >
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
> > chrome/renderer/render_thread.cc:48: #include
> > "chrome/renderer/render_view_wrapper.h"
> > On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
> >
> >> I'm actually working on removing all the chrome dependencies from
> >>
> > RenderThread,
> >
> >> so that I can move it to content (a lot of it is not chrome specific,
> >>
> > and will
> >
> >> be needed there, so I don't want to duplicate it).  so my thinking was
> >>
> > we would
> >
> >> call through the embedder api (i.e. ContentRendererClient) to inform
> >>
> > the
> >
> >> embedder that a RenderView has been created.  then Chrome can add its
> >>
> > list of
> >
> >> observers
> >>
> >
> > Ok. I did that in the next patchset.
> >
> >
> >
> >
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
> > File chrome/renderer/render_view_browsertest.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
> > chrome/renderer/render_view_browsertest.cc:1: // Copyright (c) 2011 The
> > Chromium Authors. All rights reserved.
> > On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
> >
> >> nit: does this need to be moved now?  i'm planning on putting a
> >>
> > temporary
> >
> >> override for deps checking of test files until we figure out what the
> >>
> > testing
> >
> >> situation will look like.  because we will want to have tests like
> >>
> > this for the
> >
> >> content layer
> >>
> >
> > No, reverted.
> >
> >
> >
> >
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
> > File chrome/renderer/render_view_wrapper.h (right):
> >
> >
> >
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
> > chrome/renderer/render_view_wrapper.h:22: class RenderViewWrapper :
> > public RenderViewObserver {
> > On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
> >
> >> it seems a little confusing to call this RenderViewWrapper, since it's
> >>
> > just
> >
> >> another observer, and doesn't actually contain a RenderView as a
> >>
> > member (i.e.
> >
> >> wrapping it).
> >>
> >
> > It was a poorly chosen name, but I had to name it something. In any
> > case, this is gone now.
> >
> >
> >
> >
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
> > chrome/renderer/render_view_wrapper.h:33: SearchBox* searchbox() const;
> > On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
> >
> >> The interface of RenderViewObserver allows the implementor to delete
> >>
> > themselves
> >
> >> at any time. So it would seem unfortunate to have weak pointers to
> >>
> > RVO's, since
> >
> >> that will eventually lead to using deleted objects.  I realize that a
> >>
> > few of the
> >
> >> RVO implementations will need to be accessed given a RV.  I wonder if
> >>
> > we can
> >
> >> have some templatized helper that these objects can use to
> >>
> > automatically
> >
> >> register/unregister themself on creation and then have a getter based
> >>
> > on a
> >
> >> RenderView?
> >>
> >
> > I don't have a templatized helper, but I added a |type_| string member
> > to RVOs, and for the ones we care about, we can set it to a unique per
> > class value. I also added RenderView::GetObserver(std::string type) to
> > retrieve the RVO we care about for a given RV.
> >
> >
> >  As for the tests, I think we just need to restructure them a bit so
> >>
> > that we
> >
> >> don't need to keep the PrintWebViewHelper and other objects as a weak
> >>
> > pointer.
> >
> > Ya, I got rid of the weak pointers because I added the method above.
> >
> >
> >
> >
>
http://codereview.chromium.org/6750018/diff/7024/content/renderer/render_view.cc
> > File content/renderer/render_view.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/6750018/diff/7024/content/renderer/render_view...
> > content/renderer/render_view.cc:2213: RequestTextChecking(text,
> > document_tag_, completion));
> > On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
> >
> >> http://codereview.chromium.org/6764001/ removes the spellchecking
> >>
> > functions from
> >
> >> RV completely.  that way, the content layer and the embedder api don't
> >>
> > have to
> >
> >> know anything about it
> >>
> >
> > Right, but we still have the spellcheck_provider_ to move out.
> >
> >
> > http://codereview.chromium.org/6750018/
> >

[jam, 2011-03-31 05:49:22]

On Wed, Mar 30, 2011 at 5:22 PM, <thestig@chromium.org> wrote:

> Definitely a poor man's RTTI. Assuming I can take care of that, how do you
> feel
> about the rest of the CL?
>

That's hard to say until this piece is taken out, since most of the change
uses the string->pointer lookup.


>
> Do we ever have a case where we have multiple RVOs of the same type for a
> given
> RV, and those RVOs need to be accessed?


I don't think so, off the top of my head.


> For all the RVOs that we need to access,
> currently we access them via weak pointers in a RV - thus only one RVO of a
> given type for a RV.


It would be great if we can eliminate all these cases altogether, or if
there are only one or two remaining, have them implement their own map
lookup.  The print one looks like it's only used by tests, right?  If so, I
had solved other instances of this by just creating an additional RVO for
the test.


>
>
> On 2011/03/31 00:10:28, John Abd-El-Malek wrote:
>
>> Getting RVOs by name and then casting feels like a poor man's RTTI.
>>  Combined with registering only some of the observers (as for some of
>> them,
>> there are multiple instances per RV).  I really think we want to avoid an
>> approach like that.
>>
>
>  On Wed, Mar 30, 2011 at 3:05 PM, <mailto:thestig@chromium.org> wrote:
>>
>
>  > I tried to implement your ideas in patch set 4. I didn't move all the
>> > chrome
>> > RVOs out of RVs. There's still a couple left in there.
>> >
>> >
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
>
>> > File chrome/renderer/render_thread.cc (right):
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
>
>> > chrome/renderer/render_thread.cc:10: #include <set>
>> > On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
>> >
>> >> nit: needed?
>> >>
>> >
>> > I was following the linter, but it's a moot point since I reverted all
>> > changes to render_thread.cc.
>> >
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_threa...
>
>> > chrome/renderer/render_thread.cc:48: #include
>> > "chrome/renderer/render_view_wrapper.h"
>> > On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
>> >
>> >> I'm actually working on removing all the chrome dependencies from
>> >>
>> > RenderThread,
>> >
>> >> so that I can move it to content (a lot of it is not chrome specific,
>> >>
>> > and will
>> >
>> >> be needed there, so I don't want to duplicate it).  so my thinking was
>> >>
>> > we would
>> >
>> >> call through the embedder api (i.e. ContentRendererClient) to inform
>> >>
>> > the
>> >
>> >> embedder that a RenderView has been created.  then Chrome can add its
>> >>
>> > list of
>> >
>> >> observers
>> >>
>> >
>> > Ok. I did that in the next patchset.
>> >
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
>
>> > File chrome/renderer/render_view_browsertest.cc (right):
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
>
>> > chrome/renderer/render_view_browsertest.cc:1: // Copyright (c) 2011 The
>> > Chromium Authors. All rights reserved.
>> > On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
>> >
>> >> nit: does this need to be moved now?  i'm planning on putting a
>> >>
>> > temporary
>> >
>> >> override for deps checking of test files until we figure out what the
>> >>
>> > testing
>> >
>> >> situation will look like.  because we will want to have tests like
>> >>
>> > this for the
>> >
>> >> content layer
>> >>
>> >
>> > No, reverted.
>> >
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
>
>> > File chrome/renderer/render_view_wrapper.h (right):
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
>
>> > chrome/renderer/render_view_wrapper.h:22: class RenderViewWrapper :
>> > public RenderViewObserver {
>> > On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
>> >
>> >> it seems a little confusing to call this RenderViewWrapper, since it's
>> >>
>> > just
>> >
>> >> another observer, and doesn't actually contain a RenderView as a
>> >>
>> > member (i.e.
>> >
>> >> wrapping it).
>> >>
>> >
>> > It was a poorly chosen name, but I had to name it something. In any
>> > case, this is gone now.
>> >
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/chrome/renderer/render_view_...
>
>> > chrome/renderer/render_view_wrapper.h:33: SearchBox* searchbox() const;
>> > On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
>> >
>> >> The interface of RenderViewObserver allows the implementor to delete
>> >>
>> > themselves
>> >
>> >> at any time. So it would seem unfortunate to have weak pointers to
>> >>
>> > RVO's, since
>> >
>> >> that will eventually lead to using deleted objects.  I realize that a
>> >>
>> > few of the
>> >
>> >> RVO implementations will need to be accessed given a RV.  I wonder if
>> >>
>> > we can
>> >
>> >> have some templatized helper that these objects can use to
>> >>
>> > automatically
>> >
>> >> register/unregister themself on creation and then have a getter based
>> >>
>> > on a
>> >
>> >> RenderView?
>> >>
>> >
>> > I don't have a templatized helper, but I added a |type_| string member
>> > to RVOs, and for the ones we care about, we can set it to a unique per
>> > class value. I also added RenderView::GetObserver(std::string type) to
>> > retrieve the RVO we care about for a given RV.
>> >
>> >
>> >  As for the tests, I think we just need to restructure them a bit so
>> >>
>> > that we
>> >
>> >> don't need to keep the PrintWebViewHelper and other objects as a weak
>> >>
>> > pointer.
>> >
>> > Ya, I got rid of the weak pointers because I added the method above.
>> >
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/content/renderer/render_view.cc
>
>> > File content/renderer/render_view.cc (right):
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6750018/diff/7024/content/renderer/render_view...
>
>> > content/renderer/render_view.cc:2213: RequestTextChecking(text,
>> > document_tag_, completion));
>> > On 2011/03/29 18:31:59, John Abd-El-Malek wrote:
>> >
>> >> http://codereview.chromium.org/6764001/ removes the spellchecking
>> >>
>> > functions from
>> >
>> >> RV completely.  that way, the content layer and the embedder api don't
>> >>
>> > have to
>> >
>> >> know anything about it
>> >>
>> >
>> > Right, but we still have the spellcheck_provider_ to move out.
>> >
>> >
>> > http://codereview.chromium.org/6750018/
>> >
>>
>
>
>
> http://codereview.chromium.org/6750018/
>

[Lei Zhang, 2011-04-05 07:41:16]

On 2011/03/31 05:49:22, John Abd-El-Malek wrote:
> It would be great if we can eliminate all these cases altogether, or if
> there are only one or two remaining, have them implement their own map
> lookup.  The print one looks like it's only used by tests, right?  If so, I
> had solved other instances of this by just creating an additional RVO for
> the test.

PTAL @ patch set 5.

I added a "RenderViewObserverTracker" that makes adding the code to allow a map
lookup to a given class a few lines of code.

For the tests that use PrintViewWebHelper, creating another RVO didn't help.

[jam, 2011-04-05 18:10:07]

http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
File content/renderer/render_view_observer.h (right):

http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
content/renderer/render_view_observer.h:48: virtual void
printPage(WebKit::WebFrame* frame) {}
this needs to be PrintPage (i.e. chromium style instead of webkit style)

http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
File content/renderer/render_view_observer_tracker.h (right):

http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
content/renderer/render_view_observer_tracker.h:14: //  
RENDER_VIEW_OBSERVER_TRACKER_MAP(MyRVO);
I like this approach a lot more, very nice :)

Can we get rid of the macro though?  i.e. have it as a static variable in
RenderViewObserverTracker's constructor?

[Lei Zhang, 2011-04-05 21:03:16]

http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
File content/renderer/render_view_observer.h (right):

http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
content/renderer/render_view_observer.h:48: virtual void
printPage(WebKit::WebFrame* frame) {}
On 2011/04/05 18:10:07, John Abd-El-Malek wrote:
> this needs to be PrintPage (i.e. chromium style instead of webkit style)

Done.

http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
File content/renderer/render_view_observer_tracker.h (right):

http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
content/renderer/render_view_observer_tracker.h:14: //  
RENDER_VIEW_OBSERVER_TRACKER_MAP(MyRVO);
On 2011/04/05 18:10:07, John Abd-El-Malek wrote:
> I like this approach a lot more, very nice :)
> 
> Can we get rid of the macro though?  i.e. have it as a static variable in
> RenderViewObserverTracker's constructor?

I couldn't put it in the constructor, but I put it outside of the class and that
seems to work.

[jam, 2011-04-05 21:08:35]

http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
File content/renderer/render_view_observer_tracker.h (right):

http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
content/renderer/render_view_observer_tracker.h:14: //  
RENDER_VIEW_OBSERVER_TRACKER_MAP(MyRVO);
On 2011/04/05 21:03:16, Lei Zhang wrote:
> On 2011/04/05 18:10:07, John Abd-El-Malek wrote:
> > I like this approach a lot more, very nice :)
> > 
> > Can we get rid of the macro though?  i.e. have it as a static variable in
> > RenderViewObserverTracker's constructor?
> 
> I couldn't put it in the constructor, but I put it outside of the class and
that
> seems to work.

wouldn't putting it in the header, outside any class, mean that a copy gets
created each time that file is included??

[Lei Zhang, 2011-04-05 21:30:06]

On 2011/04/05 21:08:35, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
> File content/renderer/render_view_observer_tracker.h (right):
> 
>
http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
> content/renderer/render_view_observer_tracker.h:14: //  
> RENDER_VIEW_OBSERVER_TRACKER_MAP(MyRVO);
> On 2011/04/05 21:03:16, Lei Zhang wrote:
> > On 2011/04/05 18:10:07, John Abd-El-Malek wrote:
> > > I like this approach a lot more, very nice :)
> > > 
> > > Can we get rid of the macro though?  i.e. have it as a static variable in
> > > RenderViewObserverTracker's constructor?
> > 
> > I couldn't put it in the constructor, but I put it outside of the class and
> that
> > seems to work.
> 
> wouldn't putting it in the header, outside any class, mean that a copy gets
> created each time that file is included??

I am not the biggest C++ expert, but I don't think that happens with templatized
code. At least on Linux, chrome_content_renderer_client.o doesn't have a symbol
for RenderViewObserverTracker<SearchBox>::render_view_map_, even though it
includes searchbox.h.

[jam, 2011-04-05 21:47:00]

On 2011/04/05 21:30:06, Lei Zhang wrote:
> On 2011/04/05 21:08:35, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
> > File content/renderer/render_view_observer_tracker.h (right):
> > 
> >
>
http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
> > content/renderer/render_view_observer_tracker.h:14: //  
> > RENDER_VIEW_OBSERVER_TRACKER_MAP(MyRVO);
> > On 2011/04/05 21:03:16, Lei Zhang wrote:
> > > On 2011/04/05 18:10:07, John Abd-El-Malek wrote:
> > > > I like this approach a lot more, very nice :)
> > > > 
> > > > Can we get rid of the macro though?  i.e. have it as a static variable
in
> > > > RenderViewObserverTracker's constructor?
> > > 
> > > I couldn't put it in the constructor, but I put it outside of the class
and
> > that
> > > seems to work.
> > 
> > wouldn't putting it in the header, outside any class, mean that a copy gets
> > created each time that file is included??
> 
> I am not the biggest C++ expert, but I don't think that happens with
templatized
> code. At least on Linux, chrome_content_renderer_client.o doesn't have a
symbol
> for RenderViewObserverTracker<SearchBox>::render_view_map_, even though it
> includes searchbox.h.

I'm not an expert here myself.  jyasskin from the compiler team happened to be
in my office today and I asked him.  He said this only works in gcc, but not on
windows.  maybe try sending this through a win try bot run to be sure?

[Lei Zhang, 2011-04-06 17:57:58]

On 2011/04/05 21:47:00, John Abd-El-Malek wrote:
> On 2011/04/05 21:30:06, Lei Zhang wrote:
> > On 2011/04/05 21:08:35, John Abd-El-Malek wrote:
> > >
> >
>
http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
> > > File content/renderer/render_view_observer_tracker.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
> > > content/renderer/render_view_observer_tracker.h:14: //  
> > > RENDER_VIEW_OBSERVER_TRACKER_MAP(MyRVO);
> > > On 2011/04/05 21:03:16, Lei Zhang wrote:
> > > > On 2011/04/05 18:10:07, John Abd-El-Malek wrote:
> > > > > I like this approach a lot more, very nice :)
> > > > > 
> > > > > Can we get rid of the macro though?  i.e. have it as a static variable
> in
> > > > > RenderViewObserverTracker's constructor?
> > > > 
> > > > I couldn't put it in the constructor, but I put it outside of the class
> and
> > > that
> > > > seems to work.
> > > 
> > > wouldn't putting it in the header, outside any class, mean that a copy
gets
> > > created each time that file is included??
> > 
> > I am not the biggest C++ expert, but I don't think that happens with
> templatized
> > code. At least on Linux, chrome_content_renderer_client.o doesn't have a
> symbol
> > for RenderViewObserverTracker<SearchBox>::render_view_map_, even though it
> > includes searchbox.h.
> 
> I'm not an expert here myself.  jyasskin from the compiler team happened to be
> in my office today and I asked him.  He said this only works in gcc, but not
on
> windows.  maybe try sending this through a win try bot run to be sure?

(resend because codesite was being wonky yesterday)

Win try job passed for patch set 6.

I built patchset 5 and 7 on Windows. The output chrome.dll size is the same.

[jam, 2011-04-07 01:25:39]

I'm not sure that checking the dll size is enough, i.e. what if there's padding?
 Matt: any idea on this?

[Matt Perry, 2011-04-07 01:32:21]

http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
File content/renderer/render_view_observer_tracker.h (right):

http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
content/renderer/render_view_observer_tracker.h:14: //  
RENDER_VIEW_OBSERVER_TRACKER_MAP(MyRVO);
On 2011/04/05 21:08:35, John Abd-El-Malek wrote:
> On 2011/04/05 21:03:16, Lei Zhang wrote:
> > On 2011/04/05 18:10:07, John Abd-El-Malek wrote:
> > > I like this approach a lot more, very nice :)
> > > 
> > > Can we get rid of the macro though?  i.e. have it as a static variable in
> > > RenderViewObserverTracker's constructor?
> > 
> > I couldn't put it in the constructor, but I put it outside of the class and
> that
> > seems to work.
> 
> wouldn't putting it in the header, outside any class, mean that a copy gets
> created each time that file is included??

Since it's declared as a static member in the class, there should still only be
1 instance. However, I would expect multiple definitions to be a link error.
Maybe only 1 file ever includes the headers where this macro is used? Or maybe
gcc is clever about merging the instances.

[jam, 2011-04-07 01:35:56]

On Wed, Apr 6, 2011 at 6:32 PM, <mpcomplete@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
> File content/renderer/render_view_observer_tracker.h (right):
>
>
>
http://codereview.chromium.org/6750018/diff/27004/content/renderer/render_vie...
> content/renderer/render_view_observer_tracker.h:14: //
> RENDER_VIEW_OBSERVER_TRACKER_MAP(MyRVO);
> On 2011/04/05 21:08:35, John Abd-El-Malek wrote:
>
>> On 2011/04/05 21:03:16, Lei Zhang wrote:
>> > On 2011/04/05 18:10:07, John Abd-El-Malek wrote:
>> > > I like this approach a lot more, very nice :)
>> > >
>> > > Can we get rid of the macro though?  i.e. have it as a static
>>
> variable in
>
>> > > RenderViewObserverTracker's constructor?
>> >
>> > I couldn't put it in the constructor, but I put it outside of the
>>
> class and
>
>> that
>> > seems to work.
>>
>
>  wouldn't putting it in the header, outside any class, mean that a copy
>>
> gets
>
>> created each time that file is included??
>>
>
> Since it's declared as a static member in the class, there should still
> only be 1 instance. However, I would expect multiple definitions to be a
> link error. Maybe only 1 file ever includes the headers where this macro
> is used? Or maybe gcc is clever about merging the instances.


jyasskin (compiler team) said that gcc is smart about this, but VS is not,
hence my apprehension.  i guess we can try it and we'll see if there are
problems pretty quickly.

lgtm, sorry for the delay

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