Bug 29972 – Implement URL decomposition IDL attributes for HTMLAnchorElement

• Component: DOM
• Status: RESOLVED FIXED

[Yael]

According to  http://dev.w3.org/html5/spec/text-level-semantics.html#the-a-element , JavaScript should be able to manipulate parts of the URL of href attribute.

[Yael]

Created attachment 40460 [details]
Patch.

[Darin Adler]

Comment on attachment 40460 [details]
Patch.

Looks good!

The regression test should be dumpAsText(). And ideally it should be one of the script-tests. This seems like a perfect candidate for that style of test.

This is a test of HTMLAnchorElement, so it should be in fast/dom/HTMLAnchorElement, not fast/dom/Element. So you should create that new directory.

We need a lot more test cases, with all the edge cases like passing in malformed strings, like including a ":" or other punctuation in the protocol string and many other examples like that. And we need to see how the abnormal cases behave in other browsers to see if our behavior is right. There's no reason to assume the KURL functions just happen to do everything right already.

For changes like this one of the most important issues is how similar or different our behavior is from other browsers.

review- because the test needs changes, at the very least to dumpAsText(), but I'd prefer if you did everything I mention above.

[Yael]

Thank you for the quick review. 
I will add more tests. And sorry for forgetting to add dumpAsText().

As for browser comparison, this is tricky.
I think each browser has its own bugs in this area :-)

[Yael]

Created attachment 40586 [details]
Patch

This patch adds support for manipulating parts of the URL in href. 
Processing as described in http://dev.w3.org/html5/spec/infrastructure.html#url-decomposition-idl-attributes.

The test resuls are almost always the same as FireFox, with some exceptions:
1. FireFox does not allow setting a NULL port, but this should be allowed.
2. FireFox allows setting a null protocol, but this should not be allowed.
3. Encoding of parts of the URL is not always done correctly. I'd like to fix that as a separate bug, otherwise, this patch will grow very big.

Sorry for omitting the dumpAsText() in the previous test case, it's been corrected now.

[Darin Adler]

Comment on attachment 40586 [details]
Patch

Change looks good. Still some things to fix.

> -    // Setting the port to 0 will clear any port from the URL.
> +    void clearPort();

Given that the call to check if a port is present is named hasPort(), I would call this removePort() rather than clearPort(). This is also consistent with the name of the removeFragmentIdentifier function.

>  #ifndef NDEBUG
>      void print() const;
>  #endif
> +    bool isHierarchical() const;

This seems like a strange place to tuck this function -- easy to miss it here in the same paragraph with a debug-only function. I'm also not sure we want a public function for this, and if we do we should document it I think.

> +    for (int i = 1; i < protocol.length(); i++) {
> +        if (!isSchemeChar(protocol[i]))
> +            return false;
> +    }

Since String::length returns an unsigned, I suspect this won't compile on some platforms due to unsigned/signed comparison warnings. Please make "i" an unsigned rather than an int.

But also, I would normally put the length into a local variable in a case like this, since it's used once outside the loop and then again each time through the loop. Not sure everyone would agree with me on that.

> +void KURL::clearPort()
> +{
> +    parse(m_string.left(m_hostEnd) + m_string.substring(m_portEnd));
> +}

If m_isValid is false, I'm not sure this will do the right thing. Does one of your test cases exercise that code path?

If m_hostEnd == m_portEnd, then I think this does quite a bit of extra work, and I think you should optimize that case with an early return.

>      // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations.
> -    parse(m_string.left(m_queryEnd) + "#" + s);
> +    if (s.isEmpty())
> +        parse(m_string.left(m_queryEnd));
> +    else
> +        parse(m_string.left(m_queryEnd) + "#" + s);

I think it's strange that this refuses to put "#" on for an empty fragment identifier. I suggest coding this quirk at the call site inside setHash rather than building it into the KURL function.

> +    // Setter condition:
> +    // The new value is not the empty string and input is hierarchical and uses
> +    // a server-based naming authority 
> +    if (value.isEmpty())
> +        return;
> +    KURL url = href();
> +    if (!url.isHierarchical() || url.isLocalFile())
> +        return;

I don't think !isHierarchical || isLocalFile is the right way to check if the URL uses a server-based naming authority. You should ask some people about that on webkit-dev -- we need to find some experts. I think we might need to ask SecurityOrigin this question.

And if this is the reason you exposed isHierarchical, then I think that's a mistake. Instead you should expose a function that does the entire (!isHierarchical || isLocalFile) test with an appropriate name. And keep isHierarchical private.

> +        // FireFox rejects the port if it is not numerical value.

The name Firefox doesn't have a second capital "F" in it. Normally in comments like this it's useful to be more specific, mentioning the version number you tested. Also, is Firefox what we want to cite here? Does the HTML 5 specification say anything about this? What about Internet Explorer's behavior?

> +        // Accept the port portion only if it is numerical,
> +        // and not default for the scheme.
> +        int i = separator + 1;
> +        for (; i < value.length() && isASCIIDigit(value[i]); i++);

I believe this loop structure will give a warning on some compilers. Instead I suggest making a helper function that returns true only if all characters are ASCII digits.

Did you test cases where there is leading or trailing whitespace? Does Firefox actually ignore the numbers in strings that have that whitespace?

The toUInt and toUIntStrict functions already have an out parameter "ok" that tells whether the passed in thing is a number. There's a chance you could use that instead of writing you own loop to check for ASCII digits.

> +    // Setter preprocessor:
> +    //Remove all leading U+002F SOLIDUS ("/") characters 

I don't understand the term "Setter preprocessor" term here and "Setter condition" -- are those terms from the HTML 5 standard? If so, then perhaps there's a way to make that clearer. If not, maybe you could just leave those comment lines out. They don't seem to add much.

Missing space after "//" on the second line. And it should have a period too.

> +    int i = 0;
> +    for (; i < value.length() && value[i] == '/'; i++);

Again, the semicolon on the same line as the loop will not compile with some compilers. And searching for a single character should be done by calling String::find rather than with a custom loop.

> +    //If it has no leading U+002F SOLIDUS ("/") character, prepend a U+002F SOLIDUS ("/") character to the new value 

Missing space after "//" on the second line. And it should have a period too.

> +    if (value.startsWith("/"))

A significantly more efficient way to do this check is value[0] == '/'. String's subscript operator returns 0 for characters past the end of the string so you don't have to worry about the empty string. Doing it the way you wrote it actually creates an unneeded temporary String because we haven't overloaded the function for "const char*" (which we could do to fix that problem) and also calls strlen. All unneeded.

> +    // We accept the port if it is numerical and not default for the scheme,
> +    // else remove the port.

The "remove the port" comment here makes it clear that removePort is a better name.

> +    int separator = value.find(":");

The find function will run more efficiently if you pass it ':' instead of ":".

> +    KURL url = href();
> +    if (separator > 0)  {
> +        String subvalue = value.substring(0, separator);
> +        if (!KURL::isProtocolValid(subvalue))
> +            return;
> +        url.setProtocol(subvalue);
> +    }
> +    else {
> +        if (!KURL::isProtocolValid(value))
> +            return;
> +        url.setProtocol(value);
> +    }

You could save duplicated code if you set subvalue to value when separator is -1. And in fact, that's exactly what would happen if you passed the -1 to the substring function, so you could just remove the if entirely and keep the code inside the if clause.

Also, the brace before else is supposed to be on the same line with the else.

> +    if (value.startsWith("?"))

Again, value[0] == '?' is more efficient for such cases.

Try to structure the code so that it doesn't call toUInt two times on the same string. I noticed that in a couple of cases.

The tests look good. They would be even better if they were written using the script-tests machinery, because then they would make it clear what the expected results are and you could tell if they were passing or failing just by viewing them in a web browser. An example of one of these is fast/dom/HTMLFormElement/script-tests/elements-not-in-document.js. The HTML wrapper for such tests is made by running the make-script-tests-wrappers script. To start a new directory like HTMLAnchorElement you just need to create the script-tests directory and put a TEMPLATE.html file in it. Best to svn copy the TEMPLATE.html file from another script-tests directory at the same level of the directory hierarchy, such as fast/dom/HTMLFormElement/script-tests/TEMPLATE.html in your case.

Similarly, it's helpful to write out what's being tested, not just a list of the test output. You would use the poorly-named "debug()" function in your script-tests test to write out a line of text telling what the script has done so that then you can get a sense of why the results are expected. A pretty good example of that is fast/dom/Node/script-tests/initial-values.js.

It would be good to include test cases that document what happens when the original URL is not well formed. Your test cases are good, but they are staying too close to "normal good" cases and not exercising the edge cases enough. There should be a test case that takes every single branch in each of the functions used. For example, you have to at least try setting each of the properties to the empty string and other types of illegal values.

To make the tests more economical, I suggest you write out the e entire URL after the tests rather than writing out the separate protocol, host, pathname, href, and search each time. It's OK to validate that the engine properly parses the URL after changes like this, but writing them out on separate lines is making the test seem much bigger than it is and indirectly causing us to test fewer cases. The goal here is to test the setter behavior, not to create additional tests of the parsing for the getters, so I think that's a good tradeoff.

[Yael]

While adding test cases for malformed URLs and bad input, I noticed that IE8 and Firefox 3.5.2 behave very differently.
Darin, others, could you please suggest which one is better to follow on the error cases?

[Darin Adler]

(In reply to comment #6)
> While adding test cases for malformed URLs and bad input, I noticed that IE8
> and Firefox 3.5.2 behave very differently.
> Darin, others, could you please suggest which one is better to follow on the
> error cases?

Without any further details on how they differ, it's hard for me to make a recommendation.

Generally we like to match IE if it has behavior that's reasonable, since it has the largest market share and so sites probably conform to its idiosyncrasies. But there are exceptions and this could be one of those depending on what the differences are. This may have already been explored by folks working on HTML5, so please take a look to see if that specification says anything.

[Yael]

Created attachment 41658 [details]
Patch

Style comments fixed.
More tests added.
Each test either passes on IE8 and Firefox 3.5.2, or it has some explanation of why it does not pass.
Server-based naming authority issue - I sent an e-mail to webkit-dev and asked the question on #whatwg IRC channel. Nobody seems to know the answer, so I tested IE8 and followed its behavior.

KURL character table should be updated to reflect newer specifications, e.g. RFC 3986. This was not done in this patch.

[Yael]

Created attachment 41668 [details]
Patch.

Previous patch missed one place where signed/unsigned were compared. This patch fixes that.

[Darin Adler]

Comment on attachment 41668 [details]
Patch.

Looks good!

I'm not sure you have the expected results right in the cases where what you pass in for the URL is a Windows-style file pathname.

>      bool protocolIs(const char*) const;
>      bool protocolInHTTPFamily() const;
>      bool isLocalFile() const;
> +    bool hasPort() const;

I believe I mentioned in an earlier that I think hasPort() does not belong in the paragraph with all the protocol boolean predicates. Instead I think it should go just after port(), just as hasFragmentIdentifier() goes just after fragmentIdentifier().

> +bool KURL::isProtocolValid(const String& protocol)
> +{
> +    if (protocol.isEmpty())
> +        return false;
> +    if (!isSchemeFirstChar(protocol[0]))
> +        return false;
> +    unsigned protocolLength = protocol.length();
> +    for (unsigned i = 1; i < protocolLength; i++) {
> +        if (!isSchemeChar(protocol[i]))
> +            return false;
> +    }
> +    return true;
> +}

I think this should be a non-member function like the protocolIs and protocolIsJavaScript functions rather than a KURL member function.

There is no need for the isEmpty() check, since the isSchemeFirstChar check will take care of that automatically, since indexing off the end of a String gives you a 0 character.

> +    // Don't set the host on a local file e.g. c:/path
> +    if (m_schemeEnd == 1 && m_userStart == m_schemeEnd + 1)
> +        return;

This seems to be Windows-specific. We don't have any other code special casing Windows-style paths unconditionally on all platforms, and I'm quite surprised to see this here! Maybe we can add this in another patch. Is this really the only case in the entire KURL class where we need to detect that something is a Windows-style path? And is this really the right condition for detecting such a path? While I accept that we may need special handling for such paths, if so it should be at parse time, not in the set functions, where we decide that something is a path, not a URL with an unusual one-letter URL scheme.

I think we'll need to land a first version of this patch without these special cases and then decide how to handle the Windows-style path case separately. You can land all the test cases showing what we're doing wrong with FAIL results and then we can fix them in a second patch that addresses only that issue.

> +void KURL::removePort()
> +{
> +    if (!m_isValid)
> +        return;
> +    if (m_hostEnd == m_portEnd)
> +        return;
> +    parse(m_string.left(m_hostEnd) + m_string.substring(m_portEnd));
> +}

I don't think we need the m_isValid check here. If a KURL is not valid, then m_hostEnd and m_portEnd are both guaranteed to be zero.

> +    // Don't set the port on a local file e.g. c:/path
> +    if (m_schemeEnd == 1 && m_userStart == m_schemeEnd + 1)
> +        return;

If we're going to have this check in more than one place, I think we need a named helper function named something like isWindowsStyleFilePath() -- and I have no idea why we would ever have such a thing without a "file:" prefix on it. I really think this is wrong and we need to consider handling it some other way. We should not treat the Windows drive letter as a "scheme" at parsing time.

> +    // Only do this for http and https, unless the URL is not hierarchical.
> +    if (m_protocolInHTTPFamily && hierarchical && pathEnd - pathStart == 0)

It's strange to write "pathEnd == pathStart" as "pathEnd - pathStart == 0". Maybe you can fix this while touching the code.

WebKit uses sentence-style comments rather than comments that end in a period.

> +// This function does not allow leading spaces before the port number.
> +static unsigned getPortFromString(const String& value, unsigned& portEnd)
> +{
> +    unsigned port = 0;
> +    portEnd = 0;
> +    unsigned valueLength = value.length();
> +    while (portEnd < valueLength && isASCIIDigit(value[portEnd]))
> +        portEnd++;
> +    if (portEnd > 0) {
> +        if (portEnd == valueLength)
> +            port = value.toUInt();
> +        else
> +            port = value.substring(0, portEnd).toUInt();
> +    }
> +    return port;
> +}

A function that returns a value such as a port number should not be named get in WebKit code. We use nouns for such getter functions, and "get" only when it's an out function.

Since an attempt to get a character past the end of a string returns the 0 character, there is no need to do the valueLength condition on the first if statement.

WebKit style would write this "if (portEnd)" rather "if (portEnd > 0)".

Since the substring function already special cases a substring of the entire string, there's no need for the "if (portEnd == valueLength)" check -- we can just always use substring() and it will take care of optimizing the normal case. Actually it would be clearer to use left().

Since toUInt() already returns 0 when passed the empty or null string, and the empty string is already optimized, there's no need to special case the portEnd == 0 case.

So this function becomes:

    static unsigned parsePortAtStringStart(const String& value, unsigned& portEnd)
    {
        portEnd = 0
        while (isASCIIDigit(value[portEnd]))
            ++portEnd;
        return value.substring(0, portEnd).toUInt();
    }

And given that this actually is being passed a substring, I think it would be better to make it take another argument and parse a port starting at the passed in location so the caller doesn't have to call substring.

> +            else if (separator + 1 + portEnd == value.length())
> +                url.setHostAndPort(value);
> +            else
> +                url.setHostAndPort(value.substring(0, separator + 1 + portEnd));

Again, there is no need to special case the value.length() case this since value.substring already does that.

> +void HTMLAnchorElement::setHostname(const String& value)
> +{
> +    // Before setting new value:
> +    // Remove all leading U+002F SOLIDUS ("/") characters.
> +    unsigned i = 0;
> +    unsigned hostLength = value.length();
> +    while (i < hostLength && value[i] == '/')
> +        i++;

There is no need to check against hostLength in the loop condition here because String already takes care of that.

> +    String subvalue = value.substring(i);
> +    KURL url = href();
> +    if (!url.canSetHostOrPort())
> +        return;

There is no need to fetch subvalue into a local variable here. It's only used once, and it's used after the if statement, so I suggest just doing the substring call inside the setHost function call.

> +    if (SecurityOrigin::isDefaultPortForProtocol(port, url.protocol() ))

There's an extra space on this line that should go away.

> +    // Following Firefox 3.5.2 which removes anything after the first ":"
> +    String subvalue = (separator > 0 ? value.substring(0, separator) : value);

You don't need the ?: here because signed -1 becomes unsigned UINT_MAX, and then that means the entire string. So you can just say:

    String protocol = value.substring(0, separator);

I also recommend the name protocol or newProtocol rather than subvalue.

> +    String escapedValue = (value[0] == '?') ? value.substring(1) : value;

The name escapedValue here doesn't make sense, because you do the escaping *after* this. This is just value with the leading "?" stripped off it.

I didn't study all the tests carefully his time around, but they look good. It would be good to compile a list of where you decided to match Firefox and where you decided to match IE when the two differed, so people could consider that rather than reading all the test cases.

review- because at least some of my comments above should drive you to make some changes

This is getting really close!

[Yael]

Created attachment 42233 [details]
Patch

Thank you for the review, Darin.
This updated patch takes your comments into account. I changed the test result of local file to expect "FAIL" instead of "PASS", we should revisit that in a separate patch. As a side note, IE and FF do recognize c:/ as a local file, but do not recognize c:\ as a local file. My implementation was doing the same.

I was not sure where to publish the compatibility list. In general, I always tried to match IE, and if IE threw an exception, then I tried to match FF.

One test case (line 23) in fast/loader/url-parse-1.html is failing with my patch. It expects that we always add '/' if the path is empty, but we should only add that if the URL is hierarchical. (IE8 does not add that)
Should I change the expected result in the same patch, or as a follow-up?

thanks, Yael

[Darin Adler]

Comment on attachment 42233 [details]
Patch

> +    // Returns true if it is allowed to set the host and port for the URL
> +    // This is allowed if the URL is valid, is hierarchical and has a
> +    // server based naming authority.
> +    // IE8 allows setting the hsot for foo:// protocol, so does WebKit.
> +    bool canSetHostOrPort() const { return isHierarchical(); }

Typo: "host" is spelled "hsot" here. The comment says something about a server based naming authority, but the function body doesn't have any code to do that.

I think a better comment would be:

    // Returns true if you can set the host and port for the URL.
    // Non-hierarchical URLs don't have a host and port.

Unless that comment is wrong in some way.

> +    // Returns true if the URL is hierarchical.
> +    bool canSetPathname() const { return isHierarchical(); }

This comment doesn't add anything. It just repeats whats in the function body. Please leave it out or use a comment like the one I suggested above.

>      // For canonicalization, ensure we have a '/' for no path.
> -    // Only do this for http and https.
> -    if (m_protocolInHTTPFamily && pathEnd - pathStart == 0)
> +    // Do this only for hierarchical URL with protocol http or https.
> +    if (m_protocolInHTTPFamily && hierarchical && pathEnd == pathStart)
>          *p++ = '/';

Does the bug fix above have any effect on existing code, outside the new HTMLAnchorElement functions? I assume the answer is yes, and if so, we need a test case to show the fix and to help us make sure the effect is a desirable one.

Besides null it is probably worthwhile to test setting to undefined.

Generally enough here is good that I'd like to say review+, but I don't really like those comments and I'd prefer not to land them that way, so I'll say review- so you can fix them.

[Yael]

(In reply to comment #12)
> 
> Does the bug fix above have any effect on existing code, outside the new
> HTMLAnchorElement functions? I assume the answer is yes, and if so, we need a
> test case to show the fix and to help us make sure the effect is a desirable
> one.
> 
Yes, it does affect existing code. In comment #11 I was asking about the test that is failing due to this fix. Should I change the expected result along with this patch, or as a separate patch?

[Darin Adler]

(In reply to comment #11)
> One test case (line 23) in fast/loader/url-parse-1.html is failing with my
> patch. It expects that we always add '/' if the path is empty, but we should
> only add that if the URL is hierarchical. (IE8 does not add that)
> Should I change the expected result in the same patch, or as a follow-up?

You must change the expected result in the same patch where you change behavior. All tests have to pass.

Another option is to do that particular bug fix separately in a previous patch. That would be better than including the fix with the rest of your work here. One task per patch is best.

A third option is to leave the code change for that bug fix out, and land your initial work leaving that broken. Your expected test results need to reflect the expected failure. Then you could do a follow-up that both adds the code change and changes the expected results to reflect the new successes.

[Yael]

(In reply to comment #14)
> Another option is to do that particular bug fix separately in a previous patch.
> That would be better than including the fix with the rest of your work here.
> One task per patch is best.

https://bugs.webkit.org/show_bug.cgi?id=30971 was created for this bug fix and for updating the test case.

[Yael]

Created attachment 42283 [details]
Patch

Updated the comments, and the bug fix for extra '/' was committed separately.

(In reply to comment #12)
> Besides null it is probably worthwhile to test setting to undefined.
I did test with undefined, but IE, FF and WebKit all convert undefined to "undefined" and use that as the input string, so I did not add new tests for that.

[Darin Adler]

(In reply to comment #16)
> I did test with undefined

Excellent.

> but IE, FF and WebKit all convert undefined to
> "undefined" and use that as the input string

Good to know.

> so I did not add new tests for that.

That was a mistake. You should have. The tests guarantee nobody in the future will have to ask the question about what undefined should do.

[Yael]

Created attachment 42285 [details]
Patch

A few more tests added, setting parts of the URL to undefined.

[Darin Adler]

Comment on attachment 42285 [details]
Patch

>  bool protocolIs(const String& url, const char* protocol);
>  bool protocolIsJavaScript(const String& url);
> +bool protocolIsValid(const String& protocol);

Sorry I didn’t realize this in earlier patch rounds. The other functions here work on URLs and check various aspects of the protocol. This new one works on a protocol.

I think it should be named isValidProtocol instead of protocolIsValid, to make this slightly clearer. This name change can happen in a separate patch.

I didn’t look too closely at anything else in this patch -- a cursory look seems to indicate it’s right, so r=me

[WebKit Commit Bot]

Comment on attachment 42285 [details]
Patch

Rejecting patch 42285 from commit-queue.

Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1
Last 500 characters of output:
l/Projects/build/WebCore.build/Release/WebCore.build/Script-5D0D540D0E9862F60029E223.sh

Touch /Users/eseidel/Projects/build/Release/WebCore.framework
    cd /Users/eseidel/Projects/CommitQueue/WebCore
    /usr/bin/touch -c /Users/eseidel/Projects/build/Release/WebCore.framework
** BUILD FAILED **

The following build commands failed:
Derived Sources:
	PhaseScriptExecution "/Users/eseidel/Projects/build/WebCore.build/Release/Derived Sources.build/Script-DD041FBD09D9DDBE0010AF2A.sh"
(1 failure)

[Yael]

Any suggestions on the build failure? I built and ran DRT with this patch on my MAC successfully.

[Yael]

Seems that the build problem is result of PublicDOMInterfaces.h not being updated with the new API. Strangely enough, it builds fine the second time I try.

Sam, Mark Row suggested on IRC that I ask you how to fix this problem. thanks,

[Eric Seidel (no email)]

@Yael:  I'm not sure who you're asking.  If you're asking me, I don't have an answer.  If something requires two builds to work right, that seems like a build system issue and something that probably should be solved before we try and land this patch.  The commit-queue has no way to know to build a patch twice.

[Yael]

The first build runs CodeGeneratorObjC.pm to generate the ObjC bindings, and then checks if public interfaces changed (PublicDOMInterfaces.h). Since the API of DOMHTMLAnchorElement changed, the script simply dies. (CodeGeneratorObjC.pm line 297).

The second time, the ObjC bindigns are already generated, so there is no check of SDK API, and the build succeeds. 

I would really appreciate if someone from Apple could help me resolve this problem.

[Darin Adler]

(In reply to comment #24)
> The first build runs CodeGeneratorObjC.pm to generate the ObjC bindings, and
> then checks if public interfaces changed (PublicDOMInterfaces.h). Since the API
> of DOMHTMLAnchorElement changed, the script simply dies. (CodeGeneratorObjC.pm
> line 297).

Right, that means that this change can break Mac OS X applications who depend on the current interface. I think, however, that changing read-only properties to be read-write is actually an acceptable change. We’ll need to talk to Tim Hatcher about how to resolve this. For the short term, one possibility is to put #if in the IDL file that makes the attributes be read-only in Objective-C for now, and read-write only in all other languages.

> The second time, the ObjC bindigns are already generated, so there is no check
> of SDK API, and the build succeeds. 

The fact that the build succeeds a second time is a bug in the build system. If a patch violates the rules about changing public DOM interfaces that should be a permanent build failure, not one we can work around by rebuilding.

[Timothy Hatcher]

(In reply to comment #25)
> (In reply to comment #24)
> > The first build runs CodeGeneratorObjC.pm to generate the ObjC bindings, and
> > then checks if public interfaces changed (PublicDOMInterfaces.h). Since the API
> > of DOMHTMLAnchorElement changed, the script simply dies. (CodeGeneratorObjC.pm
> > line 297).
> 
> Right, that means that this change can break Mac OS X applications who depend
> on the current interface. I think, however, that changing read-only properties
> to be read-write is actually an acceptable change. We’ll need to talk to Tim
> Hatcher about how to resolve this. For the short term, one possibility is to
> put #if in the IDL file that makes the attributes be read-only in Objective-C
> for now, and read-write only in all other languages.

Those are really the only two options (make it public or #ifdef it to be readonly), since ObjC properties can't be split between public and private headers like two getter and setter methods can.

> > The second time, the ObjC bindigns are already generated, so there is no check
> > of SDK API, and the build succeeds. 
> 
> The fact that the build succeeds a second time is a bug in the build system. If
> a patch violates the rules about changing public DOM interfaces that should be
> a permanent build failure, not one we can work around by rebuilding.

That is a bug, we need to remove the generated file so it gets rebuilt each time until the public change is resolved. Filed as bug 31178.

[Yael]

Created attachment 42584 [details]
Patch

Add #if defined(LANGUAGE_OBJECTIVE_C) && LANGUAGE_OBJECTIVE_C flag in the IDL file, to preserve the behavior for ObjC bindings.

[WebKit Commit Bot]

Comment on attachment 42584 [details]
Patch

Rejecting patch 42584 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11579 test cases.
http/tests/security/mixedContent/redirect-https-to-http-script-in-iframe.html -> failed

Exiting early after 1 failures. 9001 tests run.
256.78s total testing time

9000 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output

[Eric Seidel (no email)]

Comment on attachment 42584 [details]
Patch

That test seems unrelated.  I suspect it's just flakey.  I've filed bug 31190

[WebKit Commit Bot]

Comment on attachment 42584 [details]
Patch

Rejecting patch 42584 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11579 test cases.
http/tests/security/mixedContent/redirect-https-to-http-script-in-iframe.html -> failed

Exiting early after 1 failures. 9001 tests run.
255.35s total testing time

9000 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output

[Eric Seidel (no email)]

Two failures in a row would suggest this is a real failure.  Not sure what's wrong with the patch, but it's causing at least one layout test regression.

[Yael]

(In reply to comment #31)
> Two failures in a row would suggest this is a real failure.  Not sure what's
> wrong with the patch, but it's causing at least one layout test regression.

thanks for the effort, Eric. I will look into this failure.

[Eric Seidel (no email)]

Comment on attachment 42584 [details]
Patch

Actually I think I was wrong about the failure.  I've filed bug 31305 and lets queue this again.

[WebKit Commit Bot]

Comment on attachment 42584 [details]
Patch

Rejecting patch 42584 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11590 test cases.
http/tests/security/mixedContent/insecure-image-in-main-frame.html -> failed

Exiting early after 1 failures. 9001 tests run.
254.96s total testing time

9000 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output

[Eric Seidel (no email)]

Different, but related failure.  Not sure who is at fault here.  This patch, or bug 31305...

[Eric Seidel (no email)]

Comment on attachment 42584 [details]
Patch

Adding this back to the cq.  The commit-queue is blocked until bug 31200 is resolved anyway.

[WebKit Commit Bot]

Comment on attachment 42584 [details]
Patch

Clearing flags on attachment: 42584

Committed r50784: <http://trac.webkit.org/changeset/50784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.