Bug 183049 – Fix build error with !LOG_DISABLED

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Charlie Turner]

Fix build error with !LOG_DISABLED

[Charlie Turner]

Created attachment 334460 [details]
Patch

[Charlie Turner]

Created attachment 334609 [details]
Patch

[Charlie Turner]

Created attachment 334610 [details]
Patch for landing

[WebKit Commit Bot]

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

[Charlie Turner]

Created attachment 334611 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 334611 [details]
Patch for landing

Clearing flags on attachment: 334611

Committed r229004: <https://trac.webkit.org/changeset/229004>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37897889>

[Darin Adler]

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.

[Charlie Turner]

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

[Darin Adler]

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

[Charlie Turner]

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.

[Darin Adler]

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