IDMap derives from NonThreadSafe.

IDMap derives from NonThreadSafe.

During a code review I noticed that RenderViewHost::FromID was being called from the IO thread, but there were no automated assertions that fired.

BUG=None
TEST=DCHECK fires when I add bad code. Otherwise Chrome works without crashing in Debug build.

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

[cbentzel, 2011-03-02 18:34:54]

Is it OK for something in base/ to include something from base/threading?

[cbentzel, 2011-03-02 18:38:43]

On 2011/03/02 18:34:54, cbentzel wrote:
> Is it OK for something in base/ to include something from base/threading?

Also, it seems like NonThreadSafe is a good candidate for private inheritance,
but that's against coding style.

[brettw, 2011-03-02 18:42:47]

LGTM

http://codereview.chromium.org/6591108/diff/1/base/id_map.h
File base/id_map.h (right):

http://codereview.chromium.org/6591108/diff/1/base/id_map.h#newcode44
base/id_map.h:44: // TODO(cbentzel): Fix users of IDMap and remove this.
Unless you're actually going to change callers (which seems nontrivial and maybe
not super useful) I would probably not bother with this TODO.

[cbentzel, 2011-03-02 19:02:45]

http://codereview.chromium.org/6591108/diff/1/base/id_map.h
File base/id_map.h (right):

http://codereview.chromium.org/6591108/diff/1/base/id_map.h#newcode44
base/id_map.h:44: // TODO(cbentzel): Fix users of IDMap and remove this.
On 2011/03/02 18:42:47, brettw wrote:
> Unless you're actually going to change callers (which seems nontrivial and
maybe
> not super useful) I would probably not bother with this TODO.

Agreed. Removed.

[cbentzel, 2011-03-02 20:49:22]

Good number of tests are hitting the DCHECK - will validate if it's an actual
problem or an artifact of the tests.

[cbentzel, 2011-03-04 13:38:34]

On 2011/03/02 20:49:22, cbentzel wrote:
> Good number of tests are hitting the DCHECK - will validate if it's an actual
> problem or an artifact of the tests.

The newest patch resolves the test failures. Essentially, there were a large
number of static IDMap's which get destroyed in the main (UI) thread, but were
always accessed in the IO thread. I reset the thread in the destructor. This
does make the DCHECK's a bit more toothless, but they do catch the case I
manually caught in a code review.

[cbentzel, 2011-03-04 13:39:22]

Also - any idea why IDMap's are exempt from the POD-only static style guide
rule? Is this historical accident?

[brettw, 2011-03-04 15:55:05]

On Fri, Mar 4, 2011 at 5:39 AM,  <cbentzel@chromium.org> wrote:
> Also - any idea why IDMap's are exempt from the POD-only static style guide
> rule? Is this historical accident?

This is bad. Do you have a list of which ones were causing problems?
We should have bugs on them.

Brett

[brettw at google, 2011-03-04 15:56:49]

LGTM