Bug 174596

<rdar://problem/33183986>

Created attachment 315733 [details] v1 patch for feedback

Created attachment 315822 [details] v2 patch with small correction to typos

Just some high level usability comments: - A start point should be required, an end point should not - bisect-webkit-builds with no argument should print the help - You give mac a version, but not iOS. Why? This seems especially strange since you only list one Mac version. We should have El Capitan builds too, right? - It would be nice if the launching had a default behavior. Perhaps run-safari by default. - Could the script accept a bash command with some pre-determined variable (like $WEBKIT_PATH?) so that the running could be highly customizable?

(In reply to Jonathan Bedard from comment #4) > Just some high level usability comments: > > - A start point should be required, an end point should not I'm not convinced a starting or ending point should be required. I am ok with printing a warning if one is not passed in, but I don't think it should be mandatory. The database will only have builds in it within our retention time frame, and given the number of trunk commits, I expect the total number of revisions over 2.5 years (our retention time), will be less than 40,000. That's between 15 and 16 iterations at most. 2^16 = 65536 > - bisect-webkit-builds with no argument should print the help I think this is probably a good idea. I'm currently defaulting the platform, which I expect will not the the final version. It may work best if we require a platform to be passed in. I'm not sure how else we could accurately target other ports. > - You give mac a version, but not iOS. Why? This seems especially strange > since you only list one Mac version. We should have El Capitan builds too, > right? I listed one sample for each platform we currently support. This is probably not ideal. Maybe we should print out a list of supported platforms, if platform is not passed in? That might solve the previous thought... > - It would be nice if the launching had a default behavior. Perhaps > run-safari by default. run-safari is great for Mac... but how about WPE/GTK? Do you think it's acceptable to have a default launch behavior for each platform? We could make it easy so other platforms could simply add their default behavior to the code... what is the default launch action for iOS? Can we even realistically run this on iOS? Maybe with Simulator? > - Could the script accept a bash command with some pre-determined variable > (like $WEBKIT_PATH?) so that the running could be highly customizable? I think requiring a variable is a barrier to entry. I'd rather let each port implement their own logic.

Created attachment 316072 [details] v3 patch to address feedback

(In reply to Lucas Forschler from comment #5) Generally, I like your responses, just a few notes to add. > > - You give mac a version, but not iOS. Why? This seems especially strange > > since you only list one Mac version. We should have El Capitan builds too, > > right? > > I listed one sample for each platform we currently support. This is probably > not ideal. Maybe we should print out a list of supported platforms, if > platform is not passed in? That might solve the previous thought... > You're also using python, so if you leverage webkitpy, we could default to the current platform. > > > - It would be nice if the launching had a default behavior. Perhaps > > run-safari by default. > > run-safari is great for Mac... but how about WPE/GTK? Do you think it's > acceptable to have a default launch behavior for each platform? We could > make it easy so other platforms could simply add their default behavior to > the code... Don't WPE/GTK have MiniBrowser? > what is the default launch action for iOS? Can we even realistically run > this on iOS? Maybe with Simulator? We can't realistically run on a device, no. In fact, I wouldn't even provide device spades. We could run with the Simulator, although, that may be a good enhancement for later. > > - Could the script accept a bash command with some pre-determined variable > > (like $WEBKIT_PATH?) so that the running could be highly customizable? > > I think requiring a variable is a barrier to entry. I'd rather let each port > implement their own logic. I wouldn't suggest requiring it (I think all ports should have a default behavior) but I can see someone want to, for example, test a regression in Mail instead of Safari.

Created attachment 316101 [details] v4 patch which enables run-safari

(In reply to Lucas Forschler from comment #8) > Created attachment 316101 [details] > v4 patch which enables run-safari A few comments: I think maybe a --list to list available revisions might be nice When I try running this, I get this error: 'Can't find built framework at "/PATH_TO_WEBKIT/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/JavaScriptCore".' Seems like we might need to either have a custom run-safari or pass run-safari arguments (seems like the later is preferable) since run-safari, by default, uses your webkit build directory.

(In reply to Jonathan Bedard from comment #9) > (In reply to Lucas Forschler from comment #8) > > Created attachment 316101 [details] > > v4 patch which enables run-safari > > A few comments: > > I think maybe a --list to list available revisions might be nice > > When I try running this, I get this error: > 'Can't find built framework at > "/PATH_TO_WEBKIT/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/ > JavaScriptCore".' > > Seems like we might need to either have a custom run-safari or pass > run-safari arguments (seems like the later is preferable) since run-safari, > by default, uses your webkit build directory. I think a --list option could be cool... we can add that as a new feature request. I also think it would be good to list supported platforms/architectures, as well as revisions. As for the WebKitBuild folder: I expect it to run from your default WebKitBuild directory... that's by design. The download and extract steps put the files exactly there. (this is the same way our bots work). I don't reproduce your error... I wonder what's going on here. What command line are you passing?

(In reply to Lucas Forschler from comment #10) > (In reply to Jonathan Bedard from comment #9) > > (In reply to Lucas Forschler from comment #8) > > > Created attachment 316101 [details] > > > v4 patch which enables run-safari > > > > A few comments: > > > > I think maybe a --list to list available revisions might be nice > > > > When I try running this, I get this error: > > 'Can't find built framework at > > "/PATH_TO_WEBKIT/WebKitBuild/Debug/JavaScriptCore.framework/Versions/A/ > > JavaScriptCore".' > > > > Seems like we might need to either have a custom run-safari or pass > > run-safari arguments (seems like the later is preferable) since run-safari, > > by default, uses your webkit build directory. > > I think a --list option could be cool... we can add that as a new feature > request. I also think it would be good to list supported > platforms/architectures, as well as revisions. > > As for the WebKitBuild folder: > > I expect it to run from your default WebKitBuild directory... that's by > design. The download and extract steps put the files exactly there. (this is > the same way our bots work). I don't reproduce your error... I wonder what's > going on here. What command line are you passing? Ok, set-webkit-configuration controls this... we'll need to be smart about detecting current configuration, or telling the user to run set-webkit-configuration... or resetting it. I think resetting it for the user is not the best idea. Probably it's best to see if the chosen configuration matches what is set by the set-webkit-configuration script, and then bailing out if they don't align.

(In reply to Lucas Forschler from comment #11) > ... > > Ok, set-webkit-configuration controls this... we'll need to be smart about > detecting current configuration, or telling the user to run > set-webkit-configuration... or resetting it. I think resetting it for the > user is not the best idea. Probably it's best to see if the chosen > configuration matches what is set by the set-webkit-configuration script, > and then bailing out if they don't align. I think it's fine to set the configuration if the script has explicitly been asked for debug or release. That is inline with what we do when building and running the layout tests.

Created attachment 316392 [details] patch for review This patch adds functionality to set a WEBKIT_OUTPUDIR, which will be used instead of the default WEBKITBUILD dir. This prevents the tool from overwriting engineering built product in the WebKitBuild folder.

Comment on attachment 316392 [details] patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=316392&action=review Great work! This looks so simple compared to other logic I've seen for similar functionality. At a high level, you may consider using snake_case instead of camelCase. > Tools/Scripts/bisect-webkit-builds:47 > + 'Find rightmost value less than or equal to x' Nit: Use triple double quotes here like so: """Find rightmost value less than or requal to x""" > Tools/Scripts/bisect-webkit-builds:55 > + 'Find leftmost item greater than or equal to x' Ditto. > Tools/Scripts/bisect-webkit-builds:67 > + parser.add_argument('-p', '--platform', default='None', help='The platform to query [mac-sierra | gtk | ios-simulator | win]') Instead of default=None, you could do required=True, which would allow you to skip the sys.argv check below, and it would automatically print the help if I recall correctly. > Tools/Scripts/bisect-webkit-builds:68 > + parser.add_argument('-f', '--full', action='store_true', help='Use full archives containing debug symbols. These are significantly larger files!') Nit: When using action='store_true' also use default=False to be explicit. > Tools/Scripts/bisect-webkit-builds:73 > + exit(-1) Nit the above 3 lines can be removed via suggestions above. > Tools/Scripts/bisect-webkit-builds:77 > +def getSortedRevisions(revisionsJSON): Nit: This is a dictionary as compared to JSON; we should name it revisions_dict (or revisionsDict, if you want to use camelCase style) instead. > Tools/Scripts/bisect-webkit-builds:80 > + revisions.append(int(revision['revision'])) Nit: This could be rewritten for conciseness, like so: def getSortedRevisions(revisionsDict): revisions = [int(revision['revision']) for revision in revisionsDict['revisions']] return sorted(revisions) > Tools/Scripts/bisect-webkit-builds:86 > + url = '/'.join([url, str(revisionToDownload)]) Nit: the [ ] brackets are extraneous. > Tools/Scripts/bisect-webkit-builds:90 > + #revision = archive['revision'] Nit: No need to commit commented code :) > Tools/Scripts/bisect-webkit-builds:92 > + return s3_url This could also be rewritten more succinctly: def getS3LocationForRevision(url, revision): url = '/'.join(url, str(revision)) archives = requests.get(url).json()['archive'] archive_urls = [archive['s3_url'] for archive in archives if archive['revision'] == str(revision)] if not archive_urls: raise Exception("Could not find archive for revision '{revision}'!".format(revision=revision)) return archive_urls[0] > Tools/Scripts/bisect-webkit-builds:96 > + base_url = getRestApiUrl(options) Nit: Rest => REST (Representational State Transfer) Api => API (Application Program(ming) Interface) URL => URL (Unified Resource Link) Wow, what a mouthful. Written out that'd be base_url = getRepresentationalStateTransferApplicationProgrammingInterfaceUnifiedResourceLink. Maybe rename the function we're calling to getS3BaseURL? > Tools/Scripts/bisect-webkit-builds:118 > + promptUser Did you mean promptUser()? Also, the `else` could be omitted. > Tools/Scripts/bisect-webkit-builds:131 > + print('Default test behavior for this platform is not implemented...'.format(options.platform)) Shouldn't we probably bail out here? If so, we can remove the command = [] from above, and the `if command` check below. > Tools/Scripts/bisect-webkit-builds:136 > + return promptUser() Maybe it'd be more clear to assign the value of promptUser to a variable like "issue_reproduces" as to more easily follow the data transfer between functions, and then `return issue_reproduces`. > Tools/Scripts/bisect-webkit-builds:140 > + revisionsRemaining = (endIndex-startIndex)+1 Super-nit: Add spacing inbetween operators: revisionsRemaining = (endIndex - startIndex) + 1 > Tools/Scripts/bisect-webkit-builds:147 > + middleIndex = (startIndex + endIndex)/2 Ditto. > Tools/Scripts/bisect-webkit-builds:151 > + endRevision = revisionList[endIndex] Are these three above calls necessary? Below we only seem to be using middleIndex. > Tools/Scripts/bisect-webkit-builds:195 > + url = REST_API_URL + REST_API_MINIFIED_ENDPOINT Nit: Move this assignment above the if statement and you can remove the `else` line. I'd also recommend calling this base_api_url, instead of just url. > Tools/Scripts/bisect-webkit-builds:196 > + url = url + '-'.join([options.platform, options.architecture, options.configuration]) Then we can call this something like: full_api_url (or something better). Also, if --platform is 'None' will this still work? > Tools/Scripts/bisect-webkit-builds:200 > +def getSupportedFullPlatforms(): Can we rename this to something like: unminifiedPlatforms, instead? 'get' seems extraneous and 'Full' doesn't describe the platforms well, imo. Maybe a docstring would help? > Tools/Scripts/bisect-webkit-builds:215 > + else: Nit: else is unnecessary. > Tools/Scripts/bisect-webkit-builds:222 > + exit(-1) This can be handled with required=True. > Tools/Scripts/bisect-webkit-builds:236 > + url = getRestApiUrl(options) ditto to previous comments on this function. > Tools/Scripts/bisect-webkit-builds:237 > + r = requests.get(url) I would just go straight for revisions here like so: revisions = requests.get(url)['revisionsJSON'] sortedRevisions = sortRevisions(revisions) > Tools/Scripts/bisect-webkit-builds:240 > + startIndex, endIndex = getIndexesFromRevisions(revisionList, options.start, options.end) Nit: Indexes => Indices > Tools/Scripts/bisect-webkit-builds:247 > + print('Setting WEBKIT_OUTPUTDIR to {}'.format(outputDir)) Might be worth mentioning you're setting the env var WEBKIT_OUTPUTDIR.

Comment on attachment 316392 [details] patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=316392&action=review > Tools/Scripts/bisect-webkit-builds:31 > +import json Unused import statement. > Tools/Scripts/bisect-webkit-builds:34 > +import re Unused import statement. > Tools/Scripts/bisect-webkit-builds:39 > +# FIXME: This is the /test/ REST API, it is not yet published to production Suggest: # FIXME: whenever we publish the API to production, remove /test REST API URL. > Tools/Scripts/bisect-webkit-builds:44 > + nit: Add newline to adhere to PEP8. > Tools/Scripts/bisect-webkit-builds:47 > + 'Find rightmost value less than or equal to x' Please use `"""` as docstring delimiters. Nit: add period. > Tools/Scripts/bisect-webkit-builds:50 > + return i-1 Suggest adding spaces in your subtraction: `return i - 1` > Tools/Scripts/bisect-webkit-builds:63 > +def parseCommandLine(args): For consistency and for PEP8 style adherence we should use underscored function names: `def parse_command_line(args):` == Nit: `parse_arguments()` or `parse_args()` may be a better name for this. > Tools/Scripts/bisect-webkit-builds:77 > +def getSortedRevisions(revisionsJSON): So it seems this is really fetching available revisions for bisection. I think the function should be named as such. Also, depending on how the API is structured, could there could be a nice way to have AWS returned this already structured data? == I think this function should be the one composing the URL to fetch the revision information from instead of being on main(). > Tools/Scripts/bisect-webkit-builds:80 > + revisions.append(int(revision['revision'])) Instead of adding and then sorting, I think we can use the `bisect` module to sort the revisions at insert-time. Example: ```python import bisect revisions = [] for revision in revisionsJSON['revisions']: bisect.insort(revisions, int(revision['revision'])) ``` This will guarantee revisions is sorted. > Tools/Scripts/bisect-webkit-builds:86 > + url = '/'.join([url, str(revisionToDownload)]) We should use a built-in URL joiner like urlparse. ```python import urlparse urlparse.urljoin(url1, url2) ``` > Tools/Scripts/bisect-webkit-builds:90 > + #revision = archive['revision'] We should avoid checking in commented out code. > Tools/Scripts/bisect-webkit-builds:92 > + return s3_url So we wan't to return the latest s3_url? If so do we need the for loop for this? > Tools/Scripts/bisect-webkit-builds:98 > + command = ['python', '../BuildSlaveSupport/download-built-product', '--%s' % options.configuration, '--platform', options.platform, s3_url] We should use `.format()` instead of `%s`. > Tools/Scripts/bisect-webkit-builds:109 > +def promptUser(): Suggest a more descriptive name like: `prompt_did_reproduce()`. > Tools/Scripts/bisect-webkit-builds:111 > + print('You entered: {}'.format(var)) This seems like a helpful print statement but in efforts to follow Unix conventions we probably should stay simple and not re-iterate what the user entered. > Tools/Scripts/bisect-webkit-builds:112 > + var.lower() Since `.lower()` does not change variable in-place you would have to do `var = var.lower()` for this to work as expected. > Tools/Scripts/bisect-webkit-builds:114 > + return 'yes' Can we use bool types. i.e. return True/False? > Tools/Scripts/bisect-webkit-builds:130 > + # FIXME: Add other platform logic here For this if/else block we can probably simplify the logic like so: ```python command = ['./run-safari'] if 'ios' in options.platform: command.extend('--simulator') ``` I also opt to removing the entire FIXME else block here and have the future Lucas (or whomever) deal with the new platforms when the time comes. > Tools/Scripts/bisect-webkit-builds:134 > + subprocess.call(command) The comment above for L#130 would also get rid of this `if command` check. > Tools/Scripts/bisect-webkit-builds:153 > + return int(math.ceil(middleIndex)) middleRevision, startRevision and endRevision are unused variables. > Tools/Scripts/bisect-webkit-builds:166 > + if reproduces == 'yes': # bisect left Tying to my comment below, if `testArchive()` method returns bool types, then this can be simplified to a simple if/else. > Tools/Scripts/bisect-webkit-builds:174 > +def getIndexesFromRevisions(revisionList, startRevision, endRevision): Typo: Indeces? > Tools/Scripts/bisect-webkit-builds:175 > + if startRevision == None: `if start_revision is None:`... > Tools/Scripts/bisect-webkit-builds:181 > + if endRevision == None: `if end_revision is none:`... > Tools/Scripts/bisect-webkit-builds:183 > + endIndex = len(revisionList)-1 Add spacing around the subtraction. > Tools/Scripts/bisect-webkit-builds:191 > +def getRestApiUrl(options): To make the function more decoupled from the rest of the script, the function should be self descriptive. Something like this: ```python def rest_api_url(full_archive=False): if full_archive: [...] else: [...] ``` > Tools/Scripts/bisect-webkit-builds:193 > + url = REST_API_URL + REST_API_ENDPOINT Ditto here, we should use `urlparse` module to join URLs. > Tools/Scripts/bisect-webkit-builds:195 > + url = REST_API_URL + REST_API_MINIFIED_ENDPOINT Ditto here, we should use `urlparse` module to join URLs. > Tools/Scripts/bisect-webkit-builds:203 > + return fullPlatforms nit: perform inline return > Tools/Scripts/bisect-webkit-builds:212 > +def isSupportedPlatform(options): Ditto for `getRestApiUrl` comment. > Tools/Scripts/bisect-webkit-builds:220 > + if options.platform == 'None': This check can happen using argparse by using `required=True`. See: https://docs.python.org/2.7/library/argparse.html#required > Tools/Scripts/bisect-webkit-builds:222 > + exit(-1) Curious what the rationale is between exiting with `-1` vs `1`? > Tools/Scripts/bisect-webkit-builds:223 > + if not isSupportedPlatform(options): The `isSupportedPlatform()` function makes more sense to me if we pass in a platform, not the script options. Having said that, argparse provides us with functionality that can enforce a choice from a specific list. See choices: https://docs.python.org/2.7/library/argparse.html#choices > Tools/Scripts/bisect-webkit-builds:246 > + outputDir = os.path.abspath('/'.join([basedir, '../../WebKitBisect'])) We should use `os.path.join(url1, url2)`. Also no need to declare an inline list, we can just keep passing in parameters. > Tools/Scripts/bisect-webkit-builds:248 > + os.environ["WEBKIT_OUTPUTDIR"] = outputDir We should replace `"` with `'` to keep consistent with the file. > Tools/Scripts/bisect-webkit-builds:253 > + abspath = os.path.abspath(__file__) Suggest a more descriptive name like: `script_path`. > Tools/Scripts/bisect-webkit-builds:254 > + dname = os.path.dirname(abspath) Suggest a more descriptive name like: `directory_name` or `dirname`. > Tools/Scripts/bisect-webkit-builds:259 > + exit(result) Nit: We can simplify this by just doing `exit(main(options))` == I also noticed main() isn't really returning anything. By default it _will_ return 0 so that's fine. But when do we return exit code 1 (or non-zero)?

Comment on attachment 316392 [details] patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=316392&action=review >> Tools/Scripts/bisect-webkit-builds:246 >> + outputDir = os.path.abspath('/'.join([basedir, '../../WebKitBisect'])) > > We should use `os.path.join(url1, url2)`. > Also no need to declare an inline list, we can just keep passing in parameters. I think that we should either use a temporary directory, or notify the user where builds are going. Creating a new permanent directory without saying anything seems undesirable. My preference would be to make it fully transparent with a temporary directory.

Created attachment 316463 [details] Patch to address feedback

@Alexey: this version will use a temporary directory, and it will also clean it up when finished.

Thanks!

Created attachment 316895 [details] patch for landing

Comment on attachment 316895 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=316895&action=review The patch is starting to take good shape. There are many instances where we are simply passing the options object around instead of having specific parameters laid out for each function. Those functions should be addressed for the purpose of maintainability and readability down the road. When using the program itself we should have `run-safari` command redirect output to /dev/null since it can have a nasty spew of text that make me lose track while bisecting. Also, do we want to make a starting point required? I doubt most people would want to test the entire history since the beginning of "archive time". Also in my testing I never got a y/n prompt and CTRL + C would kill the entire program. We should have a way to contain the errors of `run-safari` to make sure the bisecting process can continue. > Tools/Scripts/bisect-webkit-builds:96 > +def get_api_url(options): I don't think this function needs to know about the entire `options` object. In general functions should be as standalone as possible and as descriptive by themselves. I suggest we leverage function optionals to end up with something like this: ```python def get_api_url(use_full_url=False): if use_full_url: base_url = urlparse.urljoin(REST_API_URL, REST_API_ENDPOINT) else: base_url = urlparse.urljoin(REST_API_URL, REST_API_MINIFIED_ENDPOINT) [...] ``` > Tools/Scripts/bisect-webkit-builds:136 > + parser = argparse.ArgumentParser(description='Get a list of WebKit archives.') Are we really just getting a list of WebKit archives or are we bisecting available WebKit archives? > Tools/Scripts/bisect-webkit-builds:170 > + print('Setting environment variable WEBKIT_OUTPUTDIR to {}'.format(temp_dir)) Remind me again why we need the environment variable? This is another place where I think the print statement is superfluous. Especially if it's something the user won't actually utilize. > Tools/Scripts/bisect-webkit-builds:196 > + return ['gtk', 'ios-simulator-10', 'mac-elcapitan', 'mac-sierra', 'win', 'wpe'] Would these two functions above {un,}minified_platforms() best serve as constants declared at the top of the file until the FIXME is addressed? > Tools/Scripts/bisect-webkit-builds:200 > + if options.full: Just like L#96 comment: I don't think this function needs to know about the entire `options` object. The signature could look something like: `def is_supported_platform(use_minified_platforms=True)` > Tools/Scripts/bisect-webkit-builds:208 > + exit(1) This is something that the argparse module should take care of with the `required` argument. > Tools/Scripts/bisect-webkit-builds:210 > + print('Unsupported platform: [{}], exiting...'.format(options.platform)) Same thing for this, instead of having a function that performs this validation, we can just give argparse a list of valid options for --platform. See my first review for links to the documentation. > Tools/Scripts/bisect-webkit-builds:214 > + print('Available Minified platforms: {}'.format(minified_platforms())) Nit: From a usability perspective, If the default value for options.full is false, we should just say 'Available platforms: {}'. And in efforts to keep it simple we should not have to tell the user every time that they can pass in --full. I think it's the Help Texts job to describe it and give us the expected discoverability. > Tools/Scripts/bisect-webkit-builds:245 > + print('Cleaning up temporary folder: {}'.format(webkit_output_dir)) If we don't print the creation I think it's simpler to no print the deletion of the folder. This looks like more of a debug statement (which we could leverage if we use the `logging` module). > Tools/Scripts/bisect-webkit-builds:246 > + shutil.rmtree(webkit_output_dir) There's an `ignore_errors=True` option that we should pass in here to make sure the function does not error out if, for example, the folder was already somehow deleted.

Created attachment 317000 [details] updated after Kocsen's feedback

Comment on attachment 317000 [details] updated after Kocsen's feedback Attachment 317000 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4242348 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-no-preflight.any.worker.html imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.any.html imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.any.worker.html imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.any.worker.html imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.any.html imported/w3c/web-platform-tests/fetch/api/cors/cors-no-preflight.any.html

Created attachment 317026 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Committed revision 220534.

(In reply to Lucas Forschler from comment #25) > Committed revision 220534. It looks like the script is triplicated in that revision. Is that expected?

Wow... I've no way to explain that. Fixed: Committed revision 220537.

The bug is still in NEW state, and there is a patch up for review. Is there actually more work to do here?

(In reply to Alexey Proskuryakov from comment #28) > The bug is still in NEW state, and there is a patch up for review. Is there > actually more work to do here?

Resolving fixed. landed in: https://trac.webkit.org/changeset/220534/webkit and https://trac.webkit.org/changeset/220537/webkit