Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2907983002:
Allow dialogs to use a custom View as their title. (Closed)
|
Created:
3 years, 6 months ago by Bret Modified:
3 years, 5 months ago CC:
chromium-reviews, groby+bubble_chromium.org, vabr+watchlistpasswordmanager_chromium.org, tfarina, hcarmona+bubble_chromium.org, gcasto+watchlist_chromium.org, rouslan+bubble_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow dialogs to use a custom View as their title.
Added a method to DialogDelegate that lets a dialog subclass specify a
View that will be used as the dialog's title. As an example, changed the
Save Password dialog and removed its ad-hoc title. Also removed
SetTitleFontList, as it's not clear how it should interact with a generic
View title, and updated the subclasses that were using it.
BUG=654115, 702196
Review-Url: https://codereview.chromium.org/2907983002
Cr-Commit-Position: refs/heads/master@{#482840}
Committed: https://chromium.googlesource.com/chromium/src/+/257ee88f3a0e1b02da7814cbc37f693494bad6d3
Patch Set 1 #Patch Set 2 : finished? #Patch Set 3 : change to dialog delegate #Patch Set 4 : do the easier thing #Patch Set 5 : minor edits #Patch Set 6 : merge #
Total comments: 8
Patch Set 7 : WIP: second iteration #
Total comments: 17
Patch Set 8 : second iteration #Patch Set 9 : minor edits #Patch Set 10 : pass strings by reference #
Total comments: 27
Patch Set 11 : addressed comments #Patch Set 12 : comments 2 #
Total comments: 10
Patch Set 13 : comments 3 #
Total comments: 8
Patch Set 14 : comments 4 #
Total comments: 6
Patch Set 15 : comments 5 #Patch Set 16 : merge #Messages
Total messages: 54 (26 generated)
The CQ bit was checked by bsep@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== this is just a backup BUG=654115 ========== to ========== Allow dialogs to use a StyledLabel as their title. Added a method to DialogDelegate that lets a specific subclass specify a StyledLabel that will be used in the dialog's title. Also changed one dialog as an example, and removed its ad-hoc title. Also extracted a common interface for Label and StyledLabel so that BubbleFrameView doesn't need to care which one it has most of the time. BUG=654115 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bsep@chromium.org to run a CQ dry run
Description was changed from ========== Allow dialogs to use a StyledLabel as their title. Added a method to DialogDelegate that lets a specific subclass specify a StyledLabel that will be used in the dialog's title. Also changed one dialog as an example, and removed its ad-hoc title. Also extracted a common interface for Label and StyledLabel so that BubbleFrameView doesn't need to care which one it has most of the time. BUG=654115 ========== to ========== Allow dialogs to use a StyledLabel as their title. Added a method to DialogDelegate that lets a dialog subclass specify a StyledLabel that will be used in the dialog's title. Also changed one dialog as an example, and removed its ad-hoc title. BUG=654115 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Allow dialogs to use a StyledLabel as their title. Added a method to DialogDelegate that lets a dialog subclass specify a StyledLabel that will be used in the dialog's title. Also changed one dialog as an example, and removed its ad-hoc title. BUG=654115 ========== to ========== Allow dialogs to use a StyledLabel as their title. Added a method to DialogDelegate that lets a dialog subclass specify a StyledLabel that will be used in the dialog's title. Also changed the Save Password dialog and removed its ad-hoc title as an example. BUG=654115,702196 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Allow dialogs to use a StyledLabel as their title. Added a method to DialogDelegate that lets a dialog subclass specify a StyledLabel that will be used in the dialog's title. Also changed the Save Password dialog and removed its ad-hoc title as an example. BUG=654115,702196 ========== to ========== Allow dialogs to use a custom StyledLabel as their title. Added a method to DialogDelegate that lets a dialog subclass specify a StyledLabel that will be used as the dialog's title. As an example, changed the Save Password dialog and removed its ad-hoc title. BUG=654115,702196 ==========
bsep@chromium.org changed reviewers: + pkasting@chromium.org, sky@chromium.org
pkasting@: general review sky@: approach and OWNERS PTAL!
https://codereview.chromium.org/2907983002/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2907983002/diff/100001/ui/views/window/dialog... ui/views/window/dialog_delegate.h:54: virtual StyledLabel* CreateTitleView(); One concern I have with adding this here is that not all dialogs end up in this code path. For example, the TaskManager won't hit this code path. And you're only changing BubbleFrameView. I think it's best to isolate this in bubble code, and in particular BubbleFrameView. I also think it would be good to avoid recreating the titleview every time. How about BubbleDialogDelegate gets a SetTitleView(). If called it's assumed the titleview updates itself as appropriate. SetTitleView() would take a View*, not a StyledLabel.
https://codereview.chromium.org/2907983002/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2907983002/diff/100001/ui/views/window/dialog... ui/views/window/dialog_delegate.h:54: virtual StyledLabel* CreateTitleView(); On 2017/06/06 23:55:10, sky wrote: > One concern I have with adding this here is that not all dialogs end up in this > code path. For example, the TaskManager won't hit this code path. And you're > only changing BubbleFrameView. I think it's best to isolate this in bubble code, > and in particular BubbleFrameView. I also think it would be good to avoid > recreating the titleview every time. How about BubbleDialogDelegate gets a > SetTitleView(). If called it's assumed the titleview updates itself as > appropriate. SetTitleView() would take a View*, not a StyledLabel. TaskManagerView is a DialogDelegateView, so I think it will hit this code path. As far as I know the only BubbleFrameView widget that don't use DialogDelegate is ScreenCaptureNotificationUIViews, and it doesn't have a title. I'm with you on not wanting to create a new title each UpdateWindowTitle call, but I'm not following your suggestion. I could plumb through an update call, after UpdateWindowTitle calls SetText, to the DialogDelegate so it can call AddStyleRange if it wants to... is that what you mean? Also I don't think it can ever be a generic View, because of AddStyleRange on the delegate end and SetBaseFontList/SizeToFit on the BubbleFrameView end.
https://codereview.chromium.org/2907983002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:238: return; Why decide not to add the title at all, rather than just making it invisible like before? It seems like the old route led to simpler code elsewhere in the file. https://codereview.chromium.org/2907983002/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2907983002/diff/100001/ui/views/window/dialog... ui/views/window/dialog_delegate.cc:95: return nullptr; Why not have this return a StyledLabel for GetWindowTitle()? Then the caller doesn't need to worry about checking one or the other, it can just check this. Where I'd like to go with this is to make GetWindowTitle() private -- so the window title is only exposed to callers through this method. There aren't that many callers of it today. However, looking at that highlights that DialogDelegate::GetAccessibleNodeData() assumes it can read the window title from GetWindowTitle(). If a class overrides CreateTitleView() and doesn't also provide a parallel accessible version in GetWindowTitle(), they'll do the wrong thing. If GetAccessibleNodeData() read the text out of the label returned here, that would work; but returning a new label each time just to read the data seems bad. If DialogDelegate owned the StyledLabel and CreateTitleView() just updated the member, we could access that way; but proper ownership becomes a bit of an issue then (probably the view has to be unique_ptr'd to this class and set_owned_by_parent(false), and I don't know if that can cause destruction order issues). Still, I don't have a better idea unless you want to limit this entirely to BubbleFrameView.
https://codereview.chromium.org/2907983002/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2907983002/diff/100001/ui/views/window/dialog... ui/views/window/dialog_delegate.h:54: virtual StyledLabel* CreateTitleView(); On 2017/06/07 00:37:46, Bret wrote: > On 2017/06/06 23:55:10, sky wrote: > > One concern I have with adding this here is that not all dialogs end up in > this > > code path. For example, the TaskManager won't hit this code path. And you're > > only changing BubbleFrameView. I think it's best to isolate this in bubble > code, > > and in particular BubbleFrameView. I also think it would be good to avoid > > recreating the titleview every time. How about BubbleDialogDelegate gets a > > SetTitleView(). If called it's assumed the titleview updates itself as > > appropriate. SetTitleView() would take a View*, not a StyledLabel. > > TaskManagerView is a DialogDelegateView, so I think it will hit this code path. The new function you are adding is only called from BubbleFrameView. TaskManagerView is a DialogDelegate, but it won't create BubbleFrameView and have this called. > As far as I know the only BubbleFrameView widget that don't use DialogDelegate > is ScreenCaptureNotificationUIViews, and it doesn't have a title. > > I'm with you on not wanting to create a new title each UpdateWindowTitle call, > but I'm not following your suggestion. I could plumb through an update call, > after UpdateWindowTitle calls SetText, to the DialogDelegate so it can call > AddStyleRange if it wants to... is that what you mean? Also I don't think it can > ever be a generic View, because of AddStyleRange on the delegate end and > SetBaseFontList/SizeToFit on the BubbleFrameView end. I'm suggesting you add a SetTitleView() function to BubbleFrameView. If SetTitleView() is called that's the View that is used as the title_ for layout and size calculation. So, roughly something like: SetTitleView(View* title_view) { default default_title_view_; default_title_view_ = nullptr; title_ = title_view; } // Default title to use for the bubble. If SetTitleView() is called this is null. Label* default_title_; // Client specified view to render the title. View* explicitly_set_title_ = nullptr; Most places in BubbleFrameView that access title_ would instead use a title() function: View* title() { return explicitly_set_title_ ? explicitly_set_title_ : default_title_; } UpdateWindowTitle becomes: void UpdateWindowTitle() { if (default_title_) default_title_->SetText(...); // else case, assume client that supplied title view updates it. } Make sense? The password code has access to the BubbleFrameView by way of GetBubbleFrameView(), so early on the password code creates the styled label and calls GetBubbleFrameView()->SetTitleView(styled_label). Does that make sense?
I think I have enough information to do another iteration of this. Thanks for the reviews so far! https://codereview.chromium.org/2907983002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:238: return; On 2017/06/07 02:40:43, Peter Kasting wrote: > Why decide not to add the title at all, rather than just making it invisible > like before? It seems like the old route led to simpler code elsewhere in the > file. The decision as to whether to use CreateTitleView or the default title needed widget_delegate we couldn't do it in the constructor (the widget isn't set yet). And if the dialog doesn't have a title UpdateWindowTitle is never called, so it had to handle a null title either way. But I think I won't need to do all that after I rework the approach. https://codereview.chromium.org/2907983002/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_delegate.cc (right): https://codereview.chromium.org/2907983002/diff/100001/ui/views/window/dialog... ui/views/window/dialog_delegate.cc:95: return nullptr; On 2017/06/07 02:40:43, Peter Kasting wrote: > Why not have this return a StyledLabel for GetWindowTitle()? > > Then the caller doesn't need to worry about checking one or the other, it can > just check this. > > Where I'd like to go with this is to make GetWindowTitle() private -- so the > window title is only exposed to callers through this method. There aren't that > many callers of it today. However, looking at that highlights that > DialogDelegate::GetAccessibleNodeData() assumes it can read the window title > from GetWindowTitle(). If a class overrides CreateTitleView() and doesn't also > provide a parallel accessible version in GetWindowTitle(), they'll do the wrong > thing. > > If GetAccessibleNodeData() read the text out of the label returned here, that > would work; but returning a new label each time just to read the data seems bad. > If DialogDelegate owned the StyledLabel and CreateTitleView() just updated the > member, we could access that way; but proper ownership becomes a bit of an issue > then (probably the view has to be unique_ptr'd to this class and > set_owned_by_parent(false), and I don't know if that can cause destruction order > issues). > > Still, I don't have a better idea unless you want to limit this entirely to > BubbleFrameView. I'll keep this in mind as I rework it, and see if I can find a way to enforce that the label title matches GetWindowTitle (even if it's just with a DCHECK). https://codereview.chromium.org/2907983002/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_delegate.h (right): https://codereview.chromium.org/2907983002/diff/100001/ui/views/window/dialog... ui/views/window/dialog_delegate.h:54: virtual StyledLabel* CreateTitleView(); On 2017/06/07 15:59:40, sky wrote: > On 2017/06/07 00:37:46, Bret wrote: > > On 2017/06/06 23:55:10, sky wrote: > > > One concern I have with adding this here is that not all dialogs end up in > > this > > > code path. For example, the TaskManager won't hit this code path. And you're > > > only changing BubbleFrameView. I think it's best to isolate this in bubble > > code, > > > and in particular BubbleFrameView. I also think it would be good to avoid > > > recreating the titleview every time. How about BubbleDialogDelegate gets a > > > SetTitleView(). If called it's assumed the titleview updates itself as > > > appropriate. SetTitleView() would take a View*, not a StyledLabel. > > > > TaskManagerView is a DialogDelegateView, so I think it will hit this code > path. > > The new function you are adding is only called from BubbleFrameView. > TaskManagerView is a DialogDelegate, but it won't create BubbleFrameView and > have this called. Okay you're right, that's kind of subtle. It doesn't have a title label anyway, but I understand your point about it being weird that it could override this function and nothing would happen. > > As far as I know the only BubbleFrameView widget that don't use DialogDelegate > > is ScreenCaptureNotificationUIViews, and it doesn't have a title. > > > > I'm with you on not wanting to create a new title each UpdateWindowTitle call, > > but I'm not following your suggestion. I could plumb through an update call, > > after UpdateWindowTitle calls SetText, to the DialogDelegate so it can call > > AddStyleRange if it wants to... is that what you mean? Also I don't think it > can > > ever be a generic View, because of AddStyleRange on the delegate end and > > SetBaseFontList/SizeToFit on the BubbleFrameView end. > > I'm suggesting you add a SetTitleView() function to BubbleFrameView. If > SetTitleView() is called that's the View that is used as the title_ for layout > and size calculation. So, roughly something like: > > SetTitleView(View* title_view) { > default default_title_view_; > default_title_view_ = nullptr; > title_ = title_view; > } > > // Default title to use for the bubble. If SetTitleView() is called this is > null. > Label* default_title_; > // Client specified view to render the title. > View* explicitly_set_title_ = nullptr; > > Most places in BubbleFrameView that access title_ would instead use a title() > function: > > View* title() { return explicitly_set_title_ ? explicitly_set_title_ : > default_title_; } > > UpdateWindowTitle becomes: > > void UpdateWindowTitle() { > if (default_title_) > default_title_->SetText(...); > // else case, assume client that supplied title view updates it. > } > > Make sense? > > The password code has access to the BubbleFrameView by way of > GetBubbleFrameView(), so early on the password code creates the styled label and > calls GetBubbleFrameView()->SetTitleView(styled_label). > > Does that make sense? Okay, your idea makes sense. I don't think the delegate gets notified when the title needs updating right now, so I'd have to add that. The other thing to resolve is SetTitleFontList... I wonder if I can just move its usages into custom Label views with my new mechanism.
On Wed, Jun 7, 2017 at 1:15 PM, <bsep@chromium.org> wrote: > I think I have enough information to do another iteration of this. Thanks > for > the reviews so far! > > > https://codereview.chromium.org/2907983002/diff/100001/ui/ > views/bubble/bubble_frame_view.cc > File ui/views/bubble/bubble_frame_view.cc (right): > > https://codereview.chromium.org/2907983002/diff/100001/ui/ > views/bubble/bubble_frame_view.cc#newcode238 > ui/views/bubble/bubble_frame_view.cc:238: return; > On 2017/06/07 02:40:43, Peter Kasting wrote: > > Why decide not to add the title at all, rather than just making it > invisible > > like before? It seems like the old route led to simpler code > elsewhere in the > > file. > > The decision as to whether to use CreateTitleView or the default title > needed widget_delegate we couldn't do it in the constructor (the widget > isn't set yet). And if the dialog doesn't have a title UpdateWindowTitle > is never called, so it had to handle a null title either way. But I > think I won't need to do all that after I rework the approach. > > https://codereview.chromium.org/2907983002/diff/100001/ui/ > views/window/dialog_delegate.cc > File ui/views/window/dialog_delegate.cc (right): > > https://codereview.chromium.org/2907983002/diff/100001/ui/ > views/window/dialog_delegate.cc#newcode95 > ui/views/window/dialog_delegate.cc:95: return nullptr; > On 2017/06/07 02:40:43, Peter Kasting wrote: > > Why not have this return a StyledLabel for GetWindowTitle()? > > > > Then the caller doesn't need to worry about checking one or the other, > it can > > just check this. > > > > Where I'd like to go with this is to make GetWindowTitle() private -- > so the > > window title is only exposed to callers through this method. There > aren't that > > many callers of it today. However, looking at that highlights that > > DialogDelegate::GetAccessibleNodeData() assumes it can read the window > title > > from GetWindowTitle(). If a class overrides CreateTitleView() and > doesn't also > > provide a parallel accessible version in GetWindowTitle(), they'll do > the wrong > > thing. > > > > If GetAccessibleNodeData() read the text out of the label returned > here, that > > would work; but returning a new label each time just to read the data > seems bad. > > If DialogDelegate owned the StyledLabel and CreateTitleView() just > updated the > > member, we could access that way; but proper ownership becomes a bit > of an issue > > then (probably the view has to be unique_ptr'd to this class and > > set_owned_by_parent(false), and I don't know if that can cause > destruction order > > issues). > > > > Still, I don't have a better idea unless you want to limit this > entirely to > > BubbleFrameView. > > I'll keep this in mind as I rework it, and see if I can find a way to > enforce that the label title matches GetWindowTitle (even if it's just > with a DCHECK). > > https://codereview.chromium.org/2907983002/diff/100001/ui/ > views/window/dialog_delegate.h > File ui/views/window/dialog_delegate.h (right): > > https://codereview.chromium.org/2907983002/diff/100001/ui/ > views/window/dialog_delegate.h#newcode54 > ui/views/window/dialog_delegate.h:54: virtual StyledLabel* > CreateTitleView(); > On 2017/06/07 15:59:40, sky wrote: > > On 2017/06/07 00:37:46, Bret wrote: > > > On 2017/06/06 23:55:10, sky wrote: > > > > One concern I have with adding this here is that not all dialogs > end up in > > > this > > > > code path. For example, the TaskManager won't hit this code path. > And you're > > > > only changing BubbleFrameView. I think it's best to isolate this > in bubble > > > code, > > > > and in particular BubbleFrameView. I also think it would be good > to avoid > > > > recreating the titleview every time. How about > BubbleDialogDelegate gets a > > > > SetTitleView(). If called it's assumed the titleview updates > itself as > > > > appropriate. SetTitleView() would take a View*, not a StyledLabel. > > > > > > TaskManagerView is a DialogDelegateView, so I think it will hit this > code > > path. > > > > The new function you are adding is only called from BubbleFrameView. > > TaskManagerView is a DialogDelegate, but it won't create > BubbleFrameView and > > have this called. > > Okay you're right, that's kind of subtle. It doesn't have a title label > anyway, but I understand your point about it being weird that it could > override this function and nothing would happen. > > > > > As far as I know the only BubbleFrameView widget that don't use > DialogDelegate > > > is ScreenCaptureNotificationUIViews, and it doesn't have a title. > > > > > > I'm with you on not wanting to create a new title each > UpdateWindowTitle call, > > > but I'm not following your suggestion. I could plumb through an > update call, > > > after UpdateWindowTitle calls SetText, to the DialogDelegate so it > can call > > > AddStyleRange if it wants to... is that what you mean? Also I don't > think it > > can > > > ever be a generic View, because of AddStyleRange on the delegate end > and > > > SetBaseFontList/SizeToFit on the BubbleFrameView end. > > > > I'm suggesting you add a SetTitleView() function to BubbleFrameView. > If > > SetTitleView() is called that's the View that is used as the title_ > for layout > > and size calculation. So, roughly something like: > > > > SetTitleView(View* title_view) { > > default default_title_view_; > > default_title_view_ = nullptr; > > title_ = title_view; > > } > > > > // Default title to use for the bubble. If SetTitleView() is called > this is > > null. > > Label* default_title_; > > // Client specified view to render the title. > > View* explicitly_set_title_ = nullptr; > > > > Most places in BubbleFrameView that access title_ would instead use a > title() > > function: > > > > View* title() { return explicitly_set_title_ ? explicitly_set_title_ > : > > default_title_; } > > > > UpdateWindowTitle becomes: > > > > void UpdateWindowTitle() { > > if (default_title_) > > default_title_->SetText(...); > > // else case, assume client that supplied title view updates it. > > } > > > > Make sense? > > > > The password code has access to the BubbleFrameView by way of > > GetBubbleFrameView(), so early on the password code creates the styled > label and > > calls GetBubbleFrameView()->SetTitleView(styled_label). > > > > Does that make sense? > > Okay, your idea makes sense. I don't think the delegate gets notified > when the title needs updating right now, so I'd have to add that. The > other thing to resolve is SetTitleFontList... I wonder if I can just > move its usages into custom Label views with my new mechanism. > Who actually calls SetFontList? I'm hoping the delegate can deal appropriately. -Scott > > https://codereview.chromium.org/2907983002/ > -- 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.
https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_dialog_delegate.h (right): https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_dialog_delegate.h:61: void AddedToWidget() override; Move up to WidgetDelegateView section above. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:242: bubble_delegate->PropagateUpdateTitleView(delegate_title_); Can you elaborate on why this path is needed? My thinking is if a client supplies a custom title view they should be responsible for keeping it in sync. If a client has set a title view, then BubbleFrameView::UpdateWindowTitle() effectively does nothing. For example, ManagePasswordsBubbleView::Refresh() is the place that calls UpdateWindowTitle(). It could just as well internally update title_view_, right? Doing this means you don't need to add UpdateTitleView or PropagateUpdateTitleView to the delegate. Also, what necessitates CreateTitleView()? Couldn't ManagePasswordsBubbleView directly call BubbleFrameView::SetTitleView()? ManagePaswordsBubbleView can override AddedToWidget and do this. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:257: DCHECK(title_view); Can you delete default_title_ here if title_view is non-null? https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:259: RemoveChildView(delegate_title_); delete implicitly removes, so no need to call this first. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:552: gfx::Size title_label_size = title()->GetPreferredSize(); In Layout code you use GetHeightForWidth(), and you use the max of the icon height and the title's preferred height. I don't think the two match. You need to use GetHeightForWidth() here too to get a consistent preferred size. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:135: View* title() const { Move above functions.
Just responding, no new patchset. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:242: bubble_delegate->PropagateUpdateTitleView(delegate_title_); On 2017/06/09 23:49:16, sky OOO wrote: > Can you elaborate on why this path is needed? My thinking is if a client > supplies a custom title view they should be responsible for keeping it in sync. > If a client has set a title view, then BubbleFrameView::UpdateWindowTitle() > effectively does nothing. > For example, ManagePasswordsBubbleView::Refresh() is the place that calls > UpdateWindowTitle(). It could just as well internally update title_view_, right? > Doing this means you don't need to add UpdateTitleView or > PropagateUpdateTitleView to the delegate. Hmm I guess I'm confused about the role of UpdateWindowTitle. It seems like some external event (for example BubbleFrameView::OnThemeChanged) should be able to tell the frame to update its title. On the other hand, it seems very unlikely that such an event would create a real change; the subclass ought to know that it's changing its title. Maybe BubbleFrameView::UpdateWindowTitle isn't actually important? > Also, what necessitates CreateTitleView()? Couldn't ManagePasswordsBubbleView > directly call BubbleFrameView::SetTitleView()? ManagePaswordsBubbleView can > override AddedToWidget and do this. I like CreateTitleView because the timing on when to create the title is a bit subtle (i.e. not in the constructor or in Init() when all other initialization is happening) and it's much simpler for a dialog creator to just override this function to get it right. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:552: gfx::Size title_label_size = title()->GetPreferredSize(); On 2017/06/09 23:49:16, sky OOO wrote: > In Layout code you use GetHeightForWidth(), and you use the max of the icon > height and the title's preferred height. I don't think the two match. You need > to use GetHeightForWidth() here too to get a consistent preferred size. I think in practice it matches, because when either Label or StyledLabel is laid out it figures out how tall it is and uses that for its preferred size. I think having a variable preferred size is a no-no anyway, but that's a different issue. I'll change this.
This is ready for review again. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_dialog_delegate.h (right): https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_dialog_delegate.h:61: void AddedToWidget() override; On 2017/06/09 23:49:16, sky OOO wrote: > Move up to WidgetDelegateView section above. Done. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:242: bubble_delegate->PropagateUpdateTitleView(delegate_title_); On 2017/06/10 00:52:01, Bret wrote: > On 2017/06/09 23:49:16, sky OOO wrote: > > Can you elaborate on why this path is needed? My thinking is if a client > > supplies a custom title view they should be responsible for keeping it in > sync. > > If a client has set a title view, then BubbleFrameView::UpdateWindowTitle() > > effectively does nothing. > > For example, ManagePasswordsBubbleView::Refresh() is the place that calls > > UpdateWindowTitle(). It could just as well internally update title_view_, > right? > > Doing this means you don't need to add UpdateTitleView or > > PropagateUpdateTitleView to the delegate. > > Hmm I guess I'm confused about the role of UpdateWindowTitle. It seems like some > external event (for example BubbleFrameView::OnThemeChanged) should be able to > tell the frame to update its title. On the other hand, it seems very unlikely > that such an event would create a real change; the subclass ought to know that > it's changing its title. Maybe BubbleFrameView::UpdateWindowTitle isn't actually > important? Added a SetupTitleView to BubbleDialogDelegateView that the subclass can call when it wants to recreate the title view. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:257: DCHECK(title_view); On 2017/06/09 23:49:16, sky OOO wrote: > Can you delete default_title_ here if title_view is non-null? Done. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:259: RemoveChildView(delegate_title_); On 2017/06/09 23:49:16, sky OOO wrote: > delete implicitly removes, so no need to call this first. Done. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:552: gfx::Size title_label_size = title()->GetPreferredSize(); On 2017/06/10 00:52:01, Bret wrote: > On 2017/06/09 23:49:16, sky OOO wrote: > > In Layout code you use GetHeightForWidth(), and you use the max of the icon > > height and the title's preferred height. I don't think the two match. You need > > to use GetHeightForWidth() here too to get a consistent preferred size. > > I think in practice it matches, because when either Label or StyledLabel is laid > out it figures out how tall it is and uses that for its preferred size. I think > having a variable preferred size is a no-no anyway, but that's a different > issue. I'll change this. I investigated this a little more (and wrote a test) and I think this is okay as-is. The title width depends on the bounds width, which isn't known at this point, so I'm not sure what else we would do here. The height does end up being max(title height, icon height) via GetInsets. It all seems actually correct so I'd rather not change it in this patch. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:135: View* title() const { On 2017/06/09 23:49:16, sky OOO wrote: > Move above functions. Done.
Description was changed from ========== Allow dialogs to use a custom StyledLabel as their title. Added a method to DialogDelegate that lets a dialog subclass specify a StyledLabel that will be used as the dialog's title. As an example, changed the Save Password dialog and removed its ad-hoc title. BUG=654115,702196 ========== to ========== Allow dialogs to use a custom StyledLabel as their title. Added a method to DialogDelegate that lets a dialog subclass specify a View that will be used as the dialog's title. As an example, changed the Save Password dialog and removed its ad-hoc title. Also removed SetTitleFontList, as it's not clear how it should interact with a generic View title, and updated the subclasses that were using it. BUG=654115,702196 ==========
Description was changed from ========== Allow dialogs to use a custom StyledLabel as their title. Added a method to DialogDelegate that lets a dialog subclass specify a View that will be used as the dialog's title. As an example, changed the Save Password dialog and removed its ad-hoc title. Also removed SetTitleFontList, as it's not clear how it should interact with a generic View title, and updated the subclasses that were using it. BUG=654115,702196 ========== to ========== Allow dialogs to use a custom View as their title. Added a method to DialogDelegate that lets a dialog subclass specify a View that will be used as the dialog's title. As an example, changed the Save Password dialog and removed its ad-hoc title. Also removed SetTitleFontList, as it's not clear how it should interact with a generic View title, and updated the subclasses that were using it. BUG=654115,702196 ==========
This seems like a safer design than before. https://codereview.chromium.org/2907983002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2907983002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:820: DCHECK_EQ(range, model_.title_brand_link_range()); Nit: (expected, actual) https://codereview.chromium.org/2907983002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/2907983002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:83: // views::BubbleDialogDelegateView: Nit: You don't directly subclass this class, or WidgetDelegate, so I wouldn't split them into separate lists. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... File ui/views/bubble/bubble_dialog_delegate.h (right): https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_dialog_delegate.h:138: void SetupTitleView(); Nit: "Setup" implied to me that I could only call this once, during startup. Maybe Update or Reset instead? https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... File ui/views/bubble/bubble_dialog_delegate_unittest.cc (right): https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_dialog_delegate_unittest.cc:356: constexpr int title_preferred_height = 20; Nit: Use kNamingStyle https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_dialog_delegate_unittest.cc:362: // Title takes up the whole bubble width when there's no icon or close button. Nit: Maybe this means the below statement should be EXPECT_EQ(bubble_delegate->width(), title_view->width()); ? https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:107: Label* BubbleFrameView::CreateDefaultTitleLabel( Nit: Since this is passing out ownership, can it return a unique_ptr to make that clear? https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:254: if (default_title_) { Nit: Conditional checks here and below are unnecessary, since deleting a null pointer is safe. I wonder if the fact that we want to delete these directly suggests we should set_owned_by_client(true) on them and store them as unique_ptrs. Maybe that just makes things more confusing. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:261: AddChildView(delegate_title_); Hmm, don't you want to ensure this title is added as the first child after the icon? https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:342: title_label_right = close_->x(); In the close-button-invisible case, the bounds have been inset by |title_margins_|, so the title won't go right to the edge of them. However, in this case, we _do_ go right to the edge of the close X. It seems like we want to maybe inset from that by title_margins_.right()? The old code also did this, which also seems wrong to me. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:104: View* title() const { Const member functions shouldn't return non-const pointers; see https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Use-co... .
https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:242: bubble_delegate->PropagateUpdateTitleView(delegate_title_); On 2017/06/10 00:52:01, Bret wrote: > On 2017/06/09 23:49:16, sky OOO wrote: > > Can you elaborate on why this path is needed? My thinking is if a client > > supplies a custom title view they should be responsible for keeping it in > sync. > > If a client has set a title view, then BubbleFrameView::UpdateWindowTitle() > > effectively does nothing. > > For example, ManagePasswordsBubbleView::Refresh() is the place that calls > > UpdateWindowTitle(). It could just as well internally update title_view_, > right? > > Doing this means you don't need to add UpdateTitleView or > > PropagateUpdateTitleView to the delegate. > > Hmm I guess I'm confused about the role of UpdateWindowTitle. It seems like some > external event (for example BubbleFrameView::OnThemeChanged) should be able to > tell the frame to update its title. On the other hand, it seems very unlikely > that such an event would create a real change; the subclass ought to know that > it's changing its title. Maybe BubbleFrameView::UpdateWindowTitle isn't actually > important? Exactly. I don't think UpdateWindowTitle is at all useful in the case of a client supplied title view. > > Also, what necessitates CreateTitleView()? Couldn't ManagePasswordsBubbleView > > directly call BubbleFrameView::SetTitleView()? ManagePaswordsBubbleView can > > override AddedToWidget and do this. > > I like CreateTitleView because the timing on when to create the title is a bit > subtle (i.e. not in the constructor or in Init() when all other initialization > is happening) and it's much simpler for a dialog creator to just override this > function to get it right. I get that the timing is tricky, but I would prefer to avoid more complexity to the base class by way of additional functions. Can you elaborate on why overriding AddedToWidget doesn't work in this case?
The CQ bit was checked by bsep@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
New patchset is quite a bit different. https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:242: bubble_delegate->PropagateUpdateTitleView(delegate_title_); On 2017/06/19 15:19:25, sky wrote: > On 2017/06/10 00:52:01, Bret wrote: > > On 2017/06/09 23:49:16, sky OOO wrote: > > > Can you elaborate on why this path is needed? My thinking is if a client > > > supplies a custom title view they should be responsible for keeping it in > > sync. > > > If a client has set a title view, then BubbleFrameView::UpdateWindowTitle() > > > effectively does nothing. > > > For example, ManagePasswordsBubbleView::Refresh() is the place that calls > > > UpdateWindowTitle(). It could just as well internally update title_view_, > > right? > > > Doing this means you don't need to add UpdateTitleView or > > > PropagateUpdateTitleView to the delegate. > > > > Hmm I guess I'm confused about the role of UpdateWindowTitle. It seems like > some > > external event (for example BubbleFrameView::OnThemeChanged) should be able to > > tell the frame to update its title. On the other hand, it seems very unlikely > > that such an event would create a real change; the subclass ought to know that > > it's changing its title. Maybe BubbleFrameView::UpdateWindowTitle isn't > actually > > important? > > Exactly. I don't think UpdateWindowTitle is at all useful in the case of a > client supplied title view. > > > > Also, what necessitates CreateTitleView()? Couldn't > ManagePasswordsBubbleView > > > directly call BubbleFrameView::SetTitleView()? ManagePaswordsBubbleView can > > > override AddedToWidget and do this. > > > > I like CreateTitleView because the timing on when to create the title is a bit > > subtle (i.e. not in the constructor or in Init() when all other initialization > > is happening) and it's much simpler for a dialog creator to just override this > > function to get it right. > > I get that the timing is tricky, but I would prefer to avoid more complexity to > the base class by way of additional functions. Can you elaborate on why > overriding AddedToWidget doesn't work in this case? After playing around with it, I ended up removing the new base class methods and made the subclasses override AddedToWidget directly like you wanted. I agree that it's better because the subclasses can manage title text updates directly instead of just destroying and re-creating the title views indirectly. I still worry about GetWindowTitle getting out of sync with the actual title text because it's not enforced for them to be the same. I'm dunno what to do about that. I suppose I could add a complicated DCHECK block that downcasts delegate_title_... but I'm not sure it's worth doing. https://codereview.chromium.org/2907983002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2907983002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:820: DCHECK_EQ(range, model_.title_brand_link_range()); On 2017/06/17 01:16:20, Peter Kasting wrote: > Nit: (expected, actual) Done. https://codereview.chromium.org/2907983002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h (right): https://codereview.chromium.org/2907983002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h:83: // views::BubbleDialogDelegateView: On 2017/06/17 01:16:20, Peter Kasting wrote: > Nit: You don't directly subclass this class, or WidgetDelegate, so I wouldn't > split them into separate lists. Moved both lists under LocationBarBubbleDelegateView. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... File ui/views/bubble/bubble_dialog_delegate.h (right): https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_dialog_delegate.h:138: void SetupTitleView(); On 2017/06/17 01:16:20, Peter Kasting wrote: > Nit: "Setup" implied to me that I could only call this once, during startup. > Maybe Update or Reset instead? No longer relevant due to other changes. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... File ui/views/bubble/bubble_dialog_delegate_unittest.cc (right): https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_dialog_delegate_unittest.cc:356: constexpr int title_preferred_height = 20; On 2017/06/17 01:16:20, Peter Kasting wrote: > Nit: Use kNamingStyle Done. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_dialog_delegate_unittest.cc:362: // Title takes up the whole bubble width when there's no icon or close button. On 2017/06/17 01:16:20, Peter Kasting wrote: > Nit: Maybe this means the below statement should be > > EXPECT_EQ(bubble_delegate->width(), title_view->width()); > > ? I split this into two EXPECTs for the width and height, so it's clearer what the comment goes with. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:107: Label* BubbleFrameView::CreateDefaultTitleLabel( On 2017/06/17 01:16:20, Peter Kasting wrote: > Nit: Since this is passing out ownership, can it return a unique_ptr to make > that clear? Done. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:254: if (default_title_) { On 2017/06/17 01:16:20, Peter Kasting wrote: > Nit: Conditional checks here and below are unnecessary, since deleting a null > pointer is safe. > > I wonder if the fact that we want to delete these directly suggests we should > set_owned_by_client(true) on them and store them as unique_ptrs. Maybe that > just makes things more confusing. There's not an obvious owner and it'd be really odd to return a unique_ptr whilst saving a raw pointer, so I made it a shared_ptr. Is this clearer? https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:261: AddChildView(delegate_title_); On 2017/06/17 01:16:20, Peter Kasting wrote: > Hmm, don't you want to ensure this title is added as the first child after the > icon? I didn't think it was important, since they shouldn't be drawing over each other. Is there another reason we should ensure this? https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:342: title_label_right = close_->x(); On 2017/06/17 01:16:20, Peter Kasting wrote: > In the close-button-invisible case, the bounds have been inset by > |title_margins_|, so the title won't go right to the edge of them. > > However, in this case, we _do_ go right to the edge of the close X. It seems > like we want to maybe inset from that by title_margins_.right()? > > The old code also did this, which also seems wrong to me. Visually this looks okay to me, since the close button is surrounded by some whitespace when it's not hovered. Subtracting title_margins_ here seems to give too much whitespace. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:104: View* title() const { On 2017/06/17 01:16:20, Peter Kasting wrote: > Const member functions shouldn't return non-const pointers; see > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Use-co... > . Done, made const and non-const getters.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:254: if (default_title_) { On 2017/06/21 22:37:54, Bret wrote: > On 2017/06/17 01:16:20, Peter Kasting wrote: > > Nit: Conditional checks here and below are unnecessary, since deleting a null > > pointer is safe. > > > > I wonder if the fact that we want to delete these directly suggests we should > > set_owned_by_client(true) on them and store them as unique_ptrs. Maybe that > > just makes things more confusing. > > There's not an obvious owner and it'd be really odd to return a unique_ptr > whilst saving a raw pointer, so I made it a shared_ptr. Is this clearer? shared_ptr is banned in Chromium -- see https://chromium-cpp.appspot.com/ . I think linked_ptr is the closest thing we have, but don't use that :) Just leave the ownership model the way you did it in the first place, it's probably best. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:261: AddChildView(delegate_title_); On 2017/06/21 22:37:54, Bret wrote: > On 2017/06/17 01:16:20, Peter Kasting wrote: > > Hmm, don't you want to ensure this title is added as the first child after the > > icon? > > I didn't think it was important, since they shouldn't be drawing over each > other. Is there another reason we should ensure this? Screen readers and accessibility software may traverse the child list in order to read things out, I think? Also tab order follows child order by default, so if there's e.g. a clickable link in the title that users can tab to you probably want to do this. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:342: title_label_right = close_->x(); On 2017/06/21 22:37:54, Bret wrote: > On 2017/06/17 01:16:20, Peter Kasting wrote: > > In the close-button-invisible case, the bounds have been inset by > > |title_margins_|, so the title won't go right to the edge of them. > > > > However, in this case, we _do_ go right to the edge of the close X. It seems > > like we want to maybe inset from that by title_margins_.right()? > > > > The old code also did this, which also seems wrong to me. > > Visually this looks okay to me, since the close button is surrounded by some > whitespace when it's not hovered. But when it's hovered, it will potentially touch the title. That seems bad. > Subtracting title_margins_ here seems to give > too much whitespace. Hmm. I wonder if there's some kind of compromise we should be doing. Can you easily screenshot what it looks like today hovered/not hovered, with a long title, as well as when you add in the title margins? Maybe the title margins are set too big and we should be using a different fixed value?
https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:254: if (default_title_) { On 2017/06/21 23:45:22, Peter Kasting wrote: > On 2017/06/21 22:37:54, Bret wrote: > > On 2017/06/17 01:16:20, Peter Kasting wrote: > > > Nit: Conditional checks here and below are unnecessary, since deleting a > null > > > pointer is safe. > > > > > > I wonder if the fact that we want to delete these directly suggests we > should > > > set_owned_by_client(true) on them and store them as unique_ptrs. Maybe that > > > just makes things more confusing. > > > > There's not an obvious owner and it'd be really odd to return a unique_ptr > > whilst saving a raw pointer, so I made it a shared_ptr. Is this clearer? > > shared_ptr is banned in Chromium -- see https://chromium-cpp.appspot.com/ . I > think linked_ptr is the closest thing we have, but don't use that :) > > Just leave the ownership model the way you did it in the first place, it's > probably best. Oops, okay. Done. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:261: AddChildView(delegate_title_); On 2017/06/21 23:45:22, Peter Kasting wrote: > On 2017/06/21 22:37:54, Bret wrote: > > On 2017/06/17 01:16:20, Peter Kasting wrote: > > > Hmm, don't you want to ensure this title is added as the first child after > the > > > icon? > > > > I didn't think it was important, since they shouldn't be drawing over each > > other. Is there another reason we should ensure this? > > Screen readers and accessibility software may traverse the child list in order > to read things out, I think? Also tab order follows child order by default, so > if there's e.g. a clickable link in the title that users can tab to you probably > want to do this. Done. https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:342: title_label_right = close_->x(); On 2017/06/21 23:45:22, Peter Kasting wrote: > On 2017/06/21 22:37:54, Bret wrote: > > On 2017/06/17 01:16:20, Peter Kasting wrote: > > > In the close-button-invisible case, the bounds have been inset by > > > |title_margins_|, so the title won't go right to the edge of them. > > > > > > However, in this case, we _do_ go right to the edge of the close X. It > seems > > > like we want to maybe inset from that by title_margins_.right()? > > > > > > The old code also did this, which also seems wrong to me. > > > > Visually this looks okay to me, since the close button is surrounded by some > > whitespace when it's not hovered. > > But when it's hovered, it will potentially touch the title. That seems bad. > > > Subtracting title_margins_ here seems to give > > too much whitespace. > > Hmm. I wonder if there's some kind of compromise we should be doing. Can you > easily screenshot what it looks like today hovered/not hovered, with a long > title, as well as when you add in the title margins? Maybe the title margins > are set too big and we should be using a different fixed value? I'll send you screenshots over email.
This is much improved! Thanks! https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/120001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:242: bubble_delegate->PropagateUpdateTitleView(delegate_title_); On 2017/06/21 22:37:54, Bret wrote: > On 2017/06/19 15:19:25, sky wrote: > > On 2017/06/10 00:52:01, Bret wrote: > > > On 2017/06/09 23:49:16, sky OOO wrote: > > > > Can you elaborate on why this path is needed? My thinking is if a client > > > > supplies a custom title view they should be responsible for keeping it in > > > sync. > > > > If a client has set a title view, then > BubbleFrameView::UpdateWindowTitle() > > > > effectively does nothing. > > > > For example, ManagePasswordsBubbleView::Refresh() is the place that calls > > > > UpdateWindowTitle(). It could just as well internally update title_view_, > > > right? > > > > Doing this means you don't need to add UpdateTitleView or > > > > PropagateUpdateTitleView to the delegate. > > > > > > Hmm I guess I'm confused about the role of UpdateWindowTitle. It seems like > > some > > > external event (for example BubbleFrameView::OnThemeChanged) should be able > to > > > tell the frame to update its title. On the other hand, it seems very > unlikely > > > that such an event would create a real change; the subclass ought to know > that > > > it's changing its title. Maybe BubbleFrameView::UpdateWindowTitle isn't > > actually > > > important? > > > > Exactly. I don't think UpdateWindowTitle is at all useful in the case of a > > client supplied title view. > > > > > > Also, what necessitates CreateTitleView()? Couldn't > > ManagePasswordsBubbleView > > > > directly call BubbleFrameView::SetTitleView()? ManagePaswordsBubbleView > can > > > > override AddedToWidget and do this. > > > > > > I like CreateTitleView because the timing on when to create the title is a > bit > > > subtle (i.e. not in the constructor or in Init() when all other > initialization > > > is happening) and it's much simpler for a dialog creator to just override > this > > > function to get it right. > > > > I get that the timing is tricky, but I would prefer to avoid more complexity > to > > the base class by way of additional functions. Can you elaborate on why > > overriding AddedToWidget doesn't work in this case? > > After playing around with it, I ended up removing the new base class methods and > made the subclasses override AddedToWidget directly like you wanted. I agree > that it's better because the subclasses can manage title text updates directly > instead of just destroying and re-creating the title views indirectly. > > I still worry about GetWindowTitle getting out of sync with the actual title > text because it's not enforced for them to be the same. I'm dunno what to do > about that. I suppose I could add a complicated DCHECK block that downcasts > delegate_title_... but I'm not sure it's worth doing. I tend to think it isn't worth bothering either. https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:110: new Label(title_text, style::CONTEXT_DIALOG_TITLE)); MakeUnique? https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:54: void SetTitleView(View* title_view); Please add test coverage of this. Maybe that setting it and forcing a layout results in a valid parent, the view being visible and with some non-empty bounds. https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:110: static_cast<const BubbleFrameView*>(this)->title()); static_cast -> const_cast https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:138: Label* default_title_; It's quite subtle why there are two titles. Please document what the two are for and when they are used. https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:139: View* delegate_title_; Why did you go with 'delegate' as the prefix for this? The title doesn't come from the delegate, so it's mildly confusing. Maybe client_title_?
https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:110: static_cast<const BubbleFrameView*>(this)->title()); On 2017/06/22 14:48:48, sky wrote: > static_cast -> const_cast The Meyers pattern is to use static_cast here, which I kind of agree with -- static_cast is basically the "safest" cast.
Thanks for pointing that out! I hadn't realized static_cast is fine in this situation. -Scott On Thu, Jun 22, 2017 at 10:49 AM, <pkasting@chromium.org> wrote: > > https://codereview.chromium.org/2907983002/diff/220001/ui/ > views/bubble/bubble_frame_view.h > File ui/views/bubble/bubble_frame_view.h (right): > > https://codereview.chromium.org/2907983002/diff/220001/ui/ > views/bubble/bubble_frame_view.h#newcode110 > ui/views/bubble/bubble_frame_view.h:110: static_cast<const > BubbleFrameView*>(this)->title()); > On 2017/06/22 14:48:48, sky wrote: > > static_cast -> const_cast > > The Meyers pattern is to use static_cast here, which I kind of agree > with -- static_cast is basically the "safest" cast. > > https://codereview.chromium.org/2907983002/ > -- 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.
https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/180001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:342: title_label_right = close_->x(); On 2017/06/22 01:38:57, Bret wrote: > On 2017/06/21 23:45:22, Peter Kasting wrote: > > On 2017/06/21 22:37:54, Bret wrote: > > > On 2017/06/17 01:16:20, Peter Kasting wrote: > > > > In the close-button-invisible case, the bounds have been inset by > > > > |title_margins_|, so the title won't go right to the edge of them. > > > > > > > > However, in this case, we _do_ go right to the edge of the close X. It > > seems > > > > like we want to maybe inset from that by title_margins_.right()? > > > > > > > > The old code also did this, which also seems wrong to me. > > > > > > Visually this looks okay to me, since the close button is surrounded by some > > > whitespace when it's not hovered. > > > > But when it's hovered, it will potentially touch the title. That seems bad. > > > > > Subtracting title_margins_ here seems to give > > > too much whitespace. > > > > Hmm. I wonder if there's some kind of compromise we should be doing. Can you > > easily screenshot what it looks like today hovered/not hovered, with a long > > title, as well as when you add in the title margins? Maybe the title margins > > are set too big and we should be using a different fixed value? > > I'll send you screenshots over email. From the email we decided to subtract close_margin here. https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:110: new Label(title_text, style::CONTEXT_DIALOG_TITLE)); On 2017/06/22 14:48:48, sky wrote: > MakeUnique? Done. https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:54: void SetTitleView(View* title_view); On 2017/06/22 14:48:48, sky wrote: > Please add test coverage of this. Maybe that setting it and forcing a layout > results in a valid parent, the view being visible and with some non-empty > bounds. See the change in bubble_dialog_delegate_unittest.cc. Added an assert for the title view's parent. The title view being visible is up to the delegate, so I didn't assert that. https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:138: Label* default_title_; On 2017/06/22 14:48:48, sky wrote: > It's quite subtle why there are two titles. Please document what the two are for > and when they are used. Done. https://codereview.chromium.org/2907983002/diff/220001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:139: View* delegate_title_; On 2017/06/22 14:48:48, sky wrote: > Why did you go with 'delegate' as the prefix for this? The title doesn't come > from the delegate, so it's mildly confusing. Maybe client_title_? It was more associated with the delegate in the earlier versions of the patch with CreateTitleView. How about "custom_title_"?
custom_title_ is perfect. LGTM!
LGTM https://codereview.chromium.org/2907983002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2907983002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:545: GetBubbleFrameView()->SetTitleView(title_); SetTitleView() basically "takes ownership" of this object -- it will handle deleting it. I think we should have it take a unique_ptr to make that clear. You might want to implement the comment below as a prerequisite to this, since it's awkward to keep a raw pointer to something you've passed away ownership to. (It's just as unsafe in the current design, but passing ownership more explicitly makes the awkwardness more obvious.) Maybe this provides enough motivation to actually implement that comment instead of saying "eh" :) https://codereview.chromium.org/2907983002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:691: title_->SetText(GetWindowTitle()); If the BubbleFrameView exposed the title() methods publicly, this could call them, which would remove the need to keep it as a member. OTOH, you'd have to cast the returned pointer, which you know is safe, but might look scary/awkward. https://codereview.chromium.org/2907983002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2907983002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:791: title_view_->SetText(GetWindowTitle()); Same comment applies as in ManagePasswordsBubbleView. https://codereview.chromium.org/2907983002/diff/240001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/240001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:340: title_label_right = close_->x() - close_margin; As I tried to convey in email, I think this would be theoretically safer: title_label_right = std::min(title_label_right, close_->x() - close_margin); This way if for some reason we set really enormous title margins, adding a close button in the corner wouldn't suddenly reduce them. This seems unlikely, but assuming things = three years from now Harmony phase 57 breaks :)
https://codereview.chromium.org/2907983002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2907983002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:545: GetBubbleFrameView()->SetTitleView(title_); On 2017/06/23 06:00:09, Peter Kasting wrote: > SetTitleView() basically "takes ownership" of this object -- it will handle > deleting it. > > I think we should have it take a unique_ptr to make that clear. You might want > to implement the comment below as a prerequisite to this, since it's awkward to > keep a raw pointer to something you've passed away ownership to. (It's just as > unsafe in the current design, but passing ownership more explicitly makes the > awkwardness more obvious.) Maybe this provides enough motivation to actually > implement that comment instead of saying "eh" :) Done. https://codereview.chromium.org/2907983002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:691: title_->SetText(GetWindowTitle()); On 2017/06/23 06:00:09, Peter Kasting wrote: > If the BubbleFrameView exposed the title() methods publicly, this could call > them, which would remove the need to keep it as a member. OTOH, you'd have to > cast the returned pointer, which you know is safe, but might look scary/awkward. The new patchset has this version. I'm not sure which I prefer, the scary ownership passing or the scary downcasting. What do you think? https://codereview.chromium.org/2907983002/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2907983002/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:791: title_view_->SetText(GetWindowTitle()); On 2017/06/23 06:00:09, Peter Kasting wrote: > Same comment applies as in ManagePasswordsBubbleView. Done. https://codereview.chromium.org/2907983002/diff/240001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/240001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:340: title_label_right = close_->x() - close_margin; On 2017/06/23 06:00:09, Peter Kasting wrote: > As I tried to convey in email, I think this would be theoretically safer: > > title_label_right = std::min(title_label_right, close_->x() - close_margin); > > This way if for some reason we set really enormous title margins, adding a close > button in the corner wouldn't suddenly reduce them. > > This seems unlikely, but assuming things = three years from now Harmony phase 57 > breaks :) Done.
You're right, either way is scary and could break. I kinda like the scary casting better than the scary ownership model. It feels less scary. If you really disagree, though, I won't override you. LGTM. https://codereview.chromium.org/2907983002/diff/260001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/260001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:257: custom_title_ = title_view.release(); Nit: Technically it might be safer to do custom_title_ = title_view.get(); AddChildViewAt(title_view.release(), 1); ...Since this avoids any window of non-ownership. But I don't care much. https://codereview.chromium.org/2907983002/diff/260001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:340: title_label_right = std::min(title_label_right, close_->x() - close_margin); Nit: In principle, I suppose we should test all the combinations of this. That might call for pulling this change into its own CL, since it's really kind of orthogonal to this one. https://codereview.chromium.org/2907983002/diff/260001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/2907983002/diff/260001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:81: static_cast<const BubbleFrameView*>(this)->title()); Nit: I kinda wonder whether just duplicating the const version might be better since it's shorter and more readable. I mean, I know it's duplicated, but...
https://codereview.chromium.org/2907983002/diff/260001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2907983002/diff/260001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:257: custom_title_ = title_view.release(); On 2017/06/27 01:13:17, Peter Kasting wrote: > Nit: Technically it might be safer to do > > custom_title_ = title_view.get(); > AddChildViewAt(title_view.release(), 1); > > ...Since this avoids any window of non-ownership. > > But I don't care much. Fair enough, done. https://codereview.chromium.org/2907983002/diff/260001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:340: title_label_right = std::min(title_label_right, close_->x() - close_margin); On 2017/06/27 01:13:18, Peter Kasting wrote: > Nit: In principle, I suppose we should test all the combinations of this. > > That might call for pulling this change into its own CL, since it's really kind > of orthogonal to this one. In principle you're right, but that would require re-plumbing the test suite because like you said we're just future-proofing here. I'm inclined to leave it for now. https://codereview.chromium.org/2907983002/diff/260001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.h (right): https://codereview.chromium.org/2907983002/diff/260001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.h:81: static_cast<const BubbleFrameView*>(this)->title()); On 2017/06/27 01:13:18, Peter Kasting wrote: > Nit: I kinda wonder whether just duplicating the const version might be better > since it's shorter and more readable. I mean, I know it's duplicated, but... I would if it were a totally trivial getter, but I think this is okay.
The CQ bit was checked by bsep@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2907983002/#ps280001 (title: "comments 5")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by bsep@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2907983002/#ps300001 (title: "merge")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1498608429904320,
"parent_rev": "95e377e8e0851d2d5fa089036b0e24a1a75f89c6", "commit_rev":
"257ee88f3a0e1b02da7814cbc37f693494bad6d3"}
Message was sent while issue was closed.
Description was changed from ========== Allow dialogs to use a custom View as their title. Added a method to DialogDelegate that lets a dialog subclass specify a View that will be used as the dialog's title. As an example, changed the Save Password dialog and removed its ad-hoc title. Also removed SetTitleFontList, as it's not clear how it should interact with a generic View title, and updated the subclasses that were using it. BUG=654115,702196 ========== to ========== Allow dialogs to use a custom View as their title. Added a method to DialogDelegate that lets a dialog subclass specify a View that will be used as the dialog's title. As an example, changed the Save Password dialog and removed its ad-hoc title. Also removed SetTitleFontList, as it's not clear how it should interact with a generic View title, and updated the subclasses that were using it. BUG=654115,702196 Review-Url: https://codereview.chromium.org/2907983002 Cr-Commit-Position: refs/heads/master@{#482840} Committed: https://chromium.googlesource.com/chromium/src/+/257ee88f3a0e1b02da7814cbc37f... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/257ee88f3a0e1b02da7814cbc37f... |
