Implement browser side of PPB_Printing resource.

Implement browser side of PPB_Printing resource.
    
This adds the browser side implementation of PPB_Printing as well as a unittest for PepperPrintingHost and a PPAPI browser_test for PPB_Printing::GetDefaultPrintSettings.
    
BUG=138333
TEST=content_unittests/browser_tests


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

[brettw, 2012-09-18 21:41:38]

This looks nice. I have a bunch of style nits and only one real comment
(callback handling in C++ wrapper).

https://chromiumcodereview.appspot.com/10826072/diff/15002/.gitmodules
File .gitmodules (right):

https://chromiumcodereview.appspot.com/10826072/diff/15002/.gitmodules#newcode1
.gitmodules:1: [submodule "breakpad/src"]
Did you mean to check this in?

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:43:
scoped_ptr<PepperPrintSettingsManager> manager(
Since you have a local var, I think you need a {} for this condition. Probably
the compiler will complain if we add another case below this.

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
File content/browser/renderer_host/pepper/pepper_print_settings_manager.cc
(right):

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
content/browser/renderer_host/pepper/pepper_print_settings_manager.cc:20: 
Remove blank.

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
content/browser/renderer_host/pepper/pepper_print_settings_manager.cc:51:
printing::PrintingContext::Create(""));
"" -> std::string()

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
content/browser/renderer_host/pepper/pepper_print_settings_manager.cc:52: if
(!context.get())
Need {} for multiline bodies as per style guide.

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
content/browser/renderer_host/pepper/pepper_print_settings_manager.cc:89: } //
namespace
two spaces before //

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
File content/browser/renderer_host/pepper/pepper_print_settings_manager.h
(right):

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
content/browser/renderer_host/pepper/pepper_print_settings_manager.h:27: virtual
~PepperPrintSettingsManager() {};
Remove ;

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
content/browser/renderer_host/pepper/pepper_print_settings_manager.h:45: // Mock
implementation for test purposes.
Is it possible to just define this in the unit test .cc file instead? Otherwise
it should probably be in a separate file for test support rather than in the
main binary.

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
File content/browser/renderer_host/pepper/pepper_printing_host.cc (right):

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
content/browser/renderer_host/pepper/pepper_printing_host.cc:18: void
PrintSettingsCallback(
You should be able to make this a member function. Bind knows about weak ptr and
won't run the callback in this case.

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
File content/browser/renderer_host/pepper/pepper_printing_host.h (right):

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
content/browser/renderer_host/pepper/pepper_printing_host.h:30: 
Need only one blank.

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
File content/browser/renderer_host/pepper/pepper_printing_host_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10826072/diff/15002/content/browser/re...
content/browser/renderer_host/pepper/pepper_printing_host_unittest.cc:34: return
lhs.width == rhs.width && lhs.height == rhs.height;
Check indenting for these functions.

https://chromiumcodereview.appspot.com/10826072/diff/15002/ppapi/cpp/dev/prin...
File ppapi/cpp/dev/printing_dev.cc (right):

https://chromiumcodereview.appspot.com/10826072/diff/15002/ppapi/cpp/dev/prin...
ppapi/cpp/dev/printing_dev.cc:112: if (result != PP_OK_COMPLETIONPENDING)
You shouldn't need to do this check in the C++ wrappers. It should be handled by
the thunk layer by the enter functions for you.

https://chromiumcodereview.appspot.com/10826072/diff/15002/ppapi/cpp/dev/prin...
ppapi/cpp/dev/printing_dev.cc:115: else if
(has_interface<PPB_Printing_Dev_0_6>()) {
Goes on previous line.

https://chromiumcodereview.appspot.com/10826072/diff/15002/ppapi/tests/test_p...
File ppapi/tests/test_printing.h (right):

https://chromiumcodereview.appspot.com/10826072/diff/15002/ppapi/tests/test_p...
ppapi/tests/test_printing.h:35: #endif // PPAPI_TESTS_TEST_PRINTING_H_
Two spaces before //

[raymes, 2012-09-18 23:43:57]

Thanks for the quick review

http://codereview.chromium.org/10826072/diff/15002/.gitmodules
File .gitmodules (right):

http://codereview.chromium.org/10826072/diff/15002/.gitmodules#newcode1
.gitmodules:1: [submodule "breakpad/src"]
On 2012/09/18 21:41:38, brettw wrote:
> Did you mean to check this in?

Nope!

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:43:
scoped_ptr<PepperPrintSettingsManager> manager(
On 2012/09/18 21:41:38, brettw wrote:
> Since you have a local var, I think you need a {} for this condition. Probably
> the compiler will complain if we add another case below this.

Done.

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_print_settings_manager.cc
(right):

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_print_settings_manager.cc:20: 
On 2012/09/18 21:41:38, brettw wrote:
> Remove blank.

Done.

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_print_settings_manager.cc:51:
printing::PrintingContext::Create(""));
On 2012/09/18 21:41:38, brettw wrote:
> "" -> std::string()

Done.

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_print_settings_manager.cc:52: if
(!context.get())
On 2012/09/18 21:41:38, brettw wrote:
> Need {} for multiline bodies as per style guide.

Done.

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_print_settings_manager.cc:89: } //
namespace
On 2012/09/18 21:41:38, brettw wrote:
> two spaces before // 

Done.

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_print_settings_manager.h
(right):

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_print_settings_manager.h:27: virtual
~PepperPrintSettingsManager() {};
On 2012/09/18 21:41:38, brettw wrote:
> Remove ;

Done.

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_print_settings_manager.h:45: // Mock
implementation for test purposes.
On 2012/09/18 21:41:38, brettw wrote:
> Is it possible to just define this in the unit test .cc file instead?
Otherwise
> it should probably be in a separate file for test support rather than in the
> main binary.

Done.

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_printing_host.cc (right):

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_printing_host.cc:18: void
PrintSettingsCallback(
On 2012/09/18 21:41:38, brettw wrote:
> You should be able to make this a member function. Bind knows about weak ptr
and
> won't run the callback in this case.
Cool I wasn't sure about that.

Done.

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_printing_host.h (right):

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_printing_host.h:30: 
On 2012/09/18 21:41:38, brettw wrote:
> Need only one blank.

Done.

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_printing_host_unittest.cc
(right):

http://codereview.chromium.org/10826072/diff/15002/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_printing_host_unittest.cc:34: return
lhs.width == rhs.width && lhs.height == rhs.height;
On 2012/09/18 21:41:38, brettw wrote:
> Check indenting for these functions.

Done.

http://codereview.chromium.org/10826072/diff/15002/ppapi/cpp/dev/printing_dev.cc
File ppapi/cpp/dev/printing_dev.cc (right):

http://codereview.chromium.org/10826072/diff/15002/ppapi/cpp/dev/printing_dev...
ppapi/cpp/dev/printing_dev.cc:112: if (result != PP_OK_COMPLETIONPENDING)
On 2012/09/18 21:41:38, brettw wrote:
> You shouldn't need to do this check in the C++ wrappers. It should be handled
by
> the thunk layer by the enter functions for you.

Done.

http://codereview.chromium.org/10826072/diff/15002/ppapi/cpp/dev/printing_dev...
ppapi/cpp/dev/printing_dev.cc:115: else if
(has_interface<PPB_Printing_Dev_0_6>()) {
On 2012/09/18 21:41:38, brettw wrote:
> Goes on previous line.

Done.

http://codereview.chromium.org/10826072/diff/15002/ppapi/tests/test_printing.h
File ppapi/tests/test_printing.h (right):

http://codereview.chromium.org/10826072/diff/15002/ppapi/tests/test_printing....
ppapi/tests/test_printing.h:35: #endif // PPAPI_TESTS_TEST_PRINTING_H_
On 2012/09/18 21:41:38, brettw wrote:
> Two spaces before //

Done.

[brettw, 2012-09-18 23:58:40]

lgtm

[commit-bot: I haz the power, 2012-09-27 19:48:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/10826072/35021

[commit-bot: I haz the power, 2012-09-27 20:21:39]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-27 21:00:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/10826072/41003

[commit-bot: I haz the power, 2012-09-27 23:30:42]

Change committed as 159150