83132 2012-04-04 02:24:22 -0700 [Qt] Separate image encoding from dataURL construction 2012-04-11 22:17:58 -0700 1 1 1 Unclassified WebKit Canvas 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 noel.gordon noel.gordon hausmann noam ossy webkit.review.bot zecke oldest_to_newest 595333 0 noel.gordon 2012-04-04 02:24:22 -0700 [Qt] Separate image encoding from dataURL construction 595334 1 135534 noel.gordon 2012-04-04 02:25:33 -0700 Created attachment 135534 Patch 599130 2 135534 hausmann 2012-04-10 03:28:42 -0700 Comment on attachment 135534 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135534&action=review > Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:383 > + compressionQuality = static_cast<int>(*quality * 100 + 0.5); Why not use ceil or round here? 599137 3 noel.gordon 2012-04-10 04:04:51 -0700 > Why not use ceil or round here? Left it as it was before, and also matches the other webkit ports. 599243 4 135534 noam 2012-04-10 07:12:02 -0700 Comment on attachment 135534 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135534&action=review > Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:382 > + if (quality && *quality >= 0.0 && *quality <= 1.0) Use 0 and 1, not 0.0 and 1.0 > Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:406 > + if (!encodeImage(m_data.m_pixmap, mimeType.substring(sizeof "image"), quality, data)) > + return "data:,"; Any reason why not to use QMimeDatabase and QMimeType::suffixes()? 599810 5 noel.gordon 2012-04-10 17:06:05 -0700 (In reply to comment #4) > (From update of attachment 135534 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135534&action=review > > > Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:382 > > + if (quality && *quality >= 0.0 && *quality <= 1.0) > > Use 0 and 1, not 0.0 and 1.0 Possible but again, just re-factoring change here, and it matches the other webkit ports, so I'd prefer to leave it as is. > > Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:406 > > + if (!encodeImage(m_data.m_pixmap, mimeType.substring(sizeof "image"), quality, data)) > > + return "data:,"; > > Any reason why not to use QMimeDatabase and QMimeType::suffixes()? Interesting, though not sure how they'd help. At routine entry we have: ASSERT(MIMETypeRegistry::isSupportedImageMIMETypeForEncoding(mimeType)); so the mimeType is already canonical. 599837 6 noel.gordon 2012-04-10 17:31:13 -0700 Thanks for the r+ Noam. I have some questions. I believe that QPixmap.save() is not safe to use off the main thread. Is that correct? Since the mimeType is known canonical via the ASSERT, why are the following lines needed? if (!mimeType.startsWith("image/")) return "data:,"; Is there a reason to prefer the QT toBase64 encoder over the base64 encoder provided by WebCore? return "data:" + mimeType + ";base64," + data.toBase64().data(); 599916 7 135534 webkit.review.bot 2012-04-10 18:47:08 -0700 Comment on attachment 135534 Patch Clearing flags on attachment: 135534 Committed r113805: <http://trac.webkit.org/changeset/113805> 599917 8 webkit.review.bot 2012-04-10 18:47:18 -0700 All reviewed patches have been landed. Closing bug. 600885 9 noel.gordon 2012-04-11 22:17:58 -0700 (In reply to comment #6) > Thanks for the r+ Noam. I have some questions. > > Since the mimeType is known canonical via the ASSERT, why are the following lines needed? > > if (!mimeType.startsWith("image/")) > return "data:,"; Filed bug 83746 about that if you'd like to review. 135534 2012-04-04 02:25:33 -0700 2012-04-10 18:47:08 -0700 Patch bug-83132-20120404192531.patch text/plain 3630 noel.gordon U3VidmVyc2lvbiBSZXZpc2lvbjogMTEzMTUyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggY2Q0ZDVmMzBlNDY0MWY5 ZmY4ZjRmOGZiNzg0YmIwNzlmMDU2YWMwNi4uMDg1NDM1ZDMzNTU1OWVjNjE0MjMzZTEzMGFhYWUx ZjA0NWFmYmJlNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDEyLTA0LTA0ICBOb2Vs IEdvcmRvbiAgPG5vZWwuZ29yZG9uQGdtYWlsLmNvbT4KKworICAgICAgICBbUXRdIFNlcGFyYXRl IGltYWdlIGVuY29kaW5nIGZyb20gZGF0YVVSTCBjb25zdHJ1Y3Rpb24KKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTgzMTMyCisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisgICAgICAgIAorICAgICAgICBSZW1vdmUgdGhlIGltcGxp Y2l0IGFzc3VtcHRpb24gdGhhdCBhIGRhdGFVUkwgaXMgdGhlIG9ubHkgZGVzaXJlZCBvdXRwdXQg Zm9ybWF0CisgICAgICAgIG9mIHRoZSBpbWFnZSBlbmNvZGluZyBwaGFzZS4KKworICAgICAgICBO byBuZXcgdGVzdHMsIHJlZmFjdG9yaW5nIG9ubHksIGNvdmVyZWQgYnkgZXhpc3RpbmcgY2FudmFz IHRlc3RzLgorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvcXQvSW1hZ2VCdWZmZXJRdC5j cHA6CisgICAgICAgIChXZWJDb3JlOjplbmNvZGVJbWFnZSk6IE91dHB1dCB0aGUgZW5jb2RlZCBp bWFnZSB0byB0aGUgcHJvdmlkZWQgUUJ5dGVBcnJheS4KKyAgICAgICAgKFdlYkNvcmUpOgorICAg ICAgICAoV2ViQ29yZTo6SW1hZ2VCdWZmZXI6OnRvRGF0YVVSTCkgRm9ybWF0IHRoZSBkYXRhVVJM IGVuY29kaW5nIG9mIHRoZSBtaW1lVHlwZQorICAgICAgICBlbmNvZGVkIGltYWdlIGRhdGEgaGVy ZS4gQ2xhcmlmeSB0aGUgY29tbWVudC4KKwogMjAxMi0wNC0wMyAgQW5kcmV5IEtvc3lha292ICA8 Y2FzZXFAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFdlYiBJbnNwZWN0b3I6IGV2ZW50IG1hcmtz IGFyZSBtaXNzaW5nIGluIHRoZSB0aW1lbGluZSBvdmVydmlldwpkaWZmIC0tZ2l0IGEvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvcXQvSW1hZ2VCdWZmZXJRdC5jcHAgYi9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9xdC9JbWFnZUJ1ZmZlclF0LmNwcAppbmRleCA0NGY4 NjBhZjJjODU4ZjM4ZGVlZjYyZDI1N2U1NmM0ZDJhYmU3MjE5Li45NzJhNmFlNDY3OTgyMGNjODgw NDYyOWE5ZDhmMTA4ZGUzNGI1ODQzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9y bS9ncmFwaGljcy9xdC9JbWFnZUJ1ZmZlclF0LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9ncmFwaGljcy9xdC9JbWFnZUJ1ZmZlclF0LmNwcApAQCAtMzc2LDkgKzM3NiwyMCBAQCB2 b2lkIEltYWdlQnVmZmVyOjpwdXRCeXRlQXJyYXkoTXVsdGlwbHkgbXVsdGlwbGllZCwgQnl0ZUFy cmF5KiBzb3VyY2UsIGNvbnN0IEludAogICAgICAgICBtX2RhdGEubV9wYWludGVyLT5yZXN0b3Jl KCk7CiB9CiAKLS8vIFdlIGdldCBhIG1pbWVUeXBlIGhlcmUgYnV0IFFJbWFnZVdyaXRlciBkb2Vz IG5vdCBzdXBwb3J0IG1pbWV0eXBlcyBidXQKLS8vIG9ubHkgZm9ybWF0cyAocG5nLCBnaWYsIGpw ZWcuLi4sIHhwbSkuIFNvIGFzc3VtZSB3ZSBnZXQgaW1hZ2UvIGFzIGltYWdlCi0vLyBtaW1ldHlw ZXMgYW5kIHRoZW4gcmVtb3ZlIHRoZSBpbWFnZS8gdG8gZ2V0IHRoZSBRdCBmb3JtYXQuCitzdGF0 aWMgYm9vbCBlbmNvZGVJbWFnZShjb25zdCBRUGl4bWFwJiBwaXhtYXAsIGNvbnN0IFN0cmluZyYg Zm9ybWF0LCBjb25zdCBkb3VibGUqIHF1YWxpdHksIFFCeXRlQXJyYXkmIGRhdGEpCit7CisgICAg aW50IGNvbXByZXNzaW9uUXVhbGl0eSA9IDEwMDsKKyAgICBpZiAocXVhbGl0eSAmJiAqcXVhbGl0 eSA+PSAwLjAgJiYgKnF1YWxpdHkgPD0gMS4wKQorICAgICAgICBjb21wcmVzc2lvblF1YWxpdHkg PSBzdGF0aWNfY2FzdDxpbnQ+KCpxdWFsaXR5ICogMTAwICsgMC41KTsKKworICAgIFFCdWZmZXIg YnVmZmVyKCZkYXRhKTsKKyAgICBidWZmZXIub3BlbihRQnVmZmVyOjpXcml0ZU9ubHkpOworICAg IGJvb2wgc3VjY2VzcyA9IHBpeG1hcC5zYXZlKCZidWZmZXIsIGZvcm1hdC51dGY4KCkuZGF0YSgp LCBjb21wcmVzc2lvblF1YWxpdHkpOworICAgIGJ1ZmZlci5jbG9zZSgpOworCisgICAgcmV0dXJu IHN1Y2Nlc3M7Cit9CisKIFN0cmluZyBJbWFnZUJ1ZmZlcjo6dG9EYXRhVVJMKGNvbnN0IFN0cmlu ZyYgbWltZVR5cGUsIGNvbnN0IGRvdWJsZSogcXVhbGl0eSkgY29uc3QKIHsKICAgICBBU1NFUlQo TUlNRVR5cGVSZWdpc3RyeTo6aXNTdXBwb3J0ZWRJbWFnZU1JTUVUeXBlRm9yRW5jb2RpbmcobWlt ZVR5cGUpKTsKQEAgLTM4NiwyNCArMzk3LDEzIEBAIFN0cmluZyBJbWFnZUJ1ZmZlcjo6dG9EYXRh VVJMKGNvbnN0IFN0cmluZyYgbWltZVR5cGUsIGNvbnN0IGRvdWJsZSogcXVhbGl0eSkgY29uCiAg ICAgaWYgKCFtaW1lVHlwZS5zdGFydHNXaXRoKCJpbWFnZS8iKSkKICAgICAgICAgcmV0dXJuICJk YXRhOiwiOwogCi0gICAgLy8gcHJlcGFyZSBvdXIgdGFyZ2V0Ci0gICAgUUJ5dGVBcnJheSBkYXRh OwotICAgIFFCdWZmZXIgYnVmZmVyKCZkYXRhKTsKLSAgICBidWZmZXIub3BlbihRQnVmZmVyOjpX cml0ZU9ubHkpOworICAgIC8vIFFJbWFnZVdyaXRlciBkb2VzIG5vdCBzdXBwb3J0IG1pbWV0eXBl cy4gSXQgZG9lcyBzdXBwb3J0IFF0IGltYWdlIGZvcm1hdHMgKHBuZywKKyAgICAvLyBnaWYsIGpw ZWcuLi4sIHhwbSkgc28gc2tpcCB0aGUgaW1hZ2UvIHRvIGdldCB0aGUgUXQgaW1hZ2UgZm9ybWF0 IHVzZWQgdG8gZW5jb2RlCisgICAgLy8gdGhlIG1fcGl4bWFwIGltYWdlLgogCi0gICAgaWYgKHF1 YWxpdHkgJiYgKnF1YWxpdHkgPj0gMC4wICYmICpxdWFsaXR5IDw9IDEuMCkgewotICAgICAgICBp ZiAoIW1fZGF0YS5tX3BpeG1hcC5zYXZlKCZidWZmZXIsIG1pbWVUeXBlLnN1YnN0cmluZyhzaXpl b2YgImltYWdlIikudXRmOCgpLmRhdGEoKSwgKnF1YWxpdHkgKiAxMDAgKyAwLjUpKSB7Ci0gICAg ICAgICAgICBidWZmZXIuY2xvc2UoKTsKLSAgICAgICAgICAgIHJldHVybiAiZGF0YTosIjsKLSAg ICAgICAgfQotICAgIH0gZWxzZSB7Ci0gICAgICAgIGlmICghbV9kYXRhLm1fcGl4bWFwLnNhdmUo JmJ1ZmZlciwgbWltZVR5cGUuc3Vic3RyaW5nKHNpemVvZiAiaW1hZ2UiKS51dGY4KCkuZGF0YSgp LCAxMDApKSB7Ci0gICAgICAgICAgICBidWZmZXIuY2xvc2UoKTsKLSAgICAgICAgICAgIHJldHVy biAiZGF0YTosIjsKLSAgICAgICAgfQotICAgIH0KLQotICAgIGJ1ZmZlci5jbG9zZSgpOworICAg IFFCeXRlQXJyYXkgZGF0YTsKKyAgICBpZiAoIWVuY29kZUltYWdlKG1fZGF0YS5tX3BpeG1hcCwg bWltZVR5cGUuc3Vic3RyaW5nKHNpemVvZiAiaW1hZ2UiKSwgcXVhbGl0eSwgZGF0YSkpCisgICAg ICAgIHJldHVybiAiZGF0YTosIjsKIAogICAgIHJldHVybiAiZGF0YToiICsgbWltZVR5cGUgKyAi O2Jhc2U2NCwiICsgZGF0YS50b0Jhc2U2NCgpLmRhdGEoKTsKIH0K