Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1680533003:
[Media Router] Enable autoresizing for the Media Router dialog for Mac. (Closed)
Description[Media Router] Enable autoresizing for the Media Router dialog for Mac.
This change uses an autoresizing dialog for Media Router on Macs rather than a fixed size dialog. Previously, the dialog could only autoresize on Linux and Windows.
Screenshots - dynamic sizing!
https://screenshot.googleplex.com/vKTA1A7HXFu.png
https://screenshot.googleplex.com/suVDcHMjuMY.png
https://screenshot.googleplex.com/kZkoCD6n9tu.png
https://screenshot.googleplex.com/iQpxy84e9CB.png
https://screenshot.googleplex.com/7aYGEVB0ggX.png
https://screenshot.googleplex.com/E3XZLwUGBii.png
https://screenshot.googleplex.com/nBNTjvbEmjT.png
https://screenshot.googleplex.com/QSEcTzbicV8.png
BUG=508544, 580272, 580273
Committed: https://crrev.com/f285202a82526e75efe12b52f5b6ab62b9b6bacb
Cr-Commit-Position: refs/heads/master@{#379659}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Changes per kmarshall@'s comments. #Messages
Total messages: 19 (11 generated)
Description was changed from ========== autoresize BUG= ========== to ========== [Media Router] Enable autoresizing for the Media Router dialog for Mac. This change uses an autoresizing dialog for Media Router on Macs rather than a fixed size dialog. Previously, the dialog could only autoresize on Linux and Windows. BUG=508544 ==========
Description was changed from ========== [Media Router] Enable autoresizing for the Media Router dialog for Mac. This change uses an autoresizing dialog for Media Router on Macs rather than a fixed size dialog. Previously, the dialog could only autoresize on Linux and Windows. BUG=508544 ========== to ========== [Media Router] Enable autoresizing for the Media Router dialog for Mac. This change uses an autoresizing dialog for Media Router on Macs rather than a fixed size dialog. Previously, the dialog could only autoresize on Linux and Windows. Screenshots - dynamic sizing! https://screenshot.googleplex.com/KHc9sZTiLrU.png https://screenshot.googleplex.com/fWFGOLfQYha.png https://screenshot.googleplex.com/PkhbvC4g7Vp.png BUG=508544 ==========
Description was changed from ========== [Media Router] Enable autoresizing for the Media Router dialog for Mac. This change uses an autoresizing dialog for Media Router on Macs rather than a fixed size dialog. Previously, the dialog could only autoresize on Linux and Windows. Screenshots - dynamic sizing! https://screenshot.googleplex.com/KHc9sZTiLrU.png https://screenshot.googleplex.com/fWFGOLfQYha.png https://screenshot.googleplex.com/PkhbvC4g7Vp.png BUG=508544 ========== to ========== [Media Router] Enable autoresizing for the Media Router dialog for Mac. This change uses an autoresizing dialog for Media Router on Macs rather than a fixed size dialog. Previously, the dialog could only autoresize on Linux and Windows. Screenshots - dynamic sizing! https://screenshot.googleplex.com/KHc9sZTiLrU.png https://screenshot.googleplex.com/fWFGOLfQYha.png https://screenshot.googleplex.com/PkhbvC4g7Vp.png BUG=508544, 580272, 580273 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Media Router] Enable autoresizing for the Media Router dialog for Mac. This change uses an autoresizing dialog for Media Router on Macs rather than a fixed size dialog. Previously, the dialog could only autoresize on Linux and Windows. Screenshots - dynamic sizing! https://screenshot.googleplex.com/KHc9sZTiLrU.png https://screenshot.googleplex.com/fWFGOLfQYha.png https://screenshot.googleplex.com/PkhbvC4g7Vp.png BUG=508544, 580272, 580273 ========== to ========== [Media Router] Enable autoresizing for the Media Router dialog for Mac. This change uses an autoresizing dialog for Media Router on Macs rather than a fixed size dialog. Previously, the dialog could only autoresize on Linux and Windows. Screenshots - dynamic sizing! https://screenshot.googleplex.com/vKTA1A7HXFu.png https://screenshot.googleplex.com/suVDcHMjuMY.png https://screenshot.googleplex.com/kZkoCD6n9tu.png https://screenshot.googleplex.com/iQpxy84e9CB.png https://screenshot.googleplex.com/7aYGEVB0ggX.png https://screenshot.googleplex.com/E3XZLwUGBii.png https://screenshot.googleplex.com/nBNTjvbEmjT.png https://screenshot.googleplex.com/QSEcTzbicV8.png BUG=508544, 580272, 580273 ==========
apacible@chromium.org changed reviewers: + imcheng@chromium.org
PTAL, thanks!
apacible@chromium.org changed reviewers: + kmarshall@chromium.org - imcheng@chromium.org
-imcheng +kmarshall PTAL, thanks!
lgtm % nits https://codereview.chromium.org/1680533003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/1680533003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:239: // |web_dialog_delegate|'s owner is |constrained_delegate|. Move this comment above the |web_dialog_delegate| declaration? https://codereview.chromium.org/1680533003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:241: gfx::Size min_size = gfx::Size(kWidth, kMinHeight); I think you can inline these size objects, since the kConstant already calls out if this is a min or max value
https://codereview.chromium.org/1680533003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc (right): https://codereview.chromium.org/1680533003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:239: // |web_dialog_delegate|'s owner is |constrained_delegate|. On 2016/03/07 20:56:00, Kevin M wrote: > Move this comment above the |web_dialog_delegate| declaration? Done. https://codereview.chromium.org/1680533003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_dialog_controller_impl.cc:241: gfx::Size min_size = gfx::Size(kWidth, kMinHeight); On 2016/03/07 20:56:00, Kevin M wrote: > I think you can inline these size objects, since the kConstant already calls out > if this is a min or max value Done.
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/1680533003/#ps40001 (title: "Changes per kmarshall@'s comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1680533003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1680533003/40001
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Enable autoresizing for the Media Router dialog for Mac. This change uses an autoresizing dialog for Media Router on Macs rather than a fixed size dialog. Previously, the dialog could only autoresize on Linux and Windows. Screenshots - dynamic sizing! https://screenshot.googleplex.com/vKTA1A7HXFu.png https://screenshot.googleplex.com/suVDcHMjuMY.png https://screenshot.googleplex.com/kZkoCD6n9tu.png https://screenshot.googleplex.com/iQpxy84e9CB.png https://screenshot.googleplex.com/7aYGEVB0ggX.png https://screenshot.googleplex.com/E3XZLwUGBii.png https://screenshot.googleplex.com/nBNTjvbEmjT.png https://screenshot.googleplex.com/QSEcTzbicV8.png BUG=508544, 580272, 580273 ========== to ========== [Media Router] Enable autoresizing for the Media Router dialog for Mac. This change uses an autoresizing dialog for Media Router on Macs rather than a fixed size dialog. Previously, the dialog could only autoresize on Linux and Windows. Screenshots - dynamic sizing! https://screenshot.googleplex.com/vKTA1A7HXFu.png https://screenshot.googleplex.com/suVDcHMjuMY.png https://screenshot.googleplex.com/kZkoCD6n9tu.png https://screenshot.googleplex.com/iQpxy84e9CB.png https://screenshot.googleplex.com/7aYGEVB0ggX.png https://screenshot.googleplex.com/E3XZLwUGBii.png https://screenshot.googleplex.com/nBNTjvbEmjT.png https://screenshot.googleplex.com/QSEcTzbicV8.png BUG=508544, 580272, 580273 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Enable autoresizing for the Media Router dialog for Mac. This change uses an autoresizing dialog for Media Router on Macs rather than a fixed size dialog. Previously, the dialog could only autoresize on Linux and Windows. Screenshots - dynamic sizing! https://screenshot.googleplex.com/vKTA1A7HXFu.png https://screenshot.googleplex.com/suVDcHMjuMY.png https://screenshot.googleplex.com/kZkoCD6n9tu.png https://screenshot.googleplex.com/iQpxy84e9CB.png https://screenshot.googleplex.com/7aYGEVB0ggX.png https://screenshot.googleplex.com/E3XZLwUGBii.png https://screenshot.googleplex.com/nBNTjvbEmjT.png https://screenshot.googleplex.com/QSEcTzbicV8.png BUG=508544, 580272, 580273 ========== to ========== [Media Router] Enable autoresizing for the Media Router dialog for Mac. This change uses an autoresizing dialog for Media Router on Macs rather than a fixed size dialog. Previously, the dialog could only autoresize on Linux and Windows. Screenshots - dynamic sizing! https://screenshot.googleplex.com/vKTA1A7HXFu.png https://screenshot.googleplex.com/suVDcHMjuMY.png https://screenshot.googleplex.com/kZkoCD6n9tu.png https://screenshot.googleplex.com/iQpxy84e9CB.png https://screenshot.googleplex.com/7aYGEVB0ggX.png https://screenshot.googleplex.com/E3XZLwUGBii.png https://screenshot.googleplex.com/nBNTjvbEmjT.png https://screenshot.googleplex.com/QSEcTzbicV8.png BUG=508544, 580272, 580273 Committed: https://crrev.com/f285202a82526e75efe12b52f5b6ab62b9b6bacb Cr-Commit-Position: refs/heads/master@{#379659} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f285202a82526e75efe12b52f5b6ab62b9b6bacb Cr-Commit-Position: refs/heads/master@{#379659}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:40001) has been created in https://codereview.chromium.org/1773073002/ by apacible@chromium.org. The reason for reverting is: Autoresizing currently breaks Print Preview. This patch needs to be reverted first since it depends on autoresizing.. |
