Document: handle indirect objects in `MediaBox` and `CropBox` entries #7873

timvandermeij · 2016-12-05T23:54:52Z

Snuffleupagus

To be totally honest, I'm imagining a much simpler and more general solution here. (Since this issue may happen again in the future if support for other "array" parameters are added).
My thinking is something like master...Snuffleupagus:issue-7872 instead, and a reduced test-case ~~should be~~ is easy to create here.
Should I submit a PR with that change, and a test-case included?

timvandermeij · 2016-12-06T21:37:58Z

I think that's a nice solution as well. Good point! The only thing I think makes sense is to create the CropBox getter as in this PR because it brings it more in line with the other page properties (most notably the MediaBox getter that we already have, and the recently added UserUnit). It will also make it easier to expose via e.g., the API later on, and it makes the view getter less cluttered with crop box sanitizing code it shouldn't really need to worry about.

That being said, I do like your solution of not including the for loops in each getter and amending the getInteritedPageProp method instead (and of course the reduced test case!). How about updating this PR with two commits, the first one being your commit (that fixes the issue) and the second one being my commit without the for loops and length variables (for additional clean-up)? My commit will then be much shorter, but we would still have the getter/name improvements.

Snuffleupagus · 2016-12-07T10:26:29Z

The only thing I think makes sense is to create the CropBox getter as in this PR because it brings it more in line with the other page properties (most notably the MediaBox getter that we already have, and the recently added UserUnit). It will also make it easier to expose via e.g., the API later on, and it makes the view getter less cluttered with crop box sanitizing code it shouldn't really need to worry about.

That's a fair point, but I've got one small reservation: This would require us to always compute the intersection, since then get cropBox() will always be defined. Could we skip doing that if cropBox === mediaBox to avoid what will often be a useless Util.intersect call? If so, I'm OK with it :-)

timvandermeij · 2016-12-07T20:52:47Z

Yes, I think that should be possible. I'll update the PR soon.

… fetch (and dereference) Arrays, and use that for the `MediaBox`/`CropBox` getters (issue 7872)

Snuffleupagus

Thank you!
r=me, with nits, assuming that the tests pass of course!

Edit: Obviously I cannot review my own patch, but yours look good :-)

Snuffleupagus · 2016-12-08T21:35:18Z

src/core/document.js

-      return shadow(this, 'view', cropBox);
+
+      var intersection = Util.intersect(cropBox, mediaBox);
+      return shadow(this, 'view', !intersection ? mediaBox : intersection);


Nit: It would probably suffice to just check intersection || mediaBox here, instead of the ternary condition.

Fixed in the new commit.

Snuffleupagus · 2016-12-08T21:36:25Z

src/core/document.js

        return shadow(this, 'view', mediaBox);
      }
-      return shadow(this, 'view', cropBox);
+


Nit: I'm not sure if we need this newline for readability, let's make the diff smaller instead :-)

Fixed in the new commit.

This patch refactors the `CropBox` code to combine fetching and validation code in a getter, like we already did for the `MediaBox` property. Combined with variable name changes, this improves readability of the code and makes the `view` getter simpler as well.

timvandermeij · 2016-12-08T21:49:35Z

/botio-linux preview

pdfjsbot · 2016-12-08T21:49:36Z

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/ee96f18edb328f3/output.txt

pdfjsbot · 2016-12-08T21:51:40Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/ee96f18edb328f3/output.txt

Total script time: 2.06 mins

Published

timvandermeij · 2016-12-08T21:55:09Z

/botio test

pdfjsbot · 2016-12-08T21:55:10Z

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/970733f8e65554e/output.txt

pdfjsbot · 2016-12-08T21:55:10Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/9c80ac6c851058d/output.txt

pdfjsbot · 2016-12-08T22:21:38Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/970733f8e65554e/output.txt

Total script time: 26.47 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2016-12-08T22:21:42Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/9c80ac6c851058d/output.txt

Total script time: 26.55 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

timvandermeij · 2016-12-08T22:24:53Z

/botio makeref

pdfjsbot · 2016-12-08T22:24:54Z

From: Bot.io (Linux)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/599d47f295aedb8/output.txt

pdfjsbot · 2016-12-08T22:24:54Z

From: Bot.io (Windows)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/2bcf17ccfb2af69/output.txt

pdfjsbot · 2016-12-08T22:51:00Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/2bcf17ccfb2af69/output.txt

Total script time: 26.10 mins

Lint: Passed
Make references: Passed
Check references: Passed

timvandermeij · 2016-12-08T22:59:41Z

Even though the message has not been posted, the Linux bot finished successfully. Merging with r=me for @Snuffleupagus' commit. Thanks!

…ndirect Document: handle indirect objects in `MediaBox` and `CropBox` entries

timvandermeij added the core label Dec 5, 2016

timvandermeij assigned Snuffleupagus Dec 5, 2016

timvandermeij force-pushed the mediabox-cropbox-indirect branch 2 times, most recently from bca1768 to ade0700 Compare December 6, 2016 00:04

Snuffleupagus requested changes Dec 6, 2016

View reviewed changes

Add a parameter to Page_getInheritedPageProp to make it possible to…

9be3aee

… fetch (and dereference) Arrays, and use that for the `MediaBox`/`CropBox` getters (issue 7872)

timvandermeij force-pushed the mediabox-cropbox-indirect branch from ade0700 to e99e225 Compare December 8, 2016 21:28

Snuffleupagus approved these changes Dec 8, 2016

View reviewed changes

timvandermeij force-pushed the mediabox-cropbox-indirect branch from e99e225 to 3800b5e Compare December 8, 2016 21:46

timvandermeij merged commit 47f03b6 into mozilla:master Dec 8, 2016

timvandermeij deleted the mediabox-cropbox-indirect branch December 8, 2016 22:59

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#7873 from timvandermeij/mediabox-cropbox-i…

197f0b5

…ndirect Document: handle indirect objects in `MediaBox` and `CropBox` entries

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document: handle indirect objects in `MediaBox` and `CropBox` entries #7873

Document: handle indirect objects in `MediaBox` and `CropBox` entries #7873

timvandermeij commented Dec 5, 2016 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

timvandermeij commented Dec 6, 2016 •

edited

Loading

Snuffleupagus commented Dec 7, 2016

timvandermeij commented Dec 7, 2016

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Dec 8, 2016

timvandermeij Dec 8, 2016

Snuffleupagus Dec 8, 2016

timvandermeij Dec 8, 2016

timvandermeij commented Dec 8, 2016

pdfjsbot commented Dec 8, 2016

pdfjsbot commented Dec 8, 2016

timvandermeij commented Dec 8, 2016

pdfjsbot commented Dec 8, 2016

pdfjsbot commented Dec 8, 2016

pdfjsbot commented Dec 8, 2016

pdfjsbot commented Dec 8, 2016

timvandermeij commented Dec 8, 2016

pdfjsbot commented Dec 8, 2016

pdfjsbot commented Dec 8, 2016

pdfjsbot commented Dec 8, 2016

timvandermeij commented Dec 8, 2016 •

edited

Loading

Document: handle indirect objects in MediaBox and CropBox entries #7873

Document: handle indirect objects in MediaBox and CropBox entries #7873

Conversation

timvandermeij commented Dec 5, 2016 • edited Loading

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

timvandermeij commented Dec 6, 2016 • edited Loading

Snuffleupagus commented Dec 7, 2016

timvandermeij commented Dec 7, 2016

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Dec 8, 2016

Choose a reason for hiding this comment

timvandermeij Dec 8, 2016

Choose a reason for hiding this comment

Snuffleupagus Dec 8, 2016

Choose a reason for hiding this comment

timvandermeij Dec 8, 2016

Choose a reason for hiding this comment

timvandermeij commented Dec 8, 2016

pdfjsbot commented Dec 8, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 8, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij commented Dec 8, 2016

pdfjsbot commented Dec 8, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 8, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 8, 2016

From: Bot.io (Linux)

Success

pdfjsbot commented Dec 8, 2016

From: Bot.io (Windows)

Success

timvandermeij commented Dec 8, 2016

pdfjsbot commented Dec 8, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 8, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 8, 2016

From: Bot.io (Windows)

Success

timvandermeij commented Dec 8, 2016 • edited Loading

Document: handle indirect objects in `MediaBox` and `CropBox` entries #7873

Document: handle indirect objects in `MediaBox` and `CropBox` entries #7873

timvandermeij commented Dec 5, 2016 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

timvandermeij commented Dec 6, 2016 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

timvandermeij commented Dec 8, 2016 •

edited

Loading