Fix broken copy-paste operations on Interstitial pages.

Fix broken copy-paste operations on Interstitial pages.

The RenderFrameHostDelegate::SetFocusedFrame function was added to keep track
of inner web contents but the base implementation in the delegate that is a
no-op broken interstitial pages which also need to keep track of the focused
frame.

BUG=635656
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2727483003
Cr-Commit-Position: refs/heads/master@{#454038}
Committed: https://chromium.googlesource.com/chromium/src/+/0ae3030718bc7e0673868ae81b33a3cccd663e62

[avallee, 2017-03-01 19:33:59]

avallee@chromium.org changed reviewers:
+ alexmos@chromium.org

[alexmos, 2017-03-01 19:40:19]

LGTM, thanks for fixing!  It'd be nice to have a test for this (fine for a
followup if you want to get this in before branch point).

[avallee, 2017-03-01 20:20:26]

The CQ bit was checked by avallee@chromium.org

[commit-bot: I haz the power, 2017-03-01 20:21:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 21:27:45]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1488399626678900, "parent_rev":
"d4337ecc7263a8707a582b006c99ede5fff76ea2", "commit_rev":
"0ae3030718bc7e0673868ae81b33a3cccd663e62"}

[commit-bot: I haz the power, 2017-03-01 21:28:25]

Description was changed from

==========
Fix broken copy-paste operations on Interstitial pages.

The RenderFrameHostDelegate::SetFocusedFrame function was added to keep track
of inner web contents but the base implementation in the delegate that is a
no-op broken interstitial pages which also need to keep track of the focused
frame.

BUG=635656
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix broken copy-paste operations on Interstitial pages.

The RenderFrameHostDelegate::SetFocusedFrame function was added to keep track
of inner web contents but the base implementation in the delegate that is a
no-op broken interstitial pages which also need to keep track of the focused
frame.

BUG=635656
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2727483003
Cr-Commit-Position: refs/heads/master@{#454038}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ae3030718bc7e0673868ae81b33...
==========

[commit-bot: I haz the power, 2017-03-01 21:28:26]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/0ae3030718bc7e0673868ae81b33...

[estark, 2017-03-01 22:59:23]

estark@chromium.org changed reviewers:
+ estark@chromium.org

[estark, 2017-03-01 22:59:24]

Random drive-by question, do either of you understand why
InterstitialPageImplTest.Copy didn't catch this regression? I wonder if that
test might be broken or not testing what it's supposed to be testing?

[alexmos, 2017-03-01 23:02:58]

On 2017/03/01 22:59:24, estark wrote:
> Random drive-by question, do either of you understand why
> InterstitialPageImplTest.Copy didn't catch this regression? I wonder if that
> test might be broken or not testing what it's supposed to be testing?

avallee@ has just fixed that test in
https://codereview.chromium.org/2728753002/, and now it should cover this.  The
reason was that InterstitialPageImplTest was calling SetFocusedFrame directly on
the FrameTree, rather than going through RenderFrameHostDelegate like regular
call (and the bug was in the delegate implementation of it).

[estark, 2017-03-01 23:04:07]

On 2017/03/01 23:02:58, alexmos wrote:
> On 2017/03/01 22:59:24, estark wrote:
> > Random drive-by question, do either of you understand why
> > InterstitialPageImplTest.Copy didn't catch this regression? I wonder if that
> > test might be broken or not testing what it's supposed to be testing?
> 
> avallee@ has just fixed that test in
> https://codereview.chromium.org/2728753002/, and now it should cover this. 
The
> reason was that InterstitialPageImplTest was calling SetFocusedFrame directly
on
> the FrameTree, rather than going through RenderFrameHostDelegate like regular
> call (and the bug was in the delegate implementation of it).

Thanks for the explanation!

[nasko, 2017-03-01 23:05:00]

On 2017/03/01 22:59:24, estark wrote:
> Random drive-by question, do either of you understand why
> InterstitialPageImplTest.Copy didn't catch this regression? I wonder if that
> test might be broken or not testing what it's supposed to be testing?

My guess will be that the SetUpInterstitialPage code explicitly sets the focused
frame, which likely doesn't happen in reality.
https://cs.chromium.org/chromium/src/content/browser/frame_host/interstitial_....
It might be worthwhile removing that line of code and seeing if it still works
and verifying whether it would've caught the issue without this particular fix.

[avallee, 2017-03-02 15:42:31]

On 2017/03/01 23:05:00, nasko (slow) wrote:
> On 2017/03/01 22:59:24, estark wrote:
> > Random drive-by question, do either of you understand why
> > InterstitialPageImplTest.Copy didn't catch this regression? I wonder if that
> > test might be broken or not testing what it's supposed to be testing?
> 
> My guess will be that the SetUpInterstitialPage code explicitly sets the
focused
> frame, which likely doesn't happen in reality.
>
https://cs.chromium.org/chromium/src/content/browser/frame_host/interstitial_....
> It might be worthwhile removing that line of code and seeing if it still works
> and verifying whether it would've caught the issue without this particular
fix.

Worth noting that InterstitialPage seems to be used quite differently on MacOS
compared to all the other platforms. Is this worth unifying in order to avoid
this type of bug?

[nasko, 2017-03-03 21:27:23]

On 2017/03/02 15:42:31, avallee wrote:
> On 2017/03/01 23:05:00, nasko (slow) wrote:
> > On 2017/03/01 22:59:24, estark wrote:
> > > Random drive-by question, do either of you understand why
> > > InterstitialPageImplTest.Copy didn't catch this regression? I wonder if
that
> > > test might be broken or not testing what it's supposed to be testing?
> > 
> > My guess will be that the SetUpInterstitialPage code explicitly sets the
> focused
> > frame, which likely doesn't happen in reality.
> >
>
https://cs.chromium.org/chromium/src/content/browser/frame_host/interstitial_....
> > It might be worthwhile removing that line of code and seeing if it still
works
> > and verifying whether it would've caught the issue without this particular
> fix.
> 
> Worth noting that InterstitialPage seems to be used quite differently on MacOS
> compared to all the other platforms. Is this worth unifying in order to avoid
> this type of bug?

I don't think so. We have a plan for completely redoing how interstitial pages
work, so any effort invested in making the current code consistent is time that
can be used towards actually fixing it right.

[avallee, 2017-03-03 21:38:59]

Thanks for the heads up.

On Fri, Mar 3, 2017, 4:27 PM <nasko@chromium.org> wrote:

> On 2017/03/02 15:42:31, avallee wrote:
> > On 2017/03/01 23:05:00, nasko (slow) wrote:
> > > On 2017/03/01 22:59:24, estark wrote:
> > > > Random drive-by question, do either of you understand why
> > > > InterstitialPageImplTest.Copy didn't catch this regression? I wonder
> if
> that
> > > > test might be broken or not testing what it's supposed to be testing?
> > >
> > > My guess will be that the SetUpInterstitialPage code explicitly sets
> the
> > focused
> > > frame, which likely doesn't happen in reality.
> > >
> >
>
>
https://cs.chromium.org/chromium/src/content/browser/frame_host/interstitial_...
> .
> > > It might be worthwhile removing that line of code and seeing if it
> still
> works
> > > and verifying whether it would've caught the issue without this
> particular
> > fix.
> >
> > Worth noting that InterstitialPage seems to be used quite differently on
> MacOS
> > compared to all the other platforms. Is this worth unifying in order to
> avoid
> > this type of bug?
>
> I don't think so. We have a plan for completely redoing how interstitial
> pages
> work, so any effort invested in making the current code consistent is time
> that
> can be used towards actually fixing it right.
>
> https://codereview.chromium.org/2727483003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.