https://codereview.chromium.org/1164883002/diff/1/Source/core/loader/FrameLoader.cpp File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1164883002/diff/1/Source/core/loader/FrameLoader.cpp#newcode977 Source/core/loader/FrameLoader.cpp:977: NavigationDisablerForBeforeUnload navigationDisabler; I have to rename the class still.
4 years, 11 months ago
(2015-06-03 00:51:50 UTC)
#2
renamed the class and fixed up the test, but still discussing the fix in crbug/492851
4 years, 11 months ago
(2015-06-03 19:43:01 UTC)
#3
renamed the class and fixed up the test, but still discussing the fix in
crbug/492851
michaeln
https://codereview.chromium.org/1164883002/diff/1/Source/core/loader/FrameLoader.cpp File Source/core/loader/FrameLoader.cpp (right): https://codereview.chromium.org/1164883002/diff/1/Source/core/loader/FrameLoader.cpp#newcode966 Source/core/loader/FrameLoader.cpp:966: dispatchUnloadEvent(); Maybe a separate issue, but An earlier (pdl ...
4 years, 11 months ago
(2015-06-03 19:43:59 UTC)
#4
4 years, 11 months ago
(2015-06-03 21:05:50 UTC)
#5
trybots look happy with layout tests
dcheng
On 2015/06/03 at 21:05:50, michaeln wrote: > trybots look happy with layout tests I think ...
4 years, 11 months ago
(2015-06-03 21:41:16 UTC)
#6
On 2015/06/03 at 21:05:50, michaeln wrote:
> trybots look happy with layout tests
I think you can simplify the test somewhat. You just need this in an iframe:
<script>
function startTest() {
var req = new XMLHttpRequest();
req.open('GET', '/xmlhttprequest/resources/endlessxml.php');
req.onabort = function() {
console.log('ABORT CALLED');
window.location.hash = 'hashNavigation';
};
req.send(null);
window.top.postMessage('navigate me', '*');
</script>
<body onload="startTest()">Navigate somewhere please</body>
Then in the parent, install an onmessage handler that tries to navigate the
subframe to about:blank, and an iframe onload handler that checks that
window[0].location == 'about:blank' instead of
'path-to-subframe.html#hashNavigation'.
Also, as I mentioned in the bug, let's try to figure out how we're blocking
non-hash navigations in unload/abort handlers, and see if we can do something
similar here instead of using NavigationDisabler.
michaeln
> I think you can simplify the test somewhat. You just need this in an ...
4 years, 11 months ago
(2015-06-03 23:12:22 UTC)
#7
> I think you can simplify the test somewhat. You just need this in an iframe:
simplified
> Also, as I mentioned in the bug, let's try to figure out how we're blocking
> non-hash navigations in unload/abort handlers, and see if we can do something
> similar here instead of using NavigationDisabler.
as is, pageDismissalEventBeingDispatched doesn't help us out here.
michaeln
hola, ptal
4 years, 11 months ago
(2015-06-04 18:44:08 UTC)
#8
hola, ptal
michaeln
ping, is the addition of this line of code ok? if (m_documentLoader) { NavigationDisabler navigationDisabler; ...
4 years, 11 months ago
(2015-06-05 01:29:34 UTC)
#9
ping, is the addition of this line of code ok?
if (m_documentLoader) {
NavigationDisabler navigationDisabler; <---- that one
m_documentLoader->detachFromFrame();
}
4 years, 11 months ago
(2015-06-05 21:08:58 UTC)
#11
looks good to me.
michaeln
I'll make a class like this... class FrameNavigationDisabler public: explicit FrameNavigationDisabler(LocalFrame* frame); ~FrameNavigationDisabler(); static bool ...
4 years, 11 months ago
(2015-06-06 01:29:19 UTC)
#12
I'll make a class like this...
class FrameNavigationDisabler
public:
explicit FrameNavigationDisabler(LocalFrame* frame);
~FrameNavigationDisabler();
static bool isNavigationAllowed(LocalFrame* frame);
};
I see two choices for how to do that.
* localstate: add an instance member m_navigationDisabledCount to LocalFrame
* globalstate: keep a global HashMap<LocalFrame*,int> s_disabledFrames
Do we have a preference?
Nate Chapin
On 2015/06/06 01:29:19, michaeln wrote: > I'll make a class like this... > > class ...
4 years, 10 months ago
(2015-06-08 16:46:35 UTC)
#13
On 2015/06/06 01:29:19, michaeln wrote:
> I'll make a class like this...
>
> class FrameNavigationDisabler
> public:
> explicit FrameNavigationDisabler(LocalFrame* frame);
> ~FrameNavigationDisabler();
>
> static bool isNavigationAllowed(LocalFrame* frame);
> };
>
> I see two choices for how to do that.
> * localstate: add an instance member m_navigationDisabledCount to LocalFrame
> * globalstate: keep a global HashMap<LocalFrame*,int> s_disabledFrames
>
> Do we have a preference?
I'd lean local, but perhaps on NavigationController instead of LocalFrame, since
we'll be enforcing it in NavigationController?
michaeln
ptal In the most recent snapshot NavigationDisablerForBeforeUnload is renamed to simply NavigationDisabler, should i undo ...
4 years, 10 months ago
(2015-06-08 23:04:23 UTC)
#14
ptal
In the most recent snapshot NavigationDisablerForBeforeUnload is renamed to
simply NavigationDisabler, should i undo that change and keep the old name?
> I'd lean local, but perhaps on NavigationController instead of LocalFrame,
since
> we'll be enforcing it in NavigationController?
Done
Nate Chapin
Two global nits: * More descriptive change name than "hashNav" * LayoutTests/http/tests/navigation/resources/navigate-during-commit.iframe.html has multiple "."s. ...
4 years, 10 months ago
(2015-06-09 00:15:20 UTC)
#15
Take a look and dcheng's comments on the bug too. https://codereview.chromium.org/1164883002/diff/100001/Source/core/loader/NavigationScheduler.cpp File Source/core/loader/NavigationScheduler.cpp (right): https://codereview.chromium.org/1164883002/diff/100001/Source/core/loader/NavigationScheduler.cpp#newcode65 ...
4 years, 10 months ago
(2015-06-09 01:15:02 UTC)
#16
> Two global nits: > * More descriptive change name than "hashNav" > * LayoutTests/http/tests/navigation/resources/navigate-during-commit.iframe.html ...
4 years, 10 months ago
(2015-06-09 01:22:02 UTC)
#17
> Two global nits:
> * More descriptive change name than "hashNav"
> *
LayoutTests/http/tests/navigation/resources/navigate-during-commit.iframe.html
> has multiple "."s.
Done
Nate Chapin
LGTM. I think we probably should do most/all of what dcheng suggestsed, but it's probably ...
4 years, 10 months ago
(2015-06-09 15:16:16 UTC)
#18
LGTM. I think we probably should do most/all of what dcheng suggestsed, but it's
probably good to not cram it all into one change.
michaeln
The CQ bit was checked by michaeln@chromium.org
4 years, 10 months ago
(2015-06-09 18:53:33 UTC)
#19
Issue 1164883002: Ignore attempts to navigate once a provisional commit has gotten too far along.
(Closed)
Created 4 years, 11 months ago by michaeln
Modified 4 years, 10 months ago
Reviewers: Nate Chapin, hush (inactive)
Base URL: https://chromium.googlesource.com/chromium/blink.git@master
Comments: 8
Chromium Code Reviews
