CFX_MapByteStringToPtr considered harmful.

CFX_MapByteStringToPtr considered harmful.

R=thestig@chromium.org

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

[Tom Sepez, 2015-08-14 20:36:15]

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

[Tom Sepez, 2015-08-14 20:36:16]

Lei, this will take some inspection :(.

[Lei Zhang, 2015-08-14 23:59:35]

Ya, this will take a while. I'm going through them...

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/global.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:184: if (!pData || pData->bDeleted) {
This is intentially calling vp.SetNull() for the bDeleted = true case, right?

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:184: if (!pData || pData->bDeleted) {
|pData| is never NULL.

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:227: if (pData && !pData->bDeleted) {
|pData| is never NULL.

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:298: if (pData) {
|pData| is never NULL.

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:439: auto it = m_mapGlobal.begin();
Can't this be written as follows?

for (auto it : m_mapGlobal)
  delete it->second;
m_mapGlobal.clear()

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:458: if (it != m_mapGlobal.end() &&
it->second) {
|it->second| is never NULL.

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:525: m_mapGlobal[propname] = pNewData;
Since this is the only setter, and |pNewData| cannot be null, we can get rid of
all the checks above.

[Lei Zhang, 2015-08-15 00:35:44]

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/font_int.h (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/font_int.h:8: 
I feel like we need some #includes here.

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:26: while (it != m_CMaps.end()) {
Range-Based For Loop?

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:70: if (CPDF_CMap* pCMap =
it->second) {
This is always true.

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render_text.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_text.cpp:15: while (it !=
m_SizeMap.end()) {
Range-Based For Loop?

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxge/ge/fx_ge_...
File core/src/fxge/ge/fx_ge_linux.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_linux.cpp:205: for (auto it = m_FontList.begin(); it !=
m_FontList.end(); ++it) {
Range-Based For Loop?

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_linux.cpp:206: CFX_ByteString bsName = it->first;
const ref?

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxge/ge/fx_ge_...
File core/src/fxge/ge/fx_ge_text.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_text.cpp:1236: while (it != m_SizeMap.end()) {
Range-Based For Loop?

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/include/fsdk_an...
File fpdfsdk/include/fsdk_annothandler.h (right):

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/include/fsdk_an...
fpdfsdk/include/fsdk_annothandler.h:320: CBA_AnnotHandlerArray m_Handlers;
Can we just get rid of |m_Handlers| and give ownership of the handlers to
|m_mapType2Handler|?

[Lei Zhang, 2015-08-15 00:47:33]

Maybe we should document the maps to indicate whether the is pointers are never
NULL. WDYT?

https://codereview.chromium.org/1297723002/diff/40001/core/include/fxcrt/fx_b...
File core/include/fxcrt/fx_basic.h (left):

https://codereview.chromium.org/1297723002/diff/40001/core/include/fxcrt/fx_b...
core/include/fxcrt/fx_basic.h:762: void AddValue(const CFX_ByteStringC& key,
void* pValue);
BTW, this is the trickier map class.

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec_icc.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_icc.cpp:529: while (profile_it !=
m_MapProfile.end()) {
Range-Based For Loops here and below?

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxge/ge/fx_ge_...
File core/src/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_fontmap.cpp:102: while (it != m_FaceMap.end()) {
Range-Based For Loop?

[Tom Sepez, 2015-08-17 20:15:26]

https://codereview.chromium.org/1297723002/diff/40001/core/include/fxcrt/fx_b...
File core/include/fxcrt/fx_basic.h (left):

https://codereview.chromium.org/1297723002/diff/40001/core/include/fxcrt/fx_b...
core/include/fxcrt/fx_basic.h:762: void AddValue(const CFX_ByteStringC& key,
void* pValue);
On 2015/08/15 00:47:33, Lei Zhang wrote:
> BTW, this is the trickier map class.

Yup.  It ripples through Dictionary objects.

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/font_int.h (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/font_int.h:8: 
On 2015/08/15 00:35:44, Lei Zhang wrote:
> I feel like we need some #includes here.

Done. Header now compiles on it own.

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:26: while (it != m_CMaps.end()) {
On 2015/08/15 00:35:44, Lei Zhang wrote:
> Range-Based For Loop?
Done; you're right its probably faster to just clear it at the end despite the
icky feeling of temporarily leaving dangling pointers in the map.

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:70: if (CPDF_CMap* pCMap =
it->second) {
On 2015/08/15 00:35:44, Lei Zhang wrote:
> This is always true.

Done.

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render_text.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_text.cpp:15: while (it !=
m_SizeMap.end()) {
On 2015/08/15 00:35:44, Lei Zhang wrote:
> Range-Based For Loop?

Done.

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec_icc.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_icc.cpp:529: while (profile_it !=
m_MapProfile.end()) {
On 2015/08/15 00:47:33, Lei Zhang wrote:
> Range-Based For Loops here and below?

Done.

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxge/ge/fx_ge_...
File core/src/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_fontmap.cpp:102: while (it != m_FaceMap.end()) {
On 2015/08/15 00:47:33, Lei Zhang wrote:
> Range-Based For Loop?

Done.

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxge/ge/fx_ge_...
File core/src/fxge/ge/fx_ge_linux.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_linux.cpp:205: for (auto it = m_FontList.begin(); it !=
m_FontList.end(); ++it) {
On 2015/08/15 00:35:44, Lei Zhang wrote:
> Range-Based For Loop?

Done.

https://codereview.chromium.org/1297723002/diff/40001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_linux.cpp:206: CFX_ByteString bsName = it->first;
On 2015/08/15 00:35:44, Lei Zhang wrote:
> const ref?
Why not.

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/include/fsdk_an...
File fpdfsdk/include/fsdk_annothandler.h (right):

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/include/fsdk_an...
fpdfsdk/include/fsdk_annothandler.h:320: CBA_AnnotHandlerArray m_Handlers;
On 2015/08/15 00:35:44, Lei Zhang wrote:
> Can we just get rid of |m_Handlers| and give ownership of the handlers to
> |m_mapType2Handler|?
Follow-up.

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/global.cpp (right):

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:184: if (!pData || pData->bDeleted) {
On 2015/08/14 23:59:35, Lei Zhang wrote:
> This is intentially calling vp.SetNull() for the bDeleted = true case, right?
Yes, seems like a bug to return with this unset.

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:184: if (!pData || pData->bDeleted) {
On 2015/08/14 23:59:35, Lei Zhang wrote:
> |pData| is never NULL.

Done.

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:227: if (pData && !pData->bDeleted) {
On 2015/08/14 23:59:35, Lei Zhang wrote:
> |pData| is never NULL.

Done.

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:298: if (pData) {
On 2015/08/14 23:59:35, Lei Zhang wrote:
> |pData| is never NULL.

Done.

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:439: auto it = m_mapGlobal.begin();
On 2015/08/14 23:59:35, Lei Zhang wrote:
> Can't this be written as follows?
> 
> for (auto it : m_mapGlobal)
>   delete it->second;
> m_mapGlobal.clear()

Done.

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:458: if (it != m_mapGlobal.end() &&
it->second) {
On 2015/08/14 23:59:35, Lei Zhang wrote:
> |it->second| is never NULL.

Done.

https://codereview.chromium.org/1297723002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/global.cpp:525: m_mapGlobal[propname] = pNewData;
On 2015/08/14 23:59:35, Lei Zhang wrote:
> Since this is the only setter, and |pNewData| cannot be null, we can get rid
of
> all the checks above.
Presume covered by the |pdata| can't be null comments above.

[Tom Sepez, 2015-08-17 20:20:18]

https://codereview.chromium.org/1297723002/diff/60001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/1297723002/diff/60001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:25: for (auto& pair : m_CMaps) {
nit: Should these be const auto& ? We're not mucking with the pointer itself ...

[Lei Zhang, 2015-08-17 22:05:03]

lgtm

https://codereview.chromium.org/1297723002/diff/60001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/1297723002/diff/60001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:25: for (auto& pair : m_CMaps) {
On 2015/08/17 20:20:18, Tom Sepez wrote:
> nit: Should these be const auto& ? We're not mucking with the pointer itself
...

If they can, then why not?

[Tom Sepez, 2015-08-17 23:20:39]

https://codereview.chromium.org/1297723002/diff/60001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/1297723002/diff/60001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:67: for (auto it = m_CMaps.begin();
it != m_CMaps.end(); ++it) {
note: made this range-based loop.

[Tom Sepez, 2015-08-17 23:26:07]

Committed patchset #5 (id:80001) manually as
ce4ffb8183af3fa2bb5133f0f7370a88e064c516 (presubmit successful).

[Lei Zhang, 2015-08-17 23:30:25]

Looks like we have more work in fx_mac_imp.cpp. Too bad we don't have try bots.