Bug 107363 - [EFL][WK2] Implement WebInspector::localizedStringsURL() on EFL
Summary: [EFL][WK2] Implement WebInspector::localizedStringsURL() on EFL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sudarsana Nagineni (babu)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-18 23:52 PST by Sudarsana Nagineni (babu)
Modified: 2013-01-21 17:06 PST (History)
14 users (show)

See Also:


Attachments
patch (1.72 KB, patch)
2013-01-19 06:28 PST, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff
patch (1.72 KB, patch)
2013-01-19 08:07 PST, Sudarsana Nagineni (babu)
benjamin: review-
Details | Formatted Diff | Diff
Patch (1.57 KB, patch)
2013-01-21 15:44 PST, Sudarsana Nagineni (babu)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sudarsana Nagineni (babu) 2013-01-18 23:52:56 PST
Implement WebInspector::localizedStringsURL() to return the URL of the localizedStrings.js.
Comment 1 Sudarsana Nagineni (babu) 2013-01-19 06:28:18 PST
Created attachment 183623 [details]
patch
Comment 2 Chris Dumez 2013-01-19 06:39:33 PST
Comment on attachment 183623 [details]
patch

Looks fine but you're not unskipping any test?
Comment 3 Sudarsana Nagineni (babu) 2013-01-19 07:00:27 PST
(In reply to comment #2)
> (From update of attachment 183623 [details])
> Looks fine but you're not unskipping any test?

Thanks Chris.

Tests were unskipped already. We have 321 Web inspector tests failing on Debug  bot now after r140044.

http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r140060%20%288198%29/results.html

http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/8198/steps/layout-test/logs/stdio
Comment 4 Chris Dumez 2013-01-19 07:03:14 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 183623 [details] [details])
> > Looks fine but you're not unskipping any test?
> 
> Thanks Chris.
> 
> Tests were unskipped already. We have 321 Web inspector tests failing on Debug  bot now after r140044.
> 
> http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r140060%20%288198%29/results.html
> 
> http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/8198/steps/layout-test/logs/stdio

Oh right. All good then :)
Comment 5 Gyuyoung Kim 2013-01-19 07:04:55 PST
Comment on attachment 183623 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/efl/WebInspectorEfl.cpp:33
>  #include <wtf/text/WTFString.h>

It looks we don't need to include WTFString.h anymore, isn't it ?
Comment 6 Sudarsana Nagineni (babu) 2013-01-19 07:58:39 PST
Comment on attachment 183623 [details]
patch

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

>> Source/WebKit2/WebProcess/WebPage/efl/WebInspectorEfl.cpp:33
>>  #include <wtf/text/WTFString.h>
> 
> It looks we don't need to include WTFString.h anymore, isn't it ?

I will check and likely remove.
Comment 7 Sudarsana Nagineni (babu) 2013-01-19 08:07:45 PST
Created attachment 183626 [details]
patch
Comment 8 Gyuyoung Kim 2013-01-19 21:31:34 PST
Comment on attachment 183626 [details]
patch

LGTM.
Comment 9 Chris Dumez 2013-01-21 03:02:04 PST
This patch fixes about 300 failures on our build bots.
Comment 10 Laszlo Gombos 2013-01-21 07:32:17 PST
(In reply to comment #9)

> This patch fixes about 300 failures on our build bots.

I was confused by this so just to clarify - these failing tests are not skipped at the moment that's why the patch does not include a change in the Skipped file. This patch will bring back the bot to green.

Patch looks good to me.
Comment 11 Chris Dumez 2013-01-21 07:45:18 PST
(In reply to comment #10)
> (In reply to comment #9)
> 
> > This patch fixes about 300 failures on our build bots.
> 
> I was confused by this so just to clarify - these failing tests are not skipped at the moment that's why the patch does not include a change in the Skipped file. This patch will bring back the bot to green.
> 
> Patch looks good to me.

This is correct. Those tests are not skipped and failing on the WK2 EFL debug build bot.
Comment 13 Benjamin Poulain 2013-01-21 12:26:00 PST
Comment on attachment 183626 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/efl/WebInspectorEfl.cpp:43
> +    StringBuilder builder;
> +    builder.appendLiteral("file://");
> +    builder.append(WebCore::inspectorResourcePath());
> +    builder.appendLiteral("/localizedStrings.js");
> +
> +    return builder.toString();

This should be using the String Operators, not String builder.
Comment 14 Sudarsana Nagineni (babu) 2013-01-21 15:44:25 PST
Created attachment 183841 [details]
Patch

Take Benjamin's feedback into consideration.
Comment 15 Sudarsana Nagineni (babu) 2013-01-21 16:48:14 PST
Comment on attachment 183841 [details]
Patch

Thanks for review!
Comment 16 WebKit Review Bot 2013-01-21 17:06:02 PST
Comment on attachment 183841 [details]
Patch

Clearing flags on attachment: 183841

Committed r140372: <http://trac.webkit.org/changeset/140372>
Comment 17 WebKit Review Bot 2013-01-21 17:06:08 PST
All reviewed patches have been landed.  Closing bug.