-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
[api-major] Remove deprecated code #8982
Conversation
e63e525
to
9babd1f
Compare
/botio-linux preview |
/botio test |
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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:
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. |
There was a problem hiding this comment.
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.
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)? |
I changed the title to include |
Oh, branch is a good idea. |
9babd1f
to
3b82f12
Compare
…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.
3b82f12
to
b651cfb
Compare
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.
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/11e55bdff1bcd9f/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/11e55bdff1bcd9f/output.txt Total script time: 2.41 mins Published |
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/4519ba80065dc7c/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/4abf3680e54d897/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/4abf3680e54d897/output.txt Total script time: 16.71 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/4519ba80065dc7c/output.txt Total script time: 22.74 mins
|
There was a problem hiding this 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 && |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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!
[api-major] Remove deprecated code
This code has been deprecated with a visible message for two years, so we can safely remove it now.