98116 2012-10-01 22:52:42 -0700 [Mac][Chromium-Mac] Implement LocaleMac::dateFormat 2012-10-04 01:25:24 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 98109 97997 98383 1 tkent tkent morrita webkit.review.bot oldest_to_newest 732284 0 tkent 2012-10-01 22:52:42 -0700 [Mac][Chromium-Mac] Implement LocaleMac::dateFormat 732365 1 166627 tkent 2012-10-02 00:18:43 -0700 Created attachment 166627 Patch 733289 2 166627 webkit.review.bot 2012-10-02 18:20:32 -0700 Comment on attachment 166627 Patch Rejecting attachment 166627 from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14129438 733291 3 tkent 2012-10-02 18:21:58 -0700 Oops, the ChangeLog has no "Reviewed by" line. 733294 4 tkent 2012-10-02 18:25:16 -0700 Committed r130243: <http://trac.webkit.org/changeset/130243> 733694 5 166627 darin 2012-10-03 07:48:29 -0700 Comment on attachment 166627 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166627&action=review > Source/WebCore/platform/text/mac/LocaleMac.mm:257 > + if (!m_dateFormat.isEmpty()) I suggest using isNull, more efficient, here. > Source/WebCore/platform/text/mac/LocaleMac.mm:260 > + RetainPtr<NSDateFormatter> formatter(AdoptNS, createShortDateFormatter()); > + m_dateFormat = String([formatter.get() dateFormat]); The explicit String() here is unneeded, I believe. I would write this: if (m_dateFormat.isNull()) m_dateFormat = [adoptNS(createShortDateFormatter()).get() dateFormat]; return m_dateFormat; And later createShortDateFormatter should be changed to return a RetainPtr instead of a raw pointer, which would eliminate the need for the adoptNS call here. 734492 6 tkent 2012-10-04 01:25:24 -0700 (In reply to comment #5) > > Source/WebCore/platform/text/mac/LocaleMac.mm:257 > > + if (!m_dateFormat.isEmpty()) > > I suggest using isNull, more efficient, here. > > > Source/WebCore/platform/text/mac/LocaleMac.mm:260 > > + RetainPtr<NSDateFormatter> formatter(AdoptNS, createShortDateFormatter()); > > + m_dateFormat = String([formatter.get() dateFormat]); > > The explicit String() here is unneeded, I believe. > > I would write this: > > if (m_dateFormat.isNull()) > m_dateFormat = [adoptNS(createShortDateFormatter()).get() dateFormat]; > return m_dateFormat; > > And later createShortDateFormatter should be changed to return a RetainPtr instead of a raw pointer, which would eliminate the need for the adoptNS call here. Thank you for the suggestions! I filed Bug 98383, and uploaded a patch for them. 166627 2012-10-02 00:18:43 -0700 2012-10-03 07:48:29 -0700 Patch bug-98116-20121002161756.patch text/plain 2420 tkent U3VidmVyc2lvbiBSZXZpc2lvbjogMTMwMTI3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDJkYjAzYWYzZmFjNGVk MDIyYWQ0YWU4NGExYzNlNjA5MjY4OWVhNS4uMzU1YzBhYTZlNDM4ZGRmOGU4ODI1Njg1YTkwNGNi MWVmMDIwNzQ3MCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDEyLTEwLTAyICBLZW50 IFRhbXVyYSAgPHRrZW50QGNocm9taXVtLm9yZz4KKworICAgICAgICBbTWFjXVtDaHJvbWl1bS1N YWNdIEltcGxlbWVudCBMb2NhbGVNYWM6OmRhdGVGb3JtYXQKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTk4MTE2CisKKyAgICAgICAgaHR0cDovL3RyYWMu d2Via2l0Lm9yZy9jaGFuZ2VzZXQvMTMwMTI3IGludHJvZHVjZWQKKyAgICAgICAgTG9jYWxpemVy OjpkYXRlRm9ybWF0LCBhbmQgdGhpcyBpcyBpdHMgaW1wbGVtZW50YXRpb24gZm9yIExvY2FsZUlD VQorICAgICAgICBjbGFzc3MuIFRoZSBjb2RlIGlzIGdvaW5nIHRvIGJlIHVzZWQgd2hlbgorICAg ICAgICBFTkFCTEVfSU5QVVRfTVVMVElQTEVfRklFTERTX1VJIGlzIGVuYWJsZWQuCisKKyAgICAg ICAgTm8gbmV3IHRlc3RzLiBUaGUgZnVuY3Rpb24gaXMgbm90IHVzZWQgeWV0LgorCisgICAgICAg ICogcGxhdGZvcm0vdGV4dC9tYWMvTG9jYWxlTWFjLmg6CisgICAgICAgIChMb2NhbGVNYWMpOiBE ZWNsYXJlIG1fZGF0ZUZvcm1hdC4KKyAgICAgICAgKiBwbGF0Zm9ybS90ZXh0L21hYy9Mb2NhbGVN YWMubW06CisgICAgICAgIChXZWJDb3JlOjpMb2NhbGVNYWM6OmRhdGVGb3JtYXQpOiBJbXBsZW1l bnRlZC4KKwogMjAxMi0xMC0wMSAgWW9zaGlmdW1pIElub3VlICA8eW9zaW5AY2hyb21pdW0ub3Jn PgogCiAgICAgICAgIEFkZGluZyBMb2NhbGl6ZXI6OmRhdGVGb3JtYXQoKSBmb3IgbXVsdGlwbGUg ZmllbGRzIGRhdGUvZGF0ZXRpbWUgaW5wdXQgVUkKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3Jl L3BsYXRmb3JtL3RleHQvbWFjL0xvY2FsZU1hYy5oIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0v dGV4dC9tYWMvTG9jYWxlTWFjLmgKaW5kZXggMWU4MTUxZDA0ZTgxNmFiMDdkMDQ2NmU5NzAyYjBj OWYzNjRlZDQ4ZC4uMmQ5ZThhZGU2OWEyZjQxNGY4YjA2OTFhNWEzODdhMzVlOTBkMGEzNSAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9tYWMvTG9jYWxlTWFjLmgKKysr IGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9tYWMvTG9jYWxlTWFjLmgKQEAgLTgyLDYg KzgyLDcgQEAgcHJpdmF0ZToKICAgICBOU0RhdGVGb3JtYXR0ZXIgKmNyZWF0ZVRpbWVGb3JtYXR0 ZXIoKTsKICAgICBOU0RhdGVGb3JtYXR0ZXIgKmNyZWF0ZVNob3J0VGltZUZvcm1hdHRlcigpOwog CisgICAgU3RyaW5nIG1fZGF0ZUZvcm1hdDsKICAgICBTdHJpbmcgbV9sb2NhbGl6ZWRUaW1lRm9y bWF0VGV4dDsKICAgICBTdHJpbmcgbV9sb2NhbGl6ZWRTaG9ydFRpbWVGb3JtYXRUZXh0OwogICAg IFZlY3RvcjxTdHJpbmc+IG1fdGltZUFNUE1MYWJlbHM7CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi Q29yZS9wbGF0Zm9ybS90ZXh0L21hYy9Mb2NhbGVNYWMubW0gYi9Tb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS90ZXh0L21hYy9Mb2NhbGVNYWMubW0KaW5kZXggZjM5ZDk5YjlhMDk4YjJkZjRiZGZkZTU4 ZjM4YmVjYmY4ZjIyMjgwNS4uYWU5ZjY3NWI3OGU2MGI5ZGJjZTQ3ZDIzMDFjMmI3ODk0NzQ0NGZi OSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9tYWMvTG9jYWxlTWFj Lm1tCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3RleHQvbWFjL0xvY2FsZU1hYy5tbQpA QCAtMjU0LDggKzI1NCwxMSBAQCBOU0RhdGVGb3JtYXR0ZXIqIExvY2FsZU1hYzo6Y3JlYXRlU2hv cnRUaW1lRm9ybWF0dGVyKCkKIAogU3RyaW5nIExvY2FsZU1hYzo6ZGF0ZUZvcm1hdCgpCiB7Ci0g ICAgLy8gRklYTUU6IFdlIHNob3VsZCBoYXZlIHJlYWwgaW1wbGVtZW50YXRpb24gb2YgTG9jYWxl TWFjOjpkYXRlRm9ybWF0KCkuCi0gICAgcmV0dXJuIGVtcHR5U3RyaW5nKCk7CisgICAgaWYgKCFt X2RhdGVGb3JtYXQuaXNFbXB0eSgpKQorICAgICAgICByZXR1cm4gbV9kYXRlRm9ybWF0OworICAg IFJldGFpblB0cjxOU0RhdGVGb3JtYXR0ZXI+IGZvcm1hdHRlcihBZG9wdE5TLCBjcmVhdGVTaG9y dERhdGVGb3JtYXR0ZXIoKSk7CisgICAgbV9kYXRlRm9ybWF0ID0gU3RyaW5nKFtmb3JtYXR0ZXIu Z2V0KCkgZGF0ZUZvcm1hdF0pOworICAgIHJldHVybiBtX2RhdGVGb3JtYXQ7CiB9CiAKIFN0cmlu ZyBMb2NhbGVNYWM6OnRpbWVGb3JtYXQoKQo=