Print Preview: Go from event driven print preview back to print preview with sync messages. The s...

Print Preview: Go from event driven print preview back to print preview with sync messages. The sync messages now go to the IO thread to avoid the potential deadlock on Windows.

BUG=91057
, and probably a few more.
TEST=included.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97820

[Lei Zhang, 2011-08-19 18:36:36]

Removed the requested_page code per discussion.

[kmadhusu, 2011-08-19 19:51:02]

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

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:485:
print_preview_ui->OnPrintPreviewRequest(request_id);
how about OnPrintPreviewRequest => UpdatePreviewRequestID ???

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:486:
settings->SetString(printing::kPreviewUIAddr,
Add a comment on why you are adding this UI address to the dictionary.

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:881:
wrapper->print_view_manager()->set_observer(NULL);
Do you still need "OnTabDestroyed" function? Initiator tab print_view_manager
observer is not set in preview workflow. I may be wrong.

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_ui.cc (right):

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_ui.cc:24: // Mapping from PrintPreviewUI
address to print preview request id;
Document that we always store the latest print preview request id.

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_ui.cc:33: static
base::LazyInstance<PrintPreviewRequestIdMapWithLock>
static is not required here.

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_ui.cc:52: // TODO(kmadhusu): Add code to
update PrintPreviewStatus so the current
Do you still need this TODO?

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_ui.cc:98: if (it ==
g_print_preview_request_id_map.Get().map.end())
How about replacing line #98-101 with

*cancel = true;
if (it != g_print_preview_request_id_map.Get().map.end())
   *cancel = (request_id != it->second);

and change the function to return void?

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:495: if (CreatePreviewDocument()) {
nit: Remove {}

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:641: if
(notify_browser_of_print_failure_) {
nit: Remove {}

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:894: if
(!job_settings.GetString(printing::kPreviewUIAddr,
Can you merge these 3 if conditions (line #894, 900, 906)?

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1054: bool
PrintWebViewHelper::PreviewPageRendered(int page_number,
PreviewPageRendered() not only sends the rendered page but also checks for
cancelling. I think its better to create a function just to check for cancelling
so that this function always send the rendered page.

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1093: return true;
Previously, We were sending an empty draft metafile if the preview is not
modifiable to continue rendering. Now we can avoid that message completely.

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1153: for (int i = 0; i <
actual_page_count_; ++i) {
nit: Remove {}

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:302: // For a valid |page_number|,
|metafile| is the rendered page.
For a valid |page_number|,
If the initiator is modifiable, |metafile| is the rendered page. 

If the initiator tab is non-modifiable, |metafile| is NULL.

[Lei Zhang, 2011-08-19 22:50:28]

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

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:485:
print_preview_ui->OnPrintPreviewRequest(request_id);
On 2011/08/19 19:51:03, kmadhusu wrote:
> how about OnPrintPreviewRequest => UpdatePreviewRequestID ???

Most of the print preview UI methods are in the form of OnFoo.

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:486:
settings->SetString(printing::kPreviewUIAddr,
On 2011/08/19 19:51:03, kmadhusu wrote:
> Add a comment on why you are adding this UI address to the dictionary. 

Done.

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:881:
wrapper->print_view_manager()->set_observer(NULL);
On 2011/08/19 19:51:03, kmadhusu wrote:
> Do you still need "OnTabDestroyed" function? Initiator tab print_view_manager
> observer is not set in preview workflow. I may be wrong. 

I believe it's still needed for http://crbug.com/83256.

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_ui.cc (right):

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_ui.cc:24: // Mapping from PrintPreviewUI
address to print preview request id;
On 2011/08/19 19:51:03, kmadhusu wrote:
> Document that we always store the latest print preview request id. 

Done.

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_ui.cc:33: static
base::LazyInstance<PrintPreviewRequestIdMapWithLock>
On 2011/08/19 19:51:03, kmadhusu wrote:
> static is not required here.

Done.

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_ui.cc:52: // TODO(kmadhusu): Add code to
update PrintPreviewStatus so the current
On 2011/08/19 19:51:03, kmadhusu wrote:
> Do you still need this TODO?

Nope.

http://codereview.chromium.org/7621087/diff/8001/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_ui.cc:98: if (it ==
g_print_preview_request_id_map.Get().map.end())
On 2011/08/19 19:51:03, kmadhusu wrote:
> How about replacing line #98-101 with
> 
> *cancel = true;
> if (it != g_print_preview_request_id_map.Get().map.end())
>    *cancel = (request_id != it->second);
> 
> and change the function to return void?
> 

Done.

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:495: if (CreatePreviewDocument()) {
On 2011/08/19 19:51:03, kmadhusu wrote:
> nit: Remove {}

Done.

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:641: if
(notify_browser_of_print_failure_) {
On 2011/08/19 19:51:03, kmadhusu wrote:
> nit: Remove {}

Done.

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:894: if
(!job_settings.GetString(printing::kPreviewUIAddr,
On 2011/08/19 19:51:03, kmadhusu wrote:
> Can you merge these 3 if conditions (line #894, 900, 906)?

Done.

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1054: bool
PrintWebViewHelper::PreviewPageRendered(int page_number,
On 2011/08/19 19:51:03, kmadhusu wrote:
> PreviewPageRendered() not only sends the rendered page but also checks for
> cancelling. I think its better to create a function just to check for
cancelling
> so that this function always send the rendered page. 
> 

Done.

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1093: return true;
On 2011/08/19 19:51:03, kmadhusu wrote:
> Previously, We were sending an empty draft metafile if the preview is not
> modifiable to continue rendering. Now we can avoid that message completely. 

Done.

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:1153: for (int i = 0; i <
actual_page_count_; ++i) {
On 2011/08/19 19:51:03, kmadhusu wrote:
> nit: Remove {}

Done.

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7621087/diff/8001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:302: // For a valid |page_number|,
|metafile| is the rendered page.
On 2011/08/19 19:51:03, kmadhusu wrote:
> For a valid |page_number|,
> If the initiator is modifiable, |metafile| is the rendered page. 
> 
> If the initiator tab is non-modifiable, |metafile| is NULL.

Done.

[kmadhusu, 2011-08-20 00:07:11]

http://codereview.chromium.org/7621087/diff/14003/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/7621087/diff/14003/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:325: preview_request_id,
nit: Fix indentation

http://codereview.chromium.org/7621087/diff/14003/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.cc (right):

http://codereview.chromium.org/7621087/diff/14003/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:24: // Mapping from PrintPreviewUI
address to most recent print preview request id;
nit: ";" -> "."

http://codereview.chromium.org/7621087/diff/14003/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7621087/diff/14003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:145: // Returns true if print preview
should continue, false on
Fix the comment.

http://codereview.chromium.org/7621087/diff/14003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:147: bool RenderPreviewPage(int
page_number);
This function can now return void.

http://codereview.chromium.org/7621087/diff/14003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:300: // Returns true if continuing,
false if cancelling.
nit: This function should return true if cancelling and false to continue.

http://codereview.chromium.org/7621087/diff/14003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:308: bool PreviewPageRendered(int
page_number, printing::Metafile* metafile);
This function can return void.

http://codereview.chromium.org/7621087/diff/14003/printing/print_job_constant...
File printing/print_job_constants.cc (right):

http://codereview.chromium.org/7621087/diff/14003/printing/print_job_constant...
printing/print_job_constants.cc:89: const int INVALID_PAGE_INDEX = -2;
If you don't use INVALID_PAGE_INDEX anywhere, please remove it from here.

[Lei Zhang, 2011-08-20 03:25:52]

http://codereview.chromium.org/7621087/diff/14003/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/7621087/diff/14003/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:325: preview_request_id,
On 2011/08/20 00:07:11, kmadhusu wrote:
> nit: Fix indentation

Done.

http://codereview.chromium.org/7621087/diff/14003/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.cc (right):

http://codereview.chromium.org/7621087/diff/14003/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:24: // Mapping from PrintPreviewUI
address to most recent print preview request id;
On 2011/08/20 00:07:11, kmadhusu wrote:
> nit: ";" -> "."

Done.

http://codereview.chromium.org/7621087/diff/14003/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7621087/diff/14003/chrome/common/print_message...
chrome/common/print_messages.h:55: IPC_STRUCT_MEMBER(std::string,
preview_ui_addr)
This is going to cause a crash when we do a memset() on the struct.

http://codereview.chromium.org/7621087/diff/14003/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7621087/diff/14003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:145: // Returns true if print preview
should continue, false on
On 2011/08/20 00:07:11, kmadhusu wrote:
> Fix the comment.

Done.

http://codereview.chromium.org/7621087/diff/14003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:147: bool RenderPreviewPage(int
page_number);
On 2011/08/20 00:07:11, kmadhusu wrote:
> This function can now return void.

No, it still need to handle the possible failures in the mac implementation.

http://codereview.chromium.org/7621087/diff/14003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:300: // Returns true if continuing,
false if cancelling.
On 2011/08/20 00:07:11, kmadhusu wrote:
> nit: This function should return true if cancelling and false to continue. 

Done.

http://codereview.chromium.org/7621087/diff/14003/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:308: bool PreviewPageRendered(int
page_number, printing::Metafile* metafile);
On 2011/08/20 00:07:11, kmadhusu wrote:
> This function can return void. 

No, if we encounter a failure, we have to let our caller know.

http://codereview.chromium.org/7621087/diff/14003/printing/print_job_constant...
File printing/print_job_constants.cc (right):

http://codereview.chromium.org/7621087/diff/14003/printing/print_job_constant...
printing/print_job_constants.cc:89: const int INVALID_PAGE_INDEX = -2;
On 2011/08/20 00:07:11, kmadhusu wrote:
> If you don't use INVALID_PAGE_INDEX anywhere, please remove it from here. 

Done.

[kmadhusu, 2011-08-22 17:56:22]

As we discussed, http://codereview.chromium.org/7693017/ CL will fix the memset
related issues. Please revert the code to store print preview ui addr str.

http://codereview.chromium.org/7621087/diff/9021/printing/print_job_constants.cc
File printing/print_job_constants.cc (right):

http://codereview.chromium.org/7621087/diff/9021/printing/print_job_constants...
printing/print_job_constants.cc:85: // Indices used to represent first page,
invalid page and complete
nit: Update the comment.

[Lei Zhang, 2011-08-22 20:22:14]

print preview uid change reverted.

http://codereview.chromium.org/7621087/diff/9021/printing/print_job_constants.cc
File printing/print_job_constants.cc (right):

http://codereview.chromium.org/7621087/diff/9021/printing/print_job_constants...
printing/print_job_constants.cc:85: // Indices used to represent first page,
invalid page and complete
On 2011/08/22 17:56:22, kmadhusu wrote:
> nit: Update the comment. 

Done.

[Lei Zhang, 2011-08-22 20:46:24]

Added a unit test for the new print preview ui code in patch set 6. The unit
test doesn't have an IO thread, so I updated
PrintPreviewUI::GetCurrentPrintPreviewStatus() to remove the thread check and
updated the comment accordingly.

[kmadhusu, 2011-08-22 21:23:57]

http://codereview.chromium.org/7621087/diff/17001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7621087/diff/17001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:130:
StopWorker(params.document_cookie);
Is it better to stop the worker before validating any params values?

http://codereview.chromium.org/7621087/diff/17001/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7621087/diff/17001/chrome/common/print_message...
chrome/common/print_messages.h:345: // and if not, what page to render next.
nit: We are not retrieving any page number from the browser. Update the comment.

http://codereview.chromium.org/7621087/diff/17001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7621087/diff/17001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1145: if (cancel) {
How about??

if (cancel)
  notify_browser_of_print_failure_ = false;
return cancel;

http://codereview.chromium.org/7621087/diff/17001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1157: }
page_number will never be a negative number. Rather than a if condition, can you
add a DCHECK()?

http://codereview.chromium.org/7621087/diff/17001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1174: if (metafile) {
You are already validating the metafile in line #1166. So you can remove the if
condition here. 

(or)

Remove lines 1166 - 1168 and add a else for this if condition.

http://codereview.chromium.org/7621087/diff/16003/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_unittest.cc (right):

http://codereview.chromium.org/7621087/diff/16003/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_unittest.cc:148: // Set and get the
individual draft pages.
nit: Update the comment.

[Lei Zhang, 2011-08-22 22:26:55]

vandebo: patch set 7 should be good to go, PTAL.

http://codereview.chromium.org/7621087/diff/17001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/7621087/diff/17001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:130:
StopWorker(params.document_cookie);
On 2011/08/22 21:23:57, kmadhusu wrote:
> Is it better to stop the worker before validating any params values?

Sure. Why not.

http://codereview.chromium.org/7621087/diff/17001/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7621087/diff/17001/chrome/common/print_message...
chrome/common/print_messages.h:345: // and if not, what page to render next.
On 2011/08/22 21:23:57, kmadhusu wrote:
> nit: We are not retrieving any page number from the browser. Update the
comment.

Done.

http://codereview.chromium.org/7621087/diff/17001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7621087/diff/17001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1145: if (cancel) {
On 2011/08/22 21:23:57, kmadhusu wrote:
> How about??
> 
> if (cancel)
>   notify_browser_of_print_failure_ = false;
> return cancel;

Done.

http://codereview.chromium.org/7621087/diff/17001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1157: }
On 2011/08/22 21:23:57, kmadhusu wrote:
> page_number will never be a negative number. Rather than a if condition, can
you
> add a DCHECK()?

Done.

http://codereview.chromium.org/7621087/diff/17001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1174: if (metafile) {
On 2011/08/22 21:23:57, kmadhusu wrote:
> You are already validating the metafile in line #1166. So you can remove the
if
> condition here. 
> 
> (or)
> 
> Remove lines 1166 - 1168 and add a else for this if condition.
> 

Removed the check here.

http://codereview.chromium.org/7621087/diff/16003/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_unittest.cc (right):

http://codereview.chromium.org/7621087/diff/16003/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_unittest.cc:148: // Set and get the
individual draft pages.
On 2011/08/22 21:23:57, kmadhusu wrote:
> nit: Update the comment.

Done.

[kmadhusu, 2011-08-22 22:51:20]

LGTM with a nit.

http://codereview.chromium.org/7621087/diff/18044/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7621087/diff/18044/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:385: // Tests that print
preview work and sending and receiving messages through
nit: Update comment.

[Lei Zhang, 2011-08-22 22:56:22]

http://codereview.chromium.org/7621087/diff/18044/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7621087/diff/18044/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_browsertest.cc:385: // Tests that print
preview work and sending and receiving messages through
On 2011/08/22 22:51:20, kmadhusu wrote:
> nit: Update comment.

Done.

[vandebo (ex-Chrome), 2011-08-23 04:30:03]

http://codereview.chromium.org/7621087/diff/16029/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.cc (right):

http://codereview.chromium.org/7621087/diff/16029/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:27: struct
PrintPreviewRequestIdMapWithLock {
nit: Is there a reason this can't be a class with methods?  I think a set, get,
and erase method would cover you. Then all the lock access would be in the
single class.

[vandebo (ex-Chrome), 2011-08-23 04:43:46]

LGTM

http://codereview.chromium.org/7621087/diff/16029/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/7621087/diff/16029/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:19: #include <map>
nit: was map missing before? I don't see a new use.

[Lei Zhang, 2011-08-23 04:44:23]

http://codereview.chromium.org/7621087/diff/16029/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/7621087/diff/16029/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:19: #include <map>
On 2011/08/23 04:43:46, vandebo wrote:
> nit: was map missing before? I don't see a new use.

Yes, for line 32.

[Lei Zhang, 2011-08-23 04:58:17]

http://codereview.chromium.org/7621087/diff/16029/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.cc (right):

http://codereview.chromium.org/7621087/diff/16029/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:27: struct
PrintPreviewRequestIdMapWithLock {
On 2011/08/23 04:30:03, vandebo wrote:
> nit: Is there a reason this can't be a class with methods?  I think a set,
get,
> and erase method would cover you. Then all the lock access would be in the
> single class.

Done.

[vandebo (ex-Chrome), 2011-08-23 05:04:10]

LG