Bug 40430 - [Chromium] Plumbing for top-level frame names
Summary: [Chromium] Plumbing for top-level frame names
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Andrew Wilson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-10 10:48 PDT by Andrew Wilson
Modified: 2010-06-18 22:27 PDT (History)
7 users (show)

See Also:


Attachments
Patch containing plumbing for passing frame names to Chromium (3.74 KB, patch)
2010-06-10 11:41 PDT, Andrew Wilson
levin: commit-queue-
Details | Formatted Diff | Diff
Updated chromium DRT to match new createView() API (6.16 KB, patch)
2010-06-12 15:03 PDT, Andrew Wilson
no flags Details | Formatted Diff | Diff
Patch that removes clearName() (3.61 KB, patch)
2010-06-17 17:12 PDT, Andrew Wilson
levin: review-
Details | Formatted Diff | Diff
Reuploaded patch so it can go through EWS again after DEPS roll. (3.53 KB, patch)
2010-06-18 10:27 PDT, Andrew Wilson
fishd: review+
fishd: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Wilson 2010-06-10 10:48:20 PDT
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
Comment 1 Andrew Wilson 2010-06-10 11:41:30 PDT
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 2 David Levin 2010-06-10 21:27:14 PDT
Comment on attachment 58395 [details]
Patch containing plumbing for passing frame names to Chromium

Marking cq- per Drew's comment in bug.
Comment 3 Andrew Wilson 2010-06-12 09:04:36 PDT
The downstream patch has landed - just waiting for anyone to r+ this patch now.
Comment 4 Andrew Wilson 2010-06-12 15:03:40 PDT
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 5 Kent Tamura 2010-06-12 19:09:28 PDT
Comment on attachment 58567 [details]
Updated chromium DRT to match new createView() API

OK.
Comment 6 Darin Fisher (:fishd, Google) 2010-06-16 13:18:20 PDT
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 7 WebKit Commit Bot 2010-06-16 13:29:08 PDT
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>
Comment 8 WebKit Commit Bot 2010-06-16 13:29:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Andrew Wilson 2010-06-16 14:45:55 PDT
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.
Comment 10 Darin Fisher (:fishd, Google) 2010-06-17 09:56:25 PDT
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?
Comment 11 Andrew Wilson 2010-06-17 11:06:00 PDT
That's what I'm doing. I have the chrome-side change out in the trybots now.
Comment 12 Andrew Wilson 2010-06-17 17:12:58 PDT
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.
Comment 13 WebKit Review Bot 2010-06-17 18:15:04 PDT
Attachment 59051 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3299308
Comment 14 Andrew Wilson 2010-06-17 18:18:54 PDT
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 15 David Levin 2010-06-17 21:27:27 PDT
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.
Comment 16 Andrew Wilson 2010-06-17 22:38:20 PDT
(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?
Comment 17 David Levin 2010-06-18 00:03:10 PDT
(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.
Comment 18 Andrew Wilson 2010-06-18 10:27:54 PDT
Created attachment 59131 [details]
Reuploaded patch so it can go through EWS again after DEPS roll.
Comment 19 Darin Fisher (:fishd, Google) 2010-06-18 13:34:28 PDT
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
Comment 20 Andrew Wilson 2010-06-18 17:16:18 PDT
Landed as r61458
Comment 21 WebKit Review Bot 2010-06-18 22:27:23 PDT
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