For http://crbug.com/9682, we would like to make it possible to show the pending URL in a newly opened tab, as long as the contents of the initial about:blank document have not been modified by another window. In that case, a URL spoof is possible and the pending URL should not be shown. This behavior would be consistent with Opera. However, there's currently no way in the Chromium WebKit API to determine if an about:blank document has been visually modified or not. (Bug 91766 investigated the didFirstVisuallyNonEmptyLayout() callback for this, but it turned out to not be appropriate.) I can listen to didUpdateLayout for potential changes to the render tree, as long as there's a way to check if the WebDocument's render tree is visually empty. I'll start by proposing a WebDocument::isRenderTreeEmpty() method, which simply ensures that the RenderBody node of the render tree is empty. I'm not super familiar with the render tree, so suggestions to improve this are welcome.
Created attachment 155638 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 155638 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155638&action=review > Source/WebKit/chromium/public/WebDocument.h:90 > + WEBKIT_EXPORT bool isRenderTreeEmpty() const; isRenderTreeEmpty -> isVisuallyEmpty ? I'm not sure we expose the concept of a "render tree" in the API.
We'd never expose a function like this to web content because it can be used to detect details of when and how we do layout... I guess that raises the question of whether we should do layout when this function is called...
Created attachment 155648 [details] Patch
(In reply to comment #4) > We'd never expose a function like this to web content because it can be used to detect details of when and how we do layout... I guess that raises the question of whether we should do layout when this function is called... Is there a reason for this to trigger a layout? I was thinking it should just be a way for the embedder to check the current state (e.g., after didUpdateLayout).
Comment on attachment 155638 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155638&action=review >> Source/WebKit/chromium/public/WebDocument.h:90 >> + WEBKIT_EXPORT bool isRenderTreeEmpty() const; > > isRenderTreeEmpty -> isVisuallyEmpty ? I'm not sure we expose the concept of a "render tree" in the API. Fixed.
> Is there a reason for this to trigger a layout? I was thinking it should just be a way for the embedder to check the current state (e.g., after didUpdateLayout). The reason would be to hide our heuristics for triggering layout. As written, we could improve the performance of the engine by delaying layout in some situations and break code that uses this API.
(In reply to comment #8) > > Is there a reason for this to trigger a layout? I was thinking it should just be a way for the embedder to check the current state (e.g., after didUpdateLayout). > > The reason would be to hide our heuristics for triggering layout. As written, we could improve the performance of the engine by delaying layout in some situations and break code that uses this API. I see. I don't suppose there's a more authoritative way to check whether the document is still blank, without requiring a layout? I'm avoiding the DOM tree because it might miss shadow DOM changes. We could try to make it clear that this is only valid since the last layout (hence it's safe to call from didUpdateLayout), but that may not be the best option.
(In reply to comment #9) > (In reply to comment #8) > > > Is there a reason for this to trigger a layout? I was thinking it should just be a way for the embedder to check the current state (e.g., after didUpdateLayout). > > > > The reason would be to hide our heuristics for triggering layout. As written, we could improve the performance of the engine by delaying layout in some situations and break code that uses this API. > > I see. I don't suppose there's a more authoritative way to check whether the document is still blank, without requiring a layout? I'm avoiding the DOM tree because it might miss shadow DOM changes. > > We could try to make it clear that this is only valid since the last layout (hence it's safe to call from didUpdateLayout), but that may not be the best option. To be clear, it sounds like the problem cases are when the render tree has changed but a layout hasn't yet occurred to reflect the change. So, this could mean we say the document is empty if it has recently been cleared but is still showing stale content, or we could say it is non-empty if it's going to be non-empty on the next layout. It does seem like the simplest way to handle this is to trigger a layout if one is needed, so that these cases don't exist. At least in my use case, I'm only calling it from didUpdateLayout, so needsLayout() will always return false. Reasonable?
Created attachment 155830 [details] Patch
(In reply to comment #10) > At least in my use case, I'm only calling it from didUpdateLayout, so needsLayout() will always return false. Reasonable? This is not the case - sometimes immediately after didUpdateLayout() needsLayout() will return true. (This is arguably a bug in rendering, but it's also true).
(In reply to comment #12) > (In reply to comment #10) > > At least in my use case, I'm only calling it from didUpdateLayout, so needsLayout() will always return false. Reasonable? > > This is not the case - sometimes immediately after didUpdateLayout() needsLayout() will return true. (This is arguably a bug in rendering, but it's also true). That's unfortunate. I wouldn't think it's a deal breaker, though, is it? In my use case, we're only calling isVisuallyEmpty() for the initial blank document until we see the first modification. We might trigger an extra layout in there in some corner cases, but it seems uncommon. The other alternative is to try to build something analogous to didFirstVisuallyNonEmptyLayout() that keeps track of state and actually returns true if the blank document has ever been modified. (didFirstVisuallyNonEmptyLayout itself is fired for blank documents that are still blank, so it's not useful for me.)
Comment on attachment 155830 [details] Patch The API LGTM. Ideally someone who knows something about the render tree would take a look at the actual implementation to make sure it's sensible.
I'm not sure I believe in the feature. But the rendering code looks fine. RenderView can't decide it's scrollbars or not w/o the help of FrameView (it's owning widget). So the layout you're forcing here might not be quite right, but it will answer your question.
(In reply to comment #15) > I'm not sure I believe in the feature. But the rendering code looks fine. > > RenderView can't decide it's scrollbars or not w/o the help of FrameView (it's owning widget). So the layout you're forcing here might not be quite right, but it will answer your question. Ah. Would it be better to upload a new patch using FrameView::needsLayout() and FrameView::layout() on the Document's view() instead? (FrameView does not appear to have a layoutIfNeeded.) I'm open to other ways to detect if the initial blank page has been modified, but I'm pretty sure I need that signal to safely fix http://crbug.com/9682 without introducing URL spoof vulnerabilities.
Created attachment 155891 [details] Patch
Comment on attachment 155891 [details] Patch I believe this will do the right thing, yes.
Comment on attachment 155891 [details] Patch Is your plan to poll this value? Is there some callback you plan to use to know when to look at it? Eric mentioned in person that he worries that this sort of polling behavior will be error prone and that we'll have spoofing vulnerabilities.
(In reply to comment #19) > (From update of attachment 155891 [details]) > Is your plan to poll this value? Is there some callback you plan to use to know when to look at it? Eric mentioned in person that he worries that this sort of polling behavior will be error prone and that we'll have spoofing vulnerabilities. I'll be polling it when WebViewClient::didUpdateLayout() is called (only for the initial document before a navigation commits, and only until the first time we see that it has been modified). Is it possible for a visual change to occur without didUpdateLayout() being called?
> Is it possible for a visual change to occur without didUpdateLayout() being called? Dunno. It's unfortunate that didFirstVisuallyNonEmptyLayout doesn't work. It's unclear to me whether we're just building yet-another-buggy-version of that notification.
I've not worked with these callbacks, so I don't know. FrameView::layout() should be the master of all layouts. As things are set up it sounds like you could end up calling yourself recursively, hopefully you're OK with that. :)
(In reply to comment #22) > I've not worked with these callbacks, so I don't know. FrameView::layout() should be the master of all layouts. As things are set up it sounds like you could end up calling yourself recursively, hopefully you're OK with that. :) It's ok as long as it's not an infinite recursion. James, when you said it's possible for needsLayout() to be true in didUpdateLayout(), is it possible for it to stay that way after an additional call to layout()? Then we'd be in trouble.
(In reply to comment #23) > (In reply to comment #22) > > I've not worked with these callbacks, so I don't know. FrameView::layout() should be the master of all layouts. As things are set up it sounds like you could end up calling yourself recursively, hopefully you're OK with that. :) > > It's ok as long as it's not an infinite recursion. James, when you said it's possible for needsLayout() to be true in didUpdateLayout(), is it possible for it to stay that way after an additional call to layout()? Then we'd be in trouble. James and I discussed this a bit and decided this is probably not the right path to pursue. The render tree is only one aspect of how a document may be modified for a spoof, and we need something more comprehensive. For example, JavaScript may try to request a permission that triggers an infobar while the pending URL is showing, or audio could be playing (non-visual). It's not clear to me whether or not there's a narrow interface anywhere to detect such interactions. DOM modifications are a start, but the infobar case is an interesting example. Will require further investigation, I suppose.