45292 2010-09-07 06:01:25 -0700 V8 generated code is missing include of V8BindingMacros.h for STRING_TO_V8PARAMETER_EXCEPTION_BLOCK 2010-09-07 09:45:26 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Android Android RESOLVED FIXED P2 Normal --- 0 kristianm webkit-unassigned antonm commit-queue dumi steveblock oldest_to_newest 274986 0 66711 kristianm 2010-09-07 06:01:25 -0700 Created attachment 66711 Proposed patch The header does not get included through other headers on Android. Introduced in this CL: http://trac.webkit.org/changeset/66664/trunk/WebCore/bindings/scripts/CodeGeneratorV8.pm 274995 1 steveblock 2010-09-07 06:23:54 -0700 The required include of V8BindingMacros.h was lost in http://trac.webkit.org/changeset/66521 274997 2 66711 steveblock 2010-09-07 06:25:36 -0700 Comment on attachment 66711 Proposed patch > + Compile fix for Android. Explicitly add needed header for > + STRING_TO_V8PARAMETER_EXCEPTION_BLOCK as they do not get included through > + other headers on Android. Need a bug URL Remember to set r? and cq? if you want the patch to be reviewed and added to the commit-queue. 275001 3 66715 kristianm 2010-09-07 06:36:41 -0700 Created attachment 66715 Updated patch with Bug# 275002 4 kristianm 2010-09-07 06:42:06 -0700 (In reply to comment #2) > (From update of attachment 66711 [details]) > > + Compile fix for Android. Explicitly add needed header for > > + STRING_TO_V8PARAMETER_EXCEPTION_BLOCK as they do not get included through > > + other headers on Android. > Need a bug URL > > Remember to set r? and cq? if you want the patch to be reviewed and added to the commit-queue. I was not yet ready for review, as I was looking for where it got introduced. Thanks for noticing the missing bug url. 275021 5 66715 steveblock 2010-09-07 07:25:23 -0700 Comment on attachment 66715 Updated patch with Bug# Is the include of V8BindingMacros.h from GenerateNormalAttrSetter() still required? 275027 6 kristianm 2010-09-07 07:36:35 -0700 (In reply to comment #5) > (From update of attachment 66715 [details]) > Is the include of V8BindingMacros.h from GenerateNormalAttrSetter() still required? This is the CL that actually caused the problem: http://trac.webkit.org/changeset/66521/trunk/WebCore/bindings/scripts/CodeGeneratorV8.pm I grepped through the generated files, and it seems like the header is never included twice at least. 275028 7 steveblock 2010-09-07 07:39:01 -0700 > I grepped through the generated files, and it seems like the header is never > included twice at least. Sure, but this doesn't mean that we don't add the header to the implIncludes map more than once. 275033 8 antonm 2010-09-07 07:56:21 -0700 (In reply to comment #7) > > I grepped through the generated files, and it seems like the header is never > > included twice at least. > Sure, but this doesn't mean that we don't add the header to the implIncludes map more than once. Sorry guys for introducing this. Could we setup BBs to watch for Android breakage? 275034 9 antonm 2010-09-07 07:57:25 -0700 Just in case: I've added more cases to use the macro. I hope you already have necessary includes on those paths. 275038 10 steveblock 2010-09-07 08:02:21 -0700 > Sorry guys for introducing this. Could we setup BBs to watch for Android > breakage? No problem. We'd love to have an Android BB, but we have a lot of upstreaming to do first! > Just in case: I've added more cases to use the macro. I hope you already have > necessary includes on those paths. It looks like the macro is only used from ConvertToV8Parameter(), to which this patch adds the include, so I think we're OK. Anton, do you know if the include from GenerateNormalAttrSetter() can be removed, or are you happy with this patch as it is? 275043 11 antonm 2010-09-07 08:13:57 -0700 (In reply to comment #10) > > Just in case: I've added more cases to use the macro. I hope you already have > > necessary includes on those paths. > It looks like the macro is only used from ConvertToV8Parameter(), to which this patch adds the include, so I think we're OK. > > Anton, do you know if the include from GenerateNormalAttrSetter() can be removed, or are you happy with this patch as it is? Yes, you're right. I think it's the best place for inclusion and we don't want to specialize unless build time is prohibitive (which should not be the case, hopefully) 275092 12 66715 commit-queue 2010-09-07 09:45:20 -0700 Comment on attachment 66715 Updated patch with Bug# Clearing flags on attachment: 66715 Committed r66888: <http://trac.webkit.org/changeset/66888> 275093 13 commit-queue 2010-09-07 09:45:26 -0700 All reviewed patches have been landed. Closing bug. 66711 2010-09-07 06:01:25 -0700 2010-09-07 06:36:41 -0700 Proposed patch patch.txt text/plain 1292 kristianm SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2Njg3NikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMTAtMDktMDcgIEtyaXN0aWFuIE1vbnNlbiAgPGtyaXN0aWFubUBn b29nbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIENvbXBpbGUgZml4IGZvciBBbmRyb2lkLiBFeHBsaWNpdGx5IGFkZCBuZWVkZWQgaGVhZGVy IGZvcgorICAgICAgICBTVFJJTkdfVE9fVjhQQVJBTUVURVJfRVhDRVBUSU9OX0JMT0NLIGFzIHRo ZXkgZG8gbm90IGdldCBpbmNsdWRlZCB0aHJvdWdoCisgICAgICAgIG90aGVyIGhlYWRlcnMgb24g QW5kcm9pZC4KKworICAgICAgICBObyBuZXcgdGVzdHMsIGp1c3QgYSBjb21waWxlIGZpeC4KKwor ICAgICAgICAqIGJpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvclY4LnBtOgorCiAyMDEwLTA5 LTA3ICBLZW50IEhhbnNlbiAgPGtlbnQuaGFuc2VuQG5va2lhLmNvbT4KIAogICAgICAgICBSZXZp ZXdlZCBieSBBbmRyZWFzIEtsaW5nLgpJbmRleDogV2ViQ29yZS9iaW5kaW5ncy9zY3JpcHRzL0Nv ZGVHZW5lcmF0b3JWOC5wbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2JpbmRpbmdzL3NjcmlwdHMv Q29kZUdlbmVyYXRvclY4LnBtCShyZXZpc2lvbiA2Njg0MSkKKysrIFdlYkNvcmUvYmluZGluZ3Mv c2NyaXB0cy9Db2RlR2VuZXJhdG9yVjgucG0JKHdvcmtpbmcgY29weSkKQEAgLTMzNzksNiArMzM3 OSw3IEBAIHN1YiBDb252ZXJ0VG9WOFBhcmFtZXRlcgogCiAgICAgZGllICJXcm9uZyBuYXRpdmUg dHlwZSBwYXNzZWQ6ICRuYXRpdmVUeXBlIiB1bmxlc3MgJG5hdGl2ZVR5cGUgPX4gL15WOFBhcmFt ZXRlci87CiAgICAgaWYgKCRzaWduYXR1cmUtPnR5cGUgZXEgIkRPTVN0cmluZyIpIHsKKyAgICAg ICAgJGltcGxJbmNsdWRlc3siVjhCaW5kaW5nTWFjcm9zLmgifSA9IDE7CiAgICAgICAgIG15ICRt YWNybyA9ICJTVFJJTkdfVE9fVjhQQVJBTUVURVJfRVhDRVBUSU9OX0JMT0NLIjsKICAgICAgICAg JG1hY3JvIC49ICJfJHN1ZmZpeCIgaWYgJHN1ZmZpeDsKICAgICAgICAgcmV0dXJuICIkbWFjcm8o JG5hdGl2ZVR5cGUsICR2YXJpYWJsZU5hbWUsICR2YWx1ZSk7Igo= 66715 2010-09-07 06:36:41 -0700 2010-09-07 09:45:20 -0700 Updated patch with Bug# patch.txt text/plain 1358 kristianm SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2Njg3NikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTcgQEAKKzIwMTAtMDktMDcgIEtyaXN0aWFuIE1vbnNlbiAgPGtyaXN0aWFubUBn b29nbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIENvbXBpbGUgZml4IGZvciBBbmRyb2lkLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDUyOTIKKyAgICAgICAgRXhwbGljaXRseSBhZGQgbmVlZGVk IGhlYWRlciBmb3IgCisgICAgICAgIFNUUklOR19UT19WOFBBUkFNRVRFUl9FWENFUFRJT05fQkxP Q0sgYXMgdGhleSBkbyBub3QgZ2V0IAorICAgICAgICBpbmNsdWRlZCB0aHJvdWdoIG90aGVyIGhl YWRlcnMgb24gQW5kcm9pZC4KKworICAgICAgICBObyBuZXcgdGVzdHMsIGp1c3QgYSBjb21waWxl IGZpeC4KKworICAgICAgICAqIGJpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvclY4LnBtOgor CiAyMDEwLTA5LTA3ICBLZW50IEhhbnNlbiAgPGtlbnQuaGFuc2VuQG5va2lhLmNvbT4KIAogICAg ICAgICBSZXZpZXdlZCBieSBBbmRyZWFzIEtsaW5nLgpJbmRleDogV2ViQ29yZS9iaW5kaW5ncy9z Y3JpcHRzL0NvZGVHZW5lcmF0b3JWOC5wbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2JpbmRpbmdz L3NjcmlwdHMvQ29kZUdlbmVyYXRvclY4LnBtCShyZXZpc2lvbiA2Njg0MSkKKysrIFdlYkNvcmUv YmluZGluZ3Mvc2NyaXB0cy9Db2RlR2VuZXJhdG9yVjgucG0JKHdvcmtpbmcgY29weSkKQEAgLTMz NzksNiArMzM3OSw3IEBAIHN1YiBDb252ZXJ0VG9WOFBhcmFtZXRlcgogCiAgICAgZGllICJXcm9u ZyBuYXRpdmUgdHlwZSBwYXNzZWQ6ICRuYXRpdmVUeXBlIiB1bmxlc3MgJG5hdGl2ZVR5cGUgPX4g L15WOFBhcmFtZXRlci87CiAgICAgaWYgKCRzaWduYXR1cmUtPnR5cGUgZXEgIkRPTVN0cmluZyIp IHsKKyAgICAgICAgJGltcGxJbmNsdWRlc3siVjhCaW5kaW5nTWFjcm9zLmgifSA9IDE7CiAgICAg ICAgIG15ICRtYWNybyA9ICJTVFJJTkdfVE9fVjhQQVJBTUVURVJfRVhDRVBUSU9OX0JMT0NLIjsK ICAgICAgICAgJG1hY3JvIC49ICJfJHN1ZmZpeCIgaWYgJHN1ZmZpeDsKICAgICAgICAgcmV0dXJu ICIkbWFjcm8oJG5hdGl2ZVR5cGUsICR2YXJpYWJsZU5hbWUsICR2YWx1ZSk7Igo=