Bug 39877 – [Chromium] get rid of named interceptor on HTMLDocument and introduce/remove accessors when named items get deleted/removed

• Component: New Bugs
• Status: RESOLVED FIXED

[anton muhin]

[Chromium] get rid of named interceptor on HTMLDocument and introduce/remove accessors when named items get deleted/removed

[anton muhin]

Created attachment 57326 [details]
Patch

[anton muhin]

Created attachment 57330 [details]
Another upload to trigger build bots

[WebKit Review Bot]

Attachment 57330 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2600031

[anton muhin]

(In reply to comment #3)
> Attachment 57330 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/2600031

This is a lie :)  V8 has been already rolled, but probably lags on bbs.

[David Levin]

(In reply to comment #4)
> (In reply to comment #3)
> > Attachment 57330 [details] [details] did not build on chromium:
> > Build output: http://webkit-commit-queue.appspot.com/results/2600031
> 
> This is a lie :)  V8 has been already rolled, but probably lags on bbs.

That is a lie. This change does break the webkit build of chromium. :) You need to change the chromium deps file in webkit (WebKit/chromium/DEPS) to point to a newer version of the chromium repository and that will pick up the new revision of v8.

[David Levin]

Comment on attachment 57330 [details]
Another upload to trigger build bots

Busted build.

[anton muhin]

(In reply to comment #6)
> (From update of attachment 57330 [details])
> Busted build.

David, please, see comment 5 (https://bugs.webkit.org/show_bug.cgi?id=39877#c5): bbs haven't rolled v8 yet, while it's already in Chromium's trunk.

[anton muhin]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 57330 [details] [details])
> > Busted build.
> 
> David, please, see comment 5 (https://bugs.webkit.org/show_bug.cgi?id=39877#c5): bbs haven't rolled v8 yet, while it's already in Chromium's trunk.

Sorry, I pointed out your comment to you which was stupid, sorry again.

[anton muhin]

Created attachment 57340 [details]
With Chromium rolled forward

[anton muhin]

New patch (with Chromium rolled forward) uploaded.  Should I make a separate patch out of this Chromium's rolling?  What's the procedure of Chromium-in-WebKit rolling?

[David Levin]

(In reply to comment #10)
> New patch (with Chromium rolled forward) uploaded.  Should I make a separate patch out of this Chromium's rolling?  What's the procedure of Chromium-in-WebKit rolling?

So far I think we roll it with the change that requires it (which helps their patch to build also!), so this is great.
(In general, I don't think it gets rolled unless someone needs it.)

[anton muhin]

(In reply to comment #11)
> (In reply to comment #10)
> > New patch (with Chromium rolled forward) uploaded.  Should I make a separate patch out of this Chromium's rolling?  What's the procedure of Chromium-in-WebKit rolling?
> 
> So far I think we roll it with the change that requires it (which helps their patch to build also!), so this is great.
> (In general, I don't think it gets rolled unless someone needs it.)

I see, thanks a lot for clarifications.

[Adam Barth]

The EWS bot isn't smart enough to handle changes to DEPS.  It's something we should fix eventually.  Sorry for the inconvenience.

[WebKit Review Bot]

Attachment 57340 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2590045

[anton muhin]

I need your assistance here.

I thought that my change to DEPS is enough as V8 was rolled before this revision (see https://bugs.webkit.org/show_bug.cgi?id=39877#c5).

I doubled checked that this version includes change I depend on.

Was my change to DEPS enough?  Is there a way to find out at which revision v8 on bb is?

[Adam Barth]

From http://trac.webkit.org/browser/trunk/WebKit/chromium/DEPS#L73 it seems we grab the V8 version from the Chromium DEPS file at this rev: http://trac.webkit.org/browser/trunk/WebKit/chromium/DEPS#L35

Maybe we need to bump that number to get the updated V8 number?  (The EWS bot is too dumb to understand that, but Levin is probably smart enough to. :) )

[anton muhin]

(In reply to comment #16)
> From http://trac.webkit.org/browser/trunk/WebKit/chromium/DEPS#L73 it seems we grab the V8 version from the Chromium DEPS file at this rev: http://trac.webkit.org/browser/trunk/WebKit/chromium/DEPS#L35
> 
> Maybe we need to bump that number to get the updated V8 number?  (The EWS bot is too dumb to understand that, but Levin is probably smart enough to. :) )

Which number?  Relevant part apparently looks like:


 72   # v8 javascript engine
 73   'v8': From('chromium_deps', 'src/v8'),
 74 

I could put the right number here, but will it be a correct thing to do?

[Adam Barth]

> Which number?  Relevant part apparently looks like:

The number on this line:

http://trac.webkit.org/browser/trunk/WebKit/chromium/DEPS#L35

[anton muhin]

(In reply to comment #18)
> > Which number?  Relevant part apparently looks like:
> 
> The number on this line:
> 
> http://trac.webkit.org/browser/trunk/WebKit/chromium/DEPS#L35

I think that's exactly the line I change.

[Adam Barth]

Ah, I see.  Yeah, the bot just doesn't understand changes to DEPS.

[David Levin]

Adding Nate as an attempt to get him to review it. (I only wanted to get the DEPS change in here.)

Passing comment:
Maybe this
     if (document->isHTMLDocument())
         ASSERT(V8Document::toNative(v8::Handle<v8::Object>::Cast(wrapper->GetPrototype())) == document);

should be
         ASSERT(!document->isHTMLDocument() || V8Document::toNative(v8::Handle<v8::Object>::Cast(wrapper->GetPrototype())) == document);

to ensure that no code is generated for it in !debug builds.

[Nate Chapin]

Comment on attachment 57340 [details]
With Chromium rolled forward

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.

[anton muhin]

Created attachment 57475 [details]
Addressing comments

[anton muhin]

(In reply to comment #23)
> Created an attachment (id=57475) [details]
> Addressing comments

David and Nate,

thanks a lot for review.  Hopefully, I've addressed all the issues.

And sorry I've uploaded new version with r?---I'd like to have another run though build bots.

If you could quickly check it and r+ it, that'd be most appreciated.  Otherwise I'd probably upload it with reviewed filled and cq+.

[WebKit Review Bot]

Attachment 57475 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2794019

[anton muhin]

Created attachment 57528 [details]
For cq+ with reviewed by

[WebKit Commit Bot]

Comment on attachment 57528 [details]
For cq+ with reviewed by

Clearing flags on attachment: 57528

Committed r60470: <http://trac.webkit.org/changeset/60470>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Reopening since this was rolled out.

[anton muhin]

Created attachment 57780 [details]
To run through build bots another time

[WebKit Commit Bot]

Comment on attachment 57780 [details]
To run through build bots another time

Clearing flags on attachment: 57780

Committed r60670: <http://trac.webkit.org/changeset/60670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/60670 might have broken GTK Linux 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/60669
http://trac.webkit.org/changeset/60670