Expose page dismissal event status through the WebKit API for chromium
*** Bug 55845 has been marked as a duplicate of this bug. ***
Chromium will find it useful to know the page dismissal status (i.e., whether a frame is in the midst of executing a beforeunload or unload handler), so expose that information through the chromium port in the WebKit API.
Created attachment 84891 [details] Patch
Attachment 84891 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8102316
Created attachment 84892 [details] Patch
This looks ok, but can you help a bit explaining how this is useful?
(In reply to comment #6) > This looks ok, but can you help a bit explaining how this is useful? In Chromium, we'd like to have the ability to suppress javascript messages (alerts, prompts, confirm dialogs) during page dismissal (as per http://crbug.com/68780). To do that, when a javascript message is being dispatched, we need to know whether an unload handler is being run. Currently, the API doesn't provide a way to know that; this patch allows that query.
Comment on attachment 84892 [details] Patch ok.
Thanks for the quick review and approval, Dimitri. It looks like the commit queue is repeatedly choking on the patch, failing an unrelated layout test (accessibility/aria-activedescendant-crash.html -> timed out). That test passes on my local client. In any case, it has nothing to do with this patch, so am wondering how to get this landed. Help?
Comment on attachment 84892 [details] Patch Clearing flags on attachment: 84892 Committed r80436: <http://trac.webkit.org/changeset/80436>
All reviewed patches have been landed. Closing bug.
(In reply to comment #11) > All reviewed patches have been landed. Closing bug. Sadly, this broke Chromium. Have you tested the patch against Chromium build?
Rolled out in http://trac.webkit.org/changeset/80437 -- broke Chromium compile.
Yikes. Yes, I built chromium, and it built fine on my machine. Sorry about the breakage. Thanks for reverting the change.
Created attachment 84911 [details] Second attempt
Comment on attachment 84911 [details] Second attempt activate the photon torpedoes.
Comment on attachment 84911 [details] Second attempt Clearing flags on attachment: 84911 Committed r80451: <http://trac.webkit.org/changeset/80451>
Comment on attachment 84911 [details] Second attempt View in context: https://bugs.webkit.org/attachment.cgi?id=84911&action=review > Source/WebKit/chromium/public/WebFrame.h:370 > + virtual bool pageDismissalEventBeingDispatched() const = 0; have the events already been dispatched or are they just about to be dispatched? usually, we name methods like this using the "did" or "will" prefix. willDispatchUnloadEvents or didDispatchUnloadEvents would be a good choices here. also, i'm a bit confused as to how this will be used. if the beforeunload dialog is cancelled (i.e., if the new navigation is cancelled because the user wanted to stay on the page), then how will the embedder know that? also, i don't see why this behavior change wouldn't be implemented entirely within webkit. i see no reason for their to be a chromium side to this behavior change. why do you need a webkit api?
(In reply to comment #19) > (From update of attachment 84911 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84911&action=review > > > Source/WebKit/chromium/public/WebFrame.h:370 > > + virtual bool pageDismissalEventBeingDispatched() const = 0; > > have the events already been dispatched or are they just about to be dispatched? > usually, we name methods like this using the "did" or "will" prefix. > > willDispatchUnloadEvents or didDispatchUnloadEvents would be a good choices here. It's actually in the "midst" of being dispatched. I.e., the FrameLoader has told the frame to dispatch the unload event; the event may block (e.g.: if there's an alert() within the event handler). Once the event handlers are done, the dispatch is "done". This API mirrors the method of the same name in FrameLoader. See how the underlying variable is set and unset at: http://trac.webkit.org/browser/trunk/Source/WebCore/loader/FrameLoader.cpp#L377(for the unload event) http://trac.webkit.org/browser/trunk/Source/WebCore/loader/FrameLoader.cpp#L2926 (for the beforeunload event) > also, i'm a bit confused as to how this will be used. if the beforeunload dialog > is cancelled (i.e., if the new navigation is cancelled because the user wanted to > stay on the page), then how will the embedder know that? This doesn't affect the beforeunload dialog. I.e., only the actual onbeforeunload() and onunload() handlers (and not the "stay or leave" dialog) are considered to be "page dismissal event" in progress. Sequence of events (as per line 2926 in FrameLoader.cpp): 1. set m_pageDismissalEventBeingDispatched = true. 2. call onbeforeunload(). 3. // perhaps it fires an alert(), which is where this API will be useful. 4. onbeforeunload() returns a value (see step 6). 5. set m_pageDismissalEventBeingDispatched = false. 6. if the return value was not NULL, dispatch the "stay or leave" dialog. > also, i don't see why this behavior change wouldn't be implemented entirely within > webkit. i see no reason for their to be a chromium side to this behavior change. > why do you need a webkit api? I think the decision on whether alert()s should be ignored during unload events should be handled on the Chromium side. Safari and other webkit clients may not want to disable alerts (which is the current behaviour in all browsers). In addition, even in Chromium, we don't always want to ignore alerts (e.g.: I think extension_host doesn't want to).
> > also, i don't see why this behavior change wouldn't be implemented entirely within > > webkit. i see no reason for their to be a chromium side to this behavior change. > > why do you need a webkit api? > > I think the decision on whether alert()s should be ignored during unload events should be handled on the Chromium side. Safari and other webkit clients may not want to disable alerts (which is the current behaviour in all browsers). In addition, even in Chromium, we don't always want to ignore alerts (e.g.: I think extension_host doesn't want to). I have to disagree. We are trying to change the behavior of the web platform here, and the definition of the web platform should reside as much as possible within the webkit repository and not be spread between multiple repositories. We should be championing this breaking change on the working groups and experimenting with this implementation in webkit behind a compile time flag to inform the html spec.
(In reply to comment #21) > > I think the decision on whether alert()s should be ignored during unload events should be handled on the Chromium side. Safari and other webkit clients may not want to disable alerts (which is the current behaviour in all browsers). In addition, even in Chromium, we don't always want to ignore alerts (e.g.: I think extension_host doesn't want to). > > I have to disagree. We are trying to change the behavior of the web platform here, and the definition of the web platform should reside as much as possible within the webkit repository and not be spread between multiple repositories. > > We should be championing this breaking change on the working groups and experimenting with this implementation in webkit behind a compile time flag to inform the html spec. I agree (that it should be standard behaviour). I went this route only because of what Ojan said at http://crbug.com/68780#c3 I think it'd be nice to implement this in Chromium for now, show that the web isn't horribly broken because of it (e.g.: no complaints about it); such evidence might give us better standing to argue for it to become standard behaviour. Should I withdraw this API and the CL in progress at http://codereview.chromium.org/6627063/?
(In reply to comment #22) > Should I withdraw this API and the CL in progress at http://codereview.chromium.org/6627063/? Er, I meant to add: Is it okay if I implement this entirely in the Chromium port of the API (i.e., in ChromeClientImpl; thus not needing any #defines), or would you like to see this in WebCore (#defined out for other clients for now)?
(In reply to comment #23) > (In reply to comment #22) > > Should I withdraw this API and the CL in progress at http://codereview.chromium.org/6627063/? > > Er, I meant to add: Is it okay if I implement this entirely in the Chromium port of the API (i.e., in ChromeClientImpl; thus not needing any #defines), or would you like to see this in WebCore (#defined out for other clients for now)? In general, I think we are best served when our implementation does not suffer from unusual constraints like this. It is better to do the cleanest possible implementation even if it has to be behind #defines. That way, your code will be easier to maintain in the future. At the very least, I think it should be completely implemented in WebKit and there should be accompanying layout tests. This way, once we prove that it is safe for the web, the cost for other WebKit ports to adopt this behavior change will be as close to zero as possible. If we don't do the above, then there will be additional work to redo the patch to be a proper WebCore patch. That adds a more cost, and there will be less incentives on our part to do this work, so we should just do it properly from the start. Pros: less engineering work, Cons: ugly #ifdefs. Seems like the pros outweigh the cons.
(In reply to comment #24) > At the very least, I think it should be completely implemented in WebKit and there should be accompanying layout tests. This way, once we prove that it is safe for the web, the cost for other WebKit ports to adopt this behavior change will be as close to zero as possible. Alright. I'll submit a revert, mark this WontFix and open a separate bug to fix it properly in webcore.
Created attachment 85833 [details] Revert
Comment on attachment 85833 [details] Revert ok.
Comment on attachment 85833 [details] Revert Clearing flags on attachment: 85833 Committed r81279: <http://trac.webkit.org/changeset/81279>
The commit-queue encountered the following flaky tests while processing attachment 85833 [details]: java/lc3/JavaObject/JavaObjectToByte-004-n.html bug 56500 (author: ap@webkit.org) The commit-queue is continuing to process your patch.