[Editing][CodeHealth] Improve result comparing in assert_selection

Improve result comparing in assert_selection

That's because assert_selection often compares long HTML fragment.

TEST=Layouttests\editing\assert_selection.html

Committed: https://crrev.com/8466c81e38957bf0075c87e94403d963904492cd
Cr-Commit-Position: refs/heads/master@{#404801}

[yoichio, 2016-07-11 05:18:35]

Description was changed from

==========
improve result compare in assert_selection

BUG=
==========

to

==========
Improve result comparing in assert_selection

That's because assert_selection often compares long HTML fragment.

TEST=Layouttests\editing\assert_selection.html
==========

[yoichio, 2016-07-11 05:18:35]

yoichio@chromium.org changed reviewers:
+ yosin@chromium.org

[yosin_UTC9, 2016-07-11 06:48:18]

Good idea!

https://codereview.chromium.org/2138653002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/editing/assert_selection.js (right):

https://codereview.chromium.org/2138653002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/editing/assert_selection.js:659: var
comparedLength = Math.min(actualText.length, expectedText.length);
Could you introduce mismatch(str1, str2) and mismatchLast(str1, str2) to cut
both end?

[yoichio, 2016-07-11 09:04:14]

https://codereview.chromium.org/2138653002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/editing/assert_selection.js (right):

https://codereview.chromium.org/2138653002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/editing/assert_selection.js:659: var
comparedLength = Math.min(actualText.length, expectedText.length);
On 2016/07/11 06:48:18, Yosi_UTC9 wrote:
> Could you introduce mismatch(str1, str2) and mismatchLast(str1, str2) to cut
> both end?

Done.

[yosin_UTC9, 2016-07-12 01:58:36]

https://codereview.chromium.org/2138653002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/editing/assert_selection.js (right):

https://codereview.chromium.org/2138653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/assert_selection.js:634: */
Please add
@return {string}

https://codereview.chromium.org/2138653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/assert_selection.js:637: var i = 0;
nit: Since out of bound access of Array returns |undefined|,
we could write:

for (let index = 0; index < str1.length; ++index) {
  if (str1[index] !== str2[index])
    return str1.substr(0, index);
}
return str1;

Note: Use |let| instead of |var|

[yosin_UTC9, 2016-07-12 02:26:54]

https://codereview.chromium.org/2138653002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/editing/assert_selection.js (right):

https://codereview.chromium.org/2138653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/assert_selection.js:635: function
commonSubstr(str1, str2) {
This function should be named |commonPrefixOf()| as it does.

I actually want to have |CreateUnifiedDiff()| in gtest.
Could you put TODO for this? I think it is good for intern project. ;-)
https://cs.chromium.org/chromium/src/testing/gtest/src/gtest.cc?q=CreateUnifi...

Yet, another option is 
https://en.wikipedia.org/wiki/Longest_common_substring_problem

It is easier but diff version is more useful.

[yoichio, 2016-07-12 04:11:06]

Patchset #4 (id:60001) has been deleted

[yoichio, 2016-07-12 04:11:11]

Patchset #3 (id:40001) has been deleted

[yoichio, 2016-07-12 04:12:31]

https://codereview.chromium.org/2138653002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/editing/assert_selection.js (right):

https://codereview.chromium.org/2138653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/assert_selection.js:634: */
On 2016/07/12 01:58:36, Yosi_UTC9 wrote:
> Please add
> @return {string}

Done.

https://codereview.chromium.org/2138653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/assert_selection.js:635: function
commonSubstr(str1, str2) {
On 2016/07/12 02:26:54, Yosi_UTC9 wrote:
> This function should be named |commonPrefixOf()| as it does.
> 
> I actually want to have |CreateUnifiedDiff()| in gtest.
> Could you put TODO for this? I think it is good for intern project. ;-)
>
https://cs.chromium.org/chromium/src/testing/gtest/src/gtest.cc?q=CreateUnifi...
> 
> Yet, another option is 
> https://en.wikipedia.org/wiki/Longest_common_substring_problem
> 
> It is easier but diff version is more useful.
> 
> 

Done.

https://codereview.chromium.org/2138653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/assert_selection.js:637: var i = 0;
On 2016/07/12 01:58:36, Yosi_UTC9 wrote:
> nit: Since out of bound access of Array returns |undefined|,
> we could write:
> 
> for (let index = 0; index < str1.length; ++index) {
>   if (str1[index] !== str2[index])
>     return str1.substr(0, index);
> }
> return str1;
> 
> Note: Use |let| instead of |var|

Done.

[yosin_UTC9, 2016-07-12 04:15:12]

lgtm

https://codereview.chromium.org/2138653002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/editing/assert_selection.js (right):

https://codereview.chromium.org/2138653002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/assert_selection.js:670: sample.remove();
nit: early return is better.

[yoichio, 2016-07-12 04:24:50]

https://codereview.chromium.org/2138653002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/editing/assert_selection.js (right):

https://codereview.chromium.org/2138653002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/editing/assert_selection.js:670: sample.remove();
On 2016/07/12 04:15:12, Yosi_UTC9 wrote:
> nit: early return is better.

Done.

[yoichio, 2016-07-12 04:24:55]

The CQ bit was checked by yoichio@chromium.org

[yoichio, 2016-07-12 04:24:55]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2138653002/#ps100001
(title: "update")

[commit-bot: I haz the power, 2016-07-12 04:25:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 05:54:17]

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

[commit-bot: I haz the power, 2016-07-12 05:54:18]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yosin_UTC9, 2016-07-12 07:04:32]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2016-07-12 07:05:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 10:00:07]

Description was changed from

==========
Improve result comparing in assert_selection

That's because assert_selection often compares long HTML fragment.

TEST=Layouttests\editing\assert_selection.html
==========

to

==========
Improve result comparing in assert_selection

That's because assert_selection often compares long HTML fragment.

TEST=Layouttests\editing\assert_selection.html
==========

[commit-bot: I haz the power, 2016-07-12 10:00:10]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-07-12 10:00:33]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-12 10:02:01]

Description was changed from

==========
Improve result comparing in assert_selection

That's because assert_selection often compares long HTML fragment.

TEST=Layouttests\editing\assert_selection.html
==========

to

==========
Improve result comparing in assert_selection

That's because assert_selection often compares long HTML fragment.

TEST=Layouttests\editing\assert_selection.html

Committed: https://crrev.com/8466c81e38957bf0075c87e94403d963904492cd
Cr-Commit-Position: refs/heads/master@{#404801}
==========

[commit-bot: I haz the power, 2016-07-12 10:02:02]

Patchset 4 (id:??) landed as
https://crrev.com/8466c81e38957bf0075c87e94403d963904492cd
Cr-Commit-Position: refs/heads/master@{#404801}