Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2932243002:
Refactor ChooserDialogView to use borders instead of content margins. (Closed)
DescriptionRefactor ChooserDialogView to use borders instead of content margins.
To simplify DialogDelegate::CreateDialogFrameView, this patch removes
the ability of callers to specify content margins. The only callsite
that was using special margins, ChooserDialogView, was refactored to
use borders like other DialogClientView subclasses. This changes its
layout slightly.
Also added a BrowserDialogTest for ChooserDialogView.
BUG=654137
Review-Url: https://codereview.chromium.org/2932243002
Cr-Commit-Position: refs/heads/master@{#480194}
Committed: https://chromium.googlesource.com/chromium/src/+/04d6c626efe0af31662061779ca60cc651062374
Patch Set 1 #Patch Set 2 : fix merge #Patch Set 3 : use constants for signal strength #
Total comments: 8
Patch Set 4 : GetTitle not virtual #
Total comments: 6
Patch Set 5 : nits #Messages
Total messages: 43 (21 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bsep@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right): https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view.cc:81: views::INSETS_DIALOG_CONTENTS))); Does this lead to doubled borders in some places (e.g. top of footnote view + bottom of content above)? https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:32: new ChooserDialogView(std::move(controller_)), web_contents); This doesn't leak, does it? It was hard for me to try to trace ownership through the API, and while other code does do this sort of thing in some places, I'm not convinced that means it's correct... https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:39: std::unique_ptr<MockChooserController> controller_; Nit: Why not own this by value instead of pointer?
https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right): https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view.cc:81: views::INSETS_DIALOG_CONTENTS))); On 2017/06/12 22:36:51, Peter Kasting wrote: > Does this lead to doubled borders in some places (e.g. top of footnote view + > bottom of content above)? No, the footnote is set off from the content by a line. This looks very similar to how it was before. https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:32: new ChooserDialogView(std::move(controller_)), web_contents); On 2017/06/12 22:36:51, Peter Kasting wrote: > This doesn't leak, does it? It was hard for me to try to trace ownership > through the API, and while other code does do this sort of thing in some places, > I'm not convinced that means it's correct... It doesn't leak, the delegate is owned by the Widget (and so gets destroyed by Widget::OnNativeWidgetDestroyed). https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view_browsertest.cc:39: std::unique_ptr<MockChooserController> controller_; On 2017/06/12 22:36:51, Peter Kasting wrote: > Nit: Why not own this by value instead of pointer? We transfer ownership to ChooserDialogView once the dialog is shown.
LGTM
The CQ bit was checked by bsep@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Refactor ChooserDialogView to use borders instead of content margins. To simplify DialogDelegate::CreateDialogFrameView, this patch removes the ability of callers to specify content margins. The only callsite that was using special margins, ChooserDialogView, was refactored to use borders like other DialogClientView subclasses. This changes its layout slightly. Also added a BrowserDialogTest for ChooserDialogView. BUG=654137 ========== to ========== Refactor ChooserDialogView to use borders instead of content margins. To simplify DialogDelegate::CreateDialogFrameView, this patch removes the ability of callers to specify content margins. The only callsite that was using special margins, ChooserDialogView, was refactored to use borders like other DialogClientView subclasses. This changes its layout slightly. Also added a BrowserDialogTest for ChooserDialogView. BUG=654137 ==========
bsep@chromium.org changed reviewers: + tapted@chromium.org
tapted@: just need ui/views OWNERS, thanks!
On 2017/06/14 01:15:28, Bret wrote: > tapted@: just need ui/views OWNERS, thanks! lgtm
The CQ bit was checked by bsep@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
bsep@chromium.org changed reviewers: + jyasskin@chromium.org
jyasskin@: need OWNERS for chrome/browser/chooser_controller PTAL, thanks!
https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/chooser_... File chrome/browser/chooser_controller/chooser_controller.h (right): https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/chooser_... chrome/browser/chooser_controller/chooser_controller.h:65: virtual base::string16 GetTitle() const; I'm a little unhappy with making this configurable by subclasses, while still providing the default implementation with a member variable in the base here. Why do subclasses need to customize this?
https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/chooser_... File chrome/browser/chooser_controller/chooser_controller.h (right): https://codereview.chromium.org/2932243002/diff/40001/chrome/browser/chooser_... chrome/browser/chooser_controller/chooser_controller.h:65: virtual base::string16 GetTitle() const; On 2017/06/15 21:33:42, Jeffrey Yasskin wrote: > I'm a little unhappy with making this configurable by subclasses, while still > providing the default implementation with a member variable in the base here. > Why do subclasses need to customize this? It's so that MockChooserController can have a title for the browser test. But you're right, it's a bit wonky. I changed it to a protected set_title method, what do you think?
lgtm https://codereview.chromium.org/2932243002/diff/60001/chrome/browser/chooser_... File chrome/browser/chooser_controller/chooser_controller.h (right): https://codereview.chromium.org/2932243002/diff/60001/chrome/browser/chooser_... chrome/browser/chooser_controller/chooser_controller.h:134: void set_title(base::string16 title) { title_ = title; } Either const base::string16& or std::move(title). I never remember which one our style guide prefers. https://codereview.chromium.org/2932243002/diff/60001/chrome/browser/chooser_... File chrome/browser/chooser_controller/mock_chooser_controller.cc (right): https://codereview.chromium.org/2932243002/diff/60001/chrome/browser/chooser_... chrome/browser/chooser_controller/mock_chooser_controller.cc:16: IDS_USB_DEVICE_CHOOSER_PROMPT_ORIGIN, Note that this superclass constructor call does set the title, just to a maybe-nonobvious value. You might not need this change at all.
juncai@chromium.org changed reviewers: + juncai@chromium.org
https://codereview.chromium.org/2932243002/diff/60001/chrome/browser/chooser_... File chrome/browser/chooser_controller/chooser_controller.h (right): https://codereview.chromium.org/2932243002/diff/60001/chrome/browser/chooser_... chrome/browser/chooser_controller/chooser_controller.h:134: void set_title(base::string16 title) { title_ = title; } drive-by comment: Since this function is only for testing, maybe add "_for_testing" explicitly in its function name to be: "set_title_for_testing()".
https://codereview.chromium.org/2932243002/diff/60001/chrome/browser/chooser_... File chrome/browser/chooser_controller/chooser_controller.h (right): https://codereview.chromium.org/2932243002/diff/60001/chrome/browser/chooser_... chrome/browser/chooser_controller/chooser_controller.h:134: void set_title(base::string16 title) { title_ = title; } On 2017/06/15 22:06:30, Jeffrey Yasskin wrote: > Either const base::string16& or std::move(title). I never remember which one our > style guide prefers. Since I just looked this up an hour ago: we prefer passing strings by const ref to pass-by-value-and-move.
https://codereview.chromium.org/2932243002/diff/60001/chrome/browser/chooser_... File chrome/browser/chooser_controller/chooser_controller.h (right): https://codereview.chromium.org/2932243002/diff/60001/chrome/browser/chooser_... chrome/browser/chooser_controller/chooser_controller.h:134: void set_title(base::string16 title) { title_ = title; } On 2017/06/15 22:26:14, Peter Kasting wrote: > On 2017/06/15 22:06:30, Jeffrey Yasskin wrote: > > Either const base::string16& or std::move(title). I never remember which one > our > > style guide prefers. > > Since I just looked this up an hour ago: we prefer passing strings by const ref > to pass-by-value-and-move. All done. https://codereview.chromium.org/2932243002/diff/60001/chrome/browser/chooser_... File chrome/browser/chooser_controller/mock_chooser_controller.cc (right): https://codereview.chromium.org/2932243002/diff/60001/chrome/browser/chooser_... chrome/browser/chooser_controller/mock_chooser_controller.cc:16: IDS_USB_DEVICE_CHOOSER_PROMPT_ORIGIN, On 2017/06/15 22:06:30, Jeffrey Yasskin wrote: > Note that this superclass constructor call does set the title, just to a > maybe-nonobvious value. You might not need this change at all. Not if the owner pointer is null. I figured setting the title directly was better than drumming up a mock RenderFrameHost just for that.
Thanks! LGTM.
The CQ bit was checked by bsep@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, pkasting@chromium.org, jyasskin@chromium.org Link to the patchset: https://codereview.chromium.org/2932243002/#ps80001 (title: "nits")
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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by bsep@chromium.org
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": 80001, "attempt_start_ts": 1497649240420290,
"parent_rev": "a7494e43f3d82f3e6232d8b7d26aa175f617aead", "commit_rev":
"04d6c626efe0af31662061779ca60cc651062374"}
Message was sent while issue was closed.
Description was changed from ========== Refactor ChooserDialogView to use borders instead of content margins. To simplify DialogDelegate::CreateDialogFrameView, this patch removes the ability of callers to specify content margins. The only callsite that was using special margins, ChooserDialogView, was refactored to use borders like other DialogClientView subclasses. This changes its layout slightly. Also added a BrowserDialogTest for ChooserDialogView. BUG=654137 ========== to ========== Refactor ChooserDialogView to use borders instead of content margins. To simplify DialogDelegate::CreateDialogFrameView, this patch removes the ability of callers to specify content margins. The only callsite that was using special margins, ChooserDialogView, was refactored to use borders like other DialogClientView subclasses. This changes its layout slightly. Also added a BrowserDialogTest for ChooserDialogView. BUG=654137 Review-Url: https://codereview.chromium.org/2932243002 Cr-Commit-Position: refs/heads/master@{#480194} Committed: https://chromium.googlesource.com/chromium/src/+/04d6c626efe0af31662061779ca6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/04d6c626efe0af31662061779ca6... |
