[Webkit-unassigned] [Bug 33940] Address last round of review comments on r53607

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33940





--- Comment #1 from Adam Barth <abarth at webkit.org>  2010-02-10 00:58:08 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.