Summary: | KURL ref() methods should be fragmentIdentifier() methods | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||
Component: | WebCore Misc. | Assignee: | Brady Eidson <beidson> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | dglazkov, vestbo | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Brady Eidson
2009-08-07 14:07:53 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 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 on attachment 34327 [details]
Proposed patch
You're my personal hero. :)
Why do we even need to check "has" before "remove"?
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. @Eric Seidel DOH, just saw your comment. Good point, there's no reason. Actually, currently removeFragmentIdentifier() blindly reallocs and reparses, whilst hasFragmentIdentifier() is cheap. There's no reason that removeFragmentIdentifier() shouldn't be a little smarter. (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 Chromium build borked (obviously). Fix coming up. (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. Chromium build fix landed as http://trac.webkit.org/changeset/46993. Sorry for the mild pain, guys. Glad we did this one, though! no worries :). |