<rdar://problem/88457996>
rdar://88457996
Created attachment 451696 [details] Patch
Created attachment 451698 [details] Patch
Comment on attachment 451698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451698&action=review > Source/WebCore/ChangeLog:12 > + (e.g. loading a blob, hooking with find-in-page messages, etc.). There are other changes in this patch, is it expected? > Source/WebCore/html/PDFDocument.cpp:58 > + PDFDocumentEventListener(PDFDocument& document) explicit. > Source/WebCore/html/PDFDocument.cpp:67 > + PDFDocument& m_document; This is not safe, there is no guarantee m_document is alive during PDFDocumentEventListener lifetime
Created attachment 451703 [details] Patch
Comment on attachment 451703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451703&action=review > Source/WebCore/html/PDFDocument.h:51 > + RefPtr<HTMLIFrameElement> m_iframe; “m_iFrame” I would imagine given the type’s capitalization, but check what we use elsewhere.
(In reply to Tim Horton from comment #6) > Comment on attachment 451703 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451703&action=review > > > Source/WebCore/html/PDFDocument.h:51 > > + RefPtr<HTMLIFrameElement> m_iframe; > > “m_iFrame” I would imagine given the type’s capitalization, but check what > we use elsewhere. I have never seen m_iFrame in our code base. I think we consistently use "iframe".
Comment on attachment 451703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451703&action=review > Source/WebCore/html/PDFDocument.cpp:153 > + m_iframe = iframe.ptr(); m_iframe = WTFMove(iframe) > Source/WebCore/html/PDFDocument.cpp:170 > + auto script = HTMLScriptElement::create(scriptTag, *this, false, false); It is a bit weird to create an element with *this while it will be added to another document. You can probably pass a Document& to injectContentScript from the event handler, in which case no need to store m_frame at all.
Created attachment 451705 [details] Patch
Created attachment 451707 [details] [fast-cq] Patch for landing
(In reply to youenn fablet from comment #8) > Comment on attachment 451703 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451703&action=review > > > Source/WebCore/html/PDFDocument.cpp:153 > > + m_iframe = iframe.ptr(); > > m_iframe = WTFMove(iframe) > > > Source/WebCore/html/PDFDocument.cpp:170 > > + auto script = HTMLScriptElement::create(scriptTag, *this, false, false); > > It is a bit weird to create an element with *this while it will be added to > another document. > You can probably pass a Document& to injectContentScript from the event > handler, in which case no need to store m_frame at all. Done, I've used the contentDocument instead of *this. The m_iframe can't be removed since injectContentScript is called from the event listener. Besides that, it's useful to have it to send messages to the iframe if we need to in the future (for find in page for instance)
Comment on attachment 451707 [details] [fast-cq] Patch for landing whoops not up to date
Created attachment 451708 [details] [fast-cq] Patch for landing
Committed r289627 (247137@main): <https://commits.webkit.org/247137@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451707 [details].
<rdar://problem/88820270>
Comment on attachment 451708 [details] [fast-cq] Patch for landing Welp, fast-cq was faster than me, will address comments manually
> Done, I've used the contentDocument instead of *this. The m_iframe can't be > removed since injectContentScript is called from the event listener. The event listener has a ScriptExecutionContext& which is probably the Document& you want to use. In that case, why would we need m_iframe?
(In reply to youenn fablet from comment #17) > > Done, I've used the contentDocument instead of *this. The m_iframe can't be > > removed since injectContentScript is called from the event listener. > > The event listener has a ScriptExecutionContext& which is probably the > Document& you want to use. > In that case, why would we need m_iframe? Addressed in https://commits.webkit.org/r289630