Bug 29904 - WebKit Mac API should provide a delegate interface for global history (7042773)
Summary: WebKit Mac API should provide a delegate interface for global history (7042773)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-09-29 16:11 PDT by Brady Eidson
Modified: 2009-10-08 20:39 PDT (History)
1 user (show)

See Also:


Attachments
Proposed fix, including some basic layouttests (66.09 KB, patch)
2009-09-29 16:59 PDT, Brady Eidson
sullivan: review-
beidson: commit-queue-
Details | Formatted Diff | Diff
Updated patch (63.33 KB, patch)
2009-09-29 17:55 PDT, Brady Eidson
sullivan: review+
beidson: commit-queue-
Details | Formatted Diff | Diff
Also add the ability to catch title changes (7.39 KB, patch)
2009-10-07 16:56 PDT, Brady Eidson
darin: review+
beidson: commit-queue-
Details | Formatted Diff | Diff
Add "populate visited links" callback, as well as ability to clear all visited links. (15.93 KB, patch)
2009-10-08 20:08 PDT, Brady Eidson
beidson: commit-queue-
Details | Formatted Diff | Diff
Updated patch that removes possible inter-test dependency issues in DRT (Thanks Mitz!) (17.43 KB, patch)
2009-10-08 20:35 PDT, Brady Eidson
mitz: review+
beidson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2009-09-29 16:11:42 PDT
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>
Comment 1 Brady Eidson 2009-09-29 16:59:08 PDT
Created attachment 40334 [details]
Proposed fix, including some basic layouttests
Comment 2 John Sullivan 2009-09-29 17:43:39 PDT
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.
Comment 3 Brady Eidson 2009-09-29 17:55:52 PDT
Created attachment 40338 [details]
Updated patch
Comment 4 John Sullivan 2009-09-29 17:58:58 PDT
Comment on attachment 40338 [details]
Updated patch

Looks good.
Comment 5 Brady Eidson 2009-09-29 18:37:18 PDT
Committed revision 48914.

c.webkit.org/changeset/48914
Comment 6 Brady Eidson 2009-09-29 18:38:33 PDT
Sorry -
http://trac.webkit.org/changeset/48914
Comment 7 Brady Eidson 2009-09-29 18:45:56 PDT
LayoutTestController changes landed in http://trac.webkit.org/changeset/48915
Comment 8 Brady Eidson 2009-10-07 16:26:43 PDT
I missed the setTitle call.  Reopening as including that is logically part of the same task here.
Comment 9 Brady Eidson 2009-10-07 16:56:16 PDT
Created attachment 40829 [details]
Also add the ability to catch title changes
Comment 10 Darin Adler 2009-10-07 16:57:30 PDT
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
Comment 11 Brady Eidson 2009-10-07 17:05:49 PDT
http://trac.webkit.org/changeset/49277
Comment 12 Brady Eidson 2009-10-08 17:53:07 PDT
Need an interface for the history delegate to populate and clear visited links.
Comment 13 Brady Eidson 2009-10-08 20:08:11 PDT
Created attachment 40926 [details]
Add "populate visited links" callback, as well as ability to clear all visited links.
Comment 14 Brady Eidson 2009-10-08 20:35:27 PDT
Created attachment 40927 [details]
Updated patch that removes possible inter-test dependency issues in DRT (Thanks Mitz!)
Comment 15 mitz 2009-10-08 20:38:10 PDT
Comment on attachment 40927 [details]
Updated patch that removes possible inter-test dependency issues in DRT (Thanks Mitz!)

r=me
Comment 16 Brady Eidson 2009-10-08 20:39:55 PDT
http://trac.webkit.org/changeset/49369