Network error page: Fix reload button for RTL languages.

Network error page:  Fix reload button for RTL languages.
The URL was incorrectly wrapped in LTR / RTL formatting
characters.

BUG=249497
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207803

[Nico, 2013-06-20 16:38:20]

lgtm

Is it possible to test this somehow?

[mmenke, 2013-06-20 17:11:00]

On 2013/06/20 16:38:20, Nico wrote:
> lgtm
> 
> Is it possible to test this somehow?

I'm assuming you mean in a browser test, rather than manually (I have tested
this manually)?  I hadn't considered it, but I think it depends on what
navigation events occur when trying to navigate to the invalid URL vs a normal
reload.

I'd like to land this as-is (Since I think this CL should be merged to M28), and
then investigate a test as a followup CL.  Sound reasonable?

[Nico, 2013-06-20 17:20:53]

On Thu, Jun 20, 2013 at 10:11 AM, <mmenke@chromium.org> wrote:

> On 2013/06/20 16:38:20, Nico wrote:
>
>> lgtm
>>
>
>  Is it possible to test this somehow?
>>
>
> I'm assuming you mean in a browser test, rather than manually (I have
> tested
> this manually)?


Yes, some form of automated test. Is it ever right for webui strings to
contain rtl/ltr markers? Maybe there could just be a dcheck for that
somewhere?


>  I hadn't considered it, but I think it depends on what
> navigation events occur when trying to navigate to the invalid URL vs a
> normal
> reload.
>
> I'd like to land this as-is (Since I think this CL should be merged to
> M28), and
> then investigate a test as a followup CL.  Sound reasonable?
>

Yes, sure.


>
>
https://codereview.chromium.**org/17151013/<https://codereview.chromium.org/1...
>

[mmenke, 2013-06-20 17:45:41]

On 2013/06/20 17:20:53, Nico wrote:
> On Thu, Jun 20, 2013 at 10:11 AM, <mailto:mmenke@chromium.org> wrote:
> 
> > On 2013/06/20 16:38:20, Nico wrote:
> >
> >> lgtm
> >>
> >
> >  Is it possible to test this somehow?
> >>
> >
> > I'm assuming you mean in a browser test, rather than manually (I have
> > tested
> > this manually)?
> 
> 
> Yes, some form of automated test. Is it ever right for webui strings to
> contain rtl/ltr markers? Maybe there could just be a dcheck for that
> somewhere?

The issue is that URLs embedded in RTL text should still be displayed LTR, by
convention.  So we do need LTR wrappers around the URL for display, just
shouldn't use it for URLs used for navigation.

> 
> 
> >  I hadn't considered it, but I think it depends on what
> > navigation events occur when trying to navigate to the invalid URL vs a
> > normal
> > reload.
> >
> > I'd like to land this as-is (Since I think this CL should be merged to
> > M28), and
> > then investigate a test as a followup CL.  Sound reasonable?
> >
> 
> Yes, sure.
> 
> 
> >
> >
>
https://codereview.chromium.**org/17151013/%3Chttps://codereview.chromium.org...>
> >

[commit-bot: I haz the power, 2013-06-21 01:04:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/17151013/1

[commit-bot: I haz the power, 2013-06-21 13:23:18]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-06-21 13:27:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/17151013/1

[commit-bot: I haz the power, 2013-06-21 13:36:18]

Change committed as 207803

[mmenke, 2013-06-25 19:39:06]

On 2013/06/21 13:36:18, I haz the power (commit-bot) wrote:
> Change committed as 207803

Thakis:  It turns out it's not possible to set the local in browser tests. 
There are some tests in locale_tests_browsertest.cc that think they're doing
this, but they fail to actually set the locale, making the tests useless.

There's a comment in chrome_browser_main.cc about the ResourceBundle already
being initialized in tests, so it bypasses some of the locale selection logic,
which may be the source of the problem.  The comment about this was made over 4
years ago, by sky, so I'm not sure those browser tests ever actually did work.

[Nico, 2013-06-25 19:47:43]

Thanks for investigating!

On Tue, Jun 25, 2013 at 12:39 PM, <mmenke@chromium.org> wrote:

> On 2013/06/21 13:36:18, I haz the power (commit-bot) wrote:
>
>> Change committed as 207803
>>
>
> Thakis:  It turns out it's not possible to set the local in browser tests.
> There are some tests in locale_tests_browsertest.cc that think they're
> doing
> this, but they fail to actually set the locale, making the tests useless.
>

Can you file a bug for this and cc the test authors?


>
> There's a comment in chrome_browser_main.cc about the ResourceBundle
> already
> being initialized in tests, so it bypasses some of the locale selection
> logic,
> which may be the source of the problem.  The comment about this was made
> over 4
> years ago, by sky, so I'm not sure those browser tests ever actually did
> work.
>
>
https://codereview.chromium.**org/17151013/<https://codereview.chromium.org/1...
>

[mmenke, 2013-06-25 20:00:23]

On 2013/06/25 19:47:43, Nico wrote:
> Thanks for investigating!
> 
> On Tue, Jun 25, 2013 at 12:39 PM, <mailto:mmenke@chromium.org> wrote:
> 
> > On 2013/06/21 13:36:18, I haz the power (commit-bot) wrote:
> >
> >> Change committed as 207803
> >>
> >
> > Thakis:  It turns out it's not possible to set the local in browser tests.
> > There are some tests in locale_tests_browsertest.cc that think they're
> > doing
> > this, but they fail to actually set the locale, making the tests useless.
> >
> 
> Can you file a bug for this and cc the test authors?

The tests have been around since initial_commit.  Filed as
http://crbug.com/254072, CCed those who have contributed the most lines since
initial commit.

> >
> > There's a comment in chrome_browser_main.cc about the ResourceBundle
> > already
> > being initialized in tests, so it bypasses some of the locale selection
> > logic,
> > which may be the source of the problem.  The comment about this was made
> > over 4
> > years ago, by sky, so I'm not sure those browser tests ever actually did
> > work.
> >
> >
>
https://codereview.chromium.**org/17151013/%3Chttps://codereview.chromium.org...>
> >