Expose mutation observers to chromium
Created attachment 180003 [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 180003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180003&action=review > Source/WebKit/chromium/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). You need to do this.
Created attachment 180009 [details] update change log with more detailed description
Created attachment 180010 [details] update change log with more detailed description
(In reply to comment #3) > (From update of attachment 180003 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180003&action=review > > > Source/WebKit/chromium/ChangeLog:8 > > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > > You need to do this. Sorry - first Webkit CL; still figuring out the workflow.
Comment on attachment 180010 [details] update change log with more detailed description Attachment 180010 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15413230
Comment on attachment 180010 [details] update change log with more detailed description Attachment 180010 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15404352
Created attachment 180057 [details] update change log with more detailed description
ok, should be ready for review now
ping?
Comment on attachment 180057 [details] update change log with more detailed description View in context: https://bugs.webkit.org/attachment.cgi?id=180057&action=review > Source/WebKit/chromium/ChangeLog:6 > + Create WebMutation{whatever} wrappers for existing WebKit classes, exposing functionality needed by chromium to detect DOM changes for the new autofill flow I think the main question with this patch is whether this is a good idea. Can you explain how you plan to use these APIs in the autofill flow? Will you need to create mutation observers for all (or a large fraction) of web pages? My understanding is that the presence of mutation observers slows down DOM operations. > Source/WebKit/chromium/public/WebMutationObserver.h:37 > +#include "platform/WebCommon.h" > +#include "platform/WebPrivatePtr.h" > +#include "platform/WebVector.h" We've changed the style for these sorts of includes.
Comment on attachment 180057 [details] update change log with more detailed description View in context: https://bugs.webkit.org/attachment.cgi?id=180057&action=review >> Source/WebKit/chromium/ChangeLog:6 >> + Create WebMutation{whatever} wrappers for existing WebKit classes, exposing functionality needed by chromium to detect DOM changes for the new autofill flow > > I think the main question with this patch is whether this is a good idea. Can you explain how you plan to use these APIs in the autofill flow? Will you need to create mutation observers for all (or a large fraction) of web pages? My understanding is that the presence of mutation observers slows down DOM operations. We'll need to create mutation observers for a whitelisted set of web pages belonging to multipage autofill flows (for a related bug on the chromium side, maybe check https://codereview.chromium.org/11539003/). Basically we need to be able to support autofill behavior on pages with ajax-y stuff, which means knowing when dom changes happen so we can query the autofill server again. Initially maybe around 20 sites, though potentially more going forward. You are correct that this could have some performance impact on dom operations, but the improvements to autofill should speed up the user experience overall. >> Source/WebKit/chromium/public/WebMutationObserver.h:37 >> +#include "platform/WebVector.h" > > We've changed the style for these sorts of includes. May I ask what to?
> >> Source/WebKit/chromium/public/WebMutationObserver.h:37 > >> +#include "platform/WebVector.h" > > > > We've changed the style for these sorts of includes. > > May I ask what to? Actually it looks like that patch was rolled out.
Comment on attachment 180057 [details] update change log with more detailed description > We'll need to create mutation observers for a whitelisted set of web pages belonging to multipage autofill flows (for a related bug on the chromium side, maybe check https://codereview.chromium.org/11539003/). Basically we need to be able to support autofill behavior on pages with ajax-y stuff, which means knowing when dom changes happen so we can query the autofill server again. Initially maybe around 20 sites, though potentially more going forward. You are correct that this could have some performance impact on dom operations, but the improvements to autofill should speed up the user experience overall. This approach sounds problematic. What happens when we expand beyond this whitelisted set of sites? Perhaps WebKit should provide more detailed callbacks to notify you of when things of interest happen in a web page. MutationObserver sounds like too general a mechanism for your needs. I wonder if we should think more carefully about how to divide the implementation of this feature between WebKit and Chromium.
(In reply to comment #15) > (From update of attachment 180057 [details]) > > We'll need to create mutation observers for a whitelisted set of web pages belonging to multipage autofill flows (for a related bug on the chromium side, maybe check https://codereview.chromium.org/11539003/). Basically we need to be able to support autofill behavior on pages with ajax-y stuff, which means knowing when dom changes happen so we can query the autofill server again. Initially maybe around 20 sites, though potentially more going forward. You are correct that this could have some performance impact on dom operations, but the improvements to autofill should speed up the user experience overall. > > This approach sounds problematic. What happens when we expand beyond this whitelisted set of sites? > > Perhaps WebKit should provide more detailed callbacks to notify you of when things of interest happen in a web page. MutationObserver sounds like too general a mechanism for your needs. > > I wonder if we should think more carefully about how to divide the implementation of this feature between WebKit and Chromium. There's always going to be a whitelist - it'll just get bigger with time. And actually, we should be able to further restrict the mutation observers to pages where we're actually expecting to need to handle dom changes - we should have that information. esprehn suggested using MutationObserver, though he wasn't completely familiar with our use case. If we were to try a different approach, I honestly wouldn't where to start - apart from this particular autofill feature, I don't do any work for Chromium or WebKit.
I feel like I lack context. In general, we try to avoid having different behavior on different web sites. Perhaps a bug thread isn't the most productive way to sort out this issue? Do you have a design doc I can read to better understand your use case?
As an example, doesn't this make Node larger by a pointer? Both EventTarget and ScriptWrappable now contribute a vtable parser.
Comment on attachment 180057 [details] update change log with more detailed description View in context: https://bugs.webkit.org/attachment.cgi?id=180057&action=review > Source/WebKit/chromium/src/WebMutationObserver.cpp:67 > + v8Options->Set(v8::String::New("characterDataOldValue"), v8::Boolean::New(true)); This is way more aggressive than necessary. You want to only observe the things you really need, not everything. This will cause more memory usage and perf impact than most use cases of WebMutationObserver actually need.
Comment on attachment 180057 [details] update change log with more detailed description View in context: https://bugs.webkit.org/attachment.cgi?id=180057&action=review > Source/WebKit/chromium/src/WebMutationObserver.cpp:59 > + v8::Persistent<v8::Context> context = v8::Context::New(); You shouldn't need to call v8::Context::New. This isn't the right approach.
I spoke with Elliott a bit and I wonder if you might be better off with a more narrowly scoped API. Given that your interest is in input fields associated with form elements, you might want to look at the code in WebCore for what happens when an input field becomes associated with a form. There are a number of callbacks that get triggered (e.g., for use by autofill and password managers). I suspect we want something that reuses those code paths rather than rebuilding them in Chromium.
So search as I might, it really isn't clear to me that any code paths currently exists that would provide the information we need. Also, there are some other old bugs on the chromium side which could probably be handled without much fuss given access to mutation observers - namely, making page translations and 'find' highlighting work on ajax-y pages.
IMHO, we should expose a more targeted API for your use case. Exposing mutation observers to Chromium creates too much of an incentive for developers to implement poorly factored designs that re-implement web platform functionality in the embedder. I have a hard time believing that we cannot create a more narrowly scoped API for your use case. If we were to implement this feature internally in WebCore, we certainly wouldn't use mutation observers to react to changes in the page. Many features in WebCore already need to react to changes in the page, and we implement them more specialized mechanisms that don't impose performance penalties.
How would you imagine such an API looking? The specific things we'd need to be able to detect for autofill are new forms, new fields on an existing form, reordering of existing fields, and changes to the name of an existing field. In practice, I would imagine (don't have data at hand) we'd mainly have to worry about the first two, but some sites do some weird stuff, and changes to the name attribute could definitely come up. My worry is that I'd basically have to re-implement MutationObservers - perhaps a lighter weight version, but still touching a fair amount of code in WebCore. One of my goals was to avoid having to make changes outside of WebKit/chromium, since this is for a chrome-only feature, but maybe that's not feasible? Out of curiosity, do you know why DOMMutationEvent was exposed to chromium? I know that's no longer going to be supported at some point, but you can do basically the same things you could with MutationObservers, but with even worse performance.
> One of my goals was to avoid having to make changes outside of WebKit/chromium, ^^^ This isn't a good goal because it leads to poorly factored designs. > Out of curiosity, do you know why DOMMutationEvent was exposed to chromium? We should remove it. It's not useful to Chromium and only serves to tempt people into using it. > I know that's no longer going to be supported at some point, but you can do basically the same things you could with MutationObservers, but with even worse performance. Correct. No one in Chromium should use DOMMutationEvent.
(In reply to comment #24) > How would you imagine such an API looking? I think you want to hook a combination of FormAssociatedElement::resetFormOwner and FormAssociatedElement::didChangeForm. With some clean up we can probably make it so you only need to hook one of them. Then I'd expose a new emedder hook in Editor like didAssociateInput.
(In reply to comment #26) > (In reply to comment #24) > > How would you imagine such an API looking? > > I think you want to hook a combination of FormAssociatedElement::resetFormOwner and FormAssociatedElement::didChangeForm. With some clean up we can probably make it so you only need to hook one of them. > > Then I'd expose a new emedder hook in Editor like didAssociateInput. That's not going to be sufficient. That can only inform us when a field is added to a form already existing on the document. If a new form is created, some fields added, and then placed inside a div. We need to know when the div is added to the document, and this won't give us that. Hence my earlier assertion that we potentially need to be informed of any node that gets added to the dom.
(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #24) > > > How would you imagine such an API looking? > > > > I think you want to hook a combination of FormAssociatedElement::resetFormOwner and FormAssociatedElement::didChangeForm. With some clean up we can probably make it so you only need to hook one of them. > > > > Then I'd expose a new emedder hook in Editor like didAssociateInput. > > That's not going to be sufficient. That can only inform us when a field is added to a form already existing on the document. If a new form is created, some fields added, and then placed inside a div. We need to know when the div is added to the document, and this won't give us that. Hence my earlier assertion that we potentially need to be informed of any node that gets added to the dom. You can use FormAssociatedElement::insertedInto or HTMLFormElement::insertedInto to catch those cases. insertionPoint->inDocument() is true then. Seems reasonable to have a formAdded notification as well. These things may not make sense in EditorClient (though for now it seems okay).
abandoning change