Bug 33940 – Address last round of review comments on r53607

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Adam Barth]

> +    // 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.

[Adam Barth]

(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.

[Adam Barth]

>>> -            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.

[Adam Barth]

Created attachment 48473 [details]
Patch

[Darin Adler]

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

[Adam Barth]

Created attachment 48534 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 48534 [details]
Patch for landing

Clearing flags on attachment: 48534

Committed r54645: <http://trac.webkit.org/changeset/54645>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.