Bug 208975 – Add a very verbose logging mode for incremental PDF loading

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Add a very verbose logging mode for incremental PDF loading

Helps to diagnose specific types of behavior we see from the data provider callbacks

[Brady Eidson]

Created attachment 393340 [details]
Patch v1

[Simon Fraser (smfr)]

Comment on attachment 393340 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=393340&action=review

> Source/WebKit/Platform/Logging.h:60
> +    M(IncrementalPDFVerbose) \

Just call it IncrementalPDF?

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:133
> +#ifndef NDEBUG

#if !LOG_DISABLED

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:135
> +    size_t addDataCallbackThread() { return ++m_dataCallbackThreadCount; }

Is this adding a thread? Or just increasing the callback count? Should it be incrementPendingCallbackCount or something?

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:385
> +#ifndef NDEBUG

#if !LOG_DISABLED

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:636
> +#ifndef NDEBUG

#if !LOG_DISABLED

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:646
> +    LOG(PDF, "%s", message.utf8().data());

LOG_WITH_STREAM(PDF, stream << message);

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:648
> +    LOG(IncrementalPDFVerbose, "%s", message.utf8().data());

LOG_WITH_STREAM(IncrementalPDF, stream << message);

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:690
> +    LOG(IncrementalPDFVerbose, "%s", stream.release().utf8().data());

LOG_WITH_STREAM all the pieces at once?

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:698
> +#ifndef NDEBUG

#if !LOG_DISABLED

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:699
> +    Ref<PDFPlugin> debugPluginRef = *((PDFPlugin*)info);

It's a bit nasty of logging changes behavior, like changing object lifetimes.

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:719
> +#ifndef NDEBUG

#if !LOG_DISABLED everywhere.

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:742
> +    debugPluginRef->pdfLog(stream.release());

LOG_WITH_STREAM directly?

[Brady Eidson]

Making most of these changes.

Unfortunately with LOG_WITH_STREAM, you (I think?) have to use the TextStream that the macro creates for you, so complicated streams I build up beforehand are a nonstarter.

[Brady Eidson]

(In reply to Brady Eidson from comment #3)
> Making most of these changes.
> 
> Unfortunately with LOG_WITH_STREAM, you (I think?) have to use the
> TextStream that the macro creates for you, so complicated streams I build up
> beforehand are a nonstarter.

(Note, the reason ::logPDF exists is to log the message to (2) different logging channels, so if somebody builds up a text stream to call into it, LOG_WITH_STREAM  won't work and stream.release() is necessary)

[Brady Eidson]

Created attachment 393342 [details]
PFL

[Brady Eidson]

Created attachment 393385 [details]
PFL

[Brady Eidson]

Created attachment 393450 [details]
PFL

[WebKit Commit Bot]

Comment on attachment 393450 [details]
PFL

Clearing flags on attachment: 393450

Committed r258384: <https://trac.webkit.org/changeset/258384>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60405343>

[Ryosuke Niwa]

Release build fixed in https://trac.webkit.org/changeset/258389.

[Brady Eidson]

(In reply to Ryosuke Niwa from comment #11)
> Release build fixed in https://trac.webkit.org/changeset/258389.

Wonder why the EWS release builders didn't hit that

[Brady Eidson]

(In reply to Brady Eidson from comment #12)
> (In reply to Ryosuke Niwa from comment #11)
> > Release build fixed in https://trac.webkit.org/changeset/258389.
> 
> Wonder why the EWS release builders didn't hit that

(And thanks for the fix!)