PrintPreview: Make ctrl-shift-p start the native print flow.

PrintPreview: Make ctrl-shift-p start the native print flow.

+ Added PrintPreviewUITest.AdvancedPrintCommandEnabled


BUG=93819
TEST=Open a tab. Press ctrl+shift+p to start native print workflow.

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

[kmadhusu, 2011-08-23 23:59:10]

thestig: I need to test these changes on win & mac to make sure the key
combinations work. Code is ready for linux. Please review and let me know your
comments.

[Lei Zhang, 2011-08-24 01:27:28]

http://codereview.chromium.org/7721001/diff/2001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7721001/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6011: (Shift+Ctrl+P)
I'm sure you are aware, but this needs to be platform specific.

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.cc:99: // Preview tab exist for
current tab or current tab is preview tab.
In this case, I think you just want to close |print_preview_tab| and call
PrintForSystemDialogNow(), instead of sending a message to the renderer and have
it call back out to the browser.

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4168:
command_updater_.UpdateCommandEnabled(IDC_ADVANCED_PRINT, true);
You don't always want to do this. For example, if a PDF has the "do not print"
bit set, this will let you print such a PDF.

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:665: void
PrintPreviewHandler::HandleShowSystemDialog(const ListValue*) {
This is just going to conflict with my cleanup CL.

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:681:
print_preview_ui->OnPrintPreviewRequest(-1);
Let's add a new method to PrintPreviewUI to do cancellations.

[kmadhusu, 2011-08-25 02:15:20]

http://codereview.chromium.org/7721001/diff/2001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7721001/diff/2001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6011: (Shift+Ctrl+P)
On 2011/08/24 01:27:28, Lei Zhang wrote:
> I'm sure you are aware, but this needs to be platform specific.

Done.

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.cc:99: // Preview tab exist for
current tab or current tab is preview tab.
On 2011/08/24 01:27:28, Lei Zhang wrote:
> In this case, I think you just want to close |print_preview_tab| and call
> PrintForSystemDialogNow(), instead of sending a message to the renderer and
have
> it call back out to the browser.

I tried to delete preview tab wrapper and to call PrintForSystemDialogNow() and
I got segmentation fault.

#0  0x00007ffff1c7ca96 in RenderViewHostManager::interstitial_page (this=0x160)
at ./content/browser/tab_contents/render_view_host_manager.h:170
#1  0x00007ffff1c7cb06 in TabContents::showing_interstitial_page (this=0x0) at
./content/browser/tab_contents/tab_contents.h:373
#2  0x00007ffff1c80d12 in printing::PrintViewManager::PrintNowInternal
(this=0x7fffd5f08200, message=0x7fffd86feaa0) at
chrome/browser/printing/print_view_manager.cc:539
#3  0x00007ffff1c7f0ca in printing::PrintViewManager::PrintForSystemDialogNow
(this=0x7fffd5f08200) at chrome/browser/printing/print_view_manager.cc:88
#4  0x00007ffff1c7f150 in printing::PrintViewManager::AdvancedPrintNow
(this=0x7fffd5f08200) at chrome/browser/printing/print_view_manager.cc:107
#5  0x00007ffff1da82ba in Browser::AdvancedPrint (this=0x7fffdedf5800) at
chrome/browser/ui/browser.cc:1762
#6  0x00007ffff1dab18b in Browser::ExecuteCommandWithDisposition
(this=0x7fffdedf5800, id=35007, disposition=CURRENT_TAB) at
chrome/browser/ui/browser.cc:2438
#7  0x00007ffff1dabd0b in Browser::ExecuteCommand (this=0x7fffdedf5800,
id=35007) at chrome/browser/ui/browser.cc:2686
#8  0x00007ffff1dab64f in Browser::ExecuteCommandIfEnabled (this=0x7fffdedf5800,
id=35007) at chrome/browser/ui/browser.cc:2554
#9  0x00007ffff1dd8c04 in BrowserWindowGtk::OnGtkAccelerator(GtkAccelGroup *,
GObject *, guint, <anonymous enum>, void *) (accel_group=0x7fffdc3d0600,
acceleratable=0x7ffff0d9ab20, keyval=112, modifier=5, user_data=0x88bf) at
chrome/browser/ui/gtk/browser_window_gtk.cc:2009
#10 0x00007fffefa35aa0 in _gtk_marshal_BOOLEAN__OBJECT_UINT_FLAGS
(closure=0x7fffdc445b40, return_value=0x7fffffffa3f0, n_param_values=<value
optimized out>, param_values=0x7fffdc415cc0, invocation_hint=<value optimized
out>, marshal_data=0x7ffff1dd8b0c) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkmarshalers.c:367
#11 0x00007fffedf5c5de in IA__g_closure_invoke (closure=0x7fffdc445b40,
return_value=0x7fffffffa3f0, n_param_values=4, param_values=0x7fffdc415cc0,
invocation_hint=0x7fffffffa3b0) at
/tmp/glib2.0.0xzuTt/glib2.0-2.24.1/gobject/gclosure.c:767
#12 0x00007fffedf70598 in signal_emit_unlocked_R (node=0x7fffdee39c80,
detail=<value optimized out>, instance=<value optimized out>,
emission_return=<value optimized out>, instance_and_params=<value optimized
out>) at /tmp/glib2.0.0xzuTt/glib2.0-2.24.1/gobject/gsignal.c:3248
#13 0x00007fffedf718b9 in IA__g_signal_emit_valist (instance=0x7fffdc3d0600,
signal_id=<value optimized out>, detail=1426, var_args=0x7fffffffa5a0) at
/tmp/glib2.0.0xzuTt/glib2.0-2.24.1/gobject/gsignal.c:2991
#14 0x00007fffedf72033 in IA__g_signal_emit (instance=0x160,
signal_id=3631213216, detail=3631213216) at
/tmp/glib2.0.0xzuTt/glib2.0-2.24.1/gobject/gsignal.c:3038
#15 0x00007fffef96c554 in IA__gtk_accel_group_activate
(accel_group=0x7fffdc3d0600, accel_quark=1426, acceleratable=0x7ffff0d9ab20,
accel_key=112, accel_mods=5) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkaccelgroup.c:889
#16 0x00007fffef96c65d in IA__gtk_accel_groups_activate (object=0x7ffff0d9ab20,
accel_key=112, accel_mods=5) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkaccelgroup.c:926
#17 0x00007fffefb605b1 in IA__gtk_window_activate_key (window=0x7ffff0d9ab20,
event=<value optimized out>) at
/build/buildd/gtk+2.0-2.20.1/gtk/gtkwindow.c:8316
#18 0x00007ffff1dd514c in BrowserWindowGtk::HandleKeyboardEvent
(this=0x7fffdee06840, event=...) at
chrome/browser/ui/gtk/browser_window_gtk.cc:1111
#19 0x00007ffff1daf19f in Browser::HandleKeyboardEvent (this=0x7fffdedf5800,
event=...) at chrome/browser/ui/browser.cc:3501
#20 0x00007ffff48d2bde in TabContents::HandleKeyboardEvent (this=0x7fffd8805d00,
event=...) at content/browser/tab_contents/tab_contents.cc:481
#21 0x00007ffff486c244 in RenderViewHost::UnhandledKeyboardEvent
(this=0x7fffdbe09480, event=...) at
content/browser/renderer_host/render_view_host.cc:1130
#22 0x00007ffff487f166 in RenderWidgetHost::ProcessKeyboardEventAck
(this=0x7fffdbe09480, type=7, processed=false) at
content/browser/renderer_host/render_widget_host.cc:1230
#23 0x00007ffff487e5f3 in RenderWidgetHost::OnMsgInputEventAck
(this=0x7fffdbe09480, message=...) at
content/browser/renderer_host/render_widget_host.cc:1022
#24 0x00007ffff487fe8e in IPC::Message::Dispatch<RenderWidgetHost,
RenderWidgetHost> (msg=0x7fffd64bb6f8, obj=0x7fffdbe09480,
sender=0x7fffdbe09480, func=(void (RenderWidgetHost::*)(RenderWidgetHost *,
const IPC::Message &)) 0x7ffff487e0fc
<RenderWidgetHost::OnMsgInputEventAck(IPC::Message const&)>) at
./ipc/ipc_message.h:151
#25 0x00007ffff487a605 in RenderWidgetHost::OnMessageReceived
(this=0x7fffdbe09480, msg=...) at
content/browser/renderer_host/render_widget_host.cc:174
#26 0x00007ffff486ab33 in RenderViewHost::OnMessageReceived
(this=0x7fffdbe09480, msg=...) at
content/browser/renderer_host/render_view_host.cc:722
#27 0x00007ffff481d647 in BrowserRenderProcessHost::OnMessageReceived
(this=0x7fffdc444000, msg=...) at
content/browser/renderer_host/browser_render_process_host.cc:786
#28 0x00007ffff349c86e in IPC::ChannelProxy::Context::OnDispatchMessage
(this=0x7fffdc447aa0, message=...) at ipc/ipc_channel_proxy.cc:256
#29 0x00007ffff349ed6d in DispatchToMethod<IPC::ChannelProxy::Context, void
(IPC::ChannelProxy::Context::*)(IPC::Message const&), IPC::Message>
(obj=0x7fffdc447aa0, method=(void
(IPC::ChannelProxy::Context::*)(IPC::ChannelProxy::Context *, const IPC::Message
&)) 0x7ffff349c7c4 <IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message
const&)>, arg=...) at ./base/tuple.h:551
#30 0x00007ffff349ea94 in RunnableMethod<IPC::ChannelProxy::Context, void
(IPC::ChannelProxy::Context::*)(IPC::Message const&), Tuple1<IPC::Message>
>::Run (this=0x7fffd64bb6c0) at ./base/task.h:349
#31 0x00007ffff27d8d77 in base::subtle::TaskClosureAdapter::Run
(this=0x7fffcf8c04e0) at base/task.cc:56
#32 0x00007ffff2794e80 in base::internal::Invoker1<false,
base::internal::InvokerStorage1<void (base::subtle::TaskClosureAdapter::*)(),
base::subtle::TaskClosureAdapter*>, void
(base::subtle::TaskClosureAdapter::*)()>::DoInvoke (base=0x7fffcf8c04b0) at
./base/bind_internal.h:595
#33 0x00007ffff2244765 in base::Callback<void()>::Run(void) const
(this=0x7fffffffbb80) at ./base/callback.h:265
#34 0x00007ffff2791f81 in MessageLoop::RunTask (this=0x7ffff0ddde00,
pending_task=...) at base/message_loop.cc:476
#35 0x00007ffff2792089 in MessageLoop::DeferOrRunPendingTask
(this=0x7ffff0ddde00, pending_task=...) at base/message_loop.cc:492
#36 0x00007ffff279289f in MessageLoop::DoWork (this=0x7ffff0ddde00) at
base/message_loop.cc:682
#37 0x00007ffff27f6dc1 in base::MessagePumpGlib::HandleDispatch
(this=0x7ffff0dc1460) at base/message_pump_glib.cc:275
#38 0x00007ffff27f6313 in (anonymous namespace)::WorkSourceDispatch
(source=0x7ffff0dc13f0, unused_func=0, unused_data=0x0) at
base/message_pump_glib.cc:108
#39 0x00007fffed8a68c2 in g_main_dispatch (context=0x7ffff0d9ba90) at
/tmp/glib2.0.0xzuTt/glib2.0-2.24.1/glib/gmain.c:1960
#40 IA__g_main_context_dispatch (context=0x7ffff0d9ba90) at
/tmp/glib2.0.0xzuTt/glib2.0-2.24.1/glib/gmain.c:2513
#41 0x00007fffed8aa748 in g_main_context_iterate (context=0x7ffff0d9ba90,
block=<value optimized out>, dispatch=<value optimized out>, self=<value
optimized out>) at /tmp/glib2.0.0xzuTt/glib2.0-2.24.1/glib/gmain.c:2591
#42 0x00007fffed8aa8fc in IA__g_main_context_iteration (context=0x7ffff0d9ba90,
may_block=1) at /tmp/glib2.0.0xzuTt/glib2.0-2.24.1/glib/gmain.c:2654
#43 0x00007ffff27f8629 in base::MessagePumpGtk::RunOnce (this=0x7ffff0dc1460,
context=0x7ffff0d9ba90, block=true) at base/message_pump_gtk.cc:41
#44 0x00007ffff27f6a74 in base::MessagePumpGlib::RunWithDispatcher
(this=0x7ffff0dc1460, delegate=0x7ffff0ddde00, dispatcher=0x0) at
base/message_pump_glib.cc:206
#45 0x00007ffff27f6e9e in base::MessagePumpGlib::Run (this=0x7ffff0dc1460,
delegate=0x7ffff0ddde00) at base/message_pump_glib.cc:298

Repro steps:
1. Open a tab.
2. Press ctrl+P to preview the tab.
3. Right before the preview tab loads press Ctrl+shift+p

So, I am leaving this the way it is.

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:4168:
command_updater_.UpdateCommandEnabled(IDC_ADVANCED_PRINT, true);
On 2011/08/24 01:27:28, Lei Zhang wrote:
> You don't always want to do this. For example, if a PDF has the "do not print"
> bit set, this will let you print such a PDF.

Fixed.

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:665: void
PrintPreviewHandler::HandleShowSystemDialog(const ListValue*) {
On 2011/08/24 01:27:28, Lei Zhang wrote:
> This is just going to conflict with my cleanup CL.

okay :)

http://codereview.chromium.org/7721001/diff/2001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:681:
print_preview_ui->OnPrintPreviewRequest(-1);
On 2011/08/24 01:27:28, Lei Zhang wrote:
> Let's add a new method to PrintPreviewUI to do cancellations.

Done.

[Lei Zhang, 2011-08-25 09:05:49]

It only crashes if you do it wrong. Try this:

  printing::PrintPreviewTabController* tab_controller =
      printing::PrintPreviewTabController::GetInstance();
  if (!tab_controller)
    return false;
  TabContents* print_preview_tab =
      tab_controller->GetPrintPreviewForTab(tab_contents());
  if (!print_preview_tab)
    return PrintNow();

  TabContents* initiator_tab =
      tab_controller->GetInitiatorTab(print_preview_tab);
  if (!initiator_tab)
    return false;

  static_cast<RenderViewHostDelegate*>(initiator_tab)->Activate();
  TabContentsWrapper* print_preview_wrapper =
      TabContentsWrapper::GetCurrentWrapperForContents(tab_contents());
  delete print_preview_wrapper;
  TabContentsWrapper* initiator_wrapper =
      TabContentsWrapper::GetCurrentWrapperForContents(initiator_tab);
  return initiator_wrapper->print_view_manager()->PrintForSystemDialogNow();

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4168: bool is_preview_tab = true;
This variable is poorly named. It's true in some cases even if the tab isn't a
print preview tab.

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/views/acc...
File chrome/browser/ui/views/accelerator_table_gtk.cc (right):

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/views/acc...
chrome/browser/ui/views/accelerator_table_gtk.cc:98: { ui::VKEY_P,             
true,  true,  false, IDC_ADVANCED_PRINT},
Do we need this for CrOS?

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:678:
print_preview_ui->OnCancelPendingPreviewRequest();
You can pull this, the associated PrintPreviewUI changes, and the JS change into
its own CL and get it committed sooner.

[kmadhusu, 2011-08-25 17:51:48]

Today morning, I tried something similar to sample code. At times, I noticed
that a user can open a preview tab and view the system dialog at the same time
for the given initiator tab. 

With my old code, we are scheduling a close event on a preview tab after showing
the native system dialog. Thereby, we maintain the association between the
preview tab and initiator tab till the system dialog is shown and then we erase
the map entry in the print preview tab controller. 

With your suggestion, 
(1) we are closing the preview tab way before we show the system dialog. For
long webpages, the system shows dialog shows up after 4-5 secs. In the mean time
any user can select Ctrl+P and hence they can see both preview tab and system
dialog at the same time.
(2) Deleting a preview tab wrapper and then calling system dialog does not
promise us that we will show system dialog after closing the preview tab.

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4168: bool is_preview_tab = true;
On 2011/08/25 09:05:49, Lei Zhang wrote:
> This variable is poorly named. It's true in some cases even if the tab isn't a
> print preview tab.

Fixed...

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/views/acc...
File chrome/browser/ui/views/accelerator_table_gtk.cc (right):

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/views/acc...
chrome/browser/ui/views/accelerator_table_gtk.cc:98: { ui::VKEY_P,             
true,  true,  false, IDC_ADVANCED_PRINT},
On 2011/08/25 09:05:49, Lei Zhang wrote:
> Do we need this for CrOS?

I don't think so. This table is used by linux and CrOS.

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:678:
print_preview_ui->OnCancelPendingPreviewRequest();
On 2011/08/25 09:05:49, Lei Zhang wrote:
> You can pull this, the associated PrintPreviewUI changes, and the JS change
into
> its own CL and get it committed sooner.

Done.

[Lei Zhang, 2011-08-25 20:43:23]

It sounds like for my idea, I'd have to fix all the subtle issues that you
observed. Having to handle a tricky issue correctly in two different code paths
is even worse than bouncing back and forth between the browser/renderer. So,
nevermind.

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/views/acc...
File chrome/browser/ui/views/accelerator_table_gtk.cc (right):

http://codereview.chromium.org/7721001/diff/11001/chrome/browser/ui/views/acc...
chrome/browser/ui/views/accelerator_table_gtk.cc:98: { ui::VKEY_P,             
true,  true,  false, IDC_ADVANCED_PRINT},
On 2011/08/25 17:51:48, kmadhusu wrote:
> On 2011/08/25 09:05:49, Lei Zhang wrote:
> > Do we need this for CrOS?
> 
> I don't think so. This table is used by linux and CrOS.

I don't believe this is used on Linux. The Linux code is in
chrome/browser/ui/gtk/accelerators_gtk.cc.

http://codereview.chromium.org/7721001/diff/15002/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7721001/diff/15002/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6015: <if expr="not is_win and not
is_macosx">
Are you trying to cover all platforms here? You can probably want "is_posix and
not is_macosx". This might be a moot point. See comment in
print_preview_data_source.cc.

http://codereview.chromium.org/7721001/diff/15002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7721001/diff/15002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4171: selected_tab_is_preview_tab =
}selected_tab_is_preview_tab| will never be true. You can't ever get to this
case, because if the content restriction exists, then you wouldn't be able to
create the print preview tab in the first place.

http://codereview.chromium.org/7721001/diff/15002/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7721001/diff/15002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:93:
l10n_util::GetStringUTF16(IDS_PRINT_PREVIEW_ADVANCED_SHORTCUT_KEY);
Since this doesn't need to be translated, just define it as
kAdvancedPrintShortcut instead of putting it in the .grd file.

[Lei Zhang, 2011-08-25 20:57:09]

http://codereview.chromium.org/7721001/diff/15002/chrome/browser/printing/pri...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/7721001/diff/15002/chrome/browser/printing/pri...
chrome/browser/printing/print_view_manager.cc:106: PrintNow();
you want to do: return PrintNow() here because PrintNow() can still decide to
fail.

[Lei Zhang, 2011-08-25 21:15:49]

http://codereview.chromium.org/7721001/diff/15002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7721001/diff/15002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:4171: selected_tab_is_preview_tab =
On 2011/08/25 20:43:23, Lei Zhang wrote:
> }selected_tab_is_preview_tab| will never be true. You can't ever get to this
> case, because if the content restriction exists, then you wouldn't be able to
> create the print preview tab in the first place.

Whoops, n/m. I take this back. This is fine as is.

[kmadhusu, 2011-08-25 22:47:39]

http://codereview.chromium.org/7721001/diff/15002/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7721001/diff/15002/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6015: <if expr="not is_win and not
is_macosx">
On 2011/08/25 20:43:23, Lei Zhang wrote:
> Are you trying to cover all platforms here? You can probably want "is_posix
and
> not is_macosx". This might be a moot point. See comment in
> print_preview_data_source.cc.

Removed IDS_PRINT_PREVIEW_ADVANCED_SHORTCUT_KEY

http://codereview.chromium.org/7721001/diff/15002/chrome/browser/printing/pri...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/7721001/diff/15002/chrome/browser/printing/pri...
chrome/browser/printing/print_view_manager.cc:106: PrintNow();
On 2011/08/25 20:57:10, Lei Zhang wrote:
> you want to do: return PrintNow() here because PrintNow() can still decide to
> fail.

Done.

http://codereview.chromium.org/7721001/diff/15002/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7721001/diff/15002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:93:
l10n_util::GetStringUTF16(IDS_PRINT_PREVIEW_ADVANCED_SHORTCUT_KEY);
On 2011/08/25 20:43:23, Lei Zhang wrote:
> Since this doesn't need to be translated, just define it as
> kAdvancedPrintShortcut instead of putting it in the .grd file.

good catch.. Fixed.

[Lei Zhang, 2011-08-25 23:01:47]

LGTM

http://codereview.chromium.org/7721001/diff/12019/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7721001/diff/12019/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:93: #if defined(OS_MACOSX)
Probably don't need this. UTF8ToUTF16 should work for all cases.

[kmadhusu, 2011-08-26 02:15:07]

http://codereview.chromium.org/7721001/diff/12019/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7721001/diff/12019/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:93: #if defined(OS_MACOSX)
On 2011/08/25 23:01:47, Lei Zhang wrote:
> Probably don't need this. UTF8ToUTF16 should work for all cases.

Done.

[Nico, 2011-08-29 23:43:48]

http://codereview.chromium.org/7721001/diff/19001/chrome/browser/global_keybo...
File chrome/browser/global_keyboard_shortcuts_mac.mm (right):

http://codereview.chromium.org/7721001/diff/19001/chrome/browser/global_keybo...
chrome/browser/global_keyboard_shortcuts_mac.mm:84: {true,  true,  false, false,
0,                'p', IDC_ADVANCED_PRINT},
The way this would normally be done on mac is to edit MainMenu.xib and add a
second menu item below "Print…" called (say) "System Print" and assign it a
shortcut in IB (cmd-shift-p is usually "Page Setup…" on mac, so maybe
cmd-opt-p). You would then set the "Alternate" checkbox. This way, when the user
clicks the file menu and hits opt, he'll see the "Print…" menu item change to
"System Print…", and also when he hits cmd-opt-p, the file menu will flash. You
might want to talk to the UI people about the string.

(You'd put the decimal value belonging to IDC_ADVANCED_PRINT into the new menu
item's tag, and it should be dispatched correctly automatically, without real
code changes.)

[kmadhusu, 2011-08-30 00:27:03]

http://codereview.chromium.org/7721001/diff/19001/chrome/browser/global_keybo...
File chrome/browser/global_keyboard_shortcuts_mac.mm (right):

http://codereview.chromium.org/7721001/diff/19001/chrome/browser/global_keybo...
chrome/browser/global_keyboard_shortcuts_mac.mm:84: {true,  true,  false, false,
0,                'p', IDC_ADVANCED_PRINT},
On 2011/08/29 23:43:48, Nico wrote:
> The way this would normally be done on mac is to edit MainMenu.xib and add a
> second menu item below "Print…" called (say) "System Print" and assign it a
> shortcut in IB (cmd-shift-p is usually "Page Setup…" on mac, so maybe
> cmd-opt-p). You would then set the "Alternate" checkbox. This way, when the
user
> clicks the file menu and hits opt, he'll see the "Print…" menu item change to
> "System Print…", and also when he hits cmd-opt-p, the file menu will flash.
You
> might want to talk to the UI people about the string.
> 
> (You'd put the decimal value belonging to IDC_ADVANCED_PRINT into the new menu
> item's tag, and it should be dispatched correctly automatically, without real
> code changes.)

Thakis: Thanks a lot for the info. I am not a mac user. I wasn't aware of this.
I will make the code changes accordingly.