46710 2010-09-28 03:26:38 -0700 [Chromium] Linking error due to wrong guard in WebIDBTransaction.h 2010-09-28 08:24:28 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 0 andreip webkit-unassigned fishd jorlow oldest_to_newest 286149 0 andreip 2010-09-28 03:26:38 -0700 [Chromium] Linking error due to wrong guard in WebIDBTransaction.h 286152 1 69034 andreip 2010-09-28 03:32:09 -0700 Created attachment 69034 Patch 286154 2 69034 jorlow 2010-09-28 03:36:09 -0700 Comment on attachment 69034 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69034&action=review r=me > WebKit/chromium/public/WebIDBTransaction.h:65 > + // FIXME: this is never called in the renderer. Find a cleaner solution. Comments that apply to the whole function really seem like they should go right above the function not in them. Is there other precedent for something like this? If not, can you please stop doing this? Also, try not to make the comments Chromium architecture specific. Maybe just say that this is only meant to be called from within WebCore? 286155 3 jorlow 2010-09-28 03:36:23 -0700 +fishd cause this is a webkit patch 286156 4 andreip 2010-09-28 03:40:34 -0700 (In reply to comment #2) > (From update of attachment 69034 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69034&action=review > > r=me > > > WebKit/chromium/public/WebIDBTransaction.h:65 > > + // FIXME: this is never called in the renderer. Find a cleaner solution. > > Comments that apply to the whole function really seem like they should go right above the function not in them. Is there other precedent for something like this? If not, can you please stop doing this? > OK ok :) > Also, try not to make the comments Chromium architecture specific. Maybe just say that this is only meant to be called from within WebCore? Erm, this bug has the [Chromium] label and the patch touches only a file in /WebKit/chromium so how can making a chromium-specific comment possibly be wrong? 286157 5 jorlow 2010-09-28 03:49:08 -0700 (In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 69034 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=69034&action=review > > > > r=me > > > > > WebKit/chromium/public/WebIDBTransaction.h:65 > > > + // FIXME: this is never called in the renderer. Find a cleaner solution. > > > > Comments that apply to the whole function really seem like they should go right above the function not in them. Is there other precedent for something like this? If not, can you please stop doing this? > > > > OK ok :) > > > Also, try not to make the comments Chromium architecture specific. Maybe just say that this is only meant to be called from within WebCore? > > Erm, this bug has the [Chromium] label and the patch touches only a file in /WebKit/chromium so how can making a chromium-specific comment possibly be wrong? WebKit code should not depend on Chromium code. Even if this code is designed to be consumed by Chromium. Plus Darin said so. :-) 286159 6 andreip 2010-09-28 03:52:59 -0700 Committed r68504: <http://trac.webkit.org/changeset/68504> 286161 7 andreip 2010-09-28 03:54:34 -0700 (In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #2) > > > (From update of attachment 69034 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=69034&action=review > > > > > > r=me > > > > > > > WebKit/chromium/public/WebIDBTransaction.h:65 > > > > + // FIXME: this is never called in the renderer. Find a cleaner solution. > > > > > > Comments that apply to the whole function really seem like they should go right above the function not in them. Is there other precedent for something like this? If not, can you please stop doing this? > > > > > > > OK ok :) > > > > > Also, try not to make the comments Chromium architecture specific. Maybe just say that this is only meant to be called from within WebCore? > > > > Erm, this bug has the [Chromium] label and the patch touches only a file in /WebKit/chromium so how can making a chromium-specific comment possibly be wrong? > > WebKit code should not depend on Chromium code. Even if this code is designed to be consumed by Chromium. > Yes, that comment did depend on Chromium code...fair enough, changed. 286252 8 jorlow 2010-09-28 08:24:08 -0700 (In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 69034 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=69034&action=review > > > > r=me > > > > > WebKit/chromium/public/WebIDBTransaction.h:65 > > > + // FIXME: this is never called in the renderer. Find a cleaner solution. > > > > Comments that apply to the whole function really seem like they should go right above the function not in them. Is there other precedent for something like this? If not, can you please stop doing this? > > > > OK ok :) > > > Also, try not to make the comments Chromium architecture specific. Maybe just say that this is only meant to be called from within WebCore? > > Erm, this bug has the [Chromium] label and the patch touches only a file in /WebKit/chromium so how can making a chromium-specific comment possibly be wrong? WebKit code should not depend on Chromium code. Even if this code is designed to be consumed by Chromium. Plus Darin said so. :-) 286253 9 jorlow 2010-09-28 08:24:28 -0700 (In reply to comment #8) > (In reply to comment #4) > > (In reply to comment #2) > > > (From update of attachment 69034 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=69034&action=review > > > > > > r=me > > > > > > > WebKit/chromium/public/WebIDBTransaction.h:65 > > > > + // FIXME: this is never called in the renderer. Find a cleaner solution. > > > > > > Comments that apply to the whole function really seem like they should go right above the function not in them. Is there other precedent for something like this? If not, can you please stop doing this? > > > > > > > OK ok :) > > > > > Also, try not to make the comments Chromium architecture specific. Maybe just say that this is only meant to be called from within WebCore? > > > > Erm, this bug has the [Chromium] label and the patch touches only a file in /WebKit/chromium so how can making a chromium-specific comment possibly be wrong? > > WebKit code should not depend on Chromium code. Even if this code is designed to be consumed by Chromium. > > Plus Darin said so. :-) Ugh...stupid bugzilla...sorry. 69034 2010-09-28 03:32:09 -0700 2010-09-28 03:36:09 -0700 Patch bug-46710-20100928113207.patch text/plain 1421 andreip ZGlmZiAtLWdpdCBhL1dlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cgYi9XZWJLaXQvY2hyb21pdW0v Q2hhbmdlTG9nCmluZGV4IGU1MmUzYjc2MzhhNDRlZGE3Y2Q3YWY5MjQ3MWRiMTdiNmEzNDZkNjMu LjBmMzkyZGVlNTRmODdmYTQzNTk4ZGI2YzllYzhkYmM3ODkxOWEyYjcgMTAwNjQ0Ci0tLSBhL1dl YktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKKysrIGIvV2ViS2l0L2Nocm9taXVtL0NoYW5nZUxvZwpA QCAtMSwzICsxLDE1IEBACisyMDEwLTA5LTI4ICBBbmRyZWkgUG9wZXNjdSAgPGFuZHJlaXBAZ29v Z2xlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBbQ2hyb21pdW1dIExpbmtpbmcgZXJyb3IgZHVlIHRvIHdyb25nIGd1YXJkIGluIFdlYklEQlRy YW5zYWN0aW9uLmgKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTQ2NzEwCisKKyAgICAgICAgUmVtb3ZlIHRoZSB3cm9uZyBndWFyZC4KKworICAgICAgICAq IHB1YmxpYy9XZWJJREJUcmFuc2FjdGlvbi5oOgorICAgICAgICAoV2ViS2l0OjpXZWJJREJUcmFu c2FjdGlvbjo6Z2V0SURCVHJhbnNhY3Rpb25CYWNrZW5kSW50ZXJmYWNlKToKKwogMjAxMC0wOS0y NiAgTWloYWkgUGFycGFyaXRhICA8bWloYWlwQGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZp ZXdlZCBieSBBZGFtIEJhcnRoLgpkaWZmIC0tZ2l0IGEvV2ViS2l0L2Nocm9taXVtL3B1YmxpYy9X ZWJJREJUcmFuc2FjdGlvbi5oIGIvV2ViS2l0L2Nocm9taXVtL3B1YmxpYy9XZWJJREJUcmFuc2Fj dGlvbi5oCmluZGV4IDBlY2Y1NWUyMjI3NDkyNTU5YjUxZDNjMTg2NzJiZmY4OTI1NDAwZDYuLjk5 ZDI0MWMwNTBiMGZiNjQwZTRmZmJjY2Q0Y2EzOTczZmIyMTg2ODcgMTAwNjQ0Ci0tLSBhL1dlYktp dC9jaHJvbWl1bS9wdWJsaWMvV2ViSURCVHJhbnNhY3Rpb24uaAorKysgYi9XZWJLaXQvY2hyb21p dW0vcHVibGljL1dlYklEQlRyYW5zYWN0aW9uLmgKQEAgLTYwLDEzICs2MCwxMiBAQCBwdWJsaWM6 CiAgICAgfQogICAgIHZpcnR1YWwgdm9pZCBzZXRDYWxsYmFja3MoV2ViSURCVHJhbnNhY3Rpb25D YWxsYmFja3MqKSB7IFdFQktJVF9BU1NFUlRfTk9UX1JFQUNIRUQoKTsgfQogCi0jaWYgV0VCS0lU X0lNUExFTUVOVEFUSU9OCiAgICAgdmlydHVhbCBXZWJDb3JlOjpJREJUcmFuc2FjdGlvbkJhY2tl bmRJbnRlcmZhY2UqIGdldElEQlRyYW5zYWN0aW9uQmFja2VuZEludGVyZmFjZSgpIGNvbnN0CiAg ICAgeworICAgICAgICAvLyBGSVhNRTogdGhpcyBpcyBuZXZlciBjYWxsZWQgaW4gdGhlIHJlbmRl cmVyLiBGaW5kIGEgY2xlYW5lciBzb2x1dGlvbi4KICAgICAgICAgV0VCS0lUX0FTU0VSVF9OT1Rf UkVBQ0hFRCgpOwogICAgICAgICByZXR1cm4gMDsKICAgICB9Ci0jZW5kaWYKIAogfTsKIAo=