Make CPDF_Object::GetString() a virtual method.

Make CPDF_Object::GetString() a virtual method.

[Lei Zhang, 2015-06-22 02:53:38]

Patchset #1 (id:1) has been deleted

[Lei Zhang, 2015-07-16 17:13:38]

Patchset #2 (id:40001) has been deleted

[Lei Zhang, 2015-07-16 17:22:18]

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

[Tom Sepez, 2015-07-16 20:24:46]

Swell.

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

https://codereview.chromium.org/1194933003/diff/60001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_objects.h:74: FX_FLOAT                           
GetNumber() const;
There are a whole bunch of these that need to be virtualized.  Are you sure you
want to jump off this cliff right now?

https://codereview.chromium.org/1194933003/diff/60001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_objects.h:96: virtual ~CPDF_Object() {}
Right, now that we are paying the price for a vtable this is OK, but why is it
still protected?

https://codereview.chromium.org/1194933003/diff/60001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_objects.h:97: void                               
Destroy();
Remove Destroy() now that we have virtual destructors and update its callers.

https://codereview.chromium.org/1194933003/diff/60001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_objects.h:204: CFX_ByteString GetString() const
override;
not sure why this needs to be protected, either.

https://codereview.chromium.org/1194933003/diff/60001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_objects.h:251: CFX_ByteString GetString() const
override;
ditto

https://codereview.chromium.org/1194933003/diff/60001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_objects.h:293: CFX_ByteString GetString() const
override;
ditto

https://codereview.chromium.org/1194933003/diff/60001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_objects.h:328: CFX_ByteString GetStringAt(FX_DWORD
index) const;
I presume this is the overriding overloaded methods gotcha?  Either way, its a
better name.

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

https://codereview.chromium.org/1194933003/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:419: return
((CPDF_Object*)m_Objects.GetAt(i))->GetDirect();
Given that m_Objects is of type CFX_PtrArray, and that CFX_PtrArray is
a typedef for CFX_ArrayTemplate<void*>, might it make sense to make 
m_Objects be CFX_ArrayTemplate<CPDF_Object*>, and lose all these casts?  Your
call as to now, later, never.

https://codereview.chromium.org/1194933003/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:424: CPDF_Object* p =
(CPDF_Object*)m_Objects.GetAt(i);
nit: I'd really like this block to be a one-liner, once the cast is gone, of the
form:
  return m_Objects.GetAt(i)->GetString();

https://codereview.chromium.org/1194933003/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:605: else
nit: else after return.

https://codereview.chromium.org/1194933003/diff/60001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1194933003/diff/60001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:954: if
(pArray->GetStringAt(count) == "DCTDecode") {
nit: is this |else if|?

[Tom Sepez, 2015-07-17 16:03:00]

At the risk of "piling on", these all could have trivial unit tests, so you can
be sure the refactoring didn't miss a case to method transformation (/ducks).