Bug 28084 – KURL ref() methods should be fragmentIdentifier() methods

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

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()

[Brady Eidson]

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.

[Darin Adler]

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

[Eric Seidel (no email)]

Comment on attachment 34327 [details]
Proposed patch

You're my personal hero. :)

Why do we even need to check "has" before "remove"?

[Brady Eidson]

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.

[Brady Eidson]

@Eric Seidel

DOH, just saw your comment.  Good point, there's no reason.

[Brady Eidson]

Actually, currently removeFragmentIdentifier() blindly reallocs and reparses, whilst hasFragmentIdentifier() is cheap.  There's no reason that removeFragmentIdentifier() shouldn't be a little smarter.

[Brady Eidson]

https://bugs.webkit.org/show_bug.cgi?id=28151

[Tor Arne Vestbø]

(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

[Dimitri Glazkov (Google)]

Chromium build borked (obviously). Fix coming up.

[Brady Eidson]

(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.

[Dimitri Glazkov (Google)]

Chromium build fix landed as http://trac.webkit.org/changeset/46993.

[Brady Eidson]

Sorry for the mild pain, guys.  Glad we did this one, though!

[Dimitri Glazkov (Google)]

no worries :).