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

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 1 #

Total comments: 2

Patch Set 2 : Style fix. #

Total comments: 4

Patch Set 3 : Comment cleanup only. #

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)
James Hawkins
10 years, 2 months ago (2010-10-06 20:27:32 UTC) #1
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.

Powered by Google App Engine
This is Rietveld 408576698