We need to enable Chromium to set the name of frames that it creates, and also to have the name of top level frames passed along when creating a view. This is the upstream portion of http://codereview.chromium.org/2775003
Created attachment 58395 [details] Patch containing plumbing for passing frame names to Chromium This is dependent on this downstream patch: http://codereview.chromium.org/2775003 so I won't land this until after that patch lands.
Comment on attachment 58395 [details] Patch containing plumbing for passing frame names to Chromium Marking cq- per Drew's comment in bug.
The downstream patch has landed - just waiting for anyone to r+ this patch now.
Created attachment 58567 [details] Updated chromium DRT to match new createView() API Realized that there was one more place in the code that needed to change to match the new createView() API (Chromium DRT).
Comment on attachment 58567 [details] Updated chromium DRT to match new createView() API OK.
Comment on attachment 58567 [details] Updated chromium DRT to match new createView() API WebKit/chromium/public/WebFrame.h:106 + virtual void setName(const WebString&) = 0; What's the difference between clearName and setName(WebString())? Looking at the code, setName(WebString()) will assign a unique name to the frame. Is that what you want this API to do? It seems like you should document this behavior.
Comment on attachment 58567 [details] Updated chromium DRT to match new createView() API Clearing flags on attachment: 58567 Committed r61278: <http://trac.webkit.org/changeset/61278>
All reviewed patches have been landed. Closing bug.
Reopening bug so I can address Darin's comments. clearName() is currently used in exactly one place - as part of TestShell::ResetBeforeTestRun(), to reset the frame state between tests. I am going to confirm the difference between clearName() and setName(WebString()) - I think the behavior is identical for top-level frames, but I'm not 100% sure - and I'll document the exact behavior.
atwilson, if we only need clearName to clear the top-level frame name, and if setName(WebString()) would be equivalent in that case, then can you also eliminate the clearName method?
That's what I'm doing. I have the chrome-side change out in the trybots now.
Created attachment 59051 [details] Patch that removes clearName() Darin, I've removed clearName() as we discussed and added documentation about setName()'s behavior. Please take a look when you get a chance.
Attachment 59051 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3299308
BTW, I can only assume the chromium build failure was spurious, since the code that generated the error was changed in this CL: http://src.chromium.org/viewvc/chrome?view=rev&revision=50171
Comment on attachment 59051 [details] Patch that removes clearName() The build failure is real. You need to change 'chromium_rev' in WebKit/chromium/DEPS to avoid it. If you do it in this patch, the build will still fail because ews doesn't do another update-webkit --chromium (aka gclient sync) after applying the patch, so it is nice to do the DEPS roll as a separate patch to ensure that your patch builds correctly.
(In reply to comment #15) > (From update of attachment 59051 [details]) > The build failure is real. > > You need to change 'chromium_rev' in WebKit/chromium/DEPS to avoid it. OK, thanks, I didn't know that. I'll submit a separate patch for that DEPS roll. Is there a way to force EWS to retry this patch, or do I just need to re-upload this same patch again after rolling DEPS?
(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 59051 [details] [details]) > > The build failure is real. > > > > You need to change 'chromium_rev' in WebKit/chromium/DEPS to avoid it. > > OK, thanks, I didn't know that. > > I'll submit a separate patch for that DEPS roll. Is there a way to force EWS to retry this patch, or do I just need to re-upload this same patch again after rolling DEPS? I don't know (ask Eric or Adam in irc?). I would guess that you need to upload a new patch.
Created attachment 59131 [details] Reuploaded patch so it can go through EWS again after DEPS roll.
Comment on attachment 59131 [details] Reuploaded patch so it can go through EWS again after DEPS roll. WebKit/chromium/public/WebFrame.h:108 + // frame name unique within the hierarchy (see FrameTree::uniqueChildName() nit: let's avoid references to WebCore implementation details in the WebKit API headers. I'm concerned that these comments will not be maintained if someone renames WebCore types. otherwise, LGTM
Landed as r61458
http://trac.webkit.org/changeset/61458 might have broken Leopard Intel Debug (Tests) The following changes are on the blame list: http://trac.webkit.org/changeset/61458 http://trac.webkit.org/changeset/61459