Bug 67255 – [EFL][DRT] Normalize file:///tmp/LayoutTests in LayoutTestController::pathToLocalResource()

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jarred Nicholls]

EFL DRT should translate file:///tmp/LayoutTests paths into a local URL that's derived from the path of the DRT application exec.

[Raphael Kubo da Costa (:rakuco)]

*** Bug 84336 has been marked as a duplicate of this bug. ***

[Joshua Lock (joshuagl)]

Created attachment 145699 [details]
Patch

[Dominik Röttsches (drott)]

(In reply to comment #2)
> Created an attachment (id=145699) [details]

Looks good to me, thanks.

[Gyuyoung Kim]

Comment on attachment 145699 [details]
Patch

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

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:198
> +        // If the url contains LayoutTests we need to remap that to

%s/url/URL/g

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:206
> +        // If the url is a child of /tmp we need to convert it to be a child

ditto.

[Joshua Lock (joshuagl)]

Created attachment 145798 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 145798 [details]
Patch

Looks fine.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 145798 [details]
Patch

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

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:181
> +// Convert a JSStringRef to a WTF::String - converted from a method of the same
> +// name in LayoutTestControllerMac.mm which returns a std::string
> +static inline String stringFromJSString(JSStringRef jsString)

Some nitpicking: have you tried operating on the UString returned by JSStringRef::ustring()? It has find() and substringSharingImpl(), so adding this method may be unnecessary.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:212
> +    if (requestedUrl.find("LayoutTests") != notFound) {
> +        // If the URL contains LayoutTests we need to remap that to
> +        // LOCAL_RESOURCE_ROOT which is the path of the LayoutTests directory
> +        // within the WebKit source tree.
> +        resourceRoot = getenv("LOCAL_RESOURCE_ROOT");
> +        String requestedRoot("/tmp/LayoutTests");
> +        size_t indexOfRootStart = requestedUrl.reverseFind(requestedRoot);
> +        indexOfSeparatorAfterRoot = indexOfRootStart + requestedRoot.length();
> +    } else if (requestedUrl.find("tmp") != notFound) {
> +        // If the URL is a child of /tmp we need to convert it to be a child
> +        // DUMPRENDERTREE_TEMP replace tmp with DUMPRENDERTREE_TEMP
> +        resourceRoot = getenv("DUMPRENDERTREE_TEMP");
> +        String requestedRoot("/tmp");
> +        size_t indexOfRootStart = requestedUrl.reverseFind(requestedRoot);
> +        indexOfSeparatorAfterRoot = indexOfRootStart + requestedRoot.length();
> +    }

The code to calculate the index of the separator seems to be duplicated in each case. What if you do something like
  if (requestedUrl.find("LayoutTests") != notFound) {
      requestedRoot = "/tmp/LayoutTests";
      resourceRoot = getenv("LOCAL_RESOURCE_ROOT");
  } else if (...) {
      requestedRoot = "/tmp";
      resourceRoot = getenv("DUMPRENDERTREE_TEMP");
  } else
      ASSERT_NOT_REACHED(); // via mac's LTC.mm

  // calculate separator index here

[Joshua Lock (joshuagl)]

Comment on attachment 145798 [details]
Patch

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

>> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:181
>> +static inline String stringFromJSString(JSStringRef jsString)
> 
> Some nitpicking: have you tried operating on the UString returned by JSStringRef::ustring()? It has find() and substringSharingImpl(), so adding this method may be unnecessary.

I have not, I looked to other LayoutController* for inspiration. Will have a go with ustring() now, I'd much prefer not to add extra methods - thanks for the hint.

>> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:212
>> +    }
> 
> The code to calculate the index of the separator seems to be duplicated in each case. What if you do something like
>   if (requestedUrl.find("LayoutTests") != notFound) {
>       requestedRoot = "/tmp/LayoutTests";
>       resourceRoot = getenv("LOCAL_RESOURCE_ROOT");
>   } else if (...) {
>       requestedRoot = "/tmp";
>       resourceRoot = getenv("DUMPRENDERTREE_TEMP");
>   } else
>       ASSERT_NOT_REACHED(); // via mac's LTC.mm
> 
>   // calculate separator index here

Much nicer, will do.

[Joshua Lock (joshuagl)]

Created attachment 146425 [details]
Patch

[Joshua Lock (joshuagl)]

Comment on attachment 145798 [details]
Patch

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

>>> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:181
>>> +static inline String stringFromJSString(JSStringRef jsString)
>> 
>> Some nitpicking: have you tried operating on the UString returned by JSStringRef::ustring()? It has find() and substringSharingImpl(), so adding this method may be unnecessary.
> 
> I have not, I looked to other LayoutController* for inspiration. Will have a go with ustring() now, I'd much prefer not to add extra methods - thanks for the hint.

I played around some with UString but instead I've opted to avoid the extra method by following a pattern I noticed in other parts of LayoutTestControllerEfl.cpp and construct a WTF::String from the characters of the JSStringRef like: String requestedUrl(url->characters()).

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #10)
> I played around some with UString but instead I've opted to avoid the extra method by following a pattern I noticed in other parts of LayoutTestControllerEfl.cpp and construct a WTF::String from the characters of the JSStringRef like: String requestedUrl(url->characters()).

Alright, it looks much better. Thank you!

[WebKit Review Bot]

Comment on attachment 146425 [details]
Patch

Clearing flags on attachment: 146425

Committed r119788: <http://trac.webkit.org/changeset/119788>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sudarsana Nagineni (babu)]

(In reply to comment #10)
> I played around some with UString but instead I've opted to avoid the extra method by following a pattern I noticed in other parts of LayoutTestControllerEfl.cpp and construct a WTF::String from the characters of the JSStringRef like: String requestedUrl(url->characters()).

The following tests started failing with this patch. It seems the problem is due to characters() not being null-terminated.

http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/952/steps/layout-test/logs/stdio

fast/dom/frame-loading-via-document-write.html = TEXT
http/tests/security/frame-loading-via-document-write.html = TEXT
http/tests/security/local-JavaScript-from-remote.html = TEXT
http/tests/security/local-iFrame-from-remote.html = TEXT

[Sudarsana Nagineni (babu)]

(In reply to comment #14)
> The following tests started failing with this patch. It seems the problem is due to characters() not being null-terminated.

New bug created for this issue: https://bugs.webkit.org/show_bug.cgi?id=88661