Closed Bug 1372113 Opened 3 years ago Closed 3 years ago

Stop finding the function symbols in PDFiumEngineShim

Categories

(Core :: Printing: Output, enhancement)

Unspecified
Windows
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: fatseng, Assigned: fatseng)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Blocks: pdf-printing
Depends on: 1368948
Assignee: nobody → fatseng
Status: NEW → ASSIGNED
I thought this bug would be convered as one of the patches in bug 1370488 to refactor PDFiumEngineShim(or ababdon) as we are going to integrate PDFium into libxul so that there is not necessary to have shim layer between PDFium api callee and caller.

Do you have other intention behind?

nit: please always leave a description or user story to indicate the reason this bug exists.
The reason of keeping shim layer is that we can verify PDFium.dll built from Chromium easier.
Comment on attachment 8877489 [details]
Bug 1372113 - Call PDFium function directly in PDFiumEngineShim

PDfium would be built into the xul (bug 1368948) so PDFiumEngineShim could
call functions directly. However, I still keep loading dynamic library for
debugging purpose.

In this patch, I remove redundant define and typedef which are written in PDFium head file and include an appropriate head file from PDFium.
Attachment #8877489 - Flags: feedback?(brsun)
Comment on attachment 8877489 [details]
Bug 1372113 - Call PDFium function directly in PDFiumEngineShim

Hi Farmer, the idea to keep shim layer for debugging purpose is good. Share some idea in my mind:
 - Use other flag then DEBUG for testing with pdfium.dll. Maybe a new definition is required.
 - Handle the whole loading and unloading stuffs of pdfium.dll purely inside PDFiumEngineShim. Maybe a new preference is required for the file path.
 - Let PDFViaEMFPrintHelper to call PDFiumEngineShim::InitLibrary and PDFiumEngineShim::DestroyLibrary as the normal usage of PDFium.
 - Don't load symbols inside PDFiumEngineShim::LoadDocument or PDFiumEngineShim::LoadMemDocument. PDFiumEngineShim::InitLibrary might be a more proper place to load pdfium.dll library and necessary symbols.
 - Don't destroy the library inside PDFiumEngineShim::CloseDocument. (You've done this in your patch already. This comment is just for completing the whole story)
 - Use a variable for reference counting in PDFiumEngineShim. Add one to the counter when PDFiumEngineShim::InitLibrary is called. Minus one to the counter when PDFiumEngineShim::DestroyLibrary is called. Call FPDF_InitLibrary and/or FPDF_DestroyLibrary only when the counter reaches zero.
 - Use the singleton pattern to retrieve the instance of PDFiumEngineShim. Hide the constructor of PDFiumEngineShim in private scope.
Attachment #8877489 - Flags: feedback?(brsun)
Probably using NS_INLINE_DECL_REFCOUNTING or NS_INLINE_DECL_THREADSAFE_REFCOUNTING can save your efforts while dealing with the reference counter.
Attachment #8877489 - Flags: feedback?(brsun)
Comment on attachment 8877489 [details]
Bug 1372113 - Call PDFium function directly in PDFiumEngineShim

https://reviewboard.mozilla.org/r/148940/#review154354

::: widget/windows/PDFiumEngineShim.cpp:19
(Diff revision 2)
> -  , mFPDF_InitLibrary(nullptr)
> +/* static */
> +already_AddRefed<PDFiumEngineShim>
> +PDFiumEngineShim::GetInstanceOrNull()
> +{
> +  if (!sPDFiumEngineShim) {
> +    sPDFiumEngineShim = new PDFiumEngineShim();

Suggest to use a smart pointer to store the newly allocated instance from the very beginning; and then let sPDFiumEngineShim store the address afterward.

::: widget/windows/PDFiumEngineShim.cpp:50
(Diff revision 2)
> -  MOZ_ASSERT(mPRLibrary);
>  }
>  
>  PDFiumEngineShim::~PDFiumEngineShim()
>  {
> +  if (mFPDF_DestroyLibrary) {

Suggest to use mInitialized as the checking here to sync the style of other call flows.

::: widget/windows/PDFiumEngineShim.cpp:54
(Diff revision 2)
>  {
> +  if (mFPDF_DestroyLibrary) {
> +    mFPDF_DestroyLibrary();
> +  }
> +
> +  if (sPDFiumEngineShim) {

nit: This checking is a little redundant.

::: widget/windows/PDFiumEngineShim.cpp:86
(Diff revision 2)
> +
>    mFPDF_InitLibrary = (FPDF_InitLibrary_Pfn)PR_FindFunctionSymbol(
>      mPRLibrary, "FPDF_InitLibrary");
>    if (!mFPDF_InitLibrary) {
>      return false;
>    }

Use NS_ENSURE_TRUE for checking here and other places as well.

::: widget/windows/PDFiumEngineShim.cpp:142
(Diff revision 2)
> +  mFPDF_CloseDocument = (FPDF_CloseDocument_Pfn) FPDF_CloseDocument;
> +  mFPDF_GetPageCount = (FPDF_GetPageCount_Pfn) FPDF_GetPageCount;
> +  mFPDF_GetPageSizeByIndex = (FPDF_GetPageSizeByIndex_Pfn) FPDF_GetPageSizeByIndex;
> +  mFPDF_LoadPage = (FPDF_LoadPage_Pfn) FPDF_LoadPage;
> +  mFPDF_ClosePage = (FPDF_ClosePage_Pfn) FPDF_ClosePage;
> +  mFPDF_RenderPage = (FPDF_RenderPage_Pfn) FPDF_RenderPage;

Suggest to remove non-used PDFium APIs which the printing feature will not use.
Comment on attachment 8877489 [details]
Bug 1372113 - Call PDFium function directly in PDFiumEngineShim

It looks good to me.
Attachment #8877489 - Flags: feedback?(brsun) → feedback+
Comment on attachment 8877489 [details]
Bug 1372113 - Call PDFium function directly in PDFiumEngineShim

https://reviewboard.mozilla.org/r/148940/#review155154

::: commit-message-97f34:8
(Diff revision 4)
> +1.PDfium would be built into the xul (bug 1368948) so PDFiumEngineShim could
> +call functions directly. However, I still keep loading dynamic library for
> +debugging purpose. Create a new preference for storing the library file path.
> +2.Remove redundant define and typedef which are written in PDFium head file.
> +3.Include an appropriate head file from PDFium.
> +4.Remove non-used PDFium APIs.

Seems like at least this part could have been a separate patch. It's better to break stuff into smaller logical chunks where it makes sense, but don't worry about it this time.
Attachment #8877489 - Flags: review?(jwatt) → review+
Depends on: 1378608
Pushed by fatseng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07255610c658
Call PDFium function directly in PDFiumEngineShim r=jwatt
Duplicate of this bug: 1358985
Duplicate of this bug: 1349139
https://hg.mozilla.org/mozilla-central/rev/07255610c658
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.