Fix build error with !LOG_DISABLED
Created attachment 334460 [details] Patch
Created attachment 334609 [details] Patch
Created attachment 334610 [details] Patch for landing
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
Created attachment 334611 [details] Patch for landing
Comment on attachment 334611 [details] Patch for landing Clearing flags on attachment: 334611 Committed r229004: <https://trac.webkit.org/changeset/229004>
All reviewed patches have been landed. Closing bug.
<rdar://problem/37897889>
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.
(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)
(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.
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.
(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.