http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js
File src/com/google/caja/plugin/domado.js (right):
http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js#newcode60
src/com/google/caja/plugin/domado.js:60: var PLACEHOLDER_theTools; //
TODO(kpreid): This is a placeholder for something to replace the
pluginId mechanism, making it into a global singleton instead. 'tools'
is the replacement for ___ properties on 'imports'.
On 2011/07/20 00:26:47, kpreid2 wrote:
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Isn't tools also akin to a view? html-emitter accesses
tools.document.
'tools' has been renamed 'domicile'.
I don't know what you mean by "view" here. The View concept in DOM? If
so, that
doesn't seem to say much. I suppose it could be seen as a
bundle-of-authorities-plus-a-document as the browser Window is, but in
this case
it contains both the (tame) document and window objects.
ok. By "view" I meant
http://www.w3.org/TR/DOM-Level-2-Views/views.html#Views-AbstractView
http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js#newcode192
src/com/google/caja/plugin/domado.js:192: enumerable:
domitaModules.getPropertyDescriptor(object, propName)
On 2011/07/20 00:26:47, kpreid2 wrote:
On 2011/07/18 21:57:01, MikeSamuel wrote:
> propName might be coerced to two different strings above if propName
can come
> from an untrusted source.
> Maybe do propName += ''; at the start.
That case shouldn't arise, but done, just for robustness.
> Does getPropertyDescriptor not return a nullish value when propName
is blank?
> Should properties default to non-enumerable if not overriding?
This is only ever used for overriding since the problem it is a
workaround for
cannot occur otherwise.
In the long run I should look into the ES5 semantics for whether this
is a bug
workaround or something that is actually needed.
k
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js
File src/com/google/caja/plugin/domado.js (right):
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode204
src/com/google/caja/plugin/domado.js:204: function (aCallback) {
On 2011/07/20 00:26:48, kpreid2 wrote:
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Do the callbacks tend to receive an argument?
>
> Is it worth doing an arity check?
>
> Do we need to check that the argument is callable by untrusted code
or is that
> not ES53-ish?
>
> Is eval a valid callback?
No idea. This function is a hacked-up version of the one from Domita,
removing
the Cajita/Valija elements; I suggest you compare them and tell me
what this
function *should* be doing.
I'll chat with Mike Stay.
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode348
src/com/google/caja/plugin/domado.js:348:
ExpandoProxyHandler.prototype.getOwnPropertyDescriptor = function (name)
{
On 2011/07/20 00:26:48, kpreid2 wrote:
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Are these name parameters guaranteed to be coerced to strings or
numbers
before
> reaching the proxy handler?
I would expect so. ES5 semantics have the property name in an ordinary
foo[bar]
coerced to string well before any internal methods of foo are invoked.
But the
final arbiter would have to be the semantics of proxies, which don't
seem to be
written down on ES Wiki.
MarkM would be the person to consult for this.
Mark, is that correct?
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode520
src/com/google/caja/plugin/domado.js:520: TameXHRConf.confide(this);
On 2011/07/20 00:26:48, kpreid2 wrote:
On 2011/07/18 21:57:01, MikeSamuel wrote:
> If someone forgets a `new` then we might confide in the global
object. Maybe
a
> simple instanceof check at the top of constructors that confide in
this would
> add a layer of protection.
This is not a risk in strict mode; 'this' will be undefined for
function calls.
I'm confused. Is domado.js running inside ES5/3 on an ES3 interpreter?
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode530
src/com/google/caja/plugin/domado.js:530: }
On 2011/07/20 00:26:48, kpreid2 wrote:
On 2011/07/18 21:57:01, MikeSamuel wrote:
> This self idiom seems to occur several times with subtle variations.
Can we
> reduce the differences so someone who has to find and change uses of
this
> idiom has an easier time operating on all of them?
Done. I've changed all "return proxy;" inside of if statements to use
this "var
self; ... return self;" pattern instead. Now whenever a constructor
may return
an object other than this, it is "return self;" at the end.
thx
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode652
src/com/google/caja/plugin/domado.js:652: var evt = { target: this };
On 2011/07/20 00:26:48, kpreid2 wrote:
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Do we use this, not the proxy because an event handler wrapper
dispatches
> to the proxy if proxy self != this?
There aren't any event handler wrappers here (see the definition of
onreadystatechange above); this is just a bug.
In fact, I don't even have access to the proxy here! (No reference to
it is
retained from this or p(this).) But, since TameXHR always creates
(rather than
taking an existing) feral XHR, we don't actually need an expando
proxy. I
therefore propose to just throw out the proxy logic here and let
clients set
properties on this object (as we do on non-proxy-supporting
environments for all
of our objects).
That would simplify a lot of the code above?
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode892
src/com/google/caja/plugin/domado.js:892: return array.push.apply(array,
rest);
On 2011/07/20 00:26:48, kpreid2 wrote:
On 2011/07/18 21:57:01, MikeSamuel wrote:
> This seems to be trusting array.push which is OK I guess if it is
only called
> on listener lists we control, but if you don't need the negative
index
handling,
> isn't it easier to just do array.splice(from, to - from) or the
negative index
> handling equivalent : array.splice((from + array.length) %
array.length, (to +
> array.length) % array.length - (from + array.length) %
array.length)?
This code is directly from Domita; I prefer not to change it now.
k
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode1665
src/com/google/caja/plugin/domado.js:1665: extendedAccessors: true,
On 2011/07/20 00:26:48, kpreid2 wrote:
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Is this necessary?
No. What I wonder about is whether, if extendedAccessors were changed
later to
have additional semantics, this would better be true. Probably this
would need
to be changed anyway. Maybe 'extendedAccessors' should be replaced
with a flag
specific to the "pass property names" semantics?
Future planning for many flavors of accessors without knowing how often
one extended feature predicts the need of other features is problematic.
I could go either way on the name as long as it's something easily
auditable -- a simple grep should pull out all uses to it and not pull
out other stuff in non-malicious code.
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode2386
src/com/google/caja/plugin/domado.js:2386:
TameBackedNode.prototype.dispatchEvent = nodeMethod(function
dispatchEvent(evt) {
On 2011/07/20 00:26:48, kpreid2 wrote:
Why is this function not anonymous? I just realized I've introduced a
name
confusion with this vs. what used to be plugin_dispatchEvent___ below.
I'm going
to rename the latter for clarity, but what is the reason for naming
this
function? (And can we bury it in a (function() {})()?)
The link above,
http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventTarget ,
defines the dispatchEvent method as part of the EventTarget interface
which HTMLElements implement.
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode3927
src/com/google/caja/plugin/domado.js:3927: src: NP.filter(false,
identity, true, identity)
On 2011/07/20 00:26:48, kpreid2 wrote:
On 2011/07/18 21:57:01, MikeSamuel wrote:
> How does the identity setter with an attr getter work here? This
turns into a
> call to setAttribute("src") which presumably invokes the URL policy?
This is a property *getter* and attribute *setter*, and this is copied
from
Domita.
> Where is the policy code that prevents adding text nodes to a script
element?
This was lost in refactoring; fixed.
I also found out that the test cases checking for this were disabled
in r3802 by
you due to crashes on IE. I think we ought to re-enable these tests at
least for
everything-but-IE; I may prepare a separate changeset for that.
Ack. Definitely.
I'm working on fixing the bugs discovered by enabling those tests, but
it's
hairy enough I'm deferring that until after this round of itsy bitsy
code review
changes.
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode5178
src/com/google/caja/plugin/domado.js:5178: //'HTMLScriptElement',
On 2011/07/20 00:26:48, kpreid2 wrote:
On 2011/07/18 21:57:01, MikeSamuel wrote:
> Why are these commented out?
Because they actually have classes, and the previous code wasn't
checking for
conflicts so it was silently overwriting the non-default constructor
with the
default.
I propose to change this so that it is a list of *all* HTML element
classes, and
change the logic so it uses the default only if there is a
non-default. Then
the only place default/non-default is decided is in the defineElement
uses
above. Good?
Please do.
http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode5267
src/com/google/caja/plugin/domado.js:5267: //}
On 2011/07/20 00:26:48, kpreid2 wrote:
On 2011/07/18 21:57:01, MikeSamuel wrote:
> I do not know yet. I think it is good to leave this catch block
around though
> to guard against IE discarding finally blocks.
Can you provide the text of a comment explaining this?
Should we just keep this error reporting but lose the cajitaStack___
logic? I'm
not sure what cajitaStack___ means.
Actually, it looks like cajitaStack___ is only mentioned in domita.js so
I think this code is definitely obsolete.
It used to be hooked into by an extra layer of rewriting that maintained
a call stack mapping back to original line numbers but that's dead now.
http://codereview.appspot.com/4535118/
