328928 - Revert nsIGlobalHistory2 to rev 1.3, and move flags to nsIGlobalHistory3

Status: RESOLVED FIXED

(Core Graveyard :: History: Global, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I believe that's what we agreed on as the way to go for history.  Then we can freeze nsIGlobalHistory2.

If there are no objections, we should just do this....

Comment 1

[Brett Wilson]

Sounds good

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Changes:

1)  Revert nsIGlobalHistory2.
2)  Add the Gecko flags stuff to nsIGlobalHistory3.
3)  Add a aTopLevel arg to addTopLevelRedirect so its claim that it provides all
    the information that addURI provides is actually true.
4)  Change the setup a tad so implementors of nsIGlobalHistory3 can fall back to
    addURI if they don't want to implement addTopLevelRedirect (saved me from
    having to copy/paste some 20 lines of code in three places).

Thoughts?  Also thoughts on which parts of this (if any) we should land on the 1.8 branch?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 213656 [details] [diff] [review]
Proposed patch

Simon, could you OK the camino changes?

Comment 4

[Brett Wilson]

Comment on attachment 213656 [details] [diff] [review]
Proposed patch

Why did you add a toplevel flag to the redirect function? I once stepped through IsFrame when I was debugging this feature initially, and found it to be quite surprisingly involved. So I question whether it's worthwhile to include this for purely theoretical future users.

Comment 5

[Brett Wilson]

It would be better for me if this was also landed on the branch. Otherwise we'll need some #ifdefs in the places code.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Why did you add a toplevel flag to the redirect function?

Because otherwise the claim that it provides all the functionality addURI provides is false.  I could just remove that claim and document that consumers who need both sets of functionality are just screwed, but this seemed like a better approach.  ;)  Note that the aTopLevel arg _is_ used in the Seamonkey history impl; non-toplevel redirects are not treated the same way as toplevel redirects.

Btw, I found the "topLevelRedirect" part of the function name pretty confusing, since it does get called for subframes... Perhaps "addDocumentRedirect" would be clearer?

Also, I'm still not sure whether we want the change to that function on the 1.8 branch, at least partly because I'm not sure how much a problem the forkage is for places.  Let me know, ok?

Also, given that we _do_ have a "toplevel" arg on addURI, there were probably non-theoretical users in the past, no?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er, the last part of that comment was a mid-air victim.  :(  Ignore it.

I'll produce a branch patch once we agree what the changes on trunk are.  Note that branch will not have the same uuid as trunk, since trunk will have extra methods...

Comment 8

[Darin Fisher]

Comment on attachment 213656 [details] [diff] [review]
Proposed patch

>Index: docshell/base/nsIGlobalHistory3.idl

>+   */
>+  unsigned long getURIGeckoFlags(in nsIURI aURI);
>+  /**

A newline here might be nice.


>Index: layout/generic/nsGfxScrollFrame.cpp

>+  nsCOMPtr<nsIGlobalHistory3> history(do_GetService(NS_GLOBALHISTORY2_CONTRACTID));
>   if (!history)
>     return NS_ERROR_FAILURE;
>   
>   PRUint32 flags;
>   nsresult rv = history->GetURIGeckoFlags(uri, &flags);

This should be changed to use the annotation service ;-)


sr=darin

Comment 9

[Brett Wilson]

(In reply to comment #8)
> This should be changed to use the annotation service ;-)

I will probably implement this in nsNavHistory as a call to the annotation service. This gives us some flexibility to change it later since the annotation service might be slow.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> This should be changed to use the annotation service ;-)

There's no annotation service that Gecko knows about, so... ;)

Brett, thoughts on the naming of the addURI method?  See comment 6?

Comment 11

[Darin Fisher]

> There's no annotation service that Gecko knows about, so... ;)

but maybe there should be ;-)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Possibly.  The fact that we're storing this information in global history is really somewhat of an implementation detail...   We could do that, but the current annotation service api doesn't seem to be ready enough yet (eg using 0 for expiration would so not get us the effect we want).

Comment 13

[Darin Fisher]

> Possibly.  The fact that we're storing this information in global history is
> really somewhat of an implementation detail...   We could do that, but the
> current annotation service api doesn't seem to be ready enough yet (eg using 0
> for expiration would so not get us the effect we want).

I'd propose that we create a Gecko-defined interface for URI annotations similar to the Gecko-defined interface for Global History.  This interface could be implemented by the embedder.

Comment 14

[Brett Wilson]

The current name sucks and I don't mind it being changed at all. I was trying to differentiate it from image redirects and stuff.

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK, so should we just define this Gecko API in this bug?  Or file a new one?

Comment 16

[Brett Wilson]

(In reply to comment #15)
> OK, so should we just define this Gecko API in this bug?  Or file a new one?

You mean for renaming the function or something else? I think doing it all at once here is fine.

Comment 17

[Simon Fraser [no longer active]]

Comment on attachment 213656 [details] [diff] [review]
Proposed patch

Camino stuff looks good.

Comment 18

[Boris Zbarsky [:bzbarsky]]

I mean for Darin's suggestion of moving this off of the history APIs altogether and having a Gecko annotation api.  It seems like new bug fodder to me, frankly, unless we have such an API designed an ready to go.  As in, I don't think I'd be able to develop one in the near future, and I want to make sure we don't ship the nsIGlobalHistory2 changes in 1.9.

Comment 19

[Darin Fisher]

yeah, i was thinking in terms of a new bug that wouldn't block this one.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

I renamed the method to addDocumentRedirect.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 branch patch

Comment 23

[Boris Zbarsky [:bzbarsky]]

"fixed" on the 1.8 branch.