42653 2010-07-20 10:59:01 -0700 IntSize / FloatSize: add invalid() and isValid() helper methods 2010-08-06 13:21:53 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) PC OS X 10.5 RESOLVED INVALID P2 Normal --- 1 tonikitoo tonikitoo darin kenneth oldest_to_newest 253818 0 tonikitoo 2010-07-20 10:59:01 -0700 In comment https://bugs.webkit.org/show_bug.cgi?id=40197#c41 , it was suggested a helper / getter for invalid IntSize's. Maybe something simple like this: static IntSize invalid() { return IntSize(-1, -1); } similarly to the static IntPoint zero() { return IntPoint(0, 0); } added in bug 37220. Thoughts? ps: If I hear it is ok, upload a patch. 253821 1 tonikitoo 2010-07-20 11:08:44 -0700 isValid() would work similarly to Qt's version: "Returns true if both the width and height is equal to or greater than 0; otherwise returns false." [1] [1] http://doc.trolltech.com/4.7-snapshot/qsize.html#isValid 253823 2 darin 2010-07-20 11:12:03 -0700 This use of -1,-1 as a magic value seems questionable to me, even though Qt uses it. 253829 3 tonikitoo 2010-07-20 11:21:56 -0700 (In reply to comment #2) > This use of -1,-1 as a magic value seems questionable to me, even though Qt uses it. In fact qt does not use it as there is not static invalid method in QSize. If we decide to not go for adding invalid() since -1,-1 is in fact "magic", I can keep using IntSize(-1, -1) where appropriated. However, least isValid() seems useful. It does have a different meaning from isEmpty and isZero. 253840 4 tonikitoo 2010-07-20 11:48:10 -0700 Other possible users of isValid() and maybe IntSize::invalid() static void constructDeletedValue(IntSize& slot) { new (&slot) IntSize(-1, -1); } static bool isDeletedValue(const IntSize& value) { return value.width() == -1 && value.height() == -1; } 253841 5 tonikitoo 2010-07-20 11:49:32 -0700 ... and: void FrameView::init() { reset(); m_margins = IntSize(-1, -1); // undefined sorry for bug-spamming. 253843 6 62096 tonikitoo 2010-07-20 11:57:31 -0700 Created attachment 62096 patch1 - v1 - isValid addition. Lets go with isValid for now. Darin? 253869 7 darin 2010-07-20 12:42:45 -0700 (In reply to comment #3) > However, least isValid() seems useful. It does have a different meaning from isEmpty and isZero. Sure, but is this a useful distinction? Again, I know Qt has it, but my starting point would be assuming we should not use invalid points. The distinction between null and empty string is an eternal headache and it would be nice to not have more things like that. 253879 8 tonikitoo 2010-07-20 12:59:42 -0700 (In reply to comment #7) > (In reply to comment #3) > > However, least isValid() seems useful. It does have a different meaning from isEmpty and isZero. > > Sure, but is this a useful distinction? Again, I know Qt has it, but my starting point would be assuming we should not use invalid points. > > The distinction between null and empty string is an eternal headache and it would be nice to not have more things like that. Imho, it is an useful distiction in the context of the rect-based hit test, for example. The situation there is the following: if one calls nodesFromRect(x, y, 0, 0), a valid rect is still built up. It means the caller of this method still wants a rect-based hit test but he (the caller) is saying to not expand the original point. In fact, that is what he will get since the formula we are using the build up the rect from the point + padding still accepts the previous method input. Here is it: inline IntRect HitTestResult::rectFromPoint(const IntPoint& p) const { return IntRect(p.x() - m_padding.width(), // x p.y() - m_padding.height(), // y 2 * m_padding.width() + 1, // weigth 2 * m_padding.height() + 1); // height } So in this context, non-negative IntSize's are valid. If user explicitly passes an invalid/undefined/negative IntSize(-anyValue, -anyValue) the current is{Empty,Zero} methods are not what we need to perform the check. Of course I can still make isValid internally in my class, but would be useful to have this int IntSize class. Hope I could had made it clear about my of use for adding this simple helper. Let me know if you still thing it is bad thing to do. 253880 9 tonikitoo 2010-07-20 13:01:16 -0700 s/weigth/width 254052 10 62096 darin 2010-07-20 16:19:23 -0700 Comment on attachment 62096 patch1 - v1 - isValid addition. Core Graphics has this concept for rectangles, but not for sizes. And it calls the concept “is null” rather than “is (in)valid”. I’m not sure our functions that manipulate sizes all respect this notion. It doesn’t do a lot of harm to add these functions, but I have to say the concept seems a bit unclear to me. Where are some of the call sites where we want to use this? 254135 11 tonikitoo 2010-07-20 23:27:47 -0700 (In reply to comment #10) I see your conceptual concerns. If we go for not adding this method, I can make it an inliner in my class, no problem. > Where are some of the call sites where we want to use this? See some possible users of this in comment #4 and comment #5, and again, the validation check of m_padding (IntSize) in HitTestResult - see bug 40197. 254138 12 darin 2010-07-20 23:30:27 -0700 (In reply to comment #11) > See some possible users of this in comment #4 and comment #5 I would prefer not to use it in either of those places. > the validation check of m_padding (IntSize) in HitTestResult - see bug 40197. I don’t know exactly what a validation check is. Do we really need a separate "invalid" value for padding? This seems unnecessarily tricky. Typically it's best to avoid special values and instead use explicit signaling. A separate boolean or some other way of saying there is no padding value, rather than a special "null" value. 254139 13 tonikitoo 2010-07-20 23:34:17 -0700 > > the validation check of m_padding (IntSize) in HitTestResult - see bug 40197. > > I don’t know exactly what a validation check is. Do we really need a separate "invalid" value for padding? This seems unnecessarily tricky. Typically it's best to avoid special values and instead use explicit signaling. A separate boolean or some other way of saying there is no padding value, rather than a special "null" value. Sure. A boolean works for me, and I am already using one. I was think it'd be more elegant to not use, though, but the change proved itself rather unneeded, so closing the bug as INVALID. Thank you for your time on this, Darin. 62096 2010-07-20 11:57:31 -0700 2010-08-06 13:21:53 -0700 patch1 - v1 - isValid addition. 0001-2010-07-20-Antonio-Gomes-tonikitoo-webkit.org.patch text/plain 2930 tonikitoo RnJvbSA4MzNkYWFiZTQxYWE5NWRjYzQ5MWY4MzI0YTdkOThjYTc2MGQ1YzQ5IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBBbnRvbmlvIEdvbWVzIDx0b25pa2l0b29Ad2Via2l0Lm9yZz4K RGF0ZTogVHVlLCAyMCBKdWwgMjAxMCAxNDo1MTozOCAtMDQwMApTdWJqZWN0OiBbUEFUQ0hdIDIw MTAtMDctMjAgIEFudG9uaW8gR29tZXMgIDx0b25pa2l0b29Ad2Via2l0Lm9yZz4KCiAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCgogICAgICAgIEludFNpemV8RmxvYXRTaXplOiBh ZGQgaXNWYWxpZCgpIGhlbHBlciBtZXRob2QuCiAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTQyNjUzCgogICAgICAgIGlzVmFsaWQgcmV0dXJucyB0cnVlIGlm IGJvdGggdGhlIHdpZHRoIGFuZCBoZWlnaHQgaXMgZXF1YWwgdG8gb3IgZ3JlYXRlcgogICAgICAg IHRoYW4gMDsgb3RoZXJ3aXNlIHJldHVybnMgZmFsc2UuCgogICAgICAgICogcGxhdGZvcm0vZ3Jh cGhpY3MvRmxvYXRTaXplLmg6CiAgICAgICAgKFdlYkNvcmU6OkZsb2F0U2l6ZTo6aXNWYWxpZCk6 CiAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9JbnRTaXplLmg6CiAgICAgICAgKFdlYkNvcmU6 OkludFNpemU6OmlzVmFsaWQpOgotLS0KIFdlYkNvcmUvQ2hhbmdlTG9nICAgICAgICAgICAgICAg ICAgICAgfCAgIDE1ICsrKysrKysrKysrKysrKwogV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9G bG9hdFNpemUuaCB8ICAgIDEgKwogV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9JbnRTaXplLmgg ICB8ICAgIDMgKystCiAzIGZpbGVzIGNoYW5nZWQsIDE4IGluc2VydGlvbnMoKyksIDEgZGVsZXRp b25zKC0pCgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9DaGFuZ2VMb2cgYi9XZWJDb3JlL0NoYW5nZUxv ZwppbmRleCA0NDBiMGU5Li4zMDhhZDQ5IDEwMDY0NAotLS0gYS9XZWJDb3JlL0NoYW5nZUxvZwor KysgYi9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEwLTA3LTIwICBBbnRv bmlvIEdvbWVzICA8dG9uaWtpdG9vQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW50U2l6ZXxGbG9hdFNpemU6IGFkZCBpc1ZhbGlk KCkgaGVscGVyIG1ldGhvZC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTQyNjUzCisKKyAgICAgICAgaXNWYWxpZCByZXR1cm5zIHRydWUgaWYgYm90aCB0 aGUgd2lkdGggYW5kIGhlaWdodCBpcyBlcXVhbCB0byBvciBncmVhdGVyCisgICAgICAgIHRoYW4g MDsgb3RoZXJ3aXNlIHJldHVybnMgZmFsc2UuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGlj cy9GbG9hdFNpemUuaDoKKyAgICAgICAgKFdlYkNvcmU6OkZsb2F0U2l6ZTo6aXNWYWxpZCk6Cisg ICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvSW50U2l6ZS5oOgorICAgICAgICAoV2ViQ29yZTo6 SW50U2l6ZTo6aXNWYWxpZCk6CisKIDIwMTAtMDctMTkgR3JhY2UgS2xvYmEgIDxrbG9iYWdAZ21h aWwuY29tPiAsIEFudG9uaW8gR29tZXMgIDx0b25pa2l0b29Ad2Via2l0Lm9yZz4KIAogICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvcGxhdGZv cm0vZ3JhcGhpY3MvRmxvYXRTaXplLmggYi9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0Zsb2F0 U2l6ZS5oCmluZGV4IGZmM2Q0ZGUuLjEyYjZmY2UgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvcGxhdGZv cm0vZ3JhcGhpY3MvRmxvYXRTaXplLmgKKysrIGIvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9G bG9hdFNpemUuaApAQCAtNjIsNiArNjIsNyBAQCBwdWJsaWM6CiAgICAgdm9pZCBzZXRIZWlnaHQo ZmxvYXQgaGVpZ2h0KSB7IG1faGVpZ2h0ID0gaGVpZ2h0OyB9CiAKICAgICBib29sIGlzRW1wdHko KSBjb25zdCB7IHJldHVybiBtX3dpZHRoIDw9IDAgfHwgbV9oZWlnaHQgPD0gMDsgfQorICAgIGJv b2wgaXNWYWxpZCgpIGNvbnN0IHsgcmV0dXJuIG1fd2lkdGggPj0gMCAmJiBtX2hlaWdodCA+PSAw OyB9CiAKICAgICBmbG9hdCBhc3BlY3RSYXRpbygpIGNvbnN0IHsgcmV0dXJuIG1fd2lkdGggLyBt X2hlaWdodDsgfQogCmRpZmYgLS1naXQgYS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0ludFNp emUuaCBiL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvSW50U2l6ZS5oCmluZGV4IDllY2YzODku LmQ2ZTA5ZWEgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvSW50U2l6ZS5o CisrKyBiL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvSW50U2l6ZS5oCkBAIC02Myw3ICs2Myw3 IEBAIGNsYXNzIEludFNpemUgewogcHVibGljOgogICAgIEludFNpemUoKSA6IG1fd2lkdGgoMCks IG1faGVpZ2h0KDApIHsgfQogICAgIEludFNpemUoaW50IHdpZHRoLCBpbnQgaGVpZ2h0KSA6IG1f d2lkdGgod2lkdGgpLCBtX2hlaWdodChoZWlnaHQpIHsgfQotICAgIAorCiAgICAgaW50IHdpZHRo KCkgY29uc3QgeyByZXR1cm4gbV93aWR0aDsgfQogICAgIGludCBoZWlnaHQoKSBjb25zdCB7IHJl dHVybiBtX2hlaWdodDsgfQogCkBAIC03Miw2ICs3Miw3IEBAIHB1YmxpYzoKIAogICAgIGJvb2wg aXNFbXB0eSgpIGNvbnN0IHsgcmV0dXJuIG1fd2lkdGggPD0gMCB8fCBtX2hlaWdodCA8PSAwOyB9 CiAgICAgYm9vbCBpc1plcm8oKSBjb25zdCB7IHJldHVybiAhbV93aWR0aCAmJiAhbV9oZWlnaHQ7 IH0KKyAgICBib29sIGlzVmFsaWQoKSBjb25zdCB7IHJldHVybiBtX3dpZHRoID49IDAgJiYgbV9o ZWlnaHQgPj0gMDsgfQogCiAgICAgZmxvYXQgYXNwZWN0UmF0aW8oKSBjb25zdCB7IHJldHVybiBz dGF0aWNfY2FzdDxmbG9hdD4obV93aWR0aCkgLyBzdGF0aWNfY2FzdDxmbG9hdD4obV9oZWlnaHQp OyB9CiAgICAgCi0tIAoxLjcuMC40Cgo=