Bug 175854

Summary: Platform code should be able to safely log in release builds
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: WebCore Misc.Assignee: Eric Carlson <eric.carlson>
Status: ASSIGNED ---    
Severity: Normal CC: buildbot, commit-queue, eric.carlson, jer.noble, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch.
none
Proposed patch.
jer.noble: review+
Patch for landing.
commit-queue: commit-queue-
Patch for landing. none

Description Eric Carlson 2017-08-22 15:22:00 PDT
Release logging should not happen in private browsing mode. Page.usesEphemeralSession() is used to check for private browsing mode, but Page isn't accessible from inside of Platform/. Instead of passing "private browsing mode" state to every Platform object that needs to do release logging, create a Logger class that Page can allocate and configure and which can be used inside of Platform/.
Comment 1 Radar WebKit Bug Importer 2017-08-22 15:25:37 PDT
<rdar://problem/34022930>
Comment 2 Eric Carlson 2017-08-22 15:34:00 PDT
Created attachment 318812 [details]
Proposed patch.
Comment 3 Eric Carlson 2017-08-22 15:45:48 PDT
Created attachment 318814 [details]
Proposed patch.
Comment 4 Jer Noble 2017-08-22 16:14:36 PDT
Comment on attachment 318814 [details]
Proposed patch.

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

r=me with maybe a nit, maybe not a nit.

> Source/WebCore/PAL/pal/Logger.h:94
> +    bool enabled() const { return m_enabled; }

nit: I don't know where we (as a project) come down on "bool enabled()" v. "bool isEnabled()".  I could go either way.
Comment 5 Eric Carlson 2017-08-23 10:58:31 PDT
Created attachment 318888 [details]
Patch for landing.
Comment 6 Eric Carlson 2017-08-23 11:00:10 PDT
(In reply to Jer Noble from comment #4)
> Comment on attachment 318814 [details]
> Proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318814&action=review
> 
> r=me with maybe a nit, maybe not a nit.
> 
> > Source/WebCore/PAL/pal/Logger.h:94
> > +    bool enabled() const { return m_enabled; }
> 
> nit: I don't know where we (as a project) come down on "bool enabled()" v.
> "bool isEnabled()".  I could go either way.

It looks like we don't have consensus, there are about twice as many "bool enabled()" as "bool isEnabled()".
Comment 7 Build Bot 2017-08-23 11:04:12 PDT
Attachment 318888 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/Logger.h:105:  The parameter name "channel" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 WebKit Commit Bot 2017-08-23 11:16:07 PDT
Comment on attachment 318888 [details]
Patch for landing.

Rejecting attachment 318888 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
bKit/Tools/TestWebKitAPI/Tests/WebCore/cocoa/WebCoreNSURLSession.mm -o /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPILibrary.build/Objects-normal/x86_64/WebCoreNSURLSession.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPILibrary.build/Objects-normal/x86_64/Logging.o Tests/WebCore/Logging.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/4369505
Comment 9 Eric Carlson 2017-08-23 12:20:34 PDT
Created attachment 318899 [details]
Patch for landing.
Comment 10 WebKit Commit Bot 2017-08-23 13:02:02 PDT
Comment on attachment 318899 [details]
Patch for landing.

Clearing flags on attachment: 318899

Committed r221095: <http://trac.webkit.org/changeset/221095>