Bug 31913 – [Android] Upstream WebCore/history/android: Require some platform specific state attached to HistoryItem.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ben Murdoch]

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.

[Ben Murdoch]

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)]

I wonder how the iPhone does this.  Or if the iPhone even bothers to hold current zoom when going between history items.

[Adam Barth]

style-queue ran check-webkit-style on attachment 43925 [details] without any errors.

[Brady Eidson]

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]

(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 on attachment 43925 [details]
Patch

cq- for manual landing.

[Ben Murdoch]

(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]

Landed as r51628.