Bug 34492

(In reply to comment #0) > Here is a simple patch that makes no semantic change and circumvents the use of > the invalidated object. Where?

Please be clear, holding arbitrary strings that are not valid URLs is an intentional feature of KURL. To remove it we need to remove all clients that depend on that. It sure would be great to have the freedom to not need that feature, but we can’t get that by just fixing the cases we have already stumbled on, we need to look at all callers. And then change the implementation so the mistake is easy to detect. I know that Chromium has been living with all these bugs all along since KURLGoogle omitted that feature, but when fixing this please keep in mind that all other platforms have not been subject to the bugs. KURLGoogle still seems like a bad idea to me. I know it’s pleasant for Chromium to share the same URL parsing with the other components in Chromium -- it would be nice to have that on the Mac too -- and someone may have the idea that it helps with security, but I believe that for the WebKit project as a whole would be great to make changes as needed to both the original KURL and KURLGoogle so their behavior is identical and then get rid of this unnecessary difference between platforms by deleting KURLGoogle.

Created attachment 47952 [details] patch Does it not work to add a patch from the bug creation page? Anyway, here is the patch.

Sorry! Wrong bug to attach the patch to. Please ignore.

(In reply to comment #4) > Sorry! Wrong bug to attach the patch to. Please ignore. Nope: I just didn't realize there were more comments, so this looked like a different bug. Clearly I'm not really used to bugzilla. Sorry for the confusion and extraneous comments.

(In reply to comment #3) > Does it not work to add a patch from the bug creation page? I never tried it before. Maybe it’s broken.

Comment on attachment 47952 [details] patch Change looks great, although I’d probably use assignment syntax rather than initialization to set up the baseAttribute local variable. Generally it's not correct to set the review flag on a patch without a change log entry. We require regression tests for bugs fixes. Since this patch fixes a bug, you must also include a regression test that fails on Chromium before this change and now succeeds with the fix. But neither the test itself nor the test results should be Chromium-specific. review- because of the lack of change log and regression test

Created attachment 47955 [details] revised patch Sorry for the incomplete patch last time. This includes the ChangeLog entry. There was already a test failing on Chromium without this change (LayoutTests/svg/W3C-SVG-1.1/struct-image-07-t.svg), so I do not think an additional regression test is necessary.

(In reply to comment #8) > Sorry for the incomplete patch last time. This includes the ChangeLog entry. > There was already a test failing on Chromium without this change > (LayoutTests/svg/W3C-SVG-1.1/struct-image-07-t.svg), so I do not think an > additional regression test is necessary. Typically a patch would include the corrected results or a change to the skipped list for a patch -- so I could see what test was changing behavior. But since Chromium has a different way of doing things, maybe that's why there's no such change here. Despite the fact that a test was failing, a simpler test that targets this behavior more directly would be far better. It could even be a dumpAsText test. Tests where success can only be judged from the pixel results are the lowest on our totem pole of test usefulness, so this existing test is only barely sufficient.

Comment on attachment 47955 [details] revised patch > +2010-02-02 Bryan Yeung <bryeung@bryeung-chrome.(none)> Strange address here. r=me despite my concerns mentioned in comments above.

Comment on attachment 47955 [details] revised patch Clearing flags on attachment: 47955 Committed r54245: <http://trac.webkit.org/changeset/54245>

All reviewed patches have been landed. Closing bug.

Committed r54264: <http://trac.webkit.org/changeset/54264>

Rolled out due to major chromium bustage with this patch. Rolllout: http://trac.webkit.org/changeset/54264

Actually, it looks like I may have been wrong. Once the canary bot gets well. I'd be happy to re-commit this (since it was rolled out incorrectly) -- sometimes it is hard to tell which patch caused the issue :(

Committed again unchanged from before http://trac.webkit.org/changeset/54282 Sorry about the mix-up here.