New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 849993 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 853953



Sign in to add a comment

Investigate using Mojo for mainstream WebUI

Project Member Reported by calamity@chromium.org, Jun 6 2018

Issue description

While there are several internal WebUIs that use Mojo for IPC (site-engagement, omnibox, discards, eoc-internals, etc), it would be good to further investigate how Mojo fares on a more user-facing, complex WebUI in order to understand the implications of using Mojo for WebUI IPC in the future.

Engineering concerns include:
- Testing
- Performance
- Design patterns

The initial plan here will be to experiment with migration of a less complex WebUI such as MD History to explore and understand the costs and benefits of doing so. This bug tracks work related to this effort.
 
Blocking: 853953
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 8

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

commit 29602085f18375313160d9d6bcbd1a5a21ec7cc3
Author: Christopher Lam <calamity@chromium.org>
Date: Thu Nov 08 02:58:52 2018

[Mojo WebUI] Modify testing framework to support Mojo Lite WebUIs.

This CL adds support to js2gtest for Mojo Lite WebUI Browser
Tests. This involves adding an extra mojo_lite_webui test type, and
adding the lite bindings to the page when a test is run.

Bug: 849993
Change-Id: Idadb098fb4c4a67535f59c80c8b4781a635da59b
Reviewed-on: https://chromium-review.googlesource.com/c/1309556
Reviewed-by: David Tseng <dtseng@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606304}
[modify] https://crrev.com/29602085f18375313160d9d6bcbd1a5a21ec7cc3/chrome/test/base/js2gtest.gni
[modify] https://crrev.com/29602085f18375313160d9d6bcbd1a5a21ec7cc3/chrome/test/base/js2gtest.js
[modify] https://crrev.com/29602085f18375313160d9d6bcbd1a5a21ec7cc3/chrome/test/base/mojo_web_ui_browser_test.cc
[modify] https://crrev.com/29602085f18375313160d9d6bcbd1a5a21ec7cc3/chrome/test/base/mojo_web_ui_browser_test.h
[modify] https://crrev.com/29602085f18375313160d9d6bcbd1a5a21ec7cc3/chrome/test/data/webui/test_api.js
[modify] https://crrev.com/29602085f18375313160d9d6bcbd1a5a21ec7cc3/chrome/test/data/webui_test_resources.grd

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 16

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

commit 39f676e6d2817c5b85d1a8ef9ba0f96cd4a970b1
Author: Ken Rockot <rockot@google.com>
Date: Fri Nov 16 05:03:49 2018

[mojo-bindings] JS lite union and FlushForTesting

Adds a |flushForTesting()| method to generated interface proxies.

This requires use of interface control message bindings, which in turn
requires union support. So union support is added here as well.

Finally, this also required compiling the interface control message
bindings into the mojo_bindings_lite.js binary, so some minimal changes
were made to generated JS code to make it compiler-friendly.

Net code size increase of about 2kB.

Bug: 849993
Change-Id: Id44d2d0e5e85693937f5bcd5419429766d25e062
Reviewed-on: https://chromium-review.googlesource.com/c/1336065
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608669}
[modify] https://crrev.com/39f676e6d2817c5b85d1a8ef9ba0f96cd4a970b1/content/test/data/lite_js_test.mojom
[modify] https://crrev.com/39f676e6d2817c5b85d1a8ef9ba0f96cd4a970b1/mojo/public/js/BUILD.gn
[modify] https://crrev.com/39f676e6d2817c5b85d1a8ef9ba0f96cd4a970b1/mojo/public/js/bindings_lite.js
[modify] https://crrev.com/39f676e6d2817c5b85d1a8ef9ba0f96cd4a970b1/mojo/public/tools/bindings/BUILD.gn
[modify] https://crrev.com/39f676e6d2817c5b85d1a8ef9ba0f96cd4a970b1/mojo/public/tools/bindings/generators/js_templates/lite/interface_definition.tmpl
[modify] https://crrev.com/39f676e6d2817c5b85d1a8ef9ba0f96cd4a970b1/mojo/public/tools/bindings/generators/js_templates/lite/module.externs.tmpl
[modify] https://crrev.com/39f676e6d2817c5b85d1a8ef9ba0f96cd4a970b1/mojo/public/tools/bindings/generators/js_templates/lite/module_definition.tmpl
[modify] https://crrev.com/39f676e6d2817c5b85d1a8ef9ba0f96cd4a970b1/mojo/public/tools/bindings/generators/js_templates/lite/union_definition.tmpl
[add] https://crrev.com/39f676e6d2817c5b85d1a8ef9ba0f96cd4a970b1/mojo/public/tools/bindings/generators/js_templates/lite/union_externs.tmpl
[modify] https://crrev.com/39f676e6d2817c5b85d1a8ef9ba0f96cd4a970b1/mojo/public/tools/bindings/generators/mojom_js_generator.py
[modify] https://crrev.com/39f676e6d2817c5b85d1a8ef9ba0f96cd4a970b1/third_party/WebKit/LayoutTests/mojo/bindings-lite.html

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 4

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

commit ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f
Author: Christopher Lam <calamity@chromium.org>
Date: Tue Dec 04 07:20:47 2018

[Downloads WebUI] Convert to Mojo.

This CL changes the MD Downloads browser communication from chrome.send
to Mojo Lite.

Bug: 849993
Change-Id: I4445f3185e4abe4769407c5741447cc7ece86747
Reviewed-on: https://chromium-review.googlesource.com/c/1090525
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613457}
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/BUILD.gn
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/browser_resources.grd
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/chrome_content_browser_manifest_overlay.json
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/resources/md_downloads/BUILD.gn
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/resources/md_downloads/browser_proxy.html
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/resources/md_downloads/browser_proxy.js
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/resources/md_downloads/externs.js
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/resources/md_downloads/item.html
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/resources/md_downloads/item.js
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/resources/md_downloads/manager.js
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/resources/md_downloads/search_service.js
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/resources/md_downloads/search_service_unittest.gtestjs
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/resources/md_downloads/toolbar.js
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/BUILD.gn
[add] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/BUILD.gn
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/OWNERS
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/downloads_list_tracker.cc
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/downloads_list_tracker.h
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/downloads_list_tracker_unittest.cc
[add] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/md_downloads.mojom
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.h
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler_unittest.cc
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/md_downloads_ui.cc
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/md_downloads_ui.h
[add] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/mock_md_downloads_page.cc
[add] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/browser/ui/webui/md_downloads/mock_md_downloads_page.h
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/test/BUILD.gn
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/test/data/webui/BUILD.gn
[add] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/test/data/webui/md_downloads/.eslintrc.js
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/test/data/webui/md_downloads/downloads_browsertest.js
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/test/data/webui/md_downloads/item_tests.js
[modify] https://crrev.com/ecd403dfb8b2f8b2a86cbc83a6240d1f0d7daf7f/chrome/test/data/webui/md_downloads/manager_tests.js

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 11

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

commit cceefec615766cbe2fbaaad334347a680eeee53f
Author: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Date: Tue Dec 11 05:51:52 2018

mojo: Add a externs_js_library target for js_libraries that type check

Some interfaces depend on many other interfaces. Before this patch users
would have to manually add externs for every interface used.

This CL introduces a new target that js_libraries can use to include
all the necessary externs to use an interface.

Bug: 849993
Change-Id: I97acb7bbbd604e7ebeb1cb5fe814e4afecefe2d6
Reviewed-on: https://chromium-review.googlesource.com/c/1369485
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: calamity <calamity@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615441}
[modify] https://crrev.com/cceefec615766cbe2fbaaad334347a680eeee53f/chrome/browser/resources/app_management/BUILD.gn
[modify] https://crrev.com/cceefec615766cbe2fbaaad334347a680eeee53f/chrome/browser/resources/md_downloads/BUILD.gn
[modify] https://crrev.com/cceefec615766cbe2fbaaad334347a680eeee53f/mojo/public/tools/bindings/mojom.gni

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 19

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

commit 94cbc94bffae9985b93e9b8381907726faf5efb7
Author: Christopher Lam <calamity@chromium.org>
Date: Wed Dec 19 06:53:53 2018

[Mojo Lite] Use record types for externs.

This CL changes Mojo Lite Closure Externs to use record types which more
closely reflects the reality of Mojo Lite bindings (that there is no
constructor for a generated class). This will allow Object literals that
implement the record type interface to assume the type of a generated
Mojo Lite struct type.

It also fixes up some nullability issues with Arrays and Maps.

Bug: 849993, 914149
Change-Id: I98d69689d4a0ffb6b773cef61041130a1934b1f5
Reviewed-on: https://chromium-review.googlesource.com/c/1372971
Commit-Queue: calamity <calamity@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#617745}
[modify] https://crrev.com/94cbc94bffae9985b93e9b8381907726faf5efb7/chrome/browser/resources/app_management/fake_page_handler.js
[modify] https://crrev.com/94cbc94bffae9985b93e9b8381907726faf5efb7/chrome/browser/resources/omnibox/omnibox_output.js
[modify] https://crrev.com/94cbc94bffae9985b93e9b8381907726faf5efb7/mojo/public/tools/bindings/generators/js_templates/lite/struct_externs.tmpl
[modify] https://crrev.com/94cbc94bffae9985b93e9b8381907726faf5efb7/mojo/public/tools/bindings/generators/mojom_js_generator.py

Project Member

Comment 7 by bugdroid1@chromium.org, Jan 14

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

commit 7661080854b5b01adbfbb06c02d74499beb09bfc
Author: Christopher Lam <calamity@chromium.org>
Date: Mon Jan 14 03:37:24 2019

[Mojo Lite] Fix Map for stringable kinds and add Closure Compiler test.

This CL makes stringable key types always return Objects, rather than a
type union with Map, and adds better type information to response
Promises.

It also adds a simple Closure Compiler target to ensure that generated
code can be accessed in certain ways.

Bug: 849993
Change-Id: I6002a577474e45b6b42185988968df8fa4ac6700
Reviewed-on: https://chromium-review.googlesource.com/c/1395870
Commit-Queue: calamity <calamity@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#622378}
[modify] https://crrev.com/7661080854b5b01adbfbb06c02d74499beb09bfc/BUILD.gn
[modify] https://crrev.com/7661080854b5b01adbfbb06c02d74499beb09bfc/mojo/public/tools/bindings/generators/js_templates/lite/interface_externs.tmpl
[add] https://crrev.com/7661080854b5b01adbfbb06c02d74499beb09bfc/mojo/public/tools/bindings/generators/js_templates/lite/test/BUILD.gn
[add] https://crrev.com/7661080854b5b01adbfbb06c02d74499beb09bfc/mojo/public/tools/bindings/generators/js_templates/lite/test/test.js
[add] https://crrev.com/7661080854b5b01adbfbb06c02d74499beb09bfc/mojo/public/tools/bindings/generators/js_templates/lite/test/test.test-mojom
[modify] https://crrev.com/7661080854b5b01adbfbb06c02d74499beb09bfc/mojo/public/tools/bindings/generators/mojom_js_generator.py

Comment 8 by dpa...@chromium.org, Jan 16 (6 days ago)

@calamity: I am trying to roll a new closure compiler version at https://chromium-review.googlesource.com/c/chromium/src/+/1414350, and it currently throws the following error

../../mojo/public/tools/bindings/generators/js_templates/lite/test/test.js:14: ERROR - initializing variable
found   : (null|{values: Array<string>})
required: {values: Array<string>}
  let result = await proxy.method1();
               ^^^^^^^^^^^^^^^^^^^^^

1 error(s), 0 warning(s), 94.1% typed

Could you help fixing that? My understanding is that the Compiler gets smarter in the latest version, and your comment at [1]

"This type is not enforced, as Closure does not handle Promise return typing."

is no longer true.

[1] https://cs.chromium.org/chromium/src/mojo/public/tools/bindings/generators/js_templates/lite/test/test.js?l=11

Comment 9 by dpa...@chromium.org, Jan 16 (6 days ago)

@calamity: Filed  issue 922671  for the compilation error. Let's continue this discussion there.

Comment 10 by calamity@google.com, Jan 17 (5 days ago)

This is great! Better type systems make me happy. I'm also going to try adding expected Closure failure cases so we can better document where the Closure type system catches problems and where it doesn't.
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 18 (5 days ago)

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

commit d41bc2dfbc94bffd9a8e6888f4130cfa9b9811cd
Author: Christopher Lam <calamity@chromium.org>
Date: Fri Jan 18 09:04:31 2019

[Downloads] Remove WebUIMessageHandler superclass from Mojo Handler.

Since MdDownloadsDOMHandler is a Mojo PageHandler now, it doesn't need
the WebUIMessageHandler superclass. It was previously left for
DisallowJavascript() calls, but that only gets called on WebUI-destroying
events anyway. So the corresponding code has been moved into the destructor.

Bug: 849993
Change-Id: I97e07b68a902529f37b9847309afbeb9ea3584cf
Reviewed-on: https://chromium-review.googlesource.com/c/1409671
Commit-Queue: calamity <calamity@chromium.org>
Reviewed-by: Dan Beam <dbeam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624043}
[modify] https://crrev.com/d41bc2dfbc94bffd9a8e6888f4130cfa9b9811cd/chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.cc
[modify] https://crrev.com/d41bc2dfbc94bffd9a8e6888f4130cfa9b9811cd/chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler.h
[modify] https://crrev.com/d41bc2dfbc94bffd9a8e6888f4130cfa9b9811cd/chrome/browser/ui/webui/md_downloads/md_downloads_dom_handler_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Jan 18 (4 days ago)

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

commit 4591322156c3b2af31c04aafbdf794f558be9fa3
Author: Christopher Lam <calamity@chromium.org>
Date: Fri Jan 18 16:20:08 2019

[Mojo WebUI] Make WebUITestHandler an abstract base class.

This CL factors out a common WebUITestHandler which is used by a
Mojo-specific and a WebUIMessageHandler-specific subclass for their
respective testing frameworks.

This cleans up the WebUITestHandler which previously subclassed both
WebUIMessageHandler and the Mojo interface, and only used half the method
implementations at a time.

BaseWebUIBrowserTest - Common test base class
 - WebUIBrowserTest - chrome.send, vanilla WebUIBrowserTest
   - uses WebUITestMessageHandler for chrome.send communication
 - MojoWebUIBrowserTest - Mojo-enabled BrowserTest
   - uses WebUITestPageHandler for Mojo-based communication

Both WebUITestMessageHandler and WebUITestPageHandler use the
WebUITestHandler to report success/failure.

Bug: 849993

Change-Id: I1284f293a63c9b0ae63d47371523f4e3696ae5a3
Reviewed-on: https://chromium-review.googlesource.com/c/1297776
Reviewed-by: Dan Beam <dbeam@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624150}
[modify] https://crrev.com/4591322156c3b2af31c04aafbdf794f558be9fa3/chrome/browser/ui/webui/web_ui_test_handler.cc
[modify] https://crrev.com/4591322156c3b2af31c04aafbdf794f558be9fa3/chrome/browser/ui/webui/web_ui_test_handler.h
[modify] https://crrev.com/4591322156c3b2af31c04aafbdf794f558be9fa3/chrome/test/base/mojo_web_ui_browser_test.cc
[modify] https://crrev.com/4591322156c3b2af31c04aafbdf794f558be9fa3/chrome/test/base/mojo_web_ui_browser_test.h
[modify] https://crrev.com/4591322156c3b2af31c04aafbdf794f558be9fa3/chrome/test/base/web_ui_browser_test.cc
[modify] https://crrev.com/4591322156c3b2af31c04aafbdf794f558be9fa3/chrome/test/base/web_ui_browser_test.h
[modify] https://crrev.com/4591322156c3b2af31c04aafbdf794f558be9fa3/chrome/test/base/web_ui_browser_test_browsertest.cc

Sign in to add a comment