[FIX]Move GetBaseURI up to nsIContent

RESOLVED FIXED in mozilla1.5alpha

Status

()

Core
DOM: Core & HTML
P1
normal
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.5alpha
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Priority: -- → P1
Target Milestone: --- → mozilla1.5alpha
I'll drink to that
Blocks: 209594

Comment 2

15 years ago
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?

Comment 4

15 years ago
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.
Created attachment 126726 [details] [diff] [review]
Patch

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.
Depends on: 166996
Summary: Move GetBaseURI up to nsIContent → [FIX]Move GetBaseURI up to nsIContent
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...
Attachment #126726 - Flags: review?(caillon)
Blocks: 211128
Attachment #126726 - Flags: review?(caillon)
Created attachment 126827 [details] [diff] [review]
Better patch

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....
Attachment #126726 - Attachment is obsolete: true
Attachment #126827 - Flags: superreview?(jst)
Attachment #126827 - Flags: review?(caillon)
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
Attachment #126827 - Flags: superreview?(jst) → superreview+
> 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...
Blocks: 211376
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.
Created attachment 126960 [details] [diff] [review]
Include that, and a cssloader change

Since review has not happened yet... ;)
Attachment #126827 - Attachment is obsolete: true
Attachment #126827 - Flags: review?(caillon)
Attachment #126960 - Flags: review?(caillon)
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
Attachment #126960 - Flags: review?(caillon) → review+
Created attachment 126975 [details] [diff] [review]
With those changes
Attachment #126960 - Attachment is obsolete: true
Checked in at "07/02/2003 19:45 PDT".
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Blocks: 47534

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: desale → general
You need to log in before you can comment on or make changes to this bug.