Switch the background color in PDF Viewer when using Material Design

Switch the background color in PDF Viewer when using Material Design

BUG=110020
Committed: https://crrev.com/04594412237a169189b3bd2b2f35d51bab6a391d
Cr-Commit-Position: refs/heads/master@{#314108}

[Alexandre Carlton, 2015-01-29 04:48:56]

alexandrec@chromium.org changed reviewers:
+ benwells@chromium.org, raymes@chromium.org, sammc@chromium.org

[Alexandre Carlton, 2015-01-29 04:48:56]

https://codereview.chromium.org/871403005/diff/20001/pdf/pdf_engine.h
File pdf/pdf_engine.h (right):

https://codereview.chromium.org/871403005/diff/20001/pdf/pdf_engine.h#newcode35
pdf/pdf_engine.h:35: const uint32 kBackgroundColorMaterial = 0xFFEEEEEE;
raymes@: I am hesitant to shift these two variables into
out_of_process_instance.cc and instance.cc, given we initialise
background_color_ to kBackgroundColor in pdfium_engine.cc.
Should we not initialise it at all and set it to kBackgroundColor should we fail
the isMaterial check in out_of_process_instance.cc?

[raymes, 2015-01-29 05:26:32]

https://codereview.chromium.org/871403005/diff/20001/pdf/out_of_process_insta...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/871403005/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:334: const char* isMaterial = NULL;
is_material

https://codereview.chromium.org/871403005/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:343: isMaterial = argv[i];
Can we just have a bool here? is_material = true

https://codereview.chromium.org/871403005/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:346: if (strcmp(isMaterial, "true") == 0)
if (is_material)

https://codereview.chromium.org/871403005/diff/40001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/871403005/diff/40001/pdf/instance.cc#newcode59
pdf/instance.cc:59: const uint32 kBackgroundColor = 0xFFCCCCCC;
We need to call SetBackgroundColor in this file or else it will be black.

[Alexandre Carlton, 2015-01-29 06:05:17]

https://codereview.chromium.org/871403005/diff/20001/pdf/out_of_process_insta...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/871403005/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:334: const char* isMaterial = NULL;
On 2015/01/29 05:26:32, raymes wrote:
> is_material

Done.

https://codereview.chromium.org/871403005/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:343: isMaterial = argv[i];
On 2015/01/29 05:26:32, raymes wrote:
> Can we just have a bool here? is_material = true

Done.

https://codereview.chromium.org/871403005/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:346: if (strcmp(isMaterial, "true") == 0)
On 2015/01/29 05:26:32, raymes wrote:
> if (is_material)

Done.

https://codereview.chromium.org/871403005/diff/40001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/871403005/diff/40001/pdf/instance.cc#newcode59
pdf/instance.cc:59: const uint32 kBackgroundColor = 0xFFCCCCCC;
On 2015/01/29 05:26:32, raymes wrote:
> We need to call SetBackgroundColor in this file or else it will be black.

Done.

[raymes, 2015-01-30 03:41:00]

lgtm

https://codereview.chromium.org/871403005/diff/80001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/871403005/diff/80001/pdf/instance.cc#newcode371
pdf/instance.cc:371: 
nit: Maybe move that line down here so it's in more of a similar place to the
other file.

https://codereview.chromium.org/871403005/diff/80001/pdf/out_of_process_insta...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/871403005/diff/80001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:55: // PDF background colors
nit: add a "."

[Alexandre Carlton, 2015-01-30 03:53:42]

https://codereview.chromium.org/871403005/diff/80001/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/871403005/diff/80001/pdf/instance.cc#newcode371
pdf/instance.cc:371: 
On 2015/01/30 03:41:00, raymes wrote:
> nit: Maybe move that line down here so it's in more of a similar place to the
> other file.

Done.

https://codereview.chromium.org/871403005/diff/80001/pdf/out_of_process_insta...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/871403005/diff/80001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:55: // PDF background colors
On 2015/01/30 03:41:00, raymes wrote:
> nit: add a "."

Done.

[Sam McNally, 2015-02-02 00:50:52]

https://codereview.chromium.org/871403005/diff/100001/pdf/pdfium/pdfium_engine.h
File pdf/pdfium/pdfium_engine.h (right):

https://codereview.chromium.org/871403005/diff/100001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.h:687: uint32 background_color_;
Is this initialized anywhere?

[Alexandre Carlton, 2015-02-02 01:51:39]

https://codereview.chromium.org/871403005/diff/100001/pdf/pdfium/pdfium_engine.h
File pdf/pdfium/pdfium_engine.h (right):

https://codereview.chromium.org/871403005/diff/100001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.h:687: uint32 background_color_;
On 2015/02/02 00:50:52, Sam McNally wrote:
> Is this initialized anywhere?

No, should it be initialised to 0xFFFFFFFF, or one of the background colours
chosen?

[Sam McNally, 2015-02-02 06:09:19]

lgtm

[Alexandre Carlton, 2015-02-02 06:37:10]

The CQ bit was checked by alexandrec@chromium.org

[commit-bot: I haz the power, 2015-02-02 06:37:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/871403005/120001

[commit-bot: I haz the power, 2015-02-02 08:09:26]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-02 08:10:18]

Patchset 7 (id:??) landed as
https://crrev.com/04594412237a169189b3bd2b2f35d51bab6a391d
Cr-Commit-Position: refs/heads/master@{#314108}