[api-minor] Change the "dc:creator" Metadata field to an Array #12838

calixteman · 2021-01-08T17:43:29Z

add scripting support for doc.info.authors
doc.info.metadata is the raw string with xml code
aims to fix issue Doc.info in scripting API being currently implemented is not spec-compliant #12619

Snuffleupagus

This is a breaking API-change, as evident by the updated unit-tests; please note that it's not correct to just change an existing API in this way!

calixteman · 2021-01-08T18:02:42Z

What's the correct way to break an api ?
Do you prefer I let dc:creator as it worked and so I can add a new property authors for example ?
Anyway according to xmp spec it seems that dc:creator is expected to be an array.

Snuffleupagus · 2021-01-09T11:13:44Z

Anyway according to xmp spec it seems that dc:creator is expected to be an array.

Can you please link relevant section of the XMP specification here?
If we're explicitly breaking an existing specification, I suppose that changes things slightly as far as I'm concerned.

Given that what we currently do is apparently wrong, the question here I suppose is how often we expect that the Metadata is used by third-party users and how badly we'd break them if we simply made the change?

Thinking about this a bit, my hunch would be that the Metadata probably isn't depended upon a whole lot in practice; hence we might be OK with this small breaking changing if we clearly label it as such (so that it's visible in the release notes).
I.e. you should update the PR title, and commit message, to start with something like:
[api-minor] Change the "dc:creator" Metadata field to an Array; add scripting support for doc.info.authors

/cc @timvandermeij What's your opinion, can we change the existing "dc:creator" Metadata field in this way?

calixteman · 2021-01-09T11:23:51Z

Anyway according to xmp spec it seems that dc:creator is expected to be an array.

Can you please link relevant section of the XMP specification here?

https://wwwimages2.adobe.com/content/dam/acom/en/devnet/xmp/pdfs/XMP%20SDK%20Release%20cc-2016-08/XMPSpecificationPart1.pdf#page=33
According to this doc, dc:creator should be a Seq but I got a Bag using Acrobat DC to generate the pdf file in this patch.

If we're explicitly breaking an existing specification, I suppose that changes things slightly as far as I'm concerned.

Given that what we currently do is apparently wrong, the question here I suppose is how often we expect that the Metadata is used by third-party users and how badly we'd break them if we simply made the change?

Thinking about this a bit, my hunch would be that the Metadata probably isn't depended upon a whole lot in practice; hence we might be OK with this small breaking changing if we clearly label it as such (so that it's visible in the release notes).
I.e. you should update the PR title, and commit message, to start with something like:
[api-minor] Change the "dc:creator" Metadata field to an Array; add scripting support for doc.info.authors

+1

/cc @timvandermeij What's your opinion, can we change the existing "dc:creator" Metadata field in this way?

Snuffleupagus

We might be OK with changing the format of "dc:creator", see #12838 (comment)

However, I've also added a couple of comments on the overall implementation here.

Snuffleupagus · 2021-01-09T11:18:11Z

src/display/api.js

@@ -2705,6 +2705,7 @@ class WorkerTransport {
      .then(results => {
        return {
          info: results[0],
+          rawMetadata: results[1],


Can you please remove this property, and instead do something like this:

Add this._data = data; after line

pdf.js/src/display/metadata.js

Line 34 in c0a6d6c

}

Add a new method, e.g.
getRaw() { return this._data; }
after line

pdf.js/src/display/metadata.js

Line 126 in c0a6d6c

}

Snuffleupagus · 2021-01-09T11:20:15Z

src/display/metadata.js

-        if (desc.childNodes[j].nodeName.toLowerCase() !== "#text") {
-          const entry = desc.childNodes[j];
+        const entry = desc.childNodes[j];
+        if (entry.nodeName.toLowerCase() !== "#text") {


Given the overall indentation level and the size of the code below, things would be slightly more readable with this condition inverted like so:

Suggested change

if (entry.nodeName.toLowerCase() !== "#text") {

if (entry.nodeName.toLowerCase() === "#text") {

continue;

}

Snuffleupagus · 2021-01-09T11:21:53Z

web/app.js

@@ -1765,6 +1767,7 @@ const PDFViewerApplication = {
      return; // The document was closed while the metadata resolved.
    }
    this.documentInfo = info;
+    this.rawMetadata = rawMetadata;


With the suggested changes in src/display/api.js and src/display/metadata.js, this is no longer needed.

Snuffleupagus · 2021-01-09T11:22:35Z

web/app.js

@@ -1655,7 +1655,8 @@ const PDFViewerApplication = {
          baseURL: this.baseUrl,
          filesize: this._contentLength,
          filename: this._docFilename,
-          metadata: this.metadata,
+          metadata: this.rawMetadata,


With the suggested changes in src/display/api.js and src/display/metadata.js, this needs to be

Suggested change

metadata: this.rawMetadata,

metadata: this.metadata?.getRaw(),

Snuffleupagus · 2021-01-09T11:24:44Z

web/app.js

@@ -1655,7 +1655,8 @@ const PDFViewerApplication = {
          baseURL: this.baseUrl,
          filesize: this._contentLength,
          filename: this._docFilename,
-          metadata: this.metadata,
+          metadata: this.rawMetadata,
+          authors: this.metadata.get("authors") || null,


Note that this.metadata can be null, hence this can throw!
Furthermore, the get method already fallback to returning null.

All in all, this line should read

Suggested change

authors: this.metadata.get("authors") || null,

authors: this.metadata?.get("authors"),

timvandermeij · 2021-01-09T11:50:49Z

If I'm reading https://wwwimages2.adobe.com/content/dam/acom/en/devnet/xmp/pdfs/XMP%20SDK%20Release%20cc-2016-08/XMPSpecificationPart1.pdf#page=33 correctly, dc:creator should indeed be a list (even though the name implies otherwise, since I'd have expected dc:creators in that case, but it's not the first time this happens in the spec), according to the "XMP usage is a list of creators" part. While it's indeed an API change, the behavior before was incorrect, so it's more like a bugfix to me, hence I'm OK with this change but only if it's clearly indicated as [api-minor].

Snuffleupagus

With the final comments addressed, this looks good to me :-)

Snuffleupagus · 2021-01-11T19:35:03Z

src/display/metadata.js

+    const nodeName = rdf ? rdf.nodeName : null;
    if (!rdf || nodeName !== "rdf:rdf" || !rdf.hasChildNodes()) {


With the lower-casing now happening in the XML-parser, you can inline the nodeName check here instead:

Suggested change

const nodeName = rdf ? rdf.nodeName : null;

if (!rdf || nodeName !== "rdf:rdf" || !rdf.hasChildNodes()) {

if (!rdf || rdf.nodeName !== "rdf:rdf" || !rdf.hasChildNodes()) {

Snuffleupagus · 2021-01-11T19:39:54Z

test/test_manifest.json

+    {  "id": "js-authors",
+       "file": "pdfs/js-authors.pdf",
+       "md5": "e892f290ae209286a29d3c17dfeab226",
+       "rounds": 1,
+       "type": "eq"
+    },


How much value does this eq test bring, since the functionality that you're adding here doesn't really show up in a reference-test as far as I can tell?

- add scripting support for doc.info.authors - doc.info.metadata is the raw string with xml code

calixteman · 2021-01-11T20:34:42Z

/botio integrationtest

pdfjsbot · 2021-01-11T20:34:43Z

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.67.70.0:8877/662eeeee2cf4552/output.txt

pdfjsbot · 2021-01-11T20:34:44Z

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://3.101.106.178:8877/198062624320a24/output.txt

pdfjsbot · 2021-01-11T20:37:45Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/662eeeee2cf4552/output.txt

Total script time: 3.01 mins

Integration Tests: Passed

pdfjsbot · 2021-01-11T20:38:33Z

From: Bot.io (Windows)

Success

Full output at http://3.101.106.178:8877/198062624320a24/output.txt

Total script time: 3.81 mins

Integration Tests: Passed

calixteman requested a review from brendandahl January 8, 2021 17:43

calixteman force-pushed the authors branch from e630956 to 2722df1 Compare January 8, 2021 17:49

Snuffleupagus requested changes Jan 8, 2021

View reviewed changes

timvandermeij added the core label Jan 8, 2021

calixteman force-pushed the authors branch from 2722df1 to 79fb2da Compare January 9, 2021 10:23

Snuffleupagus requested changes Jan 9, 2021

View reviewed changes

calixteman force-pushed the authors branch from 79fb2da to 342bc9b Compare January 11, 2021 12:34

calixteman changed the title ~~JS -- support doc.info.authors~~ [api-minor] Change the "dc:creator" Metadata field to an Array Jan 11, 2021

Snuffleupagus approved these changes Jan 11, 2021

View reviewed changes

[api-minor] Change the "dc:creator" Metadata field to an Array

43d5512

- add scripting support for doc.info.authors - doc.info.metadata is the raw string with xml code

calixteman force-pushed the authors branch from 342bc9b to 43d5512 Compare January 11, 2021 20:34

calixteman merged commit 1de1ae0 into mozilla:master Jan 12, 2021

calixteman deleted the authors branch January 12, 2021 10:45

Snuffleupagus mentioned this pull request Feb 14, 2021

Multiple authors squished together in dc: creator field #12990

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api-minor] Change the "dc:creator" Metadata field to an Array #12838

[api-minor] Change the "dc:creator" Metadata field to an Array #12838

calixteman commented Jan 8, 2021 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

calixteman commented Jan 8, 2021

Snuffleupagus commented Jan 9, 2021

calixteman commented Jan 9, 2021 •

edited

Loading

Snuffleupagus left a comment

Snuffleupagus Jan 9, 2021

Snuffleupagus Jan 9, 2021

Snuffleupagus Jan 9, 2021

Snuffleupagus Jan 9, 2021

Snuffleupagus Jan 9, 2021

timvandermeij commented Jan 9, 2021

Snuffleupagus left a comment

Snuffleupagus Jan 11, 2021

Snuffleupagus Jan 11, 2021

calixteman commented Jan 11, 2021

pdfjsbot commented Jan 11, 2021

pdfjsbot commented Jan 11, 2021

pdfjsbot commented Jan 11, 2021

pdfjsbot commented Jan 11, 2021

-        if (entry.nodeName.toLowerCase() !== "#text") {
+        if (entry.nodeName.toLowerCase() === "#text") {
+          continue;
+        }

	metadata: this.rawMetadata,
	metadata: this.metadata?.getRaw(),

	authors: this.metadata.get("authors") \|\| null,
	authors: this.metadata?.get("authors"),

		const nodeName = rdf ? rdf.nodeName : null;
		if (!rdf \|\| nodeName !== "rdf:rdf" \|\| !rdf.hasChildNodes()) {

	const nodeName = rdf ? rdf.nodeName : null;
	if (!rdf \|\| nodeName !== "rdf:rdf" \|\| !rdf.hasChildNodes()) {
	if (!rdf \|\| rdf.nodeName !== "rdf:rdf" \|\| !rdf.hasChildNodes()) {

[api-minor] Change the "dc:creator" Metadata field to an Array #12838

[api-minor] Change the "dc:creator" Metadata field to an Array #12838

Conversation

calixteman commented Jan 8, 2021 • edited Loading

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

calixteman commented Jan 8, 2021

Snuffleupagus commented Jan 9, 2021

calixteman commented Jan 9, 2021 • edited Loading

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Jan 9, 2021

Choose a reason for hiding this comment

Snuffleupagus Jan 9, 2021

Choose a reason for hiding this comment

Snuffleupagus Jan 9, 2021

Choose a reason for hiding this comment

Snuffleupagus Jan 9, 2021

Choose a reason for hiding this comment

Snuffleupagus Jan 9, 2021

Choose a reason for hiding this comment

timvandermeij commented Jan 9, 2021

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Jan 11, 2021

Choose a reason for hiding this comment

Snuffleupagus Jan 11, 2021

Choose a reason for hiding this comment

calixteman commented Jan 11, 2021

pdfjsbot commented Jan 11, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jan 11, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Jan 11, 2021

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jan 11, 2021

From: Bot.io (Windows)

Success

calixteman commented Jan 8, 2021 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

calixteman commented Jan 9, 2021 •

edited

Loading