Have Chrome Frame build its .idl artifacts into a sub-folder of SHARED_INTERMEDIATE_DIR, a global...

Have Chrome Frame build its .idl artifacts into a sub-folder of SHARED_INTERMEDIATE_DIR, a globally common output folder. It's rude to dump things in the root of the dir that aren't used project-wide.

Also explicitly list the installer -> ChromeFrame dependency in src\chrome\installer\DEPS. It was already present but now check_deps catches it due to the correctly formatted include path.Remove several redundant instances of SHARED_INTERMEDIATE_DIR being added to the include lists.

BUG=97185
TEST=Chrome Frame artifacts do not appear in e.g. build/Debug/obj/global_intermediate/
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106287

[robertshield, 2011-10-17 19:30:52]

Attempt to reland CL reviewed in http://codereview.chromium.org/8150002/ such
that it builds on after a clobber. PTAL.

[robertshield, 2011-10-18 03:22:52]

On 2011/10/17 19:30:52, robertshield wrote:
> Attempt to reland CL reviewed in http://codereview.chromium.org/8150002/ such
> that it builds on after a clobber. PTAL.

PTAL. Main change is that I rejiggered the .idl inclusion in
chrome_frame_launcher.gyp and added a couple of files that had somehow been
excluded earlier. This now builds correctly with a clobber build.

[grt (UTC plus 2), 2011-10-18 15:56:24]

http://codereview.chromium.org/8317018/diff/13002/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/8317018/diff/13002/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:38: #include
"chrome_frame/chrome_tab.h"
looks like you'll need to add "+chrome_frame" to chrome/installer/setup/DEPS to
allow this; and chrome/chrome_installer_util.gypi's installer_util target should
depend on chrome_frame:chrome_tab_idl, and should list
'<(SHARED_INTERMEDIATE_DIR)/chrome_frame/chrome_tab.h' as a source.  oh, and its
resource_include_dirs should be fixed up, too.

all because you wouldn't let me hardcode the IE low rights policy GUID.  :-)

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.gyp
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:73: 'include_dirs':
['<(SHARED_INTERMEDIATE_DIR)'],
why not also:
'resource_include_dirs': ['<(SHARED_INTERMEDIATE_DIR)'],
?  i guess not all targets that depend on this will need the .tlb file, but i
don't see the harm in including this dir here and removing it from all of the
other places.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:128: '<(SHARED_INTERMEDIATE_DIR)',
not sure this is needed. remove it and see what breaks?

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:539: 'chrome_frame_strings',
since this has sources that include chrome_tab.h, it should directly depend on
chrome_tab_idl.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:869: 'include_dirs': [
this shouldn't be here.  ideally, there should be no occurrences of
'<(SHARED_INTERMEDIATE_DIR)/chrome_frame' in this file aside from setting the
output dir for midl.exe, since all consumers use "chrome_frame/" as part of the
include path.  this specific include_dirs section shouldn't be needed on account
of the direct_dependent_settings on chrome_tab_idl.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:874: '<(SHARED_INTERMEDIATE_DIR)/chrome_frame'
how about editing resources/tlb_resource.rc to change the TYPELIB to use
"chrome_frame/chrome_tab.tlb" and either remove "/chrome_frame" from this
string, or remove resource_include_dirs altogether and switch to using
direct_dependent_settings to set this dir way up in chrome_tab_idl?

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
File chrome_frame/chrome_frame_launcher.gyp (right):

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:73: '<(SHARED_INTERMEDIATE_DIR)',
i think this is here for version.h.  could you add a comment to that effect?

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:105: '<(SHARED_INTERMEDIATE_DIR)',
same comment: i think these are all here because of the chrome_version_header
dependency.  it'd be cleaner if that target added '<(SHARED_INTERMEDIATE_DIR)'
as a resource_include_dir for direct dependents.  sigh.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:128: 'include_dirs': [
this shouldn't be needed since chrome_tab_idl specifies it as a
direct_dependent_setting.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:133: '<(SHARED_INTERMEDIATE_DIR)',
sigh

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:162: 'include_dirs': [
remove for same reason as above

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:167: '<(SHARED_INTERMEDIATE_DIR)',
more sighing

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_tab.cc#...
chrome_frame/chrome_tab.cc:8: #include "chrome_frame/chrome_tab.h"
should this still have // NOLINT since it appears out of order?

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/iids.cc
File chrome_frame/iids.cc (right):

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/iids.cc#newcode1
chrome_frame/iids.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights
reserved.
nice!

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/urlmon_bind_st...
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/urlmon_bind_st...
chrome_frame/urlmon_bind_status_callback.cc:18: #include
"chrome_frame/chrome_tab.h"
move up above chrome_frame/exception_barrier.h

[robertshield, 2011-10-18 17:31:10]

http://codereview.chromium.org/8317018/diff/13002/chrome/installer/setup/inst...
File chrome/installer/setup/install_worker.cc (right):

http://codereview.chromium.org/8317018/diff/13002/chrome/installer/setup/inst...
chrome/installer/setup/install_worker.cc:38: #include
"chrome_frame/chrome_tab.h"
On 2011/10/18 15:56:24, grt wrote:
> looks like you'll need to add "+chrome_frame" to chrome/installer/setup/DEPS
to
> allow this; and chrome/chrome_installer_util.gypi's installer_util target
should
> depend on chrome_frame:chrome_tab_idl, and should list
> '<(SHARED_INTERMEDIATE_DIR)/chrome_frame/chrome_tab.h' as a source.  oh, and
its
> resource_include_dirs should be fixed up, too.
> 
> all because you wouldn't let me hardcode the IE low rights policy GUID.  :-)

Yeah, doing the right thing is rarely easier :-)

DEPS updated. I think by chrome_installer_util.gypi you meant
chrome_installer.gypi, and this is already up to date.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.gyp
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:73: 'include_dirs':
['<(SHARED_INTERMEDIATE_DIR)'],
On 2011/10/18 15:56:24, grt wrote:
> why not also:
> 'resource_include_dirs': ['<(SHARED_INTERMEDIATE_DIR)'],
> ?  i guess not all targets that depend on this will need the .tlb file, but i
> don't see the harm in including this dir here and removing it from all of the
> other places.

The only target ever really likely to need the .tlb file is the CF DLL target,
so I feel it doesn't make sense to expand the resource_include_dir for all
dependents of this target.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:128: '<(SHARED_INTERMEDIATE_DIR)',
On 2011/10/18 15:56:24, grt wrote:
> not sure this is needed. remove it and see what breaks?

Done.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:539: 'chrome_frame_strings',
On 2011/10/18 15:56:24, grt wrote:
> since this has sources that include chrome_tab.h, it should directly depend on
> chrome_tab_idl.

Done.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:869: 'include_dirs': [
On 2011/10/18 15:56:24, grt wrote:
> this shouldn't be here.  ideally, there should be no occurrences of
> '<(SHARED_INTERMEDIATE_DIR)/chrome_frame' in this file aside from setting the
> output dir for midl.exe, since all consumers use "chrome_frame/" as part of
the
> include path.  this specific include_dirs section shouldn't be needed on
account
> of the direct_dependent_settings on chrome_tab_idl.

Done.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:874: '<(SHARED_INTERMEDIATE_DIR)/chrome_frame'
On 2011/10/18 15:56:24, grt wrote:
> how about editing resources/tlb_resource.rc to change the TYPELIB to use
> "chrome_frame/chrome_tab.tlb" and either remove "/chrome_frame" from this
> string, or remove resource_include_dirs altogether and switch to using
> direct_dependent_settings to set this dir way up in chrome_tab_idl?

Done (the former).

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
File chrome_frame/chrome_frame_launcher.gyp (right):

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:73: '<(SHARED_INTERMEDIATE_DIR)',
On 2011/10/18 15:56:24, grt wrote:
> i think this is here for version.h.  could you add a comment to that effect?

Done.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:105: '<(SHARED_INTERMEDIATE_DIR)',
On 2011/10/18 15:56:24, grt wrote:
> same comment: i think these are all here because of the chrome_version_header
> dependency.  it'd be cleaner if that target added '<(SHARED_INTERMEDIATE_DIR)'
> as a resource_include_dir for direct dependents.  sigh.

Yes, but as discussed that is not in the scope of this CL.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:128: 'include_dirs': [
On 2011/10/18 15:56:24, grt wrote:
> this shouldn't be needed since chrome_tab_idl specifies it as a
> direct_dependent_setting.

Removed.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:133: '<(SHARED_INTERMEDIATE_DIR)',
On 2011/10/18 15:56:24, grt wrote:
> sigh

I can make you a cake. Maybe that will make you feel better?

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:162: 'include_dirs': [
On 2011/10/18 15:56:24, grt wrote:
> remove for same reason as above

Done.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:167: '<(SHARED_INTERMEDIATE_DIR)',
On 2011/10/18 15:56:24, grt wrote:
> more sighing

The cake was a lie.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_tab.cc#...
chrome_frame/chrome_tab.cc:8: #include "chrome_frame/chrome_tab.h"
On 2011/10/18 15:56:24, grt wrote:
> should this still have // NOLINT since it appears out of order?

It would appear not.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/iids.cc
File chrome_frame/iids.cc (right):

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/iids.cc#newcode1
chrome_frame/iids.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights
reserved.
On 2011/10/18 15:56:24, grt wrote:
> nice!

Maybe you should make me a cake.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/urlmon_bind_st...
File chrome_frame/urlmon_bind_status_callback.cc (right):

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/urlmon_bind_st...
chrome_frame/urlmon_bind_status_callback.cc:18: #include
"chrome_frame/chrome_tab.h"
On 2011/10/18 15:56:24, grt wrote:
> move up above chrome_frame/exception_barrier.h

Done.

[grt (UTC plus 2), 2011-10-18 20:03:53]

LGTM with 1 nit.

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.gyp
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.g...
chrome_frame/chrome_frame.gyp:73: 'include_dirs':
['<(SHARED_INTERMEDIATE_DIR)'],
On 2011/10/18 17:31:10, robertshield wrote:
> On 2011/10/18 15:56:24, grt wrote:
> > why not also:
> > 'resource_include_dirs': ['<(SHARED_INTERMEDIATE_DIR)'],
> > ?  i guess not all targets that depend on this will need the .tlb file, but
i
> > don't see the harm in including this dir here and removing it from all of
the
> > other places.
> 
> The only target ever really likely to need the .tlb file is the CF DLL target,
> so I feel it doesn't make sense to expand the resource_include_dir for all
> dependents of this target.

nod.

http://codereview.chromium.org/8317018/diff/16008/chrome_frame/resources/tlb_...
File chrome_frame/resources/tlb_resource.rc (right):

http://codereview.chromium.org/8317018/diff/16008/chrome_frame/resources/tlb_...
chrome_frame/resources/tlb_resource.rc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
HA!  GOTCHA!

I'd like a chocolate cake, please.

[robertshield, 2011-10-19 02:21:28]

On 2011/10/18 20:03:53, grt wrote:
> LGTM with 1 nit.
> 
>
http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.gyp
> File chrome_frame/chrome_frame.gyp (right):
> 
>
http://codereview.chromium.org/8317018/diff/13002/chrome_frame/chrome_frame.g...
> chrome_frame/chrome_frame.gyp:73: 'include_dirs':
> ['<(SHARED_INTERMEDIATE_DIR)'],
> On 2011/10/18 17:31:10, robertshield wrote:
> > On 2011/10/18 15:56:24, grt wrote:
> > > why not also:
> > > 'resource_include_dirs': ['<(SHARED_INTERMEDIATE_DIR)'],
> > > ?  i guess not all targets that depend on this will need the .tlb file,
but
> i
> > > don't see the harm in including this dir here and removing it from all of
> the
> > > other places.
> > 
> > The only target ever really likely to need the .tlb file is the CF DLL
target,
> > so I feel it doesn't make sense to expand the resource_include_dir for all
> > dependents of this target.
> 
> nod.
> 
>
http://codereview.chromium.org/8317018/diff/16008/chrome_frame/resources/tlb_...
> File chrome_frame/resources/tlb_resource.rc (right):
> 
>
http://codereview.chromium.org/8317018/diff/16008/chrome_frame/resources/tlb_...
> chrome_frame/resources/tlb_resource.rc:1: // Copyright (c) 2009 The Chromium
> Authors. All rights reserved.
> HA!  GOTCHA!
> 
> I'd like a chocolate cake, please.

It's in the mail :)