132638 2014-05-06 19:08:51 -0700 Clean up the difference between painting focus rings and adding PDF annotations 2014-05-07 13:04:29 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 dino dino commit-queue darin esprehn+autocc glenn kondapallykalyan sam thorton oldest_to_newest 1007173 0 dino 2014-05-06 19:08:51 -0700 Clean up the difference between painting focus rings and adding PDF annotations 1007174 1 230963 dino 2014-05-06 19:13:07 -0700 Created attachment 230963 Patch 1007178 2 230963 simon.fraser 2014-05-06 19:24:37 -0700 Comment on attachment 230963 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230963&action=review > Source/WebCore/rendering/RenderObject.cpp:1016 > +void RenderObject::addPDFURLRect(PaintInfo& paintInfo, const LayoutPoint& paintOffset) This function needs to DIAF. 1007182 3 230963 commit-queue 2014-05-06 19:59:08 -0700 Comment on attachment 230963 Patch Clearing flags on attachment: 230963 Committed r168404: <http://trac.webkit.org/changeset/168404> 1007183 4 commit-queue 2014-05-06 19:59:11 -0700 All reviewed patches have been landed. Closing bug. 1007186 5 230963 darin 2014-05-06 20:11:29 -0700 Comment on attachment 230963 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=230963&action=review > Source/WebCore/rendering/RenderInline.cpp:1499 > + else if (hasOutlineAnnotation() && !theme().supportsFocusRing(&styleToUse)) > + addPDFURLRect(paintInfo, paintOffset); Why is this else? Why not an independent if? 1007191 6 dino 2014-05-06 20:43:52 -0700 (In reply to comment #5) > (From update of attachment 230963 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=230963&action=review > > > Source/WebCore/rendering/RenderInline.cpp:1499 > > + else if (hasOutlineAnnotation() && !theme().supportsFocusRing(&styleToUse)) > > + addPDFURLRect(paintInfo, paintOffset); > > Why is this else? Why not an independent if? Because the original logic was: if (A || B) { if (C) paintFocusRing(); } void paintFocusRing() { if (A) // actually paint focus ring else PDFRect() } So the new PDF path still has to check !A (the code that was in paintFocusRing). I could have done that in the separate if, but I figured the else branch was cleaner. New code is: if (A && C) paintFocusRing() else if (B && C) PDFRect() The "else if" knows that either A is false (which is what we need) or C is false (in which case it will fail itself). The alternative would be: if (A && C) paintFocusRing if (!A && B && C) PDFRect() Now that I've written it, what I committed is pretty subtle, so I'll fix it. 1007194 7 dino 2014-05-06 20:46:29 -0700 BTW, I left the "if (A)" in paintFocusRing (outlineStyleIsAuto) even though these are the only two callers. They will only call when A is true, but who knows what someone else will do in the future. 1007198 8 darin 2014-05-06 20:49:58 -0700 (In reply to comment #7) > BTW, I left the "if (A)" in paintFocusRing (outlineStyleIsAuto) even though these are the only two callers. They will only call when A is true, but who knows what someone else will do in the future. We can document such preconditions and include assertions if we prefer to do that instead of doing redundant checks in the production code. 1007406 9 dino 2014-05-07 13:04:29 -0700 Committed followup r168437: <http://trac.webkit.org/changeset/168437> 230963 2014-05-06 19:13:07 -0700 2014-05-06 20:11:29 -0700 Patch bug-132638-20140507121243.patch text/plain 6678 dino U3VidmVyc2lvbiBSZXZpc2lvbjogMTY4Mzk4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggM2RlMjg3YTEzZGQ1NDA4 NTYyY2U3MGQ4MzQ5MWM1MDAxMWE2NWNjMS4uZjA4YzUzMjhmNzM5NWUxYWJmZWNmMTUzMjQ5ZTE4 NWYyNzQ3NDNmNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDI4IEBACiAyMDE0LTA1LTA2ICBEZWFu IEphY2tzb24gIDxkaW5vQGFwcGxlLmNvbT4KIAorICAgICAgICBDbGVhbiB1cCB0aGUgZGlmZmVy ZW5jZSBiZXR3ZWVuIHBhaW50aW5nIGZvY3VzIHJpbmdzIGFuZCBhZGRpbmcgUERGIGFubm90YXRp b25zCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzI2 MzgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGUg Y29kZSB0byBhZGQgYSBQREYgYW5ub3RhdGlvbiB3aGVuIHByaW50aW5nIHdhcyBjYWxsZWQgZnJv bQorICAgICAgICBkcmF3Rm9jdXNSaW5nLCB3aGljaCBpcyBxdWl0ZSBjb25mdXNpbmcuIFdpdGgg anVzdCBsaXR0bGUgbW92ZW1lbnQKKyAgICAgICAgb2YgY29kZSwgd2UgY2FuIGRldGVjdCBhbiBh bm5vdGF0aW9uIGFuZCBjYWxsIGFkZFBERlVSTFJlY3QgaW4gdGhlCisgICAgICAgIHBhaW50IHBo YXNlLCBhbmQgZHJhd0ZvY3VzUmluZyBiZWNvbWVzIHNvbWV0aGluZyB0aGF0IG9ubHkKKyAgICAg ICAgZHJhd3MgZm9jdXMgcmluZ3MuCisKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVySW5saW5l LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlcklubGluZTo6cGFpbnRPdXRsaW5lKTogU3Bs aXQgY2FsbHMgdG8gZHJhd0ZvY3VzUmluZworICAgICAgICBhbmQgYWRkUERGVVJMUmVjdC4KKyAg ICAgICAgKiByZW5kZXJpbmcvUmVuZGVyT2JqZWN0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlJl bmRlck9iamVjdDo6cGFpbnRGb2N1c1JpbmcpOiBHZXQgcmlkIG9mIGNhbGwgdG8gUERGLgorICAg ICAgICAoV2ViQ29yZTo6UmVuZGVyT2JqZWN0OjphZGRQREZVUkxSZWN0KTogQ2hhbmdlIHRoZSBz aWduYXR1cmUgc28gaXQKKyAgICAgICAgY2FuIGJlIGNhbGxlZCBleHRlcm5hbGx5LCBhbmQgY2Fs Y3VsYXRlcyB0aGUgcmVjdGFuZ2xlIGl0c2VsZi4KKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlck9i amVjdDo6cGFpbnRPdXRsaW5lKTogU3BsaXQgdGhlIGNhbGxzLgorICAgICAgICAqIHJlbmRlcmlu Zy9SZW5kZXJPYmplY3QuaDoKKworMjAxNC0wNS0wNiAgRGVhbiBKYWNrc29uICA8ZGlub0BhcHBs ZS5jb20+CisKICAgICAgICAgW01hY10gQWxsb3cgZm9jdXMgcmluZ3MgdG8gcmVkcmF3IHRoZW1z ZWx2ZXMgaWYgbmVjZXNzYXJ5CiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xMzI1OTMKIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5n L1JlbmRlcklubGluZS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVySW5saW5l LmNwcAppbmRleCA3NjQ0ZDU4MjExZjYzNTg2ZTFmZTQ0NTQzMDExZjAyMjE4ZGJhMTE2Li4wODY3 ZmQxMmJhOWNiMzdlYTIzMzgxZWNkOWVjNmZmYmMwY2ZkNDI1IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVySW5saW5lLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9y ZW5kZXJpbmcvUmVuZGVySW5saW5lLmNwcApAQCAtMTQ5MiwxMiArMTQ5MiwxMSBAQCB2b2lkIFJl bmRlcklubGluZTo6cGFpbnRPdXRsaW5lKFBhaW50SW5mbyYgcGFpbnRJbmZvLCBjb25zdCBMYXlv dXRQb2ludCYgcGFpbnRPZgogICAgICAgICByZXR1cm47CiAgICAgCiAgICAgUmVuZGVyU3R5bGUm IHN0eWxlVG9Vc2UgPSBzdHlsZSgpOwotICAgIGlmIChzdHlsZVRvVXNlLm91dGxpbmVTdHlsZUlz QXV0bygpIHx8IGhhc091dGxpbmVBbm5vdGF0aW9uKCkpIHsKLSAgICAgICAgaWYgKCF0aGVtZSgp LnN1cHBvcnRzRm9jdXNSaW5nKCZzdHlsZVRvVXNlKSkgewotICAgICAgICAgICAgLy8gT25seSBw YWludCB0aGUgZm9jdXMgcmluZyBieSBoYW5kIGlmIHRoZSB0aGVtZSBpc24ndCBhYmxlIHRvIGRy YXcgdGhlIGZvY3VzIHJpbmcuCi0gICAgICAgICAgICBwYWludEZvY3VzUmluZyhwYWludEluZm8s IHBhaW50T2Zmc2V0LCAmc3R5bGVUb1VzZSk7Ci0gICAgICAgIH0KLSAgICB9CisgICAgLy8gT25s eSBwYWludCB0aGUgZm9jdXMgcmluZyBieSBoYW5kIGlmIHRoZSB0aGVtZSBpc24ndCBhYmxlIHRv IGRyYXcgaXQuCisgICAgaWYgKHN0eWxlVG9Vc2Uub3V0bGluZVN0eWxlSXNBdXRvKCkgJiYgIXRo ZW1lKCkuc3VwcG9ydHNGb2N1c1JpbmcoJnN0eWxlVG9Vc2UpKQorICAgICAgICBwYWludEZvY3Vz UmluZyhwYWludEluZm8sIHBhaW50T2Zmc2V0LCAmc3R5bGVUb1VzZSk7CisgICAgZWxzZSBpZiAo aGFzT3V0bGluZUFubm90YXRpb24oKSAmJiAhdGhlbWUoKS5zdXBwb3J0c0ZvY3VzUmluZygmc3R5 bGVUb1VzZSkpCisgICAgICAgIGFkZFBERlVSTFJlY3QocGFpbnRJbmZvLCBwYWludE9mZnNldCk7 CiAKICAgICBHcmFwaGljc0NvbnRleHQqIGdyYXBoaWNzQ29udGV4dCA9IHBhaW50SW5mby5jb250 ZXh0OwogICAgIGlmIChncmFwaGljc0NvbnRleHQtPnBhaW50aW5nRGlzYWJsZWQoKSkKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJPYmplY3QuY3BwIGIvU291cmNl L1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlck9iamVjdC5jcHAKaW5kZXggZDhlMzRmYjFiMjAzYTU4 Yjc4NmYwNzdiYTFhMTZkNTEwZWIwMWMwMy4uN2U2YjVmZTQ5NThkYjMzOTBjZTQ1OTdhMTI0OWNj YmQ5ZjNmNWNiMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlck9i amVjdC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlck9iamVjdC5jcHAK QEAgLTk5OSw5ICs5OTksOSBAQCB2b2lkIFJlbmRlck9iamVjdDo6ZHJhd0xpbmVGb3JCb3hTaWRl KEdyYXBoaWNzQ29udGV4dCogZ3JhcGhpY3NDb250ZXh0LCBmbG9hdCB4MQogCiB2b2lkIFJlbmRl ck9iamVjdDo6cGFpbnRGb2N1c1JpbmcoUGFpbnRJbmZvJiBwYWludEluZm8sIGNvbnN0IExheW91 dFBvaW50JiBwYWludE9mZnNldCwgUmVuZGVyU3R5bGUqIHN0eWxlKQogewotICAgIFZlY3RvcjxJ bnRSZWN0PiBmb2N1c1JpbmdSZWN0czsKLSAgICBhZGRGb2N1c1JpbmdSZWN0cyhmb2N1c1JpbmdS ZWN0cywgcGFpbnRPZmZzZXQsIHBhaW50SW5mby5wYWludENvbnRhaW5lcik7CiAgICAgaWYgKHN0 eWxlLT5vdXRsaW5lU3R5bGVJc0F1dG8oKSkgeworICAgICAgICBWZWN0b3I8SW50UmVjdD4gZm9j dXNSaW5nUmVjdHM7CisgICAgICAgIGFkZEZvY3VzUmluZ1JlY3RzKGZvY3VzUmluZ1JlY3RzLCBw YWludE9mZnNldCwgcGFpbnRJbmZvLnBhaW50Q29udGFpbmVyKTsKICNpZiBQTEFURk9STShNQUMp CiAgICAgICAgIGJvb2wgbmVlZHNSZXBhaW50OwogICAgICAgICBwYWludEluZm8uY29udGV4dC0+ ZHJhd0ZvY3VzUmluZyhmb2N1c1JpbmdSZWN0cywgc3R5bGUtPm91dGxpbmVXaWR0aCgpLCBzdHls ZS0+b3V0bGluZU9mZnNldCgpLCBkb2N1bWVudCgpLnBhZ2UoKS0+Zm9jdXNDb250cm9sbGVyKCku dGltZVNpbmNlRm9jdXNXYXNTZXQoKSwgbmVlZHNSZXBhaW50KTsKQEAgLTEwMTAsMTIgKzEwMTAs MTUgQEAgdm9pZCBSZW5kZXJPYmplY3Q6OnBhaW50Rm9jdXNSaW5nKFBhaW50SW5mbyYgcGFpbnRJ bmZvLCBjb25zdCBMYXlvdXRQb2ludCYgcGFpbnQKICNlbHNlCiAgICAgICAgIHBhaW50SW5mby5j b250ZXh0LT5kcmF3Rm9jdXNSaW5nKGZvY3VzUmluZ1JlY3RzLCBzdHlsZS0+b3V0bGluZVdpZHRo KCksIHN0eWxlLT5vdXRsaW5lT2Zmc2V0KCksIHN0eWxlLT52aXNpdGVkRGVwZW5kZW50Q29sb3Io Q1NTUHJvcGVydHlPdXRsaW5lQ29sb3IpKTsKICNlbmRpZgotICAgIH0gZWxzZQotICAgICAgICBh ZGRQREZVUkxSZWN0KHBhaW50SW5mby5jb250ZXh0LCB1bmlvblJlY3QoZm9jdXNSaW5nUmVjdHMp KTsKKyAgICB9CiB9CiAKLXZvaWQgUmVuZGVyT2JqZWN0OjphZGRQREZVUkxSZWN0KEdyYXBoaWNz Q29udGV4dCogY29udGV4dCwgY29uc3QgTGF5b3V0UmVjdCYgcmVjdCkKK3ZvaWQgUmVuZGVyT2Jq ZWN0OjphZGRQREZVUkxSZWN0KFBhaW50SW5mbyYgcGFpbnRJbmZvLCBjb25zdCBMYXlvdXRQb2lu dCYgcGFpbnRPZmZzZXQpCiB7CisgICAgVmVjdG9yPEludFJlY3Q+IGZvY3VzUmluZ1JlY3RzOwor ICAgIGFkZEZvY3VzUmluZ1JlY3RzKGZvY3VzUmluZ1JlY3RzLCBwYWludE9mZnNldCwgcGFpbnRJ bmZvLnBhaW50Q29udGFpbmVyKTsKKyAgICBJbnRSZWN0IHJlY3QgPSB1bmlvblJlY3QoZm9jdXNS aW5nUmVjdHMpOworCiAgICAgaWYgKHJlY3QuaXNFbXB0eSgpKQogICAgICAgICByZXR1cm47CiAg ICAgTm9kZSogbiA9IG5vZGUoKTsKQEAgLTEwMjQsNyArMTAyNyw3IEBAIHZvaWQgUmVuZGVyT2Jq ZWN0OjphZGRQREZVUkxSZWN0KEdyYXBoaWNzQ29udGV4dCogY29udGV4dCwgY29uc3QgTGF5b3V0 UmVjdCYgcmVjCiAgICAgY29uc3QgQXRvbWljU3RyaW5nJiBocmVmID0gdG9FbGVtZW50KG4pLT5n ZXRBdHRyaWJ1dGUoaHJlZkF0dHIpOwogICAgIGlmIChocmVmLmlzTnVsbCgpKQogICAgICAgICBy ZXR1cm47Ci0gICAgY29udGV4dC0+c2V0VVJMRm9yUmVjdChuLT5kb2N1bWVudCgpLmNvbXBsZXRl VVJMKGhyZWYpLCBwaXhlbFNuYXBwZWRJbnRSZWN0KHJlY3QpKTsKKyAgICBwYWludEluZm8uY29u dGV4dC0+c2V0VVJMRm9yUmVjdChuLT5kb2N1bWVudCgpLmNvbXBsZXRlVVJMKGhyZWYpLCBwaXhl bFNuYXBwZWRJbnRSZWN0KHJlY3QpKTsKIH0KIAogdm9pZCBSZW5kZXJPYmplY3Q6OnBhaW50T3V0 bGluZShQYWludEluZm8mIHBhaW50SW5mbywgY29uc3QgTGF5b3V0UmVjdCYgcGFpbnRSZWN0KQpA QCAtMTAzNywxMiArMTA0MCwxMSBAQCB2b2lkIFJlbmRlck9iamVjdDo6cGFpbnRPdXRsaW5lKFBh aW50SW5mbyYgcGFpbnRJbmZvLCBjb25zdCBMYXlvdXRSZWN0JiBwYWludFJlYwogCiAgICAgaW50 IG91dGxpbmVPZmZzZXQgPSBzdHlsZVRvVXNlLm91dGxpbmVPZmZzZXQoKTsKIAotICAgIGlmIChz dHlsZVRvVXNlLm91dGxpbmVTdHlsZUlzQXV0bygpIHx8IGhhc091dGxpbmVBbm5vdGF0aW9uKCkp IHsKLSAgICAgICAgaWYgKCF0aGVtZSgpLnN1cHBvcnRzRm9jdXNSaW5nKCZzdHlsZVRvVXNlKSkg ewotICAgICAgICAgICAgLy8gT25seSBwYWludCB0aGUgZm9jdXMgcmluZyBieSBoYW5kIGlmIHRo ZSB0aGVtZSBpc24ndCBhYmxlIHRvIGRyYXcgdGhlIGZvY3VzIHJpbmcuCi0gICAgICAgICAgICBw YWludEZvY3VzUmluZyhwYWludEluZm8sIHBhaW50UmVjdC5sb2NhdGlvbigpLCAmc3R5bGVUb1Vz ZSk7Ci0gICAgICAgIH0KLSAgICB9CisgICAgLy8gT25seSBwYWludCB0aGUgZm9jdXMgcmluZyBi eSBoYW5kIGlmIHRoZSB0aGVtZSBpc24ndCBhYmxlIHRvIGRyYXcgaXQuCisgICAgaWYgKHN0eWxl VG9Vc2Uub3V0bGluZVN0eWxlSXNBdXRvKCkgJiYgIXRoZW1lKCkuc3VwcG9ydHNGb2N1c1Jpbmco JnN0eWxlVG9Vc2UpKQorICAgICAgICBwYWludEZvY3VzUmluZyhwYWludEluZm8sIHBhaW50UmVj dC5sb2NhdGlvbigpLCAmc3R5bGVUb1VzZSk7CisgICAgZWxzZSBpZiAoaGFzT3V0bGluZUFubm90 YXRpb24oKSAmJiAhdGhlbWUoKS5zdXBwb3J0c0ZvY3VzUmluZygmc3R5bGVUb1VzZSkpCisgICAg ICAgIGFkZFBERlVSTFJlY3QocGFpbnRJbmZvLCBwYWludFJlY3QubG9jYXRpb24oKSk7CiAKICAg ICBpZiAoc3R5bGVUb1VzZS5vdXRsaW5lU3R5bGVJc0F1dG8oKSB8fCBzdHlsZVRvVXNlLm91dGxp bmVTdHlsZSgpID09IEJOT05FKQogICAgICAgICByZXR1cm47CmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyT2JqZWN0LmggYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJp bmcvUmVuZGVyT2JqZWN0LmgKaW5kZXggYmM3YTFhMWJmODNhYjY2MmEwNTEyNGM3MTBhMDkzYjgx M2ZiOGVkNy4uMDExMmM0ZjUwYjI4YjI2NzViYjU0ZjBjYzdmMGZkZmRkMzVhMjlhMiAxMDA2NDQK LS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlck9iamVjdC5oCisrKyBiL1NvdXJj ZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJPYmplY3QuaApAQCAtODcxLDcgKzg3MSw3IEBAIHB1 YmxpYzoKIHByb3RlY3RlZDoKICAgICB2b2lkIHBhaW50Rm9jdXNSaW5nKFBhaW50SW5mbyYsIGNv bnN0IExheW91dFBvaW50JiwgUmVuZGVyU3R5bGUqKTsKICAgICB2b2lkIHBhaW50T3V0bGluZShQ YWludEluZm8mLCBjb25zdCBMYXlvdXRSZWN0Jik7Ci0gICAgdm9pZCBhZGRQREZVUkxSZWN0KEdy YXBoaWNzQ29udGV4dCosIGNvbnN0IExheW91dFJlY3QmKTsKKyAgICB2b2lkIGFkZFBERlVSTFJl Y3QoUGFpbnRJbmZvJiwgY29uc3QgTGF5b3V0UG9pbnQmKTsKICAgICBOb2RlJiBub2RlRm9yTm9u QW5vbnltb3VzKCkgY29uc3QgeyBBU1NFUlQoIWlzQW5vbnltb3VzKCkpOyByZXR1cm4gbV9ub2Rl OyB9CiAKICAgICB2b2lkIGFkanVzdFJlY3RGb3JPdXRsaW5lQW5kU2hhZG93KExheW91dFJlY3Qm KSBjb25zdDsK