Give names to the shading types

Give names to the shading types

Currently the shading types are referenced by number. This Cl creates and enum
and updates the code to use the enum names instead of magic numbers.

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/468974316ed5f6b6f8e637ab2c7afedc7c2bfe6a

[dsinclair, 2015-10-27 00:03:05]

Patchset #1 (id:1) has been deleted

[dsinclair, 2015-10-27 00:04:18]

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

[dsinclair, 2015-10-27 00:04:19]

PTAL.

[Tom Sepez, 2015-10-27 00:43:16]

https://codereview.chromium.org/1418623011/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1418623011/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:1130: if
(pShading->m_ShadingType >= kFreeFormGouraudTriangleMeshShading) {
Really dislike >= comparisions with enums, can we make a helper function that
tests if its one of the N cases that require this?

https://codereview.chromium.org/1418623011/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp (right):

https://codereview.chromium.org/1418623011/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:107: static ShadingType
ToShadingType(int type) {
Can we avoid this?

[dsinclair, 2015-10-27 02:11:30]

https://codereview.chromium.org/1418623011/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1418623011/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:1130: if
(pShading->m_ShadingType >= kFreeFormGouraudTriangleMeshShading) {
On 2015/10/27 00:43:16, Tom Sepez wrote:
> Really dislike >= comparisions with enums, can we make a helper function that
> tests if its one of the N cases that require this?

Done.

https://codereview.chromium.org/1418623011/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp (right):

https://codereview.chromium.org/1418623011/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:107: static ShadingType
ToShadingType(int type) {
On 2015/10/27 00:43:16, Tom Sepez wrote:
> Can we avoid this?

Done.

[Tom Sepez, 2015-10-27 16:20:17]

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

https://codereview.chromium.org/1418623011/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:109: ASSERT(st >
kInvalidShading && st < kMaxShading);
We need something stronger than an assert here, since the value in question
comes directly from the document, and we'd expect to see arbitrary values in the
wild. And < comparisions with ints is probably cleaner than with enums (e.g.
we'll never cast an out-of-range value to the enum type) so maybe:

return (type > static_cast<int>(kInvalidShading) &&
        type < static_cast<int>(kMaxShading)) ?
       static_cast<ShadingType>(type) : kInvalidShading;

https://codereview.chromium.org/1418623011/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:275: FX_BOOL bGouraud = type ==
kFreeFormGouraudTriangleMeshShading ||
nit: did clang-format do this?  sigh.  Maybe helper functions above with
switches e.g.

int PointCount(ShadingType type) {
  switch(type) {
    case kMumble:
      return 16;
    case kOther:
      return 12;
    default:
      return 1;
  }
}

https://codereview.chromium.org/1418623011/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:289: int point_count =
full_point_count, color_count = full_color_count;
nit: do we need these locals?

[dsinclair, 2015-10-27 17:00:37]

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

https://codereview.chromium.org/1418623011/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:109: ASSERT(st >
kInvalidShading && st < kMaxShading);
On 2015/10/27 16:20:16, Tom Sepez wrote:
> We need something stronger than an assert here, since the value in question
> comes directly from the document, and we'd expect to see arbitrary values in
the
> wild. And < comparisions with ints is probably cleaner than with enums (e.g.
> we'll never cast an out-of-range value to the enum type) so maybe:
> 
> return (type > static_cast<int>(kInvalidShading) &&
>         type < static_cast<int>(kMaxShading)) ?
>        static_cast<ShadingType>(type) : kInvalidShading;

Done.

https://codereview.chromium.org/1418623011/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:275: FX_BOOL bGouraud = type ==
kFreeFormGouraudTriangleMeshShading ||
On 2015/10/27 16:20:16, Tom Sepez wrote:
> nit: did clang-format do this?  sigh.  Maybe helper functions above with
> switches e.g.
> 
> int PointCount(ShadingType type) {
>   switch(type) {
>     case kMumble:
>       return 16;
>     case kOther:
>       return 12;
>     default:
>       return 1;
>   }
> }


How's this? I named the magic numbers and turned it into if()'s which I think
reads easier.

https://codereview.chromium.org/1418623011/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:289: int point_count =
full_point_count, color_count = full_color_count;
On 2015/10/27 16:20:16, Tom Sepez wrote:
> nit: do we need these locals?

Nope, did the math on the full_* versions and used those below.

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

https://codereview.chromium.org/1418623011/diff/60001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_resource.h:704: bool IsMeshShading() {
I moved this to a method on the ShadingPattern so I could access it from both
files.

[Tom Sepez, 2015-10-27 17:04:26]

LGTM with nit.

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

https://codereview.chromium.org/1418623011/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:275: FX_BOOL bGouraud = type ==
kFreeFormGouraudTriangleMeshShading ||
> How's this? 
swell.

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

https://codereview.chromium.org/1418623011/diff/60001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_resource.h:704: bool IsMeshShading() {
nit: const method.

[Tom Sepez, 2015-10-27 17:07:51]

https://codereview.chromium.org/1418623011/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp (right):

https://codereview.chromium.org/1418623011/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:267: static int
kSingleCoordinatePair = 1;
nit: const int.

[Tom Sepez, 2015-10-27 17:08:57]

https://codereview.chromium.org/1418623011/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp (right):

https://codereview.chromium.org/1418623011/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:107: static ShadingType
ToShadingType(int type) {
nit: maybe put this and your int's below into namespace { } (preferred over
static).

[dsinclair, 2015-10-27 17:15:17]

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

https://codereview.chromium.org/1418623011/diff/60001/core/include/fpdfapi/fp...
core/include/fpdfapi/fpdf_resource.h:704: bool IsMeshShading() {
On 2015/10/27 17:04:25, Tom Sepez wrote:
> nit: const method.

Done.

https://codereview.chromium.org/1418623011/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp (right):

https://codereview.chromium.org/1418623011/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:107: static ShadingType
ToShadingType(int type) {
On 2015/10/27 17:08:57, Tom Sepez wrote:
> nit: maybe put this and your int's below into namespace { } (preferred over
> static).

Done.

https://codereview.chromium.org/1418623011/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:267: static int
kSingleCoordinatePair = 1;
On 2015/10/27 17:07:51, Tom Sepez wrote:
> nit: const int.

Done.

https://codereview.chromium.org/1418623011/diff/80001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp (right):

https://codereview.chromium.org/1418623011/diff/80001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:873: return;
I added handling for the two extra types here, it seems like just returning is
the righting to do because we don't generate any pixels.

[Tom Sepez, 2015-10-27 17:17:13]

++lgtm

https://codereview.chromium.org/1418623011/diff/80001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp (right):

https://codereview.chromium.org/1418623011/diff/80001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:873: return;
On 2015/10/27 17:15:17, dsinclair wrote:
> I added handling for the two extra types here, it seems like just returning is
> the righting to do because we don't generate any pixels.

Acknowledged.

[dsinclair, 2015-10-27 17:18:31]

Committed patchset #4 (id:80001) manually as
468974316ed5f6b6f8e637ab2c7afedc7c2bfe6a (presubmit successful).