[webkit-reviews] review granted: [Bug 39877] [Chromium] get rid of named interceptor on HTMLDocument and introduce/remove accessors when named items get deleted/removed : [Attachment 57340] With Chromium rolled forward

[bugzilla-daemon at webkit.org]

Nate Chapin <japhet at chromium.org> has granted anton muhin
<antonm at chromium.org>'s request for review:
Bug 39877: [Chromium] get rid of named interceptor on HTMLDocument and
introduce/remove accessors when named items get deleted/removed
https://bugs.webkit.org/show_bug.cgi?id=39877

Attachment 57340: With Chromium rolled forward
https://bugs.webkit.org/attachment.cgi?id=57340&action=review

------- Additional Comments from Nate Chapin <japhet at chromium.org>
r+ with nits.

> +    // TODO(antonm): consider passing AtomicStringImpl directly.

FIXME instead of TODO.

> +    // Just emulate a normal JS behaviour---install a property on this.
> +    // Potential problem---something with the setter below---let's hope
that's not the case.
> +    info.This()->ForceSet(name, value);

This comment confuses me a little. Is the potential problem that we might have
already set
another value to this name? Should we have an ASSERT or error handling? I just
get nervous
when I see a comment that includes the phrase "let's hope" :)

> -    wrapper->SetInternalField(V8HTMLDocument::markerIndex, marker);
> -    wrapper->SetInternalField(V8HTMLDocument::shadowIndex, marker);

I think markerIndex and shadowIndex are now unused, so they should be removed
from
GetInternalFields() in CodeGeneratorV8.pm.