Move RegisterContentSchemes from public url_constants into non-public url_schemes.

Move RegisterContentSchemes from public url_constants into non-public url_schemes.

The VC++ linker can only discard whole .obj files that aren't used.  setup.exe and npchrome_frame.dll pull in constants from url_constants.cc.  http://crrev.com/133107 put RegisterContentSchemes into that file.  This function's use of ContentClient causes all of WebKit and V8 to be pulled into setup.exe and npchrome_frame.dll.

This change here moves RegisterContentSchemes into its own .cc file to get around link.exe's .obj-level code omission policy.

This is a hack, and I feel shame for committing it.  In the long run, we need a way to prevent changes that lead to a 6x increase in the size of the binaries spat out by the official builders.  One could imagine this being done by a stage in a try run that goes red, or some build-time enforcement.  Perhaps the constants should even go in their own target so that consumers of nothing but constants don't accidentally get content's transient dependencies.

BUG=127564
TEST=none

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

[grt (UTC plus 2), 2012-05-23 04:29:28]

John: you have sufficient OWNERShip to take care of this in one fell swoop. 
Would you mind having a look?

[jam, 2012-05-23 04:59:55]

I have to agree that this is a hack :) I also agree we need to do a better job
of monitoring this stuff and turning a tree red; this issue has happened many
times with the chrome_frame binaries because of innocent-looking changes.

I think a better way to solve this would be to move RegisterContentSchemes out
of that public file. The only reason it's there is because it's called by
ChromeTestSuite. So it can be moved content/common/foo.h which
content_main_runner.cc includes and calls. Then we need some base test suite
object that both ChromeTestSuite and ContentTestSuite derive from. This would
have the duplicate parts of ContentTestSuite/ChromeTestSuite::Initialize, which
includes calling RegisterContentSchemes.

[grt (UTC plus 2), 2012-05-24 04:08:51]

Hi John,

Thanks for the suggestions.  I think I've taken care of the first part (move
RegisterContentSchemes into some .cc file that isn't in content/public) in this
CL.  I'd like to keep the scope of this CL small since I'll need to merge it
onto a release branch.  How do you feel about me opening a bug against michaeln
(the author of http://crrev.com/133107) asking him to take care of the
*TestSuite portion of your suggestion at his convenience?

https://chromiumcodereview.appspot.com/10383298/diff/12003/content/public/com...
File content/public/common/url_constants.h (right):

https://chromiumcodereview.appspot.com/10383298/diff/12003/content/public/com...
content/public/common/url_constants.h:72: void SetSavableSchemes(const char*
const* savable_schemes);
what do you think of this addition (i've intentionally left out the doc comment
for now)?  i needed a way for RegisterContentSchemes to update the array of
savable schemes somehow.  if you don't care for this, do you have an alternate
suggestion (e.g., put it in the content::internal namespace)?

[jam, 2012-05-24 05:13:47]

On 2012/05/24 04:08:51, grt wrote:
> Hi John,
> 
> Thanks for the suggestions.  I think I've taken care of the first part (move
> RegisterContentSchemes into some .cc file that isn't in content/public) in
this
> CL.  I'd like to keep the scope of this CL small since I'll need to merge it
> onto a release branch.  How do you feel about me opening a bug against
michaeln
> (the author of http://crrev.com/133107) asking him to take care of the
> *TestSuite portion of your suggestion at his convenience?

I'm not a fan of leaving code in a halfway state with a bug to finish, because
more often than not it always gets deprioritized. The suggestion of making a
base test suite is something that should be quick to do, and merge cleanly since
that code hasn't changed. What you have now won't work because of the checkdeps
failure (code in chrome can only access content/public).


> 
>
https://chromiumcodereview.appspot.com/10383298/diff/12003/content/public/com...
> File content/public/common/url_constants.h (right):
> 
>
https://chromiumcodereview.appspot.com/10383298/diff/12003/content/public/com...
> content/public/common/url_constants.h:72: void SetSavableSchemes(const char*
> const* savable_schemes);
> what do you think of this addition (i've intentionally left out the doc
comment
> for now)?  i needed a way for RegisterContentSchemes to update the array of
> savable schemes somehow.  if you don't care for this, do you have an alternate
> suggestion (e.g., put it in the content::internal namespace)?

[grt (UTC plus 2), 2012-05-24 16:02:17]

PTAL

[jam, 2012-05-24 16:17:25]

thanks, almost there

http://codereview.chromium.org/10383298/diff/19001/content/public/common/url_...
File content/public/common/url_constants.h (right):

http://codereview.chromium.org/10383298/diff/19001/content/public/common/url_...
content/public/common/url_constants.h:70: CONTENT_EXPORT const char* const*
GetSavableSchemes();
nit: i'd prefer to avoid this, and get rid of the const_cast in
render_view_impl.cc

http://codereview.chromium.org/10383298/diff/19001/content/public/common/url_...
content/public/common/url_constants.h:72: void SetSavableSchemes(const char*
const* savable_schemes);
we only put stuff in content/public that's needed by embedders. since this is
called internally by content, you'll have to rejig things a little. you can keep
the global in url_schemes.cc, and you can have the GetSavableSchemes function
above internally call another function in url_schemes to get access to it

http://codereview.chromium.org/10383298/diff/19001/content/test/content_test_...
File content/test/content_test_suite_base.h (right):

http://codereview.chromium.org/10383298/diff/19001/content/test/content_test_...
content/test/content_test_suite_base.h:13: namespace content {
nit: just put this whole class in content namespace (new code in content should
be in content namespace)

[grt (UTC plus 2), 2012-05-24 19:29:16]

Hi John,

I think I'm approaching what you're looking for here.  Let me know if I've done
the namespace changes and such in a way that fits in harmoniously with the style
in content.  My apologies for inadvertently deleting the previous patch set.

http://codereview.chromium.org/10383298/diff/10002/content/public/common/url_...
File content/public/common/url_constants.h (right):

http://codereview.chromium.org/10383298/diff/10002/content/public/common/url_...
content/public/common/url_constants.h:70: CONTENT_EXPORT const char* const*
GetSavableSchemes();
> nit: i'd prefer to avoid this, and get rid of the const_cast in
render_view_impl.cc

The double const is the right thing (callers shouldn't be permitted to modify
the strings or the array itself).  I've created
https://chromiumcodereview.appspot.com/10445025/ to fix the recipient in
src/webkit and remove the cast I'm adding here to render_view_impl.  (Note that
although the cast removal is present in my local branch, it doesn't appear in
the CL since the cast itself isn't present in ToT.)

Does this work for you?

[jam, 2012-05-24 19:49:26]

http://codereview.chromium.org/10383298/diff/10002/content/common/savable_url...
File content/common/savable_url_schemes.h (right):

http://codereview.chromium.org/10383298/diff/10002/content/common/savable_url...
content/common/savable_url_schemes.h:11: namespace internal {
nit: we don't use internal namespace in content

i meant put this function in content/common/url_schemes, no need to add
additional files

http://codereview.chromium.org/10383298/diff/10002/content/public/common/url_...
File content/public/common/url_constants.h (right):

http://codereview.chromium.org/10383298/diff/10002/content/public/common/url_...
content/public/common/url_constants.h:70: CONTENT_EXPORT const char* const*
GetSavableSchemes();
On 2012/05/24 19:29:21, grt wrote:
> > nit: i'd prefer to avoid this, and get rid of the const_cast in
> render_view_impl.cc
> 
> The double const is the right thing (callers shouldn't be permitted to modify
> the strings or the array itself).  I've created
> https://chromiumcodereview.appspot.com/10445025/ to fix the recipient in
> src/webkit and remove the cast I'm adding here to render_view_impl.  (Note
that
> although the cast removal is present in my local branch, it doesn't appear in
> the CL since the cast itself isn't present in ToT.)
> 
> Does this work for you?

I understand why you're doing this. it just makes readability harder imo. but i
don't feel strongly about this since it's technically more correct.

http://codereview.chromium.org/10383298/diff/10002/content/test/content_test_...
File content/test/content_test_suite_base.cc (right):

http://codereview.chromium.org/10383298/diff/10002/content/test/content_test_...
content/test/content_test_suite_base.cc:11: #include
"content/common/url_schemes.h"
not needed?

[jam, 2012-05-24 19:57:14]

http://codereview.chromium.org/10383298/diff/10002/content/common/savable_url...
File content/common/savable_url_schemes.h (right):

http://codereview.chromium.org/10383298/diff/10002/content/common/savable_url...
content/common/savable_url_schemes.h:11: namespace internal {
On 2012/05/24 19:49:26, John Abd-El-Malek wrote:
> nit: we don't use internal namespace in content
> 
> i meant put this function in content/common/url_schemes, no need to add
> additional files

just to be clear to save you any more round trips, I meant
content/common/url_schemes.cc has the g_savable_schemes. url_constants.cc's
GetSavableSchemes will call GetSavableSchemes in url_schemes.h

[grt (UTC plus 2), 2012-05-24 20:33:14]

http://codereview.chromium.org/10383298/diff/10002/content/common/savable_url...
File content/common/savable_url_schemes.h (right):

http://codereview.chromium.org/10383298/diff/10002/content/common/savable_url...
content/common/savable_url_schemes.h:11: namespace internal {
On 2012/05/24 19:57:15, John Abd-El-Malek wrote:
> On 2012/05/24 19:49:26, John Abd-El-Malek wrote:
> > nit: we don't use internal namespace in content
> > 
> > i meant put this function in content/common/url_schemes, no need to add
> > additional files
> 
> just to be clear to save you any more round trips, I meant
> content/common/url_schemes.cc has the g_savable_schemes. url_constants.cc's
> GetSavableSchemes will call GetSavableSchemes in url_schemes.h

That doesn't work since it brings back the original problem I'm trying to solve:
consumers of url_constants.obj will also get url_schemes.obj, which pulls in
content_client.obj and therefore all of WebKit and V8.  This is why I moved
g_savable_schemes into its own .obj file outside of public.  With the code the
way it is now, consumers of url_constants.obj will get savable_url_schemes.obj
and that's it.

Would you like me to get rid of namespace internal and rename these to
{G,S}etSavableSchemesInternal?  If not, what would be most fitting?

http://codereview.chromium.org/10383298/diff/10002/content/public/common/url_...
File content/public/common/url_constants.h (right):

http://codereview.chromium.org/10383298/diff/10002/content/public/common/url_...
content/public/common/url_constants.h:70: CONTENT_EXPORT const char* const*
GetSavableSchemes();
On 2012/05/24 19:49:26, John Abd-El-Malek wrote:
> On 2012/05/24 19:29:21, grt wrote:
> > > nit: i'd prefer to avoid this, and get rid of the const_cast in
> > render_view_impl.cc
> > 
> > The double const is the right thing (callers shouldn't be permitted to
modify
> > the strings or the array itself).  I've created
> > https://chromiumcodereview.appspot.com/10445025/ to fix the recipient in
> > src/webkit and remove the cast I'm adding here to render_view_impl.  (Note
> that
> > although the cast removal is present in my local branch, it doesn't appear
in
> > the CL since the cast itself isn't present in ToT.)
> > 
> > Does this work for you?
> 
> I understand why you're doing this. it just makes readability harder imo. but
i
> don't feel strongly about this since it's technically more correct.

I guess I'm on the other side of the readability fence.  The previous code made
me spend extra time thinking about why the array wasn't const, why there were
two const_casts, and why kDefaultSavableSchemes wasn't actually a constant.

http://codereview.chromium.org/10383298/diff/10002/content/test/content_test_...
File content/test/content_test_suite_base.cc (right):

http://codereview.chromium.org/10383298/diff/10002/content/test/content_test_...
content/test/content_test_suite_base.cc:11: #include
"content/common/url_schemes.h"
On 2012/05/24 19:49:26, John Abd-El-Malek wrote:
> not needed?

This is where RegisterContentSchemes lives.

[jam, 2012-05-24 21:50:02]

lgtm without internal namespace

http://codereview.chromium.org/10383298/diff/10002/content/common/savable_url...
File content/common/savable_url_schemes.h (right):

http://codereview.chromium.org/10383298/diff/10002/content/common/savable_url...
content/common/savable_url_schemes.h:11: namespace internal {
On 2012/05/24 20:33:14, grt wrote:
> On 2012/05/24 19:57:15, John Abd-El-Malek wrote:
> > On 2012/05/24 19:49:26, John Abd-El-Malek wrote:
> > > nit: we don't use internal namespace in content
> > > 
> > > i meant put this function in content/common/url_schemes, no need to add
> > > additional files
> > 
> > just to be clear to save you any more round trips, I meant
> > content/common/url_schemes.cc has the g_savable_schemes. url_constants.cc's
> > GetSavableSchemes will call GetSavableSchemes in url_schemes.h
> 
> That doesn't work since it brings back the original problem I'm trying to
solve:
> consumers of url_constants.obj will also get url_schemes.obj, which pulls in
> content_client.obj and therefore all of WebKit and V8.  This is why I moved
> g_savable_schemes into its own .obj file outside of public.  With the code the
> way it is now, consumers of url_constants.obj will get savable_url_schemes.obj
> and that's it.

got it

> 
> Would you like me to get rid of namespace internal and rename these to
> {G,S}etSavableSchemesInternal?  If not, what would be most fitting?

you can just get rid of the internal namespace and keep these the same as now
without an Internal suffix

[grt (UTC plus 2), 2012-05-25 03:15:03]

http://codereview.chromium.org/10383298/diff/10002/content/common/savable_url...
File content/common/savable_url_schemes.h (right):

http://codereview.chromium.org/10383298/diff/10002/content/common/savable_url...
content/common/savable_url_schemes.h:11: namespace internal {
On 2012/05/24 21:50:02, John Abd-El-Malek wrote:
> On 2012/05/24 20:33:14, grt wrote:
> > Would you like me to get rid of namespace internal and rename these to
> > {G,S}etSavableSchemesInternal?  If not, what would be most fitting?
> 
> you can just get rid of the internal namespace and keep these the same as now
> without an Internal suffix

Apologies if I'm missing something obvious, but that will mean that the
definition of GetSavableSchemes in the (non-public) savable_url_schemes.cc will
correspond with the declaration in the (public) url_constants.h (and there will
be no definition in url_constants.cc).  Is that desired (is there a precedent
for this in content already)?  Or did I totally misinterpret your suggestion? 
Please have a look at patch set 4 and let me know what you think.

[jam, 2012-05-25 08:12:04]

http://codereview.chromium.org/10383298/diff/10002/content/common/savable_url...
File content/common/savable_url_schemes.h (right):

http://codereview.chromium.org/10383298/diff/10002/content/common/savable_url...
content/common/savable_url_schemes.h:11: namespace internal {
On 2012/05/25 03:15:04, grt wrote:
> On 2012/05/24 21:50:02, John Abd-El-Malek wrote:
> > On 2012/05/24 20:33:14, grt wrote:
> > > Would you like me to get rid of namespace internal and rename these to
> > > {G,S}etSavableSchemesInternal?  If not, what would be most fitting?
> > 
> > you can just get rid of the internal namespace and keep these the same as
now
> > without an Internal suffix
> 
> Apologies if I'm missing something obvious, but that will mean that the
> definition of GetSavableSchemes in the (non-public) savable_url_schemes.cc
will
> correspond with the declaration in the (public) url_constants.h (and there
will
> be no definition in url_constants.cc).  Is that desired (is there a precedent
> for this in content already)?  Or did I totally misinterpret your suggestion? 
> Please have a look at patch set 4 and let me know what you think.

ah, I was missing that there were two functions with the same name. you can call
the one in the content/common header GetSavableSchemesInternal then. please
declare it in the header

[grt (UTC plus 2), 2012-05-25 14:09:55]

I think this may be it.  Let me know.

[grt (UTC plus 2), 2012-05-25 16:48:13]

Perhaps the last one?

http://codereview.chromium.org/10383298/diff/24005/content/public/common/url_...
File content/public/common/url_constants.cc (right):

http://codereview.chromium.org/10383298/diff/24005/content/public/common/url_...
content/public/common/url_constants.cc:62: return GetSavableSchemesInternal();
since i had to move savable_url_schemes.* into public to satisfy checkdeps,
there was no longer a need to keep this Internal method.  I've removed it in the
latest patch set.

[grt (UTC plus 2), 2012-05-25 21:02:17]

On 2012/05/25 16:48:13, grt wrote:
> Perhaps the last one?

Try runs are green at last.

[jam, 2012-05-28 07:05:03]

http://codereview.chromium.org/10383298/diff/12023/content/public/common/sava...
File content/public/common/savable_url_schemes.h (right):

http://codereview.chromium.org/10383298/diff/12023/content/public/common/sava...
content/public/common/savable_url_schemes.h:18: void SetSavableSchemes(const
char* const* savable_schemes);
we only put stuff in content/public if it's needed by embedders. since this is
only called by code in content, it doesn't belong in the public api.

how about:
-keep GetSavableSchemes in content/public/common/url_constants.h.
-savable_url_schemes.* are in content/common. the .h has a Set function and a
GetXInternal to access the global that's in the cc

[grt (UTC plus 2), 2012-05-28 14:52:23]

http://codereview.chromium.org/10383298/diff/12023/content/public/common/sava...
File content/public/common/savable_url_schemes.h (right):

http://codereview.chromium.org/10383298/diff/12023/content/public/common/sava...
content/public/common/savable_url_schemes.h:18: void SetSavableSchemes(const
char* const* savable_schemes);
On 2012/05/28 07:05:03, John Abd-El-Malek wrote:
> we only put stuff in content/public if it's needed by embedders. since this is
> only called by code in content, it doesn't belong in the public api.
> 
> how about:
> -keep GetSavableSchemes in content/public/common/url_constants.h.
> -savable_url_schemes.* are in content/common. the .h has a Set function and a
> GetXInternal to access the global that's in the cc

I've gone back to Patch Set 5, which was laid out like that, and added an
include_rules entry so that url_constants.cc can include savable_url_schemes.h
to get to GetSavableSchemesInternal.  PTAL.

[jam, 2012-05-28 17:15:29]

lgtm

[commit-bot: I haz the power, 2012-05-28 17:16:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/10383298/29002

[commit-bot: I haz the power, 2012-05-28 18:36:54]

Change committed as 139253