Closed Bug 1015115 Opened 8 years ago Closed 8 years ago

Switch to cloneInto in PDFjs

Categories

(Firefox :: PDF Viewer, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [https://github.com/mozilla/pdf.js/pull/4858] p=2 s=it-32c-31a-30b.3 [qa-])

Attachments

(1 file)

43 bytes, text/x-github-pull-request
yury
: review+
Details | Review
This is faster, more readable, and net code reduction. Patch coming up.
Flags: firefox-backlog+
Attached file Github PR
This passes the m-bc tests as well as pdfjs' unit tests. I didn't run the reference tests because I don't think this change will have any effect on them.
Attachment #8427712 - Flags: review?(ydelendik)
Whiteboard: https://github.com/mozilla/pdf.js/pull/4830
I actually have a similar patch to this code in bug 1015380. As far as I can tell, |detail| is a content object, so I don't understand why we need to expose it to content at all. Can someone enlighten me?
(In reply to Bobby Holley (:bholley) from comment #2)
> I actually have a similar patch to this code in bug 1015380. As far as I can
> tell, |detail| is a content object, so I don't understand why we need to
> expose it to content at all. Can someone enlighten me?

I would assume this to be the case for the sync case but not the async one?
(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Bobby Holley (:bholley) from comment #2)
> > I actually have a similar patch to this code in bug 1015380. As far as I can
> > tell, |detail| is a content object, so I don't understand why we need to
> > expose it to content at all. Can someone enlighten me?
> 
> I would assume this to be the case for the sync case but not the async one?

(I've updated the pull request to do the same as your patch from bug 1015380 for the sync case - this doesn't seem to break any of the mochitest-browser tests for pdfjs, so I presume it's OK.)
Marco, can you add this to the iteration backlog please?
Flags: needinfo?(mmucci)
Whiteboard: https://github.com/mozilla/pdf.js/pull/4830 → p=2 https://github.com/mozilla/pdf.js/pull/4858
Added to Iteration 32.3
Flags: needinfo?(mmucci)
Whiteboard: p=2 https://github.com/mozilla/pdf.js/pull/4858 → [https://github.com/mozilla/pdf.js/pull/4858] p=2 s=it-32c-31a-30b.3 [qa?]
Attachment #8427712 - Flags: review?(ydelendik) → review+
Depends on: 1017133
Landed in bug 1017133.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Whiteboard: [https://github.com/mozilla/pdf.js/pull/4858] p=2 s=it-32c-31a-30b.3 [qa?] → [https://github.com/mozilla/pdf.js/pull/4858] p=2 s=it-32c-31a-30b.3 [qa-]
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.