Widget annotation: implement field name according to the specification #7775
Conversation
| expect(data.annotationType).toEqual(AnnotationType.WIDGET); | ||
| expect(data.fieldName).toEqual('foo.bar.baz'); | ||
|
|
||
| firstParent = secondParent = null; |
There was a problem hiding this comment.
Nit: since these are both local variables, I don't think that you actually need to bother nulling them out.
There was a problem hiding this comment.
Fixed in the new commit.
| // them should be provided, bad PDF generators may fail to do so. | ||
| if (!dict.has('T') && !dict.has('Parent')) { | ||
| warn('Unknown field name, falling back to empty field name.'); | ||
| return ''; |
There was a problem hiding this comment.
I completely agree that it's better to just ignore these bad PDF files for now (since we don't have any broken examples to test with)!
The original code is difficult to read and, more importantly, performs actions that are not described in the specification. It replaces empty names with a backtick and an index, but this behavior is not described in the specification. While the specification is not entirely clear about what should happen in this case, it does specify that the `T` field is optional and that multiple field dictionaries may have the same fully qualified name, so to achieve this it makes the most sense to ignore missing `T` fields during construction of the field name. This is the most specification-compliant solution and, judging by opened issue mozilla#6623, also the required and expected behavior.
timvandermeij
force-pushed
the
widget-annotation-name
branch
from
6a2c312
to
1d96854
Compare
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/c62c382c53f5d3e/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/c62c382c53f5d3e/output.txt Total script time: 2.77 mins Published |
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/da3f363f87e9b0b/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/4147403e7cbaf46/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/da3f363f87e9b0b/output.txt Total script time: 25.61 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/4147403e7cbaf46/output.txt Total script time: 37.84 mins
|
…name Widget annotation: implement field name according to the specification
The original code is difficult to read and, more importantly, performs actions that are not described in the specification. It replaces empty names with a backtick and an index, but this behavior is not described in the specification. While the specification is not entirely clear about what should happen in this case, it does specify that the
Tfield is optional and that multiple field dictionaries may have the same fully qualified name, so to achieve this it makes the most sense to ignore missingTfields during construction of the field name. This is the most specification-compliant solution and, judging by opened issue #6623, also the required and expected behavior.Fixes #6623.
Part of #7613.