Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2501333002:
binding: Makes window/frames/self attributes return itself. (Closed)
Descriptionbinding: Makes window/frames/self attributes return itself.
Per HTML spec[1], window, frames, and self attributes must return
the global proxy object. This CL simply follows it.
Note that these attributes are expected to never return null.
We're currently breaking this assumption on detached frames.
[1] https://html.spec.whatwg.org/multipage/browsers.html#dom-window
BUG=618672
Committed: https://crrev.com/1c5231afadf1376cd206a64f0370fda8007254be
Cr-Commit-Position: refs/heads/master@{#433178}
Patch Set 1 #Patch Set 2 : Updated a layout test. #Patch Set 3 : Uses cached accessors. #Patch Set 4 : Stops using cached accessors for self and frames. #Patch Set 5 : Completely stopped using cached accessors. #
Messages
Total messages: 48 (31 generated)
The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yukishiino@chromium.org changed reviewers: + haraken@chromium.org
Could you review this CL?
When |x.self| is accessed, the call site already has access to |x|, so returning
|x| itself should be okay.
As we don't fully support detached frames so far,
self.setTimeout(...)
was equivalent to
(null).setTimeout(...)
on a detached frame before this CL.
After this CL, it's equivalent to
setTimeout(...)
because |self| returns the WindowProxy itself.
Either way, things wouldn't work on detached frames. But I'd like to make this
change as a step to support detached frames.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
haraken@chromium.org changed reviewers: + dcheng@chromium.org, jochen@chromium.org
Does the new behavior match other browsers? +dcheng +jochen: Do you have any concern about this change? BTW, these DOM attributes would be a good candidate for cacheable attributes.
yeah, why not just use a cached accessor for this?
On 2016/11/16 17:40:02, haraken wrote: > Does the new behavior match other browsers? FF and IE never return null for window/self/frames, even when an iframe is detached. On 2016/11/17 01:56:58, jochen (travelling til Nov 18) wrote: > yeah, why not just use a cached accessor for this? That's true. I just overlooked it. Will do.
The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated to use cached accessors. Could you take another look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/17 10:41:08, Yuki wrote: > Updated to use cached accessors. > Could you take another look? Stopped using cached accessors for |self| and |frames| because they are not [Unforgeable]. Only applied to |window| which is [Unforgeable].
On 2016/11/17 13:35:33, Yuki wrote: > On 2016/11/17 10:41:08, Yuki wrote: > > Updated to use cached accessors. > > Could you take another look? > > Stopped using cached accessors for |self| and |frames| because they are not > [Unforgeable]. Only applied to |window| which is [Unforgeable]. How is [Unforgeable] related to cachable accessors?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/11/17 15:04:59, haraken wrote: > On 2016/11/17 13:35:33, Yuki wrote: > > On 2016/11/17 10:41:08, Yuki wrote: > > > Updated to use cached accessors. > > > Could you take another look? > > > > Stopped using cached accessors for |self| and |frames| because they are not > > [Unforgeable]. Only applied to |window| which is [Unforgeable]. > > How is [Unforgeable] related to cachable accessors? Do cached accessors support [Replaceable]? I thought they don't. Anyway, some of tests are still failing after starting to use cached accessors, let me take a look.
The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/18 at 06:58:10, yukishiino wrote: > On 2016/11/17 15:04:59, haraken wrote: > > On 2016/11/17 13:35:33, Yuki wrote: > > > On 2016/11/17 10:41:08, Yuki wrote: > > > > Updated to use cached accessors. > > > > Could you take another look? > > > > > > Stopped using cached accessors for |self| and |frames| because they are not > > > [Unforgeable]. Only applied to |window| which is [Unforgeable]. > > > > How is [Unforgeable] related to cachable accessors? > > Do cached accessors support [Replaceable]? I thought they don't. > > Anyway, some of tests are still failing after starting to use cached accessors, let me take a look. I think it should work with replaceable
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== binding: Makes window/frames/self attributes return itself. Per HTML spec[1], window, frames, and self attributes must return the global proxy object. This CL simply follows it. Note that these attributes are expected to never return null. We're currently breaking this assumption on detached frames. [1] https://html.spec.whatwg.org/multipage/browsers.html#dom-window BUG=618672 ========== to ========== binding: Makes window/frames/self attributes return itself. Per HTML spec[1], window, frames, and self attributes must return the global proxy object. This CL simply follows it. Note that these attributes are expected to never return null. We're currently breaking this assumption on detached frames. [1] https://html.spec.whatwg.org/multipage/browsers.html#dom-window BUG=618672 ==========
yukishiino@chromium.org changed reviewers: + vogelheim@chromium.org
On 2016/11/18 08:25:45, jochen wrote:
> On 2016/11/18 at 06:58:10, yukishiino wrote:
> > On 2016/11/17 15:04:59, haraken wrote:
> > > On 2016/11/17 13:35:33, Yuki wrote:
> > > > On 2016/11/17 10:41:08, Yuki wrote:
> > > > > Updated to use cached accessors.
> > > > > Could you take another look?
> > > >
> > > > Stopped using cached accessors for |self| and |frames| because they are
> not
> > > > [Unforgeable]. Only applied to |window| which is [Unforgeable].
> > >
> > > How is [Unforgeable] related to cachable accessors?
> >
> > Do cached accessors support [Replaceable]? I thought they don't.
> >
> > Anyway, some of tests are still failing after starting to use cached
> accessors, let me take a look.
>
> I think it should work with replaceable
Hmm, when I made |self| as [CachedAccessor],
self = 'foo'
self // => Window object
but when I removed [CachedAccessor],
self = 'foo'
self // => 'foo'
Are you sure that cached accessors support [Replaceable]?
On 2016/11/18 at 09:06:22, yukishiino wrote: > On 2016/11/18 08:25:45, jochen wrote: > > On 2016/11/18 at 06:58:10, yukishiino wrote: > > > On 2016/11/17 15:04:59, haraken wrote: > > > > On 2016/11/17 13:35:33, Yuki wrote: > > > > > On 2016/11/17 10:41:08, Yuki wrote: > > > > > > Updated to use cached accessors. > > > > > > Could you take another look? > > > > > > > > > > Stopped using cached accessors for |self| and |frames| because they are > > not > > > > > [Unforgeable]. Only applied to |window| which is [Unforgeable]. > > > > > > > > How is [Unforgeable] related to cachable accessors? > > > > > > Do cached accessors support [Replaceable]? I thought they don't. > > > > > > Anyway, some of tests are still failing after starting to use cached > > accessors, let me take a look. > > > > I think it should work with replaceable > > Hmm, when I made |self| as [CachedAccessor], > self = 'foo' > self // => Window object > but when I removed [CachedAccessor], > self = 'foo' > self // => 'foo' > > Are you sure that cached accessors support [Replaceable]? maybe the IDL generator doesn't? but in theory, it should work :)
The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/18 09:07:57, jochen wrote: > On 2016/11/18 at 09:06:22, yukishiino wrote: > > On 2016/11/18 08:25:45, jochen wrote: > > > On 2016/11/18 at 06:58:10, yukishiino wrote: > > > > On 2016/11/17 15:04:59, haraken wrote: > > > > > On 2016/11/17 13:35:33, Yuki wrote: > > > > > > On 2016/11/17 10:41:08, Yuki wrote: > > > > > > > Updated to use cached accessors. > > > > > > > Could you take another look? > > > > > > > > > > > > Stopped using cached accessors for |self| and |frames| because they > are > > > not > > > > > > [Unforgeable]. Only applied to |window| which is [Unforgeable]. > > > > > > > > > > How is [Unforgeable] related to cachable accessors? > > > > > > > > Do cached accessors support [Replaceable]? I thought they don't. > > > > > > > > Anyway, some of tests are still failing after starting to use cached > > > accessors, let me take a look. > > > > > > I think it should work with replaceable > > > > Hmm, when I made |self| as [CachedAccessor], > > self = 'foo' > > self // => Window object > > but when I removed [CachedAccessor], > > self = 'foo' > > self // => 'foo' > > > > Are you sure that cached accessors support [Replaceable]? > > maybe the IDL generator doesn't? but in theory, it should work :) Ah, we need to update the value of the private property in the setter? Then, we don't do that for now. We need to correctly implement accessor-property-version of [Replaceable], I guess. Plus, it turned out that we have an issue about cross-origin-accessible properties + cached accessors. Currently, all the cross-origin-accessible attributes of Window are implemented as data-type properties, but cached accessors require properties to be accessor properties. It seems that we need some effort to change cross-origin-accessible attributes to accessor proeprties, so let me stop using cached accessors in this CL. Once it get ready, I'll use cached accessors. Could you take another look?
I meant that maybe we just don't generate a setter with CachedAccessor is given, or we create the CachedAccessor with the wrong attributes? anyways, it's fine to add that later
On 2016/11/18 09:48:03, jochen wrote: > I meant that maybe we just don't generate a setter with CachedAccessor is given, > or we create the CachedAccessor with the wrong attributes? > > anyways, it's fine to add that later Yeah, doing that later is fine. LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yukishiino@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== binding: Makes window/frames/self attributes return itself. Per HTML spec[1], window, frames, and self attributes must return the global proxy object. This CL simply follows it. Note that these attributes are expected to never return null. We're currently breaking this assumption on detached frames. [1] https://html.spec.whatwg.org/multipage/browsers.html#dom-window BUG=618672 ========== to ========== binding: Makes window/frames/self attributes return itself. Per HTML spec[1], window, frames, and self attributes must return the global proxy object. This CL simply follows it. Note that these attributes are expected to never return null. We're currently breaking this assumption on detached frames. [1] https://html.spec.whatwg.org/multipage/browsers.html#dom-window BUG=618672 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== binding: Makes window/frames/self attributes return itself. Per HTML spec[1], window, frames, and self attributes must return the global proxy object. This CL simply follows it. Note that these attributes are expected to never return null. We're currently breaking this assumption on detached frames. [1] https://html.spec.whatwg.org/multipage/browsers.html#dom-window BUG=618672 ========== to ========== binding: Makes window/frames/self attributes return itself. Per HTML spec[1], window, frames, and self attributes must return the global proxy object. This CL simply follows it. Note that these attributes are expected to never return null. We're currently breaking this assumption on detached frames. [1] https://html.spec.whatwg.org/multipage/browsers.html#dom-window BUG=618672 Committed: https://crrev.com/1c5231afadf1376cd206a64f0370fda8007254be Cr-Commit-Position: refs/heads/master@{#433178} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1c5231afadf1376cd206a64f0370fda8007254be Cr-Commit-Position: refs/heads/master@{#433178} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
