Bug 236510 – Inject custom styles into PDF.js to make it look like PDFKit

• Status: RESOLVED
• Resolution: FIXED
• Component: PDF

[Tim Nguyen (:ntim)]

<rdar://problem/88457996>

[Tim Nguyen (:ntim)]

rdar://88457996

[Tim Nguyen (:ntim)]

Created attachment 451696 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 451698 [details]
Patch

[youenn fablet]

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

[Tim Nguyen (:ntim)]

Created attachment 451703 [details]
Patch

[Tim Horton]

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.

[Chris Dumez]

(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".

[youenn fablet]

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.

[Tim Nguyen (:ntim)]

Created attachment 451705 [details]
Patch

[Tim Nguyen (:ntim)]

Created attachment 451707 [details]
[fast-cq] Patch for landing

[Tim Nguyen (:ntim)]

(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)

[Tim Nguyen (:ntim)]

Comment on attachment 451707 [details]
[fast-cq] Patch for landing

whoops not up to date

[Tim Nguyen (:ntim)]

Created attachment 451708 [details]
[fast-cq] Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/88820270>

[Tim Nguyen (:ntim)]

Comment on attachment 451708 [details]
[fast-cq] Patch for landing

Welp, fast-cq was faster than me, will address comments manually

[youenn fablet]

> 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?

[Tim Nguyen (:ntim)]

(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