Get rid of FX_LPCSTR cast.

Get rid of FX_LPCSTR cast.

Follow up on https://codereview.chromium.org/733693003

R=brucedawson@chromium.org, tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/659a56bcbeb12b22708fe979d12b318080d828ba

[Bo Xu, 2014-12-30 22:30:37]

bo_xu@foxitsoftware.com changed reviewers:
+ brucedawson@chromium.org, tsepez@chromium.org

[Bo Xu, 2014-12-30 22:30:38]

Bruce, Tom, please take a look at this one.

[Tom Sepez, 2014-12-30 22:53:26]

I like it. LGTM.  

What happens when you remove the deprecated cast operator?

https://codereview.chromium.org/809993004/diff/1/core/include/fxcrt/fx_string.h
File core/include/fxcrt/fx_string.h (right):

https://codereview.chromium.org/809993004/diff/1/core/include/fxcrt/fx_string...
core/include/fxcrt/fx_string.h:199: 
nit: extra blank line.

https://codereview.chromium.org/809993004/diff/1/core/src/fpdfapi/fpdf_parser...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/809993004/diff/1/core/src/fpdfapi/fpdf_parser...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2197:
pDict->SetAt(CFX_ByteStringC((key.c_str()) + 1, key.GetLength() - 1), pObj);
nit: extra parens?

https://codereview.chromium.org/809993004/diff/1/core/src/fpdfapi/fpdf_parser...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2199:
pDict->AddValue(CFX_ByteStringC((key.c_str()) + 1, key.GetLength() - 1), pObj);
nit: ditto

https://codereview.chromium.org/809993004/diff/1/core/src/fpdfapi/fpdf_parser...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2383:
pDict->SetAt(CFX_ByteStringC((key.c_str()) + 1, key.GetLength() - 1), pObj);
nit: ditt

https://codereview.chromium.org/809993004/diff/1/core/src/fpdfapi/fpdf_parser...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2385:
pDict->AddValue(CFX_ByteStringC((key.c_str()) + 1, key.GetLength() - 1), pObj);
nit: ditto

https://codereview.chromium.org/809993004/diff/1/core/src/fxcrt/fx_basic_buff...
File core/src/fxcrt/fx_basic_buffer.cpp (right):

https://codereview.chromium.org/809993004/diff/1/core/src/fxcrt/fx_basic_buff...
core/src/fxcrt/fx_basic_buffer.cpp:374: str = CFX_WideString::FromUTF16LE((const
unsigned short*)encoded.c_str(), encoded.GetLength());
nit: This cast is confusing, but let's leave it for now.

https://codereview.chromium.org/809993004/diff/1/core/src/fxge/win32/fx_win32...
File core/src/fxge/win32/fx_win32_device.cpp (right):

https://codereview.chromium.org/809993004/diff/1/core/src/fxge/win32/fx_win32...
core/src/fxge/win32/fx_win32_device.cpp:486:
((BITMAPINFOHEADER*)info.c_str())->biHeight *= -1;
nit: this cast is confusing, too, but leave it.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/javascript/JS_Va...
File fpdfsdk/src/javascript/JS_Value.cpp (right):

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/javascript/JS_Va...
fpdfsdk/src/javascript/JS_Value.cpp:403: CJS_Value::operator = (string.c_str());
nit: indent.

[brucedawson, 2014-12-30 23:13:02]

There are a bunch of places where the review tool displays the code oddly,
apparently due to spaces being used instead of tabs on some lines. If we're
going to switch to all spaces soon then this can be ignored, but otherwise it
should be kept consistent with the surrounding code.

My only other concern is on the two lines of code in util.cpp that go:

std::string cSource = CFX_ByteString::FromUnicode(sSource).c_str();

The double conversions seem inelegant, and should be avoided or at least hidden.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/fpdf_flatten.cpp
File fpdfsdk/src/fpdf_flatten.cpp (right):

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/fpdf_flatten.cpp...
fpdfsdk/src/fpdf_flatten.cpp:223: sStream.Format("q 1 0 0 1 0 0 cm /%s Do Q",
key.c_str());
What happened to the indenting here? Tabs versus spaces? The inconsistency makes
it misaligned in the review tool.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/fpdf_flatten.cpp...
fpdfsdk/src/fpdf_flatten.cpp:270: sStream.Format("q 1 0 0 1 0 0 cm /%s Do Q",
key.c_str());
Ditto on the indenting.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/fpdf_flatten.cpp...
fpdfsdk/src/fpdf_flatten.cpp:550: sTemp.Format("q %f 0 0 %f %f %f cm /%s Do
Q\n", m.a, m.d, m.e, m.f, sFormName.c_str());
Ditto on the indenting.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/fpdfdoc.cpp
File fpdfsdk/src/fpdfdoc.cpp (right):

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/fpdfdoc.cpp#newc...
fpdfsdk/src/fpdfdoc.cpp:110: FXSYS_memcpy(buffer, path.c_str(), len);
Ditto on the indenting.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/fpdfdoc.cpp#newc...
fpdfsdk/src/fpdfdoc.cpp:254: FXSYS_memcpy(buffer, bstr.c_str(), len);
Ditto on the indenting.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/fsdk_baseannot.cpp
File fpdfsdk/src/fsdk_baseannot.cpp (right):

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/fsdk_baseannot.c...
fpdfsdk/src/fsdk_baseannot.cpp:1065:
pStream->SetData((FX_BYTE*)sContents.c_str(), sContents.GetLength(), FALSE,
FALSE);
Ditto on the indenting.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/fsdk_baseform.cpp
File fpdfsdk/src/fsdk_baseform.cpp (right):

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/fsdk_baseform.cp...
fpdfsdk/src/fsdk_baseform.cpp:1919:
pRetStream->InitStream((FX_BYTE*)csStream.c_str(), csStream.GetLength(),
pStreamDict);
Ditto on the indenting.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/javascript/Publi...
File fpdfsdk/src/javascript/PublicMethods.cpp (right):

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/javascript/Publi...
fpdfsdk/src/javascript/PublicMethods.cpp:446: StrArray.SetElement(nIndex,
CJS_Value(isolate, StrTrim(p).c_str()));
Ditto on the indenting, times four in this file.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/javascript/util.cpp
File fpdfsdk/src/javascript/util.cpp (right):

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/javascript/util....
fpdfsdk/src/javascript/util.cpp:475: std::string cSource =
CFX_ByteString::FromUnicode(sSource).c_str();
Can we get a version of FromUnicode that returns std::string? Going from
CFX_WideString to CFX_ByteString to std::string seems like an excess of
conversions. Adding a FromUnicodeTostdstring function would at least centralize
the overhead.

Also, ditto on the indenting.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cpp
File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cp...
fpdfsdk/src/jsapi/fxjs_v8.cpp:254: v8::Handle<v8::String> objName =
v8::String::NewFromUtf8(isolate, bs.c_str(), v8::String::kNormalString,
bs.GetLength());
Indent, times four in this file.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/pdfwindow/PWL_Ed...
File fpdfsdk/src/pdfwindow/PWL_Edit.cpp (right):

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/pdfwindow/PWL_Ed...
fpdfsdk/src/pdfwindow/PWL_Edit.cpp:47: if (CXML_Element * pXML =
CXML_Element::Parse(sValue.c_str(), sValue.GetLength()))
Indent.

[Bo Xu, 2014-12-31 00:25:24]

@Tom, remove the deprecated one resulting in a lot of errors, most of them due
to the hidden cast in some function parameters. I would resolve them in a
separate CL.

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/javascript/util.cpp
File fpdfsdk/src/javascript/util.cpp (right):

https://codereview.chromium.org/809993004/diff/1/fpdfsdk/src/javascript/util....
fpdfsdk/src/javascript/util.cpp:475: std::string cSource =
CFX_ByteString::FromUnicode(sSource).c_str();
On 2014/12/30 23:13:02, brucedawson wrote:
> Can we get a version of FromUnicode that returns std::string? Going from
> CFX_WideString to CFX_ByteString to std::string seems like an excess of
> conversions. Adding a FromUnicodeTostdstring function would at least
centralize
> the overhead.
> 
> Also, ditto on the indenting.

I agree there are too many conversion here.
CFX_ByteString and CFX_WideString are some fundamental data structures. They
were used before in Foxit code to avoid stl(which is not good and in many places
we want to replace with stl). But I feel return a std:string from these data
types are kinda breaking the existing structure.
Looking at the extensive used std:string here is astonishing as they are rarely
mixed with those self-defined data types. Maybe we just leave this one as it is
for now. We could have a though cleanup someday.

[brucedawson, 2014-12-31 00:30:25]

Okay, sounds good about leaving the extra conversion for now. Fixing the
indenting still seems worthwhile.

lgtm

[Bo Xu, 2014-12-31 00:37:32]

On 2014/12/31 00:30:25, brucedawson wrote:
> Okay, sounds good about leaving the extra conversion for now. Fixing the
> indenting still seems worthwhile.
> 
> lgtm

Thanks. I fixed them in patch 2. Massively formatting all the code, are waiting
for XFA to be in master branch, and seems desirable :)

[Bo Xu, 2014-12-31 00:56:19]

Committed patchset #2 (id:20001) manually as
659a56bcbeb12b22708fe979d12b318080d828ba (presubmit successful).