Aryan Krishnan (Gerrit)

unread,

Jun 22, 2026, 1:49:36 PM (3 days ago) Jun 22

to pdfium-...@googlegroups.com

Aryan Krishnan voted and added 1 comment

Votes added by Aryan Krishnan

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Aryan Krishnan . resolved

Haven't gotten the chance to test locally so just running this here.

Recently cleaned my system and rebuliding would take another hour or so (Don't have the patience :)).

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Aryan Krishnan (Gerrit)

unread,

Jun 22, 2026, 3:35:24 PM (3 days ago) Jun 22

to Lei Zhang, Tom Sepez, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan, Lei Zhang and Tom Sepez

Aryan Krishnan voted and added 1 comment

Votes added by Aryan Krishnan

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 8 (Latest):

Aryan Krishnan . resolved

Hey guys, decided to add public api for the /C argument (the optional color argument for Bookmarks). Wanted to also get some thoughts on a Set API for this maybe so that users in the chromium frontend can have bookmarks of custom colours allowing them to better categorize them (perhaps red for problem sets, blue for chapter summaries etc).

Thanks!

Open in Gerrit

Related details

Attention is currently required from:

Andy Phan
Lei Zhang
Tom Sepez

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Tom Sepez (Gerrit)

unread,

Jun 22, 2026, 3:39:00 PM (3 days ago) Jun 22

to Aryan Krishnan, Lei Zhang, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan, Aryan Krishnan and Lei Zhang

Tom Sepez added 2 comments

File core/fpdfdoc/cpdf_bookmark.h

Line 19, Patchset 8 (Latest):

struct ColorReturn {
  bool success;
  FX_RGB_STRUCT<float> color;
};

Tom Sepez . unresolved

Let's use std::optional<FX_RGB_STRUCT<float>>

File public/fpdf_doc.h

Line 148, Patchset 8 (Latest):// aren't in the range or if the bookmark does not specify a color.

Tom Sepez . unresolved

needs to say experimental on all new APIs.

Open in Gerrit

Related details

Attention is currently required from:

Andy Phan
Aryan Krishnan
Lei Zhang

Submit Requirements:

Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Aryan Krishnan (Gerrit)

unread,

Jun 22, 2026, 3:55:57 PM (3 days ago) Jun 22

to Lei Zhang, Tom Sepez, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan, Lei Zhang and Tom Sepez

Aryan Krishnan voted and added 2 comments

Votes added by Aryan Krishnan

Commit-Queue

+1

2 comments

File core/fpdfdoc/cpdf_bookmark.h

Line 19, Patchset 8:struct ColorReturn {


  bool success;
  FX_RGB_STRUCT<float> color;
};

Tom Sepez . resolved

Let's use std::optional<FX_RGB_STRUCT<float>>

Aryan Krishnan

Yep mb, forgot c++ existed for a second and decided to do it the struct way (as with public API sometimes).

Done.

File public/fpdf_doc.h

Line 148, Patchset 8:// aren't in the range or if the bookmark does not specify a color.

Tom Sepez . resolved

needs to say experimental on all new APIs.

Aryan Krishnan

Forgot about that, edited now.

But... I am so confused... where did EXEs come from?

Open in Gerrit

Related details

Attention is currently required from:

Andy Phan
Lei Zhang
Tom Sepez

Submit Requirements:

Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Tom Sepez (Gerrit)

unread,

Jun 22, 2026, 3:58:29 PM (3 days ago) Jun 22

to Aryan Krishnan, Lei Zhang, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan, Aryan Krishnan and Lei Zhang

Tom Sepez voted and added 1 comment

Votes added by Tom Sepez

Code-Review

+1

1 comment

File public/fpdf_doc.h

Line 148, Patchset 8:// aren't in the range or if the bookmark does not specify a color.

Tom Sepez . resolved

needs to say experimental on all new APIs.

Aryan Krishnan

Forgot about that, edited now.
But... I am so confused... where did EXEs come from?

Tom Sepez

Not sure what you are asking, Aryan.

Open in Gerrit

Related details

Attention is currently required from:

Andy Phan
Aryan Krishnan
Lei Zhang

Submit Requirements:

Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Aryan Krishnan (Gerrit)

unread,

Jun 22, 2026, 4:07:47 PM (3 days ago) Jun 22

to Tom Sepez, Lei Zhang, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan and Lei Zhang

Aryan Krishnan added 1 comment

File public/fpdf_doc.h

Line 148, Patchset 8:// aren't in the range or if the bookmark does not specify a color.

Tom Sepez . resolved

needs to say experimental on all new APIs.

Aryan Krishnan

Forgot about that, edited now.
But... I am so confused... where did EXEs come from?

Tom Sepez

Not sure what you are asking, Aryan.

Aryan Krishnan

... Right... I saw nothing and 100% NOT a suggested edit that mentions "EXE being an example of an application type"... Yeah... I saw nothing i just... was multitasking with something about windows so... somehow thought about exe ... don't worry about it :)

Open in Gerrit

Related details

Attention is currently required from:

Andy Phan
Lei Zhang

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Lei Zhang (Gerrit)

unread,

Jun 22, 2026, 6:21:47 PM (2 days ago) Jun 22

to Aryan Krishnan, Tom Sepez, Lei Zhang, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan and Aryan Krishnan

Lei Zhang added 9 comments

Commit Message

Line 9, Patchset 9 (Latest):This CL Introduces a new public API function, FPDFBookmark_GetColor,

Lei Zhang . unresolved

Either write "Introduces" lowercase, or just write "Introduce a new ..."

Line 14, Patchset 9 (Latest):get values from the optional "C" array from the underlying bookmark

Lei Zhang . unresolved

/C

Line 13, Patchset 9 (Latest):

To support this, the CPDF_Bookmark::GetColor() method has been added to
get values from the optional "C" array from the underlying bookmark

Lei Zhang . unresolved

Use active voice. "add Foo() to get foo".

File core/fpdfdoc/cpdf_bookmark.cpp

Line 61, Patchset 9 (Latest): RetainPtr<const CPDF_Array> RGB_array = dict_->GetArrayFor("C");

Lei Zhang . unresolved

Style: No capital letters in variable names.

File fpdfsdk/fpdf_doc.cpp

Line 201, Patchset 9 (Latest): auto color = cpdf_bookmark.GetColor();

Lei Zhang . unresolved

Write the type out. Most readers will not be able to do this type deduction in their head.

Line 205, Patchset 9 (Latest): if (color->red > 1 || color->green > 1 || color->blue > 1) {

Lei Zhang . unresolved

What about color component values less than 0?

File testing/resources/bookmarks_color.in

Line 57, Patchset 9 (Latest):100 600 TD (Page1)Tj

Lei Zhang . unresolved

Maybe consistently write 1 operator per line. Space in front of Tj.

Line 81, Patchset 9 (Latest):{{object 9 0}} <<

Lei Zhang . unresolved

For better test coverage, how about having bookmark nodes with these properties?

No /C
/C is not an array
/C is an array, but with the wrong number of elements
/C with 3 elements, but out of range array element value > 1
/C with 3 elements, but out of range array element value < 0
Perfectly valid /C value

Line 86, Patchset 9 (Latest): /C [ 1 1 1 ]

Lei Zhang . unresolved

Here, line 98, and others - no spaces immediately inside the brackets. i.e. Be consistent with line 40.

Open in Gerrit

Related details

Attention is currently required from:

Andy Phan
Aryan Krishnan

Submit Requirements:

Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Aryan Krishnan (Gerrit)

unread,

Jun 23, 2026, 3:21:27 AM (2 days ago) Jun 23

to Tom Sepez, Lei Zhang, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan, Lei Zhang and Tom Sepez

Aryan Krishnan added 9 comments

Commit Message

Line 9, Patchset 9:This CL Introduces a new public API function, FPDFBookmark_GetColor,

Lei Zhang . resolved

Either write "Introduces" lowercase, or just write "Introduce a new ..."

Aryan Krishnan

Done

Line 13, Patchset 9:To support this, the CPDF_Bookmark::GetColor() method has been added to


get values from the optional "C" array from the underlying bookmark

Lei Zhang . resolved

Use active voice. "add Foo() to get foo".

Aryan Krishnan

Done

Line 14, Patchset 9:get values from the optional "C" array from the underlying bookmark

Lei Zhang . resolved

/C

Aryan Krishnan

Done

File core/fpdfdoc/cpdf_bookmark.cpp

Line 61, Patchset 9: RetainPtr<const CPDF_Array> RGB_array = dict_->GetArrayFor("C");

Lei Zhang . resolved

Style: No capital letters in variable names.

Aryan Krishnan

Done

File fpdfsdk/fpdf_doc.cpp

Line 201, Patchset 9: auto color = cpdf_bookmark.GetColor();

Lei Zhang . resolved

Write the type out. Most readers will not be able to do this type deduction in their head.

Aryan Krishnan

Done

Line 205, Patchset 9: if (color->red > 1 || color->green > 1 || color->blue > 1) {

Lei Zhang . resolved

What about color component values less than 0?

Aryan Krishnan

They should also be rejected. Added the check.

File testing/resources/bookmarks_color.in

Line 57, Patchset 9:100 600 TD (Page1)Tj

Lei Zhang . resolved

Maybe consistently write 1 operator per line. Space in front of Tj.

Aryan Krishnan

Done

Line 81, Patchset 9:{{object 9 0}} <<

Lei Zhang . resolved

For better test coverage, how about having bookmark nodes with these properties?
No /C
/C is not an array
/C is an array, but with the wrong number of elements
/C with 3 elements, but out of range array element value > 1
/C with 3 elements, but out of range array element value < 0
Perfectly valid /C value

Aryan Krishnan

Done

Line 86, Patchset 9: /C [ 1 1 1 ]

Lei Zhang . resolved

Here, line 98, and others - no spaces immediately inside the brackets. i.e. Be consistent with line 40.

Aryan Krishnan

Done

Open in Gerrit

Related details

Attention is currently required from:

Andy Phan
Lei Zhang
Tom Sepez

Submit Requirements:

Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Aryan Krishnan (Gerrit)

unread,

Jun 23, 2026, 3:21:41 AM (2 days ago) Jun 23

to Tom Sepez, Lei Zhang, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan, Lei Zhang and Tom Sepez

Aryan Krishnan voted Commit-Queue+1

Commit-Queue

+1

unsatisfied_requirement

open

diffy

Lei Zhang (Gerrit)

unread,

Jun 23, 2026, 1:41:07 PM (2 days ago) Jun 23

to Aryan Krishnan, Tom Sepez, Lei Zhang, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan, Aryan Krishnan and Tom Sepez

Lei Zhang added 2 comments

Commit Message

Line 13, Patchset 14 (Latest):To support this, This CL adds the CPDF_Bookmark::GetColor() method to

Lei Zhang . unresolved

lowercase

File fpdfsdk/fpdf_doc_embeddertest.cpp

Line 613, Patchset 14 (Latest): ScopedFPDFWideString title = GetFPDFWideString(L"A Good Beginning");

Lei Zhang . unresolved

Maybe if the bookmarks had names like "No Color", that would provide better documentation of what their color characteristics are.

Open in Gerrit

Related details

Attention is currently required from:

Andy Phan
Aryan Krishnan
Tom Sepez

Submit Requirements:

Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Aryan Krishnan (Gerrit)

unread,

Jun 23, 2026, 1:54:36 PM (2 days ago) Jun 23

to Tom Sepez, Lei Zhang, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan, Lei Zhang and Tom Sepez

Aryan Krishnan voted and added 2 comments

Votes added by Aryan Krishnan

Commit-Queue

+1

2 comments

Commit Message

Line 13, Patchset 14:To support this, This CL adds the CPDF_Bookmark::GetColor() method to

Lei Zhang . resolved

lowercase

Aryan Krishnan

Done

File fpdfsdk/fpdf_doc_embeddertest.cpp

Line 613, Patchset 14: ScopedFPDFWideString title = GetFPDFWideString(L"A Good Beginning");

Lei Zhang . resolved

Maybe if the bookmarks had names like "No Color", that would provide better documentation of what their color characteristics are.

Aryan Krishnan

Done, and now that they are self-documenting, removed the additional comments as well.

Open in Gerrit

Related details

Attention is currently required from:

Andy Phan
Lei Zhang
Tom Sepez

Submit Requirements:

Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Lei Zhang (Gerrit)

unread,

Jun 23, 2026, 2:03:33 PM (2 days ago) Jun 23

to Aryan Krishnan, Lei Zhang, Tom Sepez, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan, Aryan Krishnan and Tom Sepez

Lei Zhang voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Andy Phan
Aryan Krishnan
Tom Sepez

Submit Requirements:

Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Aryan Krishnan (Gerrit)

unread,

Jun 23, 2026, 2:06:11 PM (2 days ago) Jun 23

to Lei Zhang, Tom Sepez, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan and Tom Sepez

Aryan Krishnan voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

Andy Phan
Tom Sepez

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tom Sepez (Gerrit)

unread,

Jun 24, 2026, 2:52:20 PM (15 hours ago) Jun 24

to Aryan Krishnan, Lei Zhang, Andy Phan, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Andy Phan and Aryan Krishnan

Tom Sepez voted

Code-Review	+1
Commit-Queue	+2

Open in Gerrit

Related details

Attention is currently required from:

Andy Phan
Aryan Krishnan

Submit Requirements:

Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)

unread,

Jun 24, 2026, 4:09:22 PM (14 hours ago) Jun 24

to Aryan Krishnan, Tom Sepez, Lei Zhang, Andy Phan, pdfium-...@googlegroups.com

pdfium...@luci-project-accounts.iam.gserviceaccount.com submitted the change

Change information

Commit message:

Add FPDFBookmark_GetColor public API

This CL introduces a new public API function, FPDFBookmark_GetColor,
which allows emedders to retrieve the RGB color components of a PDF
outline item (bookmark).

To support this, this CL adds the CPDF_Bookmark::GetColor() method to
get values from the optional "/C" array from the underlying bookmark
dictionary. The API validates that the RGB color components are present,
correctly formed, and conform to the PDF specification limits (values
between 0.0 and 1.0).

Bug: 362787705

Change-Id: I246243c32c7c4b35a7f4200b610712282e66b35a

Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/150430

Commit-Queue: Tom Sepez <tse...@chromium.org>

Reviewed-by: Tom Sepez <tse...@chromium.org>

Reviewed-by: Lei Zhang <the...@chromium.org>

Files:

M core/fpdfdoc/cpdf_bookmark.cpp
M core/fpdfdoc/cpdf_bookmark.h
M fpdfsdk/fpdf_doc.cpp
M fpdfsdk/fpdf_doc_embeddertest.cpp
M fpdfsdk/fpdf_view_c_api_test.c
M public/fpdf_doc.h
A testing/resources/bookmarks_color.in
A testing/resources/bookmarks_color.pdf

Change size: L

Delta: 8 files changed, 433 insertions(+), 0 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Tom Sepez, +1 by Lei Zhang

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

Add FPDFBookmark_GetColor public API [pdfium : main]

Aryan Krishnan (Gerrit)

Aryan Krishnan voted and added 1 comment

Votes added by Aryan Krishnan

1 comment

Related details

Aryan Krishnan (Gerrit)

Aryan Krishnan voted and added 1 comment

Votes added by Aryan Krishnan

1 comment

Related details

Tom Sepez (Gerrit)

Tom Sepez added 2 comments

Related details

Aryan Krishnan (Gerrit)

Aryan Krishnan voted and added 2 comments

Votes added by Aryan Krishnan

2 comments

Related details

Tom Sepez (Gerrit)

Tom Sepez voted and added 1 comment

Votes added by Tom Sepez

1 comment

Related details

Aryan Krishnan (Gerrit)

Aryan Krishnan added 1 comment

Related details

Lei Zhang (Gerrit)

Lei Zhang added 9 comments

Related details

Aryan Krishnan (Gerrit)

Aryan Krishnan added 9 comments

Related details

Aryan Krishnan (Gerrit)

Aryan Krishnan voted Commit-Queue+1

Lei Zhang (Gerrit)

Lei Zhang added 2 comments

Related details

Aryan Krishnan (Gerrit)

Aryan Krishnan voted and added 2 comments

Votes added by Aryan Krishnan

2 comments

Related details

Lei Zhang (Gerrit)

Lei Zhang voted Code-Review+1

Related details

Aryan Krishnan (Gerrit)

Aryan Krishnan voted Commit-Queue+1

Related details

Tom Sepez (Gerrit)

Tom Sepez voted

Related details

pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)

pdfium...@luci-project-accounts.iam.gserviceaccount.com submitted the change

Change information