Bug 42317 - Pasteboard doesn't work in WebKit2
Summary: Pasteboard doesn't work in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 42780 (view as bug list)
Depends on: 45494
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-14 20:39 PDT by Maciej Stachowiak
Modified: 2010-09-24 18:37 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.86 KB, patch)
2010-09-15 17:37 PDT, Enrica Casucci
sam: review+
Details | Formatted Diff | Diff
Patch2 (12.57 KB, patch)
2010-09-16 20:23 PDT, Enrica Casucci
darin: review-
Details | Formatted Diff | Diff
Patch3 (28.96 KB, patch)
2010-09-17 17:09 PDT, Enrica Casucci
sam: review+
Details | Formatted Diff | Diff
Patch4 (15.90 KB, patch)
2010-09-23 16:52 PDT, Enrica Casucci
darin: review-
Details | Formatted Diff | Diff
Patch5 (17.24 KB, patch)
2010-09-24 16:29 PDT, Enrica Casucci
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2010-07-14 20:39:13 PDT
This makes at least the the following tests fail:

editing/pasteboard/copy-resolves-urls.html
Comment 1 Maciej Stachowiak 2010-07-14 20:39:33 PDT
<rdar://problem/7660537>
Comment 2 Sam Weinig 2010-07-21 13:13:07 PDT
*** Bug 42780 has been marked as a duplicate of this bug. ***
Comment 3 Enrica Casucci 2010-09-15 17:37:14 PDT
Created attachment 67746 [details]
Patch

This is not the full implementation, but it is a step forward.
The bug should not be resolved when this patch is reviewed and landed.
Comment 4 Enrica Casucci 2010-09-16 09:29:52 PDT
Committed revision 67631
Comment 6 Enrica Casucci 2010-09-16 20:23:32 PDT
Created attachment 67878 [details]
Patch2

Added RTF format support.
Comment 7 Darin Adler 2010-09-17 10:21:07 PDT
Comment on attachment 67878 [details]
Patch2

> +        * loader/appcache/ApplicationCacheStorage.cpp:
> +        (WebCore::ApplicationCacheStorage::setCacheDirectory): Remove assert
> +        on the cachedirectory name, now that we can load a WebView also in the
> +        Web process for WebKit2.

I don’t understand the connection. Could you explain further?
Comment 8 Darin Adler 2010-09-17 10:33:56 PDT
Comment on attachment 67878 [details]
Patch2

View in context: https://bugs.webkit.org/attachment.cgi?id=67878&action=prettypatch

I think the load of the other WebKit framework in WebKit2 should be dynamic. Under many circumstances we can use WebKit2 without ever needing to run this code. The code should only load the WebKit framework if it’s actually run.

Loading the WebKit framework for converting RTF seems OK. Loading it just to convert the URL seems like a bad idea, although perhaps OK for now.

> WebKit2/WebProcess/WebPage/WebPage.h:53
> +#ifdef __OBJC__
> +@class NSAttributedString;
> +@class NSString;
> +@class NSURL;
> +#else

Is this part needed? I know the !OBJC part is needed, but I thought that when compiling Obj-C we would always have the data types from Foundation included as part of the precompiled header.

> WebKit2/WebProcess/WebPage/WebPage.h:158
> +#if PLATFORM(MAC)
> +    NSString* userVisibleString(NSURL*);
> +    WebCore::DocumentFragment* documentFragmentFromAttributedString(NSAttributedString*, Vector<WebCore::ArchiveResource*>&);
> +#endif

Do we really need to use the Cocoa types here? That makes this code Mac-specific when it could otherwise possibly be shared with other platforms like Windows when they use CF. What about the CF types, CFStringRef, CFURLRef, and CFAttributedStringRef? At least the first two are “toll-free bridged”.

The new code is inconsistent about the position of the "*" characters. Our strange rule is that Obj-C types always have a space before the *. I would like to change that rule in the future, but I would like you to be consistent within the patch at least, if possible.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:291
> +static NSArray* excludedElementsForAttributedStringConversion()

A better style for this is to make a separate create function for the one time code, and then write:

    static NSArray *elements = createExcludedElementsArray();
    return elements;

It’s cleaner without the "== nil" and if statement and such.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:305
> +    static NSArray *elements = nil;
> +    if (elements == nil) {
> +        elements = [[NSArray alloc] initWithObjects:
> +                    // Omit style since we want style to be inline so the fragment can be easily inserted.
> +                    @"style",
> +                    // Omit xml so the result is not XHTML.
> +                    @"xml", 
> +                    // Omit tags that will get stripped when converted to a fragment anyway.
> +                    @"doctype", @"html", @"head", @"body",
> +                    // Omit deprecated tags.
> +                    @"applet", @"basefont", @"center", @"dir", @"font", @"isindex", @"menu", @"s", @"strike", @"u",
> +                    // Omit object so no file attachments are part of the fragment.
> +                    @"object", nil];

I understand that you were copying existing code, but typically we avoid code lined up with the previous lines like this, because it’s not friendly when renaming among other reasons.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:313
> +    WebView* view = [[WebView alloc] initWithFrame:NSMakeRect(0, 0, 0, 0)];

Calling the init method will call initWithFrame, so there’s no need to call initWithFrame:. Also, you can use NSEmptyRect in cases like this if you do need this rectangle.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:317
> +    NSDictionary *dictionary = [[NSDictionary alloc] initWithObjectsAndKeys:
> +                                excludedElementsForAttributedStringConversion(), NSExcludedElementsDocumentAttribute,
> +                                nil, @"WebResourceHandler", nil];

Again, I prefer to avoid lining up like this.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:319
> +    NSArray* s;

I much prefer words for local variable names over letters. I would name this “subresources”.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:324
> +    NSEnumerator *e = [s objectEnumerator];

And this, “enumerator”.

Since this code doesn’t need to run on systems older than Snow Leopard, you can use Objective-C 2.0 style enumeration which is a lot cleaner. I don’t know the syntax myself, but I know it’s the preferred style.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:327
> +        RefPtr<ArchiveResource>  ar = [r _coreResource];

There’s an extra space here. I don’t think you need to put this into a local variable. It would read better if you didn’t.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:328
> +        resources.append(ar.get());

Why is resources an array of raw pointers? What guarantees the ArchiveResource objects will stay alive? I think it probably needs to be a vector of RefPtr, unless I’m missing something.

> WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:338
> +NSString* WebPage::userVisibleString(NSURL* URL)
> +{
> +    return [URL _web_userVisibleString];
> +}

I don’t understand why we are indirecting here through a WebPage function. Why can’t WebEditorClient just call this function directly?

In fact, in both cases I am a little puzzled. It seems that putting this code into WebPage just makes WebPage more of a god class. Perhaps there’s a way we can avoid this. These functions don’t seem to rely on being members of WebPage, so they could be static members if they are going to be members, but I am not sure they get any benefit from being class members.
Comment 9 Enrica Casucci 2010-09-17 14:27:22 PDT
(In reply to comment #8)
> I think the load of the other WebKit framework in WebKit2 should be dynamic. Under many circumstances we can use WebKit2 without ever needing to run this code. The code should only load the WebKit framework if it’s actually run.

We changed that now. The next patch will not do that.
 
> Loading the WebKit framework for converting RTF seems OK. Loading it just to convert the URL seems like a bad idea, although perhaps OK for now.
> 
> > WebKit2/WebProcess/WebPage/WebPage.h:53
> > +#ifdef __OBJC__
> > +@class NSAttributedString;
> > +@class NSString;
> > +@class NSURL;
> > +#else
> 
> Is this part needed? I know the !OBJC part is needed, but I thought that when compiling Obj-C we would always have the data types from Foundation included as part of the precompiled header.

You're right, I just followed another example, but I tried what you're saying and it's not necessary when __OBJC__.
 
> > WebKit2/WebProcess/WebPage/WebPage.h:158
> > +#if PLATFORM(MAC)
> > +    NSString* userVisibleString(NSURL*);
> > +    WebCore::DocumentFragment* documentFragmentFromAttributedString(NSAttributedString*, Vector<WebCore::ArchiveResource*>&);
> > +#endif
> 
> Do we really need to use the Cocoa types here? That makes this code Mac-specific when it could otherwise possibly be shared with other platforms like Windows when they use CF. What about the CF types, CFStringRef, CFURLRef, and CFAttributedStringRef? At least the first two are “toll-free bridged”.
> 
I didn't know this was possible.

> The new code is inconsistent about the position of the "*" characters. Our strange rule is that Obj-C types always have a space before the *. I would like to change that rule in the future, but I would like you to be consistent within the patch at least, if possible.
>

I'm not sure I understand what you want me to do here. No space before * for C++ and space before * for obj-c?
 
> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:291
> > +static NSArray* excludedElementsForAttributedStringConversion()
> 
> A better style for this is to make a separate create function for the one time code, and then write:
> 
>     static NSArray *elements = createExcludedElementsArray();
>     return elements;
> 
> It’s cleaner without the "== nil" and if statement and such.
> 
> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:305
> > +    static NSArray *elements = nil;
> > +    if (elements == nil) {
> > +        elements = [[NSArray alloc] initWithObjects:
> > +                    // Omit style since we want style to be inline so the fragment can be easily inserted.
> > +                    @"style",
> > +                    // Omit xml so the result is not XHTML.
> > +                    @"xml", 
> > +                    // Omit tags that will get stripped when converted to a fragment anyway.
> > +                    @"doctype", @"html", @"head", @"body",
> > +                    // Omit deprecated tags.
> > +                    @"applet", @"basefont", @"center", @"dir", @"font", @"isindex", @"menu", @"s", @"strike", @"u",
> > +                    // Omit object so no file attachments are part of the fragment.
> > +                    @"object", nil];
> 
> I understand that you were copying existing code, but typically we avoid code lined up with the previous lines like this, because it’s not friendly when renaming among other reasons.
> 

As you said, I copied the code and I assumed that, since it was in, it was ok. What kind of alignment should I use here?

> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:313
> > +    WebView* view = [[WebView alloc] initWithFrame:NSMakeRect(0, 0, 0, 0)];
> 
> Calling the init method will call initWithFrame, so there’s no need to call initWithFrame:. Also, you can use NSEmptyRect in cases like this if you do need this rectangle.
> 

Not applicable anymore, since there is no WebView.

> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:319
> > +    NSArray* s;
> 
> I much prefer words for local variable names over letters. I would name this “subresources”.
>

Ok.
 
> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:324
> > +    NSEnumerator *e = [s objectEnumerator];
> 
> And this, “enumerator”.
> 
> Since this code doesn’t need to run on systems older than Snow Leopard, you can use Objective-C 2.0 style enumeration which is a lot cleaner. I don’t know the syntax myself, but I know it’s the preferred style.
>

I'll look it up.
 
> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:327
> > +        RefPtr<ArchiveResource>  ar = [r _coreResource];
> 
> There’s an extra space here. I don’t think you need to put this into a local variable. It would read better if you didn’t.
>

ok.
 
> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:328
> > +        resources.append(ar.get());
> 
> Why is resources an array of raw pointers? What guarantees the ArchiveResource objects will stay alive? I think it probably needs to be a vector of RefPtr, unless I’m missing something.
> 

You're probably right. It got lost in the WebKit to WebCore conversion. I'll check the WebKit version I checked in at the beginning of the week to verify if the same problem is there.

> > WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:338
> > +NSString* WebPage::userVisibleString(NSURL* URL)
> > +{
> > +    return [URL _web_userVisibleString];
> > +}
> 
> I don’t understand why we are indirecting here through a WebPage function. Why can’t WebEditorClient just call this function directly?

I don't know how to call obj-c from a cpp file.

> 
> In fact, in both cases I am a little puzzled. It seems that putting this code into WebPage just makes WebPage more of a god class. Perhaps there’s a way we can avoid this. These functions don’t seem to rely on being members of WebPage, so they could be static members if they are going to be members, but I am not sure they get any benefit from being class members.

Same reason as above.
Is it possible to do so?
Comment 10 Enrica Casucci 2010-09-17 17:09:28 PDT
Created attachment 67975 [details]
Patch3

No more use of WebView nor linking of WebKit.
I believe I've addressed all the comments.
Comment 11 Sam Weinig 2010-09-18 16:58:49 PDT
Comment on attachment 67975 [details]
Patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=67975&action=prettypatch

> WebCore/loader/EmptyClients.h:438
> +    virtual DocumentFragment* documentFragmentFromAttributedString(NSAttributedString*, Vector< RefPtr<ArchiveResource> >&) { return 0; };

No space before the RefPtr is needed.

> WebCore/page/EditorClient.h:156
> +    virtual DocumentFragment* documentFragmentFromAttributedString(NSAttributedString*, Vector< RefPtr<ArchiveResource> >&) = 0;

Here too.

> WebCore/platform/mac/PasteboardMac.mm:410
> +    Vector< RefPtr<ArchiveResource> > resources;

And here.

> WebKit2/WebProcess/WebCoreSupport/mac/WebEditorClientMac.mm:84
> +DocumentFragment* WebEditorClient::documentFragmentFromAttributedString(NSAttributedString *string, Vector< RefPtr<ArchiveResource> >& resources)
> +{

And here.

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:362
> +DocumentFragment* WebEditorClient::documentFragmentFromAttributedString(NSAttributedString *string, Vector< RefPtr<ArchiveResource> >& resources)
>  {

And here,


Otherwise, r=me.
Comment 12 Enrica Casucci 2010-09-20 11:14:13 PDT
Committed patch3. http://trac.webkit.org/changeset/67864
Comment 13 Enrica Casucci 2010-09-23 16:52:01 PDT
Created attachment 68622 [details]
Patch4

Last piece.
Comment 14 Darin Adler 2010-09-23 17:23:50 PDT
Comment on attachment 68622 [details]
Patch4

View in context: https://bugs.webkit.org/attachment.cgi?id=68622&action=review

Looks like a good general approach, but there are some things that are not quite right.

> WebKit2/UIProcess/PageClient.h:54
> +    virtual void enableMenuItem(const String& commandName, bool isEnabled) = 0;

While this terminology is used in Windows APIs -- a function named "enable" that sets whether an item is enabled or not -- I have always found it awkward. Cocoa APIs call this "setEnabled", which would lead to the name setMenuItemEnabled. But also, is this really specific to menu items? Maybe this should be named setEditCommandEnabled.

> WebKit2/UIProcess/WebPageProxy.h:135
> +    void editingCommand(const String& commandName);

Since this function performs and operation, its name should be a verb phrase, not a noun phrase. It could be executeEditingCommand, or you could find some other verb. But just "editing command" seems wrong.

> WebKit2/UIProcess/API/mac/WKView.mm:157
> +typedef HashMap<SEL, String> SelectorNameMap;

Does this type of HashMap work correctly? For selectors, we can use pointer equality rather than string equality. I think you may need to specify PtrHash explicitly to get the right behavior. I’m not sure exactly what type SEL is.

> WebKit2/UIProcess/API/mac/WKView.mm:161
> +static const SelectorNameMap* createSelectorExceptionMap()

It might be nicer to use PassOwnPtr here and call leak at the call site. Or use "leak" in the name of the function.

> WebKit2/UIProcess/API/mac/WKView.mm:197
> +#define WEBCORE_COMMAND(command, name) - (void)command:(id)sender { _data->_page->editingCommand(commandNameForSelector(_cmd)); }

What is the name argument for? It’s not used!

> WebKit2/UIProcess/API/mac/WKView.mm:213
> +    if (_data->_menuMap.isEmpty()) {

If the menu map is non-empty there’s no guarantee that the item is another menu item from the same menu. This code assumes that validateUserInterfaceItem: is called only for menu items within one menu at a time, but that’s wrong since it’s called for things that are not menu items and can be called for more than one menu at a time.

> WebKit2/UIProcess/API/mac/WKView.mm:216
> +        _data->_currentMenu = [(NSMenuItem *)item menu];

The item is not guaranteed to be a menu item. It could be a toolbar button, for example. So this cast is not correct. You could get method-not-found.

We can’t just keep a pointer to an NSMenu *. It might be deallocated and we could use a freed pointer later! If we want to keep a pointer to it we have to retain it.

> WebKit2/WebProcess/WebPage/WebPage.cpp:218
> +    if (m_page->focusController()->focusedOrMainFrame())
> +        m_page->focusController()->focusedOrMainFrame()->editor()->command(commandName).execute(argument);

Normally in WebKit code we do an early exit:

    Frame* frame = m_page->focusController()->focusedOrMainFrame();
    if (!frame)
        return;

> WebKit2/WebProcess/WebPage/WebPage.cpp:224
> +    if (frame) {

Same thing here. Early exist is the normal pattern.

> WebKit2/WebProcess/WebPage/WebPage.cpp:227
> +        if (command.isSupported())
> +            return command.isEnabled();

This can just say:

    return command.isSupported() && command.isEnabled();

But do we have to check both? Isn’t isEnabled guaranteed to return false if isSupported is false?
Comment 15 Early Warning System Bot 2010-09-23 17:27:38 PDT
Attachment 68622 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4079093
Comment 16 Sam Weinig 2010-09-23 17:43:33 PDT
> 
> > WebKit2/UIProcess/API/mac/WKView.mm:157
> > +typedef HashMap<SEL, String> SelectorNameMap;
> 
> Does this type of HashMap work correctly? For selectors, we can use pointer equality rather than string equality. I think you may need to specify PtrHash explicitly to get the right behavior. I’m not sure exactly what type SEL is.

SEL is defined as typedef struct objc_selector 	*SEL; so this should work fine.
Comment 17 Enrica Casucci 2010-09-23 18:00:15 PDT
(In reply to comment #14)
> While this terminology is used in Windows APIs -- a function named "enable" that sets whether an item is enabled or not -- I have always found it awkward. Cocoa APIs call this "setEnabled", which would lead to the name setMenuItemEnabled. But also, is this really specific to menu items? Maybe this should be named setEditCommandEnabled.

I will change it.
> 
> > WebKit2/UIProcess/WebPageProxy.h:135
> > +    void editingCommand(const String& commandName);
> 
> Since this function performs and operation, its name should be a verb phrase, not a noun phrase. It could be executeEditingCommand, or you could find some other verb. But just "editing command" seems wrong.
> 

I agree, I will change it.
> > WebKit2/UIProcess/API/mac/WKView.mm:157
> > +typedef HashMap<SEL, String> SelectorNameMap;
> 
> Does this type of HashMap work correctly? For selectors, we can use pointer equality rather than string equality. I think you may need to specify PtrHash explicitly to get the right behavior. I’m not sure exactly what type SEL is.
>

I've copied this code from WebKit. I assumed this was correct and I've looked at the definition of SEL.
I believe this is correct, also per Sam's comment.
 
> > WebKit2/UIProcess/API/mac/WKView.mm:161
> > +static const SelectorNameMap* createSelectorExceptionMap()
> 
> It might be nicer to use PassOwnPtr here and call leak at the call site. Or use "leak" in the name of the function.

Again, this is existing WebKit code, that I always assume is correct.

> > WebKit2/UIProcess/API/mac/WKView.mm:197
> > +#define WEBCORE_COMMAND(command, name) - (void)command:(id)sender { _data->_page->editingCommand(commandNameForSelector(_cmd)); }
> 

> What is the name argument for? It’s not used!
Sorry, leftover of a previous version of my implementation. Will remove it.

> 
> > WebKit2/UIProcess/API/mac/WKView.mm:213
> > +    if (_data->_menuMap.isEmpty()) {
> 
> If the menu map is non-empty there’s no guarantee that the item is another menu item from the same menu. This code assumes that validateUserInterfaceItem: is called only for menu items within one menu at a time, but that’s wrong since it’s called for things that are not menu items and can be called for more than one menu at a time.
>
You're right. I'll fix it.
 
> > WebKit2/UIProcess/API/mac/WKView.mm:216
> > +        _data->_currentMenu = [(NSMenuItem *)item menu];
> 
> The item is not guaranteed to be a menu item. It could be a toolbar button, for example. So this cast is not correct. You could get method-not-found.

I didn't know. I will change this per our conversation.

> We can’t just keep a pointer to an NSMenu *. It might be deallocated and we could use a freed pointer later! If we want to keep a pointer to it we have to retain it.
>
Same as above.
 
> > WebKit2/WebProcess/WebPage/WebPage.cpp:218
> > +    if (m_page->focusController()->focusedOrMainFrame())
> > +        m_page->focusController()->focusedOrMainFrame()->editor()->command(commandName).execute(argument);
> 
> Normally in WebKit code we do an early exit:
> 
>     Frame* frame = m_page->focusController()->focusedOrMainFrame();
>     if (!frame)
>         return;
> 
> > WebKit2/WebProcess/WebPage/WebPage.cpp:224
> > +    if (frame) {
> 
> Same thing here. Early exist is the normal pattern.
>

I can change this.
 
> > WebKit2/WebProcess/WebPage/WebPage.cpp:227
> > +        if (command.isSupported())
> > +            return command.isEnabled();
> 
> This can just say:
> 
>     return command.isSupported() && command.isEnabled();
> 
> But do we have to check both? Isn’t isEnabled guaranteed to return false if isSupported is false?

I followed what WebKit does. I will double-check if it is really needed.
Thank you for all the comments.
Comment 18 Enrica Casucci 2010-09-24 16:29:08 PDT
Created attachment 68788 [details]
Patch5

This patch addresses all the comments.
Comment 19 Early Warning System Bot 2010-09-24 17:05:45 PDT
Attachment 68788 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4021153
Comment 20 Darin Adler 2010-09-24 17:25:38 PDT
Comment on attachment 68788 [details]
Patch5

View in context: https://bugs.webkit.org/attachment.cgi?id=68788&action=review

This patch seems to break the Qt WebKit2. If you resolve that seems OK to land.

> WebKit2/UIProcess/API/mac/WKView.mm:72
> +        MenuItemInfo(): _isEnabled(false), _state(0) {};
> +        MenuItemInfo(bool isEnabled, int state): _isEnabled(isEnabled), _state(state) {};

Need a space before the ":". No semicolon needed after the "{}". We normally put a space in the "{ }".

You call this EditCommandState in the PageClient API. Why not call it that here too?

No need for underscores in this class’s members. We normally don’t use them for struct-style classes like this one. We do use them for Objective-C classes.

I suggest struct instead of class for something where everything is public.

Not sure it’s good to nest a C++ class inside an Objective-C class definition. I suggest putting it outside WKViewData.

> WebKit2/UIProcess/API/mac/WKView.mm:207
> +// FIXME: we should add here all the commands as we implement them.

Grammar mistake. Should be "We should add all the commands here as we implement them."

> WebKit2/UIProcess/API/mac/WKView.mm:227
> +        return NO;      // FIXME: we need to be able to handle other user interface elements.

We capitalize comments, using sentence style. We normally put comments only one space past the code, not 6 spaces.

> WebKit2/UIProcess/API/mac/WKView.mm:496
> +    _data->_needsDataForValidation = false;

I think the “needs data for validation” mechanism is a bit roundabout. We could try replacing it with a boolean that means “I am inside the update call” instead to see if that results in easier to read code.
Comment 21 Enrica Casucci 2010-09-24 18:37:32 PDT
Committed revision 68318.