[WIP] Fire a theme changed notification when device scan completes

[WIP] Fire a theme changed notification when device scan completes

WIP, not for formal review.

For Chrome OS, if the --top-chrome-md flag is set
to 'Default', then the MaterialDesignController
should initially assume that the MD mode to be
used is 'Material'. If the presence of a touchscreen
is detected after the async device scan completes
(which occurs on startup or on restart), change
the mode to 'Material hybrid' and fire a
theme changed notification to trigger a
relayout/repaint of the browser's top chrome UI.

BUG=596294

[tdanderson, 2016-04-20 15:46:20]

tdanderson@chromium.org changed reviewers:
+ pkasting@chromium.org

[tdanderson, 2016-04-20 15:46:20]

Hi Peter, here is an implementation of the suggestion you made in
https://codereview.chromium.org/1844033002/.

I will be posting screenshots to crbug.com/596294 to show what the tabstrip
looks like when the device restarts. I think what's happening is:

(1) Device restarts, defaults to material.
(2) Tabs are rendered with material layout and cached.
(3) Async device scan finishes, and since there is a touchscreen we switch modes
to material-hybrid and fire a theme changed notification.
(4) The notification triggers a relayout/redrawing of the top chrome, but the
tabstrip still uses the material images since they were cached.

Does that make sense? Can you advise on how I can go about getting the tabstrip
to clear its cache so that it will draw the material-hybrid tabs instead?

[Peter Kasting, 2016-04-21 22:32:39]

On 2016/04/20 15:46:20, tdanderson wrote:
> Hi Peter, here is an implementation of the suggestion you made in
> https://codereview.chromium.org/1844033002/.
> 
> I will be posting screenshots to crbug.com/596294 to show what the tabstrip
> looks like when the device restarts. I think what's happening is:
> 
> (1) Device restarts, defaults to material.
> (2) Tabs are rendered with material layout and cached.
> (3) Async device scan finishes, and since there is a touchscreen we switch
modes
> to material-hybrid and fire a theme changed notification.
> (4) The notification triggers a relayout/redrawing of the top chrome, but the
> tabstrip still uses the material images since they were cached.
> 
> Does that make sense? Can you advise on how I can go about getting the
tabstrip
> to clear its cache so that it will draw the material-hybrid tabs instead?

Look in tab.cc at how |image_cache_| is used.  We clear it in LoadTabImages(). 
This is called from OnThemeChanged().  I would check to see if that's being
reached.

[tdanderson, 2016-05-02 18:55:44]

Peter, I found the source of the problem here (it was the caching of the wrong
layout constants and wasn't related to the caching of the tab image
backgrounds). Can you please take a look at the latest WIP patch set and let me
know what you think of the direction?

https://codereview.chromium.org/1906563002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1906563002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:257: //    return *tab_size_info;
Turns out the source of the incorrect drawing was that the TabSizeInfo is cached
when defaulting to 'material' and not recalculated when the device scan
completes and we switch to hybrid. How would you feel about either:

a) never caching this at all, or
b) keeping track of the MD mode we were using when we cached, then recalculating
if the mode is different when GetTabSizeInfo() is called?

https://codereview.chromium.org/1906563002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:2511: CalculateNewTabButtonSize();
Same problem: the new tab button dimensions are calculated initially for
'material' and are never re-calculated when we switch to hybrid.

[Peter Kasting, 2016-05-03 00:58:52]

pkasting@chromium.org changed reviewers:
+ sky@chromium.org

[Peter Kasting, 2016-05-03 00:58:53]

I'm inclined to cache both of these and have the theme-changed notification
clear the cached values so they get auto-recalculated on the next call.

+sky, who added the tab size info cache and could probably speak to whether it
makes a meaningful perf difference.

[sky, 2016-05-03 16:37:03]

The caching made a meaningful difference way back when. But so much
has changed that it may not make a difference now. I think it's worth
reevaluating whether we need the cache at all.

  -Scott

On Mon, May 2, 2016 at 5:58 PM,  <pkasting@chromium.org> wrote:
> I'm inclined to cache both of these and have the theme-changed notification
> clear the cached values so they get auto-recalculated on the next call.
>
> +sky, who added the tab size info cache and could probably speak to whether
> it
> makes a meaningful perf difference.
>
> https://codereview.chromium.org/1906563002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2016-05-03 21:09:05]

On 2016/05/03 16:37:03, sky wrote:
> The caching made a meaningful difference way back when. But so much
> has changed that it may not make a difference now. I think it's worth
> reevaluating whether we need the cache at all.

Terry, maybe create a couple windows with 100 tabs each, then grab a trace of
the theme changed notification with and without caching?  That should give us a
rough idea of whether caching is meaningful.

[Peter Kasting, 2016-08-20 02:41:41]

Terry, what's the status of this CL?  It seems to have stalled.

[tdanderson, 2016-08-29 14:40:11]

On 2016/08/20 02:41:41, Peter Kasting wrote:
> Terry, what's the status of this CL?  It seems to have stalled.

I will close this CL for now to take it out of your review list
since I am not actively working on it at present, but I have 
filed crbug.com/641957 to track the work required for when I
come back to this.