New issue
Advanced search Search tips

Issue 614092 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Task



Sign in to add a comment

Split ios/web/web_state/js/resources/core.js into multiple files

Project Member Reported by eugene...@chromium.org, May 23 2016

Issue description

This came up during Mojo WebUI review, which has introduced another core.js file.

core.js from web has bunch of unrelated stuff and should be split into part, obvious are:

base.js
element_info.js
form.js
navigation.js
 
Labels: -Type-Bug Type-Feature
Description: Show this description
Cc: -jyqu...@chromium.org

Comment 4 by danyao@chromium.org, Mar 31 2017

Owner: danyao@chromium.org
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3423f23e28e4a31463540df78ed3a49b73c873c6

commit 3423f23e28e4a31463540df78ed3a49b73c873c6
Author: danyao <danyao@chromium.org>
Date: Mon Apr 03 23:56:20 2017

Remove obsolete isInternaLink_() function.

The callers were removed in https://codereview.chromium.org/2669303004.

BUG= 614092 

Review-Url: https://codereview.chromium.org/2790883004
Cr-Commit-Position: refs/heads/master@{#461582}

[modify] https://crrev.com/3423f23e28e4a31463540df78ed3a49b73c873c6/ios/web/web_state/js/resources/core.js

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd98b129dd88e75eee20e6614260c82642a21908

commit cd98b129dd88e75eee20e6614260c82642a21908
Author: danyao <danyao@chromium.org>
Date: Fri Apr 07 00:41:44 2017

Pull context menu related APIs from core.js to their own file.

These __gCrWeb APIs are only used in crw_context_menu_controller.mm.
They are moved to a new context-menu.js file:
- getElementFromPoint
- suppressNextClick
- getPageWidth

TEST=ran ios_web_unittests locally

BUG= 614092 

Review-Url: https://codereview.chromium.org/2800003002
Cr-Commit-Position: refs/heads/master@{#462713}

[modify] https://crrev.com/cd98b129dd88e75eee20e6614260c82642a21908/ios/web/BUILD.gn
[add] https://crrev.com/cd98b129dd88e75eee20e6614260c82642a21908/ios/web/web_state/js/resources/context_menu.js
[modify] https://crrev.com/cd98b129dd88e75eee20e6614260c82642a21908/ios/web/web_state/js/resources/core.js
[modify] https://crrev.com/cd98b129dd88e75eee20e6614260c82642a21908/ios/web/web_state/js/resources/web_bundle.js

Status: Started (was: Available)
Just realized that there is core_js_unittests file, which we should probably split as well:
https://cs.chromium.org/chromium/src/ios/web/web_state/js/core_js_unittest.mm?q=core_js_+package:%5Echromium$&l=1
Thanks for pointing this out! I'll move the tests to match the code in subsequent CLs.
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/851599815cb52219eab8cf90a7a0571a683a6686

commit 851599815cb52219eab8cf90a7a0571a683a6686
Author: danyao <danyao@chromium.org>
Date: Tue Apr 11 15:20:22 2017

Move unit tests for getElementFromPoint to context_menu_js_unittest.

The corresponding JavaScript implementation was moved from core.js to
context_menu.js in https://codereview.chromium.org/2800003002.

BUG= 614092 
TESTED=Ran ios_web_unittest and passed.

Review-Url: https://codereview.chromium.org/2812573004
Cr-Commit-Position: refs/heads/master@{#463637}

[modify] https://crrev.com/851599815cb52219eab8cf90a7a0571a683a6686/ios/web/BUILD.gn
[add] https://crrev.com/851599815cb52219eab8cf90a7a0571a683a6686/ios/web/web_state/js/context_menu_js_unittest.mm
[modify] https://crrev.com/851599815cb52219eab8cf90a7a0571a683a6686/ios/web/web_state/js/core_js_unittest.mm

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e1d38350c1b5c587b8bcbba0327066ac6152d27

commit 1e1d38350c1b5c587b8bcbba0327066ac6152d27
Author: danyao <danyao@chromium.org>
Date: Tue Apr 11 18:25:31 2017

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}

[modify] https://crrev.com/1e1d38350c1b5c587b8bcbba0327066ac6152d27/ios/web/BUILD.gn
[modify] https://crrev.com/1e1d38350c1b5c587b8bcbba0327066ac6152d27/ios/web/web_state/js/common_js_unittest.mm
[modify] https://crrev.com/1e1d38350c1b5c587b8bcbba0327066ac6152d27/ios/web/web_state/js/core_js_unittest.mm
[modify] https://crrev.com/1e1d38350c1b5c587b8bcbba0327066ac6152d27/ios/web/web_state/js/resources/common.js
[modify] https://crrev.com/1e1d38350c1b5c587b8bcbba0327066ac6152d27/ios/web/web_state/js/resources/core.js
[add] https://crrev.com/1e1d38350c1b5c587b8bcbba0327066ac6152d27/ios/web/web_state/js/resources/form.js
[add] https://crrev.com/1e1d38350c1b5c587b8bcbba0327066ac6152d27/ios/web/web_state/js/resources/navigation.js
[add] https://crrev.com/1e1d38350c1b5c587b8bcbba0327066ac6152d27/ios/web/web_state/js/resources/scroll_workaround.js
[modify] https://crrev.com/1e1d38350c1b5c587b8bcbba0327066ac6152d27/ios/web/web_state/js/resources/web_bundle.js

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f7838f65b679f68aca8e1b0e3886442c6e74670

commit 2f7838f65b679f68aca8e1b0e3886442c6e74670
Author: danyao <danyao@chromium.org>
Date: Tue Apr 11 20:18:25 2017

Move password-related methods from core.js to password_controller.js.

The following methods are moved:
- hasPasswordField: public API method in gCrWeb
- hasPasswordField_: private implementation

These methods are only used in password_controller.js. Unittests are also moved.

BUG= 614092 
TEST=Passwords autofill works correctly

Review-Url: https://codereview.chromium.org/2814773002
Cr-Commit-Position: refs/heads/master@{#463746}

[modify] https://crrev.com/2f7838f65b679f68aca8e1b0e3886442c6e74670/ios/chrome/browser/passwords/password_controller_unittest.mm
[modify] https://crrev.com/2f7838f65b679f68aca8e1b0e3886442c6e74670/ios/chrome/browser/passwords/resources/password_controller.js
[modify] https://crrev.com/2f7838f65b679f68aca8e1b0e3886442c6e74670/ios/web/BUILD.gn
[delete] https://crrev.com/57ba633b470b05156a7446c816bd74e7e342d923/ios/web/web_state/js/core_js_unittest.mm
[modify] https://crrev.com/2f7838f65b679f68aca8e1b0e3886442c6e74670/ios/web/web_state/js/resources/core.js

Labels: -Type-Feature Type-Task
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c1ff136fb8f5e327c7f2a70066c1c33cff539911

commit c1ff136fb8f5e327c7f2a70066c1c33cff539911
Author: danyao <danyao@chromium.org>
Date: Thu Apr 13 19:05:36 2017

Deprecate __gCrWeb['sendFaviconsToHost'] by inlining it.

Also moved didFinishNavigation() from core.js to legacy.js. This
method can probably eventually be removed (see crbug.com/546350). For
now, it is most closely related to navigation so cleaner to put it
there instead of creating a separate .js file.

This removes two of the last three methods in core.js.

BUG= 614092 
TESTED=Ran ios_chrome_unittests and passed.

Review-Url: https://codereview.chromium.org/2810263002
Cr-Commit-Position: refs/heads/master@{#464488}

[modify] https://crrev.com/c1ff136fb8f5e327c7f2a70066c1c33cff539911/ios/web/BUILD.gn
[modify] https://crrev.com/c1ff136fb8f5e327c7f2a70066c1c33cff539911/ios/web/web_state/js/resources/core.js
[add] https://crrev.com/c1ff136fb8f5e327c7f2a70066c1c33cff539911/ios/web/web_state/js/resources/legacy.js
[modify] https://crrev.com/c1ff136fb8f5e327c7f2a70066c1c33cff539911/ios/web/web_state/js/resources/navigation.js
[modify] https://crrev.com/c1ff136fb8f5e327c7f2a70066c1c33cff539911/ios/web/web_state/js/resources/web_bundle.js
[modify] https://crrev.com/c1ff136fb8f5e327c7f2a70066c1c33cff539911/ios/web/web_state/ui/crw_web_controller.mm

Project Member

Comment 14 by bugdroid1@chromium.org, Apr 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0adc0ccf7a830e33e26d1d19a87f8e38bede7477

commit 0adc0ccf7a830e33e26d1d19a87f8e38bede7477
Author: danyao <danyao@chromium.org>
Date: Thu Apr 13 20:29:00 2017

Fully deprecate core.js. Moved last method to error.js.

This is the last CL to split core.js into better encapsulated parts.
Also updated all references to web/web_state/js/resources/core.js in all
comments.

BUG= 614092 

Review-Url: https://codereview.chromium.org/2817943002
Cr-Commit-Position: refs/heads/master@{#464528}

[modify] https://crrev.com/0adc0ccf7a830e33e26d1d19a87f8e38bede7477/ios/chrome/browser/passwords/js_password_manager.h
[modify] https://crrev.com/0adc0ccf7a830e33e26d1d19a87f8e38bede7477/ios/chrome/browser/ui/contextual_search/resources/contextualsearch.js
[modify] https://crrev.com/0adc0ccf7a830e33e26d1d19a87f8e38bede7477/ios/web/BUILD.gn
[modify] https://crrev.com/0adc0ccf7a830e33e26d1d19a87f8e38bede7477/ios/web/web_state/js/common_js_unittest.mm
[delete] https://crrev.com/7ca3add13622c453c70565b8a21255133bc39687/ios/web/web_state/js/resources/core.js
[modify] https://crrev.com/0adc0ccf7a830e33e26d1d19a87f8e38bede7477/ios/web/web_state/js/resources/dialog_overrides.js
[add] https://crrev.com/0adc0ccf7a830e33e26d1d19a87f8e38bede7477/ios/web/web_state/js/resources/error.js
[modify] https://crrev.com/0adc0ccf7a830e33e26d1d19a87f8e38bede7477/ios/web/web_state/js/resources/web_bundle.js
[modify] https://crrev.com/0adc0ccf7a830e33e26d1d19a87f8e38bede7477/ios/web/web_state/ui/crw_web_controller.mm

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed96115726663cd5ed367c807310400686dc0927

commit ed96115726663cd5ed367c807310400686dc0927
Author: danyao <danyao@chromium.org>
Date: Wed Apr 19 16:00:46 2017

Remove hasPasswordField from the public __gCrWeb API.

This method is a helper for __gCrWeb.fillPasswordForm. The only usage
outside of JavaScript is in password_controller_unittest.mm, which tests
explicitly the scenario of detecting password fields in nested iframes.
This change beefs up test cases in PasswordControllerTest so the nested
iframe case is tested using the public fillPasswordForm API.

BUG= 614092 
TESTED=Ran ios_chrome_unittests

Review-Url: https://codereview.chromium.org/2825123003
Cr-Commit-Position: refs/heads/master@{#465625}

[modify] https://crrev.com/ed96115726663cd5ed367c807310400686dc0927/ios/chrome/browser/passwords/password_controller_unittest.mm
[modify] https://crrev.com/ed96115726663cd5ed367c807310400686dc0927/ios/chrome/browser/passwords/resources/password_controller.js

Status: Fixed (was: Started)

Sign in to add a comment