| | <<<Back 1 day (to 2016/08/31) | 20160901 |
| sebras | tor8: ok. so what commits on tor/master are ready for review? | 12:57.51 |
| | all? | 12:57.52 |
| tor8 | sebras: pushFloat, pushInt, etc, could be private or protected | 12:59.45 |
| sebras | tor8: hm. you called the pdf_array_len() length in js. I'm using .size() in JNI is that ok? | 13:00.06 |
| tor8 | sebras: should be; the JS and Java use different approaches to standard arrays | 13:00.33 |
| | JS has .length as a property, not as a method call | 13:00.42 |
| sebras | ok. | 13:00.44 |
| tor8 | in java we need a method, and .size() is what Array, List, Vector, etc use | 13:00.56 |
| sebras | I'll use private for the time being. as I have done with put*() and get*(). | 13:01.03 |
| tor8 | sebras: apart from that public/private issue, all on sebras-gh LGTM | 13:01.21 |
| | I see that there are still a few "if (!text || !cs) return;" and similar lines throughout | 13:01.52 |
| | did we want to turn those into IllegalArgument exceptions? | 13:02.05 |
| | ah, you are doing that in sebras-wip | 13:02.39 |
| | sebras: so to be clear, it's sebras-gh/master that LGTM | 13:03.10 |
| | sebras: in use-ioexception, doesn't GetByteArrayElements failure already throw an exception? | 13:04.50 |
| sebras | tor8: let me check. | 13:17.02 |
| tor8 | sebras: on tor/master, up to and including "Always use glyph..." could use a review | 13:17.33 |
| sebras | tor8: I agree with you about GetByteArrayElements. I realized that in the wip commit. | 13:18.01 |
| | tor8: I wonder if I should keep the "... already threw exception" comments. they are somewhat helpful as reminders to myself. | 13:18.34 |
| | tor8: push/length LGTM. | 13:22.04 |
| | tor8: 0xFFFD LGTM too. | 13:23.51 |
| | tor8: pdfextract + pdf_copy_dict also LGTM. | 13:26.30 |
| tor8 | sebras: personally I wouldn't bother | 13:28.42 |
| sebras | tor8: is Simplify PDF resource caching table handling the commit that's supposed to fix the doc->resources issue we discussed the other day? | 13:28.43 |
| tor8 | with the "already thow" comments | 13:28.55 |
| sebras | tor8: ok. | 13:29.08 |
| tor8 | sebras: yes, and also goes a bit further in reducing the number of mallocs and simplifies the code structure | 13:29.25 |
| sebras | tor8: I'm surprised to see block comment just prior to pdf_find_font_resource(). I didn't know your fingers could write those java-style comments this long after uni. :) | 13:32.54 |
| tor8 | sebras: look a bit higher ... I just moved an existing block comment; ) | 13:34.01 |
| sebras | oh.... now it all makes sense. | 13:34.21 |
| | tor8: minor nit: you don't need to check for non-NULL before calling pdf_drop_obj() in res_table_free(). | 13:35.20 |
| | tor8: I can't fault the revamped resource handling, so LGTM. | 13:39.13 |
| tor8 | sebras: true. but also a case of moving an existing bit of code in the file :) | 13:40.24 |
| | one I should fix though, now that you mention it. | 13:40.25 |
| sebras | tor8: streaming loading/opening seems alright too. | 13:47.21 |
| | I like that there is so much less code now. :) | 13:47.48 |
| | tor8: with the new call to ft_name_index() you don't set etable[i] == 0 if glyph == 0. this seems intentional, but it is a difference from before. | 13:51.11 |
| | tor8: I have yet to realize whether it is benign. | 13:51.24 |
| tor8 | sebras: it is intentional. we don't overwrite etable if no glyph was found. | 13:51.56 |
| | it results in a few progressions, and a few digressions (changes that are still wrong, but no worse than before) | 13:52.23 |
| | we start by initializing etable to the default builtin font encoding | 13:52.44 |
| | then we override individual entries using glyph names from the /Encoding differences array (which is stored in estrings[]) | 13:53.06 |
| | the problems occur when using substitute fonts, which may not always have the requested glyph names | 13:53.52 |
| sebras | tor8: looks sensible to me. | 14:12.07 |
| | tor8: and re: my patches you advice me to remove the comments and use private and possibly work the wip-part into the existing patches? | 14:12.39 |
| | tor8: in ad964e6 I also break the big fz_try() blocks into smaller parts. | 14:15.05 |
| tor8 | sebras: remove the repetitive comment notes and use private and the ones on sebras-gh/master are good to go | 14:18.31 |
| | no need to work the 'check arguments' commit into these, that can come later | 14:18.48 |
| | fz_convert_color also LGTM and can go in | 14:20.05 |
| sebras | tor8: the handling NULL colorspace thing causes issues though. | 14:22.31 |
| | tor8: hrm... I should have noted that you were to break the JNI with that pdf_load_stream() patch... | 14:42.46 |
| | tor8: I tried to adapt the JNI code to the new api, but now I get "object of range; xref size 0"... crap. | 14:57.27 |
| | I broke something. | 14:57.30 |
| | tor8: ok, so it is something in my wip patch that broke. good. then sebras/master has been updated. I want the final LGTM. | 15:03.40 |
| tor8 | sebras: move the "Update to new pdf_load_stream" commit to the beginning? | 15:05.52 |
| | all the rest on sebras/master LGTM | 15:06.24 |
| sebras | tor8: done and pushed. | 15:10.36 |
| | Forward 1 day (to 2016/09/02)>>> | |
IRC Logs