HTTPHeaderMap: Favor composition over inheritance.

HTTPHeaderMap: Favor composition over inheritance.

dllexported classes export all methods of the base class on Windows.
By making the HashMap a member instead of a base, fewer methods get
instantiated.

No intended behavior change.

BUG=409105
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181266

[Nico, 2014-09-02 21:22:06]

thakis@chromium.org changed reviewers:
+ pdr@chromium.org

[pdr., 2014-09-02 21:35:35]

On 2014/09/02 21:22:06, Nico (hiding) wrote:

I may not be the best reviewer for this change, but a quick search doesn't turn
up many better alternatives :/

I am not familiar with dllexported classes so I need a little hand holding, can
you explain the end effect for a windows user after this change?

[Nico, 2014-09-02 22:54:22]

Sure, here's my best try at explaining what's going on. (+hans so he can correct
the bits I'm going to screw up.)

But first, I'd like to say that this is a nice change purely from an interface
point of view: Now it's clear that only const_iterator methods are ever needed,
it's fairly easy to change the internal storage, we don't expose lots of
previously-inherited methods that aren't needed, etc.

Ok, on to dllexport! On posix, the components build works like this: FOO_EXPORT
expands to visibility(default) while building FOO, and to nothing if the header
is included in other targets. (And everything's built with -fvisibility=hidden,
so all methods that aren't explicitly visibility(default) are hidden.) So if a
template isn't instantiated in FOO, its code doesn't end up in library FOO, and
every downstream library that uses the template just does its own instantiation
and has a hidden copy of the template.

On Windows, things are slightly different: FOO_EXPORT becomes dllexport while
building FOO and dllimport else. So dllexport on classes instantiates template
base classes and exports all its members, so that they can be dllimported from
elsewhere. But HashTable's trace() method (now instantiated) ends up calling
trace() on a DefaultAllocator (see bug), and that's not valid code. So clang/win
errors out. As a fix, we make the HashTable a member (so not all its methods get
instantiated through dllexport), and now the trace() method is never used and
all compilers are happy.

[Nico, 2014-09-02 23:00:26]

(If that's convincing enough, please cq+. If it isn't, I'm happy to
generally write more or answer specific questions, or…)


On Tue, Sep 2, 2014 at 3:54 PM, <thakis@chromium.org> wrote:

> Sure, here's my best try at explaining what's going on. (+hans so he can
> correct
> the bits I'm going to screw up.)
>
> But first, I'd like to say that this is a nice change purely from an
> interface
> point of view: Now it's clear that only const_iterator methods are ever
> needed,
> it's fairly easy to change the internal storage, we don't expose lots of
> previously-inherited methods that aren't needed, etc.
>
> Ok, on to dllexport! On posix, the components build works like this:
> FOO_EXPORT
> expands to visibility(default) while building FOO, and to nothing if the
> header
> is included in other targets. (And everything's built with
> -fvisibility=hidden,
> so all methods that aren't explicitly visibility(default) are hidden.) So
> if a
> template isn't instantiated in FOO, its code doesn't end up in library
> FOO, and
> every downstream library that uses the template just does its own
> instantiation
> and has a hidden copy of the template.
>
> On Windows, things are slightly different: FOO_EXPORT becomes dllexport
> while
> building FOO and dllimport else. So dllexport on classes instantiates
> template
> base classes and exports all its members, so that they can be dllimported
> from
> elsewhere. But HashTable's trace() method (now instantiated) ends up
> calling
> trace() on a DefaultAllocator (see bug), and that's not valid code. So
> clang/win
> errors out. As a fix, we make the HashTable a member (so not all its
> methods get
> instantiated through dllexport), and now the trace() method is never used
> and
> all compilers are happy.
>
> https://codereview.chromium.org/536523002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[pdr., 2014-09-02 23:06:35]

On 2014/09/02 22:54:22, Nico (hiding) wrote:
> Sure, here's my best try at explaining what's going on. (+hans so he can
correct
> the bits I'm going to screw up.)
> 
> But first, I'd like to say that this is a nice change purely from an interface
> point of view: Now it's clear that only const_iterator methods are ever
needed,
> it's fairly easy to change the internal storage, we don't expose lots of
> previously-inherited methods that aren't needed, etc.
> 
> Ok, on to dllexport! On posix, the components build works like this:
FOO_EXPORT
> expands to visibility(default) while building FOO, and to nothing if the
header
> is included in other targets. (And everything's built with
-fvisibility=hidden,
> so all methods that aren't explicitly visibility(default) are hidden.) So if a
> template isn't instantiated in FOO, its code doesn't end up in library FOO,
and
> every downstream library that uses the template just does its own
instantiation
> and has a hidden copy of the template.
> 
> On Windows, things are slightly different: FOO_EXPORT becomes dllexport while
> building FOO and dllimport else. So dllexport on classes instantiates template
> base classes and exports all its members, so that they can be dllimported from
> elsewhere. But HashTable's trace() method (now instantiated) ends up calling
> trace() on a DefaultAllocator (see bug), and that's not valid code. So
clang/win
> errors out. As a fix, we make the HashTable a member (so not all its methods
get
> instantiated through dllexport), and now the trace() method is never used and
> all compilers are happy.

Thank you for this explanation. It's very clear what's going on now.

Is there a way to prevent this from happening again? A non-virtual dtor on
HashMap (and it's wtf-type friends) would make it a little more difficult to
subclass them, but that affects all subclasses and not just exported ones.

[Nico, 2014-09-02 23:08:14]

On 2014/09/02 23:06:35, pdr wrote:
> On 2014/09/02 22:54:22, Nico (hiding) wrote:
> > Sure, here's my best try at explaining what's going on. (+hans so he can
> correct
> > the bits I'm going to screw up.)
> > 
> > But first, I'd like to say that this is a nice change purely from an
interface
> > point of view: Now it's clear that only const_iterator methods are ever
> needed,
> > it's fairly easy to change the internal storage, we don't expose lots of
> > previously-inherited methods that aren't needed, etc.
> > 
> > Ok, on to dllexport! On posix, the components build works like this:
> FOO_EXPORT
> > expands to visibility(default) while building FOO, and to nothing if the
> header
> > is included in other targets. (And everything's built with
> -fvisibility=hidden,
> > so all methods that aren't explicitly visibility(default) are hidden.) So if
a
> > template isn't instantiated in FOO, its code doesn't end up in library FOO,
> and
> > every downstream library that uses the template just does its own
> instantiation
> > and has a hidden copy of the template.
> > 
> > On Windows, things are slightly different: FOO_EXPORT becomes dllexport
while
> > building FOO and dllimport else. So dllexport on classes instantiates
template
> > base classes and exports all its members, so that they can be dllimported
from
> > elsewhere. But HashTable's trace() method (now instantiated) ends up calling
> > trace() on a DefaultAllocator (see bug), and that's not valid code. So
> clang/win
> > errors out. As a fix, we make the HashTable a member (so not all its methods
> get
> > instantiated through dllexport), and now the trace() method is never used
and
> > all compilers are happy.
> 
> Thank you for this explanation. It's very clear what's going on now.
> 
> Is there a way to prevent this from happening again? A non-virtual dtor on
> HashMap (and it's wtf-type friends) would make it a little more difficult to
> subclass them, but that affects all subclasses and not just exported ones.

I think HashMap doesn't have any subclasses at the moment, so making it FINAL
would prevent this I think. (But that should likely be in a different cl)

[pdr., 2014-09-02 23:08:55]

On 2014/09/02 23:08:14, Nico (hiding) wrote:
> On 2014/09/02 23:06:35, pdr wrote:
> > On 2014/09/02 22:54:22, Nico (hiding) wrote:
> > > Sure, here's my best try at explaining what's going on. (+hans so he can
> > correct
> > > the bits I'm going to screw up.)
> > > 
> > > But first, I'd like to say that this is a nice change purely from an
> interface
> > > point of view: Now it's clear that only const_iterator methods are ever
> > needed,
> > > it's fairly easy to change the internal storage, we don't expose lots of
> > > previously-inherited methods that aren't needed, etc.
> > > 
> > > Ok, on to dllexport! On posix, the components build works like this:
> > FOO_EXPORT
> > > expands to visibility(default) while building FOO, and to nothing if the
> > header
> > > is included in other targets. (And everything's built with
> > -fvisibility=hidden,
> > > so all methods that aren't explicitly visibility(default) are hidden.) So
if
> a
> > > template isn't instantiated in FOO, its code doesn't end up in library
FOO,
> > and
> > > every downstream library that uses the template just does its own
> > instantiation
> > > and has a hidden copy of the template.
> > > 
> > > On Windows, things are slightly different: FOO_EXPORT becomes dllexport
> while
> > > building FOO and dllimport else. So dllexport on classes instantiates
> template
> > > base classes and exports all its members, so that they can be dllimported
> from
> > > elsewhere. But HashTable's trace() method (now instantiated) ends up
calling
> > > trace() on a DefaultAllocator (see bug), and that's not valid code. So
> > clang/win
> > > errors out. As a fix, we make the HashTable a member (so not all its
methods
> > get
> > > instantiated through dllexport), and now the trace() method is never used
> and
> > > all compilers are happy.
> > 
> > Thank you for this explanation. It's very clear what's going on now.
> > 
> > Is there a way to prevent this from happening again? A non-virtual dtor on
> > HashMap (and it's wtf-type friends) would make it a little more difficult to
> > subclass them, but that affects all subclasses and not just exported ones.
> 
> I think HashMap doesn't have any subclasses at the moment, so making it FINAL
> would prevent this I think. (But that should likely be in a different cl)

LGTM

[pdr., 2014-09-02 23:08:58]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-09-02 23:09:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thakis@chromium.org/536523002/40001

[commit-bot: I haz the power, 2014-09-02 23:13:05]

Committed patchset #3 (id:40001) as 181266

[gavinp, 2014-09-03 13:02:02]

On 2014/09/02 23:13:05, I haz the power (commit-bot) wrote:
> Committed patchset #3 (id:40001) as 181266

Losing  the == operator on HTTPHeaderMap broke a test I was in the process of
landing.

[gavinp, 2014-09-03 13:02:22]

On 2014/09/03 13:02:02, gavinp wrote:
> On 2014/09/02 23:13:05, I haz the power (commit-bot) wrote:
> > Committed patchset #3 (id:40001) as 181266
> 
> Losing  the == operator on HTTPHeaderMap broke a test I was in the process of
> landing.

See line
92:https://codereview.chromium.org/516123004/diff/60001/Source/modules/serviceworkers/RequestTest.cpp

[gavinp, 2014-09-03 13:15:20]

On 2014/09/03 13:02:22, gavinp wrote:
> On 2014/09/03 13:02:02, gavinp wrote:
> > On 2014/09/02 23:13:05, I haz the power (commit-bot) wrote:
> > > Committed patchset #3 (id:40001) as 181266
> > 
> > Losing  the == operator on HTTPHeaderMap broke a test I was in the process
of
> > landing.
> 
> See line
>
92:https://codereview.chromium.org/516123004/diff/60001/Source/modules/serviceworkers/RequestTest.cpp

We're adding it in our CL, if that's OK, see
https://codereview.chromium.org/516123004/#msg14

[Nico, 2014-09-03 15:45:56]

On Wed, Sep 3, 2014 at 6:15 AM, <gavinp@chromium.org> wrote:

> On 2014/09/03 13:02:22, gavinp wrote:
>
>> On 2014/09/03 13:02:02, gavinp wrote:
>> > On 2014/09/02 23:13:05, I haz the power (commit-bot) wrote:
>> > > Committed patchset #3 (id:40001) as 181266
>> >
>> > Losing  the == operator on HTTPHeaderMap broke a test I was in the
>> process
>>
> of
>
>> > landing.
>>
>
>  See line
>>
>
> 92:https://codereview.chromium.org/516123004/diff/60001/Source/modules/
> serviceworkers/RequestTest.cpp
>
> We're adding it in our CL, if that's OK, see
> https://codereview.chromium.org/516123004/#msg14
>

Sure, sounds good.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.