50226 2010-11-30 05:10:22 -0800 [GTK] Media icons are loaded again every time style-set signal is emitted on every widget 2010-12-07 04:43:57 -0800 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) PC Linux RESOLVED DUPLICATE 50623 P2 Normal --- 0 cgarcia webkit-unassigned mrobinson pnormand oldest_to_newest 314959 0 cgarcia 2010-11-30 05:10:22 -0800 We should change the colors when style-set is emitted, and load the icons only when the icon-theme changes. 314961 1 75126 cgarcia 2010-11-30 05:15:57 -0800 Created attachment 75126 Patch that fixes the issue This patch depends on the one attached to bug #50225 315622 2 75126 mrobinson 2010-12-01 06:55:46 -0800 Comment on attachment 75126 Patch that fixes the issue View in context: https://bugs.webkit.org/attachment.cgi?id=75126&action=review Awesome patch. Just a couple issues outlined below. > WebCore/platform/gtk/RenderThemeGtk.cpp:199 > + g_signal_connect(gtk_icon_theme_get_default(), "changed", G_CALLBACK(gtkIconThemeChanged), const_cast<RenderThemeGtk*>(this)); Do you really need a const_cast here? > WebCore/platform/gtk/RenderThemeGtk.cpp:794 > + initMediaStyling(true); This also reinitializes the media colors when the icon theme changes. Perhaps it would make sense to have two methods: initMediaColors and initMediaIcons. > WebCore/platform/gtk/RenderThemeGtk.h:142 > + virtual void initMediaColors(GtkStyle*); > virtual void initMediaStyling(bool force); Why are these virtual? Does any subclass override them? 315626 3 cgarcia 2010-12-01 07:08:54 -0800 (In reply to comment #2) > (From update of attachment 75126 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75126&action=review > > Awesome patch. Just a couple issues outlined below. > > > WebCore/platform/gtk/RenderThemeGtk.cpp:199 > > + g_signal_connect(gtk_icon_theme_get_default(), "changed", G_CALLBACK(gtkIconThemeChanged), const_cast<RenderThemeGtk*>(this)); > > Do you really need a const_cast here? No idea, I copy-pasted from the "style-set" signal connection. > > WebCore/platform/gtk/RenderThemeGtk.cpp:794 > > + initMediaStyling(true); > > This also reinitializes the media colors when the icon theme changes. Perhaps it would make sense to have two methods: initMediaColors and initMediaIcons. Updating the colors is cheap, so I thought it didn't hurt. > > WebCore/platform/gtk/RenderThemeGtk.h:142 > > + virtual void initMediaColors(GtkStyle*); > > virtual void initMediaStyling(bool force); > > Why are these virtual? Does any subclass override them? No idea, initMediaStyling was already virtual and I added initMediaColors based on it. 318132 4 cgarcia 2010-12-07 04:43:57 -0800 *** This bug has been marked as a duplicate of bug 50623 *** 75126 2010-11-30 05:15:57 -0800 2010-12-01 06:55:45 -0800 Patch that fixes the issue media-icon-theme-changes.diff text/plain 4064 cgarcia ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg N2Y0M2U5ZC4uYWJkMDNmMCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMyBAQAorMjAxMC0xMS0zMCAgQ2FybG9zIEdhcmNp YSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgW0dUS10gTWVkaWEgaWNvbnMgYXJlIGxvYWRlZCBhZ2Fp biBldmVyeSB0aW1lIHN0eWxlLXNldCBzaWduYWwgaXMgZW1pdHRlZCBvbiBldmVyeSB3aWRnZXQK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUwMjI2CisK KyAgICAgICAgSW5zdGVhZCBvZiB1cGRhdGluZyB0aGUgaWNvbnMgZXZlcnkgdGltZSBzdHlsZS1z ZXQgaXMgZW1pdHRlZCwgd2UKKyAgICAgICAgc2hvdWxkIG9ubHkgY2hhbmdlIHRoZSBjb2xvcnMs IGFuZCByZS1sb2FkIHRoZSBpY29ucyB3aGVuIGljb24KKyAgICAgICAgdGhlbWUgY2hhbmdlcy4K KworICAgICAgICAqIHBsYXRmb3JtL2d0ay9SZW5kZXJUaGVtZUd0ay5jcHA6CisgICAgICAgIChX ZWJDb3JlOjpSZW5kZXJUaGVtZUd0azo6aW5pdE1lZGlhQ29sb3JzKToKKyAgICAgICAgKFdlYkNv cmU6OlJlbmRlclRoZW1lR3RrOjppbml0TWVkaWFTdHlsaW5nKToKKyAgICAgICAgKFdlYkNvcmU6 Omd0a0ljb25UaGVtZUNoYW5nZWQpOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyVGhlbWVHdGs6 OlJlbmRlclRoZW1lR3RrKToKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlclRoZW1lR3RrOjpwbGF0 Zm9ybUNvbG9yc0RpZENoYW5nZSk6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJUaGVtZUd0azo6 dGhlbWVDaGFuZ2VkKToKKyAgICAgICAgKiBwbGF0Zm9ybS9ndGsvUmVuZGVyVGhlbWVHdGsuaDoK KwogMjAxMC0xMS0yOSAgRGFuIEJlcm5zdGVpbiAgPG1pdHpAYXBwbGUuY29tPgogCiAgICAgICAg IEJ1aWxkIGZpeCBmb3Igbm9uLUlDVSBwbGF0Zm9ybXMgYWZ0ZXIgcjcyODg3LgpkaWZmIC0tZ2l0 IGEvV2ViQ29yZS9wbGF0Zm9ybS9ndGsvUmVuZGVyVGhlbWVHdGsuY3BwIGIvV2ViQ29yZS9wbGF0 Zm9ybS9ndGsvUmVuZGVyVGhlbWVHdGsuY3BwCmluZGV4IDUxNzExMDYuLmE2NjVjYzcgMTAwNjQ0 Ci0tLSBhL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL1JlbmRlclRoZW1lR3RrLmNwcAorKysgYi9XZWJD b3JlL3BsYXRmb3JtL2d0ay9SZW5kZXJUaGVtZUd0ay5jcHAKQEAgLTkxLDE3ICs5MSwyMCBAQCBz dGF0aWMgUGFzc1JlZlB0cjxJbWFnZT4gbG9hZFN0b2NrSWNvbihHdGtXaWRnZXQqIHdpZGdldCwg Y29uc3QgZ2NoYXIgKmljb25OYW1lLAogICAgIHJldHVybiBpbWcucmVsZWFzZSgpOwogfQogCit2 b2lkIFJlbmRlclRoZW1lR3RrOjppbml0TWVkaWFDb2xvcnMoR3RrU3R5bGUqIHN0eWxlKQorewor ICAgIG1fcGFuZWxDb2xvciA9IHN0eWxlLT5iZ1tHVEtfU1RBVEVfTk9STUFMXTsKKyAgICBtX3Ns aWRlckNvbG9yID0gc3R5bGUtPmJnW0dUS19TVEFURV9BQ1RJVkVdOworICAgIG1fc2xpZGVyVGh1 bWJDb2xvciA9IHN0eWxlLT5iZ1tHVEtfU1RBVEVfU0VMRUNURURdOworfQorCiB2b2lkIFJlbmRl clRoZW1lR3RrOjppbml0TWVkaWFTdHlsaW5nKGJvb2wgZm9yY2UpCiB7CiAgICAgc3RhdGljIGJv b2wgc3R5bGluZ0luaXRpYWxpemVkID0gZmFsc2U7CiAKICAgICBpZiAoIXN0eWxpbmdJbml0aWFs aXplZCB8fCBmb3JjZSkgewogICAgICAgICBHdGtXaWRnZXQqIHdpZGdldCA9IEdUS19XSURHRVQo Z3RrQ29udGFpbmVyKCkpOwotICAgICAgICBHdGtTdHlsZSogc3R5bGUgPSBndGtfd2lkZ2V0X2dl dF9zdHlsZSh3aWRnZXQpOwotCi0gICAgICAgIG1fcGFuZWxDb2xvciA9IHN0eWxlLT5iZ1tHVEtf U1RBVEVfTk9STUFMXTsKLSAgICAgICAgbV9zbGlkZXJDb2xvciA9IHN0eWxlLT5iZ1tHVEtfU1RB VEVfQUNUSVZFXTsKLSAgICAgICAgbV9zbGlkZXJUaHVtYkNvbG9yID0gc3R5bGUtPmJnW0dUS19T VEFURV9TRUxFQ1RFRF07CisgICAgICAgIGluaXRNZWRpYUNvbG9ycyhndGtfd2lkZ2V0X2dldF9z dHlsZSh3aWRnZXQpKTsKIAogICAgICAgICBtX2Z1bGxzY3JlZW5CdXR0b24uY2xlYXIoKTsKICAg ICAgICAgbV9tdXRlQnV0dG9uLmNsZWFyKCk7CkBAIC0xMzAsNiArMTMzLDExIEBAIHZvaWQgUmVu ZGVyVGhlbWVHdGs6OmluaXRNZWRpYVN0eWxpbmcoYm9vbCBmb3JjZSkKICAgICAgICAgc3R5bGlu Z0luaXRpYWxpemVkID0gdHJ1ZTsKICAgICB9CiB9CisKK3N0YXRpYyB2b2lkIGd0a0ljb25UaGVt ZUNoYW5nZWQoR3RrSWNvblRoZW1lKiwgUmVuZGVyVGhlbWUqIHJlbmRlclRoZW1lKQoreworICAg IHJlbmRlclRoZW1lLT50aGVtZUNoYW5nZWQoKTsKK30KICNlbmRpZgogCiBQYXNzUmVmUHRyPFJl bmRlclRoZW1lPiBSZW5kZXJUaGVtZUd0azo6Y3JlYXRlKCkKQEAgLTE4OCw2ICsxOTYsOCBAQCBS ZW5kZXJUaGVtZUd0azo6UmVuZGVyVGhlbWVHdGsoKQogICAgICsrbW96R3RrUmVmQ291bnQ7CiAK ICNpZiBFTkFCTEUoVklERU8pCisgICAgZ19zaWduYWxfY29ubmVjdChndGtfaWNvbl90aGVtZV9n ZXRfZGVmYXVsdCgpLCAiY2hhbmdlZCIsIEdfQ0FMTEJBQ0soZ3RrSWNvblRoZW1lQ2hhbmdlZCks IGNvbnN0X2Nhc3Q8UmVuZGVyVGhlbWVHdGsqPih0aGlzKSk7CisKICAgICBpbml0TWVkaWFTdHls aW5nKGZhbHNlKTsKICNlbmRpZgogfQpAQCAtNzczLDExICs3ODMsMTkgQEAgR3RrV2lkZ2V0KiBS ZW5kZXJUaGVtZUd0azo6Z3RrU2Nyb2xsYmFyKCkKIHZvaWQgUmVuZGVyVGhlbWVHdGs6OnBsYXRm b3JtQ29sb3JzRGlkQ2hhbmdlKCkKIHsKICNpZiBFTkFCTEUoVklERU8pCi0gICAgaW5pdE1lZGlh U3R5bGluZyh0cnVlKTsKKyAgICBpbml0TWVkaWFDb2xvcnMoZ3RrX3dpZGdldF9nZXRfc3R5bGUo R1RLX1dJREdFVChndGtDb250YWluZXIoKSkpKTsKICNlbmRpZgogICAgIFJlbmRlclRoZW1lOjpw bGF0Zm9ybUNvbG9yc0RpZENoYW5nZSgpOwogfQogCit2b2lkIFJlbmRlclRoZW1lR3RrOjp0aGVt ZUNoYW5nZWQoKQoreworI2lmIEVOQUJMRShWSURFTykKKyAgICBpbml0TWVkaWFTdHlsaW5nKHRy dWUpOworI2VuZGlmCisgICAgUmVuZGVyVGhlbWU6OnRoZW1lQ2hhbmdlZCgpOworfQorCiAjaWYg RU5BQkxFKFZJREVPKQogU3RyaW5nIFJlbmRlclRoZW1lR3RrOjpleHRyYU1lZGlhQ29udHJvbHNT dHlsZVNoZWV0KCkKIHsKZGlmZiAtLWdpdCBhL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL1JlbmRlclRo ZW1lR3RrLmggYi9XZWJDb3JlL3BsYXRmb3JtL2d0ay9SZW5kZXJUaGVtZUd0ay5oCmluZGV4IGQ0 YTlkYTMuLmNlODkyZmUgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL1JlbmRlclRo ZW1lR3RrLmgKKysrIGIvV2ViQ29yZS9wbGF0Zm9ybS9ndGsvUmVuZGVyVGhlbWVHdGsuaApAQCAt NzMsNiArNzMsNyBAQCBwdWJsaWM6CiAKICAgICB2aXJ0dWFsIGRvdWJsZSBjYXJldEJsaW5rSW50 ZXJ2YWwoKSBjb25zdDsKIAorICAgIHZpcnR1YWwgdm9pZCB0aGVtZUNoYW5nZWQoKTsKICAgICB2 aXJ0dWFsIHZvaWQgcGxhdGZvcm1Db2xvcnNEaWRDaGFuZ2UoKTsKIAogICAgIC8vIFN5c3RlbSBm b250cyBhbmQgY29sb3JzLgpAQCAtMTM3LDYgKzEzOCw3IEBAIHByb3RlY3RlZDoKICAgICB2aXJ0 dWFsIHZvaWQgYWRqdXN0U2xpZGVyVGh1bWJTaXplKFJlbmRlck9iamVjdCogb2JqZWN0KSBjb25z dDsKIAogI2lmIEVOQUJMRShWSURFTykKKyAgICB2aXJ0dWFsIHZvaWQgaW5pdE1lZGlhQ29sb3Jz KEd0a1N0eWxlKik7CiAgICAgdmlydHVhbCB2b2lkIGluaXRNZWRpYVN0eWxpbmcoYm9vbCBmb3Jj ZSk7CiAgICAgdmlydHVhbCBib29sIHBhaW50TWVkaWFGdWxsc2NyZWVuQnV0dG9uKFJlbmRlck9i amVjdCosIGNvbnN0IFBhaW50SW5mbyYsIGNvbnN0IEludFJlY3QmKTsKICAgICB2aXJ0dWFsIGJv b2wgcGFpbnRNZWRpYVBsYXlCdXR0b24oUmVuZGVyT2JqZWN0KiwgY29uc3QgUGFpbnRJbmZvJiwg Y29uc3QgSW50UmVjdCYpOwo=