Fix flaky mac build due to missing permission_status.mojom.h.

Fix flaky mac build due to missing permission_status.mojom.h.

This patch adds an extra dependency on content.gyp:content_browser
to browser_app_shim in order to generate permission_status.mojom.h
before compile.

BUG=454447
Committed: https://crrev.com/563a7d861280e9922433c077c488ac6e54af44e4
Cr-Commit-Position: refs/heads/master@{#314333}

[timvolodine, 2015-02-02 18:38:05]

timvolodine@chromium.org changed reviewers:
+ jam@chromium.org

[blundell, 2015-02-02 19:20:32]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2015-02-02 19:20:33]

https://codereview.chromium.org/894103002/diff/1/chrome/browser/apps/app_shim...
File chrome/browser/apps/app_shim/browser_app_shim.gypi (right):

https://codereview.chromium.org/894103002/diff/1/chrome/browser/apps/app_shim...
chrome/browser/apps/app_shim/browser_app_shim.gypi:16:
'../content/content_common_mojo_bindings.gyp:content_common_mojo_bindings',
note: from the compile failure, it looks like the missing dependency should be
on chrome:chrome_browser. The chain of exported dependencies should ensure that
the required ordering is then satisfied.

[jam, 2015-02-02 20:39:16]

lgtm

https://codereview.chromium.org/894103002/diff/1/chrome/browser/apps/app_shim...
File chrome/browser/apps/app_shim/browser_app_shim.gypi (right):

https://codereview.chromium.org/894103002/diff/1/chrome/browser/apps/app_shim...
chrome/browser/apps/app_shim/browser_app_shim.gypi:16:
'../content/content_common_mojo_bindings.gyp:content_common_mojo_bindings',
On 2015/02/02 19:20:33, blundell wrote:
> note: from the compile failure, it looks like the missing dependency should be
> on chrome:chrome_browser. The chain of exported dependencies should ensure
that
> the required ordering is then satisfied.

afaik that can't happen or else there'll be a circular dependency per the
comment on line 13

[blundell, 2015-02-02 20:41:13]

https://codereview.chromium.org/894103002/diff/1/chrome/browser/apps/app_shim...
File chrome/browser/apps/app_shim/browser_app_shim.gypi (right):

https://codereview.chromium.org/894103002/diff/1/chrome/browser/apps/app_shim...
chrome/browser/apps/app_shim/browser_app_shim.gypi:16:
'../content/content_common_mojo_bindings.gyp:content_common_mojo_bindings',
On 2015/02/02 20:39:16, jam wrote:
> On 2015/02/02 19:20:33, blundell wrote:
> > note: from the compile failure, it looks like the missing dependency should
be
> > on chrome:chrome_browser. The chain of exported dependencies should ensure
> that
> > the required ordering is then satisfied.
> 
> afaik that can't happen or else there'll be a circular dependency per the
> comment on line 13

Oops, missed that. content_browser is probably still a better dependency than
this really specific one in that case?

[jam, 2015-02-03 05:22:13]

https://codereview.chromium.org/894103002/diff/1/chrome/browser/apps/app_shim...
File chrome/browser/apps/app_shim/browser_app_shim.gypi (right):

https://codereview.chromium.org/894103002/diff/1/chrome/browser/apps/app_shim...
chrome/browser/apps/app_shim/browser_app_shim.gypi:16:
'../content/content_common_mojo_bindings.gyp:content_common_mojo_bindings',
On 2015/02/02 20:41:13, blundell wrote:
> On 2015/02/02 20:39:16, jam wrote:
> > On 2015/02/02 19:20:33, blundell wrote:
> > > note: from the compile failure, it looks like the missing dependency
should
> be
> > > on chrome:chrome_browser. The chain of exported dependencies should ensure
> > that
> > > the required ordering is then satisfied.
> > 
> > afaik that can't happen or else there'll be a circular dependency per the
> > comment on line 13
> 
> Oops, missed that. content_browser is probably still a better dependency than
> this really specific one in that case?

sg

[timvolodine, 2015-02-03 10:47:06]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2015-02-03 10:48:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894103002/1

[blundell, 2015-02-03 10:49:42]

The CQ bit was unchecked by blundell@chromium.org

[blundell, 2015-02-03 10:50:22]

please change the dependency to be on content_browser, which will have a greater
chance of eliminating other problems of this nature that could occur in the
future. Thanks!

[timvolodine, 2015-02-03 11:06:20]

On 2015/02/03 10:50:22, blundell wrote:
> please change the dependency to be on content_browser, which will have a
greater
> chance of eliminating other problems of this nature that could occur in the
> future. Thanks!

bundell@: sure I can do that, but looking at the browser_app_shim target there
is no dependency on content_browser, so I don't see how this would resolve the
issue.. unless I am missing something

[blundell, 2015-02-03 12:20:05]

On 2015/02/03 11:06:20, timvolodine wrote:
> On 2015/02/03 10:50:22, blundell wrote:
> > please change the dependency to be on content_browser, which will have a
> greater
> > chance of eliminating other problems of this nature that could occur in the
> > future. Thanks!
> 
> bundell@: sure I can do that, but looking at the browser_app_shim target there
> is no dependency on content_browser, so I don't see how this would resolve the
> issue.. unless I am missing something

As we discussed offline, the dependency chain looks like:
shim -> chrome:browser -> content:browser -> content_common_mojo_bindings

chrome:browser and content:browser correctly transitively export the dependent
settings of content_common_mojo_bindings, so adding either of them as a
dependency will fix this problem. The right one to add would be chrome:browser
as that's the actual direct dependency, but that's not possible. Failing that,
content:browser is still a lot better than the specific mojo_bindings target.

[timvolodine, 2015-02-03 12:55:17]

On 2015/02/03 12:20:05, blundell wrote:
> On 2015/02/03 11:06:20, timvolodine wrote:
> > On 2015/02/03 10:50:22, blundell wrote:
> > > please change the dependency to be on content_browser, which will have a
> > greater
> > > chance of eliminating other problems of this nature that could occur in
the
> > > future. Thanks!
> > 
> > bundell@: sure I can do that, but looking at the browser_app_shim target
there
> > is no dependency on content_browser, so I don't see how this would resolve
the
> > issue.. unless I am missing something
> 
> As we discussed offline, the dependency chain looks like:
> shim -> chrome:browser -> content:browser -> content_common_mojo_bindings
> 
> chrome:browser and content:browser correctly transitively export the dependent
> settings of content_common_mojo_bindings, so adding either of them as a
> dependency will fix this problem. The right one to add would be chrome:browser
> as that's the actual direct dependency, but that's not possible. Failing that,
> content:browser is still a lot better than the specific mojo_bindings target.

thanks Colin for the chat!
I've modified the patch to content.gyp:content_browser as you suggested. I've
also added it to export_dependent_settings, which I understood is required as
the shim target transitively depends on content_browser. PTAL!

[blundell, 2015-02-03 13:02:02]

https://codereview.chromium.org/894103002/diff/20001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/894103002/diff/20001/content/content.gyp#newc...
content/content.gyp:158:
'content_common_mojo_bindings.gyp:content_common_mojo_bindings',
Oops, sorry for the misunderstanding. content.gyp:browser already has and
exports this dependency: it's hidden in content_browser.gypi. What I was
suggesting was adding content.gyp:browser as a dependency of the shim.

[timvolodine, 2015-02-03 13:54:18]

On 2015/02/03 13:02:02, blundell wrote:
> https://codereview.chromium.org/894103002/diff/20001/content/content.gyp
> File content/content.gyp (right):
> 
>
https://codereview.chromium.org/894103002/diff/20001/content/content.gyp#newc...
> content/content.gyp:158:
> 'content_common_mojo_bindings.gyp:content_common_mojo_bindings',
> Oops, sorry for the misunderstanding. content.gyp:browser already has and
> exports this dependency: it's hidden in content_browser.gypi. What I was
> suggesting was adding content.gyp:browser as a dependency of the shim.

ah ok, I was confused by the browser comment in shim. pls take another look.

[blundell, 2015-02-03 14:52:48]

lgtm, thanks

[timvolodine, 2015-02-03 14:58:24]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2015-02-03 14:59:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894103002/40001

[commit-bot: I haz the power, 2015-02-03 15:03:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-03 15:05:12]

Patchset 3 (id:??) landed as
https://crrev.com/563a7d861280e9922433c077c488ac6e54af44e4
Cr-Commit-Position: refs/heads/master@{#314333}