Getting rid of more (FX_LPCWSTR) casts and fixing two bugs revealed by this.

Getting rid of more (FX_LPCWSTR) casts and fixing two bugs revealed by this.

Since casts to FX_LPCWSTR have been shown to hide bugs I tried removing
more of them, targeting those places where a cast was used to force a
conversion from CFX_WideString to FX_LPCWSTR, replacing these casts with
calls to the newly added .c_str() function. This revealed two places
where the cast was hiding a bug -- where ->c_str() was required instead!

This removes ~33 FX_LPCWSTR casts and there are ~31 left, many of which
will go away in some future change.

Also includes this change:

Removing unnecessary casts from wchar_t* to wchar_t*, by various names.

Original patch from Bruce Dawson(brucedawson@chromium.org)

R=bo_xu@foxitsoftware.com, tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/b69da0b96ffdad124efd1b48d51c617bb216a98e

[Tom Sepez, 2014-11-17 18:25:23]

tsepez@chromium.org changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2014-11-17 18:25:23]

Can you rebase this once your prior CL lands?  It looks to be doing the same
changes twice.

[brucedawson, 2014-11-17 18:30:39]

On 2014/11/17 18:25:23, Tom Sepez wrote:
> Can you rebase this once your prior CL lands?  It looks to be doing the same
> changes twice.

Yep, that was the intent. Either this change can be landed (and 730993002
discarded) or I'll rebase this one as soon as 730993002 lands. This change
contains everything because having it include everything was the cleanest way to
make sure I found all the issues.

[Tom Sepez, 2014-11-17 18:31:38]

https://codereview.chromium.org/733693003/diff/20001/core/include/fxcrt/fx_st...
File core/include/fxcrt/fx_string.h (right):

https://codereview.chromium.org/733693003/diff/20001/core/include/fxcrt/fx_st...
core/include/fxcrt/fx_string.h:649: // Implicit conversion -- potentially risky
Can we remove this and fix the places where it fails? Or was that in line for a
follow-on CL?

https://codereview.chromium.org/733693003/diff/20001/core/include/fxcrt/fx_st...
core/include/fxcrt/fx_string.h:650: operator FX_LPCWSTR() const
Also, maybe we want to mark this cast operator as explicit to prevent it from
being applied without a cast?

[Tom Sepez, 2014-11-17 18:33:01]

lgtm

[Tom Sepez, 2014-11-17 18:34:38]

I think its better as two changes.  The first one is essentially cleanup and is
unlikely to need to be rolled back. This one makes some semantic changes. It
would be a shame to lose the first one if this turned out to break something. 
Thanks.

[brucedawson, 2014-11-17 18:41:55]

I commented on the comments.

https://codereview.chromium.org/733693003/diff/20001/core/include/fxcrt/fx_st...
File core/include/fxcrt/fx_string.h (right):

https://codereview.chromium.org/733693003/diff/20001/core/include/fxcrt/fx_st...
core/include/fxcrt/fx_string.h:649: // Implicit conversion -- potentially risky
On 2014/11/17 18:31:37, Tom Sepez wrote:
> Can we remove this and fix the places where it fails? Or was that in line for
a
> follow-on CL?

Follow-on CL would be better. Simply removing the conversion operator causes 357
errors (perhaps many more). The sanest way to fix them would be to add more
overrides to reduce the implicit conversion count -- not trivial.

https://codereview.chromium.org/733693003/diff/20001/core/include/fxcrt/fx_st...
core/include/fxcrt/fx_string.h:650: operator FX_LPCWSTR() const
On 2014/11/17 18:31:37, Tom Sepez wrote:
> Also, maybe we want to mark this cast operator as explicit to prevent it from
> being applied without a cast?

That leads to just as many errors and would then encourage adding casts, which
have been shown to hide errors, so I'd prefer not to take that route.

[Tom Sepez, 2014-11-17 18:43:15]

https://codereview.chromium.org/733693003/diff/40001/core/src/fxcrt/fx_xml_pa...
File core/src/fxcrt/fx_xml_parser.cpp (right):

https://codereview.chromium.org/733693003/diff/40001/core/src/fxcrt/fx_xml_pa...
core/src/fxcrt/fx_xml_parser.cpp:673: const CFX_WideString* pValue =
m_AttrMap.Lookup(bsSpace, bsName);
nit: some reason this doesn't just call into the method below it now that the
parameter has been split to match what is expected below?

[brucedawson, 2014-11-17 18:52:02]

https://codereview.chromium.org/733693003/diff/40001/core/src/fxcrt/fx_xml_pa...
File core/src/fxcrt/fx_xml_parser.cpp (right):

https://codereview.chromium.org/733693003/diff/40001/core/src/fxcrt/fx_xml_pa...
core/src/fxcrt/fx_xml_parser.cpp:673: const CFX_WideString* pValue =
m_AttrMap.Lookup(bsSpace, bsName);
On 2014/11/17 18:43:15, Tom Sepez wrote:
> nit: some reason this doesn't just call into the method below it now that the
> parameter has been split to match what is expected below?

Could do that, if there aren't any additional risks associated with making that
change.

Good also (I think) replace the assignment to attribute with:

    attribute = *pValue;

That seems like it is what is actually wanted, but I'm not certain.

[brucedawson, 2014-11-17 19:06:13]

> Good also (I think) replace the assignment to attribute with:
> 
>     attribute = *pValue;
> 
> That seems like it is what is actually wanted, but I'm not certain.

I meant to say "Could also (I think) replace...". You get the idea.

Also, I just tweaked the comments in fx_string.h. No code changes.

[Tom Sepez, 2014-11-17 19:14:02]

Still LGTM.

https://codereview.chromium.org/733693003/diff/60001/core/include/fxcrt/fx_st...
File core/include/fxcrt/fx_string.h (right):

https://codereview.chromium.org/733693003/diff/60001/core/include/fxcrt/fx_st...
core/include/fxcrt/fx_string.h:643: // Explicit conversion to raw string
nit: Maybe say "C-style wide string" rather than raw.

[brucedawson, 2014-11-17 19:34:14]

Comment updated per Tom's suggestion.

https://codereview.chromium.org/733693003/diff/60001/core/include/fxcrt/fx_st...
File core/include/fxcrt/fx_string.h (right):

https://codereview.chromium.org/733693003/diff/60001/core/include/fxcrt/fx_st...
core/include/fxcrt/fx_string.h:643: // Explicit conversion to raw string
On 2014/11/17 19:14:02, Tom Sepez wrote:
> nit: Maybe say "C-style wide string" rather than raw.

Done.

[Bo Xu, 2014-11-17 20:22:51]

On 2014/11/17 19:06:13, brucedawson wrote:
> > Good also (I think) replace the assignment to attribute with:
> > 
> >     attribute = *pValue;
> > 
> > That seems like it is what is actually wanted, but I'm not certain.
> 
> I meant to say "Could also (I think) replace...". You get the idea.
> 
> Also, I just tweaked the comments in fx_string.h. No code changes.

Seems that code was never used but this could have affected XFA. attribute =
*pValue sounds good to me. I can check the devs and see if they have specific
reason doing this...

[brucedawson, 2014-11-17 23:41:27]

On 2014/11/17 20:22:51, Bo Xu wrote:
> On 2014/11/17 19:06:13, brucedawson wrote:
> > > Good also (I think) replace the assignment to attribute with:
> > > 
> > >     attribute = *pValue;
> > > 
> > > That seems like it is what is actually wanted, but I'm not certain.
> > 
> > I meant to say "Could also (I think) replace...". You get the idea.
> > 
> > Also, I just tweaked the comments in fx_string.h. No code changes.
> 
> Seems that code was never used but this could have affected XFA. attribute =
> *pValue sounds good to me. I can check the devs and see if they have specific
> reason doing this...

I've rebased so the set of changes is smaller now.

Bo -- can you comment on whether I should further change fx_xml_parser.cpp?
Possible changes are to use *attribute = *pValue; and changing the first
CXML_Element::GetAttrValue to call the second in order to reduce code
duplication.

[Bo Xu, 2014-11-25 23:58:27]

On 2014/11/17 23:41:27, brucedawson wrote:
> On 2014/11/17 20:22:51, Bo Xu wrote:
> > On 2014/11/17 19:06:13, brucedawson wrote:
> > > > Good also (I think) replace the assignment to attribute with:
> > > > 
> > > >     attribute = *pValue;
> > > > 
> > > > That seems like it is what is actually wanted, but I'm not certain.
> > > 
> > > I meant to say "Could also (I think) replace...". You get the idea.
> > > 
> > > Also, I just tweaked the comments in fx_string.h. No code changes.
> > 
> > Seems that code was never used but this could have affected XFA. attribute =
> > *pValue sounds good to me. I can check the devs and see if they have
specific
> > reason doing this...
> 
> I've rebased so the set of changes is smaller now.
> 
> Bo -- can you comment on whether I should further change fx_xml_parser.cpp?
> Possible changes are to use *attribute = *pValue; and changing the first
> CXML_Element::GetAttrValue to call the second in order to reduce code
> duplication.

You meant attribute = *pValue?
I pinged the original devs but didn't get back from them. For me, I don't see
problem of the changes - the counter should do the right thing for the
assignment here.
And I agree, should just call the second GetAttrValue to simplify the code.

[brucedawson, 2014-11-26 23:51:48]

I updated fx_xml_parser.cpp to simplify the code. The first version of
GetAttrValue calls into the second version -- code resuse.
The second version of GetAttrValue does a simple assignment from *pValue -- code
simplification.

Take another look at fx_xml_parser.cpp?

[brucedawson, 2014-12-06 01:11:14]

On 2014/11/26 23:51:48, brucedawson wrote:
> I updated fx_xml_parser.cpp to simplify the code. The first version of
> GetAttrValue calls into the second version -- code resuse.
> The second version of GetAttrValue does a simple assignment from *pValue --
code
> simplification.
> 
> Take another look at fx_xml_parser.cpp?

Tom or Bo, is the change still good with the fx_xml_parser.cpp change?

[Bo Xu, 2014-12-08 19:32:26]

On 2014/12/06 01:11:14, brucedawson wrote:
> On 2014/11/26 23:51:48, brucedawson wrote:
> > I updated fx_xml_parser.cpp to simplify the code. The first version of
> > GetAttrValue calls into the second version -- code resuse.
> > The second version of GetAttrValue does a simple assignment from *pValue --
> code
> > simplification.
> > 
> > Take another look at fx_xml_parser.cpp?
> 
> Tom or Bo, is the change still good with the fx_xml_parser.cpp change?

I was notified by email, so just saw this. LGTM, thanks.

[brucedawson, 2014-12-08 21:10:07]

Committed patchset #7 (id:120001) manually as
b69da0b96ffdad124efd1b48d51c617bb216a98e (presubmit successful).