Address AMO Review Concerns #1118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@pdfjsbot test |
|
Processing command test by user notmasteryet. Queue size: 0 Live script output is available (after queueing is done) at: http://184.73.87.52:8989/3625563.txt [bot:processed:3625563] |
ERROR(s) foundATTENTION: There was a snapshot difference: Output: Bot response time: 26.70 mins |
|
@pdfjsbot lint |
|
Processing command lint by user brendandahl. Queue size: 0 Live script output is available (after queueing is done) at: http://184.73.87.52:8989/3626084.txt [bot:processed:3626084] |
|
All tests passed. Output: Bot response time: 0.96 mins |
|
Current regressions:
Also possible confusion from the user:
|
|
|
||
| component {2278dfd0-b75c-11e0-8257-1ba3d93c9f1a} components/pdfContentHandler.js | ||
| contract @mozilla.org/uriloader/content-handler;1?type=application/pdf {2278dfd0-b75c-11e0-8257-1ba3d93c9f1a} | ||
| contract @mozilla.org/streamconv;1?from=application/pdf&to=*/* {2278dfd0-b75c-11e0-8257-1ba3d93c9f1a} |
There was a problem hiding this comment.
do we need to change GUID since contract is changed?
|
Tested +1 after fixing anchor tags and regressions pointed out by Yury. I also noticed the following (which might get fixed when we fix the Download button): If you open a PDF from e.g. a Google result, then click on the Download button and hit the browser Back button twice, the URL goes back to the original one (e.g. the Google result URL), but the page content still displays the PDF. |
|
I've thought of another possible issue with localStorage. Now its stored based on the domain the pdf is hosted, e.g. there's no longer a centralized store for all the pdf scroll/zoom information. |
|
Everything should be working, however we still need to figure out how to verify the new events are coming from pdf.js which I have yet to figure out because the window.location now points to the pdf url. |
|
Opening in new tab / bookmarking links in the thumbs view does not work. |
| watchWindows(function(window) { | ||
| window.addEventListener(PDFJS_EVENT_ID, messageCallback, false, true); | ||
| unload(function() { | ||
| window.removeEventListener(PDFJS_EVENT_ID, messageCallback, false, true); |
There was a problem hiding this comment.
TODO: Fix this line
|
@pdfjsbot test |
|
Processing command test by user brendandahl. Queue size: 0 Live script output is available (after queueing is done) at: http://184.73.87.52:8989/3692257.txt [bot:processed:3692257] |
|
All tests passed. Output: Bot response time: 27.16 mins |
|
@notmasteryet @arturadib |
| download: function(data) { | ||
| Services.wm.getMostRecentWindow('navigator:browser').saveURL(data); | ||
| }, | ||
| setDatabase: function() { |
There was a problem hiding this comment.
Missed data as parameter
|
The URL navigation glitch is gone! Anchor links are working, including on the sidebar. The addon overall seems to be working superbly here. +1 from me, but since this is a big change let's wait for a +1 from Yury as well. |
notmasteryet
added a commit
that referenced
this pull request
Feb 1, 2012
Address AMO Review Concerns
|
time for submitting it to AMO again? |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
-Removes compatibility.js so we have less warnings.
-Gets rid of all the places we use innerHtml.
-Changes the add-on to use a stream converter instead of a content handler. This has the following pros:
The cons:
I've also stripped out all the private browsing and extension only storage stuff because this is no longer applicable because we run under content privileges.
Please download, build and try out the extension.
TODO: fix the anchor tag links so they show the correct URL. Add the url changes to browser history so the back/forward buttons work on the anchors.