nsIHTMLContent has a GetBaseURL thing that uses magic attributes in HTML... nsIXMLContent has something similar that uses xml:base. Plus there is the document base URI. Most callers don't do a very good job of looking at all of these when they need a base URI, nor should they have to. Imo, nsIContent should just have a GetBaseURI() function that does the right thing depending on the type of content, etc.

I'll drink to that

I think plugins may needs this too. OBJECT tag has a CODEBASE attribute plus calls to NPP_Get/Post/URL need to take the document's base into account.

Peter, do you think that <object> should have its own impl of GetBaseURI that takes the codebase into account? Are there times when URIs (other than codebase itself) would need to be resolved relative to the document base or xml:base for an <object> rather than relative to the codebase?

yes, to both questions. There are times when codebase shouldn't be trusted and those are based on what kind of content the tag will host. For example, codebase is important for Java but meaningless for Active-X controls.

Ok. But for any given object, all uris should be resolved either relative to codebase or not, correct? What I'd like to avoid is creating a GetBaseURI that takes codebase into account for Java, then discovering that someone needs to load something off a Java <object> from a URL _not_ relative to codebase... because there will be no way to do it.

This has some junk from bug 166996 mixed in -- I'm touching the same exact code and there is really no way I can separate the two patches out from each other... I'll be posting a cleaned-up patch once bug 166996 is checked in.

Comment on attachment 126726 [details] [diff] [review] Patch Chris, if you have time to look at this in this state, that would be great. If not, bug 166996 is clearly a bigger priority for me...

This is merged to tip (post-checkin-of-166996). There's a change in editor which is not, strictly speaking, related to this bug, but it was near a GetBaseURL caller and the code was ugly....

Comment on attachment 126827 [details] [diff] [review] Better patch - In nsIContent.h: + /** + * Get the base URL for any relative URLs within this piece + * of content. Generally, this is the document's base URL, + * but certain content carries a local base for backward + * compatibility. There's more than backwards compatibility that plays in here, especially xml:base, so you might want to mention that here too. - In nsGenericElement::GetBaseURL(): + rv = securityManager->CheckLoadURI(parentBase, ourBase, + nsIScriptSecurityManager::STANDARD); I realize you didn't make this up, but this seems like bad design to me. IMO there shouldn't be any reason to do a security check when requesting the base URI of anything, the security check should be done either when setting the base URI, or when using the base URI, not when requesting it. Thoughts? - In nsXULElement::GetBaseURL(): + // XXX TODO, should share the impl with nsGenericElement Wanna create a static method somewhere (nsGenericElement, nsContentUtils, ...) where the code for this could live and make the appropriate classes call that function? sr=jst

> There's more than backwards compatibility that plays in here Indeed. Will update. > IMO there shouldn't be any reason to do a security check when requesting the > base URI of anything, the security check should be done either when setting > the base URI, or when using the base URI, not when requesting it. Thoughts? Well... There is no way to do it when "setting", since xml:base is cumulative from the top of the tree down and thus affected by DOM manipulation. In fact, you could argue that what I do _is_ doing the "setting" check (in the absence of DOM manipulation, it would have exactly the same effect as doing the check while parsing). As for doing the check when using the baseURI, this has two drawbacks: 1) every single caller of GetBaseURL would have to do the check, as far as I can tell, and 2) There is no way to gracefully handle the following markup (served from http://foo/ : <root xml:base="http://bar/"> <kid xml:base="file:///home/"> <html:img src="test.gif" /> </kid> </root> In my opinion, in this scenario the image should be loaded from "http://bar/test.gif" (essentially a security check on the second xml:base being set at all), just like it would be if you had an HTML document at "http://foo/" that had <base href="file:///home/">. However, if the caller of GetBaseURL does the check, their only options are to use "file:///home/" or "http://foo/" for the base -- they have no knowledge of "http://bar/". > Wanna create a static method somewhere If we're sure we have no plans to ever make nsXULElement inherit from nsGenericElement, sure. Otherwise, I'd rather leave this as-is for now...

Note to self -- there is a BaseURL caller in the scriptLoader that should call it off nsIContent, not nsIDocument... If people want, I can post the additional patch for that.

Since review has not happened yet... ;)

Comment on attachment 126960 [details] [diff] [review] Include that, and a cssloader change > NS_IMETHODIMP > nsGenericElement::GetBaseURL(nsIURI** aBaseURL) const > { >- return NS_ERROR_NOT_IMPLEMENTED; >+ nsCOMPtr<nsIDocument> doc; >+ GetDocument(getter_AddRefs(doc)); How about |nsCOMPtr<nsIDocument> doc = mDocument;|? >+ >+ if (!doc && mNodeInfo) { >+ mNodeInfo->GetDocument(getter_AddRefs(doc)); >+ } > nsresult > nsGenericHTMLElement::GetBaseURL(nsIURI** aBaseURL) const > { ... >+ if (mNodeInfo->NamespaceEquals(kNameSpaceID_None)) { >+ if (doc) { >+ return doc->GetBaseURL(aBaseURL); >+ } else { Else after return. >+ *aBaseURL = nsnull; >+ return NS_OK; >+ } >+ } >Index: editor/libeditor/html/nsHTMLEditor.cpp >@@ -5979,20 +5979,17 @@ nsHTMLEditor::ParseStyleAttrIntoCSSRule( > > nsCOMPtr<nsIDocument> doc (do_QueryInterface(domdoc)); > if (!doc) > return NS_ERROR_UNEXPECTED; > nsCOMPtr <nsIURI> docURL; > doc->GetBaseURL(getter_AddRefs(docURL)); > nsCOMPtr<nsICSSParser> css; > nsCOMPtr<nsIStyleRule> mRule; >- nsComponentManager::CreateInstance(kCSSParserCID, >- nsnull, >- NS_GET_IID(nsICSSParser), >- getter_AddRefs(css)); >+ css = do_CreateInstance(kCSSParserCID); Declare the var while assigning instead of two statements? >Index: content/html/style/src/nsCSSLoader.h > nsresult CreateSheet(nsIURI* aURI, >+ nsIContent* aLinkingContent, > PRUint32 aDefaultNameSpaceID, > PRBool aSyncLoad, > StyleSheetState& aSheetState, > nsICSSStyleSheet** aSheet); Add a comment to the above method which notes when aLinkingContent can be null and when it shouldn't? I think your passing of null in CSSLoaderImpl::InternalLoadAgentSheet() is correct, but also please double check that it doesn't trigger the assert you placed into the implementation of CreateSheet(). The rest looks fine. r=caillon

Checked in at "07/02/2003 19:45 PDT".