Bug 31913 - [Android] Upstream WebCore/history/android: Require some platform specific state attached to HistoryItem.
Summary: [Android] Upstream WebCore/history/android: Require some platform specific st...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Ben Murdoch
URL:
Keywords:
Depends on:
Blocks: 31867
  Show dependency treegraph
 
Reported: 2009-11-26 08:16 PST by Ben Murdoch
Modified: 2009-12-03 04:18 PST (History)
4 users (show)

See Also:


Attachments
Patch (6.92 KB, patch)
2009-11-26 09:58 PST, Ben Murdoch
beidson: review+
benm: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Murdoch 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.
Comment 1 Ben Murdoch 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.
Comment 2 Eric Seidel (no email) 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.
Comment 3 Adam Barth 2009-11-30 12:45:55 PST
style-queue ran check-webkit-style on attachment 43925 [details] without any errors.
Comment 4 Brady Eidson 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?
Comment 5 Ben Murdoch 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?
Comment 6 Ben Murdoch 2009-12-02 13:01:32 PST
Comment on attachment 43925 [details]
Patch

cq- for manual landing.
Comment 7 Ben Murdoch 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.
Comment 8 Ben Murdoch 2009-12-03 04:18:35 PST
Landed as r51628.