Refactor SecurityOrigin::create to be easier to understand
Created attachment 115076 [details] Patch
Comment on attachment 115076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115076&action=review I'd like to see one more round. > Source/WebCore/page/SecurityOrigin.cpp:84 > + return KURL(ParsedURLString, decodeURLEscapeSequences(url.path())); Why does this work? Can you add a FIXME about what this should be? > Source/WebCore/page/SecurityOrigin.cpp:104 > + // For edge case URLs that were probably misparsed, make sure that the origin is unique. > + if (schemeRequiresAuthority(effectiveURL) && effectiveURL.host().isEmpty()) > + return true; Why do we do this? Should this be a FIXME to remove this? This smells bad... > Source/WebCore/page/SecurityOrigin.cpp:109 > + String protocol = effectiveURL.protocol().lower(); > + > + if (SchemeRegistry::shouldTreatURLSchemeAsNoAccess(protocol)) > + return true; Why do we lower the protocol here? Why isn't SchemeRegistry case agnostic? > Source/WebCore/page/SecurityOrigin.cpp:117 > + // FIXME: Should we be using shouldUseInnerURL here? > + // We originally did that check to avoid treating blob and filesystem > + // urls as unique when it is created from local file urls. This FIXME is unclear. Can you explain better what you're trying to say? > Source/WebCore/page/SecurityOrigin.cpp:151 > + : m_protocol("") > + , m_host("") > + , m_domain("") Why should this be empty strings instead of null strings? And what about using String::empty instead of ""?
Comment on attachment 115076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115076&action=review >> Source/WebCore/page/SecurityOrigin.cpp:109 >> + if (SchemeRegistry::shouldTreatURLSchemeAsNoAccess(protocol)) >> + return true; > > Why do we lower the protocol here? Why isn't SchemeRegistry case agnostic? It's not case agnostic because it uses a hash map lookup. It could lower the scheme itself, but that would be slow because it often requires a malloc. >> Source/WebCore/page/SecurityOrigin.cpp:151 >> + : m_protocol("") >> + , m_host("") >> + , m_domain("") > > Why should this be empty strings instead of null strings? And what about using String::empty instead of ""? We can't use String::empty because this class is used on multiple threads. I'm not sure whether we can get away with null strings. Currently, this constructor matches the other constructor, which is very explicit about converting null strings to empty strings. I'm inclined to experiment with changing these to null in a future patch.
Created attachment 115690 [details] Patch
Comment on attachment 115690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115690&action=review OK. > Source/WebCore/page/SecurityOrigin.cpp:196 > + if (url.protocolIs("file")) { > + // Unfortunately, we can't represent all unique origins exactly > + // the same way because we need to produce a quirky database > + // identifier for file URLs due to persistent storage in some > + // embedders of WebKit. > + origin->m_needsDatabaseIdentifierQuirkForFiles = true; > + } Can't we set this only for origins that matter to the database world?
Committed r100691: <http://trac.webkit.org/changeset/100691>