Extra parameter to pass sub-message direction for validation message

Extra parameter to pass sub-message direction for validation message

Main message and sub message direction of the validation message can differ. Pass direction of the subMsg to allow text display accordingly.

BUG=91638
R=jam, jamesr

Committed: https://crrev.com/597255e67d2d627ad17d8d313c8aa47d5dc7bd6c
Cr-Commit-Position: refs/heads/master@{#293819}

[Habib Virji, 2014-07-15 16:51:12]

Part of the changes in blink.

Blink CL: https://codereview.chromium.org/389343002/

[jam, 2014-08-18 17:55:51]

see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

https://codereview.chromium.org/391923006/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/391923006/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.cc:1902: if (main_text_hint ==
blink::WebTextDirectionLeftToRight)
nit: don't remove the brace brackets here or below. see the google style guide

https://codereview.chromium.org/391923006/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/web_test_proxy.cc (right):

https://codereview.chromium.org/391923006/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/web_test_proxy.cc:388: if (message_dir ==
blink::WebTextDirectionLeftToRight)
nit: need brace brackets

[Habib Virji, 2014-08-20 09:15:55]

@jam thanks updated to correct coding style. Can you please take a look.

https://codereview.chromium.org/391923006/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/391923006/diff/20001/content/renderer/render_...
content/renderer/render_view_impl.cc:1902: if (main_text_hint ==
blink::WebTextDirectionLeftToRight)
On 2014/08/18 17:55:51, jam wrote:
> nit: don't remove the brace brackets here or below. see the google style guide

Done.

https://codereview.chromium.org/391923006/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/web_test_proxy.cc (right):

https://codereview.chromium.org/391923006/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/web_test_proxy.cc:388: if (message_dir ==
blink::WebTextDirectionLeftToRight)
On 2014/08/18 17:55:51, jam wrote:
> nit: need brace brackets

Done.

[jam, 2014-08-21 17:32:52]

https://codereview.chromium.org/391923006/diff/40001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/391923006/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.cc:1900: && !base::i18n::IsRTL()) {
nit: indent 4 more here. also put "&&" at the start of the previous line

https://codereview.chromium.org/391923006/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.cc:1901:
base::i18n::WrapStringWithRTLFormatting(&wrapped_main_text);
this should be a 2 space indent from the brace bracket and not 4

https://codereview.chromium.org/391923006/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.cc:1907:
base::i18n::GetDisplayStringInLTRDirectionality(wrapped_sub_text);
4 space indent from wrapped and not 2

https://codereview.chromium.org/391923006/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/web_test_proxy.cc (right):

https://codereview.chromium.org/391923006/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/web_test_proxy.cc:386:
base::i18n::GetDisplayStringInLTRDirectionality(wrapped_main_text);
more indent

https://codereview.chromium.org/391923006/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/web_test_proxy.cc:394:
base::i18n::GetDisplayStringInLTRDirectionality(wrapped_sub_text);
ditto

[Habib Virji, 2014-08-26 08:55:45]

Thanks, updated. 
Only thing not clear was how many spaces, should it be in "if else" for next
line scenario. I have tried indenting it to match if statement.

https://codereview.chromium.org/391923006/diff/40001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/391923006/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.cc:1900: && !base::i18n::IsRTL()) {
On 2014/08/21 17:32:51, jam wrote:
> nit: indent 4 more here. also put "&&" at the start of the previous line

Done.

https://codereview.chromium.org/391923006/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.cc:1901:
base::i18n::WrapStringWithRTLFormatting(&wrapped_main_text);
On 2014/08/21 17:32:51, jam wrote:
> this should be a 2 space indent from the brace bracket and not 4

Done.

https://codereview.chromium.org/391923006/diff/40001/content/renderer/render_...
content/renderer/render_view_impl.cc:1907:
base::i18n::GetDisplayStringInLTRDirectionality(wrapped_sub_text);
On 2014/08/21 17:32:51, jam wrote:
> 4 space indent from wrapped and not 2

Done.

https://codereview.chromium.org/391923006/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/web_test_proxy.cc (right):

https://codereview.chromium.org/391923006/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/web_test_proxy.cc:386:
base::i18n::GetDisplayStringInLTRDirectionality(wrapped_main_text);
On 2014/08/21 17:32:52, jam wrote:
> more indent

Done.

https://codereview.chromium.org/391923006/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/web_test_proxy.cc:394:
base::i18n::GetDisplayStringInLTRDirectionality(wrapped_sub_text);
On 2014/08/21 17:32:52, jam wrote:
> ditto

Done.

[Habib Virji, 2014-08-29 08:59:31]

habib.virji@samsung.com changed reviewers:
+ jamesr@chromium.org
- jochen@chromium.org, kalman@chromium.org, kenrb@chromium.org

[Habib Virji, 2014-08-29 10:05:51]

@jam/jamesr: Blink side code has landed
(https://codereview.chromium.org/389343002/), can you please review the
corresponding chromium side changes. Thanks.

[jamesr, 2014-08-29 17:21:50]

lgtm

https://codereview.chromium.org/391923006/diff/100001/content/shell/renderer/...
File content/shell/renderer/test_runner/web_test_proxy.cc (right):

https://codereview.chromium.org/391923006/diff/100001/content/shell/renderer/...
content/shell/renderer/test_runner/web_test_proxy.cc:391: if (message_dir ==
blink::WebTextDirectionLeftToRight) {
this looks very repetitive. is there any way to share this logic with the
render_view_impl version?

[Habib Virji, 2014-09-01 11:09:31]

On 2014/08/29 17:21:50, jamesr wrote:
> lgtm
> 
>
https://codereview.chromium.org/391923006/diff/100001/content/shell/renderer/...
> File content/shell/renderer/test_runner/web_test_proxy.cc (right):
> 
>
https://codereview.chromium.org/391923006/diff/100001/content/shell/renderer/...
> content/shell/renderer/test_runner/web_test_proxy.cc:391: if (message_dir ==
> blink::WebTextDirectionLeftToRight) {
> this looks very repetitive. is there any way to share this logic with the
> render_view_impl version?

@james: Thanks for reviewing.

Have added a new function setValidationMessageDirection in RenderViewImpl to
avoid duplication. Can you please have a look.

[Habib Virji, 2014-09-02 08:20:25]

habib.virji@samsung.com changed reviewers:
+ jochen@chromium.org
- darin@chromium.org

[jamesr, 2014-09-02 19:14:56]

almost!

https://codereview.chromium.org/391923006/diff/120001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/391923006/diff/120001/content/renderer/render...
content/renderer/render_view_impl.h:386: void
setValidationMessageDirection(base::string16& main_text,
this is a chromium-style function so it should have an uppercase first letter

https://codereview.chromium.org/391923006/diff/120001/content/renderer/render...
content/renderer/render_view_impl.h:387: blink::WebTextDirection main_text_hint,
alignment is off

https://codereview.chromium.org/391923006/diff/120001/content/renderer/render...
content/renderer/render_view_impl.h:388: base::string16& sub_text,
we don't use non-const refs in chromium code.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu....
 If you want to modify the parameter pass it by pointer, if you don't pass it by
const ref

[Habib Virji, 2014-09-03 10:16:54]

Thanks updated, using pointer as need to update values.

https://codereview.chromium.org/391923006/diff/120001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/391923006/diff/120001/content/renderer/render...
content/renderer/render_view_impl.h:386: void
setValidationMessageDirection(base::string16& main_text,
On 2014/09/02 19:14:56, jamesr wrote:
> this is a chromium-style function so it should have an uppercase first letter

Done.

https://codereview.chromium.org/391923006/diff/120001/content/renderer/render...
content/renderer/render_view_impl.h:387: blink::WebTextDirection main_text_hint,
On 2014/09/02 19:14:56, jamesr wrote:
> alignment is off

Done.

https://codereview.chromium.org/391923006/diff/120001/content/renderer/render...
content/renderer/render_view_impl.h:388: base::string16& sub_text,
On 2014/09/02 19:14:56, jamesr wrote:
> we don't use non-const refs in chromium code.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu....
>  If you want to modify the parameter pass it by pointer, if you don't pass it
by
> const ref

Done.

[Habib Virji, 2014-09-05 08:30:57]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-09-05 08:32:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/habib.virji@samsung.com/391923006/160001

[commit-bot: I haz the power, 2014-09-05 08:41:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-05 08:41:20]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/56900)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Habib Virji, 2014-09-06 04:45:59]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-09-06 04:47:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/habib.virji@samsung.com/391923006/180001

[commit-bot: I haz the power, 2014-09-06 05:00:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-06 05:00:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Habib Virji, 2014-09-08 09:24:46]

habib.virji@samsung.com changed reviewers:
+ pfeldman@chromium.org, tkent@chromium.org
- jochen@chromium.org

[Habib Virji, 2014-09-08 09:26:20]

@tkent/pfeldman: render_view_impl changes was okayed by James, but
web_test_proxy changes needs your approval too.

[tkent, 2014-09-08 23:20:42]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-09-08 23:20:43]

lgtm

[commit-bot: I haz the power, 2014-09-08 23:29:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/habib.virji@samsung.com/391923006/180001

[commit-bot: I haz the power, 2014-09-09 00:32:05]

Committed patchset #10 (id:180001) as 7d5db93341b8ee2ff07aefbf6e16c444fea872c6

[commit-bot: I haz the power, 2014-09-10 03:49:11]

Patchset 10 (id:??) landed as
https://crrev.com/597255e67d2d627ad17d8d313c8aa47d5dc7bd6c
Cr-Commit-Position: refs/heads/master@{#293819}