Show IDN in error screens (DNS failure etc.)

Show IDN in error screens (DNS failure etc.)

BUG=2351
TEST=Visit unavailable IDN site and check no punycode is displayed in error screen.

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

[yuusuke, 2013-09-25 18:35:15]

Hello, everyone!

I made this relatively small patch that will probably fix all error screens
related punycode/url_escape issues.

Also, it would be nice if someone could "trybot" this patch, cos I don't have
enough permissions to run them myself.

Best regards,
Artem Kuznetcov

[tony, 2013-09-25 18:39:41]

+mmenke who I think currently owns error pages.

[mmenke, 2013-09-25 21:30:17]

On 2013/09/25 18:39:41, tony wrote:
> +mmenke who I think currently owns error pages.

I'll take a look tomorrow.  Really great to see a fix for this!

[joi, 2013-09-25 22:02:04]

//content/public LGTM.

On Wed, Sep 25, 2013 at 6:35 PM,  <yuusuke@yandex-team.ru> wrote:
> Reviewers: darin, jamesr, joi, tc,
>
> Message:
> Hello, everyone!
>
> I made this relatively small patch that will probably fix all error screens
> related punycode/url_escape issues.
>
> Also, it would be nice if someone could "trybot" this patch, cos I don't
> have
> enough permissions to run them myself.
>
> Best regards,
> Artem Kuznetcov
>
> Description:
> Show IDN in error screens (DNS failure etc.)
>
> BUG=2351
> TEST=Visit unavailable IDN site and check no punycode is displayed in error
> screen.
>
> Please review this at https://codereview.chromium.org/24503004/
>
> SVN Base: https://src.chromium.org/chrome/trunk/src/
>
> Affected files (+43, -10 lines):
>   M     android_webview/renderer/aw_content_renderer_client.h
>   M     android_webview/renderer/aw_content_renderer_client.cc
>   M     chrome/browser/chromeos/offline/offline_load_page.cc
>   M     chrome/common/localized_error.h
>   M     chrome/common/localized_error.cc
>   M     chrome/renderer/chrome_content_renderer_client.h
>   M     chrome/renderer/chrome_content_renderer_client.cc
>   M     chrome/renderer/net/net_error_helper.h
>   M     chrome/renderer/net/net_error_helper.cc
>   M     content/public/renderer/content_renderer_client.h
>   M     content/public/renderer/render_view.h
>   M     content/renderer/render_view_impl.h
>   M     content/renderer/render_view_impl.cc
>
>
> Index: android_webview/renderer/aw_content_renderer_client.cc
> ===================================================================
> --- android_webview/renderer/aw_content_renderer_client.cc      (revision
> 225096)
> +++ android_webview/renderer/aw_content_renderer_client.cc      (working
> copy)
> @@ -67,6 +67,7 @@
>      WebKit::WebFrame* /* frame */,
>      const WebKit::WebURLRequest& failed_request,
>      const WebKit::WebURLError& error,
> +    const std::string& accept_languages,
>      std::string* error_html,
>      string16* error_description) {
>    if (error_html) {
> Index: android_webview/renderer/aw_content_renderer_client.h
> ===================================================================
> --- android_webview/renderer/aw_content_renderer_client.h       (revision
> 225096)
> +++ android_webview/renderer/aw_content_renderer_client.h       (working
> copy)
> @@ -31,6 +31,7 @@
>        WebKit::WebFrame* frame,
>        const WebKit::WebURLRequest& failed_request,
>        const WebKit::WebURLError& error,
> +      const std::string& accept_languages,
>        std::string* error_html,
>        string16* error_description) OVERRIDE;
>    virtual unsigned long long VisitedLinkHash(const char* canonical_url,
> Index: chrome/browser/chromeos/offline/offline_load_page.cc
> ===================================================================
> --- chrome/browser/chromeos/offline/offline_load_page.cc        (revision
> 225096)
> +++ chrome/browser/chromeos/offline/offline_load_page.cc        (working
> copy)
> @@ -102,9 +102,11 @@
>      resource_id = IDR_OFFLINE_APP_LOAD_HTML;
>    } else {
>      const std::string locale = g_browser_process->GetApplicationLocale();
> +    const std::string accept_languages =
> +        profile->GetPrefs()->GetString(prefs::kAcceptLanguages);
>      LocalizedError::GetStrings(net::ERR_INTERNET_DISCONNECTED,
>                                 net::kErrorDomain, url_, false, locale,
> -                               &error_strings);
> +                               accept_languages, &error_strings);
>      resource_id = IDR_OFFLINE_NET_LOAD_HTML;
>    }
>
> Index: chrome/common/localized_error.cc
> ===================================================================
> --- chrome/common/localized_error.cc    (revision 225096)
> +++ chrome/common/localized_error.cc    (working copy)
> @@ -20,6 +20,7 @@
>  #include "grit/generated_resources.h"
>  #include "net/base/escape.h"
>  #include "net/base/net_errors.h"
> +#include "net/base/net_util.h"
>  #include "third_party/WebKit/public/platform/WebURLError.h"
>  #include "ui/base/l10n/l10n_util.h"
>  #include "ui/webui/web_ui_util.h"
> @@ -496,6 +497,7 @@
>                                  const GURL& failed_url,
>                                  bool is_post,
>                                  const std::string& locale,
> +                                const std::string& accept_languages,
>                                  base::DictionaryValue* error_strings) {
>    bool rtl = LocaleIsRTL();
>    error_strings->SetString("textdirection", rtl ? "rtl" : "ltr");
> @@ -530,7 +532,9 @@
>      options.suggestions = SUGGEST_NONE;
>    }
>
> -  string16 failed_url_string(UTF8ToUTF16(failed_url.spec()));
> +  string16 failed_url_string(net::FormatUrl(failed_url, accept_languages,
> +      net::kFormatUrlOmitNothing, net::UnescapeRule::NORMAL, NULL, NULL,
> +      NULL));
>    // URLs are always LTR.
>    if (rtl)
>      base::i18n::WrapStringWithLTRFormatting(&failed_url_string);
> @@ -545,10 +549,9 @@
>    base::DictionaryValue* summary = new base::DictionaryValue;
>    summary->SetString("msg",
>        l10n_util::GetStringUTF16(options.summary_resource_id));
> -  // TODO(tc): We want the unicode url and host here since they're being
> -  //           displayed.
>    summary->SetString("failedUrl", failed_url_string);
> -  summary->SetString("hostName", failed_url.host());
> +  summary->SetString("hostName", net::IDNToUnicode(failed_url.host(),
> +                                                   accept_languages));
>    summary->SetString("productName",
>                       l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
>
> Index: chrome/common/localized_error.h
> ===================================================================
> --- chrome/common/localized_error.h     (revision 225096)
> +++ chrome/common/localized_error.h     (working copy)
> @@ -33,6 +33,7 @@
>                           const GURL& failed_url,
>                           bool is_post,
>                           const std::string& locale,
> +                         const std::string& accept_languages,
>                           base::DictionaryValue* strings);
>
>    // Returns a description of the encountered error.
> Index: chrome/renderer/chrome_content_renderer_client.cc
> ===================================================================
> --- chrome/renderer/chrome_content_renderer_client.cc   (revision 225096)
> +++ chrome/renderer/chrome_content_renderer_client.cc   (working copy)
> @@ -865,6 +865,7 @@
>      WebKit::WebFrame* frame,
>      const WebKit::WebURLRequest& failed_request,
>      const WebKit::WebURLError& error,
> +    const std::string& accept_languages,
>      std::string* error_html,
>      string16* error_description) {
>    const GURL failed_url = error.unreachableURL;
> @@ -891,12 +892,13 @@
>      } else {
>        const std::string locale = RenderThread::Get()->GetLocale();
>        if (!NetErrorHelper::GetErrorStringsForDnsProbe(
> -              frame, error, is_post, locale, &error_strings)) {
> +              frame, error, is_post, locale, accept_languages,
> +              &error_strings)) {
>          // In most cases, the NetErrorHelper won't provide
> DNS-probe-specific
>          // error pages, so fall back to LocalizedError.
>          LocalizedError::GetStrings(error.reason, error.domain.utf8(),
>                                     error.unreachableURL, is_post, locale,
> -                                   &error_strings);
> +                                   accept_languages, &error_strings);
>        }
>        resource_id = IDR_NET_ERROR_HTML;
>      }
> Index: chrome/renderer/chrome_content_renderer_client.h
> ===================================================================
> --- chrome/renderer/chrome_content_renderer_client.h    (revision 225096)
> +++ chrome/renderer/chrome_content_renderer_client.h    (working copy)
> @@ -85,6 +85,7 @@
>        WebKit::WebFrame* frame,
>        const WebKit::WebURLRequest& failed_request,
>        const WebKit::WebURLError& error,
> +      const std::string& accept_languages,
>        std::string* error_html,
>        string16* error_description) OVERRIDE;
>    virtual void DeferMediaLoad(content::RenderView* render_view,
> Index: chrome/renderer/net/net_error_helper.cc
> ===================================================================
> --- chrome/renderer/net/net_error_helper.cc     (revision 225096)
> +++ chrome/renderer/net/net_error_helper.cc     (working copy)
> @@ -174,6 +174,7 @@
>      const WebKit::WebURLError& error,
>      bool is_failed_post,
>      const std::string& locale,
> +    const std::string& accept_languages,
>      base::DictionaryValue* error_strings) {
>    if (!IsMainFrame(frame))
>      return false;
> @@ -186,7 +187,7 @@
>        chrome_common_net::DNS_PROBE_POSSIBLE,
>        chrome_common_net::kDnsProbeErrorDomain,
>        error.unreachableURL,
> -      is_failed_post, locale, error_strings);
> +      is_failed_post, locale, accept_languages, error_strings);
>    return true;
>  }
>
> @@ -214,11 +215,14 @@
>
>    WebKit::WebURLError error = GetUpdatedError();
>    base::DictionaryValue error_strings;
> +  const std::string accept_languages =
> +      render_view() ? render_view()->GetAcceptLanguages() : std::string();
>    LocalizedError::GetStrings(error.reason,
>                               error.domain.utf8(),
>                               error.unreachableURL,
>                               is_failed_post_,
>                               RenderThread::Get()->GetLocale(),
> +                             accept_languages,
>                               &error_strings);
>
>    std::string json;
> Index: chrome/renderer/net/net_error_helper.h
> ===================================================================
> --- chrome/renderer/net/net_error_helper.h      (revision 225096)
> +++ chrome/renderer/net/net_error_helper.h      (working copy)
> @@ -50,6 +50,7 @@
>        const WebKit::WebURLError& error,
>        bool is_failed_post,
>        const std::string& locale,
> +      const std::string& accept_languages,
>        base::DictionaryValue* error_strings);
>
>   protected:
> Index: content/public/renderer/content_renderer_client.h
> ===================================================================
> --- content/public/renderer/content_renderer_client.h   (revision 225096)
> +++ content/public/renderer/content_renderer_client.h   (working copy)
> @@ -113,6 +113,7 @@
>        WebKit::WebFrame* frame,
>        const WebKit::WebURLRequest& failed_request,
>        const WebKit::WebURLError& error,
> +      const std::string& accept_languages,
>        std::string* error_html,
>        string16* error_description) {}
>
> Index: content/public/renderer/render_view.h
> ===================================================================
> --- content/public/renderer/render_view.h       (revision 225096)
> +++ content/public/renderer/render_view.h       (working copy)
> @@ -5,6 +5,8 @@
>  #ifndef CONTENT_PUBLIC_RENDERER_RENDER_VIEW_H_
>  #define CONTENT_PUBLIC_RENDERER_RENDER_VIEW_H_
>
> +#include <string>
> +
>  #include "base/basictypes.h"
>  #include "base/strings/string16.h"
>  #include "content/common/content_export.h"
> @@ -163,6 +165,9 @@
>    // Returns a collection of security info about |frame|.
>    virtual SSLStatus GetSSLStatusOfFrame(WebKit::WebFrame* frame) const = 0;
>
> +  // Returns |renderer_preferences_.accept_languages| value.
> +  virtual const std::string& GetAcceptLanguages() const = 0;
> +
>  #if defined(OS_ANDROID)
>    virtual void UpdateTopControlsState(TopControlsState constraints,
>                                        TopControlsState current,
> Index: content/renderer/render_view_impl.cc
> ===================================================================
> --- content/renderer/render_view_impl.cc        (revision 225096)
> +++ content/renderer/render_view_impl.cc        (working copy)
> @@ -133,6 +133,7 @@
>  #include "net/base/data_url.h"
>  #include "net/base/escape.h"
>  #include "net/base/net_errors.h"
> +#include "net/base/net_util.h"
>  #include
> "net/base/registry_controlled_domains/registry_controlled_domain.h"
>  #include "net/http/http_util.h"
>  #include "third_party/WebKit/public/platform/WebCString.h"
> @@ -2199,7 +2200,8 @@
>      error_html = &html;
>    } else {
>      GetContentClient()->renderer()->GetNavigationErrorStrings(
> -        frame, failed_request, error, &alt_html, NULL);
> +        frame, failed_request, error,
> renderer_preferences_.accept_languages,
> +        &alt_html, NULL);
>      error_html = &alt_html;
>    }
>
> @@ -3134,6 +3136,10 @@
>    return ssl_status;
>  }
>
> +const std::string& RenderViewImpl::GetAcceptLanguages() const {
> +  return renderer_preferences_.accept_languages;
> +}
> +
>  WebNavigationPolicy RenderViewImpl::decidePolicyForNavigation(
>      WebFrame* frame, WebDataSource::ExtraData* extraData,
>      const WebURLRequest& request, WebNavigationType type,
> @@ -3623,6 +3629,7 @@
>        frame,
>        failed_request,
>        error,
> +      renderer_preferences_.accept_languages,
>        NULL,
>        &params.error_description);
>    params.url = error.unreachableURL;
> @@ -3869,6 +3876,7 @@
>        frame,
>        failed_request,
>        error,
> +      renderer_preferences_.accept_languages,
>        NULL,
>        &error_description);
>    Send(new ViewHostMsg_DidFailLoadWithError(routing_id_,
> @@ -4565,7 +4573,9 @@
>    remove_params.ClearQuery();
>    remove_params.ClearRef();
>    const GURL url_to_send = failed_url.ReplaceComponents(remove_params);
> -  std::string spec_to_send = url_to_send.spec();
> +  std::string spec_to_send = UTF16ToUTF8(net::FormatUrl(url_to_send,
> +      renderer_preferences_.accept_languages, net::kFormatUrlOmitNothing,
> +      net::UnescapeRule::NORMAL, NULL, NULL, NULL));
>    // Notify link doctor of the url truncation by sending of "?" at the end.
>    if (failed_url.has_query())
>      spec_to_send.append("?");
> Index: content/renderer/render_view_impl.h
> ===================================================================
> --- content/renderer/render_view_impl.h (revision 225096)
> +++ content/renderer/render_view_impl.h (working copy)
> @@ -697,6 +697,7 @@
>                                               const std::string& value)
> OVERRIDE;
>    virtual void ClearEditCommands() OVERRIDE;
>    virtual SSLStatus GetSSLStatusOfFrame(WebKit::WebFrame* frame) const
> OVERRIDE;
> +  virtual const std::string& GetAcceptLanguages() const OVERRIDE;
>  #if defined(OS_ANDROID)
>    virtual void UpdateTopControlsState(TopControlsState constraints,
>                                        TopControlsState current,
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jói, 2013-09-25 22:04:35]

//content/public LGTM (now from @chromium.org address)

On Wed, Sep 25, 2013 at 10:00 PM, Jói Sigurðsson <joi@google.com> wrote:
> //content/public LGTM.
>
> On Wed, Sep 25, 2013 at 6:35 PM,  <yuusuke@yandex-team.ru> wrote:
>> Reviewers: darin, jamesr, joi, tc,
>>
>> Message:
>> Hello, everyone!
>>
>> I made this relatively small patch that will probably fix all error screens
>> related punycode/url_escape issues.
>>
>> Also, it would be nice if someone could "trybot" this patch, cos I don't
>> have
>> enough permissions to run them myself.
>>
>> Best regards,
>> Artem Kuznetcov
>>
>> Description:
>> Show IDN in error screens (DNS failure etc.)
>>
>> BUG=2351
>> TEST=Visit unavailable IDN site and check no punycode is displayed in error
>> screen.
>>
>> Please review this at https://codereview.chromium.org/24503004/
>>
>> SVN Base: https://src.chromium.org/chrome/trunk/src/
>>
>> Affected files (+43, -10 lines):
>>   M     android_webview/renderer/aw_content_renderer_client.h
>>   M     android_webview/renderer/aw_content_renderer_client.cc
>>   M     chrome/browser/chromeos/offline/offline_load_page.cc
>>   M     chrome/common/localized_error.h
>>   M     chrome/common/localized_error.cc
>>   M     chrome/renderer/chrome_content_renderer_client.h
>>   M     chrome/renderer/chrome_content_renderer_client.cc
>>   M     chrome/renderer/net/net_error_helper.h
>>   M     chrome/renderer/net/net_error_helper.cc
>>   M     content/public/renderer/content_renderer_client.h
>>   M     content/public/renderer/render_view.h
>>   M     content/renderer/render_view_impl.h
>>   M     content/renderer/render_view_impl.cc
>>
>>
>> Index: android_webview/renderer/aw_content_renderer_client.cc
>> ===================================================================
>> --- android_webview/renderer/aw_content_renderer_client.cc      (revision
>> 225096)
>> +++ android_webview/renderer/aw_content_renderer_client.cc      (working
>> copy)
>> @@ -67,6 +67,7 @@
>>      WebKit::WebFrame* /* frame */,
>>      const WebKit::WebURLRequest& failed_request,
>>      const WebKit::WebURLError& error,
>> +    const std::string& accept_languages,
>>      std::string* error_html,
>>      string16* error_description) {
>>    if (error_html) {
>> Index: android_webview/renderer/aw_content_renderer_client.h
>> ===================================================================
>> --- android_webview/renderer/aw_content_renderer_client.h       (revision
>> 225096)
>> +++ android_webview/renderer/aw_content_renderer_client.h       (working
>> copy)
>> @@ -31,6 +31,7 @@
>>        WebKit::WebFrame* frame,
>>        const WebKit::WebURLRequest& failed_request,
>>        const WebKit::WebURLError& error,
>> +      const std::string& accept_languages,
>>        std::string* error_html,
>>        string16* error_description) OVERRIDE;
>>    virtual unsigned long long VisitedLinkHash(const char* canonical_url,
>> Index: chrome/browser/chromeos/offline/offline_load_page.cc
>> ===================================================================
>> --- chrome/browser/chromeos/offline/offline_load_page.cc        (revision
>> 225096)
>> +++ chrome/browser/chromeos/offline/offline_load_page.cc        (working
>> copy)
>> @@ -102,9 +102,11 @@
>>      resource_id = IDR_OFFLINE_APP_LOAD_HTML;
>>    } else {
>>      const std::string locale = g_browser_process->GetApplicationLocale();
>> +    const std::string accept_languages =
>> +        profile->GetPrefs()->GetString(prefs::kAcceptLanguages);
>>      LocalizedError::GetStrings(net::ERR_INTERNET_DISCONNECTED,
>>                                 net::kErrorDomain, url_, false, locale,
>> -                               &error_strings);
>> +                               accept_languages, &error_strings);
>>      resource_id = IDR_OFFLINE_NET_LOAD_HTML;
>>    }
>>
>> Index: chrome/common/localized_error.cc
>> ===================================================================
>> --- chrome/common/localized_error.cc    (revision 225096)
>> +++ chrome/common/localized_error.cc    (working copy)
>> @@ -20,6 +20,7 @@
>>  #include "grit/generated_resources.h"
>>  #include "net/base/escape.h"
>>  #include "net/base/net_errors.h"
>> +#include "net/base/net_util.h"
>>  #include "third_party/WebKit/public/platform/WebURLError.h"
>>  #include "ui/base/l10n/l10n_util.h"
>>  #include "ui/webui/web_ui_util.h"
>> @@ -496,6 +497,7 @@
>>                                  const GURL& failed_url,
>>                                  bool is_post,
>>                                  const std::string& locale,
>> +                                const std::string& accept_languages,
>>                                  base::DictionaryValue* error_strings) {
>>    bool rtl = LocaleIsRTL();
>>    error_strings->SetString("textdirection", rtl ? "rtl" : "ltr");
>> @@ -530,7 +532,9 @@
>>      options.suggestions = SUGGEST_NONE;
>>    }
>>
>> -  string16 failed_url_string(UTF8ToUTF16(failed_url.spec()));
>> +  string16 failed_url_string(net::FormatUrl(failed_url, accept_languages,
>> +      net::kFormatUrlOmitNothing, net::UnescapeRule::NORMAL, NULL, NULL,
>> +      NULL));
>>    // URLs are always LTR.
>>    if (rtl)
>>      base::i18n::WrapStringWithLTRFormatting(&failed_url_string);
>> @@ -545,10 +549,9 @@
>>    base::DictionaryValue* summary = new base::DictionaryValue;
>>    summary->SetString("msg",
>>        l10n_util::GetStringUTF16(options.summary_resource_id));
>> -  // TODO(tc): We want the unicode url and host here since they're being
>> -  //           displayed.
>>    summary->SetString("failedUrl", failed_url_string);
>> -  summary->SetString("hostName", failed_url.host());
>> +  summary->SetString("hostName", net::IDNToUnicode(failed_url.host(),
>> +                                                   accept_languages));
>>    summary->SetString("productName",
>>                       l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));
>>
>> Index: chrome/common/localized_error.h
>> ===================================================================
>> --- chrome/common/localized_error.h     (revision 225096)
>> +++ chrome/common/localized_error.h     (working copy)
>> @@ -33,6 +33,7 @@
>>                           const GURL& failed_url,
>>                           bool is_post,
>>                           const std::string& locale,
>> +                         const std::string& accept_languages,
>>                           base::DictionaryValue* strings);
>>
>>    // Returns a description of the encountered error.
>> Index: chrome/renderer/chrome_content_renderer_client.cc
>> ===================================================================
>> --- chrome/renderer/chrome_content_renderer_client.cc   (revision 225096)
>> +++ chrome/renderer/chrome_content_renderer_client.cc   (working copy)
>> @@ -865,6 +865,7 @@
>>      WebKit::WebFrame* frame,
>>      const WebKit::WebURLRequest& failed_request,
>>      const WebKit::WebURLError& error,
>> +    const std::string& accept_languages,
>>      std::string* error_html,
>>      string16* error_description) {
>>    const GURL failed_url = error.unreachableURL;
>> @@ -891,12 +892,13 @@
>>      } else {
>>        const std::string locale = RenderThread::Get()->GetLocale();
>>        if (!NetErrorHelper::GetErrorStringsForDnsProbe(
>> -              frame, error, is_post, locale, &error_strings)) {
>> +              frame, error, is_post, locale, accept_languages,
>> +              &error_strings)) {
>>          // In most cases, the NetErrorHelper won't provide
>> DNS-probe-specific
>>          // error pages, so fall back to LocalizedError.
>>          LocalizedError::GetStrings(error.reason, error.domain.utf8(),
>>                                     error.unreachableURL, is_post, locale,
>> -                                   &error_strings);
>> +                                   accept_languages, &error_strings);
>>        }
>>        resource_id = IDR_NET_ERROR_HTML;
>>      }
>> Index: chrome/renderer/chrome_content_renderer_client.h
>> ===================================================================
>> --- chrome/renderer/chrome_content_renderer_client.h    (revision 225096)
>> +++ chrome/renderer/chrome_content_renderer_client.h    (working copy)
>> @@ -85,6 +85,7 @@
>>        WebKit::WebFrame* frame,
>>        const WebKit::WebURLRequest& failed_request,
>>        const WebKit::WebURLError& error,
>> +      const std::string& accept_languages,
>>        std::string* error_html,
>>        string16* error_description) OVERRIDE;
>>    virtual void DeferMediaLoad(content::RenderView* render_view,
>> Index: chrome/renderer/net/net_error_helper.cc
>> ===================================================================
>> --- chrome/renderer/net/net_error_helper.cc     (revision 225096)
>> +++ chrome/renderer/net/net_error_helper.cc     (working copy)
>> @@ -174,6 +174,7 @@
>>      const WebKit::WebURLError& error,
>>      bool is_failed_post,
>>      const std::string& locale,
>> +    const std::string& accept_languages,
>>      base::DictionaryValue* error_strings) {
>>    if (!IsMainFrame(frame))
>>      return false;
>> @@ -186,7 +187,7 @@
>>        chrome_common_net::DNS_PROBE_POSSIBLE,
>>        chrome_common_net::kDnsProbeErrorDomain,
>>        error.unreachableURL,
>> -      is_failed_post, locale, error_strings);
>> +      is_failed_post, locale, accept_languages, error_strings);
>>    return true;
>>  }
>>
>> @@ -214,11 +215,14 @@
>>
>>    WebKit::WebURLError error = GetUpdatedError();
>>    base::DictionaryValue error_strings;
>> +  const std::string accept_languages =
>> +      render_view() ? render_view()->GetAcceptLanguages() : std::string();
>>    LocalizedError::GetStrings(error.reason,
>>                               error.domain.utf8(),
>>                               error.unreachableURL,
>>                               is_failed_post_,
>>                               RenderThread::Get()->GetLocale(),
>> +                             accept_languages,
>>                               &error_strings);
>>
>>    std::string json;
>> Index: chrome/renderer/net/net_error_helper.h
>> ===================================================================
>> --- chrome/renderer/net/net_error_helper.h      (revision 225096)
>> +++ chrome/renderer/net/net_error_helper.h      (working copy)
>> @@ -50,6 +50,7 @@
>>        const WebKit::WebURLError& error,
>>        bool is_failed_post,
>>        const std::string& locale,
>> +      const std::string& accept_languages,
>>        base::DictionaryValue* error_strings);
>>
>>   protected:
>> Index: content/public/renderer/content_renderer_client.h
>> ===================================================================
>> --- content/public/renderer/content_renderer_client.h   (revision 225096)
>> +++ content/public/renderer/content_renderer_client.h   (working copy)
>> @@ -113,6 +113,7 @@
>>        WebKit::WebFrame* frame,
>>        const WebKit::WebURLRequest& failed_request,
>>        const WebKit::WebURLError& error,
>> +      const std::string& accept_languages,
>>        std::string* error_html,
>>        string16* error_description) {}
>>
>> Index: content/public/renderer/render_view.h
>> ===================================================================
>> --- content/public/renderer/render_view.h       (revision 225096)
>> +++ content/public/renderer/render_view.h       (working copy)
>> @@ -5,6 +5,8 @@
>>  #ifndef CONTENT_PUBLIC_RENDERER_RENDER_VIEW_H_
>>  #define CONTENT_PUBLIC_RENDERER_RENDER_VIEW_H_
>>
>> +#include <string>
>> +
>>  #include "base/basictypes.h"
>>  #include "base/strings/string16.h"
>>  #include "content/common/content_export.h"
>> @@ -163,6 +165,9 @@
>>    // Returns a collection of security info about |frame|.
>>    virtual SSLStatus GetSSLStatusOfFrame(WebKit::WebFrame* frame) const = 0;
>>
>> +  // Returns |renderer_preferences_.accept_languages| value.
>> +  virtual const std::string& GetAcceptLanguages() const = 0;
>> +
>>  #if defined(OS_ANDROID)
>>    virtual void UpdateTopControlsState(TopControlsState constraints,
>>                                        TopControlsState current,
>> Index: content/renderer/render_view_impl.cc
>> ===================================================================
>> --- content/renderer/render_view_impl.cc        (revision 225096)
>> +++ content/renderer/render_view_impl.cc        (working copy)
>> @@ -133,6 +133,7 @@
>>  #include "net/base/data_url.h"
>>  #include "net/base/escape.h"
>>  #include "net/base/net_errors.h"
>> +#include "net/base/net_util.h"
>>  #include
>> "net/base/registry_controlled_domains/registry_controlled_domain.h"
>>  #include "net/http/http_util.h"
>>  #include "third_party/WebKit/public/platform/WebCString.h"
>> @@ -2199,7 +2200,8 @@
>>      error_html = &html;
>>    } else {
>>      GetContentClient()->renderer()->GetNavigationErrorStrings(
>> -        frame, failed_request, error, &alt_html, NULL);
>> +        frame, failed_request, error,
>> renderer_preferences_.accept_languages,
>> +        &alt_html, NULL);
>>      error_html = &alt_html;
>>    }
>>
>> @@ -3134,6 +3136,10 @@
>>    return ssl_status;
>>  }
>>
>> +const std::string& RenderViewImpl::GetAcceptLanguages() const {
>> +  return renderer_preferences_.accept_languages;
>> +}
>> +
>>  WebNavigationPolicy RenderViewImpl::decidePolicyForNavigation(
>>      WebFrame* frame, WebDataSource::ExtraData* extraData,
>>      const WebURLRequest& request, WebNavigationType type,
>> @@ -3623,6 +3629,7 @@
>>        frame,
>>        failed_request,
>>        error,
>> +      renderer_preferences_.accept_languages,
>>        NULL,
>>        &params.error_description);
>>    params.url = error.unreachableURL;
>> @@ -3869,6 +3876,7 @@
>>        frame,
>>        failed_request,
>>        error,
>> +      renderer_preferences_.accept_languages,
>>        NULL,
>>        &error_description);
>>    Send(new ViewHostMsg_DidFailLoadWithError(routing_id_,
>> @@ -4565,7 +4573,9 @@
>>    remove_params.ClearQuery();
>>    remove_params.ClearRef();
>>    const GURL url_to_send = failed_url.ReplaceComponents(remove_params);
>> -  std::string spec_to_send = url_to_send.spec();
>> +  std::string spec_to_send = UTF16ToUTF8(net::FormatUrl(url_to_send,
>> +      renderer_preferences_.accept_languages, net::kFormatUrlOmitNothing,
>> +      net::UnescapeRule::NORMAL, NULL, NULL, NULL));
>>    // Notify link doctor of the url truncation by sending of "?" at the end.
>>    if (failed_url.has_query())
>>      spec_to_send.append("?");
>> Index: content/renderer/render_view_impl.h
>> ===================================================================
>> --- content/renderer/render_view_impl.h (revision 225096)
>> +++ content/renderer/render_view_impl.h (working copy)
>> @@ -697,6 +697,7 @@
>>                                               const std::string& value)
>> OVERRIDE;
>>    virtual void ClearEditCommands() OVERRIDE;
>>    virtual SSLStatus GetSSLStatusOfFrame(WebKit::WebFrame* frame) const
>> OVERRIDE;
>> +  virtual const std::string& GetAcceptLanguages() const OVERRIDE;
>>  #if defined(OS_ANDROID)
>>    virtual void UpdateTopControlsState(TopControlsState constraints,
>>                                        TopControlsState current,
>>
>>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2013-09-26 16:23:01]

I think we should add a test for this - test will be ugly, but I think it's
still worth doing, as internationalization regressions can easily go overlooked.

Tests for the error pages are in chrome/browser/errorpage_browsertest.cc.  I'm
thinking something like "LinkDoctorFail" (Which looks at the textContent of a
page), except without link doctor and a URL that uses UTF8 characters in the
host name.

Look at content/test/net/url_request_failed_job.  We can reuse that class, but
we have to add a filter to return it for some punycoded hostname (Rather than
modify URLRequestFailedJob::AddUrlHandler, this should probably done with a new
function, just for this test.  This must be called on the IO thread).  Then we
navigate to the error page, and make sure the unicode version of the URL is in
the TextContent (It may not by in the text of the page, but it will be in the
Javascript code we generate).

Describing how to set up the test makes it sound more difficult than it is.  I'm
happy to talk more about it.

https://codereview.chromium.org/24503004/diff/12001/chrome/common/localized_e...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/24503004/diff/12001/chrome/common/localized_e...
chrome/common/localized_error.cc:535: string16
failed_url_string(net::FormatUrl(failed_url, accept_languages,
Believe this formatting violates the style guide.  Each line with parameters to
the same function should be lined up.  Just move the first two parameters to the
next line to fix.

https://codereview.chromium.org/24503004/diff/12001/chrome/renderer/net/net_e...
File chrome/renderer/net/net_error_helper.cc (right):

https://codereview.chromium.org/24503004/diff/12001/chrome/renderer/net/net_e...
chrome/renderer/net/net_error_helper.cc:219: render_view() ?
render_view()->GetAcceptLanguages() : std::string();
The test that render_view() isn't NULL isn't needed - this object lasts for
exactly the lifetime of the RenderView passed into its cosntructor.

https://codereview.chromium.org/24503004/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/24503004/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:4578: net::UnescapeRule::NORMAL, NULL,
NULL, NULL));
Believe this formatting violates the style guide.  Each line with parameters to
the same function should be lined up.  Just move "url_to_send," to the next line
to fix.

https://codereview.chromium.org/24503004/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:4578: net::UnescapeRule::NORMAL, NULL,
NULL, NULL));
I've reached out to the Link Doctor team to make sure this is fine on their end.
 Hope to hear back later today.

[mmenke, 2013-09-26 16:27:16]

Oh, and to avoid triggering the link doctor in the test, just return a different
error code, like ERR_UNSAFE_PORT.

On 2013/09/26 16:23:01, mmenke wrote:
> I think we should add a test for this - test will be ugly, but I think it's
> still worth doing, as internationalization regressions can easily go
overlooked.
> 
> Tests for the error pages are in chrome/browser/errorpage_browsertest.cc.  I'm
> thinking something like "LinkDoctorFail" (Which looks at the textContent of a
> page), except without link doctor and a URL that uses UTF8 characters in the
> host name.
> 
> Look at content/test/net/url_request_failed_job.  We can reuse that class, but
> we have to add a filter to return it for some punycoded hostname (Rather than
> modify URLRequestFailedJob::AddUrlHandler, this should probably done with a
new
> function, just for this test.  This must be called on the IO thread).  Then we
> navigate to the error page, and make sure the unicode version of the URL is in
> the TextContent (It may not by in the text of the page, but it will be in the
> Javascript code we generate).
> 
> Describing how to set up the test makes it sound more difficult than it is. 
I'm
> happy to talk more about it.
> 
>
https://codereview.chromium.org/24503004/diff/12001/chrome/common/localized_e...
> File chrome/common/localized_error.cc (right):
> 
>
https://codereview.chromium.org/24503004/diff/12001/chrome/common/localized_e...
> chrome/common/localized_error.cc:535: string16
> failed_url_string(net::FormatUrl(failed_url, accept_languages,
> Believe this formatting violates the style guide.  Each line with parameters
to
> the same function should be lined up.  Just move the first two parameters to
the
> next line to fix.
> 
>
https://codereview.chromium.org/24503004/diff/12001/chrome/renderer/net/net_e...
> File chrome/renderer/net/net_error_helper.cc (right):
> 
>
https://codereview.chromium.org/24503004/diff/12001/chrome/renderer/net/net_e...
> chrome/renderer/net/net_error_helper.cc:219: render_view() ?
> render_view()->GetAcceptLanguages() : std::string();
> The test that render_view() isn't NULL isn't needed - this object lasts for
> exactly the lifetime of the RenderView passed into its cosntructor.
> 
>
https://codereview.chromium.org/24503004/diff/12001/content/renderer/render_v...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/24503004/diff/12001/content/renderer/render_v...
> content/renderer/render_view_impl.cc:4578: net::UnescapeRule::NORMAL, NULL,
> NULL, NULL));
> Believe this formatting violates the style guide.  Each line with parameters
to
> the same function should be lined up.  Just move "url_to_send," to the next
line
> to fix.
> 
>
https://codereview.chromium.org/24503004/diff/12001/content/renderer/render_v...
> content/renderer/render_view_impl.cc:4578: net::UnescapeRule::NORMAL, NULL,
> NULL, NULL));
> I've reached out to the Link Doctor team to make sure this is fine on their
end.
>  Hope to hear back later today.

[mmenke, 2013-09-26 18:28:22]

https://codereview.chromium.org/24503004/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/24503004/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:4578: net::UnescapeRule::NORMAL, NULL,
NULL, NULL));
On 2013/09/26 16:23:01, mmenke wrote:
> I've reached out to the Link Doctor team to make sure this is fine on their
end.
>  Hope to hear back later today.

After talking to the Link Doctor team, it looks like Link Doctor really does not
handle IDN domain names well, but it works better with the punycode versions, so
let's leave this as it was before, for now.  :(

[yuusuke, 2013-09-27 14:16:51]

Hello, mmenke!

I've addressed all comments, including request for new browsertest. Hope I
understood you well and have done it right. :) Plz continue the review.

https://codereview.chromium.org/24503004/diff/12001/chrome/common/localized_e...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/24503004/diff/12001/chrome/common/localized_e...
chrome/common/localized_error.cc:535: string16
failed_url_string(net::FormatUrl(failed_url, accept_languages,
On 2013/09/26 16:23:01, mmenke wrote:
> Believe this formatting violates the style guide.  Each line with parameters
to
> the same function should be lined up.  Just move the first two parameters to
the
> next line to fix.

Done.

https://codereview.chromium.org/24503004/diff/12001/chrome/renderer/net/net_e...
File chrome/renderer/net/net_error_helper.cc (right):

https://codereview.chromium.org/24503004/diff/12001/chrome/renderer/net/net_e...
chrome/renderer/net/net_error_helper.cc:219: render_view() ?
render_view()->GetAcceptLanguages() : std::string();
On 2013/09/26 16:23:01, mmenke wrote:
> The test that render_view() isn't NULL isn't needed - this object lasts for
> exactly the lifetime of the RenderView passed into its cosntructor.

Done.

https://codereview.chromium.org/24503004/diff/12001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/24503004/diff/12001/content/renderer/render_v...
content/renderer/render_view_impl.cc:4578: net::UnescapeRule::NORMAL, NULL,
NULL, NULL));
On 2013/09/26 18:28:22, mmenke wrote:
> On 2013/09/26 16:23:01, mmenke wrote:
> > I've reached out to the Link Doctor team to make sure this is fine on their
> end.
> >  Hope to hear back later today.
> 
> After talking to the Link Doctor team, it looks like Link Doctor really does
not
> handle IDN domain names well, but it works better with the punycode versions,
so
> let's leave this as it was before, for now.  :(

Done.

[mmenke, 2013-09-27 14:44:04]

Thanks for the test - looks great!  Just a couple nits.

https://codereview.chromium.org/24503004/diff/32001/chrome/browser/errorpage_...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/24503004/diff/32001/chrome/browser/errorpage_...
chrome/browser/errorpage_browsertest.cc:256: // Returns JavaScrips code that
executes plain text search for the page.
nit:  Javascript

https://codereview.chromium.org/24503004/diff/32001/chrome/browser/errorpage_...
chrome/browser/errorpage_browsertest.cc:263:
"domAutomationController.send(hasError);", value_to_search.c_str());
nit:  To make the second parameter stand out a bit more, suggest putting it on
its own line.

https://codereview.chromium.org/24503004/diff/32001/chrome/browser/errorpage_...
chrome/browser/errorpage_browsertest.cc:346: // A test fixture that substitutes
kMockHostname with IDN.
nit:  Think it's ambiguous what "kMockHostname" is.  Maybe "A test fixture that
simulates failing requests for an IDN domain name."?

https://codereview.chromium.org/24503004/diff/32001/chrome/browser/errorpage_...
chrome/browser/errorpage_browsertest.cc:373: }
optional nit:  Suggest just making these consts, it's the much more common style
in Chrome.  Just something like:

static const char kHostnamePunycode[] = "xn--d1abbgf6aiiy.xn--p1ai";
static const char kHostnameJSUnicode[] = ....

https://codereview.chromium.org/24503004/diff/32001/chrome/browser/errorpage_...
chrome/browser/errorpage_browsertest.cc:389:
ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
Think it's worth a comment that ERR_UNSAFE_PORT will not trigger the link
doctor.

https://codereview.chromium.org/24503004/diff/32001/content/test/net/url_requ...
File content/test/net/url_request_failed_job.h (right):

https://codereview.chromium.org/24503004/diff/32001/content/test/net/url_requ...
content/test/net/url_request_failed_job.h:29: static void AddUrlHandler(const
std::string& hostname);
Function overloading, while done a fair bit in Chrome, is generally discouraged.
 I suggest renaming to something like "AddUrlHandlerForHostname".

https://codereview.chromium.org/24503004/diff/32001/content/test/net/url_requ...
content/test/net/url_request_failed_job.h:36: static GURL GetMockHttpUrl(int
net_error, const std::string& hostname);
Suggest a comment above this line along the lines of "URLRequestFailedJob must
be added as a handler for |hostname| for the returned URL to return
|net_error|."

https://codereview.chromium.org/24503004/diff/32001/content/test/net/url_requ...
content/test/net/url_request_failed_job.h:37: static GURL GetMockHttpsUrl(int
net_error, const std::string& hostname);
Again, suggest adding a "ForHostname" to the end of these two.  Think it also
makes code a little clearer, despite the fact it just duplicates an argument
name.

[yuusuke, 2013-09-28 10:42:49]

Hello, mmenke!

All nits addressed. Are there any other reviewers who should participate in this
patch?

Also, I've made some changes to file in chromeos folder but wasn't able to check
if it works or at least compiles. Could anyone "trybot" this patch for chromeos
for me?

https://codereview.chromium.org/24503004/diff/32001/chrome/browser/errorpage_...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/24503004/diff/32001/chrome/browser/errorpage_...
chrome/browser/errorpage_browsertest.cc:256: // Returns JavaScrips code that
executes plain text search for the page.
On 2013/09/27 14:44:05, mmenke wrote:
> nit:  Javascript

Done.

https://codereview.chromium.org/24503004/diff/32001/chrome/browser/errorpage_...
chrome/browser/errorpage_browsertest.cc:263:
"domAutomationController.send(hasError);", value_to_search.c_str());
On 2013/09/27 14:44:05, mmenke wrote:
> nit:  To make the second parameter stand out a bit more, suggest putting it on
> its own line.

Done.

https://codereview.chromium.org/24503004/diff/32001/chrome/browser/errorpage_...
chrome/browser/errorpage_browsertest.cc:346: // A test fixture that substitutes
kMockHostname with IDN.
On 2013/09/27 14:44:05, mmenke wrote:
> nit:  Think it's ambiguous what "kMockHostname" is.  Maybe "A test fixture
that
> simulates failing requests for an IDN domain name."?

Done.

https://codereview.chromium.org/24503004/diff/32001/chrome/browser/errorpage_...
chrome/browser/errorpage_browsertest.cc:373: }
On 2013/09/27 14:44:05, mmenke wrote:
> optional nit:  Suggest just making these consts, it's the much more common
style
> in Chrome.  Just something like:
> 
> static const char kHostnamePunycode[] = "xn--d1abbgf6aiiy.xn--p1ai";
> static const char kHostnameJSUnicode[] = ....

Done.

https://codereview.chromium.org/24503004/diff/32001/chrome/browser/errorpage_...
chrome/browser/errorpage_browsertest.cc:389:
ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
On 2013/09/27 14:44:05, mmenke wrote:
> Think it's worth a comment that ERR_UNSAFE_PORT will not trigger the link
> doctor.

Done.

https://codereview.chromium.org/24503004/diff/32001/content/test/net/url_requ...
File content/test/net/url_request_failed_job.h (right):

https://codereview.chromium.org/24503004/diff/32001/content/test/net/url_requ...
content/test/net/url_request_failed_job.h:29: static void AddUrlHandler(const
std::string& hostname);
On 2013/09/27 14:44:05, mmenke wrote:
> Function overloading, while done a fair bit in Chrome, is generally
discouraged.
>  I suggest renaming to something like "AddUrlHandlerForHostname".

Done.

https://codereview.chromium.org/24503004/diff/32001/content/test/net/url_requ...
content/test/net/url_request_failed_job.h:36: static GURL GetMockHttpUrl(int
net_error, const std::string& hostname);
On 2013/09/27 14:44:05, mmenke wrote:
> Suggest a comment above this line along the lines of "URLRequestFailedJob must
> be added as a handler for |hostname| for the returned URL to return
> |net_error|."

Done.

https://codereview.chromium.org/24503004/diff/32001/content/test/net/url_requ...
content/test/net/url_request_failed_job.h:37: static GURL GetMockHttpsUrl(int
net_error, const std::string& hostname);
On 2013/09/27 14:44:05, mmenke wrote:
> Again, suggest adding a "ForHostname" to the end of these two.  Think it also
> makes code a little clearer, despite the fact it just duplicates an argument
> name.

Done.

[mmenke, 2013-09-28 16:15:56]

LGTM.

You have owner signoffs for content/public (Joi) and content/net (me).

So you still need owners for:

content/renderer/
android_webview/
chrome/

thestig@chromium.org has done a lot of error page reviews for chrome/. 
android_webview is just a simple rubberstamp review, so think any owner will do.
 I'm not sure who'd be best for the content/render change - I'd guess say darin,
who you already have as a reviewer, or jamesr, but I have no confidence in that.

https://codereview.chromium.org/24503004/diff/57001/chrome/browser/errorpage_...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/24503004/diff/57001/chrome/browser/errorpage_...
chrome/browser/errorpage_browsertest.cc:368: static const char
kHostnameJSUnicode[];
nit:  Static constants should go before function declarations

[boliu, 2013-09-28 16:30:05]

android_webview/ lgtm stamp

[yuusuke, 2013-09-28 18:56:56]

darin, jamesr, Lei Zhang,
please review remaining folders/files.

mmenke,
please click "try more bots" for the latest patch :)

https://codereview.chromium.org/24503004/diff/57001/chrome/browser/errorpage_...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/24503004/diff/57001/chrome/browser/errorpage_...
chrome/browser/errorpage_browsertest.cc:368: static const char
kHostnameJSUnicode[];
On 2013/09/28 16:15:56, mmenke wrote:
> nit:  Static constants should go before function declarations

Done.

[Lei Zhang, 2013-09-30 03:46:55]

chrome/ lgtm

[yuusuke, 2013-10-01 08:28:21]

Ping...
Still need review for content/renderer/ folder.
Anyone?

[yuusuke, 2013-10-03 07:50:57]

jamesr, jochen, darin, brettw,
please review remaining content/public/ folder.

Best regards,
Artem Kuznetcov

[jochen (gone - plz use gerrit), 2013-10-04 08:18:22]

lgtm

content/renderer lgtm

[commit-bot: I haz the power, 2013-10-04 09:08:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yuusuke@yandex-team.ru/24503004/85001

[commit-bot: I haz the power, 2013-10-04 09:59:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[yuusuke, 2013-10-04 13:06:32]

Hello, everyone!

I had to rebase the patch and make trivial change (|render_view_browsertest.cc|
file) to fix compilation.

Is it OK to just hit "commit" or do I need all 5 approvals once again?

Best regards,
Artem Kuznetcov

[jochen (gone - plz use gerrit), 2013-10-04 13:08:00]

yes, that's fine

[commit-bot: I haz the power, 2013-10-04 13:38:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yuusuke@yandex-team.ru/24503004/128001

[commit-bot: I haz the power, 2013-10-04 16:01:41]

Change committed as 227014