URLValueData::mLocalURLFlag should be checked in operator== / URIEquals

RESOLVED FIXED in Firefox 51

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8785909 [details]
Bug 1298768 - Compare mLocalURLFlag in URLValueData comparison functions.

https://reviewboard.mozilla.org/r/74918/#review72842

::: layout/style/nsCSSValue.cpp:2638
(Diff revision 1)
>             (mURI && aOther.mURI &&
>              NS_SUCCEEDED(mURI->Equals(aOther.mURI, &eq)) &&
>              eq)) &&
>            (mOriginPrincipal == aOther.mOriginPrincipal ||
> -           self.mOriginPrincipal.get()->Equals(other.mOriginPrincipal.get()));
> +           self.mOriginPrincipal.get()->Equals(other.mOriginPrincipal.get())) &&
> +          mLocalURLFlag != aOther.mLocalURLFlag;

I think it should be
mLocalURLFlag == aOther.mLocalURLFlag?

And, how about calling URIEquals here, so that we don't need to duplicate code in these two function

return NS_strcmp(nsCSSValue::GetBufferValue(mString),... && URIEquals(aOther);

::: layout/style/nsCSSValue.cpp:2668
(Diff revision 1)
>    // fast path to equality
>    return (mURI == aOther.mURI ||
>            (NS_SUCCEEDED(mURI->Equals(aOther.mURI, &eq)) && eq)) &&
>           (mOriginPrincipal == aOther.mOriginPrincipal ||
> -          self.mOriginPrincipal.get()->Equals(other.mOriginPrincipal.get()));
> +          self.mOriginPrincipal.get()->Equals(other.mOriginPrincipal.get())) &&
> +         mLocalURLFlag != aOther.mLocalURLFlag;

mLocalURLFlag == aOther.mLocalURLFlag?
Attachment #8785909 - Flags: review?(cku) → review+
(Assignee)

Comment 3

a year ago
(In reply to C.J. Ku[:cjku](UTC+8) from comment #2)
> I think it should be
> mLocalURLFlag == aOther.mLocalURLFlag?

Thanks, yes it should.

> And, how about calling URIEquals here, so that we don't need to duplicate
> code in these two function
> 
> return NS_strcmp(nsCSSValue::GetBufferValue(mString),... &&
> URIEquals(aOther);

That would be fine, but I am removing URIEquals in a later patch in bug 1297963, so I won't bother.

Comment 4

a year ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56f8bca8f8e8
Compare mLocalURLFlag in URLValueData comparison functions. r=cjku
Backed out along with bug 1297963 for browser-chrome failures.

Comment 6

a year ago
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66f6ab299414
Backed out 5 changesets (bug 1298768, bug 1297963) for causing widespread mochitest-bc failures.

Comment 7

a year ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e89e213ef1fa
Compare mLocalURLFlag in URLValueData comparison functions. r=cjku

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e89e213ef1fa
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.