PrintPreview: Print Preview is not staying associated with initiator renderer.

PrintPreview: Print Preview is not staying associated with initiator renderer.

Created a PrintPreviewDataService class to manage the data for all preview tabs. 

Renamed PrintPreviewUIHTMLSource class to PrintPreviewDataSource class. PrintPreviewDataSource is responsible for serving the data from PrintPreviewDataSrervice to the corresponding PrintPreviewUI data requests.

BUG=83151
TEST=Refer to bug report.

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

[kmadhusu, 2011-05-25 00:17:39]

thestig:[WIP] I have uploaded the CL for your reference. 

Thanks for your help.

[kmadhusu, 2011-05-25 19:54:32]

sky: Please review print_preview_data_source code.

thestig: Please review Print Preview UI code.

Thanks.

[sky, 2011-05-25 20:57:35]

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:126: :
DataSource(chrome::kChromeUIPrintHost, NULL) {
Passing in NULL means StartDataRequest is most likely going to be invoked on the
IO thread. Which I suspect means you need some thread safety in this class. Are
you sure you want that?

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:136: if (path.size() > 10
&& path.substr(0,10) == "print.pdf/") {
nit:'0,10' -> '0, 10'

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:160: path.substr(0,10) ==
"print.pdf/" &&
nit: '0,10' -> '0, 10'

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:187: void
PrintPreviewDataSource::SetDataEntry(
nit: position in file should match position in header.

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:189:
PreviewDataSrcMap::iterator i = data_source_map_.find(preview_ui_addr_str);
Replace 189-191 with call to RemoveEntry.

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.h (right):

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:43: typedef
std::pair<base::SharedMemory*, uint32> PrintPreviewData;
To make lifetime management easier, could this take RefCountedBytes? That way
you don't have to delete at line 191 new in print_preview_data_source.cc, and
you don't have to worry about leaking in destructor.

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:53: static
PrintPreviewDataSource* GetInstance();
nit: constructor/destructor before static methods.

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:55:
PrintPreviewDataSource();
Is it possible to make the constructor private?

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:70: virtual void
StartDataRequest(const std::string& path,
nit: Use OVERRIDE here and 73.

[kmadhusu, 2011-05-25 23:45:20]

sky: Addressed review comments. 

I am still working on 
1. PrintPreviewTabController unittest failures.
2. Rather than a singleton data source, we need to create one
PrintPreviewDataSource per profile.

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:126: :
DataSource(chrome::kChromeUIPrintHost, NULL) {
On 2011/05/25 20:57:35, sky wrote:
> Passing in NULL means StartDataRequest is most likely going to be invoked on
the
> IO thread. Which I suspect means you need some thread safety in this class.
Are
> you sure you want that?

Fixed. Changed Null to "MessageLoop::current()".

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:136: if (path.size() > 10
&& path.substr(0,10) == "print.pdf/") {
On 2011/05/25 20:57:35, sky wrote:
> nit:'0,10' -> '0, 10'

Done.

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:160: path.substr(0,10) ==
"print.pdf/" &&
On 2011/05/25 20:57:35, sky wrote:
> nit: '0,10' -> '0, 10'

Done.

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:187: void
PrintPreviewDataSource::SetDataEntry(
On 2011/05/25 20:57:35, sky wrote:
> nit: position in file should match position in header.

Done.

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:189:
PreviewDataSrcMap::iterator i = data_source_map_.find(preview_ui_addr_str);
On 2011/05/25 20:57:35, sky wrote:
> Replace 189-191 with call to RemoveEntry.

Done.

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.h (right):

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:43: typedef
std::pair<base::SharedMemory*, uint32> PrintPreviewData;
On 2011/05/25 20:57:35, sky wrote:
> To make lifetime management easier, could this take RefCountedBytes? That way
> you don't have to delete at line 191 new in print_preview_data_source.cc, and
> you don't have to worry about leaking in destructor.

I understand your concern. Just to be consistent with rest of the print preview
data structures, I used SharedMemory here.

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:53: static
PrintPreviewDataSource* GetInstance();
On 2011/05/25 20:57:35, sky wrote:
> nit: constructor/destructor before static methods.

Done.

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:55:
PrintPreviewDataSource();
On 2011/05/25 20:57:35, sky wrote:
> Is it possible to make the constructor private?

Since I am using "LeakySingleTonTraits", it needs to be public.

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:70: virtual void
StartDataRequest(const std::string& path,
On 2011/05/25 20:57:35, sky wrote:
> nit: Use OVERRIDE here and 73.

Done.

[sky, 2011-05-25 23:59:32]

On Wed, May 25, 2011 at 4:45 PM,  <kmadhusu@chromium.org> wrote:
> sky: Addressed review comments.
>
> I am still working on
> 1. PrintPreviewTabController unittest failures.
> 2. Rather than a singleton data source, we need to create one
> PrintPreviewDataSource per profile.

Just to clarify, the reason you aren't getting one
PrintPreviewDataSource per tab is that you're using a singleton. I
think you're going to need to change PrintPreviewDataSource not to be
a singleton, and not store the data, but instead reference some other
singleton like object that stores the data. This way
PrintPreviewDataSource is pretty stateless and you can create a new
one each time you create a print preview dom ui and everything should
work.

  -Scott
>
>
>
http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
> File chrome/browser/ui/webui/print_preview_data_source.cc (right):
>
>
http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
> chrome/browser/ui/webui/print_preview_data_source.cc:126: :
> DataSource(chrome::kChromeUIPrintHost, NULL) {
> On 2011/05/25 20:57:35, sky wrote:
>>
>> Passing in NULL means StartDataRequest is most likely going to be
>
> invoked on the
>>
>> IO thread. Which I suspect means you need some thread safety in this
>
> class. Are
>>
>> you sure you want that?
>
> Fixed. Changed Null to "MessageLoop::current()".
>
>
http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
> chrome/browser/ui/webui/print_preview_data_source.cc:136: if
> (path.size() > 10 && path.substr(0,10) == "print.pdf/") {
> On 2011/05/25 20:57:35, sky wrote:
>>
>> nit:'0,10' -> '0, 10'
>
> Done.
>
>
http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
> chrome/browser/ui/webui/print_preview_data_source.cc:160:
> path.substr(0,10) == "print.pdf/" &&
> On 2011/05/25 20:57:35, sky wrote:
>>
>> nit: '0,10' -> '0, 10'
>
> Done.
>
>
http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
> chrome/browser/ui/webui/print_preview_data_source.cc:187: void
> PrintPreviewDataSource::SetDataEntry(
> On 2011/05/25 20:57:35, sky wrote:
>>
>> nit: position in file should match position in header.
>
> Done.
>
>
http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
> chrome/browser/ui/webui/print_preview_data_source.cc:189:
> PreviewDataSrcMap::iterator i =
> data_source_map_.find(preview_ui_addr_str);
> On 2011/05/25 20:57:35, sky wrote:
>>
>> Replace 189-191 with call to RemoveEntry.
>
> Done.
>
>
http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
> File chrome/browser/ui/webui/print_preview_data_source.h (right):
>
>
http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
> chrome/browser/ui/webui/print_preview_data_source.h:43: typedef
> std::pair<base::SharedMemory*, uint32> PrintPreviewData;
> On 2011/05/25 20:57:35, sky wrote:
>>
>> To make lifetime management easier, could this take RefCountedBytes?
>
> That way
>>
>> you don't have to delete at line 191 new in
>
> print_preview_data_source.cc, and
>>
>> you don't have to worry about leaking in destructor.
>
> I understand your concern. Just to be consistent with rest of the print
> preview data structures, I used SharedMemory here.
>
>
http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
> chrome/browser/ui/webui/print_preview_data_source.h:53: static
> PrintPreviewDataSource* GetInstance();
> On 2011/05/25 20:57:35, sky wrote:
>>
>> nit: constructor/destructor before static methods.
>
> Done.
>
>
http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
> chrome/browser/ui/webui/print_preview_data_source.h:55:
> PrintPreviewDataSource();
> On 2011/05/25 20:57:35, sky wrote:
>>
>> Is it possible to make the constructor private?
>
> Since I am using "LeakySingleTonTraits", it needs to be public.
>
>
http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
> chrome/browser/ui/webui/print_preview_data_source.h:70: virtual void
> StartDataRequest(const std::string& path,
> On 2011/05/25 20:57:35, sky wrote:
>>
>> nit: Use OVERRIDE here and 73.
>
> Done.
>
> http://codereview.chromium.org/7063030/
>

[Lei Zhang, 2011-05-26 01:48:31]

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:34: var previewUIIdentifier = null;
This doesn't need to be a global variable. Just pass it as an argument to
createPDFPlugin().

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:151:
PreviewDataSrcMap::iterator i = data_source_map_.find(preview_ui_addr_str);
nit: name the iterator "it"

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:162:
GetDataEntry(path.substr(10), &data);
Why not just put this in the else if block below?

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.h (right):

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:38: //   
chrome://print/print.pdf/0x7f5470fbe510
That's a weird looking URL. Can we do chrome://print/print.pdf?ab0123ef  ??

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:46: typedef std::string
PreviewUIAddrStr;
can we not have this typedef and just use std::string?

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:60: void RemoveEntry(const
PreviewUIAddrStr& preview_ui_addr_str);
nit: Can you put them in a more logical order? Get/Set/Remove.

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:77: friend struct
LeakySingletonTraits<PrintPreviewDataSource>;
Use DefaultSingletonTraits here. Then you can make the ctor/dtor private. See
other LeakySingletonTraits usage for examples.

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

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_ui.cc:23: char preview_ui_addr[128];
Since we are using the memory address as the identifier, I think the string size
should be: (2 * sizeof(this)) + 3, where 3 = '0x' + null terminator.

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_ui.h:50: scoped_ptr<std::string>
preview_ui_addr_str_;
Why does this need to be a scoped_ptr?

[kmadhusu, 2011-05-26 15:51:22]

Created a PrintPreviewDataManager class to manage the data for all preview tabs.
PrintPreviewDataManager is profile specific.

Renamed PrintPreviewUIHTMLSource class to PrintPreviewDataSource class.
PrintPreviewDataSource is responsible for serving the data from
PrintPreviewDataManager class to the corresponding PrintPreviewUI requests.

sky: Please review print_preview_data_source, print_preview_data_manager and
other profile code.

thestig: Addressed review comments. Please review print preview UI, JS code.

Thanks.

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/resources/pri...
chrome/browser/resources/print_preview.js:34: var previewUIIdentifier = null;
On 2011/05/26 01:48:31, Lei Zhang wrote:
> This doesn't need to be a global variable. Just pass it as an argument to
> createPDFPlugin().

Done.

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.cc (right):

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:151:
PreviewDataSrcMap::iterator i = data_source_map_.find(preview_ui_addr_str);
On 2011/05/26 01:48:31, Lei Zhang wrote:
> nit: name the iterator "it"

Done.

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.cc:162:
GetDataEntry(path.substr(10), &data);
On 2011/05/26 01:48:31, Lei Zhang wrote:
> Why not just put this in the else if block below?

If data.first is null, I want to send empty_bytes (which is the else block
statements).

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.h (right):

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:38: //   
chrome://print/print.pdf/0x7f5470fbe510
On 2011/05/26 01:48:31, Lei Zhang wrote:
> That's a weird looking URL. Can we do chrome://print/print.pdf?ab0123ef  ??

Done.

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:46: typedef std::string
PreviewUIAddrStr;
On 2011/05/26 01:48:31, Lei Zhang wrote:
> can we not have this typedef and just use std::string?

Done.

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:60: void RemoveEntry(const
PreviewUIAddrStr& preview_ui_addr_str);
On 2011/05/26 01:48:31, Lei Zhang wrote:
> nit: Can you put them in a more logical order? Get/Set/Remove.

Done.

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:77: friend struct
LeakySingletonTraits<PrintPreviewDataSource>;
On 2011/05/26 01:48:31, Lei Zhang wrote:
> Use DefaultSingletonTraits here. Then you can make the ctor/dtor private. See
> other LeakySingletonTraits usage for examples.

Removed the singleton traits.

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

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_ui.cc:23: char preview_ui_addr[128];
On 2011/05/26 01:48:31, Lei Zhang wrote:
> Since we are using the memory address as the identifier, I think the string
size
> should be: (2 * sizeof(this)) + 3, where 3 = '0x' + null terminator.

Done.

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/7063030/diff/8015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_ui.h:50: scoped_ptr<std::string>
preview_ui_addr_str_;
On 2011/05/26 01:48:31, Lei Zhang wrote:
> Why does this need to be a scoped_ptr?

Fixed.

[sky, 2011-05-26 17:40:41]

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_data_source.h (right):

http://codereview.chromium.org/7063030/diff/4014/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_data_source.h:43: typedef
std::pair<base::SharedMemory*, uint32> PrintPreviewData;
On 2011/05/25 23:45:21, kmadhusu wrote:
> On 2011/05/25 20:57:35, sky wrote:
> > To make lifetime management easier, could this take RefCountedBytes? That
way
> > you don't have to delete at line 191 new in print_preview_data_source.cc,
and
> > you don't have to worry about leaking in destructor.
> 
> I understand your concern. Just to be consistent with rest of the print
preview
> data structures, I used SharedMemory here. 

Shouldn't you be striving for less error prone code? Even so, you could still
take a SharedMemory but internally use RefCountedBytes. Especially since this
class ends up serving RefCountedBytes.

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:499: virtual PrintPreviewDataManager*
GetPrintPreviewDataManager() = 0;
We're actively removing things like this from Profile. Talk with Elliot about
the right way to keep per profile state.

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_manager.cc (right):

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_manager.cc:7: #include <vector>
nuke this include.

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_manager.cc:16: }
If you don't convert to RefCountedBytes, then this leaks if the map is
non-empty.

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_manager.h (right):

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_manager.h:29: : public
base::RefCounted<PrintPreviewDataManager> {
Does this class really need to be refcounted?

[kmadhusu, 2011-05-27 16:49:22]

sky: Addressed review comments. Discussed with Elliot regarding the profile
specific code and made code changes accordingly. Made code change to store the
RefCountedBytes data in print_preview_data_source. Please review
print_preview_data_source, print_preview_data_service.

erg: Please review print_preview_data_service,
print_preview_data_service_factory and other profile related code.

thestig: Please review print preview ui code. 

Thanks.

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:499: virtual PrintPreviewDataManager*
GetPrintPreviewDataManager() = 0;
On 2011/05/26 17:40:41, sky wrote:
> We're actively removing things like this from Profile. Talk with Elliot about
> the right way to keep per profile state.

Discussed with Elliot regarding this and made code changes accordingly.

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_manager.cc (right):

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_manager.cc:7: #include <vector>
On 2011/05/26 17:40:41, sky wrote:
> nuke this include.

Done.

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_manager.cc:16: }
On 2011/05/26 17:40:41, sky wrote:
> If you don't convert to RefCountedBytes, then this leaks if the map is
> non-empty.

Fixed. Modified code to store the RefCountedBytes data.

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_manager.h (right):

http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_manager.h:29: : public
base::RefCounted<PrintPreviewDataManager> {
On 2011/05/26 17:40:41, sky wrote:
> Does this class really need to be refcounted?

No. Fixed the code.

[kmadhusu, 2011-05-27 16:51:30]

erg: Please review print_preview_data_service,
print_preview_data_service_factory and other profile related code.

Thanks.

[sky, 2011-05-27 17:06:39]

On Fri, May 27, 2011 at 9:49 AM,  <kmadhusu@chromium.org> wrote:
> sky: Addressed review comments. Discussed with Elliot regarding the profile
> specific code and made code changes accordingly. Made code change to store
> the
> RefCountedBytes data in print_preview_data_source. Please review
> print_preview_data_source, print_preview_data_service.

I should have mentioned this the other day. It really isn't necessary
to hang your data object (PrintPreviewDataService) off the profile. It
can be a singleton.

  -Scott

> erg: Please review print_preview_data_service,
> print_preview_data_service_factory and other profile related code.
>
> thestig: Please review print preview ui code.
>
> Thanks.
>
>
>
>
http://codereview.chromium.org/7063030/diff/14009/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile.h (right):
>
>
http://codereview.chromium.org/7063030/diff/14009/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile.h:499: virtual PrintPreviewDataManager*
> GetPrintPreviewDataManager() = 0;
> On 2011/05/26 17:40:41, sky wrote:
>>
>> We're actively removing things like this from Profile. Talk with
>
> Elliot about
>>
>> the right way to keep per profile state.
>
> Discussed with Elliot regarding this and made code changes accordingly.
>
>
http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
> File chrome/browser/ui/webui/print_preview_data_manager.cc (right):
>
>
http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
> chrome/browser/ui/webui/print_preview_data_manager.cc:7: #include
> <vector>
> On 2011/05/26 17:40:41, sky wrote:
>>
>> nuke this include.
>
> Done.
>
>
http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
> chrome/browser/ui/webui/print_preview_data_manager.cc:16: }
> On 2011/05/26 17:40:41, sky wrote:
>>
>> If you don't convert to RefCountedBytes, then this leaks if the map is
>> non-empty.
>
> Fixed. Modified code to store the RefCountedBytes data.
>
>
http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
> File chrome/browser/ui/webui/print_preview_data_manager.h (right):
>
>
http://codereview.chromium.org/7063030/diff/14009/chrome/browser/ui/webui/pri...
> chrome/browser/ui/webui/print_preview_data_manager.h:29: : public
> base::RefCounted<PrintPreviewDataManager> {
> On 2011/05/26 17:40:41, sky wrote:
>>
>> Does this class really need to be refcounted?
>
> No. Fixed the code.
>
> http://codereview.chromium.org/7063030/
>

[Lei Zhang, 2011-05-27 17:26:53]

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

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:88: char* preview_data
= reinterpret_cast<char*>(shared_buf->memory());
nit: static_cast

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:28: #include
"chrome/browser/printing/print_preview_data_service_factory.h"
nit: ordering

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:49: #include
"chrome/browser/printing/print_preview_data_service_factory.h"
nit: ordering

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:495: * @param {string} previewUIId
Preview UI identifier.
nit: can you just call this "previewUid" here and elsewhere?

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:564: pdfPlugin.setAttribute('src',
'chrome://print/print.pdf/' + previewUIId);
I thought we were going to do chrome://print/print.pdf?ab0123ef

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

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:14: #include
"chrome/browser/profiles/profile.h"
nit: ordering

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:126: if (path.size() > 10
&& path.substr(0, 10) == "print.pdf/") {
if you set a boolean to true here, then you don't have to repeat the similar
check below on line 149. If you ever change it afterwards, you'll only need to
change the check here.

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

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:549:
metafile->InitFromData(reinterpret_cast<const void*>(data->front()),
can you static_cast here?

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

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:27: char preview_ui_addr[(2 *
sizeof(this)) + 3];
add a comment to explain the math here.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:38:
PrintPreviewDataServiceFactory::GetForProfile(tab_contents()->profile())->
You call
PrintPreviewDataServiceFactory::GetForProfile(tab_contents()->profile()) in
several places. Is it safe to save the return value?

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_data_source_unittest.cc (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_data_source_unittest.cc:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
why is this print_preview_ui_data_source_unittest.cc and not
print_preview_data_source_unittest.cc?

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_data_source_unittest.cc:6: #include
"base/memory/ref_counted.h"
nit: not needed?

http://codereview.chromium.org/7063030/diff/18001/chrome/test/testing_profile.cc
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/test/testing_profile...
chrome/test/testing_profile.cc:168:
PrintPreviewDataServiceFactory::GetInstance()->set_test_factory(
nit: keep it in alphabetical order?

[Elliot Glaysher, 2011-05-27 17:45:50]

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_data_service.h (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_data_service.h:30: PreviewDataSourceMap;
Why is this public?

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:209:
PrintPreviewDataServiceFactory::GetForProfile(this);
Does this really need to be force initialized during the constructor? IIRC, it
was lazily constructed previously.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:360:
PrintPreviewDataServiceFactory::GetForProfile(this);
Same.

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

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:38:
PrintPreviewDataServiceFactory::GetForProfile(tab_contents()->profile())->
On 2011/05/27 17:26:53, Lei Zhang wrote:
> You call
> PrintPreviewDataServiceFactory::GetForProfile(tab_contents()->profile()) in
> several places. Is it safe to save the return value?

I'd prefer not to cache that value now that there's runtime checking in debug
mode to make sure you aren't using a freed profile/service. Could be made into a
method though.

[kmadhusu, 2011-05-27 23:44:42]

Addressed review comments.

sky: Please review print_preview_data_source, print_preview_data_service.
PrintPreviewDataService cannot be a singleton class(discussed with erg@
regarding this). But I have added the constructor and destructor as private
member functions and set the PrintPreviewServiceFactory and TestingProfile as a
friend of this class to instantiate the object. 

erg: Please review print_preview_data_service,
print_preview_data_service_factory and other profile related code.

thestig: Please review print preview ui code.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_data_service.h (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_data_service.h:30: PreviewDataSourceMap;
On 2011/05/27 17:45:50, Elliot Glaysher wrote:
> Why is this public?

It can be private. Fixed.

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

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.cc:88: char* preview_data
= reinterpret_cast<char*>(shared_buf->memory());
On 2011/05/27 17:26:53, Lei Zhang wrote:
> nit: static_cast

Done.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:28: #include
"chrome/browser/printing/print_preview_data_service_factory.h"
On 2011/05/27 17:26:53, Lei Zhang wrote:
> nit: ordering

Ordering LGTM.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.cc:209:
PrintPreviewDataServiceFactory::GetForProfile(this);
On 2011/05/27 17:45:50, Elliot Glaysher wrote:
> Does this really need to be force initialized during the constructor? IIRC, it
> was lazily constructed previously.

Forced initialization is not required. Removed the code.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:49: #include
"chrome/browser/printing/print_preview_data_service_factory.h"
On 2011/05/27 17:26:53, Lei Zhang wrote:
> nit: ordering

Ordering LGTM.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:360:
PrintPreviewDataServiceFactory::GetForProfile(this);
On 2011/05/27 17:45:50, Elliot Glaysher wrote:
> Same.

Removed.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview.js (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:495: * @param {string} previewUIId
Preview UI identifier.
On 2011/05/27 17:26:53, Lei Zhang wrote:
> nit: can you just call this "previewUid" here and elsewhere?

Done.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview.js:564: pdfPlugin.setAttribute('src',
'chrome://print/print.pdf/' + previewUIId);
On 2011/05/27 17:26:53, Lei Zhang wrote:
> I thought we were going to do chrome://print/print.pdf?ab0123ef

(repeating our in-person conversation) There is no specific URL format.
Considering other issues, I have changed the format to
"chrome://print/ab0123ef/print.pdf"

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

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:14: #include
"chrome/browser/profiles/profile.h"
On 2011/05/27 17:26:53, Lei Zhang wrote:
> nit: ordering

Fixed.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:126: if (path.size() > 10
&& path.substr(0, 10) == "print.pdf/") {
On 2011/05/27 17:26:53, Lei Zhang wrote:
> if you set a boolean to true here, then you don't have to repeat the similar
> check below on line 149. If you ever change it afterwards, you'll only need to
> change the check here.

Done.

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

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:549:
metafile->InitFromData(reinterpret_cast<const void*>(data->front()),
On 2011/05/27 17:26:53, Lei Zhang wrote:
> can you static_cast here?

Done.

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

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:27: char preview_ui_addr[(2 *
sizeof(this)) + 3];
On 2011/05/27 17:26:53, Lei Zhang wrote:
> add a comment to explain the math here.

Done.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:38:
PrintPreviewDataServiceFactory::GetForProfile(tab_contents()->profile())->
On 2011/05/27 17:45:50, Elliot Glaysher wrote:
> On 2011/05/27 17:26:53, Lei Zhang wrote:
> > You call
> > PrintPreviewDataServiceFactory::GetForProfile(tab_contents()->profile()) in
> > several places. Is it safe to save the return value?
> 
> I'd prefer not to cache that value now that there's runtime checking in debug
> mode to make sure you aren't using a freed profile/service. Could be made into
a
> method though.

Created a helper function.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_data_source_unittest.cc (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_data_source_unittest.cc:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
On 2011/05/27 17:26:53, Lei Zhang wrote:
> why is this print_preview_ui_data_source_unittest.cc and not
> print_preview_data_source_unittest.cc?

Testing the print_preview_ui member function behavior. So I preferred
print_preview_ui_data_source_unittest.

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_data_source_unittest.cc:6: #include
"base/memory/ref_counted.h"
On 2011/05/27 17:26:53, Lei Zhang wrote:
> nit: not needed?

Removed.

http://codereview.chromium.org/7063030/diff/18001/chrome/test/testing_profile.cc
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/test/testing_profile...
chrome/test/testing_profile.cc:168:
PrintPreviewDataServiceFactory::GetInstance()->set_test_factory(
On 2011/05/27 17:26:53, Lei Zhang wrote:
> nit: keep it in alphabetical order?

Done.

[Lei Zhang, 2011-05-28 00:12:38]

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_data_source_unittest.cc (right):

http://codereview.chromium.org/7063030/diff/18001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_data_source_unittest.cc:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
On 2011/05/27 23:44:42, kmadhusu wrote:
> On 2011/05/27 17:26:53, Lei Zhang wrote:
> > why is this print_preview_ui_data_source_unittest.cc and not
> > print_preview_data_source_unittest.cc?
> 
> Testing the print_preview_ui member function behavior. So I preferred
> print_preview_ui_data_source_unittest.

Then shouldn't this be print_preview_ui_unittest.cc?

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_data_service.h (right):

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_data_service.h:25: // Get the data entry
from PreviewDataSrcMap.
nit: s/Src/Source/ here and below.

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

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:27: // 3 is for '0x' and null
terminator.
How about:
// "0x" + deadc0de + '\0' = 2 + 2 * sizeof(this) + 1;

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_data_source_unittest.cc (right):

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_data_source_unittest.cc:6: //#include
"base/memory/ref_counted.h"
not removed!

[sky, 2011-05-28 00:13:40]

LGTM, but as voiced on IM I think we're better served by making
PrintPreviewDataService a singleton (not extending ProfileKeyedService). Making
PrintPreviewDataService a singleton work fines for your use case and is simpler
to understand and lighter weight than what you have now.

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_data_service_factory.h (right):

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_data_service_factory.h:22: static
PrintPreviewDataService* GetForProfile(Profile* profile);
nit: newline between 21 and 22.

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_data_service_factory.h:34: };
DISALLOW_COPY_AND_ASSIGN

[kmadhusu, 2011-05-28 01:33:27]

sky: Made code changes to make PrintPreviewDataService a singleton (not
extending ProfileKeyedService). Deleted the PrintPreviewDataServiceFactory.
Please review the latest patch. If it is LG to you. I will request thestig@ to
review print preview ui code. 

Thanks.

[kmadhusu, 2011-05-29 00:04:26]

thestig: Renamed print_preview_ui_data_source_unittest.cc to
print_prevew_ui_unittest.cc. Addressed review comments.

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_data_service.h (right):

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_data_service.h:25: // Get the data entry
from PreviewDataSrcMap.
On 2011/05/28 00:12:38, Lei Zhang wrote:
> nit: s/Src/Source/ here and below.

Done.

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

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.cc:27: // 3 is for '0x' and null
terminator.
On 2011/05/28 00:12:38, Lei Zhang wrote:
> How about:
> // "0x" + deadc0de + '\0' = 2 + 2 * sizeof(this) + 1;

Done.

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui_data_source_unittest.cc (right):

http://codereview.chromium.org/7063030/diff/23040/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui_data_source_unittest.cc:6: //#include
"base/memory/ref_counted.h"
On 2011/05/28 00:12:38, Lei Zhang wrote:
> not removed!

Done.

[Lei Zhang, 2011-05-29 00:24:23]

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_data_service.h (right):

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_data_service.h:21: //    a) there is a new
data.
nit: There is new data.

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

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:121: size_t index =
path.find("/print.pdf");
This is not 100% correct. E.g. it will parse chrome://print/foo/print.pdf/bar as
though it is chrome://print/foo/print.pdf. You probably want to check with
EndsWith() from string_util.h.

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_source.h (right):

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.h:23: //   
chrome://print/ab0123ef/print.pdf
nit: the address is actually 0xab0123ef

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:13: #include
"chrome/browser/printing/print_preview_data_service.h"
forward declare instead?

[kmadhusu, 2011-05-29 00:45:39]

thestig: Addressed review comments.

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_data_service.h (right):

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_data_service.h:21: //    a) there is a new
data.
On 2011/05/29 00:24:23, Lei Zhang wrote:
> nit: There is new data.

Done.

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

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:121: size_t index =
path.find("/print.pdf");
On 2011/05/29 00:24:23, Lei Zhang wrote:
> This is not 100% correct. E.g. it will parse chrome://print/foo/print.pdf/bar
as
> though it is chrome://print/foo/print.pdf. You probably want to check with
> EndsWith() from string_util.h.

Done.

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_source.h (right):

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.h:23: //   
chrome://print/ab0123ef/print.pdf
On 2011/05/29 00:24:23, Lei Zhang wrote:
> nit: the address is actually 0xab0123ef

Done.

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/7063030/diff/20063/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:13: #include
"chrome/browser/printing/print_preview_data_service.h"
On 2011/05/29 00:24:23, Lei Zhang wrote:
> forward declare instead?

Done.

[Lei Zhang, 2011-05-29 00:48:02]

LGTM with minor problem below

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

http://codereview.chromium.org/7063030/diff/21027/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:125: size_t index =
path.find("/print.pdf");
This should be a rfind().

[kmadhusu, 2011-05-29 00:57:49]

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

http://codereview.chromium.org/7063030/diff/21027/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.cc:125: size_t index =
path.find("/print.pdf");
On 2011/05/29 00:48:02, Lei Zhang wrote:
> This should be a rfind().

Done.

[Lei Zhang, 2011-05-29 10:12:02]

http://codereview.chromium.org/7063030/diff/20067/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_source.h (right):

http://codereview.chromium.org/7063030/diff/20067/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.h:27:
PrintPreviewDataSource();
Have you thought about changing this to:
PrintPreviewDataSource(PrintPreviewUI* preview_ui) ?

This can make things much simpler:
- the print preview tab changes can all be reverted, just request
chrome://print/print.pdf as before.
- PrintPreviewDataSource can just check path == "print.pdf" again.
-- A compromised renderer can't access another print preview tab's data via
their print preview UI identifier.

- PreviewDataSourceMap can be a std::map<PrintPreviewUI*,
scoped_refptr<RefCountedBytes> > instead.
- No longer need to do the snprintf() in PrintPreviewUI.
- No longer need |preview_ui_addr_str_| in PrintPreviewUI.

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

http://codereview.chromium.org/7063030/diff/20067/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:588: if (!data->front() ||
!data->size()) {
BTW, you don't need to check for size(), a valid front() implies size() will
return a number > 0.

[kmadhusu, 2011-05-29 16:38:04]

http://codereview.chromium.org/7063030/diff/20067/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_data_source.h (right):

http://codereview.chromium.org/7063030/diff/20067/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_data_source.h:27:
PrintPreviewDataSource();
On 2011/05/29 10:12:02, Lei Zhang wrote:
> Have you thought about changing this to:
> PrintPreviewDataSource(PrintPreviewUI* preview_ui) ?
> 
> This can make things much simpler:
> - the print preview tab changes can all be reverted, just request
> chrome://print/print.pdf as before.
> - PrintPreviewDataSource can just check path == "print.pdf" again.
> -- A compromised renderer can't access another print preview tab's data via
> their print preview UI identifier.
> 
> - PreviewDataSourceMap can be a std::map<PrintPreviewUI*,
> scoped_refptr<RefCountedBytes> > instead.
> - No longer need to do the snprintf() in PrintPreviewUI.
> - No longer need |preview_ui_addr_str_| in PrintPreviewUI.

Yeah I thought about it. But it will boils down to the original problem. 
(Sample CL which I used to confirm this is :
http://codereview.chromium.org/7082032/)

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

http://codereview.chromium.org/7063030/diff/20067/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:588: if (!data->front() ||
!data->size()) {
On 2011/05/29 10:12:02, Lei Zhang wrote:
> BTW, you don't need to check for size(), a valid front() implies size() will
> return a number > 0.

Fixed.

[Lei Zhang, 2011-05-29 21:00:39]

Ah, ok, I see how we arrived at this design. Makes sense now. LGTM.

[kmadhusu, 2011-05-30 03:40:04]

sky: Give the time constraint of M13 branch cut, I am committing this CL with
your previous LGTM comment.  I would just be more than happy to do a follow up
CL to address any further comments related to my changes.

[sky, 2011-05-31 15:55:54]

Yes, SLGTM.
Thanks for converting to a singleton!