Bug 13304 - REGRESSION(14784): WebKit doesn't dynamically reflect changes to the highlight color in System Preferences
Summary: REGRESSION(14784): WebKit doesn't dynamically reflect changes to the highligh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-04-08 03:56 PDT by mitz
Modified: 2007-05-05 22:39 PDT (History)
1 user (show)

See Also:


Attachments
proposed fix (6.69 KB, patch)
2007-05-05 07:49 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff
proposed fix (12.05 KB, patch)
2007-05-05 09:44 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2007-04-08 03:56:04 PDT
Changing the highlight color in System Preferences doesn't affect an existing instance of WebKit (that has already used highlighting). RenderTheme caches the highlight color the first time it's accessed and there's no invalidation mechanism.
Comment 1 Alexey Proskuryakov 2007-05-01 03:42:01 PDT
Isn't this a regression? Works for me with shipping Safari/WebKit.
Comment 2 mitz 2007-05-01 05:07:23 PDT
Yes, it is a regression. Thanks, Alexey!
Comment 3 Darin Adler 2007-05-04 22:18:09 PDT
<rdar://problem/5183686>
Comment 4 mitz 2007-05-05 00:00:43 PDT
The bug is not limited to selection in form controls.
Comment 5 Alexey Proskuryakov 2007-05-05 07:49:26 PDT
Created attachment 14356 [details]
proposed fix
Comment 6 Darin Adler 2007-05-05 08:08:06 PDT
Comment on attachment 14356 [details]
proposed fix

+    /* Have to just leak the cells, since statics are destroyed with no autorelease pool available */

I know you just moved this comment, but it is not true. You can create an autorelease pool of your own and then call HardRelease before draining the autorelease pool. In fact, even removeObserver: needs an autorelease pool.

On the other hand, I think we should just change it so that RenderThemeMac is never destroyed.

    RenderTheme* theme()
    {
        static RenderThemeMac* macTheme = new RenderThemeMac;
        return macTheme;
    }

r=me, because this issue is a minor one as long as you aren't seeing a crash on quit
Comment 7 Alexey Proskuryakov 2007-05-05 09:44:58 PDT
Created attachment 14358 [details]
proposed fix

I didn't get any crashes on quit, but it looks safer to change theme() in this way indeed.
Comment 8 Darin Adler 2007-05-05 15:15:29 PDT
Comment on attachment 14358 [details]
proposed fix

+    static RenderThemeMac* macTheme;
+    if (!macTheme)
+        macTheme = new RenderThemeMac;

This could just be written:

    static RenderThemeMac* macTheme = new RenderThemeMac;

The initialization only runs once.

r=me
Comment 9 Alexey Proskuryakov 2007-05-05 22:39:21 PDT
Committed revision 21274.