Issue 3610013: Printing: Convert PrintingContext into an interface implemented by the separate

Issue 3610013: Printing: Convert PrintingContext into an interface implemented by the separate (Closed)

Created:
10 years, 2 months ago by James Hawkins

Modified:
9 years, 7 months ago

Reviewers:
stuartmorgan, M-A Ruel

CC:
chromium-reviews, ben+cc_chromium.org, pam+watch_chromium.org, Paweł Hajdan Jr.

Visibility:
Public.

Description

Printing: Convert PrintingContext into an interface implemented by the separate platforms. BUG=none TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=61714

Patch Set 3 : Comment cleanup only. #

Created: 10 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+422 lines, -264 lines)			Patch
M	chrome/browser/printing/print_job_unittest.cc	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/printing/print_job_worker.h	View		3 chunks	+4 lines, -2 lines	0 comments	Download
M	chrome/browser/printing/print_job_worker.cc	View		11 chunks	+16 lines, -14 lines	0 comments	Download
M	printing/emf_win_unittest.cc	View	1	4 chunks	+9 lines, -7 lines	0 comments	Download
M	printing/printing.gyp	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	printing/printing_context.h	View	1 2	4 chunks	+36 lines, -118 lines	0 comments	Download
A	printing/printing_context.cc	View		1 chunk	+31 lines, -0 lines	0 comments	Download
A	printing/printing_context_cairo.h	View		1 chunk	+39 lines, -0 lines	0 comments	Download
M	printing/printing_context_cairo.cc	View		9 chunks	+24 lines, -26 lines	0 comments	Download
A	printing/printing_context_mac.h	View		1 chunk	+55 lines, -0 lines	0 comments	Download
M	printing/printing_context_mac.mm	View		10 chunks	+35 lines, -35 lines	0 comments	Download
A	printing/printing_context_win.h	View		1 chunk	+97 lines, -0 lines	0 comments	Download
M	printing/printing_context_win.cc	View		16 chunks	+51 lines, -52 lines	0 comments	Download
M	printing/printing_context_win_unittest.cc	View	1	4 chunks	+15 lines, -8 lines	0 comments	Download
M	printing/printing_test.h	View		1 chunk	+4 lines, -0 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

M-A Ruel

I'm not a fan of virtual functions without polymorphism but the code lgtm. http://codereview.chromium.org/3610013/diff/1/5 File ...

10 years, 2 months ago (2010-10-06 20:36:04 UTC) #2

James Hawkins

http://codereview.chromium.org/3610013/diff/1/5 File printing/emf_win_unittest.cc (right): http://codereview.chromium.org/3610013/diff/1/5#newcode79 printing/emf_win_unittest.cc:79: scoped_ptr<printing::PrintingContext> context; On 2010/10/06 20:36:04, Marc-Antoine Ruel wrote: > ...

10 years, 2 months ago (2010-10-06 20:39:54 UTC) #3

stuartmorgan

LGTM with comment nits. The pile of Windows-specific functions seems like a good enough reason ...

10 years, 2 months ago (2010-10-06 20:51:33 UTC) #4

James Hawkins

10 years, 2 months ago (2010-10-06 20:56:25 UTC) #5

I'm going to commit off the try bot results in patch set 2 since patch set 3
only changed a comment.

http://codereview.chromium.org/3610013/diff/5001/6007
File printing/printing_context.h (right):

http://codereview.chromium.org/3610013/diff/5001/6007#newcode19
printing/printing_context.h:19: // An interface implemented by objects that
describe the user selected printing
On 2010/10/06 20:51:33, stuartmorgan wrote:
> I'm not sure we should call it an interface since it has actually code. Maybe
> just call it an abstraction of the printer context?

Done.

http://codereview.chromium.org/3610013/diff/5001/6007#newcode21
printing/printing_context.h:21: // settings. Implementers of this interface
directly talk to the printer and
On 2010/10/06 20:51:33, stuartmorgan wrote:
> s/Implementers of this interface/Concrete implementations/ ?

Done.

Expand Messages | Collapse Messages