63392 2011-06-25 17:49:35 -0700 Indirectly including TextPosition.h and XPathGrammar.h causes compile errors 2011-06-28 09:57:15 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 j.mont webkit-unassigned abarth ap caseq j.mont mrowe peter.rybin webkit.review.bot oldest_to_newest 427468 0 j.mont 2011-06-25 17:49:35 -0700 When both TextPosition.h and XPathGrammar.h are included a compile-error is caused, since XPathGrammar.h defines a macro called NUMBER and TextPosition has a typedef named NUMBER. PS. I am not sure JavaScriptCore is the right component for this bug... If it is, then new test cases aren't relevant since no new code was added. 427469 1 98611 j.mont 2011-06-25 17:51:29 -0700 Created attachment 98611 Patch 427470 2 98612 j.mont 2011-06-25 18:05:02 -0700 Created attachment 98612 Patch 427489 3 ap 2011-06-26 00:22:25 -0700 We use all-uppercase words for macros, so TextPosition.h is indeed wrong using it as template parameter. A similar problem exists in InspectorValues.cpp. TextPosition has a ton of problems - from a bad name to a huge number of style errors in its code. Supporting both 1-based and 0-based offsets is a particularly bad idea - and that's why the class is templatized in the first place. It shouldn't have passed review in its current form. 427518 4 abarth 2011-06-26 07:32:55 -0700 > Supporting both 1-based and 0-based offsets is a particularly bad idea. I'm not sure we have much of an option in that regard. The point of TextPosition is for the C++ type checker to help us avoid off-by-one bugs with 1-based and 0-based line numbering systems. If you see a way to switch to only a 1-based or 0-based line numbering scheme, please write a patch. (The style issues, of course, should be fixed.) 427555 5 ap 2011-06-26 11:13:28 -0700 Do we have a real need to have both numbering systems in core code, not just when displaying a position in Web Inspector? That seems unlikely. C++ type safety only comes into play when assigning from one TextPosition to another, so off by one errors will still be likely when converting to or from integers anyway. I don't think that I'm supposed write a patch for every design/review failure that I see. 427558 6 abarth 2011-06-26 11:20:52 -0700 > Do we have a real need to have both numbering systems in core code, not just when displaying a position in Web Inspector? That seems unlikely. I don't see how to avoid this issue. If you see how to avoid it, please let the rest of us know. > C++ type safety only comes into play when assigning from one TextPosition to another, so off by one errors will still be likely when converting to or from integers anyway. Indeed. However, these values get passed around a bunch of places and it's helpful to keep track of whether they're one-based or zero-based. > I don't think that I'm supposed write a patch for every design/review failure that I see. You're claiming this machinery isn't needed, but I suspect you don't understand the design pressures that lead to its creation. If you see a way to remove it, I'd be more than happy to review such a ptach. I don't see how to do it, which is why we have these types. 427559 7 ap 2011-06-26 11:45:31 -0700 Yes, It's quite possible that I'm missing some design constraints, as I didn't look at call sites. However, fixing this seems incredibly trivial - in every place that creates TextPosition<OneBasedNumber>, subtract one when constructing. And make matching changes when looking at the values. 428371 8 98612 mrowe 2011-06-27 15:20:49 -0700 Comment on attachment 98612 Patch This change is fine and addresses a build issue. If anyone is unhappy with the design of TextPosition then please file a new bug to follow up on that. 428427 9 98612 webkit.review.bot 2011-06-27 16:16:49 -0700 Comment on attachment 98612 Patch Clearing flags on attachment: 98612 Committed r89875: <http://trac.webkit.org/changeset/89875> 428428 10 webkit.review.bot 2011-06-27 16:16:54 -0700 All reviewed patches have been landed. Closing bug. 428436 11 peter.rybin 2011-06-27 16:33:46 -0700 (In reply to comment #7) > Yes, It's quite possible that I'm missing some design constraints, as I didn't look at call sites. However, fixing this seems incredibly trivial - in every place that creates TextPosition<OneBasedNumber>, subtract one when constructing. And make matching changes when looking at the values. Alexey, 0 and 1 (as it is stated inside TextPostion.h) was meant as a part of transition phase -- the first step was to keep all integers values intact, only wrap them in annotation types. That would make the change less drastic and more safe. This way all the places that used to work with 1-based numbers now have OneBaseNumber, and the same with 0-based. As you see there has always been some kind of mixture of different number bases. In particular XMLDocumentParserQt.cpp source still contains incorrect 0 -> 1 reinterpertation, that is now revealed and documented and should be fixed. You are absolutely right -- WebKit doesn't need to keep both types. In fact they are ready to be easily merged together. You just need to literally merge their interfaces, because all methods have prepared names and constructors are hidden (and decide on internals: 0 or 1). Everything should compile and work. Unfortunately I'm not involved with WebKit so much to start doing this myself, at least doing this alone. Peter 428509 12 peter.rybin 2011-06-27 17:51:55 -0700 As a follow-up. This bug in XMLDocumentParserQt.cpp may need TextPosition0 and TextPosition1 types separate: it is proven that there is bug somewhere, because we end up reinterpreting type. Is could be much easier to track the problem down while integers are still annotated. At least that's how it seems to me now. Aside from this merge should be easy. Probably I should prepare it. Peter (In reply to comment #11) > (In reply to comment #7) > > Yes, It's quite possible that I'm missing some design constraints, as I didn't look at call sites. However, fixing this seems incredibly trivial - in every place that creates TextPosition<OneBasedNumber>, subtract one when constructing. And make matching changes when looking at the values. > > Alexey, > > 0 and 1 (as it is stated inside TextPostion.h) was meant as a part of transition phase -- the first step was to keep all integers values intact, only wrap them in annotation types. That would make the change less drastic and more safe. This way all the places that used to work with 1-based numbers now have OneBaseNumber, and the same with 0-based. As you see there has always been some kind of mixture of different number bases. In particular XMLDocumentParserQt.cpp source still contains incorrect 0 -> 1 reinterpertation, that is now revealed and documented and should be fixed. > > You are absolutely right -- WebKit doesn't need to keep both types. In fact they are ready to be easily merged together. You just need to literally merge their interfaces, because all methods have prepared names and constructors are hidden (and decide on internals: 0 or 1). Everything should compile and work. > > Unfortunately I'm not involved with WebKit so much to start doing this myself, at least doing this alone. > > Peter 428559 13 peter.rybin 2011-06-27 18:44:42 -0700 This is how merge could be done. In place of ZeroBasedNumber and OneBasedNumber the following code could be placed: class BasedNumber { public: static BasedNumber fromZeroBasedInt(int zeroBasedInt) { return BasedNumber(zeroBasedInt); } static BasedNumber fromOneBasedInt(int oneBasedInt) { return BasedNumber(oneBasedInt - 1); } BasedNumber() {} int zeroBasedInt() const { return m_value0; } int oneBasedInt() const { return m_value0 + 1; } int convertAsOneBasedInt() const { return m_value0 + 1; } int convertAsZeroBasedInt() const { return m_value0; } BasedNumber convertToOneBased() const { return *this; } BasedNumber convertToZeroBased() const { return *this; } bool operator==(BasedNumber other) { return m_value0 == other.m_value0; } bool operator!=(BasedNumber other) { return !((*this) == other); } static BasedNumber base() { return BasedNumber(0); } static BasedNumber belowBase() { return BasedNumber(-1); } private: BasedNumber(int value0) : m_value0(value0) {} int m_value0; }; typedef BasedNumber ZeroBasedNumber; typedef BasedNumber OneBasedNumber; This should compile. Obviously, this is only a first step of refactoring. I would be happy to help doing this, but probably I'm not ready to start this whole thing myself. 428694 14 ap 2011-06-28 00:50:48 -0700 Would you be willing to file a new bug, and to post these ideas there? 428937 15 peter.rybin 2011-06-28 09:57:15 -0700 (In reply to comment #14) > Would you be willing to file a new bug, and to post these ideas there? Sure. https://bugs.webkit.org/show_bug.cgi?id=63541 98611 2011-06-25 17:51:29 -0700 2011-06-25 18:04:58 -0700 Patch bug-63392-20110625175128.patch text/plain 2232 j.mont SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gODk3NTkpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAK KzIwMTEtMDYtMjUgIEp1YW4gQy4gTW9udGVtYXlvciAgPGptb250QGFwcGxlLmNvbT4KKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJbmRpcmVjdGx5IGlu Y2x1ZGluZyBUZXh0UG9zaXRpb24uaCBhbmQgWFBhdGhHcmFtbWFyLmggY2F1c2VzIGNvbXBpbGUg ZXJyb3JzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02 MzM5MgorCisgICAgICAgICogd3RmL3RleHQvVGV4dFBvc2l0aW9uLmg6CisgICAgICAgIChXVEY6 OlRleHRQb3NpdGlvbjo6VGV4dFBvc2l0aW9uKToKKyAgICAgICAgKFdURjo6VGV4dFBvc2l0aW9u OjptaW5pbXVtUG9zaXRpb24pOgorICAgICAgICAoV1RGOjpUZXh0UG9zaXRpb246OmJlbG93UmFu Z2VQb3NpdGlvbik6CisKIDIwMTEtMDYtMjQgIE5pa29sYXMgWmltbWVybWFubiAgPG56aW1tZXJt YW5uQHJpbS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgUm9iIEJ1aXMuCkluZGV4OiBTb3Vy Y2UvSmF2YVNjcmlwdENvcmUvd3RmL3RleHQvVGV4dFBvc2l0aW9uLmgKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g U291cmNlL0phdmFTY3JpcHRDb3JlL3d0Zi90ZXh0L1RleHRQb3NpdGlvbi5oCShyZXZpc2lvbiA4 OTcxMCkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvdGV4dC9UZXh0UG9zaXRpb24uaAko d29ya2luZyBjb3B5KQpAQCAtNTYsMjcgKzU2LDI2IEBAIG5hbWVzcGFjZSBXVEYgewogICogaW50 LXJlY2VpdmluZyBjb25zdHJ1Y3RvcnMgYXJlIGhpZGRlbiBmcm9tIEFQSS4KICAqLwogCi10ZW1w bGF0ZTx0eXBlbmFtZSBOVU1CRVI+Cit0ZW1wbGF0ZTx0eXBlbmFtZSBUPgogY2xhc3MgVGV4dFBv c2l0aW9uIHsKIHB1YmxpYzoKLSAgICBUZXh0UG9zaXRpb24oTlVNQkVSIGxpbmUsIE5VTUJFUiBj b2x1bW4pCisgICAgVGV4dFBvc2l0aW9uKFQgbGluZSwgVCBjb2x1bW4pCiAgICAgICAgIDogbV9s aW5lKGxpbmUpCiAgICAgICAgICwgbV9jb2x1bW4oY29sdW1uKQogICAgIHsKICAgICB9CiAgICAg VGV4dFBvc2l0aW9uKCkge30KLQogICAgIGJvb2wgb3BlcmF0b3I9PShjb25zdCBUZXh0UG9zaXRp b24mIG90aGVyKSB7IHJldHVybiBtX2xpbmUgPT0gb3RoZXIubV9saW5lICYmIG1fY29sdW1uID09 IG90aGVyLm1fY29sdW1uOyB9CiAgICAgYm9vbCBvcGVyYXRvciE9KGNvbnN0IFRleHRQb3NpdGlv biYgb3RoZXIpIHsgcmV0dXJuICEoKCp0aGlzKSA9PSBvdGhlcik7IH0KIAogICAgIC8vIEEgJ21p bmltdW0nIHZhbHVlIG9mIHBvc2l0aW9uLCB1c2VkIGFzIGEgZGVmYXVsdCB2YWx1ZS4KLSAgICBz dGF0aWMgVGV4dFBvc2l0aW9uPE5VTUJFUj4gbWluaW11bVBvc2l0aW9uKCkgeyByZXR1cm4gVGV4 dFBvc2l0aW9uPE5VTUJFUj4oTlVNQkVSOjpiYXNlKCksIE5VTUJFUjo6YmFzZSgpKTsgfQorICAg IHN0YXRpYyBUZXh0UG9zaXRpb248VD4gbWluaW11bVBvc2l0aW9uKCkgeyByZXR1cm4gVGV4dFBv c2l0aW9uPFQ+KFQ6OmJhc2UoKSwgVDo6YmFzZSgpKTsgfQogCiAgICAgLy8gQSB2YWx1ZSB3aXRo IGxpbmUgdmFsdWUgbGVzcyB0aGFuIGEgbWluaW11bTsgdXNlZCBhcyBhbiBpbXBvc3NpYmxlIHBv c2l0aW9uLgotICAgIHN0YXRpYyBUZXh0UG9zaXRpb248TlVNQkVSPiBiZWxvd1JhbmdlUG9zaXRp b24oKSB7IHJldHVybiBUZXh0UG9zaXRpb248TlVNQkVSPihOVU1CRVI6OmJlbG93QmFzZSgpLCBO VU1CRVI6OmJlbG93QmFzZSgpKTsgfQorICAgIHN0YXRpYyBUZXh0UG9zaXRpb248VD4gYmVsb3dS YW5nZVBvc2l0aW9uKCkgeyByZXR1cm4gVGV4dFBvc2l0aW9uPFQ+KFQ6OmJlbG93QmFzZSgpLCBU OjpiZWxvd0Jhc2UoKSk7IH0KIAotICAgIE5VTUJFUiBtX2xpbmU7Ci0gICAgTlVNQkVSIG1fY29s dW1uOworICAgIFQgbV9saW5lOworICAgIFQgbV9jb2x1bW47CiB9OwogCiBjbGFzcyBPbmVCYXNl ZE51bWJlcjsK 98612 2011-06-25 18:05:02 -0700 2011-06-27 16:16:49 -0700 Patch bug-63392-20110625180501.patch text/plain 2450 j.mont SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gODk3NTkpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTkgQEAK KzIwMTEtMDYtMjUgIEp1YW4gQy4gTW9udGVtYXlvciAgPGptb250QGFwcGxlLmNvbT4KKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJbmRpcmVjdGx5IGlu Y2x1ZGluZyBUZXh0UG9zaXRpb24uaCBhbmQgWFBhdGhHcmFtbWFyLmggY2F1c2VzIGNvbXBpbGUg ZXJyb3JzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02 MzM5MgorICAgICAgICAKKyAgICAgICAgV2hlbiBib3RoIFRleHRQb3NpdGlvbi5oIGFuZCBYUGF0 aEdyYW1tYXIuaCBhcmUgaW5jbHVkZWQgYSBjb21waWxlLWVycm9yCisgICAgICAgIGlzIGNhdXNl ZCwgc2luY2UgWFBhdGhHcmFtbWFyLmggZGVmaW5lcyBhIG1hY3JvIGNhbGxlZCBOVU1CRVIgYW5k IAorICAgICAgICBUZXh0UG9zaXRpb24gaGFzIGEgdHlwZWRlZiBuYW1lZCBOVU1CRVIuCisKKyAg ICAgICAgKiB3dGYvdGV4dC9UZXh0UG9zaXRpb24uaDoKKyAgICAgICAgKFdURjo6VGV4dFBvc2l0 aW9uOjpUZXh0UG9zaXRpb24pOgorICAgICAgICAoV1RGOjpUZXh0UG9zaXRpb246Om1pbmltdW1Q b3NpdGlvbik6CisgICAgICAgIChXVEY6OlRleHRQb3NpdGlvbjo6YmVsb3dSYW5nZVBvc2l0aW9u KToKKwogMjAxMS0wNi0yNCAgTmlrb2xhcyBaaW1tZXJtYW5uICA8bnppbW1lcm1hbm5AcmltLmNv bT4KIAogICAgICAgICBSZXZpZXdlZCBieSBSb2IgQnVpcy4KSW5kZXg6IFNvdXJjZS9KYXZhU2Ny aXB0Q29yZS93dGYvdGV4dC9UZXh0UG9zaXRpb24uaAo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2 YVNjcmlwdENvcmUvd3RmL3RleHQvVGV4dFBvc2l0aW9uLmgJKHJldmlzaW9uIDg5NzEwKQorKysg U291cmNlL0phdmFTY3JpcHRDb3JlL3d0Zi90ZXh0L1RleHRQb3NpdGlvbi5oCSh3b3JraW5nIGNv cHkpCkBAIC01NiwyNyArNTYsMjYgQEAgbmFtZXNwYWNlIFdURiB7CiAgKiBpbnQtcmVjZWl2aW5n IGNvbnN0cnVjdG9ycyBhcmUgaGlkZGVuIGZyb20gQVBJLgogICovCiAKLXRlbXBsYXRlPHR5cGVu YW1lIE5VTUJFUj4KK3RlbXBsYXRlPHR5cGVuYW1lIFQ+CiBjbGFzcyBUZXh0UG9zaXRpb24gewog cHVibGljOgotICAgIFRleHRQb3NpdGlvbihOVU1CRVIgbGluZSwgTlVNQkVSIGNvbHVtbikKKyAg ICBUZXh0UG9zaXRpb24oVCBsaW5lLCBUIGNvbHVtbikKICAgICAgICAgOiBtX2xpbmUobGluZSkK ICAgICAgICAgLCBtX2NvbHVtbihjb2x1bW4pCiAgICAgewogICAgIH0KICAgICBUZXh0UG9zaXRp b24oKSB7fQotCiAgICAgYm9vbCBvcGVyYXRvcj09KGNvbnN0IFRleHRQb3NpdGlvbiYgb3RoZXIp IHsgcmV0dXJuIG1fbGluZSA9PSBvdGhlci5tX2xpbmUgJiYgbV9jb2x1bW4gPT0gb3RoZXIubV9j b2x1bW47IH0KICAgICBib29sIG9wZXJhdG9yIT0oY29uc3QgVGV4dFBvc2l0aW9uJiBvdGhlcikg eyByZXR1cm4gISgoKnRoaXMpID09IG90aGVyKTsgfQogCiAgICAgLy8gQSAnbWluaW11bScgdmFs dWUgb2YgcG9zaXRpb24sIHVzZWQgYXMgYSBkZWZhdWx0IHZhbHVlLgotICAgIHN0YXRpYyBUZXh0 UG9zaXRpb248TlVNQkVSPiBtaW5pbXVtUG9zaXRpb24oKSB7IHJldHVybiBUZXh0UG9zaXRpb248 TlVNQkVSPihOVU1CRVI6OmJhc2UoKSwgTlVNQkVSOjpiYXNlKCkpOyB9CisgICAgc3RhdGljIFRl eHRQb3NpdGlvbjxUPiBtaW5pbXVtUG9zaXRpb24oKSB7IHJldHVybiBUZXh0UG9zaXRpb248VD4o VDo6YmFzZSgpLCBUOjpiYXNlKCkpOyB9CiAKICAgICAvLyBBIHZhbHVlIHdpdGggbGluZSB2YWx1 ZSBsZXNzIHRoYW4gYSBtaW5pbXVtOyB1c2VkIGFzIGFuIGltcG9zc2libGUgcG9zaXRpb24uCi0g ICAgc3RhdGljIFRleHRQb3NpdGlvbjxOVU1CRVI+IGJlbG93UmFuZ2VQb3NpdGlvbigpIHsgcmV0 dXJuIFRleHRQb3NpdGlvbjxOVU1CRVI+KE5VTUJFUjo6YmVsb3dCYXNlKCksIE5VTUJFUjo6YmVs b3dCYXNlKCkpOyB9CisgICAgc3RhdGljIFRleHRQb3NpdGlvbjxUPiBiZWxvd1JhbmdlUG9zaXRp b24oKSB7IHJldHVybiBUZXh0UG9zaXRpb248VD4oVDo6YmVsb3dCYXNlKCksIFQ6OmJlbG93QmFz ZSgpKTsgfQogCi0gICAgTlVNQkVSIG1fbGluZTsKLSAgICBOVU1CRVIgbV9jb2x1bW47CisgICAg VCBtX2xpbmU7CisgICAgVCBtX2NvbHVtbjsKIH07CiAKIGNsYXNzIE9uZUJhc2VkTnVtYmVyOwo=