Move stringify, form, navigation and scroll methods out of core.js.

Move stringify, form, navigation and scroll methods out of core.js.

- stringify() is a standalone utility so it is moved to common.js.
- form.js adds listeners to handle forms.
- navigation.js contains __gCrWeb APIs for managing navigation history.
- scroll_workaround.js isolates the workaround for crbug.com/554257.

Corresponding unit tests in core_js_unittest are also extracted.
- common_js_unittest.mm: fixed a bug in stringify(undefined), added test
  case and removed TODO.

BUG=614092
TESTED=Ran ios_web_unittests, and manually verified in simulator that
the scroll workaround still works.

Review-Url: https://codereview.chromium.org/2807213003
Cr-Commit-Position: refs/heads/master@{#463691}
Committed: https://chromium.googlesource.com/chromium/src/+/1e1d38350c1b5c587b8bcbba0327066ac6152d27

[danyao, 2017-04-10 21:21:47]

Description was changed from

==========
Move stringify, form, navigation and scroll methods out of core.js.

- stringify() is a standalone utility so it is moved to common.js.
- form.js adds listeners to handle forms.
- navigation.js contains __gCrWeb APIs for managing navigation history.
- scroll_workaround.js isolates the workaround for crbug.com/554257.

Corresponding unit tests in core_js_unittest are also extracted.
- common_js_unittest.mm: fixed a bug in stringify(undefined), added test
  case and removed TODO.

BUG=614092
==========

to

==========
Move stringify, form, navigation and scroll methods out of core.js.

- stringify() is a standalone utility so it is moved to common.js.
- form.js adds listeners to handle forms.
- navigation.js contains __gCrWeb APIs for managing navigation history.
- scroll_workaround.js isolates the workaround for crbug.com/554257.

Corresponding unit tests in core_js_unittest are also extracted.
- common_js_unittest.mm: fixed a bug in stringify(undefined), added test
  case and removed TODO.

BUG=614092
TESTED=Ran ios_web_unittests, and manually verified in simulator that
the scroll workaround still works.
==========

[danyao, 2017-04-10 21:26:17]

Description was changed from

==========
Move stringify, form, navigation and scroll methods out of core.js.

- stringify() is a standalone utility so it is moved to common.js.
- form.js adds listeners to handle forms.
- navigation.js contains __gCrWeb APIs for managing navigation history.
- scroll_workaround.js isolates the workaround for crbug.com/554257.

Corresponding unit tests in core_js_unittest are also extracted.
- common_js_unittest.mm: fixed a bug in stringify(undefined), added test
  case and removed TODO.

BUG=614092
TESTED=Ran ios_web_unittests, and manually verified in simulator that
the scroll workaround still works.
==========

to

==========
Move stringify, form, navigation and scroll methods out of core.js.

- stringify() is a standalone utility so it is moved to common.js.
- form.js adds listeners to handle forms.
- navigation.js contains __gCrWeb APIs for managing navigation history.
- scroll_workaround.js isolates the workaround for crbug.com/554257.

Corresponding unit tests in core_js_unittest are also extracted.
- common_js_unittest.mm: fixed a bug in stringify(undefined), added test
  case and removed TODO.

BUG=614092
TESTED=Ran ios_web_unittests, and manually verified in simulator that
the scroll workaround still works.
==========

[danyao, 2017-04-10 21:26:18]

danyao@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-04-10 21:42:07]

Thanks for cleanup, lgtm!

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/common...
File ios/web/web_state/js/common_js_unittest.mm (right):

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/common...
ios/web/web_state/js/common_js_unittest.mm:9: #import
"base/strings/sys_string_conversions.h"
s/import/include (because this header has Objective-C behind ifdefs)

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/common...
ios/web/web_state/js/common_js_unittest.mm:16: /** Struct for isTextField() test
data. */
/** */ comments Style Is pretty uncommon in Chrome for iOS

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/common...
ios/web/web_state/js/common_js_unittest.mm:99: TestScriptAndExpectedValue
testData[] = {
s/testData/test_data to follow C++ Style Guide inside C++ methods:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/objective-...

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/common.js (right):

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/common.js:53: * should not be used if spoofing it
would create a security vulnerability.
How about s/if spoofing it would create a security vulnerability./instead of
{@code JSON.stringify}. ?

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/form.js (right):

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/form.js:76: function
getFullyQualifiedUrl_(originalURL) {
nit: How about |var getFullyQualifiedUrl_ = function(originalUrl)| for
consistency?

[danyao, 2017-04-10 22:13:00]

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/common...
File ios/web/web_state/js/common_js_unittest.mm (right):

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/common...
ios/web/web_state/js/common_js_unittest.mm:9: #import
"base/strings/sys_string_conversions.h"
On 2017/04/10 21:42:07, Eugene But wrote:
> s/import/include (because this header has Objective-C behind ifdefs)

Done.

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/common...
ios/web/web_state/js/common_js_unittest.mm:16: /** Struct for isTextField() test
data. */
On 2017/04/10 21:42:07, Eugene But wrote:
> /** */ comments Style Is pretty uncommon in Chrome for iOS

Ah right this is Objective C. I was carrying over JavaScript style guide rules
over here. Done!

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/common...
ios/web/web_state/js/common_js_unittest.mm:99: TestScriptAndExpectedValue
testData[] = {
On 2017/04/10 21:42:07, Eugene But wrote:
> s/testData/test_data to follow C++ Style Guide inside C++ methods:
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/objective-...

Done.

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/common.js (right):

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/common.js:53: * should not be used if spoofing it
would create a security vulnerability.
On 2017/04/10 21:42:07, Eugene But wrote:
> How about s/if spoofing it would create a security vulnerability./instead of
> {@code JSON.stringify}. ?

Did you perhaps mean {@code JSONStringify}?

Changing to {@code JSON.stringify} seems a bit confusing to me because this is
right after we said this method provides an "improvement" over JSON.stringify.
It seems a bit weird to immediately recommend against using it instead of the
built-in function.

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/form.js (right):

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/form.js:76: function
getFullyQualifiedUrl_(originalURL) {
On 2017/04/10 21:42:07, Eugene But wrote:
> nit: How about |var getFullyQualifiedUrl_ = function(originalUrl)| for
> consistency?

Done.

[Eugene But (OOO till 7-30), 2017-04-11 00:12:44]

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/common.js (right):

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/common.js:53: * should not be used if spoofing it
would create a security vulnerability.
On 2017/04/10 22:12:59, danyao wrote:
> On 2017/04/10 21:42:07, Eugene But wrote:
> > How about s/if spoofing it would create a security vulnerability./instead of
> > {@code JSON.stringify}. ?
> 
> Did you perhaps mean {@code JSONStringify}?
> 
> Changing to {@code JSON.stringify} seems a bit confusing to me because this is
> right after we said this method provides an "improvement" over JSON.stringify.
> It seems a bit weird to immediately recommend against using it instead of the
> built-in function.
{@code JSONStringify} lg. I just was not sure what "if spoofing it would create
a security vulnerability" actually means :)

[danyao, 2017-04-11 15:10:39]

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/resour...
File ios/web/web_state/js/resources/common.js (right):

https://codereview.chromium.org/2807213003/diff/1/ios/web/web_state/js/resour...
ios/web/web_state/js/resources/common.js:53: * should not be used if spoofing it
would create a security vulnerability.
On 2017/04/11 00:12:44, Eugene But wrote:
> On 2017/04/10 22:12:59, danyao wrote:
> > On 2017/04/10 21:42:07, Eugene But wrote:
> > > How about s/if spoofing it would create a security vulnerability./instead
of
> > > {@code JSON.stringify}. ?
> > 
> > Did you perhaps mean {@code JSONStringify}?
> > 
> > Changing to {@code JSON.stringify} seems a bit confusing to me because this
is
> > right after we said this method provides an "improvement" over
JSON.stringify.
> > It seems a bit weird to immediately recommend against using it instead of
the
> > built-in function.
> {@code JSONStringify} lg. I just was not sure what "if spoofing it would
create
> a security vulnerability" actually means :)

Updated. There may be something weird going on with the Closure compiler
settings because private methods declared in the dot syntax (e.g.
gCrWeb.common.JSONStringify) are not minified, so they can be overriden by the
website as easily as the public methods, which defeats the recommendation here.
I made a note to investigate this for the gCrWeb API hiding work.

[danyao, 2017-04-11 16:07:21]

The CQ bit was checked by danyao@chromium.org

[danyao, 2017-04-11 16:07:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2807213003/#ps40001
(title: "Clarified usage recommendation in comment for stringify()")

[commit-bot: I haz the power, 2017-04-11 16:07:48]

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, 2017-04-11 16:11:13]

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

[commit-bot: I haz the power, 2017-04-11 16:11:15]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[danyao, 2017-04-11 16:22:51]

On 2017/04/11 16:11:15, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
>   ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
>   ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

This was caused by merging failures. Ran rebase-update and fixed all merging
failures. This should work now.

[danyao, 2017-04-11 16:22:57]

The CQ bit was checked by danyao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 16:23:41]

Dry run: 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, 2017-04-11 17:29:39]

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

[commit-bot: I haz the power, 2017-04-11 17:29:40]

Dry run: This issue passed the CQ dry run.

[danyao, 2017-04-11 17:57:29]

The CQ bit was checked by danyao@chromium.org

[danyao, 2017-04-11 17:57:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2807213003/#ps60001
(title: "Re-upload patch after rebase-update in local branch")

[commit-bot: I haz the power, 2017-04-11 17:58:27]

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, 2017-04-11 18:25:44]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491933449821170,
"parent_rev": "c69f6dd1905bf9e23d902634240c88f7d5e513c8", "commit_rev":
"1e1d38350c1b5c587b8bcbba0327066ac6152d27"}

[commit-bot: I haz the power, 2017-04-11 18:26:38]

Description was changed from

==========
Move stringify, form, navigation and scroll methods out of core.js.

- stringify() is a standalone utility so it is moved to common.js.
- form.js adds listeners to handle forms.
- navigation.js contains __gCrWeb APIs for managing navigation history.
- scroll_workaround.js isolates the workaround for crbug.com/554257.

Corresponding unit tests in core_js_unittest are also extracted.
- common_js_unittest.mm: fixed a bug in stringify(undefined), added test
  case and removed TODO.

BUG=614092
TESTED=Ran ios_web_unittests, and manually verified in simulator that
the scroll workaround still works.
==========

to

==========
Move stringify, form, navigation and scroll methods out of core.js.

- stringify() is a standalone utility so it is moved to common.js.
- form.js adds listeners to handle forms.
- navigation.js contains __gCrWeb APIs for managing navigation history.
- scroll_workaround.js isolates the workaround for crbug.com/554257.

Corresponding unit tests in core_js_unittest are also extracted.
- common_js_unittest.mm: fixed a bug in stringify(undefined), added test
  case and removed TODO.

BUG=614092
TESTED=Ran ios_web_unittests, and manually verified in simulator that
the scroll workaround still works.

Review-Url: https://codereview.chromium.org/2807213003
Cr-Commit-Position: refs/heads/master@{#463691}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e1d38350c1b5c587b8bcbba0327...
==========

[commit-bot: I haz the power, 2017-04-11 18:26:39]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1e1d38350c1b5c587b8bcbba0327...