Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1335023004:
Prepare MimeHanderViewContainer for removing swapped out RenderFrame. (Closed)
|
Created:
5 years, 3 months ago by nasko Modified:
5 years, 3 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrepare MimeHanderViewContainer for removing swapped out RenderFrame.
The current implementation of MimeHanderViewContainer is dependent on
the usage of swapped out RenderFrame for the out-of-process component.
Since swapped out state is going away, this CL removes this dependency.
Credit goes to dcheng@, who found the simplest solution.
BUG=357747
Committed: https://crrev.com/5c749f3a0fad10de6b56d49351f9b99907f16750
Cr-Commit-Position: refs/heads/master@{#348666}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add a TODO. #Messages
Total messages: 18 (5 generated)
nasko@chromium.org changed reviewers: + fsamuel@chromium.org, raymes@chromium.org
Hey Raymes and Fady, Can you review this CL? dcheng@ found a much cleaner and simple solution to get this fixed at least without out-of-process iframes. Thanks in advance! Nasko
https://codereview.chromium.org/1335023004/diff/1/extensions/renderer/guest_v... File extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc (right): https://codereview.chromium.org/1335023004/diff/1/extensions/renderer/guest_v... extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc:259: render_frame()->GetWebFrame()->callFunctionEvenIfScriptDisabled( I thought we also wanted to avoid using callFunctionEvenIfScriptDisabled?
https://codereview.chromium.org/1335023004/diff/1/extensions/renderer/guest_v... File extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc (right): https://codereview.chromium.org/1335023004/diff/1/extensions/renderer/guest_v... extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc:259: render_frame()->GetWebFrame()->callFunctionEvenIfScriptDisabled( On 2015/09/14 15:38:05, Fady Samuel wrote: > I thought we also wanted to avoid using callFunctionEvenIfScriptDisabled? Yes, but on WebRemoteFrame, since it is not implemented. This calls it on the WebLocalFrame, which means we plumb the postMessage call almost identically to how it works in regular cases.
https://codereview.chromium.org/1335023004/diff/1/extensions/renderer/guest_v... File extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc (right): https://codereview.chromium.org/1335023004/diff/1/extensions/renderer/guest_v... extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc:241: if (guest_proxy_frame->isWebLocalFrame()) { A comment or helper method would be very useful here because it's not clear what's going on.
LGTM + nit (a comment would be super helpful here).
https://codereview.chromium.org/1335023004/diff/1/extensions/renderer/guest_v... File extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc (right): https://codereview.chromium.org/1335023004/diff/1/extensions/renderer/guest_v... extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc:241: if (guest_proxy_frame->isWebLocalFrame()) { On 2015/09/14 15:39:26, Fady Samuel wrote: > A comment or helper method would be very useful here because it's not clear > what's going on. Done.
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1335023004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1335023004/20001
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 nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/1335023004/#ps20001 (title: "Add a TODO.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1335023004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1335023004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5c749f3a0fad10de6b56d49351f9b99907f16750 Cr-Commit-Position: refs/heads/master@{#348666}
Message was sent while issue was closed.
Nice work guys! :) On Tue, 15 Sep 2015 at 04:40 commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Patchset 2 (id:??) landed as > https://crrev.com/5c749f3a0fad10de6b56d49351f9b99907f16750 > Cr-Commit-Position: refs/heads/master@{#348666} > > https://codereview.chromium.org/1335023004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5c749f3a0fad10de6b56d49351f9b99907f16750 Cr-Commit-Position: refs/heads/master@{#348666} |
