Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8343046:
export DownloadRequestHandle (Closed)
|
Created:
9 years, 1 month ago by Dirk Pranke Modified:
9 years, 1 month ago CC:
chromium-reviews, jam, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, rdsmith+dwatch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionexport DownloadRequestHandle - this is needed in both content_unittests and unit_tests.
R=jam@chromium.org
BUG=90442
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107762
Patch Set 1 #
Messages
Total messages: 12 (0 generated)
On 2011/10/28 01:28:34, Dirk Pranke wrote: For the record, I don't think this should be an exported object--if it's needed in unit_tests, that's a bug in unit_tests. You can export it if you want, but I'm doing some unit_test work this quarter--any objection to my unexporting it if I can remove it from unit_tests?
It doesn't look like there's a CONTENT_EXPORT_PRIVATE, which may make sense here. For example, the net module uses NET_EXPORT_PRIVATE to expose some classes to unit tests, but that are not intended to be used by non-testing modules. Under the hood, NET_EXPORT and NET_EXPORT_PRIVATE are the same so this is more of a signal to authors than anything else. On Fri, Oct 28, 2011 at 8:16 AM, <rdsmith@chromium.org> wrote: > On 2011/10/28 01:28:34, Dirk Pranke wrote: > > For the record, I don't think this should be an exported object--if it's > needed > in unit_tests, that's a bug in unit_tests. You can export it if you want, > but > I'm doing some unit_test work this quarter--any objection to my unexporting > it > if I can remove it from unit_tests? > > > http://codereview.chromium.**org/8343046/<http://codereview.chromium.org/8343... >
lgtm
Chris is correct, we don't normally distinguish between symbols that needed for regular use and symbols only needed for tests (net is an exception in this regard, but we've since decided that this is a practice we don't want to continue). In addition, the symbol will be need to be exported even in content_unittests, since that will also link against a DLL in the component build. These are unfortunate side effects of the way symbol exporting works on windows; we need to get used to the idea that exporting symbols does not indicate that they are "public". Does that all make sense? -- Dirk On Fri, Oct 28, 2011 at 7:55 AM, Chris Bentzel <cbentzel@chromium.org> wrote: > It doesn't look like there's a CONTENT_EXPORT_PRIVATE, which may make sense > here. For example, the net module uses NET_EXPORT_PRIVATE to expose some > classes to unit tests, but that are not intended to be used by non-testing > modules. > Under the hood, NET_EXPORT and NET_EXPORT_PRIVATE are the same so this is > more of a signal to authors than anything else. > On Fri, Oct 28, 2011 at 8:16 AM, <rdsmith@chromium.org> wrote: >> >> On 2011/10/28 01:28:34, Dirk Pranke wrote: >> >> For the record, I don't think this should be an exported object--if it's >> needed >> in unit_tests, that's a bug in unit_tests. You can export it if you want, >> but >> I'm doing some unit_test work this quarter--any objection to my >> unexporting it >> if I can remove it from unit_tests? >> >> >> http://codereview.chromium.org/8343046/ > >
Yes. LGTM for my driveby.
On 2011/10/28 16:29:54, Dirk Pranke wrote: > Chris is correct, we don't normally distinguish between symbols that > needed for regular use and symbols only needed for tests (net is an > exception in this regard, but we've since decided that this is a > practice we don't want to continue). > > In addition, the symbol will be need to be exported even in > content_unittests, since that will also link against a DLL in the > component build. > > These are unfortunate side effects of the way symbol exporting works > on windows; we need to get used to the idea that exporting symbols > does not indicate that they are "public". > > Does that all make sense? I understand the practical argument, but I thought the point of CONTENT_EXPORT was to define an API, and if we're having to use it in a way that interferes with that point, shouldn't we reconsider how we're using it? > > -- Dirk > > On Fri, Oct 28, 2011 at 7:55 AM, Chris Bentzel <mailto:cbentzel@chromium.org> wrote: > > It doesn't look like there's a CONTENT_EXPORT_PRIVATE, which may make sense > > here. For example, the net module uses NET_EXPORT_PRIVATE to expose some > > classes to unit tests, but that are not intended to be used by non-testing > > modules. > > Under the hood, NET_EXPORT and NET_EXPORT_PRIVATE are the same so this is > > more of a signal to authors than anything else. > > On Fri, Oct 28, 2011 at 8:16 AM, <mailto:rdsmith@chromium.org> wrote: > >> > >> On 2011/10/28 01:28:34, Dirk Pranke wrote: > >> > >> For the record, I don't think this should be an exported object--if it's > >> needed > >> in unit_tests, that's a bug in unit_tests. You can export it if you want, > >> but > >> I'm doing some unit_test work this quarter--any objection to my > >> unexporting it > >> if I can remove it from unit_tests? > >> > >> > >> http://codereview.chromium.org/8343046/ > > > >
On 2011/10/28 17:22:27, rdsmith wrote: > On 2011/10/28 16:29:54, Dirk Pranke wrote: > > Chris is correct, we don't normally distinguish between symbols that > > needed for regular use and symbols only needed for tests (net is an > > exception in this regard, but we've since decided that this is a > > practice we don't want to continue). > > > > In addition, the symbol will be need to be exported even in > > content_unittests, since that will also link against a DLL in the > > component build. > > > > These are unfortunate side effects of the way symbol exporting works > > on windows; we need to get used to the idea that exporting symbols > > does not indicate that they are "public". > > > > Does that all make sense? > > I understand the practical argument, but I thought the point of CONTENT_EXPORT > was to define an API, and if we're having to use it in a way that interferes > with that point, shouldn't we reconsider how we're using it? > > > > > -- Dirk > > > > On Fri, Oct 28, 2011 at 7:55 AM, Chris Bentzel <mailto:cbentzel@chromium.org> > wrote: > > > It doesn't look like there's a CONTENT_EXPORT_PRIVATE, which may make sense > > > here. For example, the net module uses NET_EXPORT_PRIVATE to expose some > > > classes to unit tests, but that are not intended to be used by non-testing > > > modules. > > > Under the hood, NET_EXPORT and NET_EXPORT_PRIVATE are the same so this is > > > more of a signal to authors than anything else. > > > On Fri, Oct 28, 2011 at 8:16 AM, <mailto:rdsmith@chromium.org> wrote: > > >> > > >> On 2011/10/28 01:28:34, Dirk Pranke wrote: > > >> > > >> For the record, I don't think this should be an exported object--if it's > > >> needed > > >> in unit_tests, that's a bug in unit_tests. You can export it if you > want, > > >> but > > >> I'm doing some unit_test work this quarter--any objection to my > > >> unexporting it > > >> if I can remove it from unit_tests? > > >> > > >> > > >> http://codereview.chromium.org/8343046/ > > > > > > (In case this isn't clear) I'm not saying lgtm because my comments aren't adding me to the reviewer list and I'm ok with that. I guess "reply" no longer adds you to the reviewer list automatically; I'll need to remember that :-}.
On Fri, Oct 28, 2011 at 10:22 AM, <rdsmith@chromium.org> wrote: > On 2011/10/28 16:29:54, Dirk Pranke wrote: > >> Chris is correct, we don't normally distinguish between symbols that >> needed for regular use and symbols only needed for tests (net is an >> exception in this regard, but we've since decided that this is a >> practice we don't want to continue). >> > > In addition, the symbol will be need to be exported even in >> content_unittests, since that will also link against a DLL in the >> component build. >> > > These are unfortunate side effects of the way symbol exporting works >> on windows; we need to get used to the idea that exporting symbols >> does not indicate that they are "public". >> > > Does that all make sense? >> > > I understand the practical argument, but I thought the point of > CONTENT_EXPORT > was to define an API, not quite, as Dirk said this will also be used by unit tests the api is defined by code which is in content/public > and if we're having to use it in a way that interferes > with that point, shouldn't we reconsider how we're using it? > > -- Dirk >> > > On Fri, Oct 28, 2011 at 7:55 AM, Chris Bentzel <mailto: >> cbentzel@chromium.org> >> > wrote: > >> > It doesn't look like there's a CONTENT_EXPORT_PRIVATE, which may make >> sense >> > here. For example, the net module uses NET_EXPORT_PRIVATE to expose some >> > classes to unit tests, but that are not intended to be used by >> non-testing >> > modules. >> > Under the hood, NET_EXPORT and NET_EXPORT_PRIVATE are the same so this >> is >> > more of a signal to authors than anything else. >> > On Fri, Oct 28, 2011 at 8:16 AM, <mailto:rdsmith@chromium.org> wrote: >> >> >> >> On 2011/10/28 01:28:34, Dirk Pranke wrote: >> >> >> >> For the record, I don't think this should be an exported object--if >> it's >> >> needed >> >> in unit_tests, that's a bug in unit_tests. You can export it if >> you >> > want, > >> >> but >> >> I'm doing some unit_test work this quarter--any objection to my >> >> unexporting it >> >> if I can remove it from unit_tests? >> >> >> >> >> >> http://codereview.chromium.**org/8343046/<http://codereview.chromium.org/8343... >> > >> > >> > > > > http://codereview.chromium.**org/8343046/<http://codereview.chromium.org/8343... >
On Fri, Oct 28, 2011 at 10:26 AM, John Abd-El-Malek <jam@chromium.org> wrote: > > > On Fri, Oct 28, 2011 at 10:22 AM, <rdsmith@chromium.org> wrote: >> >> On 2011/10/28 16:29:54, Dirk Pranke wrote: >>> >>> Chris is correct, we don't normally distinguish between symbols that >>> needed for regular use and symbols only needed for tests (net is an >>> exception in this regard, but we've since decided that this is a >>> practice we don't want to continue). >> >>> In addition, the symbol will be need to be exported even in >>> content_unittests, since that will also link against a DLL in the >>> component build. >> >>> These are unfortunate side effects of the way symbol exporting works >>> on windows; we need to get used to the idea that exporting symbols >>> does not indicate that they are "public". >> >>> Does that all make sense? >> >> I understand the practical argument, but I thought the point of >> CONTENT_EXPORT >> was to define an API, > > not quite, as Dirk said this will also be used by unit tests > the api is defined by code which is in content/public > Right, what I'm saying is that CONTENT_EXPORT has nothing to do with what is considered to be the API, and we need to think of it as an annoying implementation artifact of the way linkers work, nothing more. -- Dirk
On 2011/10/28 18:25:37, Dirk Pranke wrote: > On Fri, Oct 28, 2011 at 10:26 AM, John Abd-El-Malek <mailto:jam@chromium.org> wrote: > > > > > > On Fri, Oct 28, 2011 at 10:22 AM, <mailto:rdsmith@chromium.org> wrote: > >> > >> On 2011/10/28 16:29:54, Dirk Pranke wrote: > >>> > >>> Chris is correct, we don't normally distinguish between symbols that > >>> needed for regular use and symbols only needed for tests (net is an > >>> exception in this regard, but we've since decided that this is a > >>> practice we don't want to continue). > >> > >>> In addition, the symbol will be need to be exported even in > >>> content_unittests, since that will also link against a DLL in the > >>> component build. > >> > >>> These are unfortunate side effects of the way symbol exporting works > >>> on windows; we need to get used to the idea that exporting symbols > >>> does not indicate that they are "public". > >> > >>> Does that all make sense? > >> > >> I understand the practical argument, but I thought the point of > >> CONTENT_EXPORT > >> was to define an API, > > > > not quite, as Dirk said this will also be used by unit tests > > the api is defined by code which is in content/public > > > > Right, what I'm saying is that CONTENT_EXPORT has nothing to do with > what is considered to be the API, and we need to think of it as an > annoying implementation artifact of the way linkers work, nothing > more. > > -- Dirk Got it. Sorry to be slow.
On Fri, Oct 28, 2011 at 11:28 AM, <rdsmith@chromium.org> wrote: > On 2011/10/28 18:25:37, Dirk Pranke wrote: >> >> On Fri, Oct 28, 2011 at 10:26 AM, John Abd-El-Malek >> <mailto:jam@chromium.org> > > wrote: >> >> > >> > >> > On Fri, Oct 28, 2011 at 10:22 AM, <mailto:rdsmith@chromium.org> wrote: >> >> >> >> On 2011/10/28 16:29:54, Dirk Pranke wrote: >> >>> >> >>> Chris is correct, we don't normally distinguish between symbols that >> >>> needed for regular use and symbols only needed for tests (net is an >> >>> exception in this regard, but we've since decided that this is a >> >>> practice we don't want to continue). >> >> >> >>> In addition, the symbol will be need to be exported even in >> >>> content_unittests, since that will also link against a DLL in the >> >>> component build. >> >> >> >>> These are unfortunate side effects of the way symbol exporting works >> >>> on windows; we need to get used to the idea that exporting symbols >> >>> does not indicate that they are "public". >> >> >> >>> Does that all make sense? >> >> >> >> I understand the practical argument, but I thought the point of >> >> CONTENT_EXPORT >> >> was to define an API, >> > >> > not quite, as Dirk said this will also be used by unit tests >> > the api is defined by code which is in content/public >> > > >> Right, what I'm saying is that CONTENT_EXPORT has nothing to do with >> what is considered to be the API, and we need to think of it as an >> annoying implementation artifact of the way linkers work, nothing >> more. > >> -- Dirk > > Got it. Sorry to be slow. > Not at all; this is pretty much the opposite of what everyone expects :) -- Dirk |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
