Cloud Print Dialog work.

Cloud Print Dialog work.

Allow an HTML dialog to suppress the title bar in a Chromium OS bubble window.
For the Cloud Print dialog, remember the last size requested by the contents.

BUG=chromium-os:8494
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=64824

[sanjeevr, 2010-11-01 21:37:54]

http://codereview.chromium.org/4110010/diff/1/3
File chrome/browser/chromeos/frame/bubble_frame_view.cc (right):

http://codereview.chromium.org/4110010/diff/1/3#newcode42
chrome/browser/chromeos/frame/bubble_frame_view.cc:42: if (!title_text.empty()
||
Is the .empty() check needed?

http://codereview.chromium.org/4110010/diff/1/10
File chrome/browser/printing/print_dialog_cloud.cc (right):

http://codereview.chromium.org/4110010/diff/1/10#newcode400
chrome/browser/printing/print_dialog_cloud.cc:400:
store_size->SetInteger("width", bounds.width());
Do we need defines for "width" and "height"?

http://codereview.chromium.org/4110010/diff/1/10#newcode453
chrome/browser/printing/print_dialog_cloud.cc:453: return std::wstring();
Should we remove the IDS_CLOUD_PRINT_TITLE from the .grd?

http://codereview.chromium.org/4110010/diff/1/10#newcode527
chrome/browser/printing/print_dialog_cloud.cc:527: int width = 497, height =
332;
Can we have 2 const ints here kDefaultDialogWidth and kDefaultDialogHeight?

[Scott Byer, 2010-11-01 22:59:42]

Note the better implementation of
CloudPrintFlowHandler::StoreDialogClientSize().

http://codereview.chromium.org/4110010/diff/1/3
File chrome/browser/chromeos/frame/bubble_frame_view.cc (right):

http://codereview.chromium.org/4110010/diff/1/3#newcode42
chrome/browser/chromeos/frame/bubble_frame_view.cc:42: if (!title_text.empty()
||
On 2010/11/01 21:37:54, sanjeevr wrote:
> Is the .empty() check needed?

Done.

http://codereview.chromium.org/4110010/diff/1/10
File chrome/browser/printing/print_dialog_cloud.cc (right):

http://codereview.chromium.org/4110010/diff/1/10#newcode400
chrome/browser/printing/print_dialog_cloud.cc:400:
store_size->SetInteger("width", bounds.width());
On 2010/11/01 21:37:54, sanjeevr wrote:
> Do we need defines for "width" and "height"?

Good idea.

Done.

http://codereview.chromium.org/4110010/diff/1/10#newcode453
chrome/browser/printing/print_dialog_cloud.cc:453: return std::wstring();
On 2010/11/01 21:37:54, sanjeevr wrote:
> Should we remove the IDS_CLOUD_PRINT_TITLE from the .grd?

Done.

http://codereview.chromium.org/4110010/diff/1/10#newcode527
chrome/browser/printing/print_dialog_cloud.cc:527: int width = 497, height =
332;
On 2010/11/01 21:37:54, sanjeevr wrote:
> Can we have 2 const ints here kDefaultDialogWidth and kDefaultDialogHeight?

Done.

http://codereview.chromium.org/4110010/diff/58001/59013#newcode387
chrome/browser/printing/print_dialog_cloud.cc:387: void
CloudPrintFlowHandler::StoreDialogClientSize() const {
Much more reasonable way of getting the final contents size.

[sanjeevr, 2010-11-01 23:07:08]

LGTM with a couple of nits.

http://codereview.chromium.org/4110010/diff/58001/59004
File chrome/browser/chromeos/frame/bubble_frame_view.cc (right):

http://codereview.chromium.org/4110010/diff/58001/59004#newcode43
chrome/browser/chromeos/frame/bubble_frame_view.cc:43: title_ = new
views::Label(title_text);
Nit: you don't need the temp title_text variable any more.

http://codereview.chromium.org/4110010/diff/58001/59013
File chrome/browser/printing/print_dialog_cloud.cc (right):

http://codereview.chromium.org/4110010/diff/58001/59013#newcode99
chrome/browser/printing/print_dialog_cloud.cc:99: #define kWidthString "width"
Nit: Do these defines belong in pref_names.cc/.h ? Also these should be defined
as const char[] instead of #defines.

[Scott Byer, 2010-11-01 23:41:15]

With direct paths for the prefs.

[sanjeevr, 2010-11-01 23:46:58]

LGTM