> + // Notice that this object inherits a baseURL function from StyleBase that > + // crawls the parent() relation looking for a non-0 putativeBaseURL. > + const KURL& putativeBaseURL() const { return m_baseURL; } It's good to distinguish these two functions. The name putativeBaseURL doesn't seem to helpfully make it clear which of the two to call, though. The two differences seem to be: 1) The putativeBaseURL function is a bit faster than baseURL. 2) The putativeBaseURL function returns null when m_baseURL is null rather than climbing to the parent sheet. Are there any callers that need either behavior (1) or (2) other than StyleBase::baseURL itself? If not, maybe we can find a way to avoid the putativeBaseURL function entirely. > - m_sheet = XSLStyleSheet::createEmbedded(this, m_localHref); > + KURL baseURL = KURL(ParsedURLString, m_localHref); > + m_sheet = XSLStyleSheet::createEmbedded(this, m_localHref, baseURL); Seems to me that CSSStyleSheet::createInline and XSLStyleSheet::createEmbedded are doing the same thing, for the two different types of stylesheet. Which leads me to ask: 1) Why do they have different names? 2) Why does XSLStyleSheet take two arguments instead of the one that Document::createInline does? > - m_sheet = CSSStyleSheet::create(e, String(), document->inputEncoding()); > + m_sheet = CSSStyleSheet::create(e, String(), KURL(), document->inputEncoding()); Why not use createInline here? > + stylesheetRootNode->document()->url()); // FIXME: Should we use baseURL here? > + node->document()->url()); // FIXME: Should we use baseURL here? I'd like to ask an XSL expert (Alexey?) to help us figure out how to test these or what is correct. Although we are stuck with the name "href" for the DOM function, I would have preferred to use the name "originalURL" everywhere else, since "href" is not sufficiently unambiguous.
(In reply to comment #0) > > + // Notice that this object inherits a baseURL function from StyleBase that > > + // crawls the parent() relation looking for a non-0 putativeBaseURL. > > + const KURL& putativeBaseURL() const { return m_baseURL; } > > It's good to distinguish these two functions. The name putativeBaseURL doesn't > seem to helpfully make it clear which of the two to call, though. The two > differences seem to be: > > 1) The putativeBaseURL function is a bit faster than baseURL. > 2) The putativeBaseURL function returns null when m_baseURL is null rather > than climbing to the parent sheet. > > Are there any callers that need either behavior (1) or (2) other than > StyleBase::baseURL itself? If not, maybe we can find a way to avoid the > putativeBaseURL function entirely. Turns out there are a bunch of clients. However, the name "finalURL" seems more appropriate for what these clients want, so I've renamed the whole concept to finalURL. The baseURL is computed as before, taking into account the parent sheets. > > - m_sheet = XSLStyleSheet::createEmbedded(this, m_localHref); > > + KURL baseURL = KURL(ParsedURLString, m_localHref); > > + m_sheet = XSLStyleSheet::createEmbedded(this, m_localHref, baseURL); > > Seems to me that CSSStyleSheet::createInline and XSLStyleSheet::createEmbedded > are doing the same thing, for the two different types of stylesheet. Which > leads me to ask: These are, in fact, the same concept. createEmbedded had one client, so I've renamed is to createInline. > 1) Why do they have different names? Because I originally thought they were different, but they're not. > 2) Why does XSLStyleSheet take two arguments instead of the one that > Document::createInline does? They should both take one argument. I've fixed this. > > - m_sheet = CSSStyleSheet::create(e, String(), document->inputEncoding()); > > + m_sheet = CSSStyleSheet::create(e, String(), KURL(), document->inputEncoding()); > > Why not use createInline here? Done. > > + stylesheetRootNode->document()->url()); // FIXME: Should we use baseURL here? > > > + node->document()->url()); // FIXME: Should we use baseURL here? > > I'd like to ask an XSL expert (Alexey?) to help us figure out how to test these > or what is correct. > > Although we are stuck with the name "href" for the DOM function, I would have > preferred to use the name "originalURL" everywhere else, since "href" is not > sufficiently unambiguous. I've renamed this to originalURL. Will post a patch shortly.
>>> - m_sheet = CSSStyleSheet::create(e, String(), document->inputEncoding()); >>> + m_sheet = CSSStyleSheet::create(e, String(), KURL(), document->inputEncoding()); >> >> Why not use createInline here? > > Done. I lied. This needs to pass in the inputEncoding. We could make a createInline that takes an input encoding, but it might not be worthwhile for this one call site.
Created attachment 48473 [details] Patch
Comment on attachment 48473 [details] Patch > + KURL finalURL = KURL(ParsedURLString, m_localHref); Syntax here is a little strange. It should just be a constructor rather than looking like an assignment statement. r=me
Created attachment 48534 [details] Patch for landing
Comment on attachment 48534 [details] Patch for landing Clearing flags on attachment: 48534 Committed r54645: <http://trac.webkit.org/changeset/54645>
All reviewed patches have been landed. Closing bug.