Fix use_canvas_skia = 0

Fix use_canvas_skia = 0

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285735

[zhaoze.zhou, 2014-07-16 23:23:10]

Please take a look.

[zhaoze.zhou, 2014-07-17 12:53:46]

thakis@chromium.org: Please take a look

[Nico, 2014-07-17 16:18:57]

Looks like asvitkine might know something about this.

[Alexei Svitkine (slow), 2014-07-17 16:36:45]

lgtm

[zhaoze.zhou, 2014-07-17 16:39:48]

The CQ bit was checked by zhaoze.zhou@partner.samsung.com

[commit-bot: I haz the power, 2014-07-17 16:41:01]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/zhaoze.zhou@partner.samsung.com/382633...

[Nico, 2014-07-17 16:46:06]

lgtm

[commit-bot: I haz the power, 2014-07-17 20:05:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-07-17 21:43:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-17 21:43:35]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[zhaoze.zhou, 2014-07-18 20:18:07]

On 2014/07/17 16:36:45, Alexei Svitkine wrote:
> lgtm

I have changed some stuff in the ui.gni because the gn test failed. Please take
a look. Thanks.

[Alexei Svitkine (slow), 2014-07-18 20:19:10]

lgtm

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

https://codereview.chromium.org/382633002/diff/200001/ui/gfx/BUILD.gn#newcode316
ui/gfx/BUILD.gn:316: if(use_canvas_skia){
Nit: add spaces after if and before {

[Nico, 2014-07-18 20:19:57]

still lgtm

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

https://codereview.chromium.org/382633002/diff/200001/ui/gfx/BUILD.gn#newcode316
ui/gfx/BUILD.gn:316: if(use_canvas_skia){
nit: add a space between if and (

https://codereview.chromium.org/382633002/diff/200001/ui/gfx/BUILD.gn#newcode317
ui/gfx/BUILD.gn:317: sources -= ["canvas_notimplemented.cc",]
nit: I think one-line lists usually don't have a trailing ,

[zhaoze.zhou, 2014-07-18 20:25:14]

The CQ bit was checked by zhaoze.zhou@partner.samsung.com

[commit-bot: I haz the power, 2014-07-18 20:26:25]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/zhaoze.zhou@partner.samsung.com/382633...

[zhaoze.zhou, 2014-07-18 20:35:39]

brettw@chromium.org: Please take a look.

[Nico, 2014-07-18 20:39:43]

On 2014/07/18 20:35:39, zhaoze.zhou wrote:
> mailto:brettw@chromium.org: Please take a look.

brettw: is ui.gni intentionally in build/config instead of somewhere below ui/?

[zhaoze.zhou, 2014-07-18 20:41:11]

On 2014/07/18 20:39:43, Nico (away) wrote:
> On 2014/07/18 20:35:39, zhaoze.zhou wrote:
> > mailto:brettw@chromium.org: Please take a look.
> 
> brettw: is ui.gni intentionally in build/config instead of somewhere below
ui/?

Yes, it's in build/config

[Nico, 2014-07-18 20:42:52]

On Fri, Jul 18, 2014 at 1:41 PM, <zhaoze.zhou@partner.samsung.com> wrote:

> On 2014/07/18 20:39:43, Nico (away) wrote:
>
>> On 2014/07/18 20:35:39, zhaoze.zhou wrote:
>> > mailto:brettw@chromium.org: Please take a look.
>>
>
>  brettw: is ui.gni intentionally in build/config instead of somewhere below
>>
> ui/?
>
> Yes, it's in build/config
>

Yes, that's what I wrote. I mean, is it there intentionally, or was it just
not moved to ui yet? Having this in build/config seems strange to me. But
maybe there's some good reason.


>
> https://codereview.chromium.org/382633002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-07-18 23:22:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-18 23:25:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 23:25:08]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[brettw, 2014-07-23 20:50:33]

https://codereview.chromium.org/382633002/diff/220001/build/config/ui.gni
File build/config/ui.gni (right):

https://codereview.chromium.org/382633002/diff/220001/build/config/ui.gni#new...
build/config/ui.gni:50: if(is_android){
use_canvas_skia is only ever used once in the entire GYP build. This should just
be coded there. Can that be converted to a check on the is_android flag and we
completely skip this flag?

[zhaoze.zhou, 2014-07-24 14:28:45]

https://codereview.chromium.org/382633002/diff/220001/build/config/ui.gni
File build/config/ui.gni (right):

https://codereview.chromium.org/382633002/diff/220001/build/config/ui.gni#new...
build/config/ui.gni:50: if(is_android){
On 2014/07/23 20:50:32, brettw wrote:
> use_canvas_skia is only ever used once in the entire GYP build. This should
just
> be coded there. Can that be converted to a check on the is_android flag and we
> completely skip this flag?

Hi, I am not clear what you mean. Do you mean I need to completely delete the
use_canvas_skia in the gypi file?

[brettw, 2014-07-24 16:35:23]

On 2014/07/24 14:28:45, zhaoze.zhou wrote:
> https://codereview.chromium.org/382633002/diff/220001/build/config/ui.gni
> File build/config/ui.gni (right):
> 
>
https://codereview.chromium.org/382633002/diff/220001/build/config/ui.gni#new...
> build/config/ui.gni:50: if(is_android){
> On 2014/07/23 20:50:32, brettw wrote:
> > use_canvas_skia is only ever used once in the entire GYP build. This should
> just
> > be coded there. Can that be converted to a check on the is_android flag and
we
> > completely skip this flag?
> 
> Hi, I am not clear what you mean. Do you mean I need to completely delete the
> use_canvas_skia in the gypi file?

I'm OK if you don't change the GYP build. But I don't think we should be adding
this global to the GN build. Instead, just code the android condition into the
one GN file that needs it.

[zhaoze.zhou, 2014-07-24 18:00:16]

On 2014/07/24 16:35:23, brettw wrote:
> On 2014/07/24 14:28:45, zhaoze.zhou wrote:
> > https://codereview.chromium.org/382633002/diff/220001/build/config/ui.gni
> > File build/config/ui.gni (right):
> > 
> >
>
https://codereview.chromium.org/382633002/diff/220001/build/config/ui.gni#new...
> > build/config/ui.gni:50: if(is_android){
> > On 2014/07/23 20:50:32, brettw wrote:
> > > use_canvas_skia is only ever used once in the entire GYP build. This
should
> > just
> > > be coded there. Can that be converted to a check on the is_android flag
and
> we
> > > completely skip this flag?
> > 
> > Hi, I am not clear what you mean. Do you mean I need to completely delete
the
> > use_canvas_skia in the gypi file?
> 
> I'm OK if you don't change the GYP build. But I don't think we should be
adding
> this global to the GN build. Instead, just code the android condition into the
> one GN file that needs it.

Hi, the intent of this patch is to fix the build problem, when disable
use_canvas_skia 
in other platforms except android. So, if we don't have use_canvas_skia flag in
the 
GN build, the GN build will enable use_canvas_skia by default. Thanks.

[zhaoze.zhou, 2014-07-24 22:46:37]

On 2014/07/24 18:00:16, zhaoze.zhou wrote:
> On 2014/07/24 16:35:23, brettw wrote:
> > On 2014/07/24 14:28:45, zhaoze.zhou wrote:
> > > https://codereview.chromium.org/382633002/diff/220001/build/config/ui.gni
> > > File build/config/ui.gni (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/382633002/diff/220001/build/config/ui.gni#new...
> > > build/config/ui.gni:50: if(is_android){
> > > On 2014/07/23 20:50:32, brettw wrote:
> > > > use_canvas_skia is only ever used once in the entire GYP build. This
> should
> > > just
> > > > be coded there. Can that be converted to a check on the is_android flag
> and
> > we
> > > > completely skip this flag?
> > > 
> > > Hi, I am not clear what you mean. Do you mean I need to completely delete
> the
> > > use_canvas_skia in the gypi file?
> > 
> > I'm OK if you don't change the GYP build. But I don't think we should be
> adding
> > this global to the GN build. Instead, just code the android condition into
the
> > one GN file that needs it.
> 
> Hi, the intent of this patch is to fix the build problem, when disable
> use_canvas_skia 
> in other platforms except android. So, if we don't have use_canvas_skia flag
in
> the 
> GN build, the GN build will enable use_canvas_skia by default. Thanks.

Do you think my modification of the BUILD.gn is OK if we do not add the
use_canvas_skia
flag in GN

[brettw, 2014-07-25 19:43:00]

lgtm

[zhaoze.zhou, 2014-07-25 19:43:46]

The CQ bit was checked by zhaoze.zhou@partner.samsung.com

[commit-bot: I haz the power, 2014-07-25 19:46:01]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/zhaoze.zhou@partner.samsung.com/382633...

[zhaoze.zhou, 2014-07-25 21:44:20]

The CQ bit was checked by zhaoze.zhou@partner.samsung.com

[zhaoze.zhou, 2014-07-25 21:44:22]

The CQ bit was unchecked by zhaoze.zhou@partner.samsung.com

[zhaoze.zhou, 2014-07-25 21:51:13]

The CQ bit was checked by zhaoze.zhou@partner.samsung.com

[commit-bot: I haz the power, 2014-07-25 21:51:49]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/zhaoze.zhou@partner.samsung.com/382633...

[commit-bot: I haz the power, 2014-07-26 00:10:07]

Change committed as 285735