Do not prompt for reload when security panel is opened on an interstitial

Do not prompt for reload when security panel is opened on an interstitial

Because interstitials are not real navigations, the security panel
sidebar doesn't work properly for an interstitial, and also doesn't
provide that much more useful information than what's shown in the
Overview view. For this reason, we were already hiding the origins
sidebar when you navigate to an interstitial while the security panel is
open. This CL extends that behavior to when you open DevTools while an
interstitial is already showing.

As a drive-by, this CL also fixes a bug where requests that finished after
the interstitial was shown would still show up in the sidebar.

Also note that this CL only partially addresses the linked bug. This CL
removes the confusing/useless prompt to reload, but doesn't fix the fact
that refreshing on an interstitial does weird things (sends you back to
the previous page). The latter weirdness has been split into
https://crbug.com/669316.

BUG=637299
TEST=Navigate to https://expired.badssl.com. Open the DevTools security
panel. Observe that there is no prompt to reload in the sidebar.

Committed: https://crrev.com/f8d89eaf15d68b685f1d1e0503a2057df1082a75
Cr-Commit-Position: refs/heads/master@{#434887}

[estark, 2016-11-21 22:37:11]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 22:38:12]

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

[estark, 2016-11-21 22:38:20]

Description was changed from

==========
Do not prompt for reload when security panel is opened on an interstitial

Because interstitials are not real navigations, the security panel
sidebar doesn't work properly for an interstitial, and also doesn't
provide that much more useful information than what's shown in the
Overview view. For this reason, we were already hiding the origins
sidebar when you navigate to an interstitial while the security panel is
open. This CL extends that behavior to when you open DevTools while an
interstitial is already showing. To do so, we attach an
interstitialIsShowing boolean to the securityStateChanged event which
gets sent to the security panel when it's opened.

Note that we still need to listen for interstitials shown/hidden while
the security panel is open and we can't rely on the securityStateChanged
event to hear about those interstitials, because there's no guarantee
that a securityStateChanged event will be sent when an interstitial
attaches.

Also note that this CL only partially addresses the bug; it removes the
confusing/useless prompt to reload, but doesn't fix the fact that
refreshing on an interstitial sends you back to the previous page.

BUG=637299
==========

to

==========
Do not prompt for reload when security panel is opened on an interstitial

Because interstitials are not real navigations, the security panel
sidebar doesn't work properly for an interstitial, and also doesn't
provide that much more useful information than what's shown in the
Overview view. For this reason, we were already hiding the origins
sidebar when you navigate to an interstitial while the security panel is
open. This CL extends that behavior to when you open DevTools while an
interstitial is already showing. To do so, we attach an
interstitialIsShowing boolean to the securityStateChanged event which
gets sent to the security panel when it's opened.

Note that we still need to listen for interstitials shown/hidden while
the security panel is open and we can't rely on the securityStateChanged
event to hear about those interstitials, because there's no guarantee
that a securityStateChanged event will be sent when an interstitial
attaches.

Also note that this CL only partially addresses the linked bug. This CL
removes the confusing/useless prompt to reload, but doesn't fix the fact
that refreshing on an interstitial does weird things (sends you back to
the previous page).

BUG=637299
==========

[estark, 2016-11-21 22:40:17]

Description was changed from

==========
Do not prompt for reload when security panel is opened on an interstitial

Because interstitials are not real navigations, the security panel
sidebar doesn't work properly for an interstitial, and also doesn't
provide that much more useful information than what's shown in the
Overview view. For this reason, we were already hiding the origins
sidebar when you navigate to an interstitial while the security panel is
open. This CL extends that behavior to when you open DevTools while an
interstitial is already showing. To do so, we attach an
interstitialIsShowing boolean to the securityStateChanged event which
gets sent to the security panel when it's opened.

Note that we still need to listen for interstitials shown/hidden while
the security panel is open and we can't rely on the securityStateChanged
event to hear about those interstitials, because there's no guarantee
that a securityStateChanged event will be sent when an interstitial
attaches.

Also note that this CL only partially addresses the linked bug. This CL
removes the confusing/useless prompt to reload, but doesn't fix the fact
that refreshing on an interstitial does weird things (sends you back to
the previous page).

BUG=637299
==========

to

==========
Do not prompt for reload when security panel is opened on an interstitial

Because interstitials are not real navigations, the security panel
sidebar doesn't work properly for an interstitial, and also doesn't
provide that much more useful information than what's shown in the
Overview view. For this reason, we were already hiding the origins
sidebar when you navigate to an interstitial while the security panel is
open. This CL extends that behavior to when you open DevTools while an
interstitial is already showing. To do so, we attach an
interstitialIsShowing boolean to the securityStateChanged event which
gets sent to the security panel when it's opened.

As a drive-by, this CL also fixes a bug where requests that finished after
the interstitial was shown would still show up in the sidebar.

Note that we still need to listen for interstitials shown/hidden while
the security panel is open and we can't rely on the securityStateChanged
event to hear about those interstitials, because there's no guarantee
that a securityStateChanged event will be sent when an interstitial
attaches.

Also note that this CL only partially addresses the linked bug. This CL
removes the confusing/useless prompt to reload, but doesn't fix the fact
that refreshing on an interstitial does weird things (sends you back to
the previous page).

BUG=637299
==========

[estark, 2016-11-21 22:40:24]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 22:40:40]

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

[estark, 2016-11-21 22:42:05]

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

[estark, 2016-11-21 22:42:06]

dgozman, can you please review?

https://codereview.chromium.org/2522733002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2522733002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:496:
newParent.hidden = this._originsAreHidden;
This is the drive-by bug fix I mentioned in the CL description: once an
interstitial is shown and the sidebar origins list is hidden, requests that
finish after that should also be hidden.

[commit-bot: I haz the power, 2016-11-22 00:55:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 00:55:13]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-11-22 21:44:38]

https://codereview.chromium.org/2522733002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2522733002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:906: { "name":
"interstitialIsShowing", "type": "boolean", "description": "True if an
interstitial is currently showing.", "optional": true }
Instead of this, we should send Page.interstitialShown event when client does
Page.enable with visible interstitial. Then we don't need a separate event and
it will all be handled by existing code.

[estark, 2016-11-22 23:04:54]

On 2016/11/22 21:44:38, dgozman wrote:
>
https://codereview.chromium.org/2522733002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):
> 
>
https://codereview.chromium.org/2522733002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/inspector/browser_protocol.json:906: { "name":
> "interstitialIsShowing", "type": "boolean", "description": "True if an
> interstitial is currently showing.", "optional": true }
> Instead of this, we should send Page.interstitialShown event when client does
> Page.enable with visible interstitial. Then we don't need a separate event and
> it will all be handled by existing code.

I tried that initially, but it didn't seem to work, I think because the security
panel wasn't enabled/attached yet. Does that sound plausible to you? (IIRC I was
triggering the event in PageHandler::SetRenderFrameHost. Maybe Enable would
work? I can debug it more tonight or tomorrow.)

[dgozman, 2016-11-23 01:19:13]

On 2016/11/22 23:04:54, estark wrote:
> On 2016/11/22 21:44:38, dgozman wrote:
> >
>
https://codereview.chromium.org/2522733002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):
> > 
> >
>
https://codereview.chromium.org/2522733002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/inspector/browser_protocol.json:906: {
"name":
> > "interstitialIsShowing", "type": "boolean", "description": "True if an
> > interstitial is currently showing.", "optional": true }
> > Instead of this, we should send Page.interstitialShown event when client
does
> > Page.enable with visible interstitial. Then we don't need a separate event
and
> > it will all be handled by existing code.
> 
> I tried that initially, but it didn't seem to work, I think because the
security
> panel wasn't enabled/attached yet. Does that sound plausible to you? (IIRC I
was
> triggering the event in PageHandler::SetRenderFrameHost. Maybe Enable would
> work? I can debug it more tonight or tomorrow.)

I think it should work. Since we should handle the flag on the SecurityModel
level, it doesn't matter whether panel is there yet.

[estark, 2016-11-28 22:59:51]

On 2016/11/23 01:19:13, dgozman wrote:
> On 2016/11/22 23:04:54, estark wrote:
> > On 2016/11/22 21:44:38, dgozman wrote:
> > >
> >
>
https://codereview.chromium.org/2522733002/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/inspector/browser_protocol.json
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2522733002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/inspector/browser_protocol.json:906: {
> "name":
> > > "interstitialIsShowing", "type": "boolean", "description": "True if an
> > > interstitial is currently showing.", "optional": true }
> > > Instead of this, we should send Page.interstitialShown event when client
> does
> > > Page.enable with visible interstitial. Then we don't need a separate event
> and
> > > it will all be handled by existing code.
> > 
> > I tried that initially, but it didn't seem to work, I think because the
> security
> > panel wasn't enabled/attached yet. Does that sound plausible to you? (IIRC I
> was
> > triggering the event in PageHandler::SetRenderFrameHost. Maybe Enable would
> > work? I can debug it more tonight or tomorrow.)
> 
> I think it should work. Since we should handle the flag on the SecurityModel
> level, it doesn't matter whether panel is there yet.

Sorry for the delay. We're listening for the interstitial events on
SecurityPanel, though, not SecurityModel, so the panel object isn't around to
receive them. Should I be listening for the interstitialShown/interstitialHidden
on SecurityModel instead?

[dgozman, 2016-11-29 00:09:32]

On 2016/11/28 22:59:51, estark wrote:
> On 2016/11/23 01:19:13, dgozman wrote:
> > On 2016/11/22 23:04:54, estark wrote:
> > > On 2016/11/22 21:44:38, dgozman wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2522733002/diff/20001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/inspector/browser_protocol.json
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2522733002/diff/20001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/inspector/browser_protocol.json:906: {
> > "name":
> > > > "interstitialIsShowing", "type": "boolean", "description": "True if an
> > > > interstitial is currently showing.", "optional": true }
> > > > Instead of this, we should send Page.interstitialShown event when client
> > does
> > > > Page.enable with visible interstitial. Then we don't need a separate
event
> > and
> > > > it will all be handled by existing code.
> > > 
> > > I tried that initially, but it didn't seem to work, I think because the
> > security
> > > panel wasn't enabled/attached yet. Does that sound plausible to you? (IIRC
I
> > was
> > > triggering the event in PageHandler::SetRenderFrameHost. Maybe Enable
would
> > > work? I can debug it more tonight or tomorrow.)
> > 
> > I think it should work. Since we should handle the flag on the SecurityModel
> > level, it doesn't matter whether panel is there yet.
> 
> Sorry for the delay. We're listening for the interstitial events on
> SecurityPanel, though, not SecurityModel, so the panel object isn't around to
> receive them. Should I be listening for the
interstitialShown/interstitialHidden
> on SecurityModel instead?

I think it would be easier to have a flag _isInterstitialShown in
ResourceTreeModel instead, since it receives the actual events from backend.

[estark, 2016-11-29 01:47:22]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[estark, 2016-11-29 01:48:31]

Description was changed from

==========
Do not prompt for reload when security panel is opened on an interstitial

Because interstitials are not real navigations, the security panel
sidebar doesn't work properly for an interstitial, and also doesn't
provide that much more useful information than what's shown in the
Overview view. For this reason, we were already hiding the origins
sidebar when you navigate to an interstitial while the security panel is
open. This CL extends that behavior to when you open DevTools while an
interstitial is already showing. To do so, we attach an
interstitialIsShowing boolean to the securityStateChanged event which
gets sent to the security panel when it's opened.

As a drive-by, this CL also fixes a bug where requests that finished after
the interstitial was shown would still show up in the sidebar.

Note that we still need to listen for interstitials shown/hidden while
the security panel is open and we can't rely on the securityStateChanged
event to hear about those interstitials, because there's no guarantee
that a securityStateChanged event will be sent when an interstitial
attaches.

Also note that this CL only partially addresses the linked bug. This CL
removes the confusing/useless prompt to reload, but doesn't fix the fact
that refreshing on an interstitial does weird things (sends you back to
the previous page).

BUG=637299
==========

to

==========
Do not prompt for reload when security panel is opened on an interstitial

Because interstitials are not real navigations, the security panel
sidebar doesn't work properly for an interstitial, and also doesn't
provide that much more useful information than what's shown in the
Overview view. For this reason, we were already hiding the origins
sidebar when you navigate to an interstitial while the security panel is
open. This CL extends that behavior to when you open DevTools while an
interstitial is already showing.

As a drive-by, this CL also fixes a bug where requests that finished after
the interstitial was shown would still show up in the sidebar.

Also note that this CL only partially addresses the linked bug. This CL
removes the confusing/useless prompt to reload, but doesn't fix the fact
that refreshing on an interstitial does weird things (sends you back to
the previous page). The latter weirdness has been split into
https://crbug.com/669316.

BUG=637299
TEST=Navigate to https://expired.badssl.com. Open the DevTools security
panel. Observe that there is no prompt to reload in the sidebar.
==========

[commit-bot: I haz the power, 2016-11-29 01:48:44]

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

[estark, 2016-11-29 01:49:36]

On 2016/11/29 00:09:32, dgozman wrote:
> On 2016/11/28 22:59:51, estark wrote:
> > On 2016/11/23 01:19:13, dgozman wrote:
> > > On 2016/11/22 23:04:54, estark wrote:
> > > > On 2016/11/22 21:44:38, dgozman wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2522733002/diff/20001/third_party/WebKit/Sour...
> > > > > File third_party/WebKit/Source/core/inspector/browser_protocol.json
> > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2522733002/diff/20001/third_party/WebKit/Sour...
> > > > > third_party/WebKit/Source/core/inspector/browser_protocol.json:906: {
> > > "name":
> > > > > "interstitialIsShowing", "type": "boolean", "description": "True if an
> > > > > interstitial is currently showing.", "optional": true }
> > > > > Instead of this, we should send Page.interstitialShown event when
client
> > > does
> > > > > Page.enable with visible interstitial. Then we don't need a separate
> event
> > > and
> > > > > it will all be handled by existing code.
> > > > 
> > > > I tried that initially, but it didn't seem to work, I think because the
> > > security
> > > > panel wasn't enabled/attached yet. Does that sound plausible to you?
(IIRC
> I
> > > was
> > > > triggering the event in PageHandler::SetRenderFrameHost. Maybe Enable
> would
> > > > work? I can debug it more tonight or tomorrow.)
> > > 
> > > I think it should work. Since we should handle the flag on the
SecurityModel
> > > level, it doesn't matter whether panel is there yet.
> > 
> > Sorry for the delay. We're listening for the interstitial events on
> > SecurityPanel, though, not SecurityModel, so the panel object isn't around
to
> > receive them. Should I be listening for the
> interstitialShown/interstitialHidden
> > on SecurityModel instead?
> 
> I think it would be easier to have a flag _isInterstitialShown in
> ResourceTreeModel instead, since it receives the actual events from backend.

Ah, I see. PTAL at patchset #3.

[dgozman, 2016-11-29 02:07:23]

Nice work! lgtm

https://codereview.chromium.org/2522733002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js
(right):

https://codereview.chromium.org/2522733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:877:
this._resourceTreeModel.setIsInterstitialShowing(true);
Let's just assign directly to a private field here (it's file-private), and do
not expose the setter.

[estark, 2016-11-29 02:11:36]

Thanks dgozman!

https://codereview.chromium.org/2522733002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js
(right):

https://codereview.chromium.org/2522733002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:877:
this._resourceTreeModel.setIsInterstitialShowing(true);
On 2016/11/29 02:07:23, dgozman wrote:
> Let's just assign directly to a private field here (it's file-private), and do
> not expose the setter.

Done.

[estark, 2016-11-29 02:11:48]

The CQ bit was checked by estark@chromium.org

[estark, 2016-11-29 02:11:49]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2522733002/#ps60001
(title: "remove isInterstitialShowing setter")

[commit-bot: I haz the power, 2016-11-29 02:12:41]

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, 2016-11-29 04:16:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480385508807520,
"parent_rev": "8027a4da36a614c5dd63757b0329866d6e482e63", "commit_rev":
"d10c3d3d06024fa9c34b72cb1f39322c5dc70887"}

[commit-bot: I haz the power, 2016-11-29 04:17:04]

Description was changed from

==========
Do not prompt for reload when security panel is opened on an interstitial

Because interstitials are not real navigations, the security panel
sidebar doesn't work properly for an interstitial, and also doesn't
provide that much more useful information than what's shown in the
Overview view. For this reason, we were already hiding the origins
sidebar when you navigate to an interstitial while the security panel is
open. This CL extends that behavior to when you open DevTools while an
interstitial is already showing.

As a drive-by, this CL also fixes a bug where requests that finished after
the interstitial was shown would still show up in the sidebar.

Also note that this CL only partially addresses the linked bug. This CL
removes the confusing/useless prompt to reload, but doesn't fix the fact
that refreshing on an interstitial does weird things (sends you back to
the previous page). The latter weirdness has been split into
https://crbug.com/669316.

BUG=637299
TEST=Navigate to https://expired.badssl.com. Open the DevTools security
panel. Observe that there is no prompt to reload in the sidebar.
==========

to

==========
Do not prompt for reload when security panel is opened on an interstitial

Because interstitials are not real navigations, the security panel
sidebar doesn't work properly for an interstitial, and also doesn't
provide that much more useful information than what's shown in the
Overview view. For this reason, we were already hiding the origins
sidebar when you navigate to an interstitial while the security panel is
open. This CL extends that behavior to when you open DevTools while an
interstitial is already showing.

As a drive-by, this CL also fixes a bug where requests that finished after
the interstitial was shown would still show up in the sidebar.

Also note that this CL only partially addresses the linked bug. This CL
removes the confusing/useless prompt to reload, but doesn't fix the fact
that refreshing on an interstitial does weird things (sends you back to
the previous page). The latter weirdness has been split into
https://crbug.com/669316.

BUG=637299
TEST=Navigate to https://expired.badssl.com. Open the DevTools security
panel. Observe that there is no prompt to reload in the sidebar.
==========

[commit-bot: I haz the power, 2016-11-29 04:17:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-29 04:22:23]

Description was changed from

==========
Do not prompt for reload when security panel is opened on an interstitial

Because interstitials are not real navigations, the security panel
sidebar doesn't work properly for an interstitial, and also doesn't
provide that much more useful information than what's shown in the
Overview view. For this reason, we were already hiding the origins
sidebar when you navigate to an interstitial while the security panel is
open. This CL extends that behavior to when you open DevTools while an
interstitial is already showing.

As a drive-by, this CL also fixes a bug where requests that finished after
the interstitial was shown would still show up in the sidebar.

Also note that this CL only partially addresses the linked bug. This CL
removes the confusing/useless prompt to reload, but doesn't fix the fact
that refreshing on an interstitial does weird things (sends you back to
the previous page). The latter weirdness has been split into
https://crbug.com/669316.

BUG=637299
TEST=Navigate to https://expired.badssl.com. Open the DevTools security
panel. Observe that there is no prompt to reload in the sidebar.
==========

to

==========
Do not prompt for reload when security panel is opened on an interstitial

Because interstitials are not real navigations, the security panel
sidebar doesn't work properly for an interstitial, and also doesn't
provide that much more useful information than what's shown in the
Overview view. For this reason, we were already hiding the origins
sidebar when you navigate to an interstitial while the security panel is
open. This CL extends that behavior to when you open DevTools while an
interstitial is already showing.

As a drive-by, this CL also fixes a bug where requests that finished after
the interstitial was shown would still show up in the sidebar.

Also note that this CL only partially addresses the linked bug. This CL
removes the confusing/useless prompt to reload, but doesn't fix the fact
that refreshing on an interstitial does weird things (sends you back to
the previous page). The latter weirdness has been split into
https://crbug.com/669316.

BUG=637299
TEST=Navigate to https://expired.badssl.com. Open the DevTools security
panel. Observe that there is no prompt to reload in the sidebar.

Committed: https://crrev.com/f8d89eaf15d68b685f1d1e0503a2057df1082a75
Cr-Commit-Position: refs/heads/master@{#434887}
==========

[commit-bot: I haz the power, 2016-11-29 04:22:24]

Patchset 4 (id:??) landed as
https://crrev.com/f8d89eaf15d68b685f1d1e0503a2057df1082a75
Cr-Commit-Position: refs/heads/master@{#434887}