Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS -- Add 'util' object #12530

Merged
merged 1 commit into from
Nov 6, 2020
Merged

JS -- Add 'util' object #12530

merged 1 commit into from
Nov 6, 2020

Conversation

calixteman
Copy link
Contributor

This patch provides an implementation of the util object as described:

src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed everything from the top of the file until _getTZ so far, so this is the first set of comments. It's quite a lot of code and requires careful matching with the spec, so I'll do it in chunks. Nice work so far!

src/scripting_api/util.js Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now reviewed everything up until scand.

src/scripting_api/util.js Outdated Show resolved Hide resolved
K: data => {
return (1 + ((data.hours + 23) % 24)).toString();
},
mm: data => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's conflicting with he MM for month... I don't really know how to handle that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to check how acrobat (last version) is dealing with that stuff and astonishingly it doesn't handle the 3rd argument (i.e. bXFAPicture), so it's quite confusing.

Copy link
Contributor

@timvandermeij timvandermeij Nov 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Acrobat isn't handling it at all and there is confusion in the specification, I'd really like to remove it completely from this patch (i.e, remove _printdXFAPicture and its call, but keep the bXFAPicture in printd for API consistency, just don't do anything with it and add a comment why we ignore it). It will be more consistent with Acrobat (our reference implementation) and that avoids us from filling in any blanks we can't verify. Moreover, I don't think this functionality is actually used often, and it would make the overall patch considerable smaller. It can always be added again in a follow-up PR.

src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
src/scripting_api/util.js Outdated Show resolved Hide resolved
@calixteman calixteman force-pushed the js_utils branch 4 times, most recently from 6ee443a to 5513a4e Compare November 2, 2020 16:00
@calixteman
Copy link
Contributor Author

Do you know why CI is broken ?

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Nov 2, 2020

Do you know why CI is broken ?

Because the new files aren't included in the lib/lib-es5 builds; the following should fix it:

diff --git a/gulpfile.js b/gulpfile.js
index a8344793c..71446553a 100644
--- a/gulpfile.js
+++ b/gulpfile.js
@@ -1273,9 +1273,9 @@ function buildLib(defines, dir) {
   return merge([
     gulp.src(
       [
-        "src/{core,display,shared}/*.js",
+        "src/{core,display,scripting_api,shared}/*.js",
         "!src/shared/{cffStandardStrings,fonts_utils}.js",
-        "src/{pdf,pdf.worker}.js",
+        "src/{pdf,pdf.scripting,pdf.worker}.js",
       ],
       { base: "src/" }
     ),

@brendandahl
Copy link
Contributor

/botio-linux unittest

@pdfjsbot
Copy link

pdfjsbot commented Nov 6, 2020

From: Bot.io (Linux m4)


Received

Command cmd_unittest from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/754d1514ec9104a/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 6, 2020

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/754d1514ec9104a/output.txt

Total script time: 3.50 mins

  • Unit Tests: FAILED

@brendandahl
Copy link
Contributor

/botio-linux unittest

@pdfjsbot
Copy link

pdfjsbot commented Nov 6, 2020

From: Bot.io (Linux m4)


Received

Command cmd_unittest from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/08682f62953f841/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 6, 2020

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/08682f62953f841/output.txt

Total script time: 3.53 mins

  • Unit Tests: Passed

@brendandahl brendandahl merged commit 018fd43 into mozilla:master Nov 6, 2020
@timvandermeij
Copy link
Contributor

Looks good to me too. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants