Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(119)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 23460022: Fix print preview size calculation. (Closed)

Created:
7 years, 3 months ago by Rune Fevang
Modified:
7 years, 3 months ago
CC:
chromium-reviews
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Fix print preview size calculation. The size of the print preview should be based off of the available area for dialogs, not the size of the web contents view. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221957

Patch Set 1 #

Total comments: 2

Patch Set 2 : comment #

Total comments: 2

Patch Set 3 : WebContentsModalDialogHost can be NULL #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -9 lines) Patch
M chrome/browser/printing/print_preview_dialog_controller.cc View 1 2 2 chunks +12 lines, -8 lines 0 comments Download
M chrome/browser/ui/browser_window.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 11 (0 generated)
Rune Fevang
7 years, 3 months ago (2013-09-06 20:59:07 UTC) #1
Vitaly Buka (NO REVIEWS)
lgtm https://codereview.chromium.org/23460022/diff/1/chrome/browser/printing/print_preview_dialog_controller.cc File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/23460022/diff/1/chrome/browser/printing/print_preview_dialog_controller.cc#newcode133 chrome/browser/printing/print_preview_dialog_controller.cc:133: GetMaximumDialogSize(); Shouldn't we put at least kMinDialogSize in ...
7 years, 3 months ago (2013-09-06 21:14:29 UTC) #2
Rune Fevang
https://codereview.chromium.org/23460022/diff/1/chrome/browser/printing/print_preview_dialog_controller.cc File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/23460022/diff/1/chrome/browser/printing/print_preview_dialog_controller.cc#newcode133 chrome/browser/printing/print_preview_dialog_controller.cc:133: GetMaximumDialogSize(); On 2013/09/06 21:14:29, Vitaly Buka wrote: > Shouldn't ...
7 years, 3 months ago (2013-09-06 22:27:55 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rfevang@chromium.org/23460022/5001
7 years, 3 months ago (2013-09-06 22:28:13 UTC) #4
Mike Wittman
https://codereview.chromium.org/23460022/diff/5001/chrome/browser/printing/print_preview_dialog_controller.cc File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/23460022/diff/5001/chrome/browser/printing/print_preview_dialog_controller.cc#newcode134 chrome/browser/printing/print_preview_dialog_controller.cc:134: *size = browser->window()->GetWebContentsModalDialogHost()-> Probably should check the value returned ...
7 years, 3 months ago (2013-09-06 22:37:31 UTC) #5
Rune Fevang
https://codereview.chromium.org/23460022/diff/5001/chrome/browser/printing/print_preview_dialog_controller.cc File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/23460022/diff/5001/chrome/browser/printing/print_preview_dialog_controller.cc#newcode134 chrome/browser/printing/print_preview_dialog_controller.cc:134: *size = browser->window()->GetWebContentsModalDialogHost()-> On 2013/09/06 22:37:31, Mike Wittman wrote: ...
7 years, 3 months ago (2013-09-06 22:53:07 UTC) #6
Mike Wittman
On 2013/09/06 22:53:07, Rune Fevang wrote: > https://codereview.chromium.org/23460022/diff/5001/chrome/browser/printing/print_preview_dialog_controller.cc > File chrome/browser/printing/print_preview_dialog_controller.cc (right): > > https://codereview.chromium.org/23460022/diff/5001/chrome/browser/printing/print_preview_dialog_controller.cc#newcode134 ...
7 years, 3 months ago (2013-09-06 22:58:30 UTC) #7
Rune Fevang
sky, could you do the OWNER review for the browser_window file?
7 years, 3 months ago (2013-09-06 22:59:58 UTC) #8
sky
LGTM
7 years, 3 months ago (2013-09-08 02:36:53 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rfevang@chromium.org/23460022/27001
7 years, 3 months ago (2013-09-08 07:59:47 UTC) #10
commit-bot: I haz the power
7 years, 3 months ago (2013-09-08 23:34:47 UTC) #11
Message was sent while issue was closed. 
Change committed as 221957

Powered by Google App Engine
This is Rietveld 408576698