21844 2008-10-23 17:47:56 -0700 Remove setFocusRingColorChangeFunction() 2008-10-24 13:43:30 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC All RESOLVED FIXED P2 Minor --- 1 pkasting pkasting oldest_to_newest 96457 0 pkasting 2008-10-23 17:47:56 -0700 setFocusRingColorChangeFunction() seems totally useless, as far as I can tell. Only the Mac port implements it at all. In ColorMac.mm, it sets a file-static variable -- which is then never used by anyone. 96470 1 24630 pkasting 2008-10-23 18:34:26 -0700 Created attachment 24630 patch v1 Removing this function causes the Mac port to no longer hook up an observer until focusRingColor() is called. I don't think that can have a practical effect, since focusRingColor() is the only way callers can actually get the color the observer sets. If anything, this makes the code (insignificantly) more efficient. 96517 2 pkasting 2008-10-24 10:07:41 -0700 I've been thinking about this code more, and I'm not sure this patch is precisely right. It maintains what the current code does, but I think the current code may be broken. There are two questions at stake: (1) Is the custom CSS style to get the focus ring color used by any consumers? (2) Should this style and/or the focus ring itself redraw immediately in response to system theme changes, or can such changes only happen when the browser isn't running? If (1) is "no", then focusRingColor() can disappear and the remaining code can move into the GraphicsContext code that draws the focus ring. If (2) is "yes", then instead of removing setFocusRingColorChangeFunction(), the color change observer needs to be updated to call that function, and either that function or some other function the observer calls needs to redraw the focus ring. Only if the answers are, respectively, "yes" and "no" is this patch precisely what we want. 96523 3 pkasting 2008-10-24 10:49:23 -0700 On IRC, Maciej notes that the default focus ring is just a CSS rule: outline: auto 5px -webkit-focus-ring-color In this case, the answer to (1) is definitely "yes". But it makes me wonder why GraphicsContext::drawFocusRing() is needed if the focus ring is just a CSS outline. Maybe one is used in some scenarios and one in others? 96530 4 24630 hyatt 2008-10-24 11:29:18 -0700 Comment on attachment 24630 patch v1 r=me, should go even further and move focusRingColor stuff into the Theme eventually. This should wait until i have theme refactored though. :) 96567 5 pkasting 2008-10-24 13:43:30 -0700 Fixed in r37858. 24630 2008-10-23 18:34:26 -0700 2008-10-24 11:29:18 -0700 patch v1 patch text/plain 4620 pkasting SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAzNzgzNCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTggQEAKKzIwMDgtMTAtMjMgIFBldGVyIEthc3RpbmcgIDxwa2FzdGluZ0Bnb29n bGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMTg0NAorICAgICAgICBS ZW1vdmUgc2V0Rm9jdXNSaW5nQ29sb3JDaGFuZ2VGdW5jdGlvbi4KKworICAgICAgICAqIHBhZ2Uv UGFnZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpQYWdlOjpQYWdlKToKKyAgICAgICAgKiBwbGF0 Zm9ybS9ncmFwaGljcy9Db2xvci5oOgorICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL21hYy9D b2xvck1hYy5tbToKKyAgICAgICAgKFdlYkNvcmU6OmZvY3VzUmluZ0NvbG9yKToKKyAgICAgICAg KiBwbGF0Zm9ybS9ncmFwaGljcy9xdC9HcmFwaGljc0NvbnRleHRRdC5jcHA6CisgICAgICAgICog cGxhdGZvcm0vZ3JhcGhpY3Mvd2luL0NvbG9yU2FmYXJpLmNwcDoKKwogMjAwOC0xMC0yMyAgRGFy aW4gRmlzaGVyICA8ZGFyaW5AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEVy aWMgU2VpZGVsLgpJbmRleDogV2ViQ29yZS9wYWdlL1BhZ2UuY3BwCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdl YkNvcmUvcGFnZS9QYWdlLmNwcAkocmV2aXNpb24gMzc4MzQpCisrKyBXZWJDb3JlL3BhZ2UvUGFn ZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTEzMCw3ICsxMzAsNiBAQCBQYWdlOjpQYWdlKENocm9t ZUNsaWVudCogY2hyb21lQ2xpZW50LCBDCiB7CiAgICAgaWYgKCFhbGxQYWdlcykgewogICAgICAg ICBhbGxQYWdlcyA9IG5ldyBIYXNoU2V0PFBhZ2UqPjsKLSAgICAgICAgc2V0Rm9jdXNSaW5nQ29s b3JDaGFuZ2VGdW5jdGlvbihzZXROZWVkc1JlYXBwbHlTdHlsZXMpOwogICAgICAgICAKICAgICAg ICAgbmV0d29ya1N0YXRlTm90aWZpZXIoKS5zZXROZXR3b3JrU3RhdGVDaGFuZ2VkRnVuY3Rpb24o bmV0d29ya1N0YXRlQ2hhbmdlZCk7CiAgICAgfQpJbmRleDogV2ViQ29yZS9wbGF0Zm9ybS9ncmFw aGljcy9Db2xvci5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvQ29s b3IuaAkocmV2aXNpb24gMzc4MzQpCisrKyBXZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0NvbG9y LmgJKHdvcmtpbmcgY29weSkKQEAgLTE0Miw3ICsxNDIsNiBAQCBpbmxpbmUgYm9vbCBvcGVyYXRv ciE9KGNvbnN0IENvbG9yJiBhLCBjCiB9CiAKIENvbG9yIGZvY3VzUmluZ0NvbG9yKCk7Ci12b2lk IHNldEZvY3VzUmluZ0NvbG9yQ2hhbmdlRnVuY3Rpb24odm9pZCAoKikoKSk7CiAKICNpZiBQTEFU Rk9STShDRykKIENHQ29sb3JSZWYgY2dDb2xvcihjb25zdCBDb2xvciYpOwpJbmRleDogV2ViQ29y ZS9wbGF0Zm9ybS9ncmFwaGljcy9tYWMvQ29sb3JNYWMubW0KPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29y ZS9wbGF0Zm9ybS9ncmFwaGljcy9tYWMvQ29sb3JNYWMubW0JKHJldmlzaW9uIDM3ODM0KQorKysg V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9tYWMvQ29sb3JNYWMubW0JKHdvcmtpbmcgY29weSkK QEAgLTM5LDggKzM5LDYgQEAgbmFtZXNwYWNlIFdlYkNvcmUgewogLy8gTlNDb2xvciBjYWxscyBk b24ndCB0aHJvdywgc28gbm8gbmVlZCB0byBibG9jayBDb2NvYSBleGNlcHRpb25zIGluIHRoaXMg ZmlsZQogCiBzdGF0aWMgUkdCQTMyIG9sZEFxdWFGb2N1c1JpbmdDb2xvciA9IDB4RkY3REFERDk7 Ci1zdGF0aWMgYm9vbCB0aW50SXNLbm93bjsKLXN0YXRpYyB2b2lkICgqdGludENoYW5nZUZ1bmN0 aW9uKSgpOwogc3RhdGljIFJHQkEzMiBzeXN0ZW1Gb2N1c1JpbmdDb2xvcjsKIHN0YXRpYyBib29s IHVzZU9sZEFxdWFGb2N1c1JpbmdDb2xvcjsKIApAQCAtMTI2LDI5ICsxMjQsMTcgQEAgQ0dDb2xv clJlZiBjZ0NvbG9yKGNvbnN0IENvbG9yJiBjKQogICAgIHJldHVybiBDR0NvbG9yRnJvbU5TQ29s b3IobnNDb2xvcihjKSk7CiB9CiAKLXN0YXRpYyB2b2lkIG9ic2VydmVUaW50KCkKLXsKLSAgICBB U1NFUlQoIXRpbnRJc0tub3duKTsKLSAgICBbW05TTm90aWZpY2F0aW9uQ2VudGVyIGRlZmF1bHRD ZW50ZXJdIGFkZE9ic2VydmVyOltXZWJDb3JlQ29udHJvbFRpbnRPYnNlcnZlciBjbGFzc10KLSAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHNlbGVjdG9yOkBzZWxl Y3Rvcihjb250cm9sVGludERpZENoYW5nZSkKLSAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICBuYW1lOk5TQ29udHJvbFRpbnREaWRDaGFuZ2VOb3RpZmljYXRp b24KLSAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgb2JqZWN0 Ok5TQXBwXTsKLSAgICBbV2ViQ29yZUNvbnRyb2xUaW50T2JzZXJ2ZXIgY29udHJvbFRpbnREaWRD aGFuZ2VdOwotICAgIHRpbnRJc0tub3duID0gdHJ1ZTsKLX0KLQotdm9pZCBzZXRGb2N1c1JpbmdD b2xvckNoYW5nZUZ1bmN0aW9uKHZvaWQgKCpmdW5jdGlvbikoKSkKLXsKLSAgICBBU1NFUlQoIXRp bnRDaGFuZ2VGdW5jdGlvbik7Ci0gICAgdGludENoYW5nZUZ1bmN0aW9uID0gZnVuY3Rpb247Ci0g ICAgaWYgKCF0aW50SXNLbm93bikKLSAgICAgICAgb2JzZXJ2ZVRpbnQoKTsKLX0KLQogQ29sb3Ig Zm9jdXNSaW5nQ29sb3IoKQogewotICAgIGlmICghdGludElzS25vd24pCi0gICAgICAgIG9ic2Vy dmVUaW50KCk7CisgICAgc3RhdGljIGJvb2wgdGludElzS25vd24gPSBmYWxzZTsKKyAgICBpZiAo IXRpbnRJc0tub3duKSB7CisgICAgICAgIFtbTlNOb3RpZmljYXRpb25DZW50ZXIgZGVmYXVsdENl bnRlcl0gYWRkT2JzZXJ2ZXI6W1dlYkNvcmVDb250cm9sVGludE9ic2VydmVyIGNsYXNzXQorICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHNlbGVjdG9yOkBz ZWxlY3Rvcihjb250cm9sVGludERpZENoYW5nZSkKKyAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgbmFtZTpOU0NvbnRyb2xUaW50RGlkQ2hhbmdlTm90 aWZpY2F0aW9uCisgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICBvYmplY3Q6TlNBcHBdOworICAgICAgICBbV2ViQ29yZUNvbnRyb2xUaW50T2JzZXJ2ZXIg Y29udHJvbFRpbnREaWRDaGFuZ2VdOworICAgICAgICB0aW50SXNLbm93biA9IHRydWU7CisgICAg fQogICAgIAogICAgIGlmICh1c2VzVGVzdE1vZGVGb2N1c1JpbmdDb2xvcigpKQogICAgICAgICBy ZXR1cm4gb2xkQXF1YUZvY3VzUmluZ0NvbG9yOwpJbmRleDogV2ViQ29yZS9wbGF0Zm9ybS9ncmFw aGljcy9xdC9HcmFwaGljc0NvbnRleHRRdC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wbGF0 Zm9ybS9ncmFwaGljcy9xdC9HcmFwaGljc0NvbnRleHRRdC5jcHAJKHJldmlzaW9uIDM3ODM0KQor KysgV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9xdC9HcmFwaGljc0NvbnRleHRRdC5jcHAJKHdv cmtpbmcgY29weSkKQEAgLTY1MCw3ICs2NTAsNiBAQCB2b2lkIEdyYXBoaWNzQ29udGV4dDo6Y2xp cChjb25zdCBGbG9hdFJlCiAgKiBSZW5kZXJUaGVtZSBoYW5kbGVzIGRyYXdpbmcgZm9jdXMgb24g d2lkZ2V0cyB3aGljaCAKICAqIG5lZWQgaXQuCiAgKi8KLXZvaWQgc2V0Rm9jdXNSaW5nQ29sb3JD aGFuZ2VGdW5jdGlvbih2b2lkICgqKSgpKSB7IH0KIENvbG9yIGZvY3VzUmluZ0NvbG9yKCkgeyBy ZXR1cm4gQ29sb3IoMCwgMCwgMCk7IH0KIHZvaWQgR3JhcGhpY3NDb250ZXh0OjpkcmF3Rm9jdXNS aW5nKGNvbnN0IENvbG9yJiBjb2xvcikKIHsKSW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhp Y3Mvd2luL0NvbG9yU2FmYXJpLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL3BsYXRmb3JtL2dy YXBoaWNzL3dpbi9Db2xvclNhZmFyaS5jcHAJKHJldmlzaW9uIDM3ODM0KQorKysgV2ViQ29yZS9w bGF0Zm9ybS9ncmFwaGljcy93aW4vQ29sb3JTYWZhcmkuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02 OCw5ICs2OCw0IEBAIENvbG9yIGZvY3VzUmluZ0NvbG9yKCkKICAgICByZXR1cm4gZm9jdXNSaW5n Q29sb3I7CiB9CiAKLXZvaWQgc2V0Rm9jdXNSaW5nQ29sb3JDaGFuZ2VGdW5jdGlvbih2b2lkICgq KSgpKQotewotICAgIG5vdEltcGxlbWVudGVkKCk7Ci19Ci0KIH0gLy8gbmFtZXNwYWNlIFdlYkNv cmUK