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 Bugs | Assignee: | 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
anton muhin
2010-05-28 06:57:35 PDT
Created attachment 57326 [details]
Patch
Created attachment 57330 [details]
Another upload to trigger build bots
Attachment 57330 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2600031 (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. (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 on attachment 57330 [details]
Another upload to trigger build bots
Busted build.
(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. (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. Created attachment 57340 [details]
With Chromium rolled forward
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? (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.) (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. The EWS bot isn't smart enough to handle changes to DEPS. It's something we should fix eventually. Sorry for the inconvenience. Attachment 57340 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2590045 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? 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. :) ) (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? > Which number? Relevant part apparently looks like: The number on this line: http://trac.webkit.org/browser/trunk/WebKit/chromium/DEPS#L35 (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. Ah, I see. Yeah, the bot just doesn't understand changes to DEPS. 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 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. Created attachment 57475 [details]
Addressing comments
(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+. Attachment 57475 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2794019 Created attachment 57528 [details]
For cq+ with reviewed by
Comment on attachment 57528 [details] For cq+ with reviewed by Clearing flags on attachment: 57528 Committed r60470: <http://trac.webkit.org/changeset/60470> All reviewed patches have been landed. Closing bug. Reopening since this was rolled out. Created attachment 57780 [details]
To run through build bots another time
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> All reviewed patches have been landed. Closing bug. 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 |