Bug 28084 - KURL ref() methods should be fragmentIdentifier() methods
Summary: KURL ref() methods should be fragmentIdentifier() methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-07 14:07 PDT by Brady Eidson
Modified: 2009-08-10 12:15 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch (24.65 KB, patch)
2009-08-07 14:14 PDT, Brady Eidson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2009-08-07 14:07:53 PDT
KURL ref() methods should be fragmentIdentifier() methods.

HTML 5 has standardized on the terminology "fragment identifier" for the fragment/ref string after the #.

Having ref() be a method causes headaches, being overloaded with the "ref()" of ref pointer fame.

I have a cleanup that replaces:
ref(), hasRef(), setRef(const String&), removeRef(), equalIgnoringRef()
with
fragmentIdentifier(), hasFragmentIdentifier(), setFragmentIdentifier(const String&), removeFragmentIdentifier(), equalIgnoringFragmentIdentifier()
Comment 1 Brady Eidson 2009-08-07 14:14:43 PDT
Created attachment 34327 [details]
Proposed patch

Pretty mechanical change.

But there are some very subtle "null versus empty" changes, also, so even if this get's an r+, note that I'm still running all the layout tests on all the platforms I have access to, just to make sure those subtle changes don't break anything, and I'll finish that and land after the weekend.
Comment 2 Darin Adler 2009-08-07 14:45:27 PDT
Comment on attachment 34327 [details]
Proposed patch

> -    const KURL& url = this->url();
> -    return url.ref().isEmpty() ? "" : "#" + url.ref();
> +    const String& fragmentIdentifier = this->url().fragmentIdentifier();
> +    return fragmentIdentifier.isEmpty() ? "" : "#" + fragmentIdentifier;

No need for the this-> any more here.

r=me
Comment 3 Eric Seidel (no email) 2009-08-07 14:48:26 PDT
Comment on attachment 34327 [details]
Proposed patch

You're my personal hero. :)

Why do we even need to check "has" before "remove"?
Comment 4 Brady Eidson 2009-08-10 09:35:29 PDT
Sending        WebCore/ChangeLog
Sending        WebCore/accessibility/AccessibilityRenderObject.cpp
Sending        WebCore/bindings/js/JSLocationCustom.cpp
Sending        WebCore/css/CSSCursorImageValue.cpp
Sending        WebCore/history/HistoryItem.cpp
Sending        WebCore/html/HTMLAnchorElement.cpp
Sending        WebCore/html/HTMLFrameElementBase.cpp
Sending        WebCore/loader/FrameLoader.cpp
Sending        WebCore/loader/appcache/ApplicationCache.cpp
Sending        WebCore/loader/appcache/ApplicationCacheGroup.cpp
Sending        WebCore/loader/appcache/ApplicationCacheResource.h
Sending        WebCore/loader/appcache/ApplicationCacheStorage.cpp
Sending        WebCore/loader/appcache/ManifestParser.cpp
Sending        WebCore/page/Location.cpp
Sending        WebCore/page/Page.cpp
Sending        WebCore/platform/KURL.cpp
Sending        WebCore/platform/KURL.h
Sending        WebCore/rendering/RenderPartObject.cpp
Sending        WebCore/workers/WorkerLocation.cpp
Transmitting file data ...................
Committed revision 46978.

Two platforms I can build and test on are clear, I'll keep a close eye on the bots.
Comment 5 Brady Eidson 2009-08-10 09:36:01 PDT
@Eric Seidel

DOH, just saw your comment.  Good point, there's no reason.
Comment 6 Brady Eidson 2009-08-10 09:38:14 PDT
Actually, currently removeFragmentIdentifier() blindly reallocs and reparses, whilst hasFragmentIdentifier() is cheap.  There's no reason that removeFragmentIdentifier() shouldn't be a little smarter.
Comment 7 Brady Eidson 2009-08-10 09:41:24 PDT
https://bugs.webkit.org/show_bug.cgi?id=28151
Comment 8 Tor Arne Vestbø 2009-08-10 09:43:44 PDT
(In reply to comment #4)
> Two platforms I can build and test on are clear, I'll keep a close eye on the
> bots.

http://build.webkit.org/builders/GTK%20Linux%20Release/builds/2853/steps/compile-webkit/logs/stdio
Comment 9 Dimitri Glazkov (Google) 2009-08-10 09:48:36 PDT
Chromium build borked (obviously). Fix coming up.
Comment 10 Brady Eidson 2009-08-10 10:08:43 PDT
(In reply to comment #8)
> (In reply to comment #4)
> > Two platforms I can build and test on are clear, I'll keep a close eye on the
> > bots.
> 
> http://build.webkit.org/builders/GTK%20Linux%20Release/builds/2853/steps/compile-webkit/logs/stdio

Sending        gtk/ChangeLog
Sending        gtk/webkit/webkitdownload.cpp
Transmitting file data ..
Committed revision 46980.

Hope that's it, I'll keep watching.
Comment 11 Dimitri Glazkov (Google) 2009-08-10 11:46:06 PDT
Chromium build fix landed as http://trac.webkit.org/changeset/46993.
Comment 12 Brady Eidson 2009-08-10 11:49:48 PDT
Sorry for the mild pain, guys.  Glad we did this one, though!
Comment 13 Dimitri Glazkov (Google) 2009-08-10 12:15:42 PDT
no worries :).