Fix NTP so that it updates on theme change....

Fix NTP so that it updates on theme change.

BUG= http://crbug.com/20392
TEST= Open several windows and new tabs.  Change theme.  Note that NTP background images all update correctly.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27841

[Mark Mentovai, 2009-10-01 23:22:42]

LGTM.  The comment should not hold this up, because even if there is a leak,
it's an existing one that's not really being made substantially worse by this
change.

http://codereview.chromium.org/243067/diff/4001/5001
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/243067/diff/4001/5001#newcode1723
Line 1723: new DOMUIThemeSource(GetProfile())));
I remember having this question when I changed this code up in the first place,
and I guess I never really looked for the answer like I meant to.  Do we know
what happens with the DOMUIThemeSource object?  Is it leaked?  Does it just
replace the one that was previously in effect?

http://codereview.chromium.org/243067/diff/4001/5002
File chrome/browser/dom_ui/new_tab_ui.h (right):

http://codereview.chromium.org/243067/diff/4001/5002#newcode43
Line 43: // Reset the CSS caches on theme change and startup.
"on theme change and startup" doesn't belong here.  Sure, that's when this will
be used, but this is the header - this is where you should be documenting the
interface, not a may-or-may-not-be-complete list of use cases.

[Miranda Callahan, 2009-10-01 23:37:53]

http://codereview.chromium.org/243067/diff/4001/5001
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/243067/diff/4001/5001#newcode1723
Line 1723: new DOMUIThemeSource(GetProfile())));
On 2009/10/01 23:22:42, Mark Mentovai wrote:
> I remember having this question when I changed this code up in the first
place,
> and I guess I never really looked for the answer like I meant to.  Do we know
> what happens with the DOMUIThemeSource object?  Is it leaked?  Does it just
> replace the one that was previously in effect?

Well, it seems to replace the one that was previously in effect, which leaks the
old one -- this wouldn't be hard to fix, I think, if the chrome_url_data_manager
owns the data in its data_sources_ map.  On the other hand, there's a
TODO(jackson) sitting there that says "A new data source with the same name
should not clobber the old one," so a different fix seems to be on somebody's
plate.  I will leave it, for now.

http://codereview.chromium.org/243067/diff/4001/5002
File chrome/browser/dom_ui/new_tab_ui.h (right):

http://codereview.chromium.org/243067/diff/4001/5002#newcode43
Line 43: // Reset the CSS caches on theme change and startup.
On 2009/10/01 23:22:42, Mark Mentovai wrote:
> "on theme change and startup" doesn't belong here.  Sure, that's when this
will
> be used, but this is the header - this is where you should be documenting the
> interface, not a may-or-may-not-be-complete list of use cases.

Done.