Bug 65990 - MarkupAccumulator: make resolution of URLs implicit to appendQuotedURLAttributeValue()
Summary: MarkupAccumulator: make resolution of URLs implicit to appendQuotedURLAttribu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-10 09:10 PDT by Ryosuke Niwa
Modified: 2011-08-10 12:46 PDT (History)
4 users (show)

See Also:

Attachments
Patch (4.14 KB, patch)
2011-08-10 11:54 PDT, Benjamin Poulain 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-08-10 09:10:03 PDT 
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.
Comment 1 Benjamin Poulain 2011-08-10 09:57:39 PDT 
Will do, don't worry :)
Comment 2 Benjamin Poulain 2011-08-10 11:54:34 PDT 
Created attachment 103512 [details]
Patch
Comment 3 Benjamin Poulain 2011-08-10 11:57:27 PDT 
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.
Comment 4 Ryosuke Niwa 2011-08-10 12:02:49 PDT 
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?
Comment 5 Benjamin Poulain 2011-08-10 12:12:07 PDT 
(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.
Comment 6 Ryosuke Niwa 2011-08-10 12:14:08 PDT 
Comment on attachment 103512 [details]
Patch

ok
Comment 7 WebKit Review Bot 2011-08-10 12:46:01 PDT 
Comment on attachment 103512 [details]
Patch

Clearing flags on attachment: 103512

Committed r92786: <http://trac.webkit.org/changeset/92786>
Comment 8 WebKit Review Bot 2011-08-10 12:46:06 PDT 
All reviewed patches have been landed.  Closing bug.