[webkit-reviews] review denied: [Bug 29904] WebKit Mac API should provide a delegate interface for global history (7042773) : [Attachment 40334] Proposed fix, including some basic layouttests

[bugzilla-daemon at webkit.org]

John Sullivan <sullivan at apple.com> has denied Brady Eidson
<beidson at apple.com>'s request for review:
Bug 29904: WebKit Mac API should provide a delegate interface for global
history (7042773)
https://bugs.webkit.org/show_bug.cgi?id=29904

Attachment 40334: Proposed fix, including some basic layouttests
https://bugs.webkit.org/attachment.cgi?id=40334&action=review

------- Additional Comments from John Sullivan <sullivan at apple.com>
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.