82442 2012-03-28 02:45:03 -0700 Consider removal of removes jsNull()/v8::Null() from JSInternalsCustom.cpp/V8InternalsCustom.cpp 2012-03-28 22:47:39 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 82319 1 code.vineet webkit-unassigned eric.carlson haraken webkit.review.bot oldest_to_newest 589783 0 code.vineet 2012-03-28 02:45:03 -0700 This has comeup with https://bugs.webkit.org/show_bug.cgi?id=82319#c3 We would like to know if it an expected behaviour for Internals.idl to return null when the vector is empty? 589802 1 134250 code.vineet 2012-03-28 03:04:39 -0700 Created attachment 134250 proposed patch Attacjing proposed patch. This will test if any test case failures. Please us know if any objections/suggestions. Thanks. 589808 2 haraken 2012-03-28 03:12:10 -0700 eric.carlson: Would you take a look at the change? (See also https://bugs.webkit.org/show_bug.cgi?id=82319) The behavior when the Vector is empty is not spec-ed, and we guess that we can remove the code that returns null when the Vector is empty. 589809 3 haraken 2012-03-28 03:13:35 -0700 (In reply to comment #1) > Attacjing proposed patch. This will test if any test case failures. If no existing tests are affected, maybe we want to add test cases for this. 589836 4 code.vineet 2012-03-28 04:17:18 -0700 (In reply to comment #3) > (In reply to comment #1) > > Attaching proposed patch. This will test if any test case failures. > > If no existing tests are affected, maybe we want to add test cases for this. Currently there is already a test case http://trac.webkit.org/browser/trunk/LayoutTests/fast/harness/user-preferred-language.html?rev=105315#L20 which checks for empty vector but this wont fail even if we remove if (languages.isEmpty()) return jsNull(); Because all the platform have the implementation for platformLanguages(). 590081 5 eric.carlson 2012-03-28 10:14:10 -0700 (In reply to comment #2) > eric.carlson: Would you take a look at the change? (See also https://bugs.webkit.org/show_bug.cgi?id=82319) The behavior when the Vector is empty is not spec-ed, and we guess that we can remove the code that returns null when the Vector is empty. This seems like a reasonable change to me. 590108 6 code.vineet 2012-03-28 10:33:47 -0700 (In reply to comment #5) > (In reply to comment #2) > > eric.carlson: Would you take a look at the change? (See also https://bugs.webkit.org/show_bug.cgi?id=82319) The behavior when the Vector is empty is not spec-ed, and we guess that we can remove the code that returns null when the Vector is empty. > > This seems like a reasonable change to me. As per comment #4 the test case covers empty vector case. Do I need to add more coverage for the test? 590492 7 134250 haraken 2012-03-28 15:08:31 -0700 Comment on attachment 134250 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=134250&action=review > Source/WebCore/ChangeLog:9 > + No new tests. LayoutTests/fast/harness/user-preferred-language.html should pass > + even after these changes. OK, it would be hard to add a new test case for this change. 590859 8 134250 webkit.review.bot 2012-03-28 22:47:34 -0700 Comment on attachment 134250 proposed patch Clearing flags on attachment: 134250 Committed r112502: <http://trac.webkit.org/changeset/112502> 590860 9 webkit.review.bot 2012-03-28 22:47:39 -0700 All reviewed patches have been landed. Closing bug. 134250 2012-03-28 03:04:39 -0700 2012-03-28 22:47:34 -0700 proposed patch intermediate_001.diff text/plain 2275 code.vineet SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExMjM3NykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDEyLTAzLTI4ICBWaW5lZXQg Q2hhdWRoYXJ5ICA8cmdmNzQ4QG1vdG9yb2xhLmNvbT4KKworICAgICAgICBDb25zaWRlciByZW1v dmFsIG9mIHJlbW92ZXMganNOdWxsKCkvdjg6Ok51bGwoKSBmcm9tIEpTSW50ZXJuYWxzQ3VzdG9t LmNwcC9WOEludGVybmFsc0N1c3RvbS5jcHAKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTgyNDQyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiBMYXlvdXRUZXN0cy9mYXN0L2hhcm5lc3Mv dXNlci1wcmVmZXJyZWQtbGFuZ3VhZ2UuaHRtbCBzaG91bGQgcGFzcworICAgICAgICBldmVuIGFm dGVyIHRoZXNlIGNoYW5nZXMuCisKKyAgICAgICAgKiB0ZXN0aW5nL2pzL0pTSW50ZXJuYWxzQ3Vz dG9tLmNwcDogUmVtb3ZlIGpzTnVsbCgpIGlmIFZlY3RvciBpcyBlbXB0eS4KKyAgICAgICAgKFdl YkNvcmU6OkpTSW50ZXJuYWxzOjp1c2VyUHJlZmVycmVkTGFuZ3VhZ2VzKToKKyAgICAgICAgKiB0 ZXN0aW5nL3Y4L1Y4SW50ZXJuYWxzQ3VzdG9tLmNwcDogUmVtb3ZlIHY4OjpOdWxsKCkgaWYgVmVj dG9yIGlzIGVtcHR5LgorICAgICAgICAoV2ViQ29yZTo6VjhJbnRlcm5hbHM6OnVzZXJQcmVmZXJy ZWRMYW5ndWFnZXNBY2Nlc3NvckdldHRlcik6CisKIDIwMTItMDMtMjggIExpIFlpbiAgPGxpLnlp bkBpbnRlbC5jb20+CiAKICAgICAgICAgW1dlYlNvY2tldF1UaGUgU2VjLVdlYlNvY2tldC1Qcm90 b2NvbCBtdXN0IG5vdCBhcHBlYXIgbW9yZSB0aGFuIG9uY2UgaW4gYW4gSFRUUCByZXNwb25zZQpJ bmRleDogU291cmNlL1dlYkNvcmUvdGVzdGluZy9qcy9KU0ludGVybmFsc0N1c3RvbS5jcHAKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvdGVzdGluZy9qcy9KU0ludGVybmFsc0N1c3RvbS5j cHAJKHJldmlzaW9uIDExMjI4MCkKKysrIFNvdXJjZS9XZWJDb3JlL3Rlc3RpbmcvanMvSlNJbnRl cm5hbHNDdXN0b20uY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zNiw4ICszNiw2IEBAIEpTVmFsdWUg SlNJbnRlcm5hbHM6OnVzZXJQcmVmZXJyZWRMYW5ndWEKIHsKICAgICBJbnRlcm5hbHMqIGltcCA9 IHN0YXRpY19jYXN0PEludGVybmFscyo+KGltcGwoKSk7CiAgICAgY29uc3QgVmVjdG9yPFN0cmlu Zz4gbGFuZ3VhZ2VzID0gaW1wLT51c2VyUHJlZmVycmVkTGFuZ3VhZ2VzKCk7Ci0gICAgaWYgKGxh bmd1YWdlcy5pc0VtcHR5KCkpCi0gICAgICAgIHJldHVybiBqc051bGwoKTsKICAgICAKICAgICBN YXJrZWRBcmd1bWVudEJ1ZmZlciBhcnJheTsKICAgICBWZWN0b3I8U3RyaW5nPjo6Y29uc3RfaXRl cmF0b3IgZW5kID0gbGFuZ3VhZ2VzLmVuZCgpOwpJbmRleDogU291cmNlL1dlYkNvcmUvdGVzdGlu Zy92OC9WOEludGVybmFsc0N1c3RvbS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUv dGVzdGluZy92OC9WOEludGVybmFsc0N1c3RvbS5jcHAJKHJldmlzaW9uIDExMjI4MCkKKysrIFNv dXJjZS9XZWJDb3JlL3Rlc3RpbmcvdjgvVjhJbnRlcm5hbHNDdXN0b20uY3BwCSh3b3JraW5nIGNv cHkpCkBAIC0zNSw4ICszNSw2IEBAIHY4OjpIYW5kbGU8djg6OlZhbHVlPiBWOEludGVybmFsczo6 dXNlclAKIHsKICAgICBJbnRlcm5hbHMqIGludGVybmFscyA9IFY4SW50ZXJuYWxzOjp0b05hdGl2 ZShpbmZvLkhvbGRlcigpKTsKICAgICBjb25zdCBWZWN0b3I8U3RyaW5nPiBsYW5ndWFnZXMgPSBp bnRlcm5hbHMtPnVzZXJQcmVmZXJyZWRMYW5ndWFnZXMoKTsKLSAgICBpZiAobGFuZ3VhZ2VzLmlz RW1wdHkoKSkKLSAgICAgICAgcmV0dXJuIHY4OjpOdWxsKCk7CiAKICAgICB2ODo6TG9jYWw8djg6 OkFycmF5PiBhcnJheSA9IHY4OjpBcnJheTo6TmV3KGxhbmd1YWdlcy5zaXplKCkpOwogICAgIFZl Y3RvcjxTdHJpbmc+Ojpjb25zdF9pdGVyYXRvciBlbmQgPSBsYW5ndWFnZXMuZW5kKCk7Cg==