Refactor away most of ExtensionRendererInfo

Refactor away all the duplicate extension data structures in
renderer processes by sending the full extension object instead.

ExtensionRendererInfo remains, but it is now just a 
convenience wrapper around a map of Extension objects.

This allows us to reuse all the helper methods on Extension,
ExtensionIconSet, ExtensionExtent, etc without duplicating 
them in the renderer.

Also changed broadcasts to renderers to send only changed
information, not entire set of extension data again.

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

[Elliot Glaysher, 2011-01-24 19:00:28]

I [heart] refactorings. Some very nitty nits.

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
File chrome/common/render_messages_params.h (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
chrome/common/render_messages_params.h:843: 
dtor

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_renderer_info.h (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_renderer_info.h:57:
DISALLOW_COPY_AND_ASSIGN(ExtensionRendererInfo);
DISALLOW should be the final line of the class.

[Matt Perry, 2011-01-24 20:08:34]

Hooray! Mostly LGTM.

Now, can we figure out a way to unify our three different Extension List
classes? (ExtensionService, ExtensionInfoMap, ExtensionRendererInfo)

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
File chrome/common/render_messages_params.cc (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
chrome/common/render_messages_params.cc:1512:
l->append("<ViewMsg_ExtensionLoaded_Params>");
nit: might also put the extension ID here

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
File chrome/common/render_messages_params.h (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
chrome/common/render_messages_params.h:841: const
ViewMsg_ExtensionLoaded_Params& other);
when is this copy constructor used?

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_app_bindings.cc (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_app_bindings.cc:61:
RenderThread::current()->extensions()->GetByURL(url) != NULL;
note that RenderThread::current() will be NULL in unit tests.

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/e...
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/e...
chrome/renderer/extensions/event_bindings.cc:268: ExtensionRendererInfo*
extensions = RenderThread::current()->extensions();
use the render_thread global instead, since RenderThread::current() is NULL in
unit tests. You'll have to make the extensions getter a virtual on
RenderThreadBase.

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_renderer_info.h (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_renderer_info.h:49: scoped_refptr<const
Extension> GetByID(const std::string& id);
Can we just return a raw pointer here? The caller doesn't need to take
ownership, does it?

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/localized_er...
File chrome/renderer/localized_error.h (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/localized_er...
chrome/renderer/localized_error.h:45: const scoped_refptr<const Extension>& app,
raw pointer here too?

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

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:1891: scoped_refptr<const Extension> extension;
just use raw ptrs

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/user_script_...
File chrome/renderer/user_script_slave.cc (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/user_script_...
chrome/renderer/user_script_slave.cc:206: scoped_refptr<const Extension>
extension =
raw ptr

[Aaron Boodman, 2011-01-25 00:27:33]

Addressed all comments. One question for Matt here: 

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/c...

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
File chrome/common/render_messages_params.cc (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
chrome/common/render_messages_params.cc:1512:
l->append("<ViewMsg_ExtensionLoaded_Params>");
On 2011/01/24 20:08:34, Matt Perry wrote:
> nit: might also put the extension ID here

We do not have the ID handy, all we have is the key. I've modified the IPC
message to store the ID in addition, though it is only needed for this logging.

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
File chrome/common/render_messages_params.h (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
chrome/common/render_messages_params.h:841: const
ViewMsg_ExtensionLoaded_Params& other);
On 2011/01/24 20:08:34, Matt Perry wrote:
> when is this copy constructor used?

It is used when ViewMsg_ExtensionLoaded is instantiated. It only happens on
older compilers, and I didn't dig into the IPC macro pile to figure exactly what
causes it. I added a comment explaining this.

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
chrome/common/render_messages_params.h:843: 
On 2011/01/24 19:00:28, Elliot Glaysher wrote:
> dtor

Done.

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_app_bindings.cc (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_app_bindings.cc:61:
RenderThread::current()->extensions()->GetByURL(url) != NULL;
On 2011/01/24 20:08:34, Matt Perry wrote:
> note that RenderThread::current() will be NULL in unit tests.

Really? RenderThread::current() is backed by lazy TLS. It seems like it will
just create an instance.

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/e...
File chrome/renderer/extensions/event_bindings.cc (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/e...
chrome/renderer/extensions/event_bindings.cc:268: ExtensionRendererInfo*
extensions = RenderThread::current()->extensions();
On 2011/01/24 20:08:34, Matt Perry wrote:
> use the render_thread global instead, since RenderThread::current() is NULL in
> unit tests. You'll have to make the extensions getter a virtual on
> RenderThreadBase.

Done.

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_renderer_info.h (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_renderer_info.h:49: scoped_refptr<const
Extension> GetByID(const std::string& id);
On 2011/01/24 20:08:34, Matt Perry wrote:
> Can we just return a raw pointer here? The caller doesn't need to take
> ownership, does it?

Done.

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_renderer_info.h:57:
DISALLOW_COPY_AND_ASSIGN(ExtensionRendererInfo);
On 2011/01/24 19:00:28, Elliot Glaysher wrote:
> DISALLOW should be the final line of the class.

Done.

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/localized_er...
File chrome/renderer/localized_error.h (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/localized_er...
chrome/renderer/localized_error.h:45: const scoped_refptr<const Extension>& app,
On 2011/01/24 20:08:34, Matt Perry wrote:
> raw pointer here too?

Done.

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

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:1891: scoped_refptr<const Extension> extension;
On 2011/01/24 20:08:34, Matt Perry wrote:
> just use raw ptrs

Done.

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/user_script_...
File chrome/renderer/user_script_slave.cc (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/user_script_...
chrome/renderer/user_script_slave.cc:206: scoped_refptr<const Extension>
extension =
On 2011/01/24 20:08:34, Matt Perry wrote:
> raw ptr

Done.

[Matt Perry, 2011-01-25 00:45:54]

LGTM

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
File chrome/common/render_messages_params.cc (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
chrome/common/render_messages_params.cc:1512:
l->append("<ViewMsg_ExtensionLoaded_Params>");
On 2011/01/25 00:27:33, Aaron Boodman wrote:
> On 2011/01/24 20:08:34, Matt Perry wrote:
> > nit: might also put the extension ID here
> 
> We do not have the ID handy, all we have is the key. I've modified the IPC
> message to store the ID in addition, though it is only needed for this
logging.

In that case, I don't think it's really needed, just a nice to have. But if
you've already added it, may as well keep it.

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
File chrome/common/render_messages_params.h (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/common/render_message...
chrome/common/render_messages_params.h:841: const
ViewMsg_ExtensionLoaded_Params& other);
On 2011/01/25 00:27:33, Aaron Boodman wrote:
> On 2011/01/24 20:08:34, Matt Perry wrote:
> > when is this copy constructor used?
> 
> It is used when ViewMsg_ExtensionLoaded is instantiated. It only happens on
> older compilers, and I didn't dig into the IPC macro pile to figure exactly
what
> causes it. I added a comment explaining this.

I see. In that case, maybe making a deep copy of the manifest is not needed. You
could do
  manifest(other.release())
instead, though manifest might need to be marked mutable.

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_app_bindings.cc (right):

http://codereview.chromium.org/6242010/diff/9001/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_app_bindings.cc:61:
RenderThread::current()->extensions()->GetByURL(url) != NULL;
On 2011/01/25 00:27:33, Aaron Boodman wrote:
> On 2011/01/24 20:08:34, Matt Perry wrote:
> > note that RenderThread::current() will be NULL in unit tests.
> 
> Really? RenderThread::current() is backed by lazy TLS. It seems like it will
> just create an instance.

Oh, well maybe things have changed since I last looked. If unit tests don't
crash, then I guess this is fine.