Bug 72661 - Switch to a more modern approach to retrieving the startup volume name
Summary: Switch to a more modern approach to retrieving the startup volume name
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Rowe (bdash)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-17 14:37 PST by Mark Rowe (bdash)
Modified: 2011-11-21 13:44 PST (History)
5 users (show)

See Also:


Attachments
Patch v1 (3.58 KB, patch)
2011-11-17 14:40 PST, Mark Rowe (bdash)
mitz: review+
Details | Formatted Diff | Diff
Patch v2 (9.28 KB, patch)
2011-11-20 16:17 PST, Mark Rowe (bdash)
mitz: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2011-11-17 14:37:38 PST
As it says.
Comment 1 Mark Rowe (bdash) 2011-11-17 14:40:13 PST
Created attachment 115686 [details]
Patch v1

Patch!
Comment 2 Alexey Proskuryakov 2011-11-18 11:25:37 PST
This method is used to manipulate a path, and it seems very fragile to expect that display name matches path component. If nothing else, path components use different Unicode normalization than regular strings in OS X.
Comment 3 Darin Adler 2011-11-18 11:57:00 PST
Comment on attachment 115686 [details]
Patch v1

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

> Source/WebKit/mac/Misc/WebNSFileManagerExtras.mm:95
>  - (NSString *)_webkit_startupVolumeName
>  {
> -    NSString *path = [self _webkit_carbonPathForPath:@"/"];
> -    return [path substringToIndex:[path length]-1];
> +    return [[[NSFileManager defaultManager] componentsToDisplayForPath:@"/"] objectAtIndex:0];
>  }

While it may work OK, this doesn’t seem quite right. This method is not used to display the volume name. It’s used to compare the name with the first component in a string extracted using -[NSString pathComponents], which I do not believe does any “display” processing. So if “components to display” has any meaning other than “components” for volumes then the code will not work well.
Comment 4 Darin Adler 2011-11-18 11:57:23 PST
(In reply to comment #2)
> This method is used to manipulate a path, and it seems very fragile to expect that display name matches path component. If nothing else, path components use different Unicode normalization than regular strings in OS X.

Ah, it seems that Alexey and I are saying the same thing.
Comment 5 Mark Rowe (bdash) 2011-11-20 16:17:31 PST
Created attachment 116006 [details]
Patch v2

This patch uses DiskArbitration to find the volume name of the startup volume.
Comment 6 Mark Rowe (bdash) 2011-11-20 16:18:47 PST
This is the same way of retrieving the name that DiskArbitration uses internally when setting up the /Volumes/Blargh mount point directories and symlinks.
Comment 7 WebKit Review Bot 2011-11-20 16:44:55 PST
Comment on attachment 116006 [details]
Patch v2

Attachment 116006 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10533091

New failing tests:
platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html
Comment 8 Mark Rowe (bdash) 2011-11-21 13:44:44 PST
Landed in r100954.