Bug 31913

Summary: [Android] Upstream WebCore/history/android: Require some platform specific state attached to HistoryItem.
Product: WebKit Reporter: Ben Murdoch <benm>
Component: WebCore Misc.Assignee: Ben Murdoch <benm>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, android-webkit-unforking, beidson, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Android   
Bug Depends on:    
Bug Blocks: 31867    
Attachments:
Description Flags
Patch beidson: review+, benm: commit-queue-

Ben Murdoch
Reported 2009-11-26 08:16:26 PST
Master bug for upstreaming Android history changes: https://bugs.webkit.org/show_bug.cgi?id=31867 As is the case with the Mac and Qt ports, Android requires some state stored with each WebCore::HistoryItem (for example, the zoom scale of the page). Patch to follow.
Attachments
Patch (6.92 KB, patch)
2009-11-26 09:58 PST, Ben Murdoch
beidson: review+
benm: commit-queue-
Ben Murdoch
Comment 1 2009-11-26 09:58:33 PST
Created attachment 43925 [details] Patch This patch adds Android specific member data to HistoryItem.h and implements Android specific functionality in new files added to the history/android directory.
Eric Seidel (no email)
Comment 2 2009-11-29 07:44:38 PST
I wonder how the iPhone does this. Or if the iPhone even bothers to hold current zoom when going between history items.
Adam Barth
Comment 3 2009-11-30 12:45:55 PST
style-queue ran check-webkit-style on attachment 43925 [details] without any errors.
Brady Eidson
Comment 4 2009-12-02 11:29:45 PST
Comment on attachment 43925 [details] Patch > Index: WebCore/history/HistoryItem.h > =================================================================== > +#if PLATFORM(ANDROID) > +#include "AndroidWebHistoryBridge.h" > +#endif > + > namespace WebCore { Since nothing in the header needs the full class declaration, this should be a forward declaration instead of an #include I really hate the name "bridge." It implies a completely different pattern than what I think you're going for here. We used to call the platform/API glue interface between WebCore and WebKit the "bridge". Your bridge object seems to simply be a container for platform specific data. Can't you call it something else that isn't as ambiguous?
Ben Murdoch
Comment 5 2009-12-02 11:46:48 PST
(In reply to comment #4) > (From update of attachment 43925 [details]) > > > Index: WebCore/history/HistoryItem.h > > =================================================================== > > > +#if PLATFORM(ANDROID) > > +#include "AndroidWebHistoryBridge.h" > > +#endif > > + > > namespace WebCore { > > Since nothing in the header needs the full class declaration, this should be a > forward declaration instead of an #include Good point, thanks. I'll fix this on landing. > > I really hate the name "bridge." It implies a completely different pattern > than what I think you're going for here. We used to call the platform/API glue > interface between WebCore and WebKit the "bridge". Your bridge object seems to > simply be a container for platform specific data. Can't you call it something > else that isn't as ambiguous? Well, it's both... it holds platform specific data and the implementation of AndroidWebHistoryBridge::updateHistoryItem() does live in WebKit, so it's also a bridge connecting WebCore and WebKit?
Ben Murdoch
Comment 6 2009-12-02 13:01:32 PST
Comment on attachment 43925 [details] Patch cq- for manual landing.
Ben Murdoch
Comment 7 2009-12-03 03:37:59 PST
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 43925 [details] [details]) > > > > > Index: WebCore/history/HistoryItem.h > > > =================================================================== > > > > > +#if PLATFORM(ANDROID) > > > +#include "AndroidWebHistoryBridge.h" > > > +#endif > > > + > > > namespace WebCore { > > > > Since nothing in the header needs the full class declaration, this should be a > > forward declaration instead of an #include > > Good point, thanks. I'll fix this on landing. > Actually, we do need the include, as the new member for Android is a RefPtr.
Ben Murdoch
Comment 8 2009-12-03 04:18:35 PST
Landed as r51628.
Note You need to log in before you can comment on or make changes to this bug.