Add site_instance_id to ProcessMap::Item.

Add site_instance_id to ProcessMap::Item.

This is needed because there can be two site instances for the
same extension/process pair. Boo.

The linear scan for Contains() is a bummer. I thought about
more exotic data structures, but it didn't seem worth it.

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

[Charlie Reis, 2011-12-02 00:45:11]

LGTM with minor comments inline.  Thanks!

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_info_map.h (right):

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_info_map.h:59: int site_instance_id);
Conceptually I kind of view it as a map of (extension_id, site_instance_id) to
process_id and would order it that way, but I'm also fine with it as written.

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/pr...
File chrome/browser/extensions/process_map.h (right):

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/pr...
chrome/browser/extensions/process_map.h:26: //
Maybe add a note:
 - A single hosted app can have more than one SiteInstance in the same process,
if we're over the process limit and force them to share a process.

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/pr...
chrome/browser/extensions/process_map.h:68: int site_instance_id);
Ditto.

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/pr...
File chrome/browser/extensions/process_map_unittest.cc (right):

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/pr...
chrome/browser/extensions/process_map_unittest.cc:46: EXPECT_EQ(3u, map.size());
Can you add Insert("a", 1, 2) somewhere above, and then verify that
Contains("a", 1) is still true after Remove("a", 1, 1)?  That's the case I was
concerned about originally (two SiteInstances for the same Extension in the same
process).

[Aaron Boodman, 2011-12-02 05:32:45]

will land soon

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_info_map.h (right):

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_info_map.h:59: int site_instance_id);
On 2011/12/02 00:45:11, creis wrote:
> Conceptually I kind of view it as a map of (extension_id, site_instance_id) to
> process_id and would order it that way, but I'm also fine with it as written.

Meh.

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/pr...
File chrome/browser/extensions/process_map.h (right):

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/pr...
chrome/browser/extensions/process_map.h:26: //
On 2011/12/02 00:45:11, creis wrote:
> Maybe add a note:
>  - A single hosted app can have more than one SiteInstance in the same
process,
> if we're over the process limit and force them to share a process.

Done.

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/pr...
File chrome/browser/extensions/process_map_unittest.cc (right):

http://codereview.chromium.org/8769022/diff/2001/chrome/browser/extensions/pr...
chrome/browser/extensions/process_map_unittest.cc:46: EXPECT_EQ(3u, map.size());
On 2011/12/02 00:45:11, creis wrote:
> Can you add Insert("a", 1, 2) somewhere above, and then verify that
> Contains("a", 1) is still true after Remove("a", 1, 1)?  That's the case I was
> concerned about originally (two SiteInstances for the same Extension in the
same
> process).

Good point, done.

[Robert Nagy, 2011-12-06 20:31:14]

Hello Aaron,

It seems that this change has broke *BSD builds and I was unable to figure out
the root cause because I can't see any platform related change in this patchset.
I was wondering if this is a gcc bug or an unsupported feature in older gcc?
chrome/browser/extensions/process_map.cc: In member function 'bool
extensions::ProcessMap::Insert(const std::string&, int, int)':
chrome/browser/extensions/process_map.cc:63: error: no matching function for
call to 'extensions::ProcessMap::Item::Item(extensions::ProcessMap::Item)'
chrome/browser/extensions/process_map.cc: In member function 'bool
extensions::ProcessMap::Remove(const std::string&, int, int)':
chrome/browser/extensions/process_map.cc:68: error: no matching function for
call to 'extensions::ProcessMap::Item::Item(extensions::ProcessMap::Item)'

[Aaron Boodman, 2011-12-06 21:42:01]

On 2011/12/06 20:31:14, Robert Nagy wrote:
> Hello Aaron,
> 
> It seems that this change has broke *BSD builds and I was unable to figure out
> the root cause because I can't see any platform related change in this
patchset.
> I was wondering if this is a gcc bug or an unsupported feature in older gcc?
> chrome/browser/extensions/process_map.cc: In member function 'bool
> extensions::ProcessMap::Insert(const std::string&, int, int)':
> chrome/browser/extensions/process_map.cc:63: error: no matching function for
> call to 'extensions::ProcessMap::Item::Item(extensions::ProcessMap::Item)'
> chrome/browser/extensions/process_map.cc: In member function 'bool
> extensions::ProcessMap::Remove(const std::string&, int, int)':
> chrome/browser/extensions/process_map.cc:68: error: no matching function for
> call to 'extensions::ProcessMap::Item::Item(extensions::ProcessMap::Item)'

Hi Robert,

It is probably because of the explicit keyword on the Item() constructor. Can
you double-check that removing that fixes the problem? if so I will commit a
change.

[Robert Nagy, 2011-12-06 22:39:08]

On 2011/12/06 21:42:01, Aaron Boodman wrote:
> On 2011/12/06 20:31:14, Robert Nagy wrote:
> > Hello Aaron,
> > 
> > It seems that this change has broke *BSD builds and I was unable to figure
out
> > the root cause because I can't see any platform related change in this
> patchset.
> > I was wondering if this is a gcc bug or an unsupported feature in older gcc?
> > chrome/browser/extensions/process_map.cc: In member function 'bool
> > extensions::ProcessMap::Insert(const std::string&, int, int)':
> > chrome/browser/extensions/process_map.cc:63: error: no matching function for
> > call to 'extensions::ProcessMap::Item::Item(extensions::ProcessMap::Item)'
> > chrome/browser/extensions/process_map.cc: In member function 'bool
> > extensions::ProcessMap::Remove(const std::string&, int, int)':
> > chrome/browser/extensions/process_map.cc:68: error: no matching function for
> > call to 'extensions::ProcessMap::Item::Item(extensions::ProcessMap::Item)'
> 
> Hi Robert,
> 
> It is probably because of the explicit keyword on the Item() constructor. Can
> you double-check that removing that fixes the problem? if so I will commit a
> change.

Hey,

Yes, that made it happy.

[Aaron Boodman, 2011-12-07 20:24:57]

On 2011/12/06 22:39:08, Robert Nagy wrote:
> On 2011/12/06 21:42:01, Aaron Boodman wrote:
> > On 2011/12/06 20:31:14, Robert Nagy wrote:
> > > Hello Aaron,
> > > 
> > > It seems that this change has broke *BSD builds and I was unable to figure
> out
> > > the root cause because I can't see any platform related change in this
> > patchset.
> > > I was wondering if this is a gcc bug or an unsupported feature in older
gcc?
> > > chrome/browser/extensions/process_map.cc: In member function 'bool
> > > extensions::ProcessMap::Insert(const std::string&, int, int)':
> > > chrome/browser/extensions/process_map.cc:63: error: no matching function
for
> > > call to 'extensions::ProcessMap::Item::Item(extensions::ProcessMap::Item)'
> > > chrome/browser/extensions/process_map.cc: In member function 'bool
> > > extensions::ProcessMap::Remove(const std::string&, int, int)':
> > > chrome/browser/extensions/process_map.cc:68: error: no matching function
for
> > > call to 'extensions::ProcessMap::Item::Item(extensions::ProcessMap::Item)'
> > 
> > Hi Robert,
> > 
> > It is probably because of the explicit keyword on the Item() constructor.
Can
> > you double-check that removing that fixes the problem? if so I will commit a
> > change.
> 
> Hey,
> 
> Yes, that made it happy.

ok, i've committed this. hopefully it sticks.