Bug 39877

Summary: [Chromium] get rid of named interceptor on HTMLDocument and introduce/remove accessors when named items get deleted/removed
Product: WebKit Reporter: anton muhin <antonm>
Component: New BugsAssignee: anton muhin <antonm>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dglazkov, eric, japhet, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 39990    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Another upload to trigger build bots
none
With Chromium rolled forward
none
Addressing comments
none
For cq+ with reviewed by
none
To run through build bots another time none

Description anton muhin 2010-05-28 06:57:35 PDT
[Chromium] get rid of named interceptor on HTMLDocument and introduce/remove accessors when named items get deleted/removed
Comment 1 anton muhin 2010-05-28 07:07:27 PDT
Created attachment 57326 [details]
Patch
Comment 2 anton muhin 2010-05-28 07:41:49 PDT
Created attachment 57330 [details]
Another upload to trigger build bots
Comment 3 WebKit Review Bot 2010-05-28 08:34:35 PDT
Attachment 57330 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2600031
Comment 4 anton muhin 2010-05-28 08:43:32 PDT
(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.
Comment 5 David Levin 2010-05-28 10:18:17 PDT
(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.
Comment 6 David Levin 2010-05-28 10:18:34 PDT
Comment on attachment 57330 [details]
Another upload to trigger build bots

Busted build.
Comment 7 anton muhin 2010-05-28 10:21:59 PDT
(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.
Comment 8 anton muhin 2010-05-28 10:23:06 PDT
(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.
Comment 9 anton muhin 2010-05-28 10:25:44 PDT
Created attachment 57340 [details]
With Chromium rolled forward
Comment 10 anton muhin 2010-05-28 10:26:51 PDT
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?
Comment 11 David Levin 2010-05-28 10:28:59 PDT
(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.)
Comment 12 anton muhin 2010-05-28 10:30:32 PDT
(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.
Comment 13 Adam Barth 2010-05-28 10:49:52 PDT
The EWS bot isn't smart enough to handle changes to DEPS.  It's something we should fix eventually.  Sorry for the inconvenience.
Comment 14 WebKit Review Bot 2010-05-28 11:08:00 PDT
Attachment 57340 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2590045
Comment 15 anton muhin 2010-05-28 11:29:00 PDT
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?
Comment 16 Adam Barth 2010-05-28 11:33:13 PDT
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. :) )
Comment 17 anton muhin 2010-05-28 11:35:38 PDT
(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?
Comment 18 Adam Barth 2010-05-28 11:46:22 PDT
> Which number?  Relevant part apparently looks like:

The number on this line:

http://trac.webkit.org/browser/trunk/WebKit/chromium/DEPS#L35
Comment 19 anton muhin 2010-05-28 11:49:13 PDT
(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.
Comment 20 Adam Barth 2010-05-28 11:55:53 PDT
Ah, I see.  Yeah, the bot just doesn't understand changes to DEPS.
Comment 21 David Levin 2010-05-28 12:21:25 PDT
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.
Comment 22 Nate Chapin 2010-05-28 12:40:56 PDT
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.
Comment 23 anton muhin 2010-05-31 09:08:50 PDT
Created attachment 57475 [details]
Addressing comments
Comment 24 anton muhin 2010-05-31 09:11:33 PDT
(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+.
Comment 25 WebKit Review Bot 2010-05-31 09:30:45 PDT
Attachment 57475 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2794019
Comment 26 anton muhin 2010-06-01 04:10:13 PDT
Created attachment 57528 [details]
For cq+ with reviewed by
Comment 27 WebKit Commit Bot 2010-06-01 04:31:06 PDT
Comment on attachment 57528 [details]
For cq+ with reviewed by

Clearing flags on attachment: 57528

Committed r60470: <http://trac.webkit.org/changeset/60470>
Comment 28 WebKit Commit Bot 2010-06-01 04:31:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Eric Seidel (no email) 2010-06-01 11:26:43 PDT
Reopening since this was rolled out.
Comment 30 anton muhin 2010-06-03 09:31:43 PDT
Created attachment 57780 [details]
To run through build bots another time
Comment 31 WebKit Commit Bot 2010-06-04 02:58:18 PDT
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>
Comment 32 WebKit Commit Bot 2010-06-04 02:58:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 WebKit Review Bot 2010-06-04 03:22:50 PDT
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