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.
Created attachment 183609 [details] proposed change
Comment on attachment 183609 [details] proposed change Why should we do this?
(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.
(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. :)
(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.
(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!
Created attachment 183691 [details] remove the change in Platform.h
Comment on attachment 183691 [details] remove the change in Platform.h LGTM.
Comment on attachment 183691 [details] remove the change in Platform.h Clearing flags on attachment: 183691 Committed r140283: <http://trac.webkit.org/changeset/140283>
All reviewed patches have been landed. Closing bug.
Comment on attachment 183691 [details] 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.
(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.
(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. :)