Implement HasPermission() method in PermissionService.

Implement HasPermission() method in PermissionService.

This patch implements the HasPermission() method in the mojo PermissionService.
This methiod will be required for the Permissions API, see
https://w3c.github.io/permissions/

BUG=430238
Committed: https://crrev.com/277b23d74855b51ebe67ae0da7cd9e0b053044cf
Cr-Commit-Position: refs/heads/master@{#307302}

Committed: https://crrev.com/14570267ac4d5dc473d29f256e8d044e9bfcc8d2
Cr-Commit-Position: refs/heads/master@{#307504}

Committed: https://crrev.com/089c07c966a9dc169f0822b90d92400bf23e19f7
Cr-Commit-Position: refs/heads/master@{#307948}

[timvolodine, 2014-11-21 18:39:38]

timvolodine@chromium.org changed reviewers:
+ blundell@chromium.org, mlamouri@chromium.org

[blundell, 2014-11-21 20:19:45]

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

[blundell, 2014-11-21 20:19:46]

blundell -> qsr

I'm going off on vacation for a week :).

[qsr, 2014-11-24 11:51:12]

https://codereview.chromium.org/750633003/diff/1/content/browser/permissions/...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/750633003/diff/1/content/browser/permissions/...
content/browser/permissions/permission_service_impl.cc:61: // to obtain the
profile.
The previous comment is wrong. Could you remove it. To change it in some kind of
TODO if you have plan to fix it.

[mlamouri (slow - plz ping), 2014-11-24 16:01:19]

Tim and I discussed this CL face to face. For the sake of transparency, I left a
few comments summarizing my feedback.

https://codereview.chromium.org/750633003/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/750633003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.h:202: content::WebContents*
web_contents,
We want to pass a BrowserContext here so we don't depend on WebContents.

https://codereview.chromium.org/750633003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.h:203: const GURL&
requesting_frame,
s/requesting_frame/requesting_origin/

Also, you might want to pass the embedding_origin like in
PermissionContextBase::GetPermissionStatus().

https://codereview.chromium.org/750633003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.h:204: const
base::Callback<void(bool)>& result_callback) override;
The callback shouldn't pass a |bool| back. We want to have more information, at
least ASK/GRANTED/DENIED.

https://codereview.chromium.org/750633003/diff/1/content/browser/permissions/...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/750633003/diff/1/content/browser/permissions/...
content/browser/permissions/permission_service_impl.cc:61: // to obtain the
profile.
On 2014/11/24 11:51:11, qsr wrote:
> The previous comment is wrong. Could you remove it. To change it in some kind
of
> TODO if you have plan to fix it.

We must implement HasPermission() to work in a non-frame context. Tim and I
discussed a plan to fix that where we could request the BrowserContext to
PermissionServiceContext.

[timvolodine, 2014-11-24 18:54:22]

thanks for the comments. they should be fixed now.

https://codereview.chromium.org/750633003/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/750633003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.h:202: content::WebContents*
web_contents,
On 2014/11/24 16:01:19, Mounir Lamouri wrote:
> We want to pass a BrowserContext here so we don't depend on WebContents.

Done.

https://codereview.chromium.org/750633003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.h:203: const GURL&
requesting_frame,
On 2014/11/24 16:01:19, Mounir Lamouri wrote:
> s/requesting_frame/requesting_origin/
> 
> Also, you might want to pass the embedding_origin like in
> PermissionContextBase::GetPermissionStatus().

Done.

https://codereview.chromium.org/750633003/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.h:204: const
base::Callback<void(bool)>& result_callback) override;
On 2014/11/24 16:01:19, Mounir Lamouri wrote:
> The callback shouldn't pass a |bool| back. We want to have more information,
at
> least ASK/GRANTED/DENIED.

done.
To do that I extracted permission_status enum into content/public/common to be
able to include in the content/public/browser/content_browser_client.h

https://codereview.chromium.org/750633003/diff/1/content/browser/permissions/...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/750633003/diff/1/content/browser/permissions/...
content/browser/permissions/permission_service_impl.cc:61: // to obtain the
profile.
On 2014/11/24 11:51:11, qsr wrote:
> The previous comment is wrong. Could you remove it. To change it in some kind
of
> TODO if you have plan to fix it.

Done.

https://codereview.chromium.org/750633003/diff/1/content/browser/permissions/...
content/browser/permissions/permission_service_impl.cc:61: // to obtain the
profile.
On 2014/11/24 16:01:19, Mounir Lamouri wrote:
> On 2014/11/24 11:51:11, qsr wrote:
> > The previous comment is wrong. Could you remove it. To change it in some
kind
> of
> > TODO if you have plan to fix it.
> 
> We must implement HasPermission() to work in a non-frame context. Tim and I
> discussed a plan to fix that where we could request the BrowserContext to
> PermissionServiceContext.

Done.
permission_context has an additional constructor now and an extra member
render_process_host_.

[mlamouri (slow - plz ping), 2014-11-24 19:06:01]

It looks great! I have a few minor comments below. Hopefully, this will be ready
soon! :)

https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:601: return
content::PERMISSION_STATUS_GRANTED;
nit: CONTENT_SETTING_SESSION_ONLY should also link to _GRANTED.

https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:607: return
content::PERMISSION_STATUS_ASK;
I think it would be better to not have a |default|. Instead, you should have a
case for _DEFAULT and _NUM_SETTINGS and call NOTREACHED() if you end up there.

https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2029:
result_callback.Run(ContentSettingToPermissionStatus(result));
Why do you use a callback instead of returning directly the value? It seems that
the call is synchronous, right?

https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.h:200: virtual void HasPermission(
nit: I think GetPermissionStatus() would be a better name here.

https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:123: : GURL(origin);
I think it would be better to have a
PermissionServiceContext::GetEmbeddingOrigin() instead of having the logic here.

https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:138:
callback.Run(content_setting);
nit: s/content_setting/permission_status/

https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.h:65: PermissionStatus
content_setting);
nit: s/content_setting/permission_status/

https://codereview.chromium.org/750633003/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/750633003/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.cc:246:
result_callback.Run(PERMISSION_STATUS_ASK);
nit: PERMISSION_STATUS_DENIED.

https://codereview.chromium.org/750633003/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/750633003/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:458: virtual void HasPermission(
nit: could you rename this GetPermissionStatus()?

https://codereview.chromium.org/750633003/diff/20001/content/public/common/pe...
File content/public/common/permission_status.mojom (right):

https://codereview.chromium.org/750633003/diff/20001/content/public/common/pe...
content/public/common/permission_status.mojom:5: module content;
nit: upload this CL with --no-find-copies, otherwise, it thinks that
permission_status.mojom is a copy of some other file.

[timvolodine, 2014-11-27 17:44:33]

Patchset #4 (id:60001) has been deleted

[timvolodine, 2014-11-27 17:53:42]

fixed comments, ptal!

looks like the ios bots are still failing,
qsr@, blundell@: any ideas why we exclude mojom on iOS?
 ['OS != "ios"', {
    'dependencies': [
      'content_common_mojo_bindings',
    ]
 }]

https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:601: return
content::PERMISSION_STATUS_GRANTED;
On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> nit: CONTENT_SETTING_SESSION_ONLY should also link to _GRANTED.

Done.

https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:607: return
content::PERMISSION_STATUS_ASK;
On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> I think it would be better to not have a |default|. Instead, you should have a
> case for _DEFAULT and _NUM_SETTINGS and call NOTREACHED() if you end up there.

Done.

https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2029:
result_callback.Run(ContentSettingToPermissionStatus(result));
On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> Why do you use a callback instead of returning directly the value? It seems
that
> the call is synchronous, right?

Done.

https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.h:200: virtual void HasPermission(
On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> nit: I think GetPermissionStatus() would be a better name here.

Done.

https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:123: : GURL(origin);
On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> I think it would be better to have a
> PermissionServiceContext::GetEmbeddingOrigin() instead of having the logic
here.

done. but we still need the |origin| so cannot move the logic completely to
context.

https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:138:
callback.Run(content_setting);
On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> nit: s/content_setting/permission_status/

Done.

https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.h (right):

https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.h:65: PermissionStatus
content_setting);
On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> nit: s/content_setting/permission_status/

Done.

https://codereview.chromium.org/750633003/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/750633003/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.cc:246:
result_callback.Run(PERMISSION_STATUS_ASK);
On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> nit: PERMISSION_STATUS_DENIED.

changed to denied, but, RequestPermission above does return "true" by default.
should this not return the default value (which would be ASK)?

https://codereview.chromium.org/750633003/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/750633003/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:458: virtual void HasPermission(
On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> nit: could you rename this GetPermissionStatus()?

Done.

https://codereview.chromium.org/750633003/diff/20001/content/public/common/pe...
File content/public/common/permission_status.mojom (right):

https://codereview.chromium.org/750633003/diff/20001/content/public/common/pe...
content/public/common/permission_status.mojom:5: module content;
On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> nit: upload this CL with --no-find-copies, otherwise, it thinks that
> permission_status.mojom is a copy of some other file.

Done.

[mlamouri (slow - plz ping), 2014-11-27 18:18:29]

lgtm w/ nits.

Thanks! :)

https://codereview.chromium.org/750633003/diff/80001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/750633003/diff/80001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.h:200: virtual
content::PermissionStatus GetPermissionStatus(
nit: remove "virtual", we no longer require "virtual" with "override". It's
actually one or the other now.

[timvolodine, 2014-11-27 18:34:01]

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

[timvolodine, 2014-11-27 18:34:01]

+tsepez@ : for mojom files

[qsr, 2014-11-28 08:45:08]

> looks like the ios bots are still failing,
> qsr@, blundell@: any ideas why we exclude mojom on iOS?
>  ['OS != "ios"', {
>     'dependencies': [
>       'content_common_mojo_bindings',
>     ]
>  }]

 I don't know. That seems kind of weird. I think colin had worked to have some
part of mojo works on iOS, so I expect mojom to be working. Does something break
if you remove this exclusion?

> 
>
https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
> chrome/browser/chrome_content_browser_client.cc:601: return
> content::PERMISSION_STATUS_GRANTED;
> On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> > nit: CONTENT_SETTING_SESSION_ONLY should also link to _GRANTED.
> 
> Done.
> 
>
https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
> chrome/browser/chrome_content_browser_client.cc:607: return
> content::PERMISSION_STATUS_ASK;
> On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> > I think it would be better to not have a |default|. Instead, you should have
a
> > case for _DEFAULT and _NUM_SETTINGS and call NOTREACHED() if you end up
there.
> 
> Done.
> 
>
https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
> chrome/browser/chrome_content_browser_client.cc:2029:
> result_callback.Run(ContentSettingToPermissionStatus(result));
> On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> > Why do you use a callback instead of returning directly the value? It seems
> that
> > the call is synchronous, right?
> 
> Done.
> 
>
https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
> File chrome/browser/chrome_content_browser_client.h (right):
> 
>
https://codereview.chromium.org/750633003/diff/20001/chrome/browser/chrome_co...
> chrome/browser/chrome_content_browser_client.h:200: virtual void
HasPermission(
> On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> > nit: I think GetPermissionStatus() would be a better name here.
> 
> Done.
> 
>
https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
> File content/browser/permissions/permission_service_impl.cc (right):
> 
>
https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
> content/browser/permissions/permission_service_impl.cc:123: : GURL(origin);
> On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> > I think it would be better to have a
> > PermissionServiceContext::GetEmbeddingOrigin() instead of having the logic
> here.
> 
> done. but we still need the |origin| so cannot move the logic completely to
> context.
> 
>
https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
> content/browser/permissions/permission_service_impl.cc:138:
> callback.Run(content_setting);
> On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> > nit: s/content_setting/permission_status/
> 
> Done.
> 
>
https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
> File content/browser/permissions/permission_service_impl.h (right):
> 
>
https://codereview.chromium.org/750633003/diff/20001/content/browser/permissi...
> content/browser/permissions/permission_service_impl.h:65: PermissionStatus
> content_setting);
> On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> > nit: s/content_setting/permission_status/
> 
> Done.
> 
>
https://codereview.chromium.org/750633003/diff/20001/content/public/browser/c...
> File content/public/browser/content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/750633003/diff/20001/content/public/browser/c...
> content/public/browser/content_browser_client.cc:246:
> result_callback.Run(PERMISSION_STATUS_ASK);
> On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> > nit: PERMISSION_STATUS_DENIED.
> 
> changed to denied, but, RequestPermission above does return "true" by default.
> should this not return the default value (which would be ASK)?
> 
>
https://codereview.chromium.org/750633003/diff/20001/content/public/browser/c...
> File content/public/browser/content_browser_client.h (right):
> 
>
https://codereview.chromium.org/750633003/diff/20001/content/public/browser/c...
> content/public/browser/content_browser_client.h:458: virtual void
HasPermission(
> On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> > nit: could you rename this GetPermissionStatus()?
> 
> Done.
> 
>
https://codereview.chromium.org/750633003/diff/20001/content/public/common/pe...
> File content/public/common/permission_status.mojom (right):
> 
>
https://codereview.chromium.org/750633003/diff/20001/content/public/common/pe...
> content/public/common/permission_status.mojom:5: module content;
> On 2014/11/24 19:06:01, Mounir Lamouri wrote:
> > nit: upload this CL with --no-find-copies, otherwise, it thinks that
> > permission_status.mojom is a copy of some other file.
> 
> Done.

[timvolodine, 2014-11-28 14:49:19]

https://codereview.chromium.org/750633003/diff/80001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/750633003/diff/80001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.h:200: virtual
content::PermissionStatus GetPermissionStatus(
On 2014/11/27 18:18:29, Mounir Lamouri wrote:
> nit: remove "virtual", we no longer require "virtual" with "override". It's
> actually one or the other now.

Done.

[timvolodine, 2014-11-28 14:50:05]

ok I've tweaked content.gyp a bit, but get the same issue on iOS bots:
../content/public/browser/content_browser_client.h:23:10: fatal error:
'content/public/common/permission_status.mojom.h' file not found

interestingly this issue appears to be flaky on linux_chromium_gn_rel and
win_gpu, mac_gpu bots -- i.e. if I retry them a second time the problem goes
away. Can this have to do with the order in which mojom.h files are generated?

[qsr, 2014-11-28 15:09:54]

On 2014/11/28 14:50:05, timvolodine wrote:
> ok I've tweaked content.gyp a bit, but get the same issue on iOS bots:
> ../content/public/browser/content_browser_client.h:23:10: fatal error:
> 'content/public/common/permission_status.mojom.h' file not found
> 
> interestingly this issue appears to be flaky on linux_chromium_gn_rel and
> win_gpu, mac_gpu bots -- i.e. if I retry them a second time the problem goes
> away. Can this have to do with the order in which mojom.h files are generated?

 Hum, usually this means that some dependency is missing.

[Tom Sepez, 2014-12-01 18:42:38]

Messages LGTM

https://codereview.chromium.org/750633003/diff/140001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/750633003/diff/140001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:597: content::PermissionStatus
nit: The style guide would say to put the function name on the same line as the
return type, and put the newline after the "("

https://codereview.chromium.org/750633003/diff/140001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2030: ?
context->GetPermissionStatus(requesting_origin.GetOrigin(),
nit: ? and : prior to line break.

https://codereview.chromium.org/750633003/diff/140001/content/browser/permiss...
File content/browser/permissions/permission_service_context.cc (right):

https://codereview.chromium.org/750633003/diff/140001/content/browser/permiss...
content/browser/permissions/permission_service_context.cc:82: : GURL();
nit: ":" on previous line.

[blundell, 2014-12-02 09:14:08]

For the iOS issue, I think you need to move the mojo dependencies in
content_app.gypi out of the !iOS section so that the right include dirs are
supplied on iOS.

[timvolodine, 2014-12-02 14:35:42]

On 2014/12/02 09:14:08, blundell wrote:
> For the iOS issue, I think you need to move the mojo dependencies in
> content_app.gypi out of the !iOS section so that the right include dirs are
> supplied on iOS.

blundell@: I've tried your suggestion regarding content_app.gypi but it looks
like it still cannot find the include.. any other ideas?

[blundell, 2014-12-02 14:44:30]

On 2014/12/02 14:35:42, timvolodine wrote:
> On 2014/12/02 09:14:08, blundell wrote:
> > For the iOS issue, I think you need to move the mojo dependencies in
> > content_app.gypi out of the !iOS section so that the right include dirs are
> > supplied on iOS.
> 
> blundell@: I've tried your suggestion regarding content_app.gypi but it looks
> like it still cannot find the include.. any other ideas?

It looks like the test_support_content target needs the same dependencies. Note
that that target doesn't have those dependencies even when not on iOS...it must
be getting saved by some transitive dependencies in the !iOS list.

[timvolodine, 2014-12-02 18:18:14]

On 2014/12/02 14:44:30, blundell wrote:
> On 2014/12/02 14:35:42, timvolodine wrote:
> > On 2014/12/02 09:14:08, blundell wrote:
> > > For the iOS issue, I think you need to move the mojo dependencies in
> > > content_app.gypi out of the !iOS section so that the right include dirs
are
> > > supplied on iOS.
> > 
> > blundell@: I've tried your suggestion regarding content_app.gypi but it
looks
> > like it still cannot find the include.. any other ideas?
> 
> It looks like the test_support_content target needs the same dependencies.
Note
> that that target doesn't have those dependencies even when not on iOS...it
must
> be getting saved by some transitive dependencies in the !iOS list.

I've tried including the dependencies on two targets:
- '../mojo/edk/mojo_edk.gyp:mojo_system_impl',
- '../mojo/mojo_base.gyp:mojo_environment_chromium',

in a couple of gyp files (including content_tests.gypi) I doesn't seem like it
makes any difference though. I've tried changing more includes but felt like I
was diverging.. also I don't have an iOS environment so any changes go via
trybots which is slow..
- can it have to do with export_dependent_settings?
- is there a simple way to force mojo header file generation in a gyp?
(e.g. including the above two targets and a dependency doesn't seem to work, but
I am probably missing something)

[mlamouri (slow - plz ping), 2014-12-02 22:51:43]

Colin and Benjamin, do you think someone from the iOS team in Paris can have a
look at that? Might be easier than Tim trying things and sending them to the try
server?

[blundell, 2014-12-03 11:21:22]

On 2014/12/02 22:51:43, Mounir Lamouri wrote:
> Colin and Benjamin, do you think someone from the iOS team in Paris can have a
> look at that? Might be easier than Tim trying things and sending them to the
try
> server?

I can dust off my Mac and look into it this afternoon :).

[timvolodine, 2014-12-03 12:14:16]

On 2014/12/03 11:21:22, blundell wrote:
> On 2014/12/02 22:51:43, Mounir Lamouri wrote:
> > Colin and Benjamin, do you think someone from the iOS team in Paris can have
a
> > look at that? Might be easier than Tim trying things and sending them to the
> try
> > server?
> 
> I can dust off my Mac and look into it this afternoon :).

thanks Colin! would appreciate that greatly :)

[blundell, 2014-12-03 13:56:20]

I've repro'd the build problem, looking into it now.

[blundell, 2014-12-03 15:43:46]

Hi Tim,

The build fails flakily, indicating that there's a missing hard_dependency
somewhere along the chain from content_common_mojo_bindings (generating
permission_status.mojom.h) to the targets that are failing to find the generated
header. I pointed out where I think the dependency needs to be propagated based
on the fact that you've added an include of that file in
content_browser_client.h. I don't think this is an iOS-specific problem
actually; probably the xcode build with iOS just happens to trigger the bad
ordering more reliably than the ninja builds on other platforms.

https://codereview.chromium.org/750633003/diff/300001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/750633003/diff/300001/content/content.gyp#new...
content/content.gyp:165: 'content_common',
You should export_dependent_settings on content_common_mojo_bindings here.

https://codereview.chromium.org/750633003/diff/300001/content/content.gyp#new...
content/content.gyp:311: 'content_common_mojo_bindings',
and here.

[timvolodine, 2014-12-03 19:23:34]

https://codereview.chromium.org/750633003/diff/300001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/750633003/diff/300001/content/content.gyp#new...
content/content.gyp:165: 'content_common',
On 2014/12/03 15:43:45, blundell wrote:
> You should export_dependent_settings on content_common_mojo_bindings here.

Done.

https://codereview.chromium.org/750633003/diff/300001/content/content.gyp#new...
content/content.gyp:311: 'content_common_mojo_bindings',
On 2014/12/03 15:43:46, blundell wrote:
> and here.

Done.

[timvolodine, 2014-12-03 19:26:20]

On 2014/12/03 15:43:46, blundell wrote:
> Hi Tim,
> 
> The build fails flakily, indicating that there's a missing hard_dependency
> somewhere along the chain from content_common_mojo_bindings (generating
> permission_status.mojom.h) to the targets that are failing to find the
generated
> header. I pointed out where I think the dependency needs to be propagated
based
> on the fact that you've added an include of that file in
> content_browser_client.h. I don't think this is an iOS-specific problem
> actually; probably the xcode build with iOS just happens to trigger the bad
> ordering more reliably than the ninja builds on other platforms.
> 

blundell@:

Hi Colin, thanks a lot for looking into this. The 'export_dependent_settings'
appear to resolve the issue :). However it leads to another issue where android
bots fail due to "multiple dex files for GeolocationService". I've removed the
dependency on content.gyp:content_common in content_java_test_support target in
content_tests.gypi (which I copied from your patch crbug.com/743093004). This
appears to cure android bots. Looking green now yeey :) (the
android_dbg_tests_recipe failure looks unrelated).

Also, instead of adding export_dependent_settings in multiple places I've
condensed them to one section in content_common target. It makes sense to me,
but PTAL to be sure.

[timvolodine, 2014-12-03 19:28:55]

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

[timvolodine, 2014-12-03 19:28:56]

+avi@ : as content/ owner

[blundell, 2014-12-04 12:21:44]

https://codereview.chromium.org/750633003/diff/400001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/750633003/diff/400001/content/content.gyp#new...
content/content.gyp:196: 'content_common_mojo_bindings',
This isn't technically correct, since no code under //content/common uses the
generated headers from this target (at all, let alone in public headers...I
wonder why the dependency is there in the first place, but obviously I wouldn't
try to investigate *that* in this CL).

[timvolodine, 2014-12-04 12:58:01]

https://codereview.chromium.org/750633003/diff/400001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/750633003/diff/400001/content/content.gyp#new...
content/content.gyp:196: 'content_common_mojo_bindings',
On 2014/12/04 12:21:44, blundell wrote:
> This isn't technically correct, since no code under //content/common uses the
> generated headers from this target (at all, let alone in public headers...I
> wonder why the dependency is there in the first place, but obviously I
wouldn't
> try to investigate *that* in this CL).

my rationale was that:
permission_service.mojom and geolocation_service.mojom which live in
content/common include permission_status.mojom and mojo_geoposition.mojom
respectively which live in content/public/common/.

but I can split it up again although it would look less structured.

[blundell, 2014-12-04 13:02:23]

On 2014/12/04 12:58:01, timvolodine wrote:
> https://codereview.chromium.org/750633003/diff/400001/content/content.gyp
> File content/content.gyp (right):
> 
>
https://codereview.chromium.org/750633003/diff/400001/content/content.gyp#new...
> content/content.gyp:196: 'content_common_mojo_bindings',
> On 2014/12/04 12:21:44, blundell wrote:
> > This isn't technically correct, since no code under //content/common uses
the
> > generated headers from this target (at all, let alone in public headers...I
> > wonder why the dependency is there in the first place, but obviously I
> wouldn't
> > try to investigate *that* in this CL).
> 
> my rationale was that:
> permission_service.mojom and geolocation_service.mojom which live in
> content/common include permission_status.mojom and mojo_geoposition.mojom
> respectively which live in content/public/common/.
> 
> but I can split it up again although it would look less structured.

That's not the way I would think about it. All of the mojoms live in the same
gyp target (content_common_mojo_bindings); the question is which other targets
need to depend (and possibly export_dependent_settings) on that target. The main
reason that I'm pushing back is that if //content/browser includes generated
mojoms from content_common_mojo_bindings in its public headers, then it really
should both directly depend on content_common_mojo_bindings and
export_dependent_settings on it; any other organization is fragile (e.g., what
if someone later figured out that content_browser didn't need to
export_dependent_settings on content_common?).

[timvolodine, 2014-12-04 14:51:50]

On 2014/12/04 13:02:23, blundell wrote:
> On 2014/12/04 12:58:01, timvolodine wrote:
> > https://codereview.chromium.org/750633003/diff/400001/content/content.gyp
> > File content/content.gyp (right):
> > 
> >
>
https://codereview.chromium.org/750633003/diff/400001/content/content.gyp#new...
> > content/content.gyp:196: 'content_common_mojo_bindings',
> > On 2014/12/04 12:21:44, blundell wrote:
> > > This isn't technically correct, since no code under //content/common uses
> the
> > > generated headers from this target (at all, let alone in public
headers...I
> > > wonder why the dependency is there in the first place, but obviously I
> > wouldn't
> > > try to investigate *that* in this CL).
> > 
> > my rationale was that:
> > permission_service.mojom and geolocation_service.mojom which live in
> > content/common include permission_status.mojom and mojo_geoposition.mojom
> > respectively which live in content/public/common/.
> > 
> > but I can split it up again although it would look less structured.
> 
> That's not the way I would think about it. All of the mojoms live in the same
> gyp target (content_common_mojo_bindings); the question is which other targets
> need to depend (and possibly export_dependent_settings) on that target. The
main
> reason that I'm pushing back is that if //content/browser includes generated
> mojoms from content_common_mojo_bindings in its public headers, then it really
> should both directly depend on content_common_mojo_bindings and
> export_dependent_settings on it; any other organization is fragile (e.g., what
> if someone later figured out that content_browser didn't need to
> export_dependent_settings on content_common?).

ok no problem ;) done.

[blundell, 2014-12-04 14:58:58]

those changes lgtm, but please wait for Chris' response on
https://codereview.chromium.org/743093004/ before landing the removal of
content_common from the dependencies of content_java_test_support; I still don't
know whether that's an OK thing to do :).

[timvolodine, 2014-12-05 12:23:27]

On 2014/12/04 14:58:58, blundell wrote:
> those changes lgtm, but please wait for Chris' response on
> https://codereview.chromium.org/743093004/ before landing the removal of
> content_common from the dependencies of content_java_test_support; I still
don't
> know whether that's an OK thing to do :).

alright this appears to be ok.

avi@ would you be able to review pls?

[Avi (use Gerrit), 2014-12-05 15:44:01]

I don't know anything about that Mojo stuff, but the content stuff lgtm.

[timvolodine, 2014-12-08 18:28:39]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-12-08 18:30:09]

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

[commit-bot: I haz the power, 2014-12-08 19:23:12]

Committed patchset #24 (id:480001)

[commit-bot: I haz the power, 2014-12-08 19:24:05]

Patchset 24 (id:??) landed as
https://crrev.com/277b23d74855b51ebe67ae0da7cd9e0b053044cf
Cr-Commit-Position: refs/heads/master@{#307302}

[Ryan Sleevi, 2014-12-08 19:40:13]

A revert of this CL (patchset #24 id:480001) has been created in
https://codereview.chromium.org/786063002/ by rsleevi@chromium.org.

The reason for reverting is: Causes Android Builder (dbg) to fail compile.

http://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28db...

FAILED: /b/build/goma/gomacc
/b/build/slave/Android_Builder__dbg_/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++
-MMD -MF obj/android_webview/native/webview_native.aw_contents_statics.o.d
-DV8_DEPRECATION_WARNINGS -D_FILE_OFFSET_BITS=64 -DNO_TCMALLOC -DDISABLE_NACL
-DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1
-DUSE_PROPRIETARY_CODECS -DENABLE_BROWSER_CDMS -DENABLE_CONFIGURATION_POLICY
-DENABLE_NOTIFICATIONS -DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DDONT_EMBED_BUILD_METADATA
-DENABLE_AUTOFILL_DIALOG=1 -DCLD_VERSION=1 -DENABLE_PRINTING=1
-DENABLE_BASIC_PRINTING=1 -DENABLE_SUPERVISED_USERS=1 -DVIDEO_HOLE=1
-DENABLE_LOAD_COMPLETION_HACKS=1 -DV8_USE_EXTERNAL_STARTUP_DATA
-DMOJO_USE_SYSTEM_IMPL -DPOSIX_AVOID_MMAP -DSK_ENABLE_INST_COUNT=0
-DSK_SUPPORT_GPU=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"'
-DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1
-DGR_GL_IGNORE_ES3_MSAA=0 -DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT
-DSK_SUPPORT_LEGACY_TEXTRENDERMODE -DSK_IGNORE_GPU_LAYER_HOISTING
-DSK_BUILD_FOR_ANDROID -DSK_USE_POSIX_THREADS -DU_USING_ICU_NAMESPACE=0
-DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DUSE_LIBPCI=1 -DUSE_OPENSSL=1
-DUSE_OPENSSL_CERTS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DANDROID
-D__GNU_SOURCE=1 -DUSE_STLPORT=1 -D_STLP_USE_PTR_SPECIALIZATIONS=1
'-DCHROME_BUILD_ID=""' -DHAVE_SYS_UIO_H -DDYNAMIC_ANNOTATIONS_ENABLED=1
-DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -Igen -I../.. -I../../skia/config
-I../../third_party/khronos -I../../gpu -I../../third_party/WebKit/Source
-Igen/angle -I../../third_party/WebKit -I../../third_party/skia/src/core
-I../../third_party/skia/include/core -I../../third_party/skia/include/effects
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports
-I../../third_party/skia/include/utils -I../../skia/ext
-I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
-I../../third_party/boringssl/src/include -Igen/android_webview
-fstack-protector --param=ssp-buffer-size=4 -Werror -fno-strict-aliasing -Wall
-Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe
-fPIC -Wno-unused-local-typedefs -march=armv7-a -mtune=generic-armv7-a
-mfpu=vfpv3-d16 -mfloat-abi=softfp -mthumb -fno-tree-sra -fno-caller-saves
-Wno-psabi -mthumb-interwork -ffunction-sections -funwind-tables -g
-fstack-protector -fno-short-enums -finline-limit=64 -Wa,--noexecstack
--sysroot=/b/build/slave/Android_Builder__dbg_/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm
-isystem/b/build/slave/Android_Builder__dbg_/build/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/stlport
-Os -g -gdwarf-4 -fdata-sections -ffunction-sections -fomit-frame-pointer
-funwind-tables -g1 -fno-exceptions -fno-rtti -fno-threadsafe-statics
-fvisibility-inlines-hidden -Wsign-compare -Wno-abi -std=gnu++11 -Wno-narrowing
-Wno-literal-suffix  -c ../../android_webview/native/aw_contents_statics.cc -o
obj/android_webview/native/webview_native.aw_contents_statics.o
In file included from ../../android_webview/browser/aw_browser_context.h:18:0,
                 from ../../android_webview/native/aw_contents_statics.cc:7:
../../content/public/browser/content_browser_client.h:23:59: fatalerror:
content/public/common/permission_status.mojom.h: No such file or directory
 #include "content/public/common/permission_status.mojom.h".

[timvolodine, 2014-12-09 15:50:25]

Patchset #28 (id:560001) has been deleted

[timvolodine, 2014-12-09 17:17:44]

On 2014/12/08 19:40:13, Ryan Sleevi wrote:
> A revert of this CL (patchset #24 id:480001) has been created in
> https://codereview.chromium.org/786063002/ by mailto:rsleevi@chromium.org.
> [...]

relanding with a fix..

[timvolodine, 2014-12-09 17:17:51]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-12-09 17:18:57]

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

[commit-bot: I haz the power, 2014-12-09 18:18:52]

Committed patchset #28 (id:580001)

[commit-bot: I haz the power, 2014-12-09 18:19:40]

Patchset 28 (id:??) landed as
https://crrev.com/14570267ac4d5dc473d29f256e8d044e9bfcc8d2
Cr-Commit-Position: refs/heads/master@{#307504}

[Nico, 2014-12-10 21:27:06]

A revert of this CL (patchset #28 id:580001) has been created in
https://codereview.chromium.org/792173002/ by thakis@chromium.org.

The reason for reverting is: This still causes build failures, I just got this
locally:

ninja -C out/Release/ chrome
ninja: Entering directory `out/Release/'
[46/16655] CXX obj/content/public/app/content_app_both.content_main_delegate.o
FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/content/public/app/content_app_both.content_main_delegate.o.d
-DCONTENT_IMPLEMENTATION -DV8_DEPRECATION_WARNINGS
-D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DDISABLE_NACL
-DCHROMIUM_BUILD -DCR_CLANG_REVISION=218707 -DUSE_LIBJPEG_TURBO=1
-DENABLE_ONE_CLICK_SIGNIN -DENABLE_PRE_SYNC_BACKUP -DENABLE_REMOTING=1
-DENABLE_WEBRTC=1 -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY
-DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1
-DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DDONT_EMBED_BUILD_METADATA
-DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1
-DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
-DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1
-DCLD_VERSION=2 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1
-DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1
-DENABLE_WIFI_BOOTSTRAPPING=1 -DENABLE_LOAD_COMPLETION_HACKS=1
-DV8_USE_EXTERNAL_STARTUP_DATA -DMOJO_USE_SYSTEM_IMPL -DSK_ENABLE_INST_COUNT=0
-DSK_SUPPORT_GPU=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"'
-DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1
-DGR_GL_IGNORE_ES3_MSAA=0 -DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT
-DSK_SUPPORT_LEGACY_TEXTRENDERMODE -DSK_IGNORE_GPU_LAYER_HOISTING
-DSK_SUPPORT_LEGACY_NewRasterPMColor -DSK_USE_POSIX_THREADS
-DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION
-DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
-DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -Igen
-I../.. -I../../third_party/khronos -I../../gpu -I../../skia/config
-I../../third_party/WebKit/Source -I../../third_party/WebKit
-I../../third_party/skia/src/core -I../../third_party/skia/include/core
-I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf
-I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy
-I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe
-I../../third_party/skia/include/ports -I../../third_party/skia/include/utils
-I../../third_party/skia/include/utils/mac -I../../skia/ext
-I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
-isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk
-O2 -fvisibility=hidden -Werror -Wnewline-eof -mmacosx-version-min=10.6 -arch
x86_64 -Wall -Wendif-labels -Wextra -Wno-unused-parameter
-Wno-missing-field-initializers -Wno-selector-type-mismatch -Wheader-hygiene
-Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-deprecated-register -Wexit-time-destructors -std=c++11 -fno-rtti
-fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics -Xclang
-load -Xclang
/Volumes/MacintoshHD2/src/chromefetch/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics
-fno-strict-aliasing  -c ../../content/public/app/content_main_delegate.cc -o
obj/content/public/app/content_app_both.content_main_delegate.o
In file included from ../../content/public/app/content_main_delegate.cc:8:
../../content/public/browser/content_browser_client.h:22:10: fatal error:
'content/public/common/permission_status.mojom.h' file not found
#include "content/public/common/permission_status.mojom.h"
         ^
1 error generated.
[46/16655] CXX obj/content/app/content_app_both.content_main_runner.o
FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/content/app/content_app_both.content_main_runner.o.d
-DCONTENT_IMPLEMENTATION -DV8_DEPRECATION_WARNINGS
-D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DDISABLE_NACL
-DCHROMIUM_BUILD -DCR_CLANG_REVISION=218707 -DUSE_LIBJPEG_TURBO=1
-DENABLE_ONE_CLICK_SIGNIN -DENABLE_PRE_SYNC_BACKUP -DENABLE_REMOTING=1
-DENABLE_WEBRTC=1 -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY
-DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1
-DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DDONT_EMBED_BUILD_METADATA
-DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1
-DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
-DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1
-DCLD_VERSION=2 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1
-DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1
-DENABLE_WIFI_BOOTSTRAPPING=1 -DENABLE_LOAD_COMPLETION_HACKS=1
-DV8_USE_EXTERNAL_STARTUP_DATA -DMOJO_USE_SYSTEM_IMPL -DSK_ENABLE_INST_COUNT=0
-DSK_SUPPORT_GPU=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"'
-DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1
-DGR_GL_IGNORE_ES3_MSAA=0 -DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT
-DSK_SUPPORT_LEGACY_TEXTRENDERMODE -DSK_IGNORE_GPU_LAYER_HOISTING
-DSK_SUPPORT_LEGACY_NewRasterPMColor -DSK_USE_POSIX_THREADS
-DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION
-DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
-DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -Igen
-I../.. -I../../third_party/khronos -I../../gpu -I../../skia/config
-I../../third_party/WebKit/Source -I../../third_party/WebKit
-I../../third_party/skia/src/core -I../../third_party/skia/include/core
-I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf
-I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy
-I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe
-I../../third_party/skia/include/ports -I../../third_party/skia/include/utils
-I../../third_party/skia/include/utils/mac -I../../skia/ext
-I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
-isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk
-O2 -fvisibility=hidden -Werror -Wnewline-eof -mmacosx-version-min=10.6 -arch
x86_64 -Wall -Wendif-labels -Wextra -Wno-unused-parameter
-Wno-missing-field-initializers -Wno-selector-type-mismatch -Wheader-hygiene
-Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-deprecated-register -Wexit-time-destructors -std=c++11 -fno-rtti
-fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics -Xclang
-load -Xclang
/Volumes/MacintoshHD2/src/chromefetch/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -fcolor-diagnostics
-fno-strict-aliasing  -c ../../content/app/content_main_runner.cc -o
obj/content/app/content_app_both.content_main_runner.o
In file included from ../../content/app/content_main_runner.cc:36:
../../content/public/browser/content_browser_client.h:22:10: fatal error:
'content/public/common/permission_status.mojom.h' file not found
#include "content/public/common/permission_status.mojom.h"
         ^
1 error generated..

[timvolodine, 2014-12-11 18:16:58]

On 2014/12/10 21:27:06, Nico wrote:
> A revert of this CL (patchset #28 id:580001) has been created in
> https://codereview.chromium.org/792173002/ by mailto:thakis@chromium.org.
> 
> The reason for reverting is: This still causes build failures, I just got this
> locally:
> 
> ninja -C out/Release/ chrome
> ninja: Entering directory `out/Release/'
> [46/16655] CXX obj/content/public/app/content_app_both.content_main_delegate.o
> FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
> obj/content/public/app/content_app_both.content_main_delegate.o.d
> [..]
> -fno-strict-aliasing  -c ../../content/app/content_main_runner.cc -o
> obj/content/app/content_app_both.content_main_runner.o
> In file included from ../../content/app/content_main_runner.cc:36:
> ../../content/public/browser/content_browser_client.h:22:10: fatal error:
> 'content/public/common/permission_status.mojom.h' file not found
> #include "content/public/common/permission_status.mojom.h"
>          ^
> 1 error generated..

ok apparently that happens only in static configuration. I've added some more
dependencies to to content_app, content_child and content_ppapi_plugin and
verified on most targets (on linux), relanding...

[timvolodine, 2014-12-11 18:17:09]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-12-11 18:18:23]

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

[commit-bot: I haz the power, 2014-12-11 19:32:34]

Committed patchset #29 (id:600001)

[commit-bot: I haz the power, 2014-12-11 19:33:17]

Patchset 29 (id:??) landed as
https://crrev.com/089c07c966a9dc169f0822b90d92400bf23e19f7
Cr-Commit-Position: refs/heads/master@{#307948}

[xhwang, 2015-01-21 06:52:22]

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

[xhwang, 2015-01-21 06:52:24]

https://codereview.chromium.org/750633003/diff/600001/content/public/common/p...
File content/public/common/permission_status.mojom (right):

https://codereview.chromium.org/750633003/diff/600001/content/public/common/p...
content/public/common/permission_status.mojom:10: ASK
What does "ASK" exactly mean here? If a permission request triggers an infobar
for user's action. Will the requestpermission() call wait for user's decision,
or will we get "ASK" as the permission status immediately?

[mlamouri (slow - plz ping), 2015-01-21 10:00:48]

On 2015/01/21 at 06:52:24, xhwang wrote:
>
https://codereview.chromium.org/750633003/diff/600001/content/public/common/p...
> File content/public/common/permission_status.mojom (right):
> 
>
https://codereview.chromium.org/750633003/diff/600001/content/public/common/p...
> content/public/common/permission_status.mojom:10: ASK
> What does "ASK" exactly mean here? If a permission request triggers an infobar
for user's action. Will the requestpermission() call wait for user's decision,
or will we get "ASK" as the permission status immediately?

"Ask" is meant to be used for HasPermission(), not RequestPermission(). If the
permission request is meant to show a prompt, the HasPermission() call with
return "Ask". RequestPermission() should only return "Ask" if it did not get an
answer from the user.

[xhwang, 2015-01-21 18:49:59]

On 2015/01/21 10:00:48, Mounir Lamouri wrote:
> On 2015/01/21 at 06:52:24, xhwang wrote:
> >
>
https://codereview.chromium.org/750633003/diff/600001/content/public/common/p...
> > File content/public/common/permission_status.mojom (right):
> > 
> >
>
https://codereview.chromium.org/750633003/diff/600001/content/public/common/p...
> > content/public/common/permission_status.mojom:10: ASK
> > What does "ASK" exactly mean here? If a permission request triggers an
infobar
> for user's action. Will the requestpermission() call wait for user's decision,
> or will we get "ASK" as the permission status immediately?
> 
> "Ask" is meant to be used for HasPermission(), not RequestPermission(). If the
> permission request is meant to show a prompt, the HasPermission() call with
> return "Ask". RequestPermission() should only return "Ask" if it did not get
an
> answer from the user.

Thanks for the clarification! Since this isn't obvious, some documentation in
the mojom file would be really helpful.

For "RequestPermission() should only return "Ask" if it did not get an answer
from the user", when will this happen? For example, will there be a timeout on
how long to wait for user's action?

Also, we don't have a cancelPermissionRequest call in the mojom interface.
Current GeolocationDispatcher doesn't do much in cancelPermissionRequest()
either
(https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/g...).
If there's an infobar already shown 
for user action, I guess GeolocationDispatcher::cancelPermissionRequest() won't
cancel that info bar (maybe I am missing something here). Is this expected?

[xhwang, 2015-01-21 18:53:25]

On 2015/01/21 18:49:59, xhwang wrote:
> On 2015/01/21 10:00:48, Mounir Lamouri wrote:
> > On 2015/01/21 at 06:52:24, xhwang wrote:
> > >
> >
>
https://codereview.chromium.org/750633003/diff/600001/content/public/common/p...
> > > File content/public/common/permission_status.mojom (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/750633003/diff/600001/content/public/common/p...
> > > content/public/common/permission_status.mojom:10: ASK
> > > What does "ASK" exactly mean here? If a permission request triggers an
> infobar
> > for user's action. Will the requestpermission() call wait for user's
decision,
> > or will we get "ASK" as the permission status immediately?
> > 
> > "Ask" is meant to be used for HasPermission(), not RequestPermission(). If
the
> > permission request is meant to show a prompt, the HasPermission() call with
> > return "Ask". RequestPermission() should only return "Ask" if it did not get
> an
> > answer from the user.
> 
> Thanks for the clarification! Since this isn't obvious, some documentation in
> the mojom file would be really helpful.
> 
> For "RequestPermission() should only return "Ask" if it did not get an answer
> from the user", when will this happen? For example, will there be a timeout on
> how long to wait for user's action?
> 
> Also, we don't have a cancelPermissionRequest call in the mojom interface.
> Current GeolocationDispatcher doesn't do much in cancelPermissionRequest()
> either
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/g...).
> If there's an infobar already shown 
> for user action, I guess GeolocationDispatcher::cancelPermissionRequest()
won't
> cancel that info bar (maybe I am missing something here). Is this expected?

BTW, what does |user_gesture| exactly mean in RequestPermission? Thanks!

[jam, 2015-02-02 17:44:51]

this is still causing compile failures, i.e.
http://build.chromium.org/p/chromium/builders/Mac/builds/33023/steps/compile/...

[timvolodine, 2015-02-02 17:53:53]

On 2015/02/02 17:44:51, jam wrote:
> this is still causing compile failures, i.e.
>
http://build.chromium.org/p/chromium/builders/Mac/builds/33023/steps/compile/...

hmm apparently the mac build is flaky, filed crbug.com/454447 will investigate.