140467 2015-01-14 14:46:44 -0800 Buildfix after r178434 2015-01-14 15:33:01 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 140347 1 ossy ossy cdumez darin ossy oldest_to_newest 1061311 0 ossy 2015-01-14 14:46:44 -0800 Buildfix after r178434 1061312 1 244641 ossy 2015-01-14 14:46:55 -0800 Created attachment 244641 Patch 1061315 2 244641 cdumez 2015-01-14 14:49:38 -0800 Comment on attachment 244641 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244641&action=review > Source/WebCore/css/StyleBuilderConverter.h:990 > perspective = primitiveValue.computeLength<float>(styleResolver.state().cssToLengthConversionData()); This can potentially return a negative value. So that the one 2 lines below. > Source/WebCore/css/StyleBuilderConverter.h:-996 > - return perspective < 0 ? Nullopt : perspective; you are changing behavior as we are no longer making sure the values returned by primitiveValue.computeLength() and primitiveValue.getDoubleValue() are positive. 1061318 3 244641 cdumez 2015-01-14 14:54:14 -0800 Comment on attachment 244641 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244641&action=review > Source/WebCore/css/StyleBuilderConverter.h:996 > + return perspective; maybe just "return perspective < 0 ? Optional<float>() : Optional<float>(perspective);" ? 1061325 4 244646 ossy 2015-01-14 14:59:53 -0800 Created attachment 244646 Patch 1061327 5 244641 ossy 2015-01-14 15:00:47 -0800 Comment on attachment 244641 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244641&action=review >> Source/WebCore/css/StyleBuilderConverter.h:-996 >> - return perspective < 0 ? Nullopt : perspective; > > you are changing behavior as we are no longer making sure the values returned by primitiveValue.computeLength() and primitiveValue.getDoubleValue() are positive. Good point, I thought it handles only the default -1 value. >> Source/WebCore/css/StyleBuilderConverter.h:996 >> + return perspective; > > maybe just "return perspective < 0 ? Optional<float>() : Optional<float>(perspective);" ? Nullopt is good, only the second one should be changed. 1061329 6 244646 cdumez 2015-01-14 15:02:45 -0800 Comment on attachment 244646 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review > Source/WebCore/css/StyleBuilderConverter.h:996 > + return perspective < 0 ? Nullopt : Optional<float>(perspective); We still have different types on each side. I am surprised this one works but my original code didn't. Even if this works, I still prefer to have a Optional<float>() on the left side for consistency. 1061331 7 244646 cdumez 2015-01-14 15:04:36 -0800 Comment on attachment 244646 Patch r=me with Optional<float>() on left side. 1061332 8 244646 cdumez 2015-01-14 15:07:00 -0800 Comment on attachment 244646 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review > Source/WebCore/ChangeLog:3 > + Buildfix after r178434 Also, we might want to say EFL here as other ports built just fine I believe. 1061333 9 ossy 2015-01-14 15:08:29 -0800 (In reply to comment #6) > Comment on attachment 244646 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244646&action=review > > > Source/WebCore/css/StyleBuilderConverter.h:996 > > + return perspective < 0 ? Nullopt : Optional<float>(perspective); > > We still have different types on each side. I am surprised this one works > but my original code didn't. Even if this works, I still prefer to have a > Optional<float>() on the left side for consistency. https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L50 Optional(NulloptTag) implicit ctor is called, we don't call it explicitly anywhere. 1061334 10 ossy 2015-01-14 15:10:10 -0800 (In reply to comment #8) > Comment on attachment 244646 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244646&action=review > > > Source/WebCore/ChangeLog:3 > > + Buildfix after r178434 > > Also, we might want to say EFL here as other ports built just fine I believe. It isn't related to EFL at all, but stricter GCC options: "error: enumeral and non-enumeral type in conditional expression [-Werror]" 1061336 11 244646 cdumez 2015-01-14 15:11:01 -0800 Comment on attachment 244646 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review >>> Source/WebCore/css/StyleBuilderConverter.h:996 >>> + return perspective < 0 ? Nullopt : Optional<float>(perspective); >> >> We still have different types on each side. I am surprised this one works but my original code didn't. Even if this works, I still prefer to have a Optional<float>() on the left side for consistency. > > https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L50 > > Optional(NulloptTag) implicit ctor is called, we don't call it > explicitly anywhere. Well, then in my previous code, implicit Optional(const Length& value) was called and EFL had trouble building it. I don't see the difference. 1061338 12 244646 cdumez 2015-01-14 15:11:45 -0800 Comment on attachment 244646 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review >>>> Source/WebCore/css/StyleBuilderConverter.h:996 >>>> + return perspective < 0 ? Nullopt : Optional<float>(perspective); >>> >>> We still have different types on each side. I am surprised this one works but my original code didn't. Even if this works, I still prefer to have a Optional<float>() on the left side for consistency. >> >> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L50 >> >> Optional(NulloptTag) implicit ctor is called, we don't call it >> explicitly anywhere. > > Well, then in my previous code, implicit Optional(const Length& value) was called and EFL had trouble building it. I don't see the difference. https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L55 1061340 13 244646 cdumez 2015-01-14 15:14:36 -0800 Comment on attachment 244646 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review >>>>> Source/WebCore/css/StyleBuilderConverter.h:996 >>>>> + return perspective < 0 ? Nullopt : Optional<float>(perspective); >>>> >>>> We still have different types on each side. I am surprised this one works but my original code didn't. Even if this works, I still prefer to have a Optional<float>() on the left side for consistency. >>> >>> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L50 >>> >>> Optional(NulloptTag) implicit ctor is called, we don't call it >>> explicitly anywhere. >> >> Well, then in my previous code, implicit Optional(const Length& value) was called and EFL had trouble building it. I don't see the difference. > > https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L55 I am worried that with stricter build options your new code may not build either because we have different types on each side (even if it is sufficient for the current EFL build options). 1061344 14 244646 cdumez 2015-01-14 15:20:38 -0800 Comment on attachment 244646 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review >>> Source/WebCore/ChangeLog:3 >>> + Buildfix after r178434 >> >> Also, we might want to say EFL here as other ports built just fine I believe. > > It isn't related to EFL at all, but stricter GCC options: > "error: enumeral and non-enumeral type in conditional expression [-Werror]" Ok, fair enough. 1061345 15 ossy 2015-01-14 15:21:05 -0800 (In reply to comment #13) > Comment on attachment 244646 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244646&action=review > >> Well, then in my previous code, implicit Optional(const Length& value) was called and EFL had trouble building it. I don't see the difference. > > > > https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L55 It seems GCC doesn't try to call implicit Optional(const Length& value). If it did, both size should have the same type. > I am worried that with stricter build options your new code may not build > either because we have different types on each side (even if it is > sufficient for the current EFL build options). Don't worry, I tried it, it builds. ;) I think GCC is clever enough to call the proper ctor for NullOptTag after seeing Optional<float>(perspective). But it isn't able to call implicit ctor for both side in the same time. 1061348 16 244646 darin 2015-01-14 15:23:09 -0800 Comment on attachment 244646 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244646&action=review >>>>>> Source/WebCore/css/StyleBuilderConverter.h:996 >>>>>> + return perspective < 0 ? Nullopt : Optional<float>(perspective); >>>>> >>>>> We still have different types on each side. I am surprised this one works but my original code didn't. Even if this works, I still prefer to have a Optional<float>() on the left side for consistency. >>>> >>>> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L50 >>>> >>>> Optional(NulloptTag) implicit ctor is called, we don't call it >>>> explicitly anywhere. >>> >>> Well, then in my previous code, implicit Optional(const Length& value) was called and EFL had trouble building it. I don't see the difference. >> >> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L55 > > I am worried that with stricter build options your new code may not build either because we have different types on each side (even if it is sufficient for the current EFL build options). I suggest we use an if statement instead of ? : to solve this problem. 1061355 17 cdumez 2015-01-14 15:29:31 -0800 (In reply to comment #16) > Comment on attachment 244646 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244646&action=review > > >>>>>> Source/WebCore/css/StyleBuilderConverter.h:996 > >>>>>> + return perspective < 0 ? Nullopt : Optional<float>(perspective); > >>>>> > >>>>> We still have different types on each side. I am surprised this one works but my original code didn't. Even if this works, I still prefer to have a Optional<float>() on the left side for consistency. > >>>> > >>>> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L50 > >>>> > >>>> Optional(NulloptTag) implicit ctor is called, we don't call it > >>>> explicitly anywhere. > >>> > >>> Well, then in my previous code, implicit Optional(const Length& value) was called and EFL had trouble building it. I don't see the difference. > >> > >> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/Optional.h?rev=178372&order=name#L55 > > > > I am worried that with stricter build options your new code may not build either because we have different types on each side (even if it is sufficient for the current EFL build options). > > I suggest we use an if statement instead of ? : to solve this problem. Sure, works for me. 1061356 18 ossy 2015-01-14 15:33:01 -0800 Fixed by https://trac.webkit.org/changeset/178454 244641 2015-01-14 14:46:55 -0800 2015-01-14 14:59:46 -0800 Patch bug-140467-20150114144659.patch text/plain 1729 ossy U3VidmVyc2lvbiBSZXZpc2lvbjogMTc4NDQ0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYTIwOTAzNTE2MTVjZDI4 NzE0OTJmZDM1OTdiOGNkNTAxODNkMTIyYS4uMGQ4MTdjNWFmMzEzZjAwZjk4ZTNhNjU2YTczY2Qx YWI1YzJkZWI5YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDE1LTAxLTE0ICBDc2Fi YSBPc3p0cm9nb27DoWMgIDxvc3N5QHdlYmtpdC5vcmc+CisKKyAgICAgICAgQnVpbGRmaXggYWZ0 ZXIgcjE3ODQzNAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTQwNDY3CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgKiBjc3MvU3R5bGVCdWlsZGVyQ29udmVydGVyLmg6CisgICAgICAgIChXZWJDb3JlOjpTdHls ZUJ1aWxkZXJDb252ZXJ0ZXI6OmNvbnZlcnRQZXJzcGVjdGl2ZSk6CisKIDIwMTUtMDEtMTQgIEJh cnRsb21pZWogR2FqZGEgIDxiLmdhamRhQHNhbXN1bmcuY29tPgogCiAgICAgICAgIFtNU0VdIElt cGxlbWVudCBwZXIgVHJhY2tCdWZmZXIgYnVmZmVyZWQuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi Q29yZS9jc3MvU3R5bGVCdWlsZGVyQ29udmVydGVyLmggYi9Tb3VyY2UvV2ViQ29yZS9jc3MvU3R5 bGVCdWlsZGVyQ29udmVydGVyLmgKaW5kZXggMmY1OWMzY2RkZWY4NDcwODQxNmM4Mzc0NGMwZjlh ZDRhNzU1MGU3MS4uM2M5YjZlNDM5NWZiZDAyNDg2ZDVlM2RkNWI4MWE4NDJjN2M1NDQwMSAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvY3NzL1N0eWxlQnVpbGRlckNvbnZlcnRlci5oCisrKyBi L1NvdXJjZS9XZWJDb3JlL2Nzcy9TdHlsZUJ1aWxkZXJDb252ZXJ0ZXIuaApAQCAtOTg1LDcgKzk4 NSw3IEBAIGlubGluZSBPcHRpb25hbDxmbG9hdD4gU3R5bGVCdWlsZGVyQ29udmVydGVyOjpjb252 ZXJ0UGVyc3BlY3RpdmUoU3R5bGVSZXNvbHZlciYKICAgICBpZiAocHJpbWl0aXZlVmFsdWUuZ2V0 VmFsdWVJRCgpID09IENTU1ZhbHVlTm9uZSkKICAgICAgICAgcmV0dXJuIDAuZjsKIAotICAgIGZs b2F0IHBlcnNwZWN0aXZlID0gLTE7CisgICAgT3B0aW9uYWw8ZmxvYXQ+IHBlcnNwZWN0aXZlOwog ICAgIGlmIChwcmltaXRpdmVWYWx1ZS5pc0xlbmd0aCgpKQogICAgICAgICBwZXJzcGVjdGl2ZSA9 IHByaW1pdGl2ZVZhbHVlLmNvbXB1dGVMZW5ndGg8ZmxvYXQ+KHN0eWxlUmVzb2x2ZXIuc3RhdGUo KS5jc3NUb0xlbmd0aENvbnZlcnNpb25EYXRhKCkpOwogICAgIGVsc2UgaWYgKHByaW1pdGl2ZVZh bHVlLmlzTnVtYmVyKCkpCkBAIC05OTMsNyArOTkzLDcgQEAgaW5saW5lIE9wdGlvbmFsPGZsb2F0 PiBTdHlsZUJ1aWxkZXJDb252ZXJ0ZXI6OmNvbnZlcnRQZXJzcGVjdGl2ZShTdHlsZVJlc29sdmVy JgogICAgIGVsc2UKICAgICAgICAgQVNTRVJUX05PVF9SRUFDSEVEKCk7CiAKLSAgICByZXR1cm4g cGVyc3BlY3RpdmUgPCAwID8gTnVsbG9wdCA6IHBlcnNwZWN0aXZlOworICAgIHJldHVybiBwZXJz cGVjdGl2ZTsKIH0KIAogaW5saW5lIE9wdGlvbmFsPExlbmd0aD4gU3R5bGVCdWlsZGVyQ29udmVy dGVyOjpjb252ZXJ0TWFycXVlZUluY3JlbWVudChTdHlsZVJlc29sdmVyJiBzdHlsZVJlc29sdmVy LCBDU1NWYWx1ZSYgdmFsdWUpCg== 244646 2015-01-14 14:59:53 -0800 2015-01-14 15:04:36 -0800 Patch bug-140467-20150114145958.patch text/plain 1346 ossy U3VidmVyc2lvbiBSZXZpc2lvbjogMTc4NDQ0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYTIwOTAzNTE2MTVjZDI4 NzE0OTJmZDM1OTdiOGNkNTAxODNkMTIyYS4uMGQ4MTdjNWFmMzEzZjAwZjk4ZTNhNjU2YTczY2Qx YWI1YzJkZWI5YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDE1LTAxLTE0ICBDc2Fi YSBPc3p0cm9nb27DoWMgIDxvc3N5QHdlYmtpdC5vcmc+CisKKyAgICAgICAgQnVpbGRmaXggYWZ0 ZXIgcjE3ODQzNAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTQwNDY3CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgKiBjc3MvU3R5bGVCdWlsZGVyQ29udmVydGVyLmg6CisgICAgICAgIChXZWJDb3JlOjpTdHls ZUJ1aWxkZXJDb252ZXJ0ZXI6OmNvbnZlcnRQZXJzcGVjdGl2ZSk6CisKIDIwMTUtMDEtMTQgIEJh cnRsb21pZWogR2FqZGEgIDxiLmdhamRhQHNhbXN1bmcuY29tPgogCiAgICAgICAgIFtNU0VdIElt cGxlbWVudCBwZXIgVHJhY2tCdWZmZXIgYnVmZmVyZWQuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi Q29yZS9jc3MvU3R5bGVCdWlsZGVyQ29udmVydGVyLmggYi9Tb3VyY2UvV2ViQ29yZS9jc3MvU3R5 bGVCdWlsZGVyQ29udmVydGVyLmgKaW5kZXggMmY1OWMzY2RkZWY4NDcwODQxNmM4Mzc0NGMwZjlh ZDRhNzU1MGU3MS4uODhlZjQ3YWU1MmU2MGU4OGNiNjc4YTI4MmRhMTZhNThiYzg1YzY5MCAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvY3NzL1N0eWxlQnVpbGRlckNvbnZlcnRlci5oCisrKyBi L1NvdXJjZS9XZWJDb3JlL2Nzcy9TdHlsZUJ1aWxkZXJDb252ZXJ0ZXIuaApAQCAtOTkzLDcgKzk5 Myw3IEBAIGlubGluZSBPcHRpb25hbDxmbG9hdD4gU3R5bGVCdWlsZGVyQ29udmVydGVyOjpjb252 ZXJ0UGVyc3BlY3RpdmUoU3R5bGVSZXNvbHZlciYKICAgICBlbHNlCiAgICAgICAgIEFTU0VSVF9O T1RfUkVBQ0hFRCgpOwogCi0gICAgcmV0dXJuIHBlcnNwZWN0aXZlIDwgMCA/IE51bGxvcHQgOiBw ZXJzcGVjdGl2ZTsKKyAgICByZXR1cm4gcGVyc3BlY3RpdmUgPCAwID8gTnVsbG9wdCA6IE9wdGlv bmFsPGZsb2F0PihwZXJzcGVjdGl2ZSk7CiB9CiAKIGlubGluZSBPcHRpb25hbDxMZW5ndGg+IFN0 eWxlQnVpbGRlckNvbnZlcnRlcjo6Y29udmVydE1hcnF1ZWVJbmNyZW1lbnQoU3R5bGVSZXNvbHZl ciYgc3R5bGVSZXNvbHZlciwgQ1NTVmFsdWUmIHZhbHVlKQo=