Conversation
src/pdf.js
Outdated
There was a problem hiding this comment.
Why have a variable here? Everything leaks to the global name space, so I don't see the benefits of this one.
There was a problem hiding this comment.
Everything leaks to the global name space
What do you mean by "everything"? Did you see the self-executing function below?
There was a problem hiding this comment.
The prototypes like TilingPattern are just about getting added to window object, right? So what's the win of using a window.PDF object and place things on it later again?
There was a problem hiding this comment.
So what's the win of using a window.PDF object and place things on it later again?
I guess this question was asked before you noticed the global wrapper?
There was a problem hiding this comment.
Is it likely that this name might conflict another variable in a page where pdf.js is included? Would it be better to call it PDFJS instead?
There was a problem hiding this comment.
good idea. PDFJS also makes it easier to search for API calls across files.
|
@arturadib, sorry, I've missed reading the comments :(
The From my own experience, I spend quite some time in the debugger, so we should make sure that interaction is as easy/speedy as possible. For Bespin/Skywriter, the build process ripped out all the |
Makefile
Outdated
There was a problem hiding this comment.
This sed-command does not seem to be doing anything unless my sed-fu is completely off. Could you explain what the intention is for this line? Btw in git bash there is no option -E. There is option -e.
There was a problem hiding this comment.
Looks like gnu sed and bsd sed aren't the same. -E is interpret regular expression as extended(modern) regular expression. For our purposes we probably don't need that though.
There was a problem hiding this comment.
I generally use -E as a default rule as I use modern regular expressions. It's not needed here so I removed it.
By "link" do you mean So if I understand you correctly, in Bespin by default all development is done by including |
Yes. You can take a look at it here: http://hg.mozilla.org/labs/bespin/file/d498b93b0d28/frontend/index.html# Basically, the build script replaced the |
|
@arturadib mentioned on IRC that jquery uses the one file for development as well. As far as I can tell from looking at the test/index.hml, it seems like they use the non-build version for testing as well: https://github.com/jquery/jquery/blob/master/test/index.html#L11 |
|
@jviereck Please check out the updated pull request text. Dev mode is the default now. Also, can you please download this branch and test it out (including enabling workers)? If you want you can test Thanks! |
|
New devmode seems to be working fine for me. Extension, web, and workers work also. |
|
@pdfjsbot test |
|
Processing command test by user jviereck. Queue size: 0 Live script output is available (after queueing is done) at: http://184.73.87.52:8989/2527108.txt [bot:processed:2527108] |
|
ERROR(s) found Output: Bot response time: 0.76 mins |
|
Cloning the branch locally: Julian-Vierecks-MacBook-Pro:pdf_arturadib jviereck$ make test
cd test; \
python test.py --reftest \
--browserManifestFile=resources/browser_manifests/browser_manifest.json \
--manifestFile=test_manifest.json
Traceback (most recent call last):
File "test.py", line 579, in
main()
File "test.py", line 566, in main
browsers = setUp(options)
File "test.py", line 321, in setUp
assert not ANAL or os.path.isfile('../build/pdf.js') and os.path.isdir('../.git')
AssertionError
make: *** [browser-test] Error 1
|
Do we have to do that by hand everytime? If yes can we find something smart to avoid that? |
Makefile
Outdated
|
Globally this sounds good to me and I agree that we need to merge that soon (and thanks for not requiring to install anything else for packaging the files :)) |
I guess you answered that yourself already :) |
src/pdf.js
Outdated
There was a problem hiding this comment.
Would it be good to name this function? Like (function executeSelfPdfJs() {?
There was a problem hiding this comment.
Sure, pdfjsWrapper
|
Looks great! Thanks :) |
Conflicts: pdf.js
|
@pdfjsbot test |
|
Processing command test by user arturadib. Queue size: 0 Live script output is available (after queueing is done) at: http://184.73.87.52:8989/2534765.txt [bot:processed:2534765] |
|
All tests passed. Output: Bot response time: 29.20 mins |
|
I mostly agree with all this split, so let's do it right now to not block to much on that. |
Everything should be working, including the worker stuff.
Major topics:
src/build/pdf.js.Usemake pdfjsto generate the file, andmake watchto monitor file changes and regenerate target automatically. (Only dependency ismakeandpython, already required by our tests).worker.js.pdf.jsdetects its environment (master vs. worker) and initializes appropriatelyglobal object(UPDATE: the global namespace is nowPDFPDFJS). Everything else is wrapped under a self-executing functionwindow[]hack has been modified to use the objectShadingsThis was done with @brendandahl - if you guys could take a look at this relatively soon that would be helpful, otherwise it might get gitrotten quickly.
Thanks!
UPDATE:
Changing things a bit after discussion with Julian.
make production. The following (comment) tags found insrc/pdf.jsandweb/viewer.htmlwill instruct the script to do its magic:PDFJSSCRIPT_INCLUDE_ALL,PDFJSSCRIPT_INCLUDE_BUILD, andPDFJSSCRIPT_REMOVE, which will respectively cat-combine allsrc/*.jsfiles,<script>-includebuild/pdf.js, and remove the corresponding line.make watch, as this is now unnecessarymake productiondepends onsed, but that's OK since contributors don't have to mess with the production script.Some time in the near future the bot will periodically issue
make productionand update ourgh-pages.@cgjones @vingtetun @notmasteryet @jviereck @wfwalker