Bug 26006 - Sort out lexical / dynamic global object for window.location
Summary: Sort out lexical / dynamic global object for window.location
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-25 03:15 PDT by Adam Barth
Modified: 2009-05-25 14:50 PDT (History)
3 users (show)

See Also:


Attachments
patch (28.45 KB, patch)
2009-05-25 03:27 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
patch (30.08 KB, patch)
2009-05-25 09:52 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
patch with lexicalFrame (30.08 KB, patch)
2009-05-25 12:10 PDT, Adam Barth
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2009-05-25 03:15:00 PDT
Our choice of lexicalGlobalObject or dynamicGlobalObject not correct.  The correct choices appear to be:

1) Use dynamicGlobalObject to find the user gesture.
2) Use dynamicGlobalObject to complete URLs.
3) Use lexicalGlobalObject to find the referrer.
4) Use lexicalGlobalObject for the frame navigation checks.
5) Use lexicalGlobalObject for the XSS checks.

Patch forthcoming.
Comment 1 Adam Barth 2009-05-25 03:27:01 PDT
Created attachment 30647 [details]
patch

This patch isn't fully vetted yet, but it's bedtime.  Hopefully I'm nominate it for review tomorrow.
Comment 2 Sam Weinig 2009-05-25 09:18:38 PDT
I would prefer we don't continue to use the term activeFrame, as I think it conveys nothing.  In fact, in the code you are changing, it was used to refer to the frame associated with the dynamic global object, but now you are associating it with the lexical global object. 
Comment 3 Adam Barth 2009-05-25 09:27:44 PDT
> I would prefer we don't continue to use the term activeFrame,

Sure.  What term would you like?  How about "calling Frame?"
Comment 4 Sam Weinig 2009-05-25 09:39:20 PDT
How about the lexicalFrame and the dynamicFrame.  Not the best names for sure, but they at least evoke the terms we use.
Comment 5 Adam Barth 2009-05-25 09:46:22 PDT
> How about the lexicalFrame and the dynamicFrame.  Not the best names for sure,
> but they at least evoke the terms we use.

I'd like to use the same term in both the JSC and V8 bindings so they're easier to keep in sync.  The problem with lexicalFrame is that V8 would think that's a different frame because it would give you the lexicalFrame for the *current* function (i.e., the one we're implementing) not the lexical frame for the function that called this one...
Comment 6 Adam Barth 2009-05-25 09:52:52 PDT
Created attachment 30653 [details]
patch

This patch uses the "callingFrame" name.  We can change the name later.  There are still a bunch more of these lexical/dynamic issues to sort out (e.g., window.open).
Comment 7 Sam Weinig 2009-05-25 11:53:47 PDT
While I understand your desire to have a name that works with the terminology the v8 bindings folks decided to use, I don't think we should make it harder for the webkit folks because of that.  CallingFrame doesn't state which global object it is using clearly, and therefore is less than ideal for me.  I would much prefer to go with lexicalFrame and dynamicFrame and have the v8 bindings change to accommodate.
Comment 8 Adam Barth 2009-05-25 12:01:07 PDT
Comment on attachment 30653 [details]
patch

Ok.  I'll spin up a new patch.
Comment 9 Adam Barth 2009-05-25 12:10:41 PDT
Created attachment 30657 [details]
patch with lexicalFrame

I changed the JSC code to lexicalFrame and left the V8 code with callingFrame.  That way the variables match the terminology for each bindings.  (This patch is still compiling, but I can fix any typos before landing.)
Comment 10 Sam Weinig 2009-05-25 12:21:22 PDT
Comment on attachment 30657 [details]
patch with lexicalFrame

Assuming it all compiles and stuff, r=me.  Thanks!
Comment 11 Adam Barth 2009-05-25 14:50:34 PDT
Transmitting file data ..................
Committed revision 44135.