-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Remove the Firefox extension building code. #9566
Conversation
I think that after this lands we also need to update both bots so that links to the extension are no longer in the previews, update the wiki and check if any add-on issues can be closed or redirected upstream. Firefox still maintains its own version of the extension upstream, which is embedded in each Firefox release, right? (It will probably become a WebExtension anyway, and we also have an open issue for that here too.) |
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.
While I completely understand, and agree with, the reasoning behind this PR there's one important caveat to consider: Note how we're currently handling preferences in the extension and, most importantly, the mozilla-central code.
Currently we're simply bundling https://github.com/mozilla/pdf.js/blob/master/web/default_preferences.json into the relevant files using the preprocessor; see https://github.com/mozilla/pdf.js/pull/9566/files#diff-fac62551eadfe79f141900988b8e8a8dL80 and https://github.com/mozilla/pdf.js/pull/9566/files#diff-c64446a1f7e4337255356cd285b15ca5L33.
This (currently) means that if any changes are made to the default_preferences.json
file, those are automatically mirrored to the relevant mozilla-central files.
If we completely remove this code from the GitHub repo it would have the effect of making any preference changes a two-step process, where you'd need to also submit a mozilla-central patch simultaneously for the preference changes.
Edit: Please see PR #9571 for one possible way of addressing these issues.
7bae399
to
c0d4ccd
Compare
Yeah. |
@Snuffleupagus good to go now? |
We probably need to do one more mozilla-central update first, to sync the changes from PR #9571, but after that we should be good here :-) |
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.
(It will probably become a WebExtension anyway, and we also have an open issue for that here too.)
That is #8533.
README.md
Outdated
+ Please note that the extension is *not* guaranteed to be compatible with Firefox versions that are *older* than the current Nightly version, see the [Release Calendar](https://wiki.mozilla.org/RapidRelease/Calendar#Past_branch_dates). | ||
|
||
+ The extension *may* also work in Seamonkey, provided that it is based on a Firefox version as above (see [Which version of Firefox does SeaMonkey 2.x correspond with?](https://wiki.mozilla.org/SeaMonkey/FAQ#General)), but we do *not* guarantee compatibility. | ||
PDF.js is built into version 19+ of Firefox |
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.
Add a period at the end.
gulpfile.js
Outdated
preprocessJS(FIREFOX_CONTENT_DIR + 'PdfjsChromeUtils.jsm', defines, true) | ||
.pipe(replace(/\bPDFJSSCRIPT_PREF_PREFIX\b/g, MOZCENTRAL_PREF_PREFIX)) | ||
.pipe(gulp.dest(MOZCENTRAL_CONTENT_DIR)), | ||
.pipe(gulp.dest(MOZCENTRAL_CONTENT_DIR)) |
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.
Add a comma at the end.
c0d4ccd
to
1c3986a
Compare
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.
LGTM: I think we need to remove that from gulp web
(https://github.com/brendandahl/pdf.js/blob/1c3986a8e21ef8d1204fe0d8f465a4a51f03599c/gulpfile.js#L1071) and remove extension signing from botio
c6e4270
to
17bdad0
Compare
Firefox no longer supports this legacy extension and it is a pain to sync changes from here and mozilla central.
17bdad0
to
121e436
Compare
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.
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @brendandahl received. Current queue size: 0 Live output at: http://54.67.70.0:8877/3ee839408e98f19/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/3ee839408e98f19/output.txt Total script time: 2.31 mins Published |
Remove the Firefox extension building code.
… files. r=pdfjs-reviewers,calixte The `PREF_PREFIX` handling is a left-over from years ago, back when we built an *old-style* Firefox extension in the PDF.js project. Given that this was removed over five years ago, see mozilla/pdf.js#9566, we should be able to slightly simplify preference names now. Differential Revision: https://phabricator.services.mozilla.com/D181370
… files. r=pdfjs-reviewers,calixte The `PREF_PREFIX` handling is a left-over from years ago, back when we built an *old-style* Firefox extension in the PDF.js project. Given that this was removed over five years ago, see mozilla/pdf.js#9566, we should be able to slightly simplify preference names now. Differential Revision: https://phabricator.services.mozilla.com/D181370
Firefox no longer supports this legacy extension and it is a pain to
sync changes from here and mozilla central.
There's still a bit more cleanup we could do for locale's, but I'd like to leave that for a follow up bug.