Handle titlebar text updates properly

Handle titlebar text updates properly

Previously the area of the text was not invalidated prior to the title drawing. As such all kind of results were possible. 

BUG=152597, 151827
TEST=visual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=159494

[Mr4D (OOO till 08-26), 2012-09-28 19:44:10]

Hello sky!

this is as discussed. I was running an extended session with all window types
and tested that the problems were gone. I also run the unit tests locally and
everything seemed to be in order.

Please have a look!

[msw, 2012-09-28 20:10:51]

http://codereview.chromium.org/10993087/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/10993087/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:382:
SchedulePaintInRect(title_bounds_);
This particular implementation may be unnecessary after http://crrev.com/159087,
but I haven't looked closely at this patch.

Now, a views::Label title schedules a paint internally on text changes, etc.
I'd favor a solution of using more Labels titles in NonClientView, if possible.
Standard views components should simplify these common critical tasks.

[Mr4D (OOO till 08-26), 2012-09-28 20:30:48]

Please see my comment. 

Furthermore: I had a build problem on the server which required a new upload.

http://codereview.chromium.org/10993087/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/10993087/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:382:
SchedulePaintInRect(title_bounds_);
This particular: You may be right. I was adding this since I am not sure how
many broken cases we have. So I might be able to remove this.

for views::Label - well - there are several different things being used,
dependent on which frame you are looking: there is a frame_painter, there are
labels and there are self drawing frames. But instead of adding another Label
object to every frame object it might be easier to simplify the entire frame
handling. (not with this CL for sure - especially because this CL is targetted
towards M23).

[msw, 2012-09-28 20:42:50]

http://codereview.chromium.org/10993087/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/10993087/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:382:
SchedulePaintInRect(title_bounds_);
On 2012/09/28 20:30:48, Mr4D wrote:
> This particular: You may be right. I was adding this since I am not sure how
> many broken cases we have. So I might be able to remove this.
> 
> for views::Label - well - there are several different things being used,
> dependent on which frame you are looking: there is a frame_painter, there are
> labels and there are self drawing frames. But instead of adding another Label
> object to every frame object it might be easier to simplify the entire frame
> handling. (not with this CL for sure - especially because this CL is targetted
> towards M23).

Okay, let me know if you'd like my full review here or help with any followup.

http://codereview.chromium.org/10993087/diff/9001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/10993087/diff/9001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:394: if
(!frame()->IsFullscreen() && window_title_)
nit: you don't need to check |window_title_| here, it's always non-NULL.

[Mr4D (OOO till 08-26), 2012-09-28 21:54:47]

Addressed build stopper in Windows and comment from msw.

http://codereview.chromium.org/10993087/diff/9001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/10993087/diff/9001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:394: if
(!frame()->IsFullscreen() && window_title_)
Done.

[sky, 2012-09-28 22:47:12]

http://codereview.chromium.org/10993087/diff/7003/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right):

http://codereview.chromium.org/10993087/diff/7003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.h:41: virtual void
UpdateWindowTitle() OVERRIDE { }
{} (for previous lines too).

http://codereview.chromium.org/10993087/diff/7003/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

http://codereview.chromium.org/10993087/diff/7003/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:742: non_client_view_->UpdateWindowTitle();
Don't you need to do all this after updating the native_widget_'s title? If you
call this before updating the native title then anyone that asks for the updated
window title won't get the right thing.

Additionally you should Layout() after updating the window title, since that may
cause things to need a bigger size.

[Mr4D (OOO till 08-26), 2012-09-29 00:44:25]

Addressed. Please have another look!

http://codereview.chromium.org/10993087/diff/7003/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right):

http://codereview.chromium.org/10993087/diff/7003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/glass_browser_frame_view.h:41: virtual void
UpdateWindowTitle() OVERRIDE { }
On 2012/09/28 22:47:12, sky wrote:
> {} (for previous lines too).

Done. (Note: I did a quick search for '{ }' and stopped at >20000 hits. Are you
sure that there should be no space?)

http://codereview.chromium.org/10993087/diff/7003/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

http://codereview.chromium.org/10993087/diff/7003/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:742: non_client_view_->UpdateWindowTitle();
On 2012/09/28 22:47:12, sky wrote:
> Don't you need to do all this after updating the native_widget_'s title? If
you
> call this before updating the native title then anyone that asks for the
updated
> window title won't get the right thing.
> 
> Additionally you should Layout() after updating the window title, since that
may
> cause things to need a bigger size.

Okay. Changed. Not sure if this might cause problems though since it looked like
that there was a reason for the order.

But tested it in various situations and it worked.

[sky, 2012-10-01 13:48:10]

On Fri, Sep 28, 2012 at 5:44 PM,  <skuhne@chromium.org> wrote:
> Addressed. Please have another look!
>
>
>
>
>
http://codereview.chromium.org/10993087/diff/7003/chrome/browser/ui/views/fra...
> File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right):
>
>
http://codereview.chromium.org/10993087/diff/7003/chrome/browser/ui/views/fra...
> chrome/browser/ui/views/frame/glass_browser_frame_view.h:41: virtual
> void UpdateWindowTitle() OVERRIDE { }
> On 2012/09/28 22:47:12, sky wrote:
>>
>> {} (for previous lines too).
>
>
> Done. (Note: I did a quick search for '{ }' and stopped at >20000 hits.
> Are you sure that there should be no space?)

See style guide:

  Foo(int b) : Bar(), baz_(b) {}  // No spaces inside empty braces.

   -Scott
>
>
> http://codereview.chromium.org/10993087/diff/7003/ui/views/widget/widget.cc
> File ui/views/widget/widget.cc (right):
>
>
http://codereview.chromium.org/10993087/diff/7003/ui/views/widget/widget.cc#n...
> ui/views/widget/widget.cc:742: non_client_view_->UpdateWindowTitle();
> On 2012/09/28 22:47:12, sky wrote:
>>
>> Don't you need to do all this after updating the native_widget_'s
>
> title? If you
>>
>> call this before updating the native title then anyone that asks for
>
> the updated
>>
>> window title won't get the right thing.
>
>
>> Additionally you should Layout() after updating the window title,
>
> since that may
>>
>> cause things to need a bigger size.
>
>
> Okay. Changed. Not sure if this might cause problems though since it
> looked like that there was a reason for the order.
>
> But tested it in various situations and it worked.
>
> http://codereview.chromium.org/10993087/

[sky, 2012-10-01 13:51:08]

LGTM

[commit-bot: I haz the power, 2012-10-01 14:15:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/10993087/23001

[commit-bot: I haz the power, 2012-10-01 14:19:09]

Retried try job too often for step(s) crypto_unittests, unit_tests,
cacheinvalidation_unittests, remoting_unittests, jingle_unittests,
nacl_integration, ipc_tests, interactive_ui_tests, browser_tests, net_unittests,
content_unittests, gpu_unittests, base_unittests, sync_integration_tests,
media_unittests, safe_browsing_tests, content_browsertests, sql_unittests,
printing_unittests, sync_unit_tests, check_deps