Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
342053003:
Use OnPaint() to paint the content of IndicatorView. (Closed)
|
Created:
6 years, 6 months ago by Jun Mukai Modified:
6 years, 6 months ago Reviewers:
xiyuan CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionUse OnPaint() to paint the content of IndicatorView.
BUG=383015
R=xiyuan@chromium.org
TEST=manually
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278525
Patch Set 1 #
Total comments: 6
Patch Set 2 : fix #Messages
Total messages: 12 (0 generated)
This feels hacky. Is the problem about indicator remains visible on network error? Have we double checked that we really hide it?
https://codereview.chromium.org/342053003/diff/1/ui/app_list/views/speech_vie... File ui/app_list/views/speech_view.cc (left): https://codereview.chromium.org/342053003/diff/1/ui/app_list/views/speech_vie... ui/app_list/views/speech_view.cc:275: indicator_->SetVisible(false); Would a "indicator_->SchedulePaint()" here help?
https://codereview.chromium.org/342053003/diff/1/ui/app_list/views/speech_vie... File ui/app_list/views/speech_view.cc (left): https://codereview.chromium.org/342053003/diff/1/ui/app_list/views/speech_vie... ui/app_list/views/speech_view.cc:275: indicator_->SetVisible(false); On 2014/06/18 23:13:05, xiyuan wrote: > Would a "indicator_->SchedulePaint()" here help? I tried SchedulePaint but it didn't help.
https://codereview.chromium.org/342053003/diff/1/ui/app_list/views/speech_vie... File ui/app_list/views/speech_view.cc (left): https://codereview.chromium.org/342053003/diff/1/ui/app_list/views/speech_vie... ui/app_list/views/speech_view.cc:275: indicator_->SetVisible(false); On 2014/06/18 23:13:51, Jun Mukai wrote: > On 2014/06/18 23:13:05, xiyuan wrote: > > Would a "indicator_->SchedulePaint()" here help? > > I tried SchedulePaint but it didn't help. Try put it before "indicator_->SetVisible(false);" or call parent's SchedulePaint? It seems SchedulePaint is a noop if visible_ is false.
https://codereview.chromium.org/342053003/diff/1/ui/app_list/views/speech_vie... File ui/app_list/views/speech_view.cc (left): https://codereview.chromium.org/342053003/diff/1/ui/app_list/views/speech_vie... ui/app_list/views/speech_view.cc:275: indicator_->SetVisible(false); On 2014/06/18 23:16:02, xiyuan wrote: > On 2014/06/18 23:13:51, Jun Mukai wrote: > > On 2014/06/18 23:13:05, xiyuan wrote: > > > Would a "indicator_->SchedulePaint()" here help? > > > > I tried SchedulePaint but it didn't help. > > Try put it before "indicator_->SetVisible(false);" or call parent's > SchedulePaint? It seems SchedulePaint is a noop if visible_ is false. Tried all pattern of SchedulePaint(), child_at(0)->SchedulePaint(), indicator_->SchedulePaint(), before and after SetVisible, and nothing helps.
https://codereview.chromium.org/342053003/diff/1/ui/app_list/views/speech_vie... File ui/app_list/views/speech_view.cc (left): https://codereview.chromium.org/342053003/diff/1/ui/app_list/views/speech_vie... ui/app_list/views/speech_view.cc:275: indicator_->SetVisible(false); On 2014/06/19 00:58:14, Jun Mukai wrote: > On 2014/06/18 23:16:02, xiyuan wrote: > > On 2014/06/18 23:13:51, Jun Mukai wrote: > > > On 2014/06/18 23:13:05, xiyuan wrote: > > > > Would a "indicator_->SchedulePaint()" here help? > > > > > > I tried SchedulePaint but it didn't help. > > > > Try put it before "indicator_->SetVisible(false);" or call parent's > > SchedulePaint? It seems SchedulePaint is a noop if visible_ is false. > > Tried all pattern of SchedulePaint(), child_at(0)->SchedulePaint(), > indicator_->SchedulePaint(), before and after SetVisible, and nothing helps. Ah, I think I've found why. indicator_animator_ may control the location and something *after* this visibility change. Let me check this.
https://codereview.chromium.org/342053003/diff/1/ui/app_list/views/speech_vie... File ui/app_list/views/speech_view.cc (left): https://codereview.chromium.org/342053003/diff/1/ui/app_list/views/speech_vie... ui/app_list/views/speech_view.cc:275: indicator_->SetVisible(false); On 2014/06/19 01:00:57, Jun Mukai wrote: > On 2014/06/19 00:58:14, Jun Mukai wrote: > > On 2014/06/18 23:16:02, xiyuan wrote: > > > On 2014/06/18 23:13:51, Jun Mukai wrote: > > > > On 2014/06/18 23:13:05, xiyuan wrote: > > > > > Would a "indicator_->SchedulePaint()" here help? > > > > > > > > I tried SchedulePaint but it didn't help. > > > > > > Try put it before "indicator_->SetVisible(false);" or call parent's > > > SchedulePaint? It seems SchedulePaint is a noop if visible_ is false. > > > > Tried all pattern of SchedulePaint(), child_at(0)->SchedulePaint(), > > indicator_->SchedulePaint(), before and after SetVisible, and nothing helps. > > Ah, I think I've found why. indicator_animator_ may control the location and > something *after* this visibility change. Let me check this. Turns out I was totally pointless :( Everything was actually fine, we do not even need additional SchedulePaint(). The problem is that IndicatorView overrides Paint() method. Paint() has long and common logic including the visibility check, and calls several painting methods. In this purpose, overriding Paint() is a bad idea, OnPaint() is better. Uploaded a new patchset for this. PTAL.
LGTM Good work. That totally escaped my eyes too.
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/342053003/20001
Message was sent while issue was closed.
Change committed as 278525 |
