Bug 24949 – Back out some of the changes to FrameLoader in r42055 to unbreak Chrome.

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

[Jeremy Moskovich]

r42055 removed setCurrentHistoryItem() and etProvisionalHistoryItem() from FrameLoader and made loadURL and closeURL private.

Chrome uses these functions, the attached patch is to back out those parts of the CL.

[Jeremy Moskovich]

Created attachment 29093 [details]
patch1

[Darin Adler]

Comment on attachment 29093 [details]
patch1

Where is the Chromium code that's making these calls? I did a grep on the entire WebCore and WebKit tree and didn't find it.

I'm almost certain the Chromium code is broken and needs to be changed, but I can't tell that for sure unless I can see it.

[Darin Fisher (:fishd, Google)]

Hi Darin,

We are using these FrameLoader methods from our WebKit layer, which we have not yet upstreamed.  We are knee deep in cleaning up that layer so that we can upstream it.

We use loadURL for browser driven navigations, and we use closeURL as part of our asynchronous procedure for shutting down a WebView.  (WebView closure is rather complicated in Chrome for good reasons, which I can explain if you like.)

We need setCurrentHistoryItem and setProvisionalHistoryItem to support the way we have implement back/forward.  You see, we don't keep the back/forward list in the same process as WebKit.  You might recall the BackForwardListClient interface from me that you reviewed several months back.  That (as well as these FrameLoader methods) enable us to maintain the back/forward list separately.  So, to navigate back or forward, we instead navigate to a particular HistoryItem.  Getting that to work properly requires a bit of work.

While we work to upstream all of the Chromium WebKit layer, I hope that you will be supportive of re-exposing these methods on FrameLoader.  I think it would be a lot easier to refactor our code once it lives in svn.webkit.org.  Of course, if there are any obvious alternatives to these methods, I would love to hear about them.

Can we at least land Jeremy's change as a bustage fix?

[Darin Adler]

(In reply to comment #3)
> We use loadURL for browser driven navigations

You should be using loadFrameRequestWithFormAndValues for this.

> we use closeURL as part of our asynchronous procedure for shutting down a WebView.

You may want to use detachFromParent() for this.

> We need setCurrentHistoryItem and setProvisionalHistoryItem to support the way
> we have implement back/forward.

OK. Bad news.

> Can we at least land Jeremy's change as a bustage fix?

All right. We need quite a bit of improvement in FrameLoader, so the sooner you can get the Chromium loading code checked into the WebKit tree, the better. This is just the first step of some major repair. It looks like the Chrome requirements are actively getting in the way of fixing the design here!

[Darin Fisher (:fishd, Google)]

> You should be using loadFrameRequestWithFormAndValues for this.

Whoops.  I "misspoke"... we use loadURL when creating a child frame.  I'm sure it should be easy to replace this usage then.

> > we use closeURL as part of our asynchronous procedure for shutting down a WebView.
> 
> You may want to use detachFromParent() for this.

Hmm, I'm not sure that that will work as is...  unfortunately, we need just the first part which runs unload handlers.


> All right.

Thank you!


> We need quite a bit of improvement in FrameLoader, so the sooner you
> can get the Chromium loading code checked into the WebKit tree, the better.
> This is just the first step of some major repair. It looks like the Chrome
> requirements are actively getting in the way of fixing the design here!

Yeah, I really don't like holding up progress on improving FrameLoader.  It is an ugly beast.  I'd be great if we could discuss further the funny requirements Chromium has on navigation.

[Darin Fisher (:fishd, Google)]

It looks like we need to apply a change like http://trac.webkit.org/changeset/42104 to the Chromium WebKit layer in order to eliminate our dependency on FrameLoader::loadURL and setProvisionalHistoryItem.

I'm afraid we still need setCurrentHistoryItem and closeURL for now.

[Jeremy Moskovich]

Created attachment 29120 [details]
patch2

Per-Darin's comment, we still need closeURL() and setCurrentHistoryItem()

[Darin Fisher (:fishd, Google)]

Comment on attachment 29120 [details]
patch2

LGTM

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/42130