| Summary: | [chromium] Update Chromium port's usage of url_util::IsStandard | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brett Wilson (Google) <brettw> | ||||||||
| Component: | Platform | Assignee: | Brett Wilson (Google) <brettw> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | bauerb, darin, dglazkov, fishd, jorlow, pfeldman, tkent, tony, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Brett Wilson (Google)
2010-02-11 13:43:55 PST
Created attachment 48673 [details] Patch Do not commit queue this since it needs a coordinated landing with http://codereview.chromium.org/579004 on the Chromium side. Attachment 48673 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/KURLGoogle.cpp:586: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
Total errors found: 1
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 48673 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/262201 Created attachment 48674 [details]
Path with style fixed.
See above, this will make the Chromium build fail.
Attachment 48674 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/261234 Comment on attachment 48674 [details]
Path with style fixed.
Looks reasonable. I didn't understand the relation between
- m_url.utf8String().length(),
and the rest of the patch. Also, please fix the indentation in V8LocationCustom to be four spaces. (Not sure why the style elf missed that.)
(In reply to comment #6) > (From update of attachment 48674 [details]) > Looks reasonable. I didn't understand the relation between > > - m_url.utf8String().length(), > > and the rest of the patch. This change is to match the signature change of url_util::IsStandard introduced in googleurl, revision 123. (In reply to comment #8) > Checked in as r54997. Do you plan on landing the chromium side of this soon? It causes the commit queue and webkit-patch to no longer work. This broke the canaries and anyone on a hybrid build crome/webkit checkout. I would just revert it, but it seems that you knew this would happen and checked it in anyway, which makes me wonder if reverting it would cause other badness. Please don't do this in the future, though. (In reply to comment #11) > This broke the canaries and anyone on a hybrid build crome/webkit checkout. I > would just revert it, but it seems that you knew this would happen and checked > it in anyway, which makes me wonder if reverting it would cause other badness. > > Please don't do this in the future, though. After looking at canaries I just reverted the change. It is a two-sided commit that should be coordinated with sheriff. Once I've reverted it, I came here to post a note and found out the discussion. I still think I did the right thing. I did coordinate with the sheriff. What am I supposed to do differently? What do you mean by "don't do this again!" This is the only way to land two sided patches! The merge landed when I was asleep and I came in to do the part that I coordinated with the sheriff I would land, and it's reverted! And now I have to do THE ENTIRE THING AGAIN. This makes me VERY frustrated to the point where I wish I never worked on this bug. Dimitri clarified that I need to wait for a green revision right before the checkin. I also suspect landing this is impossible when the sheriff is in another time zone. Created attachment 49592 [details]
Third try
This is the same as the previous patch without the change to the call to url_util::IsStandard. I added a temporary backwards-compat version of IsStandard to googleurl to aid in landing this. I'll land the fix to the call as a separate pass.
Committed in r55319 |