Issue 8856006: WebKit changes needed for supporting filesystem URLs natively in GURL/KURL.

Issue 8856006: WebKit changes needed for supporting filesystem URLs natively in GURL/KURL. (Closed)

Created:
9 years ago by ericu

Modified:
8 years, 10 months ago

Reviewers:
abarth-chromium

Base URL:
http://svn.webkit.org/repository/webkit/trunk

Visibility:
Public.

Description

WebKit changes needed for supporting filesystem URLs natively in GURL/KURL. See also the chromium [http://codereview.chromium.org/7811006/] and googleurl [http://codereview.appspot.com/4961060/] changes. BUG=none TEST=existing tests

Patch Set 1 #

Patch Set 2 : Fix escaping of %2F. #

Patch Set 3 : Merged out latest webkit. #

Total comments: 12

Patch Set 4 : Mainly style fixes #

Patch Set 5 : comment fix #

Patch Set 6 : Remove dead constructor code. #

Created: 9 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+76 lines, -2 lines)			Patch
M	Source/WebCore/fileapi/EntryBase.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/WebCore/page/SecurityOrigin.cpp	View	1 2 3 4 5	1 chunk	+4 lines, -0 lines	0 comments	Download
M	Source/WebCore/platform/KURL.h	View	1 2 3	1 chunk	+9 lines, -0 lines	0 comments	Download
M	Source/WebCore/platform/KURLGoogle.cpp	View	1 2 3	6 chunks	+54 lines, -1 line	0 comments	Download
M	Source/WebCore/platform/KURLGooglePrivate.h	View	1 2 3	4 chunks	+8 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

abarth-chromium

http://codereview.chromium.org/8856006/diff/4001/Source/WebCore/page/SecurityOrigin.cpp File Source/WebCore/page/SecurityOrigin.cpp (right): http://codereview.chromium.org/8856006/diff/4001/Source/WebCore/page/SecurityOrigin.cpp#newcode91 Source/WebCore/page/SecurityOrigin.cpp:91: return KURL(ParsedURLString, decodeURLEscapeSequences(url.path())); This shouldn't be conditional on ENABLE(FILE_SYSTEM). ...

9 years ago (2011-12-20 07:03:15 UTC) #1

ericu

9 years ago (2011-12-20 23:55:33 UTC) #2

http://codereview.chromium.org/8856006/diff/4001/Source/WebCore/page/Security...
File Source/WebCore/page/SecurityOrigin.cpp (right):

http://codereview.chromium.org/8856006/diff/4001/Source/WebCore/page/Security...
Source/WebCore/page/SecurityOrigin.cpp:91: return KURL(ParsedURLString,
decodeURLEscapeSequences(url.path()));
On 2011/12/20 07:03:15, abarth wrote:
> This shouldn't be conditional on ENABLE(FILE_SYSTEM).  You should just
replaces
> this the goofy call to KURL() with url.innerURL().

Currently innerURL only exists if ENABLE(FILE_SYSTEM), and is 0 for blob URLs. 
It looks like blob URLs will hit here and use the KURL constructor somehow, but
I'm not sure quite how.

http://codereview.chromium.org/8856006/diff/4001/Source/WebCore/page/Security...
Source/WebCore/page/SecurityOrigin.cpp:157: 
On 2011/12/20 07:03:15, abarth wrote:
> None of this code should be here anymore.  It all got refactored into more
> sensible places.

I took out the non-FILE_SYSTEM code, but left my stuff.  Is there a more
sensible place for it too?

Also, m_filePath below is in a different namespace for filesystem:file: URLs vs.
file: URLs.  Will that ever get invoked in Chromium, or do we always just have
blank paths for file SecurityOrigins?

http://codereview.chromium.org/8856006/diff/4001/Source/WebCore/platform/KURL.h
File Source/WebCore/platform/KURL.h (right):

http://codereview.chromium.org/8856006/diff/4001/Source/WebCore/platform/KURL...
Source/WebCore/platform/KURL.h:228: const KURL* inner_url() const { return
m_url.inner_url(); }
On 2011/12/20 07:03:15, abarth wrote:
> This function should be in WebKit style and named innerURL().

Done.

http://codereview.chromium.org/8856006/diff/4001/Source/WebCore/platform/KURL...
File Source/WebCore/platform/KURLGoogle.cpp (right):

http://codereview.chromium.org/8856006/diff/4001/Source/WebCore/platform/KURL...
Source/WebCore/platform/KURLGoogle.cpp:187: m_inner_url = adoptPtr(new
KURL(o.m_inner_url->copy()));
On 2011/12/20 07:03:15, abarth wrote:
> m_innerURL.  Can we avoid calling the KURL parser here?  I would have expecetd
> the m_innerURL to be kept as a KURL and we can make a copy of it without
needing
> to invoke the parser.

I'm not sure what you mean.  m_inner_url is a KURL, and I don't see an
invocation of the parser here, just KURL's compiler-generated copy constructor.

http://codereview.chromium.org/8856006/diff/4001/Source/WebCore/platform/KURL...
Source/WebCore/platform/KURLGoogle.cpp:297: url_parse::Parsed* inner_parsed =
m_parsed.inner_parsed();
On 2011/12/20 07:03:15, abarth wrote:
> innerParsed

Done.

http://codereview.chromium.org/8856006/diff/4001/Source/WebCore/platform/KURL...
Source/WebCore/platform/KURLGoogle.cpp:303: inner_parsed->Length() -
inner_parsed->scheme.begin)));
On 2011/12/20 07:03:15, abarth wrote:
> There's no 80 column limit in WebKit.  You can go nuts and have excitingly
long
> lines!

s/excitingly/excruciatingly/, but OK ;'>.

abarth-chromium

9 years ago (2011-12-21 00:00:52 UTC) #3

Let's take this discussion to a code review on bugs.webkit.org.

Expand Messages | Collapse Messages