Use std::map as CPDF_Dictionary's underlying store.

Use std::map as CPDF_Dictionary's underlying store.

Replaces CFX_CMapByteStringToPtr. XFA still uses CFX_CMapByteStringToPtr
so it's not completely removed just yet.

Adds begin()/end() to CPDF_Dictionary and removes the
GetStartPos()/GetNextElement() functions to traverse the dictionary.
Callers are changed accordingly. AddValue() is also removed.

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

Committed: https://pdfium.googlesource.com/pdfium/+/14f39950451bb9c2a11fbc7173fd47367410f80f

[Oliver Chang, 2015-12-21 22:53:08]

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

[Oliver Chang, 2015-12-21 22:53:20]

Description was changed from

==========
Use std::map as CPDF_Dictionary's underlying store.

Replaces CFX_CMapByteStringToPtr. CFX_CMapByteStringToPtr is also
removed since CPDF_Dictionary was the only user of it.

Adds begin()/end() to CPDF_Dictionary and removes the
GetStartPos()/GetNextElement() functions to traverse the dictionary.
Callers are changed accordingly. AddValue() is also removed.
==========

to

==========
Use std::map as CPDF_Dictionary's underlying store.

Replaces CFX_CMapByteStringToPtr. CFX_CMapByteStringToPtr is also
removed since CPDF_Dictionary was the only user of it.

Adds begin()/end() to CPDF_Dictionary and removes the
GetStartPos()/GetNextElement() functions to traverse the dictionary.
Callers are changed accordingly. AddValue() is also removed.

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

[Oliver Chang, 2015-12-21 22:56:12]

Description was changed from

==========
Use std::map as CPDF_Dictionary's underlying store.

Replaces CFX_CMapByteStringToPtr. CFX_CMapByteStringToPtr is also
removed since CPDF_Dictionary was the only user of it.

Adds begin()/end() to CPDF_Dictionary and removes the
GetStartPos()/GetNextElement() functions to traverse the dictionary.
Callers are changed accordingly. AddValue() is also removed.

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

to

==========
Use std::map as CPDF_Dictionary's underlying store.

Replaces CFX_CMapByteStringToPtr. XFA still uses CFX_CMapByteStringToPtr
so it's not completely removed just yet.

Adds begin()/end() to CPDF_Dictionary and removes the
GetStartPos()/GetNextElement() functions to traverse the dictionary.
Callers are changed accordingly. AddValue() is also removed.

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

[Oliver Chang, 2015-12-21 23:00:00]

ptal, no rush. Depending on how much extra work needs to be done in XFA, I might
not land this for a while (after vacation).

One big caveat with this is that lookups incur the cost of a CFX_ByteString
construction (ie a string copy) as that's the key type used for the map.

I couldn't think of a clean way to get around this, any suggestions? Shame that
there's no chance that we'll be able to use C++14's new map::find any time
soon....

[Tom Sepez, 2016-01-05 19:41:56]

Last time I tried this, I had some test failures based on the ordering and
underlying stability guarantees of the FX representation.  Did all the tests
pass locally?

https://codereview.chromium.org/1541703003/diff/20001/core/include/fpdfapi/fp...
File core/include/fpdfapi/fpdf_objects.h (right):

https://codereview.chromium.org/1541703003/diff/20001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_objects.h:342: typedef std::map<CFX_ByteString,
CPDF_Object*>::iterator iterator;
nit: using

https://codereview.chromium.org/1541703003/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1541703003/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:261: for (const auto& it :
pThis->m_Map) {
nit: (const auto& it : *pThis)  ?

[Oliver Chang, 2016-01-07 16:04:25]

that's strange, i'm not seeing any tests fail locally. do you remember what
those guarantees were?

https://codereview.chromium.org/1541703003/diff/20001/core/include/fpdfapi/fp...
File core/include/fpdfapi/fpdf_objects.h (right):

https://codereview.chromium.org/1541703003/diff/20001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_objects.h:342: typedef std::map<CFX_ByteString,
CPDF_Object*>::iterator iterator;
On 2016/01/05 at 19:41:55, Tom Sepez wrote:
> nit: using

Done.

https://codereview.chromium.org/1541703003/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1541703003/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:261: for (const auto& it :
pThis->m_Map) {
On 2016/01/05 at 19:41:56, Tom Sepez wrote:
> nit: (const auto& it : *pThis)  ?

Done.

[Charles Lew, 2016-01-07 16:44:49]

https://codereview.chromium.org/1541703003/diff/40001/core/include/fpdfapi/fp...
File core/include/fpdfapi/fpdf_objects.h (right):

https://codereview.chromium.org/1541703003/diff/40001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_objects.h:437: std::map<CFX_ByteString, CPDF_Object*>
m_Map;
Not sure if it is a good idea, but is it possible to involve a "Symbol" type as
the key here, thus reducing the string comparison into a simple symbol identity
comparison?

[Tom Sepez, 2016-01-07 17:00:22]

On 2016/01/07 16:04:25, Oliver Chang wrote:
> that's strange, i'm not seeing any tests fail locally. do you remember what
> those guarantees were?
> 
>
https://codereview.chromium.org/1541703003/diff/20001/core/include/fpdfapi/fp...
> File core/include/fpdfapi/fpdf_objects.h (right):
> 
>
https://codereview.chromium.org/1541703003/diff/20001/core/include/fpdfapi/fp...
> core/include/fpdfapi/fpdf_objects.h:342: typedef std::map<CFX_ByteString,
> CPDF_Object*>::iterator iterator;
> On 2016/01/05 at 19:41:55, Tom Sepez wrote:
> > nit: using
> 
> Done.
> 
>
https://codereview.chromium.org/1541703003/diff/20001/core/src/fpdfapi/fpdf_p...
> File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):
> 
>
https://codereview.chromium.org/1541703003/diff/20001/core/src/fpdfapi/fpdf_p...
> core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:261: for (const auto& it
:
> pThis->m_Map) {
> On 2016/01/05 at 19:41:56, Tom Sepez wrote:
> > nit: (const auto& it : *pThis)  ?
> 
> Done.


See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pdfium...
-- I think iFPDF_GetNamedDest() that returns an index to the caller of the nth
named dest in the dictionary.

[Tom Sepez, 2016-01-07 21:43:21]

>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pdfium...
> -- I think iFPDF_GetNamedDest() that returns an index to the caller of the nth
> named dest in the dictionary.

I had thought this would be a concern because either javascript or the fpdfsdk
might have the ability to create a new named dest after document parse time, and
that the index would not be stable.  But a web search says that JS can't do
this, and I didn't see an fpdfsdk API for doing the same. Chrome doesn't call
this, so it will be OK; I was worrying about other users.

[Tom Sepez, 2016-01-07 21:45:16]

https://codereview.chromium.org/1541703003/diff/40001/core/include/fpdfapi/fp...
File core/include/fpdfapi/fpdf_objects.h (right):

https://codereview.chromium.org/1541703003/diff/40001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_objects.h:437: std::map<CFX_ByteString, CPDF_Object*>
m_Map;
On 2016/01/07 16:44:49, crlf0710 wrote:
> Not sure if it is a good idea, but is it possible to involve a "Symbol" type
as
> the key here, thus reducing the string comparison into a simple symbol
identity
> comparison?
We don't have the notion of an 'atomic string' (as in blink) in pdfium.

[Oliver Chang, 2016-01-07 23:01:33]

On 2016/01/07 21:43:21, Tom Sepez wrote:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pdfium...
> > -- I think iFPDF_GetNamedDest() that returns an index to the caller of the
nth
> > named dest in the dictionary.
> 
> I had thought this would be a concern because either javascript or the fpdfsdk
> might have the ability to create a new named dest after document parse time,
and
> that the index would not be stable.  But a web search says that JS can't do
> this, and I didn't see an fpdfsdk API for doing the same. Chrome doesn't call
> this, so it will be OK; I was worrying about other users.

Ah, I misunderstood. I thought you meant that the ordering expectation was
stronger: that iterating the key/value pairs gives the same order as it appears
in the document too.

To be safe, we can probably preserve insertion order by making m_Map's value be
an iterator to a std::list? (what to do when setting a value whose key already
exists?) The CPDF_Dictionary iterator would just be typedefed to the std::list
iterator.

[Tom Sepez, 2016-01-07 23:05:23]

> To be safe, we can probably preserve insertion order by making m_Map's value
be
> an iterator to a std::list? (what to do when setting a value whose key already
> exists?) The CPDF_Dictionary iterator would just be typedefed to the std::list
> iterator.

Nah, I've convinced myself we're good. The map can't change, and since it is an
ordered map, we'll get the same sequence every time.  I'd expect one of the
embedder tests needs updating to whatever order we now produce.

[Tom Sepez, 2016-01-08 00:33:52]

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_e...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp (right):

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:1780: if (key == "Encrypt" ||
key == "Size" || key == "Filter" ||
NB: similar to line 184, but missing "Type".  Wonder if thats a bug.  Either
way, consolidate the others in a helper function someday.

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:1795: if
(m_File.AppendDWord(pValue->GetObjNum()) < 0) {
NB: also a helper method to append  "objnum 0 R" - look a all the " 0 R"
literals.

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp (right):

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:1216: CPDF_ColorSpace* pRet =
nullptr;
nit: this could go inside the loop.

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:690: return m_Map.find(key)
!= m_Map.end();
nit: can use pdfium::ContainsKey() helper function.

[Oliver Chang, 2016-01-08 23:14:58]

Tried this patch on Windows and things seem to work. XFA doesn't look like too
much more work from a quick glance.

Only changes in latest patchset are in 	fpdfsdk/src/fpdfedit_embeddertest.cpp,
and the changes for the inlined comments.

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_e...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp (right):

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:1780: if (key == "Encrypt" ||
key == "Size" || key == "Filter" ||
On 2016/01/08 00:33:52, Tom Sepez wrote:
> NB: similar to line 184, but missing "Type".  Wonder if thats a bug.  Either
> way, consolidate the others in a helper function someday.

ack. added a TODO.

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:1795: if
(m_File.AppendDWord(pValue->GetObjNum()) < 0) {
On 2016/01/08 00:33:52, Tom Sepez wrote:
> NB: also a helper method to append  "objnum 0 R" - look a all the " 0 R"
> literals.

ack. added a TODO.

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp (right):

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp:1216: CPDF_ColorSpace* pRet =
nullptr;
On 2016/01/08 00:33:52, Tom Sepez wrote:
> nit: this could go inside the loop.

Done.

[Oliver Chang, 2016-01-08 23:15:42]

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1541703003/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:690: return m_Map.find(key)
!= m_Map.end();
On 2016/01/08 00:33:52, Tom Sepez wrote:
> nit: can use pdfium::ContainsKey() helper function.

Done.

[Tom Sepez, 2016-01-09 00:38:42]

lgtm

[Oliver Chang, 2016-01-11 16:30:23]

Description was changed from

==========
Use std::map as CPDF_Dictionary's underlying store.

Replaces CFX_CMapByteStringToPtr. XFA still uses CFX_CMapByteStringToPtr
so it's not completely removed just yet.

Adds begin()/end() to CPDF_Dictionary and removes the
GetStartPos()/GetNextElement() functions to traverse the dictionary.
Callers are changed accordingly. AddValue() is also removed.

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

to

==========
Use std::map as CPDF_Dictionary's underlying store.

Replaces CFX_CMapByteStringToPtr. XFA still uses CFX_CMapByteStringToPtr
so it's not completely removed just yet.

Adds begin()/end() to CPDF_Dictionary and removes the
GetStartPos()/GetNextElement() functions to traverse the dictionary.
Callers are changed accordingly. AddValue() is also removed.

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

Committed:
https://pdfium.googlesource.com/pdfium/+/14f39950451bb9c2a11fbc7173fd47367410...
==========

[Oliver Chang, 2016-01-11 16:30:24]

Committed patchset #4 (id:60001) manually as
14f39950451bb9c2a11fbc7173fd47367410f80f (presubmit successful).