Bug 29904 – WebKit Mac API should provide a delegate interface for global history (7042773)

• Component: History
• Status: RESOLVED FIXED

[Brady Eidson]

Managing the Global History store is something that WebKit client's should be able to do if they choose.

WebKit Mac API should provide a delegate interface for global history, using it instead of the built-in WebHistory if the interface is implemented.

In radar as <rdar://problem/7042773>

[Brady Eidson]

Created attachment 40334 [details]
Proposed fix, including some basic layouttests

[John Sullivan]

Comment on attachment 40334 [details]
Proposed fix, including some basic layouttests

Since WebNavigationData is intended to be a public class eventually, I think the header should be named WebNavigationData.h rather than WebNavigationDataPrivate.h. This would also eliminate the ambiguity about whether WebNavigationDataPrivate.h is a private header file for WebNavigationData, or a header file for WebNavigationDataPrivate.

Same thing with WebHistoryDelegatePrivate.h.

I think the long-ass init method for WebNavigationData should be declared in the "eventually public" header so it is possible for clients to create these objects, even if they are typically created only by the framework. This is similar to NSEvent, which has public long-ass convenience constructors though most clients never create one. (There's even a comment in NSEvent.h that says "apps will rarely create these objects".)

Perhaps the long-ass init method should be replaced by a long-ass convenience constructor instead, to follow even more closely the NSEvent pattern. I don't really have an opinion one way or another. If you only make the convenience constructor public, then clients can't make one of these things that isn't autoreleased, but that's probably fine for a (relatively) rarely created object like this. Perhaps someone else feels strongly about this (maybe Darin?).

Maybe webView:didNavigateWithNavigationData:forFrame: should be webView:didNavigateWithNavigationData:inFrame:? It seems like the navigation is "in" a frame rather than "for" a frame.

"didNavigateWithNavigationData" is an awkward phrase, though I think it's unambiguous. I don't have a better suggestion.

The methods in the eventually-public header files will need comments, eventually.

The rest looks fine to me. Love the layout tests. I'm giving this an r- only for the header filenames and to get the init method declared in the header file.

[Brady Eidson]

Created attachment 40338 [details]
Updated patch

[John Sullivan]

Comment on attachment 40338 [details]
Updated patch

Looks good.

[Brady Eidson]

Committed revision 48914.

c.webkit.org/changeset/48914

[Brady Eidson]

Sorry -
http://trac.webkit.org/changeset/48914

[Brady Eidson]

LayoutTestController changes landed in http://trac.webkit.org/changeset/48915

[Brady Eidson]

I missed the setTitle call.  Reopening as including that is logically part of the same task here.

[Brady Eidson]

Created attachment 40829 [details]
Also add the ability to catch title changes

[Darin Adler]

Comment on attachment 40829 [details]
Also add the ability to catch title changes

> +        if (implementations->setTitleFunc)
> +            CallHistoryDelegate(implementations->setTitleFunc, view, @selector(webView:updateHistoryTitle:forURL:), (NSString *)title, (NSString *)url);
> +    
> +        return;

I'd probably do early return here instead of nesting the call inside an if.

r=me

[Brady Eidson]

http://trac.webkit.org/changeset/49277

[Brady Eidson]

Need an interface for the history delegate to populate and clear visited links.

[Brady Eidson]

Created attachment 40926 [details]
Add "populate visited links" callback, as well as ability to clear all visited links.

[Brady Eidson]

Created attachment 40927 [details]
Updated patch that removes possible inter-test dependency issues in DRT (Thanks Mitz!)

Comment on attachment 40927 [details]
Updated patch that removes possible inter-test dependency issues in DRT (Thanks Mitz!)

r=me

[Brady Eidson]

http://trac.webkit.org/changeset/49369