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

[bugzilla-daemon at webkit.org]

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

           Summary: Address last round of review comments on r53607
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: CSS
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: abarth at webkit.org
                CC: darin at apple.com


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

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