Bug 26180 - If "otherOrigin == this" in SecurityOrigin::equal, it's obviously equal.
Summary: If "otherOrigin == this" in SecurityOrigin::equal, it's obviously equal.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-03 19:19 PDT by Jeremy Orlow
Modified: 2009-06-04 11:45 PDT (History)
0 users

See Also:


Attachments
A fix. (1.14 KB, patch)
2009-06-03 19:27 PDT, Jeremy Orlow
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2009-06-03 19:19:51 PDT
If "otherOrigin == this" in SecurityOrigin::equal, it's obviously equal.

From http://lists.macosforge.org/pipermail/webkit-dev/2009-June/008038.html

> SecurityOrigin was created to cleanup the way we do same-origin checks in
> the JavaScript bindings, and as such, carry some of that baggage (eg. domain
> relaxing).  The main design constraint then was that it was a cheap compare
> for two origins representing the same document (pointer compare) as that is
> common case.  The code evolved from there.

Makes sense.  Note that SecurityOrigin::equal does not currently do a fast
path check of "this == other" (to just do a pointer compare as you
mentioned) so this is probably low hanging fruit to make things a bit
faster.  (Though, as you said, it's not showing up on current profiles...so
it's probably not a huge deal.)
Comment 1 Jeremy Orlow 2009-06-03 19:27:42 PDT
Created attachment 30937 [details]
A fix.
Comment 2 Sam Weinig 2009-06-03 19:42:29 PDT
Wouldn't it makes more sense to do this in isSameSchemeHostPort?
Comment 3 Jeremy Orlow 2009-06-03 20:23:38 PDT
I don't think so.  I only see isSameSchemeHostPort used a couple places, but they don't seem like they'd benefit from such an optimization [1].  ::equal, on the other hand, is called pretty often (it's used a lot in hash tables).  It seems a bit painful to require another function call plus a branch or two for a function that's not called very often.

We could put the == check in both places.  Doing one additional branch isn't going to hurt the slow path.  It doesn't seem necessary though.

[1] http://www.google.com/codesearch?hl=en&q=isSameSchemeHostPort+package%3A%22git%3A%2F%2Fandroid.git.kernel.org%2Fplatform%2Fexternal%2Fwebkit.git%22
Comment 4 Darin Adler 2009-06-04 09:18:59 PDT
Comment on attachment 30937 [details]
A fix.

Seems fine. r=me
Comment 5 Brent Fulgham 2009-06-04 11:10:54 PDT
Assign for landing.
Comment 6 Brent Fulgham 2009-06-04 11:34:50 PDT
Landed in @r44420.