Remove LoadURLExternally from blink public API [chromium/src : master]
Charlie Harrison (Gerrit)
Charlie Harrison uploaded patch set #4 to this change.
Remove LoadURLExternally from blink public API
Bug: None
Change-Id: I71f8f6b412ca17e8c7d465c3e500b96704baab49
---
M content/browser/web_contents/web_contents_impl.cc
M content/public/renderer/render_frame.h
M content/renderer/render_frame_impl.cc
M content/renderer/render_frame_impl.h
M content/shell/test_runner/web_frame_test_client.cc
M content/shell/test_runner/web_frame_test_client.h
M content/shell/test_runner/web_frame_test_proxy.h
M third_party/WebKit/Source/core/frame/LocalFrameClient.h
M third_party/WebKit/Source/core/loader/EmptyClients.h
M third_party/WebKit/Source/core/loader/FrameLoader.cpp
M third_party/WebKit/Source/web/LocalFrameClientImpl.cpp
M third_party/WebKit/Source/web/LocalFrameClientImpl.h
M third_party/WebKit/public/web/WebFrameClient.h
13 files changed, 23 insertions(+), 114 deletions(-)
To view, visit change 571139. To unsubscribe, visit settings.
Charlie Harrison (Gerrit)
Charlie Harrison uploaded patch set #5 to this change.
Remove LoadURLExternally from blink public API
This patch essentially just moves the single caller of LoadURLExternally
into an OpenURL call inside DecidePolicyForNavigation.
Bug: None
Change-Id: I71f8f6b412ca17e8c7d465c3e500b96704baab49
---
M content/browser/web_contents/web_contents_impl.cc
M content/public/renderer/render_frame.h
M content/renderer/render_frame_impl.cc
M content/renderer/render_frame_impl.h
M content/shell/test_runner/web_frame_test_client.cc
M content/shell/test_runner/web_frame_test_client.h
M content/shell/test_runner/web_frame_test_proxy.h
M third_party/WebKit/Source/core/frame/LocalFrameClient.h
M third_party/WebKit/Source/core/loader/EmptyClients.h
M third_party/WebKit/Source/core/loader/FrameLoader.cpp
M third_party/WebKit/Source/web/LocalFrameClientImpl.cpp
M third_party/WebKit/Source/web/LocalFrameClientImpl.h
M third_party/WebKit/public/web/WebFrameClient.h
13 files changed, 23 insertions(+), 114 deletions(-)
To view, visit change 571139. To unsubscribe, visit settings.
Charlie Harrison (Gerrit)
Charlie Harrison posted comments on this change.
Patch set 5:
Set Ready For Review
Charlie Harrison (Gerrit)
Charlie Harrison would like Daniel Cheng to review this change.
Remove LoadURLExternally from blink public API
This patch essentially just moves the single caller of LoadURLExternally
into an OpenURL call inside DecidePolicyForNavigation.
Bug: None
Change-Id: I71f8f6b412ca17e8c7d465c3e500b96704baab49
---
M content/browser/web_contents/web_contents_impl.cc
M content/public/renderer/render_frame.h
M content/renderer/render_frame_impl.cc
M content/renderer/render_frame_impl.h
M content/shell/test_runner/web_frame_test_client.cc
M content/shell/test_runner/web_frame_test_client.h
M content/shell/test_runner/web_frame_test_proxy.h
M third_party/WebKit/Source/core/frame/LocalFrameClient.h
M third_party/WebKit/Source/core/loader/EmptyClients.h
M third_party/WebKit/Source/core/loader/FrameLoader.cpp
M third_party/WebKit/Source/web/LocalFrameClientImpl.cpp
M third_party/WebKit/Source/web/LocalFrameClientImpl.h
M third_party/WebKit/public/web/WebFrameClient.h
13 files changed, 23 insertions(+), 114 deletions(-)
To view, visit change 571139. To unsubscribe, visit settings.
Charlie Harrison (Gerrit)
Hey Daniel, this is the refactor we talked about earlier. It doesn't really change the DecidePolicyForNavigation API in a fundamental way, but it does remove LoadURLExternally.
Bots seem happy, and the mac failures are not related I think.
One thing I'm not sure about is the call to exit fullscreen is gone. I'm not entirely sure this is a big deal because there are a lot of code paths not going through LoadURLExternally that spawn new windows.
In any case, low priority review since it's just a clean up.
Charlie Harrison (Gerrit)
Daniel, are you interested in landing this patch?
To view, visit change 571139. To unsubscribe, or for help writing mail filters, visit settings.
Charlie Harrison (Gerrit)
Patch set 5:Code-Review +1
To view, visit change 571139. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
Sorry for missing this one. Yeah I think we should try to do this.
2 comments:
File content/browser/web_contents/web_contents_impl.cc:
Patch Set #5, Line 2861:
// TODO(csharrison): Do we need to exit fullscreen here?I think this might have been related to us trying to make sure fullscreen mode is exited when new popups are opened (to keep popups from obscuring fullscreen UI).
+avi, who is moving this plumbing to be browser-based
File content/renderer/render_frame_impl.cc:
Patch Set #5, Line 5519:
info.default_policy, false, false, info.triggering_event_info);Very similar lines to this appear in a lot of places. I wonder if we should still have a helper for this? In particular maybe it'd be useful to just pass |info| and have the helper extract out the necessary fields (or maybe we can pass it to OpenURL itself...?)
To view, visit change 571139. To unsubscribe, or for help writing mail filters, visit settings.
Avi Drissman (Gerrit)
Patch set 5:Code-Review +1
2 comments:
Patch Set #5, Line 2861:
// TODO(csharrison): Do we need to exit fullscreen here?
I think this might have been related to us trying to make sure fullscreen mode is exited when new po […]
My CL is https://chromium-review.googlesource.com/c/606987, and I'm tying into the popup code. You shouldn't have to worry about this.
File third_party/WebKit/Source/web/LocalFrameClientImpl.cpp:
These are being removed in https://chromium-review.googlesource.com/c/571139
where I'm moving the unfullscreening for popups to the browser process. Do not worry about removing them here either.
To view, visit change 571139. To unsubscribe, or for help writing mail filters, visit settings.
Charlie Harrison (Gerrit)
Thanks!
3 comments:
My CL is https://chromium-review.googlesource.com/c/606987, and I'm tying into the popup code. […]
Ack, thanks Avi.
File content/renderer/render_frame_impl.cc:
Patch Set #5, Line 5519:
// TODO(creis): Deprecate this logic once we can rely on rel=noreferrer
Very similar lines to this appear in a lot of places. […]
I just simplified OpenURL directly so it only needs a few params plus the |info|. Much nicer!
File third_party/WebKit/Source/web/LocalFrameClientImpl.cpp:
These are being removed in https://chromium-review.googlesource.com/c/571139 […]
Ack
To view, visit change 571139. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
LGTM with a nit
Patch set 6:Code-Review +1
1 comment:
File content/renderer/render_frame_impl.cc:
Patch Set #6, Line 5402:
RenderViewImpl::GetReferrerFromRequest(frame_, info.url_request));Any chance we can get rid of this too, since OpenURL() does this now too?
To view, visit change 571139. To unsubscribe, or for help writing mail filters, visit settings.
Charlie Harrison (Gerrit)
Thanks folks
Patch set 7:Commit-Queue +2
To view, visit change 571139. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"dcheng review" https://chromium-review.googlesource.com/c/571139/7
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/571139/7
Bot data: {"action": "start", "triggered_at": "2017-08-22T17:11:53.0Z", "cq_cfg_revision": "81f1a3f0cb07446abeff7dd01ff1b8bae94ffe0d", "revision": "1123568d9e804bd10862aa34da22ee2c0e76cc0c"}
To view, visit change 571139. To unsubscribe, or for help writing mail filters, visit settings.
Charlie Harrison (Gerrit)
1 comment:
File content/renderer/render_frame_impl.cc:
Patch Set #6, Line 5402:
if (is_content_initiated && IsTopLevelNavigation(frame_) &&
Any chance we can get rid of this too, since OpenURL() does this now too?
Done
To view, visit change 571139. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
Commit Bot merged this change.
Remove LoadURLExternally from blink public API
This patch essentially just moves the single caller of LoadURLExternally
into an OpenURL call inside DecidePolicyForNavigation.
Bug: None
Change-Id: I71f8f6b412ca17e8c7d465c3e500b96704baab49
Reviewed-on: https://chromium-review.googlesource.com/571139
Commit-Queue: Charlie Harrison <cshar...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Charlie Harrison <cshar...@chromium.org>
Reviewed-by: Avi Drissman <a...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496402}
---
M content/public/renderer/render_frame.h
M content/renderer/render_frame_impl.cc
M content/renderer/render_frame_impl.h
M content/shell/test_runner/web_frame_test_client.cc
M content/shell/test_runner/web_frame_test_client.h
M content/shell/test_runner/web_frame_test_proxy.h
M third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp
M third_party/WebKit/Source/core/exported/LocalFrameClientImpl.h
M third_party/WebKit/Source/core/frame/LocalFrameClient.h
M third_party/WebKit/Source/core/loader/EmptyClients.h
M third_party/WebKit/Source/core/loader/FrameLoader.cpp
M third_party/WebKit/public/web/WebFrameClient.h
12 files changed, 41 insertions(+), 166 deletions(-)
To view, visit change 571139. To unsubscribe, or for help writing mail filters, visit settings.