114133 2013-04-07 16:04:20 -0700 [GTK] Remove platform specific implementation of KURL::fileSystemPath() 2013-04-15 08:27:30 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 paroga paroga commit-queue gustavo mrobinson philn xan.lopez oldest_to_newest 870568 0 paroga 2013-04-07 16:04:20 -0700 [GTK] Remove platform specific implementation of KURL::fileSystemPath() 870569 1 196815 paroga 2013-04-07 16:07:45 -0700 Created attachment 196815 Patch 871654 2 gustavo 2013-04-09 05:47:45 -0700 There is one thing to be considered here: the GTK+ implementation of KRUL::fileSystemPath ends up calling FileSystemGtk's filenameToString(). That code uses glib's file naming functions which are smart enough to deal with systems that use encodings other than UTF-8 and even names that cannot be encoded to anything. We will lose that feature by adopting the shared implementation. I don't mind, but I'd like to hear other opinions. 871656 3 paroga 2013-04-09 05:53:19 -0700 (In reply to comment #2) > That code uses glib's file naming functions which are smart enough to deal with systems that use encodings other than UTF-8 and even names that cannot be encoded to anything. For what is the encoding stuff needed? Input of KRUL::fileSystemPath is UTF16 and output is UTF16? Any special encondig of the filesytem has to be done somewhere else (e.g. at http://trac.webkit.org/browser/trunk/Source/WebCore/platform/gtk/FileSystemGtk.cpp?rev=148014#L56) IMHO. 871808 4 mrobinson 2013-04-09 07:11:40 -0700 (In reply to comment #2) > There is one thing to be considered here: the GTK+ implementation of KRUL::fileSystemPath ends up calling FileSystemGtk's filenameToString(). That code uses glib's file naming functions which are smart enough to deal with systems that use encodings other than UTF-8 and even names that cannot be encoded to anything. We will lose that feature by adopting the shared implementation. I don't mind, but I'd like to hear other opinions. The old code looks a bit wrong too: RefPtr<GFile> file = adoptGRef(g_file_new_for_path(path().utf8().data())); GOwnPtr<char> filename(g_file_get_path(file.get())); return filenameToString(filename.get()); Here it's using a UTF-8 string to produce a filename, while it should instead be using a system-encoded string. As long as translation to a system-encoded string happens later, I think it might be okay to use the shared implementation. 875504 5 paroga 2013-04-15 01:16:15 -0700 (In reply to comment #4) > (In reply to comment #2) > The old code looks a bit wrong too: > > RefPtr<GFile> file = adoptGRef(g_file_new_for_path(path().utf8().data())); > GOwnPtr<char> filename(g_file_get_path(file.get())); > return filenameToString(filename.get()); > > Here it's using a UTF-8 string to produce a filename, while it should instead be using a system-encoded string. As long as translation to a system-encoded string happens later, I think it might be okay to use the shared implementation. The more importent failure is, that path() returns a percent-encoded string and needs a decodeURLEscapeSequences(). So this code never worked with non-ASCII file names anyway. So what about landing it? 875849 6 196815 commit-queue 2013-04-15 08:27:28 -0700 Comment on attachment 196815 Patch Clearing flags on attachment: 196815 Committed r148440: <http://trac.webkit.org/changeset/148440> 875850 7 commit-queue 2013-04-15 08:27:30 -0700 All reviewed patches have been landed. Closing bug. 196815 2013-04-07 16:07:45 -0700 2013-04-15 08:27:28 -0700 Patch bug-114133-20130408010729.patch text/plain 3768 paroga U3VidmVyc2lvbiBSZXZpc2lvbjogMTQ3ODg0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNzVhNWY5ZDkwMDMzMGYx OGU1YTBlYmU4ODU1YzgxZmU4NDhjMTQ2Zi4uOGFiZTg0M2E3YjUxYjJjNTNmZDAzODY5NWI1NTI5 ODAyZTEwY2Q2NCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDIwIEBACiAyMDEzLTA0LTA3ICBQYXRy aWNrIEdhbnN0ZXJlciAgPHBhcm9nYUB3ZWJraXQub3JnPgogCisgICAgICAgIFtHVEtdIFJlbW92 ZSBwbGF0Zm9ybSBzcGVjaWZpYyBpbXBsZW1lbnRhdGlvbiBvZiBLVVJMOjpmaWxlU3lzdGVtUGF0 aCgpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTQx MzMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICByMTQ3 ODgzIGFkZGVkIGEgZ2VuZXJpYyBpbXBsZW1lbnRhdGlvbiBvZiB0aGlzIGZ1bmN0aW9uLAorICAg ICAgICB3aGljaCBjYW4gYmUgdXNlZCBieSBhbGwgcG9ydHMgdG8gc2hhcmUgdGhlIGNvZGUuCisK KyAgICAgICAgKiBHTlVtYWtlZmlsZS5saXN0LmFtOgorICAgICAgICAqIHBsYXRmb3JtL0tVUkwu Y3BwOgorICAgICAgICAoV2ViQ29yZSk6CisgICAgICAgICogcGxhdGZvcm0vZ3RrL0tVUkxHdGsu Y3BwOiBSZW1vdmVkLgorCisyMDEzLTA0LTA3ICBQYXRyaWNrIEdhbnN0ZXJlciAgPHBhcm9nYUB3 ZWJraXQub3JnPgorCiAgICAgICAgIEFkZCBkZWZhdWx0IGltcGxlbWVudGF0aW9uIGZvciBLVVJM OjpmaWxlU3lzdGVtUGF0aCgpCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xMDgzMjYKIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvR05VbWFrZWZp bGUubGlzdC5hbSBiL1NvdXJjZS9XZWJDb3JlL0dOVW1ha2VmaWxlLmxpc3QuYW0KaW5kZXggNGQz YWQ2YTFjZjllOTIzNWVkOWM2MDljZjgwMWIxNjQzMmNkMzBiNy4uZjEzZjVlYmJkYTdlZDdhZWZk YjFhYjQ2NTg5MzhlYTU5MTMxYTAxOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvR05VbWFr ZWZpbGUubGlzdC5hbQorKysgYi9Tb3VyY2UvV2ViQ29yZS9HTlVtYWtlZmlsZS5saXN0LmFtCkBA IC02MjM4LDcgKzYyMzgsNiBAQCBwbGF0Zm9ybWd0a19zb3VyY2VzICs9IFwKIAlTb3VyY2UvV2Vi Q29yZS9wbGF0Zm9ybS9ndGsvR3RrVXRpbGl0aWVzLmggXAogCVNvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL2d0ay9HdGtWZXJzaW9uaW5nLmMgXAogCVNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2d0ay9H dGtWZXJzaW9uaW5nLmggXAotCVNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2d0ay9LVVJMR3RrLmNw cCBcCiAJU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL0tleUJpbmRpbmdUcmFuc2xhdG9yLmNw cCBcCiAJU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL0tleUJpbmRpbmdUcmFuc2xhdG9yLmgg XAogCVNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2d0ay9MYW5ndWFnZUd0ay5jcHAgXApkaWZmIC0t Z2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vS1VSTC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9w bGF0Zm9ybS9LVVJMLmNwcAppbmRleCBjMWNlMGFmZDc0Zjk1ZmRhZmZjYzJhY2JjZWRhOWI2ODgz OTJhZjBhLi4zYWZjY2QwZmU3M2JlZWU5MTY0ZTZiYmI2ZjFlMDhkMmI3NmU2MmJjIDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9LVVJMLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29y ZS9wbGF0Zm9ybS9LVVJMLmNwcApAQCAtNjMxLDcgKzYzMSw3IEBAIFN0cmluZyBLVVJMOjpiYXNl QXNTdHJpbmcoKSBjb25zdAogICAgIHJldHVybiBtX3N0cmluZy5sZWZ0KG1fcGF0aEFmdGVyTGFz dFNsYXNoKTsKIH0KIAotI2lmICFQTEFURk9STShHVEspICYmICFQTEFURk9STShRVCkgJiYgIVVT RShDRikKKyNpZiAhUExBVEZPUk0oUVQpICYmICFVU0UoQ0YpCiBTdHJpbmcgS1VSTDo6ZmlsZVN5 c3RlbVBhdGgoKSBjb25zdAogewogICAgIGlmICghaXNWYWxpZCgpIHx8ICFpc0xvY2FsRmlsZSgp KQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL0tVUkxHdGsuY3BwIGIv U291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL0tVUkxHdGsuY3BwCmRlbGV0ZWQgZmlsZSBtb2Rl IDEwMDY0NAppbmRleCA4ZDFmZWFjYTk2YTRhMDlmNGJiZDA3MDgwOWI2OGUxODU0NDE2OGYyLi4w MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwCi0tLSBhL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2d0ay9LVVJMR3RrLmNwcAorKysgL2Rldi9udWxsCkBAIC0xLDQyICswLDAg QEAKLS8qCi0gKiBDb3B5cmlnaHQgKEMpIDIwMDggQ29sbGFib3JhIEx0ZC4KLSAqCi0gKiAgVGhp cyBsaWJyYXJ5IGlzIGZyZWUgc29mdHdhcmU7IHlvdSBjYW4gcmVkaXN0cmlidXRlIGl0IGFuZC9v cgotICogIG1vZGlmeSBpdCB1bmRlciB0aGUgdGVybXMgb2YgdGhlIEdOVSBMZXNzZXIgR2VuZXJh bCBQdWJsaWMKLSAqICBMaWNlbnNlIGFzIHB1Ymxpc2hlZCBieSB0aGUgRnJlZSBTb2Z0d2FyZSBG b3VuZGF0aW9uOyBlaXRoZXIKLSAqICB2ZXJzaW9uIDIgb2YgdGhlIExpY2Vuc2UsIG9yIChhdCB5 b3VyIG9wdGlvbikgYW55IGxhdGVyIHZlcnNpb24uCi0gKgotICogIFRoaXMgbGlicmFyeSBpcyBk aXN0cmlidXRlZCBpbiB0aGUgaG9wZSB0aGF0IGl0IHdpbGwgYmUgdXNlZnVsLAotICogIGJ1dCBX SVRIT1VUIEFOWSBXQVJSQU5UWTsgd2l0aG91dCBldmVuIHRoZSBpbXBsaWVkIHdhcnJhbnR5IG9m Ci0gKiAgTUVSQ0hBTlRBQklMSVRZIG9yIEZJVE5FU1MgRk9SIEEgUEFSVElDVUxBUiBQVVJQT1NF LiAgU2VlIHRoZSBHTlUKLSAqICBMZXNzZXIgR2VuZXJhbCBQdWJsaWMgTGljZW5zZSBmb3IgbW9y ZSBkZXRhaWxzLgotICoKLSAqICBZb3Ugc2hvdWxkIGhhdmUgcmVjZWl2ZWQgYSBjb3B5IG9mIHRo ZSBHTlUgTGVzc2VyIEdlbmVyYWwgUHVibGljCi0gKiAgTGljZW5zZSBhbG9uZyB3aXRoIHRoaXMg bGlicmFyeTsgaWYgbm90LCB3cml0ZSB0byB0aGUgRnJlZSBTb2Z0d2FyZQotICogIEZvdW5kYXRp b24sIEluYy4sIDUxIEZyYW5rbGluIFN0cmVldCwgRmlmdGggRmxvb3IsIEJvc3RvbiwgTUEgIDAy MTEwLTEzMDEgIFVTQQotICovCi0KLSNpbmNsdWRlICJjb25maWcuaCIKLSNpbmNsdWRlICJLVVJM LmgiCi0KLSNpbmNsdWRlICJGaWxlU3lzdGVtLmgiCi0jaW5jbHVkZSA8Z2lvL2dpby5oPgotI2lu Y2x1ZGUgPHd0Zi9nb2JqZWN0L0dPd25QdHIuaD4KLSNpbmNsdWRlIDx3dGYvZ29iamVjdC9HUmVm UHRyLmg+Ci0jaW5jbHVkZSA8d3RmL3RleHQvQ1N0cmluZy5oPgotCi0jaW5jbHVkZSA8Z2xpYi5o PgotCi1uYW1lc3BhY2UgV2ViQ29yZSB7Ci0KLVN0cmluZyBLVVJMOjpmaWxlU3lzdGVtUGF0aCgp IGNvbnN0Ci17Ci0gICAgaWYgKCFpc1ZhbGlkKCkgfHwgIWlzTG9jYWxGaWxlKCkpCi0gICAgICAg IHJldHVybiBTdHJpbmcoKTsKLQotICAgIEdSZWZQdHI8R0ZpbGU+IGZpbGUgPSBhZG9wdEdSZWYo Z19maWxlX25ld19mb3JfcGF0aChwYXRoKCkudXRmOCgpLmRhdGEoKSkpOwotICAgIEdPd25QdHI8 Y2hhcj4gZmlsZW5hbWUoZ19maWxlX2dldF9wYXRoKGZpbGUuZ2V0KCkpKTsKLSAgICByZXR1cm4g ZmlsZW5hbWVUb1N0cmluZyhmaWxlbmFtZS5nZXQoKSk7Ci19Ci0KLX0gLy8gbmFtZXNwYWNlIFdl YkNvcmUK