Added resources.

Added resources.
Virtual_driver_setup now has localization support.
All dlls and exes now have a version resource.
Common (untranslated) strings are now pulled from a common resource file.
Installer and port monitor now have separate .gyp files.
Still to do: Integrate with TC and add all languages.

BUG=
TEST=

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

[Albert Bodenhamer, 2011-05-06 22:58:26]

Feel free to wait for Monday on this.

[sanjeevr, 2011-05-09 19:39:11]

http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/win/install/setup.cc (right):

http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/win/install/setup.cc:160:
cloud_print::LoadLocalString(IDS_XPS_DRIVER, buffer, MAX_PATH);
Do the DLL names need to be in the resource?

http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/win/virtual_driver_common_resources.rc (right):

http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/win/virtual_driver_common_resources.rc:47:
IDS_GCP_URL                "http://www.google.com/cloudprint"
I am not sure why these need to be in a resource file if we are not translating
them.

http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/win/virtual_driver_helpers.h (right):

http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/win/virtual_driver_helpers.h:35: void
LoadLocalString(DWORD string_id, wchar_t* target, DWORD target_size);
This should take an std::wstring* as out argument. Don't we already have methods
in base to load a string from resources?

[Albert Bodenhamer, 2011-05-09 20:51:25]

On Mon, May 9, 2011 at 12:39 PM, <sanjeevr@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
> File cloud_print/virtual_driver/win/install/setup.cc (right):
>
>
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/win/install/setup.cc:160:
> cloud_print::LoadLocalString(IDS_XPS_DRIVER, buffer, MAX_PATH);
> Do the DLL names need to be in the resource?
>
>
I like the idea of keeping it data driven.  The resources effectively become
a configuration file and if we need to we can access strings from either the
installer or port monitor dll.  For example, both need to know the name of
the port monitor DLL and this lets me put it in a single location.  I've
also been thinking of switching to a PPD instead of a GPD and this way I
could do it just by changing the resources.


>
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
> File cloud_print/virtual_driver/win/virtual_driver_common_resources.rc
> (right):
>
>
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/win/virtual_driver_common_resources.rc:47:
> IDS_GCP_URL                "http://www.google.com/cloudprint"
> I am not sure why these need to be in a resource file if we are not
> translating them.
>
>
See above.


>
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
> File cloud_print/virtual_driver/win/virtual_driver_helpers.h (right):
>
>
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/win/virtual_driver_helpers.h:35: void
> LoadLocalString(DWORD string_id, wchar_t* target, DWORD target_size);
> This should take an std::wstring* as out argument. Don't we already have
> methods in base to load a string from resources?
>
> Most places I'm using it immediately hand the string off to a Windows API
call that expects a wchar_t array.  I could make it use a wstring or
string16 but that would involve either a lot of const casting or copying to
a wchar_t array.  It seemed saner this way.

There ARE functions for loading from a resource but they all assume that
each language will be pulled from it's own DLL.  That doesn't really make
sense for something like this.  The native LoadString function does exactly
what I need.




>
> http://codereview.chromium.org/6930019/
>

[sanjeevr, 2011-05-09 21:37:59]

On 2011/05/09 20:51:25, Albert Bodenhamer wrote:
> On Mon, May 9, 2011 at 12:39 PM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=sanjeevr@chromium.org>
wrote:
> 
> >
> >
> >
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
> > File cloud_print/virtual_driver/win/install/setup.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
> > cloud_print/virtual_driver/win/install/setup.cc:160:
> > cloud_print::LoadLocalString(IDS_XPS_DRIVER, buffer, MAX_PATH);
> > Do the DLL names need to be in the resource?
> >
> >
> I like the idea of keeping it data driven.  The resources effectively become
> a configuration file and if we need to we can access strings from either the
> installer or port monitor dll.  For example, both need to know the name of
> the port monitor DLL and this lets me put it in a single location.  I've
> also been thinking of switching to a PPD instead of a GPD and this way I
> could do it just by changing the resources.
>

Fair enough, but the way the rest of Chrome handles this is by using a config
file (so it is truly data driven) and not by specifying stuff in the resources.
You need to compile the code anyway if you change the resource and in that case,
you might as well keep it in a consts cc file.
 
> 
> >
> >
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
> > File cloud_print/virtual_driver/win/virtual_driver_common_resources.rc
> > (right):
> >
> >
> >
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
> > cloud_print/virtual_driver/win/virtual_driver_common_resources.rc:47:
> > IDS_GCP_URL                "http://www.google.com/cloudprint"
> > I am not sure why these need to be in a resource file if we are not
> > translating them.
> >
> >
> See above.
> 
> 
> >
> >
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
> > File cloud_print/virtual_driver/win/virtual_driver_helpers.h (right):
> >
> >
> >
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
> > cloud_print/virtual_driver/win/virtual_driver_helpers.h:35: void
> > LoadLocalString(DWORD string_id, wchar_t* target, DWORD target_size);
> > This should take an std::wstring* as out argument. Don't we already have
> > methods in base to load a string from resources?
> >
> > Most places I'm using it immediately hand the string off to a Windows API
> call that expects a wchar_t array.  I could make it use a wstring or
> string16 but that would involve either a lot of const casting or copying to
> a wchar_t array.  It seemed saner this way.
> 
> There ARE functions for loading from a resource but they all assume that
> each language will be pulled from it's own DLL.  That doesn't really make
> sense for something like this.  The native LoadString function does exactly
> what I need.

Shielding callers from having to worry about buffer sizes is a win. The method
itself should return a string16. The method can compute the required length
using LoadString.

> 
> 
> 
> 
> >
> > http://codereview.chromium.org/6930019/
> >

[Albert Bodenhamer, 2011-05-10 00:47:47]

Ok, give it another look.

Rather than make 2 calls to LoadString to determine the space needed for the
resource I'm creating a fixed sized buffer on the stack and DCHECKing if
LoadString fails.  Since we control the strings and there are only a couple
of them I think that's adequate.  It will show up quickly and is easy enough
to change if we need to do more in the future.

On Mon, May 9, 2011 at 2:37 PM, <sanjeevr@chromium.org> wrote:

> On 2011/05/09 20:51:25, Albert Bodenhamer wrote:
>
>> On Mon, May 9, 2011 at 12:39 PM,
>>
> <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=sanjeevr@chromium.org>
>
> wrote:
>
>  >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
>
>> > File cloud_print/virtual_driver/win/install/setup.cc (right):
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
>
>> > cloud_print/virtual_driver/win/install/setup.cc:160:
>> > cloud_print::LoadLocalString(IDS_XPS_DRIVER, buffer, MAX_PATH);
>> > Do the DLL names need to be in the resource?
>> >
>> >
>> I like the idea of keeping it data driven.  The resources effectively
>> become
>> a configuration file and if we need to we can access strings from either
>> the
>> installer or port monitor dll.  For example, both need to know the name of
>> the port monitor DLL and this lets me put it in a single location.  I've
>> also been thinking of switching to a PPD instead of a GPD and this way I
>> could do it just by changing the resources.
>>
>
>
> Fair enough, but the way the rest of Chrome handles this is by using a
> config
> file (so it is truly data driven) and not by specifying stuff in the
> resources.
> You need to compile the code anyway if you change the resource and in that
> case,
> you might as well keep it in a consts cc file.
>
>
>
>  >
>> >
>>
>
>
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
>
>> > File cloud_print/virtual_driver/win/virtual_driver_common_resources.rc
>> > (right):
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
>
>> > cloud_print/virtual_driver/win/virtual_driver_common_resources.rc:47:
>> > IDS_GCP_URL                "http://www.google.com/cloudprint"
>> > I am not sure why these need to be in a resource file if we are not
>> > translating them.
>> >
>> >
>> See above.
>>
>
>
>  >
>> >
>>
>
>
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
>
>> > File cloud_print/virtual_driver/win/virtual_driver_helpers.h (right):
>> >
>> >
>> >
>>
>
>
>
http://codereview.chromium.org/6930019/diff/11001/cloud_print/virtual_driver/...
>
>> > cloud_print/virtual_driver/win/virtual_driver_helpers.h:35: void
>> > LoadLocalString(DWORD string_id, wchar_t* target, DWORD target_size);
>> > This should take an std::wstring* as out argument. Don't we already have
>> > methods in base to load a string from resources?
>> >
>> > Most places I'm using it immediately hand the string off to a Windows
>> API
>> call that expects a wchar_t array.  I could make it use a wstring or
>> string16 but that would involve either a lot of const casting or copying
>> to
>> a wchar_t array.  It seemed saner this way.
>>
>
>  There ARE functions for loading from a resource but they all assume that
>> each language will be pulled from it's own DLL.  That doesn't really make
>> sense for something like this.  The native LoadString function does
>> exactly
>> what I need.
>>
>
> Shielding callers from having to worry about buffer sizes is a win. The
> method
> itself should return a string16. The method can compute the required length
> using LoadString.
>
>
>
>
>
>
>  >
>> > http://codereview.chromium.org/6930019/
>> >
>>
>
>
>
> http://codereview.chromium.org/6930019/
>

[sanjeevr, 2011-05-10 16:18:47]

One final round of comments.

http://codereview.chromium.org/6930019/diff/16001/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/win/virtual_driver_helpers.cc (right):

http://codereview.chromium.org/6930019/diff/16001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/win/virtual_driver_helpers.cc:85: DCHECK_NE(0,
count);
Can you change the DCHECK to a CHECK? And also write a comment here saying we
never expect the strings to be longer.

http://codereview.chromium.org/6930019/diff/16001/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/win/virtual_driver_helpers.h (right):

http://codereview.chromium.org/6930019/diff/16001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/win/virtual_driver_helpers.h:28: void
GetPortMonitorDllName(FilePath* path);
Since this is just a name, I think you should return an std::string. Also
returning it might make the calling code more readable than using an out
argument.

http://codereview.chromium.org/6930019/diff/16001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/win/virtual_driver_helpers.h:35: void
LoadLocalString(DWORD string_id, string16* target);
Again, you could probably return the target instead of using an out argument.

[sanjeevr, 2011-05-10 23:39:28]

LGTM

[sanjeevr, 2011-05-10 23:45:59]

http://codereview.chromium.org/6930019/diff/22001/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/win/install/setup.cc (right):

http://codereview.chromium.org/6930019/diff/22001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/win/install/setup.cc:212: HRESULT result = S_OK;
This seems to be a typo.

[Albert Bodenhamer, 2011-05-11 21:46:12]

Forgot to mail last comments.

http://codereview.chromium.org/6930019/diff/16001/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/win/virtual_driver_helpers.cc (right):

http://codereview.chromium.org/6930019/diff/16001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/win/virtual_driver_helpers.cc:85: DCHECK_NE(0,
count);
On 2011/05/10 16:18:47, sanjeevr wrote:
> Can you change the DCHECK to a CHECK? And also write a comment here saying we
> never expect the strings to be longer.

Done.

http://codereview.chromium.org/6930019/diff/16001/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/win/virtual_driver_helpers.h (right):

http://codereview.chromium.org/6930019/diff/16001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/win/virtual_driver_helpers.h:28: void
GetPortMonitorDllName(FilePath* path);
On 2011/05/10 16:18:47, sanjeevr wrote:
> Since this is just a name, I think you should return an std::string. Also
> returning it might make the calling code more readable than using an out
> argument.

Done.

http://codereview.chromium.org/6930019/diff/16001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/win/virtual_driver_helpers.h:35: void
LoadLocalString(DWORD string_id, string16* target);
On 2011/05/10 16:18:47, sanjeevr wrote:
> Again, you could probably return the target instead of using an out argument.

Done.