88837 2012-06-11 22:36:08 -0700 [Chromium] Add WebDocument.images 2012-06-13 23:32:09 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 66687 0 jgreenwald webkit-unassigned abarth dglazkov fishd jamesr tkent+wkapi webkit.review.bot oldest_to_newest 646628 0 jgreenwald 2012-06-11 22:36:08 -0700 This is an API needed for Chrome. 647445 1 147184 jgreenwald 2012-06-12 16:09:40 -0700 Created attachment 147184 Patch 647446 2 webkit.review.bot 2012-06-12 16:11:52 -0700 Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. 647449 3 jamesr 2012-06-12 16:14:53 -0700 (In reply to comment #0) > This is an API needed for Chrome. Why? 647611 4 147184 abarth 2012-06-12 19:43:40 -0700 Comment on attachment 147184 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147184&action=review A few minor nits below. The main thing to improve about this patch is the ChangeLog. The ChangeLog should explain why we're making this change. In this case, I suspect the reason is that this API is needed by Chrome on Android for some purpose. It would be nice to elaborate a bit about that purpose, if you're able to. In the particular case of this patch, it's not a big deal because we're mirroring a long-established DOM API, but it's still a good practice when contributing to WebKit. Thanks for the patch! > Source/WebKit/chromium/src/WebDocument.cpp:142 > +void WebDocument::images(WebVector<WebElement>& results) const Why "const"? You're ending up const_casting the Document anyway. It seems better to model this after body() and head() and skip the const. > Source/WebKit/chromium/src/WebDocument.cpp:150 > + // Strange but true, sometimes node can be 0. I'd skip this comment. WebKit usually only includes comments if they something about why the code is the way it is. 648252 5 147184 jgreenwald 2012-06-13 10:57:40 -0700 Comment on attachment 147184 Patch I'll expand on the ChangeLog when I upload the next patch. Thanks for taking a look! View in context: https://bugs.webkit.org/attachment.cgi?id=147184&action=review >> Source/WebKit/chromium/src/WebDocument.cpp:142 >> +void WebDocument::images(WebVector<WebElement>& results) const > > Why "const"? You're ending up const_casting the Document anyway. It seems better to model this after body() and head() and skip the const. I originally modeled this after forms(), which is const. Actually, isn't body() const as well? Either way, I can remove const from this method. >> Source/WebKit/chromium/src/WebDocument.cpp:150 >> + // Strange but true, sometimes node can be 0. > > I'd skip this comment. WebKit usually only includes comments if they something about why the code is the way it is. Ok. For reference, the comment was copied from forms(). 648254 6 abarth 2012-06-13 10:59:27 -0700 Yeah, you're discovering that WebDocument.cpp was written a long time ago and we've learned a bunch since then. :) 648294 7 147374 jgreenwald 2012-06-13 11:36:51 -0700 Created attachment 147374 Patch 648298 8 147374 abarth 2012-06-13 11:40:00 -0700 Comment on attachment 147374 Patch Thanks! 648843 9 147374 webkit.review.bot 2012-06-13 23:32:04 -0700 Comment on attachment 147374 Patch Clearing flags on attachment: 147374 Committed r120284: <http://trac.webkit.org/changeset/120284> 648844 10 webkit.review.bot 2012-06-13 23:32:09 -0700 All reviewed patches have been landed. Closing bug. 147184 2012-06-12 16:09:40 -0700 2012-06-13 11:36:47 -0700 Patch bug-88837-20120612160939.patch text/plain 2655 jgreenwald U3VidmVyc2lvbiBSZXZpc2lvbjogMTIwMTA5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmluZGV4 IGNlOGZjZjE0OTg1Y2IzMDdiOTA5ZTgxMDE1ZjQ1NWI4YzEyZTM5NDUuLjk5Mjk2ZWNmMzgzOWI4 ZWVjMzc5NDBiM2I1ZjU4Mzk4N2I4MDZmMWYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTItMDYtMTIgIEplc3NlIEdyZWVud2FsZCAgPGpncmVlbndhbGRA Y2hyb21pdW0ub3JnPgorCisgICAgICAgIFtjaHJvbWl1bV0gQWRkIFdlYkRvY3VtZW50LmltYWdl cworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODg4MzcK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHB1Ymxp Yy9XZWJEb2N1bWVudC5oOgorICAgICAgICAoV2ViRG9jdW1lbnQpOgorICAgICAgICAqIHNyYy9X ZWJEb2N1bWVudC5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYkRvY3VtZW50OjppbWFnZXMpOgor ICAgICAgICAoV2ViS2l0KToKKwogMjAxMi0wNi0xMiAgQWRyaWVubmUgV2Fsa2VyICA8ZW5uZUBn b29nbGUuY29tPgogCiAgICAgICAgIFtjaHJvbWl1bV0gRml4IGluY29ycmVjdCBMYXllckNocm9t aXVtIHNjcm9sbCBwb3NpdGlvbiBmb3IgUlRMIG92ZXJmbG93IHBhZ2VzCmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViS2l0L2Nocm9taXVtL3B1YmxpYy9XZWJEb2N1bWVudC5oIGIvU291cmNlL1dlYktp dC9jaHJvbWl1bS9wdWJsaWMvV2ViRG9jdW1lbnQuaAppbmRleCA2YzBkZjMxYmNiYzYxNGZhYjIy ZDYzYzRlYmNiMmFlNTZjMTEwYjg0Li5jYzFkMGExY2NjYTZlY2M2Yzc5NzI2MTYxYmE4MWRiOTA4 ODEyNDE5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3B1YmxpYy9XZWJEb2N1 bWVudC5oCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vcHVibGljL1dlYkRvY3VtZW50LmgK QEAgLTk1LDYgKzk1LDcgQEAgcHVibGljOgogICAgIFdFQktJVF9FWFBPUlQgV2ViU3RyaW5nIHRp dGxlKCkgY29uc3Q7CiAgICAgV0VCS0lUX0VYUE9SVCBXZWJOb2RlQ29sbGVjdGlvbiBhbGwoKTsK ICAgICBXRUJLSVRfRVhQT1JUIHZvaWQgZm9ybXMoV2ViVmVjdG9yPFdlYkZvcm1FbGVtZW50PiYp IGNvbnN0OworICAgIFdFQktJVF9FWFBPUlQgdm9pZCBpbWFnZXMoV2ViVmVjdG9yPFdlYkVsZW1l bnQ+JikgY29uc3Q7CiAgICAgV0VCS0lUX0VYUE9SVCBXZWJVUkwgY29tcGxldGVVUkwoY29uc3Qg V2ViU3RyaW5nJikgY29uc3Q7CiAgICAgV0VCS0lUX0VYUE9SVCBXZWJFbGVtZW50IGdldEVsZW1l bnRCeUlkKGNvbnN0IFdlYlN0cmluZyYpIGNvbnN0OwogICAgIFdFQktJVF9FWFBPUlQgV2ViTm9k ZSBmb2N1c2VkTm9kZSgpIGNvbnN0OwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9jaHJvbWl1 bS9zcmMvV2ViRG9jdW1lbnQuY3BwIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvV2ViRG9j dW1lbnQuY3BwCmluZGV4IGQ0Y2QxOGIwMDczYTNjNjY4MGU3MDQ1MmY2MjQzZGI0NzkxMGUxNGQu LjZiMjk5YzM1MWNmODg1MWY0M2U0MDIxYWRkNWY3YmM3M2ViZjBhZDcgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL1dlYkRvY3VtZW50LmNwcAorKysgYi9Tb3VyY2UvV2Vi S2l0L2Nocm9taXVtL3NyYy9XZWJEb2N1bWVudC5jcHAKQEAgLTEzOSw2ICsxMzksMjEgQEAgV2Vi Tm9kZUNvbGxlY3Rpb24gV2ViRG9jdW1lbnQ6OmFsbCgpCiAgICAgcmV0dXJuIFdlYk5vZGVDb2xs ZWN0aW9uKHVud3JhcDxEb2N1bWVudD4oKS0+YWxsKCkpOwogfQogCit2b2lkIFdlYkRvY3VtZW50 OjppbWFnZXMoV2ViVmVjdG9yPFdlYkVsZW1lbnQ+JiByZXN1bHRzKSBjb25zdAoreworICAgIFJl ZlB0cjxIVE1MQ29sbGVjdGlvbj4gaW1hZ2VzID0gY29uc3RfY2FzdDxEb2N1bWVudCo+KGNvbnN0 VW53cmFwPERvY3VtZW50PigpKS0+aW1hZ2VzKCk7CisgICAgc2l6ZV90IHNvdXJjZUxlbmd0aCA9 IGltYWdlcy0+bGVuZ3RoKCk7CisgICAgVmVjdG9yPFdlYkVsZW1lbnQ+IHRlbXA7CisgICAgdGVt cC5yZXNlcnZlQ2FwYWNpdHkoc291cmNlTGVuZ3RoKTsKKyAgICBmb3IgKHNpemVfdCBpID0gMDsg aSA8IHNvdXJjZUxlbmd0aDsgKytpKSB7CisgICAgICAgIE5vZGUqIG5vZGUgPSBpbWFnZXMtPml0 ZW0oaSk7CisgICAgICAgIC8vIFN0cmFuZ2UgYnV0IHRydWUsIHNvbWV0aW1lcyBub2RlIGNhbiBi ZSAwLgorICAgICAgICBpZiAobm9kZSAmJiBub2RlLT5pc0hUTUxFbGVtZW50KCkpCisgICAgICAg ICAgICB0ZW1wLmFwcGVuZChXZWJFbGVtZW50KHN0YXRpY19jYXN0PEVsZW1lbnQqPihub2RlKSkp OworICAgIH0KKyAgICByZXN1bHRzLmFzc2lnbih0ZW1wKTsKK30KKwogdm9pZCBXZWJEb2N1bWVu dDo6Zm9ybXMoV2ViVmVjdG9yPFdlYkZvcm1FbGVtZW50PiYgcmVzdWx0cykgY29uc3QKIHsKICAg ICBSZWZQdHI8SFRNTENvbGxlY3Rpb24+IGZvcm1zID0gY29uc3RfY2FzdDxEb2N1bWVudCo+KGNv bnN0VW53cmFwPERvY3VtZW50PigpKS0+Zm9ybXMoKTsK 147374 2012-06-13 11:36:51 -0700 2012-06-13 23:32:04 -0700 Patch bug-88837-20120613113651.patch text/plain 2671 jgreenwald U3VidmVyc2lvbiBSZXZpc2lvbjogMTIwMTA5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmluZGV4 IGNlOGZjZjE0OTg1Y2IzMDdiOTA5ZTgxMDE1ZjQ1NWI4YzEyZTM5NDUuLjhkZWRlNjE1YzIzYjgx ZjA4OTg3ZDlkMjIyZWEyMmI1NWM0YjkzZjMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMTkgQEAKKzIwMTItMDYtMTMgIEplc3NlIEdyZWVud2FsZCAgPGpncmVlbndhbGRA Z29vZ2xlLmNvbT4KKworICAgICAgICBbQ2hyb21pdW1dIEFkZCBXZWJEb2N1bWVudC5pbWFnZXMK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTg4ODM3CisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhpcyBtZXRo b2QgaXMgbmVlZGVkIG9uIEFuZHJvaWQgdG8gc3VwcG9ydCB0aGUgZXhpc3RpbmcKKyAgICAgICAg V2ViVmlldy5kb2N1bWVudEhhc0ltYWdlcyBKYXZhIEFQSS4KKworICAgICAgICAqIHB1YmxpYy9X ZWJEb2N1bWVudC5oOgorICAgICAgICAoV2ViRG9jdW1lbnQpOgorICAgICAgICAqIHNyYy9XZWJE b2N1bWVudC5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYkRvY3VtZW50OjppbWFnZXMpOgorICAg ICAgICAoV2ViS2l0KToKKwogMjAxMi0wNi0xMiAgQWRyaWVubmUgV2Fsa2VyICA8ZW5uZUBnb29n bGUuY29tPgogCiAgICAgICAgIFtjaHJvbWl1bV0gRml4IGluY29ycmVjdCBMYXllckNocm9taXVt IHNjcm9sbCBwb3NpdGlvbiBmb3IgUlRMIG92ZXJmbG93IHBhZ2VzCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViS2l0L2Nocm9taXVtL3B1YmxpYy9XZWJEb2N1bWVudC5oIGIvU291cmNlL1dlYktpdC9j aHJvbWl1bS9wdWJsaWMvV2ViRG9jdW1lbnQuaAppbmRleCA2YzBkZjMxYmNiYzYxNGZhYjIyZDYz YzRlYmNiMmFlNTZjMTEwYjg0Li5mMjI5NmI3ZWVjMjM3YzY3YWNhYjk3NzNmNWMyOWRkZmY3OTFi MjZjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3B1YmxpYy9XZWJEb2N1bWVu dC5oCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vcHVibGljL1dlYkRvY3VtZW50LmgKQEAg LTk1LDYgKzk1LDcgQEAgcHVibGljOgogICAgIFdFQktJVF9FWFBPUlQgV2ViU3RyaW5nIHRpdGxl KCkgY29uc3Q7CiAgICAgV0VCS0lUX0VYUE9SVCBXZWJOb2RlQ29sbGVjdGlvbiBhbGwoKTsKICAg ICBXRUJLSVRfRVhQT1JUIHZvaWQgZm9ybXMoV2ViVmVjdG9yPFdlYkZvcm1FbGVtZW50PiYpIGNv bnN0OworICAgIFdFQktJVF9FWFBPUlQgdm9pZCBpbWFnZXMoV2ViVmVjdG9yPFdlYkVsZW1lbnQ+ Jik7CiAgICAgV0VCS0lUX0VYUE9SVCBXZWJVUkwgY29tcGxldGVVUkwoY29uc3QgV2ViU3RyaW5n JikgY29uc3Q7CiAgICAgV0VCS0lUX0VYUE9SVCBXZWJFbGVtZW50IGdldEVsZW1lbnRCeUlkKGNv bnN0IFdlYlN0cmluZyYpIGNvbnN0OwogICAgIFdFQktJVF9FWFBPUlQgV2ViTm9kZSBmb2N1c2Vk Tm9kZSgpIGNvbnN0OwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvV2Vi RG9jdW1lbnQuY3BwIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvV2ViRG9jdW1lbnQuY3Bw CmluZGV4IGQ0Y2QxOGIwMDczYTNjNjY4MGU3MDQ1MmY2MjQzZGI0NzkxMGUxNGQuLjA2M2Y3MTg5 MmU3NTg1NzA1ZjUwNmQ5YjcwYTQ4ZDI2MDVmYzg0NmYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJL aXQvY2hyb21pdW0vc3JjL1dlYkRvY3VtZW50LmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0L2Nocm9t aXVtL3NyYy9XZWJEb2N1bWVudC5jcHAKQEAgLTEzOSw2ICsxMzksMjAgQEAgV2ViTm9kZUNvbGxl Y3Rpb24gV2ViRG9jdW1lbnQ6OmFsbCgpCiAgICAgcmV0dXJuIFdlYk5vZGVDb2xsZWN0aW9uKHVu d3JhcDxEb2N1bWVudD4oKS0+YWxsKCkpOwogfQogCit2b2lkIFdlYkRvY3VtZW50OjppbWFnZXMo V2ViVmVjdG9yPFdlYkVsZW1lbnQ+JiByZXN1bHRzKQoreworICAgIFJlZlB0cjxIVE1MQ29sbGVj dGlvbj4gaW1hZ2VzID0gdW53cmFwPERvY3VtZW50PigpLT5pbWFnZXMoKTsKKyAgICBzaXplX3Qg c291cmNlTGVuZ3RoID0gaW1hZ2VzLT5sZW5ndGgoKTsKKyAgICBWZWN0b3I8V2ViRWxlbWVudD4g dGVtcDsKKyAgICB0ZW1wLnJlc2VydmVDYXBhY2l0eShzb3VyY2VMZW5ndGgpOworICAgIGZvciAo c2l6ZV90IGkgPSAwOyBpIDwgc291cmNlTGVuZ3RoOyArK2kpIHsKKyAgICAgICAgTm9kZSogbm9k ZSA9IGltYWdlcy0+aXRlbShpKTsKKyAgICAgICAgaWYgKG5vZGUgJiYgbm9kZS0+aXNIVE1MRWxl bWVudCgpKQorICAgICAgICAgICAgdGVtcC5hcHBlbmQoV2ViRWxlbWVudChzdGF0aWNfY2FzdDxF bGVtZW50Kj4obm9kZSkpKTsKKyAgICB9CisgICAgcmVzdWx0cy5hc3NpZ24odGVtcCk7Cit9CisK IHZvaWQgV2ViRG9jdW1lbnQ6OmZvcm1zKFdlYlZlY3RvcjxXZWJGb3JtRWxlbWVudD4mIHJlc3Vs dHMpIGNvbnN0CiB7CiAgICAgUmVmUHRyPEhUTUxDb2xsZWN0aW9uPiBmb3JtcyA9IGNvbnN0X2Nh c3Q8RG9jdW1lbnQqPihjb25zdFVud3JhcDxEb2N1bWVudD4oKSktPmZvcm1zKCk7Cg==