Bug 40430 – [Chromium] Plumbing for top-level frame names

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Andrew Wilson]

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

[Andrew Wilson]

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.

[David Levin]

Comment on attachment 58395 [details]
Patch containing plumbing for passing frame names to Chromium

Marking cq- per Drew's comment in bug.

[Andrew Wilson]

The downstream patch has landed - just waiting for anyone to r+ this patch now.

[Andrew Wilson]

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).

[Kent Tamura]

Comment on attachment 58567 [details]
Updated chromium DRT to match new createView() API

OK.

[Darin Fisher (:fishd, Google)]

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.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andrew Wilson]

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.

[Darin Fisher (:fishd, Google)]

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?

[Andrew Wilson]

That's what I'm doing. I have the chrome-side change out in the trybots now.

[Andrew Wilson]

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.

[WebKit Review Bot]

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

[Andrew Wilson]

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

[David Levin]

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.

[Andrew Wilson]

(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?

[David Levin]

(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.

[Andrew Wilson]

Created attachment 59131 [details]
Reuploaded patch so it can go through EWS again after DEPS roll.

[Darin Fisher (:fishd, Google)]

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

[Andrew Wilson]

Landed as r61458

[WebKit Review Bot]

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