Bug 183049

Summary: Fix build error with !LOG_DISABLED
Product: WebKit Reporter: Charlie Turner <cturner>
Component: New BugsAssignee: Charlie Turner <cturner>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, dbates, esprehn+autocc, ews-watchlist, kangil.han, pnormand, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description Charlie Turner 2018-02-22 11:33:11 PST
Fix build error with !LOG_DISABLED
Comment 1 Charlie Turner 2018-02-22 11:34:49 PST
Created attachment 334460 [details]
Patch
Comment 2 Charlie Turner 2018-02-26 04:03:48 PST
Created attachment 334609 [details]
Patch
Comment 3 Charlie Turner 2018-02-26 04:21:19 PST
Created attachment 334610 [details]
Patch for landing
Comment 4 WebKit Commit Bot 2018-02-26 04:22:52 PST
Comment on attachment 334610 [details]
Patch for landing

Rejecting attachment 334610 [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-02', 'validate-changelog', '--check-oops', '--non-interactive', 334610, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/6674187
Comment 5 Charlie Turner 2018-02-26 04:32:37 PST
Created attachment 334611 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2018-02-26 05:07:46 PST
Comment on attachment 334611 [details]
Patch for landing

Clearing flags on attachment: 334611

Committed r229004: <https://trac.webkit.org/changeset/229004>
Comment 7 WebKit Commit Bot 2018-02-26 05:07:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2018-02-26 05:08:19 PST
<rdar://problem/37897889>
Comment 9 Darin Adler 2018-02-26 18:26:30 PST
Comment on attachment 334611 [details]
Patch for landing

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

> Source/WebCore/dom/messageports/MessagePortChannel.h:64
>      String logString() const { return makeString(m_ports[0].logString(), ":", m_ports[1].logString()); }

This should be ':' instead of ":" for slightly better performance.
Comment 10 Charlie Turner 2018-02-27 02:40:50 PST
(In reply to Darin Adler from comment #9)
> Comment on attachment 334611 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334611&action=review
> 
> > Source/WebCore/dom/messageports/MessagePortChannel.h:64
> >      String logString() const { return makeString(m_ports[0].logString(), ":", m_ports[1].logString()); }
> 
> This should be ':' instead of ":" for slightly better performance.

Can place this into a separate issue if it's causing you performance regressions; this bug wasn't touching that line, I wanted to keep it specific to the build fix rather than speculative performance improvements on my end (I don't have any benchmarking infrastructure for such things)
Comment 11 Darin Adler 2018-02-28 14:40:19 PST
(In reply to Charlie Turner from comment #10)
> Can place this into a separate issue

Sure that would be fine. But you really don’t have to do anything.

> if it's causing you performance regressions

That seems like a strange turn of phrase. Definitely no "regression" here.

I was just commenting about best practices, as I do whenever I see code near code being reviewed, something I’ve always done through the whole lifetime of the WebKit project.

I understand this may be your first time seeing it.

> this bug wasn't touching that line, I wanted to keep it
> specific to the build fix rather than speculative performance improvements
> on my end (I don't have any benchmarking infrastructure for such things)

Yes, wasn’t criticizing what you have done.

I was pointing out for anyone looking at this bug who cares to learn best practices, including but not limited to you, that this happens to use a slightly less efficient idiom.
Comment 12 Charlie Turner 2018-03-01 06:06:19 PST
Hi Darin,

(In reply to Darin Adler from comment #11)
> > if it's causing you performance regressions
> 
> That seems like a strange turn of phrase. Definitely no "regression" here.
> 
> I was just commenting about best practices, as I do whenever I see code near
> code being reviewed, something I’ve always done through the whole lifetime
> of the WebKit project.
> 
> I understand this may be your first time seeing it.

I appreciate it :). I assumed you wanted it changing with this patch, but now I know better.

I never know what the rdar bug importer is doing, and assumed perhaps Apple was running performance testing builds that flagged it with LOG_DISABLED=0 in release.
Comment 13 Darin Adler 2018-03-01 09:03:09 PST
(In reply to Charlie Turner from comment #12)
> I never know what the rdar bug importer is doing

The "bug importer" just registers the bug in Apple’s internal bug system. It doesn’t generally mean anything to us working on the WebKit project except that Apple plans to track this bug as it is fixed and the fix is integrated. I wish there was a simple way of annotating WebKit bugs that was less distracting since it has no significance for people outside Apple.

There could be some kind of discussion of a bug inside Apple that is not visible in bugs.webkit.org because there are people at Apple who work on WebKit who sit next to each other and they do talk to each other. And there are *occasional* subjects discussed in email and Apple’s internal bug system, but generally seeing the "bug importer" thing on a bug does not indicate anything going on for a particular bug.