Move the Protocol Handler URL's syntax to blink [chromium/src : main]

Javier Fernandez (Gerrit)

unread,

Jun 27, 2022, 8:47:49 AM6/27/22

to Dominick Ng, Dmitry Gozman, Mike West, alexmo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, creis...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org

Attention is currently required from: Dmitry Gozman, Dominick Ng, Mike West.

Javier Fernandez would like Dominick Ng, Dmitry Gozman and Mike West to review this change.

View Change

Move the Protocol Handler URL's syntax to blink

The Custom Handlers spec defines some URL's syntax checks [1] that
must be passed in order to accept registerProtocolHandler requests.
These checks are implemented in blink's navigatorcontentutils module
and intended to be executed by the renderer process only.

The idea of this CL is to move the URL's syntax check logic to the
blink's common folder, specifically the protocol_handler_utils.cc
file, where there are several static functions executed by both, the
renderer and browser processes.

Thanks to this refactoring, this CL also adds the URL's syntax checks
in the WebContentsImpl class, so that they are executed in by the
browser process.


Change-Id: I56fafa1d3392d5b79aab28c799e51d4b40f6a1b4
---
M components/custom_handlers/protocol_handler.cc
M content/browser/web_contents/web_contents_impl.cc
M content/browser/web_contents/web_contents_impl_unittest.cc
M third_party/blink/common/custom_handlers/protocol_handler_utils.cc
M third_party/blink/public/common/custom_handlers/protocol_handler_utils.h
M third_party/blink/renderer/modules/navigatorcontentutils/navigator_content_utils.cc
6 files changed, 109 insertions(+), 10 deletions(-)

To view, visit change 3692750. To unsubscribe, or for help writing mail filters, visit settings.

Javier Fernandez (Gerrit)

unread,

Jun 27, 2022, 8:47:55 AM6/27/22

to alexmo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, creis...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org, Dominick Ng, Dmitry Gozman, Mike West, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Gyuyoung Kim

Attention is currently required from: Dmitry Gozman, Dominick Ng, Mike West.

View Change

To view, visit change 3692750. To unsubscribe, or for help writing mail filters, visit settings.

Javier Fernandez (Gerrit)

unread,

Jun 27, 2022, 8:50:26 AM6/27/22

to Fabio Rocha, Samuel Tang, alexmo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, creis...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org, Dominick Ng, Dmitry Gozman, Mike West

Attention is currently required from: Dmitry Gozman, Dominick Ng, Fabio Rocha, Mike West, Samuel Tang.

Javier Fernandez would like Fabio Rocha and Samuel Tang to review this change.

View Change

Move the Protocol Handler URL's syntax to blink

The Custom Handlers spec defines some URL's syntax checks [1] that
must be passed in order to accept registerProtocolHandler requests.
These checks are implemented in blink's navigatorcontentutils module
and intended to be executed by the renderer process only.

The idea of this CL is to move the URL's syntax check logic to the
blink's common folder, specifically the protocol_handler_utils.cc
file, where there are several static functions executed by both, the
renderer and browser processes.

Thanks to this refactoring, this CL also adds the URL's syntax checks
in the WebContentsImpl class, so that they are executed in by the
browser process.


Change-Id: I56fafa1d3392d5b79aab28c799e51d4b40f6a1b4
---
M components/custom_handlers/protocol_handler.cc
M content/browser/web_contents/web_contents_impl.cc
M content/browser/web_contents/web_contents_impl_unittest.cc
M third_party/blink/common/custom_handlers/protocol_handler_utils.cc
M third_party/blink/public/common/custom_handlers/protocol_handler_utils.h
M third_party/blink/renderer/modules/navigatorcontentutils/navigator_content_utils.cc
6 files changed, 109 insertions(+), 10 deletions(-)

To view, visit change 3692750. To unsubscribe, or for help writing mail filters, visit settings.

Javier Fernandez (Gerrit)

unread,

Jun 27, 2022, 8:50:32 AM6/27/22

to alexmo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, creis...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org, Fabio Rocha, Samuel Tang, Dominick Ng, Dmitry Gozman, Mike West, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Gyuyoung Kim

Attention is currently required from: Dmitry Gozman, Dominick Ng, Fabio Rocha, Mike West, Samuel Tang.

View Change

1 comment:

Patchset:
- Patch Set #5:
  Adding Fabio and Samuel in Cc, since this change may affect the PWA as Custom Handler feature.

To view, visit change 3692750. To unsubscribe, or for help writing mail filters, visit settings.

Dominick Ng (Gerrit)

unread,

Jun 27, 2022, 7:51:52 PM6/27/22

to Javier Fernandez, alexmo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, creis...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org, Dominick Ng, Fabio Rocha, Samuel Tang, Dmitry Gozman, Mike West, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Gyuyoung Kim

Attention is currently required from: Dmitry Gozman, Fabio Rocha, Javier Fernandez, Mike West, Samuel Tang.

Patch set 5:Code-Review +1

View Change

6 comments:

Patchset:
- Patch Set #5:
  LGTM % nits

File content/browser/web_contents/web_contents_impl.cc:

Patch Set #5, Line 332:


  // TODO(jfernandez): Should we include syntax checks (step 3) as we do in the
  // renderer process ?

Remove?

File third_party/blink/common/custom_handlers/protocol_handler_utils.cc:
- Patch Set #5, Line 29:
```
 if (full_url.is_empty() || !full_url.is_valid()) {
    return URLSyntaxErrorCode::kInvalidURL;
  }
```
  Here and above: be consistent with the use of braces. This file seems to consistently omit them for 1-line conditionals.
File third_party/blink/public/common/custom_handlers/protocol_handler_utils.h:
- Patch Set #5, Line 23: kInvalidURL
  Nit: kInvalidUrl (style guide says to only capitalise the first letter of acronyms)

File third_party/blink/renderer/modules/navigatorcontentutils/navigator_content_utils.cc:

Patch Set #5, Line 153: DCHECK
Nit: DCHECK_EQ

Patch Set #5, Line 141:

 if (code == URLSyntaxErrorCode::kMissingToken) {
    error_message =
        "The url provided ('" + user_url + "') does not contain '%s'.";
    return false;
  }
  if (code == URLSyntaxErrorCode::kInvalidURL) {
    error_message =
        "The custom handler URL created by removing '%s' and prepending '" +
        base_url.GetString() + "' is invalid.";
    return false;
  }

  DCHECK(code == URLSyntaxErrorCode::kNoError);

Consider using a switch statement on the enum value - then you don't need the DCHECK since the switch will guarantee it

To view, visit change 3692750. To unsubscribe, or for help writing mail filters, visit settings.

Javier Fernandez (Gerrit)

unread,

Jun 28, 2022, 5:38:39 PM6/28/22

to alexmo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, creis...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org, Dominick Ng, Fabio Rocha, Samuel Tang, Dmitry Gozman, Mike West, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Gyuyoung Kim

Attention is currently required from: Dmitry Gozman, Fabio Rocha, Mike West, Samuel Tang.

View Change

3 comments:

Patchset:
- Patch Set #5:
  Thanks for the reviews.
  I'll submit ASAP a new patch with the suggested changes.
File content/browser/web_contents/web_contents_impl.cc:

Patch Set #5, Line 332:


  // TODO(jfernandez): Should we include syntax checks (step 3) as we do in the
  // renderer process ?

Remove?

- Ack
File third_party/blink/common/custom_handlers/protocol_handler_utils.cc:

Patch Set #5, Line 29:

 if (full_url.is_empty() || !full_url.is_valid()) {
    return URLSyntaxErrorCode::kInvalidURL;
  }

- Here and above: be consistent with the use of braces. […]
  Ack

To view, visit change 3692750. To unsubscribe, or for help writing mail filters, visit settings.

Mike West (Gerrit)

unread,

Jun 29, 2022, 10:42:21 AM6/29/22

to Javier Fernandez, alexmo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, creis...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org, Dominick Ng, Fabio Rocha, Samuel Tang, Dmitry Gozman, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Gyuyoung Kim

Attention is currently required from: Dmitry Gozman, Fabio Rocha, Javier Fernandez, Samuel Tang.

Patch set 7:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #7:
  LGTM.

To view, visit change 3692750. To unsubscribe, or for help writing mail filters, visit settings.

Dmitry Gozman (Gerrit)

unread,

Jun 29, 2022, 12:29:29 PM6/29/22

to Javier Fernandez, alexmo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, creis...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org, Mike West, Dominick Ng, Fabio Rocha, Samuel Tang, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Gyuyoung Kim

Attention is currently required from: Fabio Rocha, Javier Fernandez, Samuel Tang.

Patch set 7:Code-Review +1

View Change

To view, visit change 3692750. To unsubscribe, or for help writing mail filters, visit settings.

Javier Fernandez (Gerrit)

unread,

Jun 29, 2022, 5:59:47 PM6/29/22

to alexmo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, creis...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org, Dmitry Gozman, Mike West, Dominick Ng, Fabio Rocha, Samuel Tang, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Gyuyoung Kim

Attention is currently required from: Fabio Rocha, Javier Fernandez, Samuel Tang.

Patch set 7:Commit-Queue +2

View Change

To view, visit change 3692750. To unsubscribe, or for help writing mail filters, visit settings.

Javier Fernandez (Gerrit)

unread,

Jun 29, 2022, 6:35:04 PM6/29/22

to alexmo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, creis...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org, Dmitry Gozman, Mike West, Dominick Ng, Fabio Rocha, Samuel Tang, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Gyuyoung Kim

Chromium LUCI CQ (Gerrit)

unread,

Jun 29, 2022, 7:36:41 PM6/29/22

to Javier Fernandez, alexmo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, creis...@chromium.org, kinuko...@chromium.org, navigation...@chromium.org, Dmitry Gozman, Mike West, Dominick Ng, Fabio Rocha, Samuel Tang, Tricium, chromium...@chromium.org, Gyuyoung Kim

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Mike West: Looks good to me
  Javier Fernandez: Commit
  Dmitry Gozman: Looks good to me
  Dominick Ng: Looks good to me

Move the Protocol Handler URL's syntax to blink

The Custom Handlers spec defines some URL's syntax checks [1] that
must be passed in order to accept registerProtocolHandler requests.
These checks are implemented in blink's navigatorcontentutils module
and intended to be executed by the renderer process only.

The idea of this CL is to move the URL's syntax check logic to the
blink's common folder, specifically the protocol_handler_utils.cc
file, where there are several static functions executed by both, the
renderer and browser processes.

Thanks to this refactoring, this CL also adds the URL's syntax checks
in the WebContentsImpl class, so that they are executed in by the
browser process.


Change-Id: I56fafa1d3392d5b79aab28c799e51d4b40f6a1b4

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3692750
Reviewed-by: Dominick Ng <domi...@chromium.org>
Reviewed-by: Mike West <mk...@chromium.org>
Reviewed-by: Dmitry Gozman <dgo...@chromium.org>
Commit-Queue: Javier Fernandez <jfern...@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1019393}


---
M components/custom_handlers/protocol_handler.cc
M content/browser/web_contents/web_contents_impl.cc
M content/browser/web_contents/web_contents_impl_unittest.cc
M third_party/blink/common/custom_handlers/protocol_handler_utils.cc
M third_party/blink/public/common/custom_handlers/protocol_handler_utils.h
M third_party/blink/renderer/modules/navigatorcontentutils/navigator_content_utils.cc

6 files changed, 124 insertions(+), 21 deletions(-)

To view, visit change 3692750. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward