Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2692113002:
harmony: fix sizing on webusb chooser dialog (Closed)
|
Created:
3 years, 10 months ago by Elly Fong-Jones Modified:
3 years, 9 months ago CC:
chromium-reviews, msramek+watch_chromium.org, groby+bubble_chromium.org, tfarina, raymes+watch_chromium.org, msw+watch_chromium.org, hcarmona+bubble_chromium.org, markusheintz_, rouslan+bubble_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionharmony: fix sizing on webusb chooser dialog
The change in https://codereview.chromium.org/2654323002 removed the
preferred size of the content view used in this dialog, but did not
replace it with a dialog-level preferred size. This change implements GetPreferredSize() on DeviceChooserContentView.
BUG=610430
Review-Url: https://codereview.chromium.org/2692113002
Cr-Commit-Position: refs/heads/master@{#453657}
Committed: https://chromium.googlesource.com/chromium/src/+/b7312b4e50b6e10b9382e6adeddf9441112db608
Patch Set 1 #
Total comments: 10
Patch Set 2 : fix non-harmony size #
Total comments: 4
Patch Set 3 : const -> constexpr #
Total comments: 2
Patch Set 4 : add unit test #Patch Set 5 : ensure minimum size is respected #Patch Set 6 : use preferred instead of minimum #
Total comments: 4
Patch Set 7 : move GetPreferredSize override #
Messages
Total messages: 43 (19 generated)
Description was changed from ========== harmony: fix sizing on webusb chooser dialog The change in https://codereview.chromium.org/2654323002 removed the minimum size of the content view used in this dialog, but did not replace it with a dialog-level minimum size. This change: 1) Has BubbleDialogDelegateView honor its own GetMinimumSize(); 2) Implements GetMinimumSize on ChooserBubbleUiViewDelegate BUG=610430 ========== to ========== harmony: fix sizing on webusb chooser dialog The change in https://codereview.chromium.org/2654323002 removed the minimum size of the content view used in this dialog, but did not replace it with a dialog-level minimum size. This change: 1) Has BubbleDialogDelegateView honor its own GetMinimumSize(); 2) Implements GetMinimumSize on ChooserBubbleUiViewDelegate BUG=610430 ==========
ellyjones@chromium.org changed reviewers: + juncai@chromium.org, sky@chromium.org
juncai, sky: ptal? :)
https://codereview.chromium.org/2692113002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/2692113002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:58: gfx::Size GetMinimumSize() const override; It seems that setting the chooser size is similar to what ChooserDialogView::CreateClientView() does: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/choos... I am wondering if it is possible to move the logic to DeviceChooserContentView? https://cs.chromium.org/chromium/src/chrome/browser/ui/views/device_chooser_c... It used to use the GetPreferredSize(): https://codereview.chromium.org/2654323002/diff/140001/chrome/browser/ui/view... https://codereview.chromium.org/2692113002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:148: const int kHeight = 150; Change 150 to 320, same as: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/choos... https://codereview.chromium.org/2692113002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:149: int width = LayoutDelegate::Get() I did some test on my Linux machine and this width is 0, so the chooser's size is still not correct. https://codereview.chromium.org/2692113002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:151: LayoutDelegate::DialogWidth::SMALL) Change SMALL to MEDIUM, same as: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/choos... https://codereview.chromium.org/2692113002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:152: : 300; Change 300 to 402, same as: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/choos...
juncai: ptal? :) https://codereview.chromium.org/2692113002/diff/1/chrome/browser/ui/views/web... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/2692113002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:58: gfx::Size GetMinimumSize() const override; On 2017/02/13 22:42:21, juncai wrote: > It seems that setting the chooser size is similar to what > ChooserDialogView::CreateClientView() does: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/choos... > I am wondering if it is possible to move the logic to DeviceChooserContentView? > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/device_chooser_c... > > It used to use the GetPreferredSize(): > https://codereview.chromium.org/2654323002/diff/140001/chrome/browser/ui/view... Moving it out of DeviceChooserContentView is deliberate, because Harmony specifies the width of entire dialogs, so we need to impose a certain width on the entire dialog, not just the content view. It does lead to a little code duplication, though. https://codereview.chromium.org/2692113002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:148: const int kHeight = 150; On 2017/02/13 22:42:21, juncai wrote: > Change 150 to 320, same as: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/choos... Done. https://codereview.chromium.org/2692113002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:149: int width = LayoutDelegate::Get() On 2017/02/13 22:42:21, juncai wrote: > I did some test on my Linux machine and this width is 0, so the chooser's size > is still not correct. Yep, this code is actually wrong. Fixed. https://codereview.chromium.org/2692113002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:151: LayoutDelegate::DialogWidth::SMALL) On 2017/02/13 22:42:21, juncai wrote: > Change SMALL to MEDIUM, same as: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/choos... Done. https://codereview.chromium.org/2692113002/diff/1/chrome/browser/ui/views/web... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:152: : 300; On 2017/02/13 22:42:21, juncai wrote: > Change 300 to 402, same as: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/choos... Done.
LGTM with nits. https://codereview.chromium.org/2692113002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/2692113002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:148: const int kHeight = 320; nit: here uses const int, and at: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/choos... it uses constexpr int. Maybe can make them consistent and use one style. https://codereview.chromium.org/2692113002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:149: const int kDefaultWidth = 402; ditto.
done :) sky: ptal? :) https://codereview.chromium.org/2692113002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/2692113002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:148: const int kHeight = 320; On 2017/02/14 18:30:13, juncai wrote: > nit: here uses const int, and at: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/choos... > it uses constexpr int. > > Maybe can make them consistent and use one style. Done. https://codereview.chromium.org/2692113002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:149: const int kDefaultWidth = 402; On 2017/02/14 18:30:13, juncai wrote: > ditto. Done.
https://codereview.chromium.org/2692113002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_dialog_delegate.cc (right): https://codereview.chromium.org/2692113002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_dialog_delegate.cc:234: gfx::Size size = GetWidget()->client_view()->GetPreferredSize(); Can you write test coverage of this?
sky: ptal? :) https://codereview.chromium.org/2692113002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_dialog_delegate.cc (right): https://codereview.chromium.org/2692113002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_dialog_delegate.cc:234: gfx::Size size = GetWidget()->client_view()->GetPreferredSize(); On 2017/02/14 21:24:35, sky wrote: > Can you write test coverage of this? Done.
Thanks for the test, LGTM
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from juncai@chromium.org Link to the patchset: https://codereview.chromium.org/2692113002/#ps60001 (title: "add unit test")
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ellyjones@chromium.org to run a CQ dry run
On 2017/02/16 16:06:57, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux > (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) sky: I changed BubbleFrameView::GetPreferredSize() to never return anything smaller than ::GetMinimumSize() would return - what do you think? Is this gross?
Dry run: 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
Dry run: This issue passed the CQ dry run.
On 2017/02/22 17:36:36, Elly Fong-Jones wrote: > On 2017/02/16 16:06:57, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux > > (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > sky: I changed BubbleFrameView::GetPreferredSize() to never return anything > smaller than ::GetMinimumSize() would return - what do you think? Is this gross? That does seem gross. Can you describe why you need it? I would think GetWidget()->client_view()->GetPreferredSize() should be enough.
On 2017/02/22 21:16:50, sky wrote: > On 2017/02/22 17:36:36, Elly Fong-Jones wrote: > > On 2017/02/16 16:06:57, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux > > > (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > > > sky: I changed BubbleFrameView::GetPreferredSize() to never return anything > > smaller than ::GetMinimumSize() would return - what do you think? Is this > gross? > > That does seem gross. Can you describe why you need it? I would think > GetWidget()->client_view()->GetPreferredSize() should be enough. Hm, perhaps I ought to just impose the minimum in ChooseBubbleUiViewDelegate::GetPreferredSize(). I'll try that and see if it avoids this hack.
The CQ bit was checked by ellyjones@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...
On 2017/02/23 21:35:51, Elly Fong-Jones wrote: > On 2017/02/22 21:16:50, sky wrote: > > On 2017/02/22 17:36:36, Elly Fong-Jones wrote: > > > On 2017/02/16 16:06:57, commit-bot: I haz the power wrote: > > > > Try jobs failed on following builders: > > > > linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux > > > > (JOB_FAILED, > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > > > > > sky: I changed BubbleFrameView::GetPreferredSize() to never return anything > > > smaller than ::GetMinimumSize() would return - what do you think? Is this > > gross? > > > > That does seem gross. Can you describe why you need it? I would think > > GetWidget()->client_view()->GetPreferredSize() should be enough. > > Hm, perhaps I ought to just impose the minimum in > ChooseBubbleUiViewDelegate::GetPreferredSize(). I'll try that and see if it > avoids this hack. Ok, I now have no idea why I decided to use GetMinimumSize() for this instead of GetPreferredSize(). I changed this CL over to use preferred size and it a) is simpler and b) works. sky: wdyt?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2692113002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/2692113002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:148: constexpr int kHeight = 320; I'm assuming these numbers are right. https://codereview.chromium.org/2692113002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_dialog_delegate.cc (right): https://codereview.chromium.org/2692113002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_dialog_delegate.cc:235: size.SetToMax(GetPreferredSize()); I would expect GetPreferredSize() to include the preferred size of the client view. In other words, I'm suggesting these two lines should be: const gfx::Size size = GetPreferredSize(); (or just use GetPreferredSize() below). Is GetPreferredSize() not including the preferred size of the client-view? If not, could you investigate why it isn't.
tapted@chromium.org changed reviewers: + tapted@chromium.org
https://codereview.chromium.org/2692113002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_dialog_delegate.cc (right): https://codereview.chromium.org/2692113002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_dialog_delegate.cc:235: size.SetToMax(GetPreferredSize()); On 2017/02/24 21:15:19, sky wrote: > I would expect GetPreferredSize() to include the preferred size of the client > view. In other words, I'm suggesting these two lines should be: > > const gfx::Size size = GetPreferredSize(); > > (or just use GetPreferredSize() below). > > Is GetPreferredSize() not including the preferred size of the client-view? If > not, could you investigate why it isn't. DialogClientView actually has some bugs around feeding up min/max/preferred sizes when incorporating the requests of both the buttons row and the ContentsView. I'm fixing them in https://codereview.chromium.org/2706423002/ (with a test \o/). It's possible that CL fixes things here too...
https://codereview.chromium.org/2692113002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): https://codereview.chromium.org/2692113002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:158: return device_chooser_content_view_; OK - so here's the trap :) even though ChooserBubbleUiViewDelegate is a views::BubbleDialogDelegateView, it's overriding views::BubbleDialogDelegateView::GetContentsView to return |device_chooser_content_view_| instead of |this|. |this| isn't ever actually inserted into the view hierarchy, so its GetPreferredSize is being ignored -- the method above won't be called. If we instead move that GetPreferredSize() implementation to DeviceChooserContentView, then the dialog gets the correct size without requiring the changes in BubbleDialogDelegate in this CL. I tried that with and without my pending changes in https://codereview.chromium.org/2706423002/ and it seems to layout correctly in both cases, so no need to wait for me - this should be orthogonal :)
On 2017/02/28 11:17:33, tapted wrote: > https://codereview.chromium.org/2692113002/diff/100001/chrome/browser/ui/view... > File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right): > > https://codereview.chromium.org/2692113002/diff/100001/chrome/browser/ui/view... > chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:158: return > device_chooser_content_view_; > OK - so here's the trap :) > > even though ChooserBubbleUiViewDelegate is a views::BubbleDialogDelegateView, > it's overriding views::BubbleDialogDelegateView::GetContentsView to return > |device_chooser_content_view_| instead of |this|. |this| isn't ever actually > inserted into the view hierarchy, so its GetPreferredSize is being ignored -- > the method above won't be called. Ooooh, good find! I would not have spotted this. > If we instead move that GetPreferredSize() implementation to > DeviceChooserContentView, then the dialog gets the correct size without > requiring the changes in BubbleDialogDelegate in this CL. Yep, this does work. It doesn't achieve the goal of having the sizing happen at the dialog level, but I'm not sure that is feasible without a big change to ChooserBubbleUiViewDelegate's understanding of what a "content view" is, and this is an M58 regression, so let's do this for now. > I tried that with and without my pending changes in > https://codereview.chromium.org/2706423002/ and it seems to layout correctly in > both cases, so no need to wait for me - this should be orthogonal :) Done. sky: this look better?
LGTM
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from juncai@chromium.org Link to the patchset: https://codereview.chromium.org/2692113002/#ps120001 (title: "move GetPreferredSize override")
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": 120001, "attempt_start_ts": 1488305235695120,
"parent_rev": "04a2ae170b008a068d2b5c1e992bc11b0422f5e7", "commit_rev":
"b7312b4e50b6e10b9382e6adeddf9441112db608"}
Message was sent while issue was closed.
Description was changed from ========== harmony: fix sizing on webusb chooser dialog The change in https://codereview.chromium.org/2654323002 removed the minimum size of the content view used in this dialog, but did not replace it with a dialog-level minimum size. This change: 1) Has BubbleDialogDelegateView honor its own GetMinimumSize(); 2) Implements GetMinimumSize on ChooserBubbleUiViewDelegate BUG=610430 ========== to ========== harmony: fix sizing on webusb chooser dialog The change in https://codereview.chromium.org/2654323002 removed the minimum size of the content view used in this dialog, but did not replace it with a dialog-level minimum size. This change: 1) Has BubbleDialogDelegateView honor its own GetMinimumSize(); 2) Implements GetMinimumSize on ChooserBubbleUiViewDelegate BUG=610430 Review-Url: https://codereview.chromium.org/2692113002 Cr-Commit-Position: refs/heads/master@{#453657} Committed: https://chromium.googlesource.com/chromium/src/+/b7312b4e50b6e10b9382e6adeddf... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/b7312b4e50b6e10b9382e6adeddf...
Message was sent while issue was closed.
On 2017/02/28 19:10:49, commit-bot: I haz the power wrote: > Committed patchset #7 (id:120001) as > https://chromium.googlesource.com/chromium/src/+/b7312b4e50b6e10b9382e6adeddf... BTW, since now the change is only at: //chrome/browser/ui/views/device_chooser_content_view.* The description of this CL seems out of date. Maybe update it?
Message was sent while issue was closed.
On 2017/02/28 20:00:08, juncai wrote: > On 2017/02/28 19:10:49, commit-bot: I haz the power wrote: > > Committed patchset #7 (id:120001) as > > > https://chromium.googlesource.com/chromium/src/+/b7312b4e50b6e10b9382e6adeddf... > > BTW, since now the change is only at: > //chrome/browser/ui/views/device_chooser_content_view.* > The description of this CL seems out of date. Maybe update it? ping again, :). Hey, I think the CL description needs to be updated.
Message was sent while issue was closed.
Description was changed from ========== harmony: fix sizing on webusb chooser dialog The change in https://codereview.chromium.org/2654323002 removed the minimum size of the content view used in this dialog, but did not replace it with a dialog-level minimum size. This change: 1) Has BubbleDialogDelegateView honor its own GetMinimumSize(); 2) Implements GetMinimumSize on ChooserBubbleUiViewDelegate BUG=610430 Review-Url: https://codereview.chromium.org/2692113002 Cr-Commit-Position: refs/heads/master@{#453657} Committed: https://chromium.googlesource.com/chromium/src/+/b7312b4e50b6e10b9382e6adeddf... ========== to ========== harmony: fix sizing on webusb chooser dialog The change in https://codereview.chromium.org/2654323002 removed the preferred size of the content view used in this dialog, but did not replace it with a dialog-level preferred size. This change implements GetPreferredSize() on DeviceChooserContentView. BUG=610430 Review-Url: https://codereview.chromium.org/2692113002 Cr-Commit-Position: refs/heads/master@{#453657} Committed: https://chromium.googlesource.com/chromium/src/+/b7312b4e50b6e10b9382e6adeddf... ==========
Message was sent while issue was closed.
On 2017/03/02 21:28:17, juncai wrote: > On 2017/02/28 20:00:08, juncai wrote: > > On 2017/02/28 19:10:49, commit-bot: I haz the power wrote: > > > Committed patchset #7 (id:120001) as > > > > > > https://chromium.googlesource.com/chromium/src/+/b7312b4e50b6e10b9382e6adeddf... > > > > BTW, since now the change is only at: > > //chrome/browser/ui/views/device_chooser_content_view.* > > The description of this CL seems out of date. Maybe update it? > > ping again, :). > > Hey, I think the CL description needs to be updated. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
