Bug 24949 - Back out some of the changes to FrameLoader in r42055 to unbreak Chrome.
: Back out some of the changes to FrameLoader in r42055 to unbreak Chrome.
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-03-30 16:13 PST by
Modified: 2009-03-31 09:21 PST (History)


Attachments
patch1 (5.88 KB, patch)
2009-03-30 16:16 PST, Jeremy Moskovich
no flags Review Patch | Details | Formatted Diff | Diff
patch2 (3.07 KB, patch)
2009-03-31 09:17 PST, Jeremy Moskovich
fishd: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-03-30 16:13:16 PST
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.
------- Comment #1 From 2009-03-30 16:16:34 PST -------
Created an attachment (id=29093) [details]
patch1
------- Comment #2 From 2009-03-30 16:25:35 PST -------
(From update of attachment 29093 [details])
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.
------- Comment #3 From 2009-03-30 16:47:03 PST -------
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?
------- Comment #4 From 2009-03-30 16:52:05 PST -------
(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!
------- Comment #5 From 2009-03-30 17:32:42 PST -------
> 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.
------- Comment #6 From 2009-03-31 09:11:50 PST -------
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.
------- Comment #7 From 2009-03-31 09:17:06 PST -------
Created an attachment (id=29120) [details]
patch2

Per-Darin's comment, we still need closeURL() and setCurrentHistoryItem()
------- Comment #8 From 2009-03-31 09:19:19 PST -------
(From update of attachment 29120 [details])
LGTM
------- Comment #9 From 2009-03-31 09:21:38 PST -------
Landed as:  http://trac.webkit.org/changeset/42130