[OOPIF PDF] Add PdfViewerPrivate extension API endpoints [chromium/src : main]

Andy Phan (Gerrit)

unread,

Oct 5, 2023, 8:01:42 PM10/5/23

to Lei Zhang, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Lei Zhang.

Andy Phan would like Lei Zhang to review this change.

[OOPIF PDF] Add PdfViewerPrivate extension API endpoints

Add extension functions GetStreamInfo() and SetPdfPluginAttributes().
These will be used to load the PDF Viewer in OOPIF without a GuestView.

Functionality is very similar to extensions::MimeHandlerServiceImpl's
GetStreamInfo() and SetPdfPluginAttributes(), but instead of using a
service, use an extension API function instead.

Currently, these functions are unused in this CL. Future CLs will make
use of the new API endpoints. See https://crrev.com/c/4691287.

Bug: 1445746
Change-Id: I7b0439a0b65393c5e651dc8e06b0bd21cfe0ad64
---
M chrome/browser/extensions/api/pdf_viewer_private/BUILD.gn
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.h
A chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
M chrome/common/extensions/api/pdf_viewer_private.idl
M chrome/test/BUILD.gn
M extensions/browser/extension_function_histogram_value.h
M tools/metrics/histograms/enums.xml
M tools/typescript/definitions/pdf_viewer_private.d.ts
9 files changed, 495 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 5, 2023, 8:01:48 PM10/5/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Lei Zhang, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Lei Zhang.

Patch set 13:Commit-Queue +1

View Change

1 comment:

Patchset:
- Patch Set #13:
  +thestig@ for initial review, PTAL. Thanks!

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 5, 2023, 8:08:44 PM10/5/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Lei Zhang, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Lei Zhang.

This change meets the code coverage requirements.

Patch set 12:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Lei Zhang (Gerrit)

unread,

Oct 6, 2023, 7:15:09 PM10/6/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, findit...@appspot.gserviceaccount.com, Lei Zhang, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan.

View Change

2 comments:

Commit Message:
- Patch Set #13, Line 14: service
  What does "service" mean here? The other end that JS talks to is the browser process.
File chrome/common/extensions/api/pdf_viewer_private.idl:
- Patch Set #13, Line 9: dictionary StreamInfo {
  It is safe to assume this is the same as the existing mimeHandlerPrivate API?

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Lei Zhang (Gerrit)

unread,

Oct 6, 2023, 7:36:38 PM10/6/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, findit...@appspot.gserviceaccount.com, Lei Zhang, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan.

View Change

10 comments:

File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc:
- Patch Set #13, Line 27: namespace SetPdfPluginAttributes =
  Move this down to keep the list sorted.
- Patch Set #13, Line 46: Mojo strings
  Not using Mojo here.
- Patch Set #13, Line 51: // TODO(sammc): Send as bytes instead of a string and let the client decide
  1) Given the paragraph above, haven't this effectively already been decided?
  2) Is sammc@ really going to work on this?
- Patch Set #13, Line 57: std::string* current_value = result.FindString(header_name);
  Want to try adding test coverage for this part of the function?
- Patch Set #13, Line 59: *current_value += ", " + header_value;
  Use base::StrAppend() here?
- Patch Set #13, Line 205: if (!embedder_frame) {
  In other related code, similar code has this as `CHECK(embedder_frame);`. Should this just CHECK() as well?
File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc:
- Patch Set #13, Line 35: namespace {} // namespace
  Was this suppose to wrap the constants above?
- Patch Set #13, Line 149: extensions::
  Not needed inside namespace extensions. Another case below.
- Patch Set #13, Line 155: EXPECT_EQ
  EXPECT_FALSE()
- Patch Set #13, Line 189: EXPECT_EQ
  EXPECT_TRUE()

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 9, 2023, 5:47:24 PM10/9/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Andy Phan.

Andy Phan uploaded patch set #14 to this change.

View Change

[OOPIF PDF] Add PdfViewerPrivate extension API endpoints

Add extension functions GetStreamInfo() and SetPdfPluginAttributes().
These will be used to load the PDF Viewer in OOPIF without a GuestView.

Functionality is very similar to extensions::MimeHandlerServiceImpl's
GetStreamInfo() and SetPdfPluginAttributes(), but instead of using a
service, use an extension API function instead.

Currently, these functions are unused in this CL. Future CLs will make
use of the new API endpoints. See https://crrev.com/c/4691287.

Bug: 1445746
Change-Id: I7b0439a0b65393c5e651dc8e06b0bd21cfe0ad64
---
M chrome/browser/extensions/api/pdf_viewer_private/BUILD.gn
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.h
A chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
M chrome/common/extensions/api/pdf_viewer_private.idl
M chrome/test/BUILD.gn
M extensions/browser/extension_function_histogram_value.h
M tools/metrics/histograms/enums.xml
M tools/typescript/definitions/pdf_viewer_private.d.ts

9 files changed, 523 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 9, 2023, 5:48:53 PM10/9/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, findit...@appspot.gserviceaccount.com, Lei Zhang, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Lei Zhang.

View Change

12 comments:

Commit Message:

- Patch Set #13, Line 14: service
  What does "service" mean here? The other end that JS talks to is the browser process.

- "service" refers to MimeHandlerServiceImpl and mime_handler::MimeHandlerService.
  https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/api/mime_handler_private/mime_handler_private.h;l=17
File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc:

- Patch Set #13, Line 27: namespace SetPdfPluginAttributes =
  Move this down to keep the list sorted.

- Done

- Patch Set #13, Line 46: Mojo strings
  Not using Mojo here.

- Removed mention of Mojo.

- Patch Set #13, Line 51: // TODO(sammc): Send as bytes instead of a string and let the client decide

- 1) Given the paragraph above, haven't this effectively already been decided? […]
  Looking at previous CRs [1], I think the TODO was for MimeHandlerView cases not related to PDF, so this seems unnecessary for PDF-only case.
  Removed.
  [1] https://codereview.chromium.org/1092163004/diff/40001/extensions/browser/api/mime_handler_private/mime_handler_private.cc

- Patch Set #13, Line 57: std::string* current_value = result.FindString(header_name);
  Want to try adding test coverage for this part of the function?

- Sure, added a test for it.

- Patch Set #13, Line 59: *current_value += ", " + header_value;
  Use base::StrAppend() here?

- Done

- Patch Set #13, Line 205: if (!embedder_frame) {

- In other related code, similar code has this as `CHECK(embedder_frame);`. […]
  Sure, done. Removed related tests.
File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc:

- Patch Set #13, Line 35: namespace {} // namespace
  Was this suppose to wrap the constants above?

- Yes, fixed.

- Patch Set #13, Line 149: extensions::
  Not needed inside namespace extensions. Another case below.

- Removed

- Patch Set #13, Line 155: EXPECT_EQ
  EXPECT_FALSE()

- Done

- Patch Set #13, Line 189: EXPECT_EQ
  EXPECT_TRUE()

- Done
File chrome/common/extensions/api/pdf_viewer_private.idl:

- Patch Set #13, Line 9: dictionary StreamInfo {
  It is safe to assume this is the same as the existing mimeHandlerPrivate API?

- Yes, it is the same.

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 9, 2023, 6:43:12 PM10/9/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Lei Zhang, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Lei Zhang.

This change meets the code coverage requirements.

Patch set 14:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Lei Zhang (Gerrit)

unread,

Oct 9, 2023, 7:45:14 PM10/9/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Lei Zhang, findit...@appspot.gserviceaccount.com, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan.

Patch set 14:Code-Review +1

View Change

3 comments:

Commit Message:

- Patch Set #13, Line 14: service

- "service" refers to MimeHandlerServiceImpl and mime_handler::MimeHandlerService. […]
  Given most services run outside of the browser process, e.g. Everything in chrome/services. Do you see how this sentence, as written, can potentially give the reader a false impression of what's happening?
File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc:
- Patch Set #14, Line 113: const GURL handler_url = GURL("handler_url");
  Just inline these variables into the only place that uses them? Given the values are variable names are the same, they are already somewhat self-documenting.
File chrome/common/extensions/api/pdf_viewer_private.idl:

- Patch Set #13, Line 9: dictionary StreamInfo {

- Yes, it is the same.
  Acknowledged

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 10, 2023, 12:12:42 AM10/10/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Andy Phan.

Andy Phan uploaded patch set #15 to this change.

View Change

[OOPIF PDF] Add PdfViewerPrivate extension API endpoints

Add extension functions GetStreamInfo() and SetPdfPluginAttributes().
These will be used to load the PDF Viewer in OOPIF without a GuestView.

Functionality is very similar to extensions::MimeHandlerServiceImpl's
GetStreamInfo() and SetPdfPluginAttributes(), but instead of using a
service, use an extension API function instead.

Currently, these functions are unused in this CL. Future CLs will make
use of the new API endpoints. See https://crrev.com/c/4691287.

Bug: 1445746
Change-Id: I7b0439a0b65393c5e651dc8e06b0bd21cfe0ad64
---
M chrome/browser/extensions/api/pdf_viewer_private/BUILD.gn
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.h
A chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
M chrome/common/extensions/api/pdf_viewer_private.idl
M chrome/test/BUILD.gn
M extensions/browser/extension_function_histogram_value.h
M tools/metrics/histograms/enums.xml
M tools/typescript/definitions/pdf_viewer_private.d.ts

9 files changed, 520 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 10, 2023, 1:04:00 AM10/10/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Lei Zhang, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan.

This change meets the code coverage requirements.

Patch set 15:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 10, 2023, 12:43:15 PM10/10/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Andy Phan uploaded patch set #16 to this change.

View Change

[OOPIF PDF] Add PdfViewerPrivate extension API endpoints

Add extension functions GetStreamInfo() and SetPdfPluginAttributes().
These will be used to load the PDF Viewer in OOPIF without a GuestView.

Functionality is very similar to extensions::MimeHandlerServiceImpl's
GetStreamInfo() and SetPdfPluginAttributes(), but instead of using

mime_handler::MimeHandlerService, use an extension API function instead.



Currently, these functions are unused in this CL. Future CLs will make
use of the new API endpoints. See https://crrev.com/c/4691287.

Bug: 1445746
Change-Id: I7b0439a0b65393c5e651dc8e06b0bd21cfe0ad64
---
M chrome/browser/extensions/api/pdf_viewer_private/BUILD.gn
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.h
A chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
M chrome/common/extensions/api/pdf_viewer_private.idl
M chrome/test/BUILD.gn
M extensions/browser/extension_function_histogram_value.h
M tools/metrics/histograms/enums.xml
M tools/typescript/definitions/pdf_viewer_private.d.ts
9 files changed, 520 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 10, 2023, 12:45:47 PM10/10/23

to Demetrios Papadopoulos, Tim, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Lei Zhang, findit...@appspot.gserviceaccount.com

Attention is currently required from: Demetrios Papadopoulos, Tim.

Andy Phan would like Demetrios Papadopoulos and Tim to review this change.

View Change

[OOPIF PDF] Add PdfViewerPrivate extension API endpoints

Add extension functions GetStreamInfo() and SetPdfPluginAttributes().
These will be used to load the PDF Viewer in OOPIF without a GuestView.

Functionality is very similar to extensions::MimeHandlerServiceImpl's
GetStreamInfo() and SetPdfPluginAttributes(), but instead of using
mime_handler::MimeHandlerService, use an extension API function instead.

Currently, these functions are unused in this CL. Future CLs will make
use of the new API endpoints. See https://crrev.com/c/4691287.

Bug: 1445746
Change-Id: I7b0439a0b65393c5e651dc8e06b0bd21cfe0ad64
---
M chrome/browser/extensions/api/pdf_viewer_private/BUILD.gn
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.h
A chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
M chrome/common/extensions/api/pdf_viewer_private.idl
M chrome/test/BUILD.gn
M extensions/browser/extension_function_histogram_value.h
M tools/metrics/histograms/enums.xml
M tools/typescript/definitions/pdf_viewer_private.d.ts
9 files changed, 520 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 10, 2023, 12:45:56 PM10/10/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Demetrios Papadopoulos, Tim, Lei Zhang, findit...@appspot.gserviceaccount.com, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Demetrios Papadopoulos, Tim.

View Change

3 comments:

Patchset:
- Patch Set #16:
  +dpapad@ for pdf_viewer_private.d.ts.
  +tjudkins@ for extension-related files and for approval of the private extension API.
  PTAL. Thanks!
Commit Message:

- Patch Set #13, Line 14: service

- Given most services run outside of the browser process, e.g. Everything in chrome/services. […]
  Changed to "mime_handler::MimeHandlerService".
File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc:

- Patch Set #14, Line 113: const GURL handler_url = GURL("handler_url");

- Just inline these variables into the only place that uses them? Given the values are variable names […]
  Done

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Demetrios Papadopoulos (Gerrit)

unread,

Oct 10, 2023, 1:59:34 PM10/10/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Tim, Lei Zhang, findit...@appspot.gserviceaccount.com, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan, Tim.

View Change

1 comment:

File tools/typescript/definitions/pdf_viewer_private.d.ts:
- Patch Set #16, Line 18: Object
  s/Object/object/ ?
  More importantly, have you actually tried ot use this member in TS code? As it is currently typed this will give type errors that will require type casting to resolve. See minimal example and suggested alternative below.
  https://www.typescriptlang.org/play?#code/DYUwLgBAhgXBD2AjAViAxpAvBA3gXwG4AoNeAOwGd5QA6YeAcwAooBtAcgAsRh72BdAJQEIAelEQAKgE8ADiAggATkvhKiRUJERwASujUATADwUwSgJZkGAGghnL1gHwRs+YqUrUQdRk0Qc3LzwAsJiEgDi8PCGAIRAA

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 10, 2023, 2:19:48 PM10/10/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Andy Phan, Tim.

Andy Phan uploaded patch set #17 to this change.

View Change

[OOPIF PDF] Add PdfViewerPrivate extension API endpoints

Add extension functions GetStreamInfo() and SetPdfPluginAttributes().
These will be used to load the PDF Viewer in OOPIF without a GuestView.

Functionality is very similar to extensions::MimeHandlerServiceImpl's
GetStreamInfo() and SetPdfPluginAttributes(), but instead of using
mime_handler::MimeHandlerService, use an extension API function instead.

Currently, these functions are unused in this CL. Future CLs will make
use of the new API endpoints. See https://crrev.com/c/4691287.

Bug: 1445746
Change-Id: I7b0439a0b65393c5e651dc8e06b0bd21cfe0ad64
---
M chrome/browser/extensions/api/pdf_viewer_private/BUILD.gn
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.h
A chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
M chrome/common/extensions/api/pdf_viewer_private.idl
M chrome/test/BUILD.gn
M extensions/browser/extension_function_histogram_value.h
M tools/metrics/histograms/enums.xml
M tools/typescript/definitions/pdf_viewer_private.d.ts

9 files changed, 518 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 10, 2023, 2:21:12 PM10/10/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Demetrios Papadopoulos, Tim, Lei Zhang, findit...@appspot.gserviceaccount.com, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Demetrios Papadopoulos, Tim.

View Change

1 comment:

File tools/typescript/definitions/pdf_viewer_private.d.ts:

- Patch Set #16, Line 18: Object

- s/Object/object/ ? […]
  This was taken from [1]. I don't think responseHeaders is actually used in TS code; it just gets passed from the browser process, to the extension process (using the extension API), and then passed to the plugin process. Changed to use Record<string, string>, though.
  [1] https://source.chromium.org/chromium/chromium/src/+/main:tools/typescript/definitions/mime_handler_private.d.ts;l=18

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Demetrios Papadopoulos (Gerrit)

unread,

Oct 10, 2023, 2:44:18 PM10/10/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Tim, Lei Zhang, findit...@appspot.gserviceaccount.com, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan, Tim.

View Change

1 comment:

File tools/typescript/definitions/pdf_viewer_private.d.ts:

- Patch Set #16, Line 18: Object

- This was taken from [1].
  Ack. Closure compiler does not have the same type expressiveness and strictness of the TS compiler.

 I don't think responseHeaders is actually used in TS code

- Can we not update this file at all then? Is there a presubmit that is complaining? From my perspective, given the TODO at line 6, I think it is reasonable to only minimally update such files as needed, until the TODO is addressed (which BTW you could try since the script that does this has already been added to the repo).

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 10, 2023, 3:45:57 PM10/10/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Demetrios Papadopoulos, Tim, Lei Zhang, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Tim.

This change meets the code coverage requirements.

Patch set 17:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 10, 2023, 4:29:34 PM10/10/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Demetrios Papadopoulos, Tim, Lei Zhang, findit...@appspot.gserviceaccount.com, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Demetrios Papadopoulos, Tim.

View Change

1 comment:

File tools/typescript/definitions/pdf_viewer_private.d.ts:

- Patch Set #16, Line 18: Object

- > This was taken from [1]. […]
  To clarify, the changes in this file are still necessary. The added API calls are used in https://chromium-review.googlesource.com/c/chromium/src/+/4691287. Without the changes in this file, there are compiler errors.
  What I meant was that responseHeaders isn't actually accessed or modified after the API calls.

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Demetrios Papadopoulos (Gerrit)

unread,

Oct 10, 2023, 5:17:06 PM10/10/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Tim, Lei Zhang, findit...@appspot.gserviceaccount.com, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan, Tim.

Patch set 17:Code-Review +1

View Change

2 comments:

Patchset:
- Patch Set #17:
  LGTM with clarification below.
File tools/typescript/definitions/pdf_viewer_private.d.ts:

- Patch Set #16, Line 18: Object

- What I meant was that responseHeaders isn't actually accessed or modified after the API calls.
  Ack. Similarly, I should have clarified that I meant to minimally update this file (use as few attributes as needed) to satisfy the currently existing code that uses these definitions. If/when future code that relies on non-documented attributes comes along, we can update this file at that time.

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Tim (Gerrit)

unread,

Oct 10, 2023, 5:31:54 PM10/10/23

to Devlin Cronin, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Andy Phan, Demetrios Papadopoulos, Lei Zhang, findit...@appspot.gserviceaccount.com

Attention is currently required from: Andy Phan, Devlin Cronin.

Tim would like Devlin Cronin to review this change authored by Andy Phan.

View Change

[OOPIF PDF] Add PdfViewerPrivate extension API endpoints

Add extension functions GetStreamInfo() and SetPdfPluginAttributes().
These will be used to load the PDF Viewer in OOPIF without a GuestView.

Functionality is very similar to extensions::MimeHandlerServiceImpl's
GetStreamInfo() and SetPdfPluginAttributes(), but instead of using
mime_handler::MimeHandlerService, use an extension API function instead.

Currently, these functions are unused in this CL. Future CLs will make
use of the new API endpoints. See https://crrev.com/c/4691287.

Bug: 1445746
Change-Id: I7b0439a0b65393c5e651dc8e06b0bd21cfe0ad64
---
M chrome/browser/extensions/api/pdf_viewer_private/BUILD.gn
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.h
A chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
M chrome/common/extensions/api/pdf_viewer_private.idl
M chrome/test/BUILD.gn
M extensions/browser/extension_function_histogram_value.h
M tools/metrics/histograms/enums.xml
M tools/typescript/definitions/pdf_viewer_private.d.ts
9 files changed, 518 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Tim (Gerrit)

unread,

Oct 10, 2023, 5:32:04 PM10/10/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Devlin Cronin, Demetrios Papadopoulos, Lei Zhang, findit...@appspot.gserviceaccount.com, K. Moon, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan, Devlin Cronin.

View Change

2 comments:

Patchset:
- Patch Set #17:
  This looks great, the custom bindings for mimeHandlerPrivate are very gnarly and this looks much cleaner! Will the kPdfOopif feature eventually just be the default and allow us to get rid of the mimeHandlerPrivate side?
  This looks good to me, but also copying Devlin on to do a pass to see if there's anything major from the extensions side I might have missed.
File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc:
- Patch Set #17, Line 104:
```
auto stream_info =
      base::Value::Dict()
          .Set("mimeType", stream->mime_type())
          .Set("originalUrl", stream->original_url().spec())
          .Set("streamUrl", stream->stream_url().spec())
          .Set("tabId", stream->tab_id())
          .Set("responseHeaders",
               CreateResponseHeadersDict(stream->response_headers()))
          .Set("embedded", stream->embedded());
```
  You can actually rely on the auto-generated types here, so something like:
```
#include "extensions/common/api/pdf_viewer_private.h"
...
api::pdfViewerPrivate::StreamInfo stream_info;
stream_info.mime_type = stream->mime_type();
stream_info.original_url = stream->original_url().spec();
...
return RespondNow(WithArguments(stream_info.ToValue()));
```
  That saves on any potential mistakes in the strings for attribute names and ensures things have the correct types involved.

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 10, 2023, 6:57:15 PM10/10/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Andy Phan, Demetrios Papadopoulos, Devlin Cronin, Lei Zhang.

Andy Phan uploaded patch set #18 to this change.

View Change

The following approvals got outdated and were removed: Code-Coverage+1 by findit...@appspot.gserviceaccount.com, Code-Review+1 by Demetrios Papadopoulos, Code-Review+1 by Lei Zhang

[OOPIF PDF] Add PdfViewerPrivate extension API endpoints

Add extension functions GetStreamInfo() and SetPdfPluginAttributes().
These will be used to load the PDF Viewer in OOPIF without a GuestView.

Functionality is very similar to extensions::MimeHandlerServiceImpl's
GetStreamInfo() and SetPdfPluginAttributes(), but instead of using
mime_handler::MimeHandlerService, use an extension API function instead.

Currently, these functions are unused in this CL. Future CLs will make
use of the new API endpoints. See https://crrev.com/c/4691287.

Bug: 1445746
Change-Id: I7b0439a0b65393c5e651dc8e06b0bd21cfe0ad64
---
M chrome/browser/extensions/api/pdf_viewer_private/BUILD.gn
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.h
A chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
M chrome/common/extensions/api/pdf_viewer_private.idl
M chrome/test/BUILD.gn
M extensions/browser/extension_function_histogram_value.h
M tools/metrics/histograms/enums.xml

8 files changed, 502 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 10, 2023, 7:50:01 PM10/10/23

to K. Moon, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Demetrios Papadopoulos, Devlin Cronin, Tim, Lei Zhang, findit...@appspot.gserviceaccount.com

Attention is currently required from: Devlin Cronin, Lei Zhang, Tim.

Andy Phan removed K. Moon from this change.

View Change

[OOPIF PDF] Add PdfViewerPrivate extension API endpoints

Add extension functions GetStreamInfo() and SetPdfPluginAttributes().
These will be used to load the PDF Viewer in OOPIF without a GuestView.

Functionality is very similar to extensions::MimeHandlerServiceImpl's
GetStreamInfo() and SetPdfPluginAttributes(), but instead of using
mime_handler::MimeHandlerService, use an extension API function instead.

Currently, these functions are unused in this CL. Future CLs will make
use of the new API endpoints. See https://crrev.com/c/4691287.

Bug: 1445746
Change-Id: I7b0439a0b65393c5e651dc8e06b0bd21cfe0ad64
---
M chrome/browser/extensions/api/pdf_viewer_private/BUILD.gn
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.h
A chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
M chrome/common/extensions/api/pdf_viewer_private.idl
M chrome/test/BUILD.gn
M extensions/browser/extension_function_histogram_value.h
M tools/metrics/histograms/enums.xml
8 files changed, 502 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 10, 2023, 7:50:09 PM10/10/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Demetrios Papadopoulos, Devlin Cronin, Tim, Lei Zhang, findit...@appspot.gserviceaccount.com, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Devlin Cronin, Lei Zhang, Tim.

View Change

3 comments:

Patchset:
- Patch Set #17:
  This looks great, the custom bindings for mimeHandlerPrivate are very gnarly and this looks much cle […]
  Yes, eventually, the kPdfOopif feature will be the default for the PDF viewer case. As for the mimeHandlerPrivate side, I believe it is still used by other MIME handlers like QuickOffice, although that will eventually be deprecated, too.
File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc:

Patch Set #17, Line 104:

auto stream_info =
      base::Value::Dict()
          .Set("mimeType", stream->mime_type())
          .Set("originalUrl", stream->original_url().spec())
          .Set("streamUrl", stream->stream_url().spec())
          .Set("tabId", stream->tab_id())
          .Set("responseHeaders",
               CreateResponseHeadersDict(stream->response_headers()))
          .Set("embedded", stream->embedded());

- You can actually rely on the auto-generated types here, so something like: […]
  Done, thanks.
File tools/typescript/definitions/pdf_viewer_private.d.ts:

- Patch Set #16, Line 18: Object

- > What I meant was that responseHeaders isn't actually accessed or modified after the API calls. […]
  Ack, moved to https://chromium-review.googlesource.com/c/chromium/src/+/4691287/12 and removing from reviewer list for this CL.

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 10, 2023, 7:51:26 PM10/10/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Demetrios Papadopoulos, Devlin Cronin, Tim, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Devlin Cronin, Lei Zhang, Tim.

This change meets the code coverage requirements.

Patch set 18:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Devlin Cronin (Gerrit)

unread,

Oct 12, 2023, 4:58:06 PM10/12/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, findit...@appspot.gserviceaccount.com, Demetrios Papadopoulos, Devlin Cronin, Tim, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan, Lei Zhang, Tim.

View Change

10 comments:

Patchset:
- Patch Set #18:
  Thank you! This generally looks good. A few comments, but most should be pretty straightforward.
File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc:
- Patch Set #18, Line 36: api::pdf_viewer_private::StreamInfo::ResponseHeaders CreateResponseHeaders(
  nit: add a function comment
- Patch Set #18, Line 57: base::StrAppend(current_value, {", ", header_value});
  are all headers affected here guaranteed to work with comma-appending? Some headers are separated differently (e.g. I think cookie and content-type are semicolon-separated)
- Patch Set #18, Line 111: CreateResponseHeaders(stream->response_headers());
  this has an interesting effect of instantiating response_headers with an empty set even if it were previously null. Is that desirable?
File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc:
- Patch Set #18, Line 28: kExpectedStreamInfo
  optional nit: since this has placeholders, I'd lean towards calling it a kExpectedStreamInfoTemplate or similar
- Patch Set #18, Line 37: constexpr char kExpectedStreamInfoResponseHeaders[] = R"({
  for ones that are only used in one place, I'd recommend moving the variable to the test itself. That way, the reader doesn't have to look at a separate definition to see the expectation
- Patch Set #18, Line 56: class PdfViewerPrivateApiUnitTest : public ChromeRenderViewHostTestHarness {};
  nit: prefer `using PdfViewerPrivateApiUnitTest = ChromeRenderViewHostTestHarness;`
  Though this might be moot with the below...
- Patch Set #18, Line 65:
```
  auto function = base::MakeRefCounted<PdfViewerPrivateGetStreamInfoFunction>();
  content::RenderFrameHostTester::For(main_rfh())
      ->InitializeRenderFrameIfNeeded();
  auto* extension_host = content::RenderFrameHostTester::For(main_rfh())
                             ->AppendChild("extension_host");
  function->SetRenderFrameHost(extension_host);
```
  these lines seem to appear in most / all tests. Should we have it be a method on the test suite?
- Patch Set #18, Line 203:
```
  int frame_tree_node_id = main_rfh()->GetFrameTreeNodeId();
  pdf::PdfViewerStreamManager::Get()->AddStreamContainer(
      frame_tree_node_id,
      pdf_test_util::GenerateSampleStreamContainer(frame_tree_node_id));
```
  nit, too, would probably benefit from a helper function (`CreateStreamContainer()` or similar)
File chrome/common/extensions/api/pdf_viewer_private.idl:
- Patch Set #18, Line 52: [supportsPromises] static void getStreamInfo(GetStreamInfoCallback callback);
  nit: wrap at 80 char

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 13, 2023, 9:44:58 AM10/13/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Andy Phan, Lei Zhang, Tim.

Andy Phan uploaded patch set #19 to this change.

View Change

[OOPIF PDF] Add PdfViewerPrivate extension API endpoints

Add extension functions GetStreamInfo() and SetPdfPluginAttributes().
These will be used to load the PDF Viewer in OOPIF without a GuestView.

Functionality is very similar to extensions::MimeHandlerServiceImpl's
GetStreamInfo() and SetPdfPluginAttributes(), but instead of using
mime_handler::MimeHandlerService, use an extension API function instead.

Currently, these functions are unused in this CL. Future CLs will make
use of the new API endpoints. See https://crrev.com/c/4691287.

Bug: 1445746
Change-Id: I7b0439a0b65393c5e651dc8e06b0bd21cfe0ad64
---
M chrome/browser/extensions/api/pdf_viewer_private/BUILD.gn
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.h
A chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
M chrome/common/extensions/api/pdf_viewer_private.idl
M chrome/test/BUILD.gn
M extensions/browser/extension_function_histogram_value.h
M tools/metrics/histograms/enums.xml

8 files changed, 389 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 13, 2023, 1:49:04 PM10/13/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Demetrios Papadopoulos, Devlin Cronin, Tim, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan, Lei Zhang, Tim.

This change meets the code coverage requirements.

Patch set 19:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 13, 2023, 2:29:08 PM10/13/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Andy Phan, Lei Zhang, Tim.

Andy Phan uploaded patch set #20 to this change.

View Change

[OOPIF PDF] Add PdfViewerPrivate extension API endpoints

Add extension functions GetStreamInfo() and SetPdfPluginAttributes().
These will be used to load the PDF Viewer in OOPIF without a GuestView.

Functionality is very similar to extensions::MimeHandlerServiceImpl's
GetStreamInfo() and SetPdfPluginAttributes(), but instead of using
mime_handler::MimeHandlerService, use an extension API function instead.

pdfViewerPrivate.StreamInfo and pdfViewerPrivate.PdfPluginAttributes are
based on mimeHandlerPrivate.StreamInfo and
mimeHandlerPrivate.PdfPluginAttributes. However, for the PDF viewer, the
mime type and response headers fields in StreamInfo are unused, so don't
include those fields.



Currently, these functions are unused in this CL. Future CLs will make
use of the new API endpoints. See https://crrev.com/c/4691287.

Bug: 1445746
Change-Id: I7b0439a0b65393c5e651dc8e06b0bd21cfe0ad64
---
M chrome/browser/extensions/api/pdf_viewer_private/BUILD.gn
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.h
A chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
M chrome/common/extensions/api/pdf_viewer_private.idl
M chrome/test/BUILD.gn
M extensions/browser/extension_function_histogram_value.h
M tools/metrics/histograms/enums.xml
8 files changed, 389 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 13, 2023, 2:34:17 PM10/13/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Andy Phan, Lei Zhang, Tim.

Andy Phan uploaded patch set #21 to this change.

View Change

8 files changed, 392 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 13, 2023, 2:35:52 PM10/13/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, findit...@appspot.gserviceaccount.com, Demetrios Papadopoulos, Devlin Cronin, Tim, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Devlin Cronin, Lei Zhang, Tim.

View Change

10 comments:

Patchset:
- Patch Set #21:
  Removed mimeType and responseHeaders fields from StreamInfo, since I don't think they're actually used in PDF viewer code. Adjusted future CLs, added CL comment, added comments in pdf_viewer_private.idl, and removed response header tests.
File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc:

- Patch Set #18, Line 36: api::pdf_viewer_private::StreamInfo::ResponseHeaders CreateResponseHeaders(
  nit: add a function comment

- Obsolete.

- Patch Set #18, Line 57: base::StrAppend(current_value, {", ", header_value});

- are all headers affected here guaranteed to work with comma-appending? Some headers are separated d […]
  Obsolete.
  (The existing code this was based off of only used comma-appending, which may or may not be a bug. https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/api/mime_handler_private/mime_handler_private.cc;l=23)

- Patch Set #18, Line 111: CreateResponseHeaders(stream->response_headers());

- this has an interesting effect of instantiating response_headers with an empty set even if it were p […]
  Obsolete.
File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc:

- Patch Set #18, Line 28: kExpectedStreamInfo

- optional nit: since this has placeholders, I'd lean towards calling it a kExpectedStreamInfoTemplate […]
  Done

- Patch Set #18, Line 37: constexpr char kExpectedStreamInfoResponseHeaders[] = R"({

- for ones that are only used in one place, I'd recommend moving the variable to the test itself. […]
  Obsolete.

- Patch Set #18, Line 56: class PdfViewerPrivateApiUnitTest : public ChromeRenderViewHostTestHarness {};

- nit: prefer `using PdfViewerPrivateApiUnitTest = ChromeRenderViewHostTestHarness;` […]
  Obsolete, after applying other comments.

Patch Set #18, Line 65:

  auto function = base::MakeRefCounted<PdfViewerPrivateGetStreamInfoFunction>();
  content::RenderFrameHostTester::For(main_rfh())
      ->InitializeRenderFrameIfNeeded();
  auto* extension_host = content::RenderFrameHostTester::For(main_rfh())
                             ->AppendChild("extension_host");
  function->SetRenderFrameHost(extension_host);

- these lines seem to appear in most / all tests. […]
  The "extension_host" setup can be done as part of the test setup, so moved to there. The function will still be initialized in the actual test, though.

Patch Set #18, Line 203:


  int frame_tree_node_id = main_rfh()->GetFrameTreeNodeId();
  pdf::PdfViewerStreamManager::Get()->AddStreamContainer(
      frame_tree_node_id,
      pdf_test_util::GenerateSampleStreamContainer(frame_tree_node_id));

nit, too, would probably benefit from a helper function (`CreateStreamContainer()` or similar)

- Done, thanks.
File chrome/common/extensions/api/pdf_viewer_private.idl:

- Patch Set #18, Line 52: [supportsPromises] static void getStreamInfo(GetStreamInfoCallback callback);
  nit: wrap at 80 char

- Done

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 13, 2023, 3:35:13 PM10/13/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Demetrios Papadopoulos, Devlin Cronin, Tim, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Devlin Cronin, Lei Zhang, Tim.

This change meets the code coverage requirements.

Patch set 21:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 15, 2023, 9:07:22 PM10/15/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Demetrios Papadopoulos, Devlin Cronin, Tim, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Devlin Cronin, Lei Zhang, Tim.

This change meets the code coverage requirements.

Patch set 22:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 16, 2023, 7:42:38 AM10/16/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Devlin Cronin, Lei Zhang, Tim.

Andy Phan uploaded patch set #23 to this change.

View Change

8 files changed, 403 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 16, 2023, 10:00:50 AM10/16/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Demetrios Papadopoulos, Devlin Cronin, Tim, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Devlin Cronin, Lei Zhang, Tim.

This change meets the code coverage requirements.

Patch set 23:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Devlin Cronin (Gerrit)

unread,

Oct 16, 2023, 6:03:16 PM10/16/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Devlin Cronin, findit...@appspot.gserviceaccount.com, Demetrios Papadopoulos, Tim, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan, Lei Zhang, Tim.

Patch set 23:Code-Review +1

View Change

3 comments:

Patchset:
- Patch Set #23:
  Thanks, Andy! LGTM!

File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc:

Patch Set #23, Line 111:

  const base::Value::Dict expected =
      base::test::ParseJsonDict(base::StringPrintf(
          kExpectedStreamInfoTemplate, frame_tree_node_id, frame_tree_node_id));

nit: this is unnecessary with base::test::IsJson():

```
EXPECT_THAT(*result_dict,
            base::test::IsJson(base::StringPrintf(
                kExpectedStreamInfoTemplate,
                frame_tree_node_id, frame_tree_node_id)));
                ```

File chrome/common/extensions/api/pdf_viewer_private.idl:
- Patch Set #23, Line 70: static void setPdfPluginAttributes(PdfPluginAttributes attributes);
  nit: even though there's no callback value expected here, let's pass a callback and include [supportsPromises]:
```
```
  callback VoidCallback = void();
  ...
  [supportsPromises] static void setPdfPluginAttributes(
      PdfPluginAttributes attributes,
      optional VoidCallback callback);
```
```
  Two reasons:
  1) That way, an extension can still call `await setPdfPluginAttributes(...)` and know that the operation was _complete_ when the await resolves
  2) We're moving towards having all APIs support promises, so this will be needed for that anyway : )
  This would have no impact on an extension that chooses to just fire-and-forget the API.

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Demetrios Papadopoulos (Gerrit)

unread,

Oct 16, 2023, 8:27:36 PM10/16/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Devlin Cronin, findit...@appspot.gserviceaccount.com, Tim, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan, Devlin Cronin, Lei Zhang, Tim.

View Change

1 comment:

File chrome/common/extensions/api/pdf_viewer_private.idl:

- Patch Set #23, Line 70: static void setPdfPluginAttributes(PdfPluginAttributes attributes);

- nit: even though there's no callback value expected here, let's pass a callback and include [support […]
  FWIW, since the PDF Viewer is not yet updated to MV3 (https://crbug.com/1282227), it would not be able to use the Promise-based version of the API anyway. Perhaps this should be mentioned somewhere to not confuse future readers.

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 17, 2023, 7:38:13 PM10/17/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Andy Phan, Lei Zhang, Tim.

Andy Phan uploaded patch set #24 to this change.

View Change

8 files changed, 401 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 17, 2023, 7:41:46 PM10/17/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Devlin Cronin, findit...@appspot.gserviceaccount.com, Demetrios Papadopoulos, Tim, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Devlin Cronin.

View Change

2 comments:

File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc:

Patch Set #23, Line 111:

  const base::Value::Dict expected =
      base::test::ParseJsonDict(base::StringPrintf(
          kExpectedStreamInfoTemplate, frame_tree_node_id, frame_tree_node_id));

- nit: this is unnecessary with base::test::IsJson(): […]
  Done, thanks. Out of curiosity, what makes this better than the original version?
File chrome/common/extensions/api/pdf_viewer_private.idl:

- Patch Set #23, Line 70: static void setPdfPluginAttributes(PdfPluginAttributes attributes);

- FWIW, since the PDF Viewer is not yet updated to MV3 (https://crbug. […]
  Done, thanks.

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 17, 2023, 7:42:06 PM10/17/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Tim, Devlin Cronin, findit...@appspot.gserviceaccount.com, Lei Zhang

Attention is currently required from: Devlin Cronin.

Andy Phan has uploaded this change for review.

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 17, 2023, 8:25:38 PM10/17/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Tim, Devlin Cronin, Demetrios Papadopoulos, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Devlin Cronin.

This change meets the code coverage requirements.

Patch set 24:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Devlin Cronin (Gerrit)

unread,

Oct 20, 2023, 5:57:15 PM10/20/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Tim, Devlin Cronin, findit...@appspot.gserviceaccount.com, Demetrios Papadopoulos, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan.

Patch set 24:Code-Review +1

View Change

1 comment:

File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc:

Patch Set #23, Line 111:

  const base::Value::Dict expected =
      base::test::ParseJsonDict(base::StringPrintf(
          kExpectedStreamInfoTemplate, frame_tree_node_id, frame_tree_node_id));

- Done, thanks. […]
  IMO, this is more direct -- you have your expected JSON string and are comparing directly against that, rather than taking your expected JSON string, then converting that into a dict, and comparing to that. I _believe_ it also results in fewer conversions, but that's both an implementation detail (and not guaranteed) and not terribly relevant since this is a unittest.

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 25, 2023, 3:18:33 PM10/25/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Andy Phan.

Andy Phan uploaded patch set #25 to this change.

View Change

8 files changed, 419 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 25, 2023, 8:07:24 PM10/25/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Andy Phan.

Andy Phan uploaded patch set #26 to this change.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 25, 2023, 8:59:51 PM10/25/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Tim, Devlin Cronin, Demetrios Papadopoulos, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan.

This change meets the code coverage requirements.

Patch set 26:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Oct 26, 2023, 5:43:07 PM10/26/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Andy Phan uploaded patch set #27 to this change.

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Oct 26, 2023, 6:46:15 PM10/26/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Tim, Devlin Cronin, Demetrios Papadopoulos, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

This change meets the code coverage requirements.

Patch set 27:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Nov 6, 2023, 7:17:23 PM11/6/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Andy Phan uploaded patch set #28 to this change.

View Change

8 files changed, 455 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Nov 6, 2023, 7:59:03 PM11/6/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Andy Phan.

Andy Phan uploaded patch set #29 to this change.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Nov 6, 2023, 9:22:51 PM11/6/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Tim, Devlin Cronin, Demetrios Papadopoulos, Lei Zhang, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

This change meets the code coverage requirements.

Patch set 29:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Lei Zhang (Gerrit)

unread,

Nov 7, 2023, 12:44:40 PM11/7/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Lei Zhang, Tim, Devlin Cronin, findit...@appspot.gserviceaccount.com, Demetrios Papadopoulos, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Attention is currently required from: Andy Phan.

Patch set 29:Code-Review +1

View Change

1 comment:

File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc:
- Patch Set #29, Line 70: content::WebContents::FromRenderFrameHost(render_frame_host()));
  Just use GetSenderWebContents()? Same for line 183.
  Or WDYT about making a helper function that takes a RFH to combine the nearly identical code in the 2 Run() methods in this CL?

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Nov 7, 2023, 1:42:32 PM11/7/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention is currently required from: Andy Phan.

Andy Phan uploaded patch set #30 to this change.

View Change

8 files changed, 440 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Nov 7, 2023, 2:36:00 PM11/7/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Lei Zhang, Tim, Devlin Cronin, findit...@appspot.gserviceaccount.com, Demetrios Papadopoulos, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

View Change

2 comments:

File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc:

- Patch Set #29, Line 70: content::WebContents::FromRenderFrameHost(render_frame_host()));

- Just use GetSenderWebContents()? Same for line 183. […]
  Made into a helper function.
File chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc:

Patch Set #23, Line 111:

  const base::Value::Dict expected =
      base::test::ParseJsonDict(base::StringPrintf(
          kExpectedStreamInfoTemplate, frame_tree_node_id, frame_tree_node_id));

- IMO, this is more direct -- you have your expected JSON string and are comparing directly against th […]
  Thanks for the insight!

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Nov 7, 2023, 2:54:08 PM11/7/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Lei Zhang, Tim, Devlin Cronin, Demetrios Papadopoulos, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

This change meets the code coverage requirements.

Patch set 30:Code-Coverage +1

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Andy Phan (Gerrit)

unread,

Nov 7, 2023, 3:37:38 PM11/7/23

to asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Lei Zhang, Tim, Devlin Cronin, findit...@appspot.gserviceaccount.com, Demetrios Papadopoulos, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org

Patch set 31:Commit-Queue +2

View Change

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Nov 7, 2023, 3:42:55 PM11/7/23

to Andy Phan, asvitkine...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, Lei Zhang, Tim, Devlin Cronin, findit...@appspot.gserviceaccount.com, Demetrios Papadopoulos, Chromium Metrics Reviews, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change



29 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
Insertions: 11, Deletions: 12.

The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
Insertions: 22, Deletions: 36.

The diff is too large to show. Please review the diff.
```

Approvals:
  Devlin Cronin: Looks good to me
  findit...@appspot.gserviceaccount.com: Ok
  Lei Zhang: Looks good to me
  Andy Phan: Commit

[OOPIF PDF] Add PdfViewerPrivate extension API endpoints

Add extension functions GetStreamInfo() and SetPdfPluginAttributes().
These will be used to load the PDF Viewer in OOPIF without a GuestView.

Functionality is very similar to extensions::MimeHandlerServiceImpl's
GetStreamInfo() and SetPdfPluginAttributes(), but instead of using
mime_handler::MimeHandlerService, use an extension API function instead.

pdfViewerPrivate.StreamInfo and pdfViewerPrivate.PdfPluginAttributes are
based on mimeHandlerPrivate.StreamInfo and
mimeHandlerPrivate.PdfPluginAttributes. However, for the PDF viewer, the
mime type and response headers fields in StreamInfo are unused, so don't
include those fields.

Currently, these functions are unused in this CL. Future CLs will make
use of the new API endpoints. See https://crrev.com/c/4691287.

Bug: 1445746
Change-Id: I7b0439a0b65393c5e651dc8e06b0bd21cfe0ad64

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4682380
Commit-Queue: Andy Phan <andy...@chromium.org>
Reviewed-by: Lei Zhang <the...@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
Code-Coverage: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1221164}


---
M chrome/browser/extensions/api/pdf_viewer_private/BUILD.gn
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.cc
M chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api.h
A chrome/browser/extensions/api/pdf_viewer_private/pdf_viewer_private_api_unittest.cc
M chrome/common/extensions/api/pdf_viewer_private.idl
M chrome/test/BUILD.gn
M extensions/browser/extension_function_histogram_value.h
M tools/metrics/histograms/enums.xml
8 files changed, 440 insertions(+), 0 deletions(-)

To view, visit change 4682380. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward