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.

Sign up for GitHub

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

Jump to bottom

Remove the FIREFOX build flag, since it's completely unused and simplify a couple of PDFJSDev checks #11489

Merged
merged 2 commits into from
Jan 24, 2020
Merged

Remove the FIREFOX build flag, since it's completely unused and simplify a couple of PDFJSDev checks #11489

merged 2 commits into from
Jan 24, 2020

Conversation

Snuffleupagus
Copy link
Collaborator

  • Remove the FIREFOX build flag, since it's completely unused

    After PR 9566, which removed all of the old Firefox extension code, the FIREFOX build flag is no longer used for anything.
    It thus seems to me that it should be removed, for a couple of reasons:

    • It's simply dead code now, which only serves to add confusion when looking at the PDFJSDev calls.
    • It used to be that MOZCENTRAL and FIREFOX was almost always used together. However, ever since PR 9566 there's obviously been no effort put into keeping the FIREFOX build flags up to date.
    • In the event that a new, Webextension based, Firefox addon is created in the future you'd still need to audit all MOZCENTRAL (and possibly CHROME) build flags to see what'd make sense for the addon.

  • Simplify, and tweak, a couple of PDFJSDev checks

    This removes a couple of, thanks to preceeding code, unnecessary typeof PDFJSDev checks, and also fixes a couple of incorrectly implemented (my fault) checks intended for TESTING builds.

@Snuffleupagus
Copy link
Collaborator Author

/botio-linux test

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2020

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/5f06766655d1407/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2020

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/5f06766655d1407/output.txt

Total script time: 19.20 mins

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

Image differences available at: http://54.67.70.0:8877/5f06766655d1407/reftest-analyzer.html#web=eq.log

@timvandermeij
Copy link
Contributor

/botio-windows test

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2020

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/79117f5d7c2a4db/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2020

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/79117f5d7c2a4db/output.txt

Total script time: 2.13 mins

  • Font tests: FAILED
  • Unit tests: FAILED
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/79117f5d7c2a4db/reftest-analyzer.html#web=eq.log

@Snuffleupagus Snuffleupagus force-pushed the rm-FIREFOX-define branch 3 times, most recently from 1767785 to ff4a3c2 Compare January 12, 2020 22:03
@Snuffleupagus
Copy link
Collaborator Author

I should probably split out the third commit into a separate PR, since it's not directly relevant to the other changes.

While I briefly considered simply removing the pre-processor checks altogether, since doing so wouldn't break anything, that might look too out-of-place in non-MOZCENTRAL builds.

However, I'm still on the fence about the above since running the telemetry code in other builds shouldn't break anything now. Obviously it's not necessary in e.g. GENERIC builds, but it'd help ensure that this important code is better tested. @timvandermeij Opinions?

@timvandermeij
Copy link
Contributor

If the pre-processor checks don't do anything because the telemetry reporting in GENERIC build is replaced by a dummy method, I'm fine with removing them to simplify the code. I do wonder what we actually gain in terms of testing coverage because we would essentially be checking if calls to a dummy method can be made locally, correct? Actually testing the telemetry code locally is not really possible without a proper mock I think...

@Snuffleupagus
Copy link
Collaborator Author

I do wonder what we actually gain in terms of testing coverage because we would essentially be checking if calls to a dummy method can be made locally, correct?

That's correct, but my (perhaps unnecessary) idea was that running e.g. this and this code in more builds wouldn't be a bad idea!?

@timvandermeij
Copy link
Contributor

I see. That sounds reasonable so we at least cover that code.

@Snuffleupagus
Remove the FIREFOX build flag, since it's completely unused
7322a24 
After PR 9566, which removed all of the old Firefox extension code, the `FIREFOX` build flag is no longer used for anything.
It thus seems to me that it should be removed, for a couple of reasons:
 - It's simply dead code now, which only serves to add confusion when looking at the `PDFJSDev` calls.
 - It used to be that `MOZCENTRAL` and `FIREFOX` was *almost* always used together. However, ever since PR 9566 there's obviously been no effort put into keeping the `FIREFOX` build flags up to date.
 - In the event that a new, Webextension based, Firefox addon is created in the future you'd still need to audit all `MOZCENTRAL` (and possibly `CHROME`) build flags to see what'd make sense for the addon.
@Snuffleupagus
Simplify, and tweak, a couple of PDFJSDev checks
a399435 
This removes a couple of, thanks to preceeding code, unnecessary `typeof PDFJSDev` checks, and also fixes a couple of incorrectly implemented (my fault) checks intended for `TESTING` builds.
@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/ced65788429f9e8/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/ced65788429f9e8/output.txt

Total script time: 1.75 mins

Published

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

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/fd65f259152967c/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/026689fcf5053e5/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/026689fcf5053e5/output.txt

Total script time: 25.93 mins

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

Image differences available at: http://54.215.176.217:8877/026689fcf5053e5/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/fd65f259152967c/output.txt

Total script time: 60.00 mins

@Snuffleupagus
Copy link
Collaborator Author

/botio-linux test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/8b3dea5a1e9e926/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/8b3dea5a1e9e926/output.txt

Total script time: 19.39 mins

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

Image differences available at: http://54.67.70.0:8877/8b3dea5a1e9e926/reftest-analyzer.html#web=eq.log

@timvandermeij timvandermeij merged commit d2d9441 into mozilla:master Jan 24, 2020
@timvandermeij
Copy link
Contributor

Good clean-up!

@Snuffleupagus Snuffleupagus deleted the rm-FIREFOX-define branch January 24, 2020 23:14
IsaacSchemm added a commit to IsaacSchemm/pdf.js-seamonkey that referenced this pull request May 8, 2021
@IsaacSchemm
Revert "Merge pull request mozilla#11489 from Snuffleupagus/rm-FIREFO…
41f33c5 
…X-define"

This reverts commit d2d9441, reversing
changes made to 9791d7e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timvandermeij timvandermeij timvandermeij approved these changes

Assignees
No one assigned
Labels
core other viewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Snuffleupagus @pdfjsbot @timvandermeij