65990 2011-08-10 09:10:03 -0700 MarkupAccumulator: make resolution of URLs implicit to appendQuotedURLAttributeValue() 2011-08-10 12:46:06 -0700 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 rniwa benjamin abarth benjamin tony webkit.review.bot oldest_to_newest 449188 0 rniwa 2011-08-10 09:10:03 -0700 http://trac.webkit.org/changeset/92769 merged two code paths resolveURLIfNeeded. Now that there's exactly one place where resolveURLIfNeeded and resolveURLIfNeeded are called. We should probably merge these two functions. 449205 1 benjamin 2011-08-10 09:57:39 -0700 Will do, don't worry :) 449275 2 103512 benjamin 2011-08-10 11:54:34 -0700 Created attachment 103512 Patch 449277 3 benjamin 2011-08-10 11:57:27 -0700 Just moved appendQuotedURLAttributeValue() to an internal function, so appendAttribute() is the only function the client of the interface should use. I kept resolveURLIfNeeded() because it makes the code more readable IMHO. 449283 4 103512 rniwa 2011-08-10 12:02:49 -0700 Comment on attachment 103512 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103512&action=review > Source/WebCore/editing/MarkupAccumulator.cpp:187 > + const String resolvedURLString = resolveURLIfNeeded(element, attribute.value()); > UChar quoteChar = '\"'; > - String strippedURLString = urlString.stripWhiteSpace(); > + String strippedURLString = resolvedURLString.stripWhiteSpace(); Maybe do this one in one line? i.e. String strippedURLString = resolveURLIfNeeded(element, attribute.value()).stripWhiteSpace(); Also, can we make resolveURLIfNeeded a static local function since it doesn't need to access any member variable? 449286 5 benjamin 2011-08-10 12:12:07 -0700 (In reply to comment #4) > (From update of attachment 103512 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103512&action=review > > > Source/WebCore/editing/MarkupAccumulator.cpp:187 > > + const String resolvedURLString = resolveURLIfNeeded(element, attribute.value()); > > UChar quoteChar = '\"'; > > - String strippedURLString = urlString.stripWhiteSpace(); > > + String strippedURLString = resolvedURLString.stripWhiteSpace(); > > Maybe do this one in one line? i.e. > String strippedURLString = resolveURLIfNeeded(element, attribute.value()).stripWhiteSpace(); The non-stripped url is used in the general case. The stripped version is only used in the JavaScript case. I am just following the original code. > Also, can we make resolveURLIfNeeded a static local function since it doesn't need to access any member variable? It does need m_resolveURLsMethod. 449289 6 103512 rniwa 2011-08-10 12:14:08 -0700 Comment on attachment 103512 Patch ok 449314 7 103512 webkit.review.bot 2011-08-10 12:46:01 -0700 Comment on attachment 103512 Patch Clearing flags on attachment: 103512 Committed r92786: <http://trac.webkit.org/changeset/92786> 449315 8 webkit.review.bot 2011-08-10 12:46:06 -0700 All reviewed patches have been landed. Closing bug. 103512 2011-08-10 11:54:34 -0700 2011-08-10 12:46:01 -0700 Patch bug-65990-20110810205432.patch text/plain 4242 benjamin U3VidmVyc2lvbiBSZXZpc2lvbjogOTI3NzgKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBjMDEwZmE5NWEyOGVkNTVi ODgyZjI2YzM5YTEwOGVjMjk1ODFjNjA3Li43NDBkNWE1Y2M2ZGU2ZGEyODIwZmZmNzc0ZTg5OTYx MDYxMTc0YzJlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMTEtMDgtMTAgIEJlbmph bWluIFBvdWxhaW4gIDxpa2lwb3VAZ21haWwuY29tPgorCisgICAgICAgIE1hcmt1cEFjY3VtdWxh dG9yOiBtYWtlIHJlc29sdXRpb24gb2YgVVJMcyBpbXBsaWNpdCB0byBhcHBlbmRRdW90ZWRVUkxB dHRyaWJ1dGVWYWx1ZSgpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVn LmNnaT9pZD02NTk5MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIEluc3RlYWQgb2YgcmVzb2x2aW5nIHRoZSBVUkxzIGluIHRoZSBjYWxsIHNpdGUgb2Yg YXBwZW5kUXVvdGVkVVJMQXR0cmlidXRlVmFsdWUoKSwKKyAgICAgICAgdGhlIFVSTCBpcyBub3cg cmVzb2x2ZWQgaWYgbmVjZXNzYXJ5IGluIGFwcGVuZFF1b3RlZFVSTEF0dHJpYnV0ZVZhbHVlKCku CisKKyAgICAgICAgKiBlZGl0aW5nL01hcmt1cEFjY3VtdWxhdG9yLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6Ok1hcmt1cEFjY3VtdWxhdG9yOjphcHBlbmRRdW90ZWRVUkxBdHRyaWJ1dGVWYWx1ZSk6 CisgICAgICAgIChXZWJDb3JlOjpNYXJrdXBBY2N1bXVsYXRvcjo6YXBwZW5kQXR0cmlidXRlKToK KyAgICAgICAgKiBlZGl0aW5nL01hcmt1cEFjY3VtdWxhdG9yLmg6CisKIDIwMTEtMDgtMTAgIFZz ZXZvbG9kIFZsYXNvdiAgPHZzZXZpa0BjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgV2ViIEluc3Bl Y3RvcjogY29uc29sZSBtZXNzYWdlcyBtYXJrZXJzIGFyZSBsb3N0IGluIFJlc291cmNlcyBwYW5l bCBhZnRlciBwYWdlIHJlbG9hZApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvZWRpdGluZy9N YXJrdXBBY2N1bXVsYXRvci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9lZGl0aW5nL01hcmt1cEFjY3Vt dWxhdG9yLmNwcAppbmRleCA5MTcxYzRiNzBiN2IyMmMyMzBiMGFiMjQ5ZWM2ODA4MTJiN2Y2YWI3 Li5hMmY1ZWZjZDY5ZTRiMGFlOWEyMTZkYjM2OTViNzJmNmM5MGY1NTE5IDEwMDY0NAotLS0gYS9T b3VyY2UvV2ViQ29yZS9lZGl0aW5nL01hcmt1cEFjY3VtdWxhdG9yLmNwcAorKysgYi9Tb3VyY2Uv V2ViQ29yZS9lZGl0aW5nL01hcmt1cEFjY3VtdWxhdG9yLmNwcApAQCAtMTc5LDEwICsxNzksMTIg QEAgdm9pZCBNYXJrdXBBY2N1bXVsYXRvcjo6YXBwZW5kQ3VzdG9tQXR0cmlidXRlcyhWZWN0b3I8 VUNoYXI+JiwgRWxlbWVudCosIE5hbWVzcGEKIHsKIH0KIAotdm9pZCBNYXJrdXBBY2N1bXVsYXRv cjo6YXBwZW5kUXVvdGVkVVJMQXR0cmlidXRlVmFsdWUoVmVjdG9yPFVDaGFyPiYgcmVzdWx0LCBj b25zdCBTdHJpbmcmIHVybFN0cmluZykKK3ZvaWQgTWFya3VwQWNjdW11bGF0b3I6OmFwcGVuZFF1 b3RlZFVSTEF0dHJpYnV0ZVZhbHVlKFZlY3RvcjxVQ2hhcj4mIHJlc3VsdCwgY29uc3QgRWxlbWVu dCogZWxlbWVudCwgY29uc3QgQXR0cmlidXRlJiBhdHRyaWJ1dGUpCiB7CisgICAgQVNTRVJUKGVs ZW1lbnQtPmlzVVJMQXR0cmlidXRlKGNvbnN0X2Nhc3Q8QXR0cmlidXRlKj4oJmF0dHJpYnV0ZSkp KTsKKyAgICBjb25zdCBTdHJpbmcgcmVzb2x2ZWRVUkxTdHJpbmcgPSByZXNvbHZlVVJMSWZOZWVk ZWQoZWxlbWVudCwgYXR0cmlidXRlLnZhbHVlKCkpOwogICAgIFVDaGFyIHF1b3RlQ2hhciA9ICdc Iic7Ci0gICAgU3RyaW5nIHN0cmlwcGVkVVJMU3RyaW5nID0gdXJsU3RyaW5nLnN0cmlwV2hpdGVT cGFjZSgpOworICAgIFN0cmluZyBzdHJpcHBlZFVSTFN0cmluZyA9IHJlc29sdmVkVVJMU3RyaW5n LnN0cmlwV2hpdGVTcGFjZSgpOwogICAgIGlmIChwcm90b2NvbElzSmF2YVNjcmlwdChzdHJpcHBl ZFVSTFN0cmluZykpIHsKICAgICAgICAgLy8gbWluaW1hbCBlc2NhcGluZyBmb3IgamF2YXNjcmlw dCB1cmxzCiAgICAgICAgIGlmIChzdHJpcHBlZFVSTFN0cmluZy5jb250YWlucygnIicpKSB7CkBA IC0xOTksNyArMjAxLDcgQEAgdm9pZCBNYXJrdXBBY2N1bXVsYXRvcjo6YXBwZW5kUXVvdGVkVVJM QXR0cmlidXRlVmFsdWUoVmVjdG9yPFVDaGFyPiYgcmVzdWx0LCBjb24KIAogICAgIC8vIEZJWE1F OiBUaGlzIGRvZXMgbm90IGZ1bGx5IG1hdGNoIG90aGVyIGJyb3dzZXJzLiBGaXJlZm94IHBlcmNl bnQtZXNjYXBlcyBub24tQVNDSUkgY2hhcmFjdGVycyBmb3IgaW5uZXJIVE1MLgogICAgIHJlc3Vs dC5hcHBlbmQocXVvdGVDaGFyKTsKLSAgICBhcHBlbmRBdHRyaWJ1dGVWYWx1ZShyZXN1bHQsIHVy bFN0cmluZywgZmFsc2UpOworICAgIGFwcGVuZEF0dHJpYnV0ZVZhbHVlKHJlc3VsdCwgcmVzb2x2 ZWRVUkxTdHJpbmcsIGZhbHNlKTsKICAgICByZXN1bHQuYXBwZW5kKHF1b3RlQ2hhcik7CiB9CiAK QEAgLTM4OSw3ICszOTEsNyBAQCB2b2lkIE1hcmt1cEFjY3VtdWxhdG9yOjphcHBlbmRBdHRyaWJ1 dGUoVmVjdG9yPFVDaGFyPiYgb3V0LCBFbGVtZW50KiBlbGVtZW50LCBjbwogICAgIG91dC5hcHBl bmQoJz0nKTsKIAogICAgIGlmIChlbGVtZW50LT5pc1VSTEF0dHJpYnV0ZShjb25zdF9jYXN0PEF0 dHJpYnV0ZSo+KCZhdHRyaWJ1dGUpKSkKLSAgICAgICAgYXBwZW5kUXVvdGVkVVJMQXR0cmlidXRl VmFsdWUob3V0LCByZXNvbHZlVVJMSWZOZWVkZWQoZWxlbWVudCwgYXR0cmlidXRlLnZhbHVlKCkp KTsKKyAgICAgICAgYXBwZW5kUXVvdGVkVVJMQXR0cmlidXRlVmFsdWUob3V0LCBlbGVtZW50LCBh dHRyaWJ1dGUpOwogICAgIGVsc2UgewogICAgICAgICBvdXQuYXBwZW5kKCdcIicpOwogICAgICAg ICBhcHBlbmRBdHRyaWJ1dGVWYWx1ZShvdXQsIGF0dHJpYnV0ZS52YWx1ZSgpLCBkb2N1bWVudElz SFRNTCk7CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9lZGl0aW5nL01hcmt1cEFjY3VtdWxh dG9yLmggYi9Tb3VyY2UvV2ViQ29yZS9lZGl0aW5nL01hcmt1cEFjY3VtdWxhdG9yLmgKaW5kZXgg MWRkMWI2ZGVkMWIzYzQwZjg2NGM2NGMyODdjNmE3MjJkNWJmODU0Yi4uMjhhMmJlNTE5MTRmNmQ5 ZGM3NGE5MzQ0YTEzNzU3NmYyNjUzYmU5ZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvZWRp dGluZy9NYXJrdXBBY2N1bXVsYXRvci5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL2VkaXRpbmcvTWFy a3VwQWNjdW11bGF0b3IuaApAQCAtODAsNyArODAsNiBAQCBwcm90ZWN0ZWQ6CiAgICAgdm9pZCBj b25jYXRlbmF0ZU1hcmt1cChWZWN0b3I8VUNoYXI+JiBvdXQpOwogICAgIHZvaWQgYXBwZW5kQXR0 cmlidXRlVmFsdWUoVmVjdG9yPFVDaGFyPiYgcmVzdWx0LCBjb25zdCBTdHJpbmcmIGF0dHJpYnV0 ZSwgYm9vbCBkb2N1bWVudElzSFRNTCk7CiAgICAgdmlydHVhbCB2b2lkIGFwcGVuZEN1c3RvbUF0 dHJpYnV0ZXMoVmVjdG9yPFVDaGFyPiYsIEVsZW1lbnQqLCBOYW1lc3BhY2VzKik7Ci0gICAgdm9p ZCBhcHBlbmRRdW90ZWRVUkxBdHRyaWJ1dGVWYWx1ZShWZWN0b3I8VUNoYXI+JiByZXN1bHQsIGNv bnN0IFN0cmluZyYgdXJsU3RyaW5nKTsKICAgICB2b2lkIGFwcGVuZE5vZGVWYWx1ZShWZWN0b3I8 VUNoYXI+JiBvdXQsIGNvbnN0IE5vZGUqLCBjb25zdCBSYW5nZSosIEVudGl0eU1hc2spOwogICAg IGJvb2wgc2hvdWxkQWRkTmFtZXNwYWNlRWxlbWVudChjb25zdCBFbGVtZW50Kik7CiAgICAgYm9v bCBzaG91bGRBZGROYW1lc3BhY2VBdHRyaWJ1dGUoY29uc3QgQXR0cmlidXRlJiwgTmFtZXNwYWNl cyYpOwpAQCAtMTA1LDYgKzEwNCw3IEBAIHByb3RlY3RlZDoKIAogcHJpdmF0ZToKICAgICBTdHJp bmcgcmVzb2x2ZVVSTElmTmVlZGVkKGNvbnN0IEVsZW1lbnQqLCBjb25zdCBTdHJpbmcmIHVybFN0 cmluZykgY29uc3Q7CisgICAgdm9pZCBhcHBlbmRRdW90ZWRVUkxBdHRyaWJ1dGVWYWx1ZShWZWN0 b3I8VUNoYXI+JiByZXN1bHQsIGNvbnN0IEVsZW1lbnQqLCBjb25zdCBBdHRyaWJ1dGUmKTsKICAg ICB2b2lkIHNlcmlhbGl6ZU5vZGVzV2l0aE5hbWVzcGFjZXMoTm9kZSosIE5vZGUqIG5vZGVUb1Nr aXAsIEVDaGlsZHJlbk9ubHksIGNvbnN0IE5hbWVzcGFjZXMqKTsKIAogICAgIFZlY3RvcjxTdHJp bmc+IG1fc3VjY2VlZGluZ01hcmt1cDsK