Bug 65990 – MarkupAccumulator: make resolution of URLs implicit to appendQuotedURLAttributeValue()

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

http://trac.webkit.org/changeset/92769 merged two code paths resolveURLIfNeeded.  Now that there's exactly one place where resolveURLIfNeeded and resolveURLIfNeeded are called.  We should probably merge these two functions.

[Benjamin Poulain]

Will do, don't worry :)

[Benjamin Poulain]

Created attachment 103512 [details]
Patch

[Benjamin Poulain]

Just moved appendQuotedURLAttributeValue() to an internal function, so appendAttribute() is the only function the client of the interface should use.

I kept resolveURLIfNeeded() because it makes the code more readable IMHO.

[Ryosuke Niwa]

Comment on attachment 103512 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103512&action=review

> Source/WebCore/editing/MarkupAccumulator.cpp:187
> +    const String resolvedURLString = resolveURLIfNeeded(element, attribute.value());
>      UChar quoteChar = '\"';
> -    String strippedURLString = urlString.stripWhiteSpace();
> +    String strippedURLString = resolvedURLString.stripWhiteSpace();

Maybe do this one in one line?  i.e.
String strippedURLString = resolveURLIfNeeded(element, attribute.value()).stripWhiteSpace();

Also, can we make resolveURLIfNeeded a static local function since it doesn't need to access any member variable?

[Benjamin Poulain]

(In reply to comment #4)
> (From update of attachment 103512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103512&action=review
> 
> > Source/WebCore/editing/MarkupAccumulator.cpp:187
> > +    const String resolvedURLString = resolveURLIfNeeded(element, attribute.value());
> >      UChar quoteChar = '\"';
> > -    String strippedURLString = urlString.stripWhiteSpace();
> > +    String strippedURLString = resolvedURLString.stripWhiteSpace();
> 
> Maybe do this one in one line?  i.e.
> String strippedURLString = resolveURLIfNeeded(element, attribute.value()).stripWhiteSpace();

The non-stripped url is used in the general case. The stripped version is only used in the JavaScript case.  I am just following the original code.
 
> Also, can we make resolveURLIfNeeded a static local function since it doesn't need to access any member variable?

It does need m_resolveURLsMethod.

[Ryosuke Niwa]

Comment on attachment 103512 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 103512 [details]
Patch

Clearing flags on attachment: 103512

Committed r92786: <http://trac.webkit.org/changeset/92786>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.