Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2600253003:
Fix plugin placeholders not loading (Closed)
|
Created:
3 years, 12 months ago by Nate Chapin Modified:
3 years, 11 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix plugin placeholders not loading
I have made a truly wonderful mess of WebViewPlugin, via:
https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c529751d29e8a0188
https://chromium.googlesource.com/chromium/src/+/8ed4bf69eca5e2696aeab731a8e9a15e8f379f2b
My fundamental misunderstanding (I think!) was regarding what
WebViewPlugin is listening to via the various interfaces it subclasses.
It uses WebFrameClient and WebViewClient to observe its internal
WebView, while it uses WebPlugin and RenderViewObserver to interact
with the real external page. In the patches above, I mixed and matched
messages from those 2 sources because I thought they were coming from
the same context.
This patch reverts the damage done in those patches, and restores
ReplayReceivedData, more-or-less the way it was before I
started breaking stuff. It then continues the work I started in
https://chromium.googlesource.com/chromium/src/+/8e1badd5fbc054cf4ffc9303e8cb0aa22ce55ca6
and moves the subclassing of WebViewClient to a private helper class.
This helper, renamed to WebViewHelper, now handles all callbacks coming
from the internal WebView (i.e., the "WebView" in "WebViewPlugin"), and
WebViewPlugin continues subclassing WebPlugin and RenderViewObserver
to observe the outside world. This will hopefully make it harder to
screw things up in the same way in the future.
BUG=675585
Committed: https://crrev.com/b08418cd6d5f472ca454216aab8fd06cf9a19a36
Cr-Commit-Position: refs/heads/master@{#441401}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address bauerb's comments #
Messages
Total messages: 22 (16 generated)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Split better Revert "Listen to didReceiveResponse() to get the response in WebViewPlugin." This reverts commit 8ed4bf69eca5e2696aeab731a8e9a15e8f379f2b. Partial revert BUG= ========== to ========== Fix plugin placeholders not loading I have made a truly wonderful mess of WebViewPlugin, via: https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297... https://chromium.googlesource.com/chromium/src/+/8ed4bf69eca5e2696aeab731a8e9... My fundamental misunderstanding (I think!) was regarding what WebViewPlugin is listening to via the various interfaces it subclasses. It uses WebFrameClient and WebViewClient to observe its internal WebView, while it uses WebPlugin and RenderViewObserver to interact with the real external page. In the patches above, I mixed and matched messages from those 2 sources because I thought they were coming from the same context. This patch reverts the damage done in those patches, and restores ReplayReceivedData, more-or-less the way it was before I started breaking stuff. It then continues the work I started in https://chromium.googlesource.com/chromium/src/+/8e1badd5fbc054cf4ffc9303e8cb... and moves the subclassing of WebViewClient to a private helper class. This helper, renamed to WebViewHelper, now handles all callbacks coming from the internal WebView (i.e., the "WebView" in "WebViewPlugin"), and WebViewPlugin continues subclassing WebPlugin and RenderViewObserver to observe the outside world. BUG= ==========
Description was changed from ========== Fix plugin placeholders not loading I have made a truly wonderful mess of WebViewPlugin, via: https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297... https://chromium.googlesource.com/chromium/src/+/8ed4bf69eca5e2696aeab731a8e9... My fundamental misunderstanding (I think!) was regarding what WebViewPlugin is listening to via the various interfaces it subclasses. It uses WebFrameClient and WebViewClient to observe its internal WebView, while it uses WebPlugin and RenderViewObserver to interact with the real external page. In the patches above, I mixed and matched messages from those 2 sources because I thought they were coming from the same context. This patch reverts the damage done in those patches, and restores ReplayReceivedData, more-or-less the way it was before I started breaking stuff. It then continues the work I started in https://chromium.googlesource.com/chromium/src/+/8e1badd5fbc054cf4ffc9303e8cb... and moves the subclassing of WebViewClient to a private helper class. This helper, renamed to WebViewHelper, now handles all callbacks coming from the internal WebView (i.e., the "WebView" in "WebViewPlugin"), and WebViewPlugin continues subclassing WebPlugin and RenderViewObserver to observe the outside world. BUG= ========== to ========== Fix plugin placeholders not loading I have made a truly wonderful mess of WebViewPlugin, via: https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297... https://chromium.googlesource.com/chromium/src/+/8ed4bf69eca5e2696aeab731a8e9... My fundamental misunderstanding (I think!) was regarding what WebViewPlugin is listening to via the various interfaces it subclasses. It uses WebFrameClient and WebViewClient to observe its internal WebView, while it uses WebPlugin and RenderViewObserver to interact with the real external page. In the patches above, I mixed and matched messages from those 2 sources because I thought they were coming from the same context. This patch reverts the damage done in those patches, and restores ReplayReceivedData, more-or-less the way it was before I started breaking stuff. It then continues the work I started in https://chromium.googlesource.com/chromium/src/+/8e1badd5fbc054cf4ffc9303e8cb... and moves the subclassing of WebViewClient to a private helper class. This helper, renamed to WebViewHelper, now handles all callbacks coming from the internal WebView (i.e., the "WebView" in "WebViewPlugin"), and WebViewPlugin continues subclassing WebPlugin and RenderViewObserver to observe the outside world. This will hopefully make it harder to screw things up in the same way in the future. BUG=675585 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
japhet@chromium.org changed reviewers: + bauerb@chromium.org, tommycli@chromium.org
In which I try to both repair the damage I have done and make it harder for someone to repeat my mistakes :)
LGTM with some nits: https://codereview.chromium.org/2600253003/diff/1/components/plugins/renderer... File components/plugins/renderer/webview_plugin.cc (right): https://codereview.chromium.org/2600253003/diff/1/components/plugins/renderer... components/plugins/renderer/webview_plugin.cc:92: if (focused_) { Nit: It looks like the style for single-line bodies in this file is to leave out the braces. https://codereview.chromium.org/2600253003/diff/1/components/plugins/renderer... components/plugins/renderer/webview_plugin.cc:253: // ApplyWebPreferences before making a WebLocalFrame so that the frame sees a Nit: Move this comment right before the call to ApplyWebPreferences()? https://codereview.chromium.org/2600253003/diff/1/components/plugins/renderer... components/plugins/renderer/webview_plugin.cc:314: // This should never happen; see also crbug.com/545039 for context. https://crbug.com/545039 has been marked fixed; can we remove this comment and make this a DCHECK?
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
https://codereview.chromium.org/2600253003/diff/1/components/plugins/renderer... File components/plugins/renderer/webview_plugin.cc (right): https://codereview.chromium.org/2600253003/diff/1/components/plugins/renderer... components/plugins/renderer/webview_plugin.cc:92: if (focused_) { On 2017/01/03 17:01:33, Bernhard (just came back) wrote: > Nit: It looks like the style for single-line bodies in this file is to leave out > the braces. Done. I had just reverted to what I had deleted, but no reason not to leave the code better than I found it :) https://codereview.chromium.org/2600253003/diff/1/components/plugins/renderer... components/plugins/renderer/webview_plugin.cc:253: // ApplyWebPreferences before making a WebLocalFrame so that the frame sees a On 2017/01/03 17:01:33, Bernhard (just came back) wrote: > Nit: Move this comment right before the call to ApplyWebPreferences()? Done. https://codereview.chromium.org/2600253003/diff/1/components/plugins/renderer... components/plugins/renderer/webview_plugin.cc:314: // This should never happen; see also crbug.com/545039 for context. On 2017/01/03 17:01:33, Bernhard (just came back) wrote: > https://crbug.com/545039 has been marked fixed; can we remove this comment and > make this a DCHECK? Double-checked with chrishtr, who added the comment/CHECK. He agreed DCHECK was sufficient, but things the comment should remain.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2600253003/#ps20001 (title: "Address bauerb's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483551004009500,
"parent_rev": "8699c193ae01c882dbc035ef4048537f4dc0775c", "commit_rev":
"34ab9ea8ab10cb5c91c27a9adb6b6861151bb840"}
Message was sent while issue was closed.
Description was changed from ========== Fix plugin placeholders not loading I have made a truly wonderful mess of WebViewPlugin, via: https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297... https://chromium.googlesource.com/chromium/src/+/8ed4bf69eca5e2696aeab731a8e9... My fundamental misunderstanding (I think!) was regarding what WebViewPlugin is listening to via the various interfaces it subclasses. It uses WebFrameClient and WebViewClient to observe its internal WebView, while it uses WebPlugin and RenderViewObserver to interact with the real external page. In the patches above, I mixed and matched messages from those 2 sources because I thought they were coming from the same context. This patch reverts the damage done in those patches, and restores ReplayReceivedData, more-or-less the way it was before I started breaking stuff. It then continues the work I started in https://chromium.googlesource.com/chromium/src/+/8e1badd5fbc054cf4ffc9303e8cb... and moves the subclassing of WebViewClient to a private helper class. This helper, renamed to WebViewHelper, now handles all callbacks coming from the internal WebView (i.e., the "WebView" in "WebViewPlugin"), and WebViewPlugin continues subclassing WebPlugin and RenderViewObserver to observe the outside world. This will hopefully make it harder to screw things up in the same way in the future. BUG=675585 ========== to ========== Fix plugin placeholders not loading I have made a truly wonderful mess of WebViewPlugin, via: https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297... https://chromium.googlesource.com/chromium/src/+/8ed4bf69eca5e2696aeab731a8e9... My fundamental misunderstanding (I think!) was regarding what WebViewPlugin is listening to via the various interfaces it subclasses. It uses WebFrameClient and WebViewClient to observe its internal WebView, while it uses WebPlugin and RenderViewObserver to interact with the real external page. In the patches above, I mixed and matched messages from those 2 sources because I thought they were coming from the same context. This patch reverts the damage done in those patches, and restores ReplayReceivedData, more-or-less the way it was before I started breaking stuff. It then continues the work I started in https://chromium.googlesource.com/chromium/src/+/8e1badd5fbc054cf4ffc9303e8cb... and moves the subclassing of WebViewClient to a private helper class. This helper, renamed to WebViewHelper, now handles all callbacks coming from the internal WebView (i.e., the "WebView" in "WebViewPlugin"), and WebViewPlugin continues subclassing WebPlugin and RenderViewObserver to observe the outside world. This will hopefully make it harder to screw things up in the same way in the future. BUG=675585 Review-Url: https://codereview.chromium.org/2600253003 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix plugin placeholders not loading I have made a truly wonderful mess of WebViewPlugin, via: https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297... https://chromium.googlesource.com/chromium/src/+/8ed4bf69eca5e2696aeab731a8e9... My fundamental misunderstanding (I think!) was regarding what WebViewPlugin is listening to via the various interfaces it subclasses. It uses WebFrameClient and WebViewClient to observe its internal WebView, while it uses WebPlugin and RenderViewObserver to interact with the real external page. In the patches above, I mixed and matched messages from those 2 sources because I thought they were coming from the same context. This patch reverts the damage done in those patches, and restores ReplayReceivedData, more-or-less the way it was before I started breaking stuff. It then continues the work I started in https://chromium.googlesource.com/chromium/src/+/8e1badd5fbc054cf4ffc9303e8cb... and moves the subclassing of WebViewClient to a private helper class. This helper, renamed to WebViewHelper, now handles all callbacks coming from the internal WebView (i.e., the "WebView" in "WebViewPlugin"), and WebViewPlugin continues subclassing WebPlugin and RenderViewObserver to observe the outside world. This will hopefully make it harder to screw things up in the same way in the future. BUG=675585 Review-Url: https://codereview.chromium.org/2600253003 ========== to ========== Fix plugin placeholders not loading I have made a truly wonderful mess of WebViewPlugin, via: https://chromium.googlesource.com/chromium/src/+/8489f1bf78c2c6f296e0df6c5297... https://chromium.googlesource.com/chromium/src/+/8ed4bf69eca5e2696aeab731a8e9... My fundamental misunderstanding (I think!) was regarding what WebViewPlugin is listening to via the various interfaces it subclasses. It uses WebFrameClient and WebViewClient to observe its internal WebView, while it uses WebPlugin and RenderViewObserver to interact with the real external page. In the patches above, I mixed and matched messages from those 2 sources because I thought they were coming from the same context. This patch reverts the damage done in those patches, and restores ReplayReceivedData, more-or-less the way it was before I started breaking stuff. It then continues the work I started in https://chromium.googlesource.com/chromium/src/+/8e1badd5fbc054cf4ffc9303e8cb... and moves the subclassing of WebViewClient to a private helper class. This helper, renamed to WebViewHelper, now handles all callbacks coming from the internal WebView (i.e., the "WebView" in "WebViewPlugin"), and WebViewPlugin continues subclassing WebPlugin and RenderViewObserver to observe the outside world. This will hopefully make it harder to screw things up in the same way in the future. BUG=675585 Committed: https://crrev.com/b08418cd6d5f472ca454216aab8fd06cf9a19a36 Cr-Commit-Position: refs/heads/master@{#441401} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b08418cd6d5f472ca454216aab8fd06cf9a19a36 Cr-Commit-Position: refs/heads/master@{#441401} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
