Bug 55844 – Expose page dismissal event status through the WebKit API for chromium

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit API

[Sreeram Ramachandran]

Expose page dismissal event status through the WebKit API for chromium

[Sreeram Ramachandran]

*** Bug 55845 has been marked as a duplicate of this bug. ***

[Sreeram Ramachandran]

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.

[Sreeram Ramachandran]

Created attachment 84891 [details]
Patch

[WebKit Review Bot]

Attachment 84891 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8102316

[Sreeram Ramachandran]

Created attachment 84892 [details]
Patch

[Dimitri Glazkov (Google)]

This looks ok, but can you help a bit explaining how this is useful?

[Sreeram Ramachandran]

(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.

[Dimitri Glazkov (Google)]

Comment on attachment 84892 [details]
Patch

ok.

[Sreeram Ramachandran]

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?

[WebKit Commit Bot]

Comment on attachment 84892 [details]
Patch

Clearing flags on attachment: 84892

Committed r80436: <http://trac.webkit.org/changeset/80436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

(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?

[Dimitri Glazkov (Google)]

Rolled out in http://trac.webkit.org/changeset/80437 -- broke Chromium compile.

[Sreeram Ramachandran]

Yikes. Yes, I built chromium, and it built fine on my machine. Sorry about the breakage. Thanks for reverting the change.

[Sreeram Ramachandran]

Created attachment 84911 [details]
Second attempt

[Dimitri Glazkov (Google)]

Comment on attachment 84911 [details]
Second attempt

activate the photon torpedoes.

[WebKit Commit Bot]

Comment on attachment 84911 [details]
Second attempt

Clearing flags on attachment: 84911

Committed r80451: <http://trac.webkit.org/changeset/80451>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

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?

[Sreeram Ramachandran]

(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).

[Darin Fisher (:fishd, Google)]

> > 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.

[Sreeram Ramachandran]

(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/?

[Sreeram Ramachandran]

(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)?

[Darin Fisher (:fishd, Google)]

(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.

[Sreeram Ramachandran]

(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.

[Sreeram Ramachandran]

Created attachment 85833 [details]
Revert

[Dimitri Glazkov (Google)]

Comment on attachment 85833 [details]
Revert

ok.

[WebKit Commit Bot]

Comment on attachment 85833 [details]
Revert

Clearing flags on attachment: 85833

Committed r81279: <http://trac.webkit.org/changeset/81279>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 85833 [details]:

java/lc3/JavaObject/JavaObjectToByte-004-n.html bug 56500 (author: [e-mail])
The commit-queue is continuing to process your patch.