Bug 31498

per discussion w/ eseidel, I'll start on this.

Created attachment 47175 [details] Patch

Just some items that I noticed in passing (drive by comments): 1. WebKitTools/simplejson/README.webkit refers to LICENSE.txt and there is none. 2. None of the files appear to have their license in them. (For example, the files in Scripts/webkitpy/layout_tests refer to LICENSE but there is no such file -- even if there was, there is no such .) 3. Several files have TODO(name) vs FIXME (which is done in WebKit land). 4. The python method/function names don't seem to follow PEP8 (lowercase with underscores). 5. There seem to be a lot of chrome specific reference in here that aren't in WebKit at all (For example there is a reference to "chrome\test\data\ssl\certs\root_ca_cert.crt" and test_output_formatter.bat uses chromiumpaths, etc.)

The patch is not correctly based. Consider using webkit-patch upload. Also, the patch is huge. I'm not sure how to meaningfully review it.

(In reply to comment #3) > Just some items that I noticed in passing (drive by comments): > 1. WebKitTools/simplejson/README.webkit refers to LICENSE.txt and there is > none. Hmm. The downstream version didn't have a LICENSE as far as I know. I'll hunt one down after the patch lands, if that's okay. > 2. None of the files appear to have their license in them. (For example, the > files in Scripts/webkitpy/layout_tests refer to LICENSE but there is no such > file -- even if there was, there is no such .) Odd. I'll take a look at that. > 3. Several files have TODO(name) vs FIXME (which is done in WebKit land). I'll clean that up after the patch lands, if that's okay ... > 4. The python method/function names don't seem to follow PEP8 (lowercase with > underscores). The code was cleaned up to PEP-8 compliance as enforced by pep8.py ; I think we'll gradually rename the functions over time to get the rest of the cleanup. > 5. There seem to be a lot of chrome specific reference in here that aren't in > WebKit at all (For example there is a reference to > "chrome\test\data\ssl\certs\root_ca_cert.crt" and test_output_formatter.bat > uses chromiumpaths, etc.) Yeah, the code doesn't run independently of chromium yet, and there's still a bunch of cleanup to do. I submitted this as-is per discussion with eseidel and some other people on #webkit who were keen to see it land so they could help contribute to the upstreaming and cleanup.

(In reply to comment #4) > The patch is not correctly based. Consider using webkit-patch upload. I don't understand what this means. I used webkit-patch post (I think). Is that different? > > Also, the patch is huge. I'm not sure how to meaningfully review it. > I'm expecting eseidel to review it :) Since the only intended purpose at the moment is to be able to run the chromium tests, you could compare it against the downstream version as a sanity check. Even if I were to attempt to upload it in somewhat separable chunks, most of them would still be huge :(

(In reply to comment #5) > > 4. The python method/function names don't seem to follow PEP8 (lowercase with > > underscores). > > The code was cleaned up to PEP-8 compliance as enforced by pep8.py ; I think > we'll gradually rename the functions over time to get the rest of the cleanup. The concern with this idea (and related responses) because once it lands, there is much less motivation to clean up things. (I've done it too unfortunately at times where I have less time to clean up something that has landed vs something that hasn't.) The "I think" and "gradually" above reinforce this perception. Alternatively this could be cleaned up downstream and then landed in WebKit.

(In reply to comment #6) > (In reply to comment #4) > > The patch is not correctly based. Consider using webkit-patch upload. > > I don't understand what this means. I used webkit-patch post (I think). Is that > different? Oh, you're right. It is correctly based (created from the proper directory). PrettyPatch tricked me. Not sure why it didn't apply to TOT.

I don't think this patch want's a "code-review". We know the code works, we know it will need further cleanuyp/style-adjusting, testing, reorg, etc. over time. The one thing we do need to review, is licenses. WebKit style is to have a license at the top of every file. I also think we need to specify who the "chromium authors" Are since we dont have any of the chromium license files in our tree. Other than that, I'm ready to rs=me this change. So: we need to add a BSD license header to teh top of every file, with a list of copyright holders (I don't think the chromium authors counts here, or?) At least thats my understanding of webkit's contribution requirements. Please someone correct me if i've misunderstood.

Created attachment 47206 [details] Start of described python program (In reply to comment #9) > I don't think this patch want's a "code-review". We know the code works, we > know it will need further cleanuyp/style-adjusting, testing, reorg, etc. over > time. I'm *very* concerned about landing this code as is. Landing it removes any urgency to fix its bad style. If the past is a guide, this will languish, and the file will be in the wrong style for a long time to come and serve as a bad example of style that is checked in. Then folks will use it as an example of style that WebKit except when code reviews are done -- also based on recent experience :(. *Instead* one could take a little time to write a relatively simple python program to fix the function names and do this upstream. This may be a useful program if anything else needs to come from chromium (due to chromium's unfortunate python style): Here's an outline of that program: for each file in python patch for each line if line matches r"def\s+([A-Za-z_][A-Za-z_0-9])*\s*\(": get lower case function name put orig function name in dict with lower case name for each file in python patch for each line for each item in dict if line matches r"\s+"+item_from_dict: replace item_from_dict with lowercase version write file Now this may make some mistakes, but a code review should be able to verify it as well as running the program as a double check. I attached a python program that I started a while ago (when I had some spare moments) which is headed in this direction but still needs work to finish it off.

> So: we need to add a BSD license header to teh top of every file, with a list > of copyright holders (I don't think the chromium authors counts here, or?) In principle, you need to list all the Chromium Authors as they are the copyright holders. In practice, you should look at the SVN log and see who actually touched the file. Everyone employed by Google can be listed as Google, Inc because the code was produces as a work-for-hire. I am not a lawyer but I play one on the Interwebs.

Comment on attachment 47175 [details] Patch As I understand it, there will be another revision of the patch. r- for now.

Created attachment 47389 [details] file that changes any MixedCase function names to lower_case_with_underscores I've attached the script I'm using to do the bulk-rename of any function and method names. There are a couple of big gotchas with the script: 1) If there are namespace collisions, they will not be caught. In other words, if you want to rename foo.Run() and not bar.Run(), you're out of luck. 2) It won't rename any occurrences of the names that appear inside strings or multi-line comments And, obviously, it can't rename occurrences that you don't tell it about, so if other files are calling these functions, they'll break. As far as I can tell, for the task at hand, none of these gotchas matter.

Created attachment 47645 [details] revised patch with pep-8 compliant function names and an initial pass at the licenses. Only for review - do not commit

We could do this commit in various pieces. For one, it would be simple to add simplejson in one commit. I'm not sure what review comments are really expected for a .5MB patch. It's basically unreviewable at this size. I'm also not sure it needs much review beyond "the licenses look right". This is a huge hunk of code which will need to be worked over in th webkit repository, and would be impossible to work over via the review process on this bug.

Created attachment 47651 [details] ready for review + commit (license text updated

(In reply to comment #15) > I'm not sure what review comments are really expected for a .5MB patch. It's > basically unreviewable at this size. > > I'm also not sure it needs much review beyond "the licenses look right". This > is a huge hunk of code which will need to be worked over in th webkit > repository, and would be impossible to work over via the review process on this > bug. This sounds a lot like comments for the patches attached to Bug 16401. Why can't this script be made to run first before landing? How will landing it in the repository help to get it fixed?

(In reply to comment #17) > (In reply to comment #15) > > I'm not sure what review comments are really expected for a .5MB patch. It's > > basically unreviewable at this size. > > > > I'm also not sure it needs much review beyond "the licenses look right". This > > is a huge hunk of code which will need to be worked over in th webkit > > repository, and would be impossible to work over via the review process on this > > bug. > > This sounds a lot like comments for the patches attached to Bug 16401. Why > can't this script be made to run first before landing? How will landing it in > the repository help to get it fixed? Hi David, I think perhaps you misunderstand. The "file that changes any MixedCase function" script has already been run and we're not planning to land it; it's just attached for reference. Rather, we're talking about the last attachment ( https://bugs.webkit.org/attachment.cgi?id=47651 ), which are the actual files that we're upstreaming from chromium.org now that they've been reformatted using the script.

(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #15) > > > I'm not sure what review comments are really expected for a .5MB patch. It's > > > basically unreviewable at this size. > > > > > > I'm also not sure it needs much review beyond "the licenses look right". This > > > is a huge hunk of code which will need to be worked over in th webkit > > > repository, and would be impossible to work over via the review process on this > > > bug. > > > > This sounds a lot like comments for the patches attached to Bug 16401. Why > > can't this script be made to run first before landing? How will landing it in > > the repository help to get it fixed? > > I think perhaps you misunderstand. The "file that changes any MixedCase > function" script has already been run and we're not planning to land it; it's > just attached for reference. Rather, we're talking about the last attachment ( > https://bugs.webkit.org/attachment.cgi?id=47651 ), which are the actual files > that we're upstreaming from chromium.org now that they've been reformatted > using the script. No, I was referring to the run_webkit_tests script itself. Why does checking it into the WebKit repository make it easier to work on (e.g., is the script really so big that one person can't fix it up before checking it in)? What are the barriers to making it actually work (or do something useful) before being committed?

(In reply to comment #19) > No, I was referring to the run_webkit_tests script itself. Why does checking > it into the WebKit repository make it easier to work on (e.g., is the script > really so big that one person can't fix it up before checking it in)? What are > the barriers to making it actually work (or do something useful) before being > committed? Oh, sorry, I misunderstood. The script as it is runs fine for running the chromium version of the test suite. The intent of checking this in at all was (a) to move this stuff upstream alongside the rest of the chromium port and (b) make this implementation available for other ports to use, since it has some advantages over the perl versions. Eric and I are working on getting it to work for the base webkit ports, but there was demand (from Eric and some others) to check this in so that they could help with the porting, rather than waiting until I had every i dotted.

Created attachment 47745 [details] Patch

Created attachment 47746 [details] Patch

Comment on attachment 47746 [details] Patch Looks great! Obviously we still have a lot of work here. Thank you for your persistence!

Committed r54091: <http://trac.webkit.org/changeset/54091>

Created attachment 47747 [details] Patch

Comment on attachment 47747 [details] Patch Looks good to me. Classes are *soo* much cleaner than what we have in the perl code. This change is nice and small and seems pretty straightforward to me. Thanks!

Comment on attachment 47747 [details] Patch Was committed as <http://trac.webkit.org/changeset/54093

Created attachment 47749 [details] Patch

Comment on attachment 47749 [details] Patch Looks non-harmful. So glad to finally have this runable from webkit.org

Committed r54094: <http://trac.webkit.org/changeset/54094>

Created attachment 47750 [details] Patch

Comment on attachment 47750 [details] Patch After talking in person, let's wait on this part of the patch, to see if anyone is actually still using this code. Looks like we're otherwise done here! And can close this bug.

Would r54903 or r54904 have caused any changes in Windows expected results? Right around r54093-r54095 the number of failures on Windows went from 4 to 18. I sent a comment to the author of r54095, and just wanted to put one here to ask if this could have changed Windows expected results (especially in media/).

(In reply to comment #33) > Would r54903 or r54904 have caused any changes in Windows expected results? > Right around r54093-r54095 the number of failures on Windows went from 4 to 18. > > I sent a comment to the author of r54095, and just wanted to put one here to > ask if this could have changed Windows expected results (especially in media/). Not likely. This code should be completely unused at the moment.

(In reply to comment #34) > (In reply to comment #33) > > Would r54903 or r54904 have caused any changes in Windows expected results? > > Right around r54093-r54095 the number of failures on Windows went from 4 to 18. > > > > I sent a comment to the author of r54095, and just wanted to put one here to > > ask if this could have changed Windows expected results (especially in media/). > > Not likely. This code should be completely unused at the moment. Agreed, it looks like it was r54095. Sorry for the spam.