107365 2013-01-19 00:28:29 -0800 Simplify a list of negative PLATFORM() tests 2013-01-20 17:28:59 -0800 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 laszlo.gombos webkit-unassigned benjamin eric ojan.autocc webkit.review.bot oldest_to_newest 811339 0 laszlo.gombos 2013-01-19 00:28:29 -0800 Document the list of ports that support Windows and use the documented list to simplify a serious of negative PLATFORM() tests into a simpler list of positive tests. 811341 1 183609 laszlo.gombos 2013-01-19 00:36:01 -0800 Created attachment 183609 proposed change 811342 2 183609 eric 2013-01-19 00:41:11 -0800 Comment on attachment 183609 proposed change Why should we do this? 811343 3 laszlo.gombos 2013-01-19 00:48:25 -0800 (In reply to comment #2) > (From update of attachment 183609 [details]) > Why should we do this? The change in Platform.h is basically an ASSERT for the build system. Self-documenting code. The change in config.h IMHO makes the code more readable and maintainable. I would not feel comfortable making this change without the change in Platform.h. 811344 4 eric 2013-01-19 00:50:04 -0800 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 183609 [details] [details]) > > Why should we do this? > > The change in Platform.h is basically an ASSERT for the build system. Self-documenting code. > > The change in config.h IMHO makes the code more readable and maintainable. I would not feel comfortable making this change without the change in Platform.h. The config.h change is r=me. The Platform.h change is what I'm confused by. :) I'm just not sure why it's important to have a list of ports which support windows. We don't have the same for any other platform. :) 811351 5 laszlo.gombos 2013-01-19 01:15:27 -0800 (In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 183609 [details] [details] [details]) > The Platform.h change is what I'm confused by. :) I'm just not sure why it's important to have a list of ports which support windows. I do not really know the definite list of PLATFORM()s that support OS(WINDOWS) - I just made a best effort to list them. The only way to know is to run the test in Platform.h in all the bots/ports and see which one fails/who screams. I was thinking that perhaps we should leave this code in there so that the next guy making a similar change would not have this problem. If I made an error in my list of Windows ports than the change in config.h is wrong. > We don't have the same for any other platform. :) I can add a list for other OS()s. Another example from Platform.h: #if OS(DARWIN) && !PLATFORM(GTK) && !PLATFORM(QT) #define ENABLE_PURGEABLE_MEMORY 1 #endif I do not really know which ports would have ENABLE_PURGEABLE_MEMORY turned on. If I could replace the test with: #if OS(DARWIN) && (PLATFORM(MAC) || PLATFORM(WX)) than I would find the code more readable and maintainable. Not a big deal, I am OK either way, just wanted to make sure that I make my case. 811352 6 eric 2013-01-19 01:25:37 -0800 (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > (From update of attachment 183609 [details] [details] [details] [details]) > #if OS(DARWIN) && !PLATFORM(GTK) && !PLATFORM(QT) > #define ENABLE_PURGEABLE_MEMORY 1 > #endif > > I do not really know which ports would have ENABLE_PURGEABLE_MEMORY turned on. If I could replace the test with: I agree. This is a big (and growing) problem for WebKit. That's part of what: https://bugs.webkit.org/show_bug.cgi?id=85456 was about. I'm glad that others (yourself included) are looking into this, and trying to simplify our features system so that we can acxtually tell what's on/off for various ports! 811695 7 183691 laszlo.gombos 2013-01-20 15:27:32 -0800 Created attachment 183691 remove the change in Platform.h 811696 8 183691 eric 2013-01-20 15:48:23 -0800 Comment on attachment 183691 remove the change in Platform.h LGTM. 811697 9 183691 webkit.review.bot 2013-01-20 16:13:11 -0800 Comment on attachment 183691 remove the change in Platform.h Clearing flags on attachment: 183691 Committed r140283: <http://trac.webkit.org/changeset/140283> 811698 10 webkit.review.bot 2013-01-20 16:13:15 -0800 All reviewed patches have been landed. Closing bug. 811699 11 183691 benjamin 2013-01-20 16:47:07 -0800 Comment on attachment 183691 remove the change in Platform.h View in context: https://bugs.webkit.org/attachment.cgi?id=183691&action=review > Source/WebCore/ChangeLog:11 > + No new tests as there is no new functionality. Please don't say that. Some people literally believe they do not need tests for bug fix, or make up excuses. This particular sentence is becoming a red flag. I think we can be better examples in our changelogs regarding testing/not testing. 811707 12 eric 2013-01-20 17:25:00 -0800 (In reply to comment #11) > (From update of attachment 183691 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183691&action=review > > > Source/WebCore/ChangeLog:11 > > + No new tests as there is no new functionality. > > Please don't say that. > Some people literally believe they do not need tests for bug fix, or make up excuses. This particular sentence is becoming a red flag. > > I think we can be better examples in our changelogs regarding testing/not testing. Perhaps you should start a Webkit-dev thread on the subject. I was not aware of the concen. 811708 13 eric 2013-01-20 17:28:59 -0800 (In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 183691 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=183691&action=review > > > > > Source/WebCore/ChangeLog:11 > > > + No new tests as there is no new functionality. > > > > Please don't say that. > > Some people literally believe they do not need tests for bug fix, or make up excuses. This particular sentence is becoming a red flag. > > > > I think we can be better examples in our changelogs regarding testing/not testing. > > Perhaps you should start a Webkit-dev thread on the subject. I was not aware of the concen. I failed to type my intended smilie. :) 183609 2013-01-19 00:36:01 -0800 2013-01-20 15:27:32 -0800 proposed change 107365.patch text/plain 2608 laszlo.gombos SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxNDAyMzgpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEzLTAxLTE5ICBMYXN6bG8gR29tYm9zICA8bC5n b21ib3NAc2Ftc3VuZy5jb20+CisKKyAgICAgICAgRG9jdW1lbnQgdGhlIGxpc3Qgb2YgcG9ydHMg dGhhdCBzdXBwb3J0IFdpbmRvd3MKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTEwNzM2NQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIENhcHR1cmUgdGhlIGZ1bGwgbGlzdCBvZiBwb3J0cyB0aGF0IHN1cHBvcnQg V2luZG93cy4KKyAgICAgICAgVGhpcyBsaXN0IGNhbiBiZSB1c2VkIHRvIG1ha2UgYXNzdW1wdGlv bnMgZWxzZXdoZXJlIGluCisgICAgICAgIHNvdXJjZSB0cmVlLgorCisgICAgICAgICogd3RmL1Bs YXRmb3JtLmg6CisKIDIwMTMtMDEtMTggIExhc3psbyBHb21ib3MgIDxsLmdvbWJvc0BzYW1zdW5n LmNvbT4KIAogICAgICAgICBSZW1vdmUgdW5uZWNlc3NhcnkgUExBVEZPUk0oKSB0ZXN0cwpJbmRl eDogU291cmNlL1dURi93dGYvUGxhdGZvcm0uaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV1RGL3d0 Zi9QbGF0Zm9ybS5oCShyZXZpc2lvbiAxNDAyMzMpCisrKyBTb3VyY2UvV1RGL3d0Zi9QbGF0Zm9y bS5oCSh3b3JraW5nIGNvcHkpCkBAIC00NjcsNiArNDY3LDE0IEBACiAjZGVmaW5lIFdURl9QTEFU Rk9STV9JT1NfU0lNVUxBVE9SIDEKICNlbmRpZgogCisvKiBEb2N1bWVudCB0aGUgbGlzdCBvZiBw b3J0cyB0aGF0IHN1cHBvcnRzIFdpbmRvd3MgKi8KKy8qIEZJWE1FOiBnaXZlIHRoZSBXSU5DRSBw b3J0IGl0cyB2ZXJ5IG93biBQTEFURk9STShXSU5DRSkgbWFjcm8gKi8KKyNpZiBPUyhXSU5ET1dT KQorI2lmICEoUExBVEZPUk0oQ0hST01JVU0pIHx8IFBMQVRGT1JNKEdUSykgfHwgUExBVEZPUk0o UVQpIHx8IFBMQVRGT1JNKFdJTikgfHwgUExBVEZPUk0oV0lOX0NBSVJPKSB8fCBQTEFURk9STShX WCkgfHwgT1MoV0lOQ0UpKQorI2Vycm9yICJZb3UgaGF2ZSB0byBhZGQgeW91ciBwb3J0IHRvIHRo ZSBsaXN0IG9mIHBvcnRzIHRoYXQgc3VwcG9ydHMgV2luZG93cy4iCisjZW5kaWYKKyNlbmRpZgor CiAvKiBHcmFwaGljcyBlbmdpbmVzICovCiAKIC8qIFVTRShDRykgYW5kIFBMQVRGT1JNKENJKSAq LwpJbmRleDogU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9X ZWJDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTQwMjM4KQorKysgU291cmNlL1dlYkNvcmUvQ2hh bmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTMtMDEtMTkgIExhc3ps byBHb21ib3MgIDxsLmdvbWJvc0BzYW1zdW5nLmNvbT4KKworICAgICAgICBEb2N1bWVudCB0aGUg bGlzdCBvZiBwb3J0cyB0aGF0IHN1cHBvcnQgV2luZG93cworICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTA3MzY1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgU2ltcGxpZnkgYSBsaXN0IG9mIG5lZ2F0aXZlIFBM QVRGT1JNKCkgdGVzdHMgaW50byBhIHNpbXBsZXIgbGlzdAorICAgICAgICBvZiBwb3NpdGl2ZSB0 ZXN0cyBmb3IgYmV0dGVyIHJlYWRhYmlsaXR5IGFuZCBtYWludGVuYW5jZS4KKworICAgICAgICBO byBuZXcgdGVzdHMgYXMgdGhlcmUgaXMgbm8gbmV3IGZ1bmN0aW9uYWxpdHkuCisKKyAgICAgICAg KiBjb25maWcuaDoKKwogMjAxMy0wMS0xOCAgSHVhbmcgRG9uZ3N1bmcgIDxsdXh0ZWxsYUBjb21w YW55MTAwLm5ldD4KIAogICAgICAgICBbTWFjXSBSZW1vdmUgdW51c2VkIHBhZ2VTY2FsZUZhY3Rv ciBhbmQgcG9zaXRpb25SZWxhdGl2ZVRvQmFzZSBhcmd1bWVudHMgaW4gR3JhcGhpY3NMYXllckNB LgpJbmRleDogU291cmNlL1dlYkNvcmUvY29uZmlnLmgKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dl YkNvcmUvY29uZmlnLmgJKHJldmlzaW9uIDE0MDIzMykKKysrIFNvdXJjZS9XZWJDb3JlL2NvbmZp Zy5oCSh3b3JraW5nIGNvcHkpCkBAIC0yOSw3ICsyOSw3IEBACiAKICNpbmNsdWRlIDx3dGYvUGxh dGZvcm0uaD4KIAotI2lmIE9TKFdJTkRPV1MpICYmICFPUyhXSU5DRSkgJiYgIVBMQVRGT1JNKFFU KSAmJiAhUExBVEZPUk0oQ0hST01JVU0pICYmICFQTEFURk9STShHVEspICYmICFQTEFURk9STShX WCkKKyNpZiBQTEFURk9STShXSU4pIHx8IFBMQVRGT1JNKFdJTl9DQUlSTykKICNpbmNsdWRlIDxX ZWJDb3JlL1dlYkNvcmVIZWFkZXJEZXRlY3Rpb24uaD4KICNlbmRpZgogCg== 183691 2013-01-20 15:27:32 -0800 2013-01-20 16:47:07 -0800 remove the change in Platform.h 107365.patch text/plain 1154 laszlo.gombos SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE0MDIzOSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDEzLTAxLTIwICBMYXN6bG8g R29tYm9zICA8bC5nb21ib3NAc2Ftc3VuZy5jb20+CisKKyAgICAgICAgU2ltcGxpZnkgYSBsaXN0 IG9mIG5lZ2F0aXZlIFBMQVRGT1JNKCkgdGVzdHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTEwNzM2NQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIFNpbXBsaWZ5IGEgbGlzdCBvZiBuZWdhdGl2ZSBQTEFURk9S TSgpIHRlc3RzIGludG8gYSBzaW1wbGVyIGxpc3QKKyAgICAgICAgb2YgcG9zaXRpdmUgdGVzdHMg Zm9yIGJldHRlciByZWFkYWJpbGl0eSBhbmQgbWFpbnRlbmFuY2UuCisKKyAgICAgICAgTm8gbmV3 IHRlc3RzIGFzIHRoZXJlIGlzIG5vIG5ldyBmdW5jdGlvbmFsaXR5LgorCisgICAgICAgICogY29u ZmlnLmg6CisKIDIwMTMtMDEtMTkgIFphbiBEb2JlcnNlayAgPHpkb2JlcnNla0BpZ2FsaWEuY29t PgogCiAgICAgICAgIFVucmV2aWV3ZWQgR1RLIGJ1aWxkIGZpeC4KSW5kZXg6IFNvdXJjZS9XZWJD b3JlL2NvbmZpZy5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2NvbmZpZy5oCShyZXZp c2lvbiAxNDAyMzkpCisrKyBTb3VyY2UvV2ViQ29yZS9jb25maWcuaAkod29ya2luZyBjb3B5KQpA QCAtMjksNyArMjksNyBAQAogCiAjaW5jbHVkZSA8d3RmL1BsYXRmb3JtLmg+CiAKLSNpZiBPUyhX SU5ET1dTKSAmJiAhT1MoV0lOQ0UpICYmICFQTEFURk9STShRVCkgJiYgIVBMQVRGT1JNKENIUk9N SVVNKSAmJiAhUExBVEZPUk0oR1RLKSAmJiAhUExBVEZPUk0oV1gpCisjaWYgUExBVEZPUk0oV0lO KSAmJiAhT1MoV0lOQ0UpCiAjaW5jbHVkZSA8V2ViQ29yZS9XZWJDb3JlSGVhZGVyRGV0ZWN0aW9u Lmg+CiAjZW5kaWYKIAo=