Bug 188185

Created attachment 346118 [details]
Patch

<rdar://problem/1234567>

Comment on attachment 346118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346118&action=review

We should add tests for this change. Use the _unittests.py files in Tools/Scripts/webkitpy/tool/steps as examples. You can run the WebKitPy tests using script test-webkitpy.

> Tools/ChangeLog:3
> +        test wkbi with webkit-patch upload

This title does not make sense. I think you meant, CC WebKit Bug Importer when uploading a patch.

> Tools/ChangeLog:8
> +

We should explain that the purpose of this change is to automatically CC WebKit Bug Importer and add keyword InRadar on Bugzilla bugs when a patch is uploaded to the bug that contain a radar URL.

> Tools/Scripts/webkitpy/tool/steps/addwkbi.py:27
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +#     * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#     * Redistributions in binary form must reproduce the above
> +# copyright notice, this list of conditions and the following disclaimer
> +# in the documentation and/or other materials provided with the
> +# distribution.
> +#     * Neither the name of Google Inc. nor the names of its
> +# contributors may be used to endorse or promote products derived from
> +# this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

This is not the correct license block. Please use the Apple license block text. The text is available at <https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/LICENSE-APPLE>.

> Tools/Scripts/webkitpy/tool/steps/addwkbi.py:46
> +    def GetRadarLinkFromDiff(self, diff):

This name does not match our code style convention of using all lowercase with underscores separating words.

> Tools/Scripts/webkitpy/tool/steps/addwkbi.py:58
> +        matcher = re.compile(r'(<?rdar:\/\/problems?\/)?(?P<radar_id>-?\d{7,})>?')
> +        match = matcher.search(diff)
> +
> +        if not match:
> +                return None
> +
> +        id = int(match.group('radar_id'))
> +        if id < 0:
> +            return None
> +
> +        link = '<rdar://problem/{}>'.format(id)
> +        return link

This code finds the first radar URL in the entire diff and returns it. This could cause us to falsely associate the bug with a radar URL that appears in an added or moved comment. It seems sufficient to look for the radar URL only in the added ChangeLog entries. Toward this we can make use of DiffParser to parse a patch and the ChangeLog parsing logic in file <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/checkout/changelog.py?rev=225733>. You may find inspiration for how to do this logic by looking at the class ValidateChangeLogs: <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/steps/validatechangelogs.py?rev=150404>.