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

[api-major] Remove deprecated code #8982

Merged
merged 5 commits into from Oct 3, 2017

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Oct 1, 2017

This code has been deprecated with a visible message for two years, so we can safely remove it now.

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@timvandermeij
Copy link
Contributor Author

/botio test

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Looks good to me, with the comment addressed (and sign-off from other devs).

@yurydelendik Are you OK with removing these deprecated API methods at this point?

@@ -933,12 +933,6 @@ var PDFPageProxy = (function PDFPageProxyClosure() {
intentState.renderTasks.push(internalRenderTask);
var renderTask = internalRenderTask.task;

// Obsolete parameter support
if (params.continueCallback) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this code is removed, then the following JSDoc comment should also be removed:

pdf.js/src/display/api.js

Lines 759 to 762 in 50b1a91

* @property {function} continueCallback - (deprecated) A function that will be
* called each time the rendering is paused. To continue
* rendering call the function that is the first argument
* to the callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the new commit.

@yurydelendik
Copy link
Contributor

yurydelendik commented Oct 2, 2017

Since we are trying to stick to the http://semver.org/, it's actually "[api-major]" label. Means, we need to start planning 2.0. Can we open "2.0 release" project and place this there (along with removal typed array stubs)?

@timvandermeij timvandermeij changed the title [api-minor] Remove deprecated code [api-major] Remove deprecated code Oct 2, 2017
@timvandermeij
Copy link
Contributor Author

I changed the title to include [api-major]. Should we create a separate branch for 2.0 or can we just merge this in the current master?

@yurydelendik
Copy link
Contributor

Oh, branch is a good idea.

@timvandermeij timvandermeij added this to In progress in Version 2.0 Oct 2, 2017
@timvandermeij timvandermeij changed the base branch from master to version-2.0 October 2, 2017 20:46
…e API

This is deprecated since January 2015 with a visible message, so we can
safely remove this now.
This is deprecated since October 2015 with a visible message, so we can
safely remove this now.
This is deprecated since November 2015 with a visible message, so we
can safely remove this now.
This is deprecated since May 2017 with a visible message, so we can
safely remove this now.
This is deprecated since October 2015 with a visible message, so we can
safely remove this now.
@mozilla mozilla deleted a comment from pdfjsbot Oct 2, 2017
@mozilla mozilla deleted a comment from pdfjsbot Oct 2, 2017
@mozilla mozilla deleted a comment from pdfjsbot Oct 2, 2017
@mozilla mozilla deleted a comment from pdfjsbot Oct 2, 2017
@mozilla mozilla deleted a comment from pdfjsbot Oct 2, 2017
@mozilla mozilla deleted a comment from pdfjsbot Oct 2, 2017
@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Oct 2, 2017

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/11e55bdff1bcd9f/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Oct 2, 2017

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/11e55bdff1bcd9f/output.txt

Total script time: 2.41 mins

Published

@timvandermeij
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Oct 2, 2017

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/4519ba80065dc7c/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Oct 2, 2017

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/4abf3680e54d897/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Oct 2, 2017

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/4abf3680e54d897/output.txt

Total script time: 16.71 mins

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

@pdfjsbot
Copy link

pdfjsbot commented Oct 2, 2017

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/4519ba80065dc7c/output.txt

Total script time: 22.74 mins

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

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Looks good to me (just one perhaps off-topic question); nice code-removal, thank you!

params.nativeImageDecoderSupport = params.nativeImageDecoderSupport ||
(params.disableNativeImageDecoder === true ? NativeImageDecoding.NONE :
NativeImageDecoding.DECODE);
NativeImageDecoding.DECODE;
if (params.nativeImageDecoderSupport !== NativeImageDecoding.DECODE &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could probably be considered slightly out of scope, but I have to ask.

Do we really need to print a warning message for this particular API parameter, considering that we don't do that for any other ones (and doing it everywhere would require adding a lot of code all over the place)?

Hence I'm wondering if it wouldn't suffice to just replace the current code with something along the following lines?

if (!params.nativeImageDecoderSupport ||
    !(params.nativeImageDecoderSupport === NativeImageDecoding.DECODE ||
      params.nativeImageDecoderSupport === NativeImageDecoding.NONE ||
      params.nativeImageDecoderSupport === NativeImageDecoding.DISPLAY)) {
  params.nativeImageDecoderSupport = NativeImageDecoding.DECODE;
}

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 would personally be fine with that, but let's do that in a different PR because I'd like to keep this one limited to only removing deprecated code. Nevertheless, this is a good point and can be picked up later!

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

Successfully merging this pull request may close these issues.

None yet

4 participants