Fix running gn gen for //ui/* targets on iOS.

Fix running gn gen for //ui/* targets on iOS.

This CL only fixes the errors encountered when running "gn gen" with
target_os set to ios. It does not yet ensure that the targets compile
nor that the unit tests pass.

iOS does not build //third_party/WebKit, so remove those dependencies
from the BUILD.gn files that are transitively used.

Fix some "sources" exclusion rules that removed that that were only
added when "is_ios" was false, thus causing an error while running
"gn gen".

BUG=459705
Committed: https://crrev.com/6630e6f74714bf7d82f3c1cc207604307cd32a53
Cr-Commit-Position: refs/heads/master@{#340432}

[sdefresne, 2015-07-22 16:54:01]

sdefresne@chromium.org changed reviewers:
+ danakj@chromium.org, sadrul@chromium.org

[sdefresne, 2015-07-22 16:54:01]

PTAL

[Dirk Pranke, 2015-07-22 16:56:33]

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

[Dirk Pranke, 2015-07-22 16:56:33]

lgtm

[danakj, 2015-07-22 18:25:28]

https://codereview.chromium.org/1247593008/diff/1/ui/compositor/BUILD.gn
File ui/compositor/BUILD.gn (left):

https://codereview.chromium.org/1247593008/diff/1/ui/compositor/BUILD.gn#oldc...
ui/compositor/BUILD.gn:137: "//third_party/WebKit/public:blink_minimal",
This looks historical. Do we actually need this still?

[danakj, 2015-07-22 18:26:23]

https://codereview.chromium.org/1247593008/diff/1/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/1247593008/diff/1/ui/gfx/BUILD.gn#newcode518
ui/gfx/BUILD.gn:518: if (!is_android && !is_ios) {
This looks the same as before? Why are you changing it?

[Dirk Pranke, 2015-07-22 18:49:36]

https://codereview.chromium.org/1247593008/diff/1/ui/compositor/BUILD.gn
File ui/compositor/BUILD.gn (left):

https://codereview.chromium.org/1247593008/diff/1/ui/compositor/BUILD.gn#oldc...
ui/compositor/BUILD.gn:137: "//third_party/WebKit/public:blink_minimal",
On 2015/07/22 18:25:27, danakj wrote:
> This looks historical. Do we actually need this still?

You're suggesting maybe we never need blink headers in the compositor, even on
desktop platforms?

https://codereview.chromium.org/1247593008/diff/1/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/1247593008/diff/1/ui/gfx/BUILD.gn#newcode518
ui/gfx/BUILD.gn:518: if (!is_android && !is_ios) {
On 2015/07/22 18:26:23, danakj wrote:
> This looks the same as before? Why are you changing it?

I think you're right that this is the same as before, logically. 

In GN code we've moved more to an additive style where 
we try to only list sources in += blocks, especially when we
can't count on sources_assignment_filter to strip them out.

However, such a change isn't required and we're not going
back through all of the code necessarily to rewrite things.

So I think this change is a win but only a minor one.

[danakj, 2015-07-22 19:03:14]

https://codereview.chromium.org/1247593008/diff/1/ui/compositor/BUILD.gn
File ui/compositor/BUILD.gn (left):

https://codereview.chromium.org/1247593008/diff/1/ui/compositor/BUILD.gn#oldc...
ui/compositor/BUILD.gn:137: "//third_party/WebKit/public:blink_minimal",
On 2015/07/22 18:49:36, Dirk Pranke wrote:
> On 2015/07/22 18:25:27, danakj wrote:
> > This looks historical. Do we actually need this still?
> 
> You're suggesting maybe we never need blink headers in the compositor, even on
> desktop platforms?

I don't see any includes of third_party/WebKit in ui/compositor, nor is it
allowed in DEPS anymore.

[Dirk Pranke, 2015-07-22 19:07:01]

On 2015/07/22 19:03:14, danakj wrote:
> On 2015/07/22 18:49:36, Dirk Pranke wrote:
> > On 2015/07/22 18:25:27, danakj wrote:
> > > This looks historical. Do we actually need this still?
> > 
> > You're suggesting maybe we never need blink headers in the compositor, even
on
> > desktop platforms?
> 
> I don't see any includes of third_party/WebKit in ui/compositor, nor is it
> allowed in DEPS anymore.

We should delete it unconditionally, then. I was a bit surprised by the
dependency but didn't follow up on it.

[sdefresne, 2015-07-23 07:38:03]

PTAL

https://codereview.chromium.org/1247593008/diff/1/ui/compositor/BUILD.gn
File ui/compositor/BUILD.gn (left):

https://codereview.chromium.org/1247593008/diff/1/ui/compositor/BUILD.gn#oldc...
ui/compositor/BUILD.gn:137: "//third_party/WebKit/public:blink_minimal",
On 2015/07/22 at 19:03:14, danakj wrote:
> On 2015/07/22 18:49:36, Dirk Pranke wrote:
> > On 2015/07/22 18:25:27, danakj wrote:
> > > This looks historical. Do we actually need this still?
> > 
> > You're suggesting maybe we never need blink headers in the compositor, even
on
> > desktop platforms?
> 
> I don't see any includes of third_party/WebKit in ui/compositor, nor is it
allowed in DEPS anymore.

Done.

https://codereview.chromium.org/1247593008/diff/1/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/1247593008/diff/1/ui/gfx/BUILD.gn#newcode518
ui/gfx/BUILD.gn:518: if (!is_android && !is_ios) {
On 2015/07/22 at 18:49:36, Dirk Pranke wrote:
> On 2015/07/22 18:26:23, danakj wrote:
> > This looks the same as before? Why are you changing it?
> 
> I think you're right that this is the same as before, logically. 
> 
> In GN code we've moved more to an additive style where 
> we try to only list sources in += blocks, especially when we
> can't count on sources_assignment_filter to strip them out.
> 
> However, such a change isn't required and we're not going
> back through all of the code necessarily to rewrite things.
> 
> So I think this change is a win but only a minor one.


It is not identical, the previous code was (summarized):

if (!is_ios) {
  sources + = [ "render_text_unittest.cc" ]
}

if (is_android || is_ios) {
  sources -= [ "render_text_unittest.cc" ]
}

The second conditional was causing failures on iOS since it tried to remove a
file that was not in the sources list. My change uses the additive style.

[Dirk Pranke, 2015-07-23 15:10:05]

lgtm.

[sadrul, 2015-07-23 15:29:10]

Can you clarify in the CL title that 'Fix //ui/* targets on iOS.' means fixing
'gn gen', and not fixing building+running the targets?

https://codereview.chromium.org/1247593008/diff/20001/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/1247593008/diff/20001/ui/base/BUILD.gn#newcod...
ui/base/BUILD.gn:778: }
This doesn't look right. |is_win| implies |use_aura|. Can you try just 'if
(use_aura)' here instead (since we use view_prop on all aura platforms)?

https://codereview.chromium.org/1247593008/diff/20001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/1247593008/diff/20001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:317: "gestures/blink/web_gesture_curve_impl_unittest.cc",
Do you not need to exclude this file on ios?

[danakj, 2015-07-23 19:07:30]

ui/compositor ui/gfx LGTM

[sdefresne, 2015-07-25 18:04:31]

sadrul: PTAL

https://codereview.chromium.org/1247593008/diff/20001/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/1247593008/diff/20001/ui/base/BUILD.gn#newcod...
ui/base/BUILD.gn:778: }
On 2015/07/23 at 15:29:10, sadrul wrote:
> This doesn't look right. |is_win| implies |use_aura|. Can you try just 'if
(use_aura)' here instead (since we use view_prop on all aura platforms)?

Sure, Done. In the previous patch just flipped the old conditional when moving
from substractive to additive pattern without checking whether the condition
made sense or not, thank you for noticing this old conditional.

https://codereview.chromium.org/1247593008/diff/20001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/1247593008/diff/20001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:317: "gestures/blink/web_gesture_curve_impl_unittest.cc",
On 2015/07/23 at 15:29:10, sadrul wrote:
> Do you not need to exclude this file on ios?

Good catch, thank you.

[sadrul, 2015-07-25 20:33:09]

lgtm

https://codereview.chromium.org/1247593008/diff/40001/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/1247593008/diff/40001/ui/base/BUILD.gn#newcod...
ui/base/BUILD.gn:776: if (is_win) {
This should be use_aura (since we use view_prop on all aura platforms). I
understand that this test is currently run only on Windows, but it would be nice
to fix this to run on all aura platforms while you are here.

[sdefresne, 2015-07-26 08:41:25]

On 2015/07/25 at 20:33:09, sadrul wrote:
> lgtm
> 
> https://codereview.chromium.org/1247593008/diff/40001/ui/base/BUILD.gn
> File ui/base/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1247593008/diff/40001/ui/base/BUILD.gn#newcod...
> ui/base/BUILD.gn:776: if (is_win) {
> This should be use_aura (since we use view_prop on all aura platforms). I
understand that this test is currently run only on Windows, but it would be nice
to fix this to run on all aura platforms while you are here.

Thank you for the review.

I've tried enabling view_prop_unittest.cc on all aura platform, but it does not
compile on Linux. I thus think this should be done in a followup CL (as can be
seen in patchset #4). For the record, the error is:

../../ui/base/view_prop_unittest.cc:19:32: error: reinterpret_cast from 'int' to
'gfx::AcceleratedWidget' (aka 'unsigned long') is not allowed
  gfx::AcceleratedWidget nv1 = reinterpret_cast<gfx::AcceleratedWidget>(1);
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[sadrul, 2015-07-26 14:08:28]

On 2015/07/26 08:41:25, sdefresne wrote:
> On 2015/07/25 at 20:33:09, sadrul wrote:
> > lgtm
> > 
> > https://codereview.chromium.org/1247593008/diff/40001/ui/base/BUILD.gn
> > File ui/base/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1247593008/diff/40001/ui/base/BUILD.gn#newcod...
> > ui/base/BUILD.gn:776: if (is_win) {
> > This should be use_aura (since we use view_prop on all aura platforms). I
> understand that this test is currently run only on Windows, but it would be
nice
> to fix this to run on all aura platforms while you are here.
> 
> Thank you for the review.
> 
> I've tried enabling view_prop_unittest.cc on all aura platform, but it does
not
> compile on Linux. I thus think this should be done in a followup CL (as can be
> seen in patchset #4). For the record, the error is:
> 
> ../../ui/base/view_prop_unittest.cc:19:32: error: reinterpret_cast from 'int'
to
> 'gfx::AcceleratedWidget' (aka 'unsigned long') is not allowed
>   gfx::AcceleratedWidget nv1 = reinterpret_cast<gfx::AcceleratedWidget>(1);
>                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Sounds good. Thank you.

[sdefresne, 2015-07-27 07:07:30]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-07-27 07:07:30]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
dpranke@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1247593008/#ps80001
(title: "Revert back to only building view_prop_unittest.cc on Windows")

[commit-bot: I haz the power, 2015-07-27 07:07:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1247593008/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1247593008/80001

[commit-bot: I haz the power, 2015-07-27 07:12:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-27 07:12:39]

Patchset 5 (id:??) landed as
https://crrev.com/6630e6f74714bf7d82f3c1cc207604307cd32a53
Cr-Commit-Position: refs/heads/master@{#340432}