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

Enable linting of Firefox specific code in viewer.js #5966

Merged
merged 1 commit into from
Apr 27, 2015
Merged

Enable linting of Firefox specific code in viewer.js #5966

merged 1 commit into from
Apr 27, 2015

Conversation

Snuffleupagus
Copy link
Collaborator

  • This patch uncomments a large portion of the Firefox specific code in viewer.js, by using a preprocessor "trick", to enable linting. Doing this actually uncovered some issues, e.g. variables defined multiple times.
  • This patch also fixes a spelling error, adobe pdfwritter -> abobe pdfwriter, in the KNOWN_GENERATORS list used when reporting telemetry data in Firefox.

Please note: there's still some Firefox specific code that is commented out, but it's usually just one (or two) lines of code. Hence the risk that errors creep in should be much lower, compared to entire code-blocks.

@timvandermeij
Copy link
Contributor

Doing this actually uncovered some issues, e.g. variables defined multiple times.

Are these also addressed in this patch or are these changes only to enable linting?

@Snuffleupagus
Copy link
Collaborator Author

Are these also addressed in this patch or are these changes only to enable linting?

Yes. And sorry about not mentioning that, since I now see the potential for confusion!
Off the top of my head, the lint warnings were about: viewer.js#L1046 (variable declaration) and viewer.js#L602 (missing braces).

The /* jshint -W027 */ comments were only added to disable lint warnings about unreachable code after return statements.

var versionId = String(info.PDFFormatVersion).slice(-1) | 0;
var generatorId = 0;
var KNOWN_GENERATORS = [
'acrobat distiller', 'acrobat pdfwritter', 'adobe livecycle',
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're here, can be change acrobat pdfwritter to acrobat pdfwriter (i.e., one t)? If you can mention that change in the commit description, then it's fine to do it in this commit I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely great catch, thank you!
This should explain why the telemetry stats are empty for bucket 2, see e.g. the telemetry data for Firefox 36 and the enumeration reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, I hadn't even noticed that. I have already updated the wiki page (where the typo was also visible), so if we fix it here too than that problem is also resolved.

 - This patch uncomments a large portion of the Firefox specific code in viewer.js, by using a preprocessor "trick", to enable linting. Doing this actually uncovered some issues, e.g. variables defined multiple times.

 - This patch also fixes a spelling error, `adobe pdfwritter` -> `abobe pdfwriter`, in the `KNOWN_GENERATORS` list used when reporting telemetry data in Firefox.

*Please note:* there's still some Firefox specific code that is commented out, but it's usually just one (or two) lines of code. Hence the risk that errors creep in should be much lower, compared to entire code-blocks.
@Snuffleupagus
Copy link
Collaborator Author

I've confirmed, with the built addon, that fips197.pdf from the test suite now reports the correct result for PDF_VIEWER_DOCUMENT_GENERATOR in about:telemetry.

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/5c34e967260ea5d/output.txt

timvandermeij added a commit that referenced this pull request Apr 27, 2015
…code

Enable linting of Firefox specific code in viewer.js
@timvandermeij timvandermeij merged commit c91f736 into mozilla:master Apr 27, 2015
@Snuffleupagus Snuffleupagus deleted the lint-viewer-preprocesser-code branch April 27, 2015 18:05
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.

3 participants