Bug 34859 – [chromium] Update Chromium port's usage of url_util::IsStandard

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Brett Wilson (Google)]

I'm working on a google-url patch: http://codereview.chromium.org/564011 which requires a small update to the Chromium port's usage of IsStandard in KURLGoogle.cpp.

This bug is to track that update.

[Brett Wilson (Google)]

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.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 48673 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/262201

[Brett Wilson (Google)]

Created attachment 48674 [details]
Path with style fixed.

See above, this will make the Chromium build fail.

[WebKit Review Bot]

Attachment 48674 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/261234

[Adam Barth]

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

[Bernhard Bauer]

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

[Brett Wilson (Google)]

Checked in as r54997.

[Tony Chang]

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

[Kent Tamura]

r54998 fixed the build error by r54997.

[Jeremy Orlow]

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.

[Pavel Feldman]

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

[Brett Wilson (Google)]

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.

[Brett Wilson (Google)]

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.

[Brett Wilson (Google)]

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.

[Brett Wilson (Google)]

Committed in r55319