Remove default argument from CPDF_Dictionary::SetAt().

Remove default argument from CPDF_Dictionary::SetAt().

Also fix a potential object leak.

R=tsepez@chromium.org

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

[Lei Zhang, 2015-10-24 00:40:32]

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

[Tom Sepez, 2015-10-24 16:50:02]

lgtm

https://codereview.chromium.org/1414963005/diff/1/core/include/fpdfapi/fpdf_o...
File core/include/fpdfapi/fpdf_objects.h (right):

https://codereview.chromium.org/1414963005/diff/1/core/include/fpdfapi/fpdf_o...
core/include/fpdfapi/fpdf_objects.h:409: void SetAt(const CFX_ByteStringC& key,
CPDF_Object* pObj);
rant: this should be the only form of these; the others combine insertion with
object creation; its a mistake to conflate those two ideas.

https://codereview.chromium.org/1414963005/diff/1/core/src/fpdfapi/fpdf_page/...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp (right):

https://codereview.chromium.org/1414963005/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:80:
nonstd::unique_ptr<CPDF_Object, ReleaseDeleter<CPDF_Object>> pObj(
Is there a point at even constructing an object if the key is empty?  I guess we
move the parser forward ...

https://codereview.chromium.org/1414963005/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1414963005/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:751: m_Map.Lookup(key,
(void*&)pExisting);
nit: you know this cast is my favorite pet peeve ...

[Lei Zhang, 2015-10-26 21:58:22]

Please review again.

https://codereview.chromium.org/1414963005/diff/1/core/include/fpdfapi/fpdf_o...
File core/include/fpdfapi/fpdf_objects.h (right):

https://codereview.chromium.org/1414963005/diff/1/core/include/fpdfapi/fpdf_o...
core/include/fpdfapi/fpdf_objects.h:409: void SetAt(const CFX_ByteStringC& key,
CPDF_Object* pObj);
On 2015/10/24 16:50:01, Tom Sepez wrote:
> rant: this should be the only form of these; the others combine insertion with
> object creation; its a mistake to conflate those two ideas.

Right. The caller can check if it's a reference and go through SetAtReference()
below.

https://codereview.chromium.org/1414963005/diff/1/core/src/fpdfapi/fpdf_page/...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp (right):

https://codereview.chromium.org/1414963005/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:80:
nonstd::unique_ptr<CPDF_Object, ReleaseDeleter<CPDF_Object>> pObj(
On 2015/10/24 16:50:01, Tom Sepez wrote:
> Is there a point at even constructing an object if the key is empty?  I guess
we
> move the parser forward ...

I guess that's the only point.

https://codereview.chromium.org/1414963005/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1414963005/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:751: m_Map.Lookup(key,
(void*&)pExisting);
On 2015/10/24 16:50:01, Tom Sepez wrote:
> nit: you know this cast is my favorite pet peeve ...

Done.

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

https://codereview.chromium.org/1414963005/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:99: if (pCSObj &&
!pCSObj->GetObjNum()) {
Since there's no object number, I would expect the clone to also have no object
number. Thus this should have been the simpler version of SetAt() all along.

I can't say anything about the lifetime of |pCSObj| other than it remains the
same as before.

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

https://codereview.chromium.org/1414963005/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:848: CPDF_Object* pElement =
This is a shadow variable. This function is a bit crazy and there's lots of
manual Release() calls all over. That'll have to wait for another day.

[Tom Sepez, 2015-10-26 22:14:00]

lgtm

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

https://codereview.chromium.org/1414963005/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:99: if (pCSObj &&
!pCSObj->GetObjNum()) {
On 2015/10/26 21:58:22, Lei Zhang wrote:
> Since there's no object number, I would expect the clone to also have no
object
> number. Thus this should have been the simpler version of SetAt() all along.
> 
> I can't say anything about the lifetime of |pCSObj| other than it remains the
> same as before.

Acknowledged.

[Lei Zhang, 2015-10-26 22:36:21]

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