Bug 84766

Summary: REGRESSION (r109022) Safari not placing service data on pasteboard
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: HTML EditingAssignee: Enrica Casucci <enrica>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, jeffm, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: OS X 10.7   
Attachments:
Description Flags
Patch ap: review+, ap: commit-queue-

Enrica Casucci
Reported 2012-04-24 14:08:17 PDT
This is a regression introduced during one of the changes to move access to the pasteboard out of the WebProcess. The support for OS X Services requires that all the write operation to the pasteboard are made synchronously. <rdar://problem/11085756>
Attachments
Patch (15.80 KB, patch)
2012-04-24 14:19 PDT, Enrica Casucci
ap: review+
ap: commit-queue-
Enrica Casucci
Comment 1 2012-04-24 14:19:07 PDT
WebKit Review Bot
Comment 2 2012-04-24 14:23:57 PDT
Attachment 138642 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit2/WebProcess/WebPage/WebPage.h:414: The parameter name "handle" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Enrica Casucci
Comment 3 2012-04-24 14:31:30 PDT
(In reply to comment #2) > Attachment 138642 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 > Source/WebKit2/WebProcess/WebPage/WebPage.h:414: The parameter name "handle" adds no information, so it should be removed. [readability/parameter_name] [5] > Total errors found: 1 in 11 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Already fixed.
Jeff Miller
Comment 4 2012-04-24 15:17:19 PDT
Comment on attachment 138642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138642&action=review > Source/WebCore/ChangeLog:8 > + the pasteboard occurr synchronously. This behavior was changed with r109022. Type - occurr > Source/WebKit2/ChangeLog:8 > + the pasteboard occurr synchronously. This behavior was changed with r109022. Ditto.
Jeff Miller
Comment 5 2012-04-24 15:17:47 PDT
(In reply to comment #4) > (From update of attachment 138642 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138642&action=review > > > Source/WebCore/ChangeLog:8 > > + the pasteboard occurr synchronously. This behavior was changed with r109022. > > Type - occurr I meant typo, not type. :) > > > Source/WebKit2/ChangeLog:8 > > + the pasteboard occurr synchronously. This behavior was changed with r109022. > > Ditto.
Alexey Proskuryakov
Comment 6 2012-04-24 15:25:24 PDT
Comment on attachment 138642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138642&action=review > Source/WebCore/editing/mac/EditorMac.mm:33 > #import "DocumentFragment.h" > +#import "DOMRangeInternal.h" This is out of order, style bit should have complained. > Source/WebCore/editing/mac/EditorMac.mm:50 > #import "htmlediting.h" > +#import "WebNSAttributedStringExtras.h" This too - upper case comes first in ASCII order. > Source/WebCore/editing/mac/EditorMac.mm:318 > +PassRefPtr<SharedBuffer> Editor::getBufferSelectionForPasteboard(const String& pasteboardType) Did this code come from WebKit1? Can it be deleted there now? This most definitely doesn't look like something we want to maintain in two places forever. > Source/WebKit2/UIProcess/WebPageProxy.h:473 > + PassRefPtr<WebCore::SharedBuffer> getBufferSelectionForPasteboard(const String& pasteboardType); I'm not sure if I can parse "buffer selection". Maybe "PasteboardDataFromSelection"? Or "SerializedAttributedStringFromSelection"? This applies to the whole cascade of functions, of course, not just this proxy wrapper. > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:-263 > - if (!isValid()) > - return false; Why is this not necessary any more? > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:278 > + return SharedBuffer::create(static_cast<unsigned char *>(sharedMemoryBuffer->data()), size); Style nit: star goes with the type.
Enrica Casucci
Comment 7 2012-04-24 15:42:29 PDT
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 138642 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=138642&action=review > > > > > Source/WebCore/ChangeLog:8 > > > + the pasteboard occurr synchronously. This behavior was changed with r109022. > > > > Type - occurr > > I meant typo, not type. :) > > > > > > Source/WebKit2/ChangeLog:8 > > > + the pasteboard occurr synchronously. This behavior was changed with r109022. > > > > Ditto. I'll fix it. Thanks! :-)
Enrica Casucci
Comment 8 2012-04-24 15:45:45 PDT
(In reply to comment #6) > (From update of attachment 138642 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138642&action=review > > > Source/WebCore/editing/mac/EditorMac.mm:33 > > #import "DocumentFragment.h" > > +#import "DOMRangeInternal.h" > > This is out of order, style bit should have complained. > > > Source/WebCore/editing/mac/EditorMac.mm:50 > > #import "htmlediting.h" > > +#import "WebNSAttributedStringExtras.h" > > This too - upper case comes first in ASCII order. Will do. > > > Source/WebCore/editing/mac/EditorMac.mm:318 > > +PassRefPtr<SharedBuffer> Editor::getBufferSelectionForPasteboard(const String& pasteboardType) > > Did this code come from WebKit1? Can it be deleted there now? > > This most definitely doesn't look like something we want to maintain in two places forever. > No, this code comes from PasteboardMac. I'll refactor it, to avoid duplicates. > > Source/WebKit2/UIProcess/WebPageProxy.h:473 > > + PassRefPtr<WebCore::SharedBuffer> getBufferSelectionForPasteboard(const String& pasteboardType); > > I'm not sure if I can parse "buffer selection". Maybe "PasteboardDataFromSelection"? Or "SerializedAttributedStringFromSelection"? I think I'll go for dataFromSelection and stringFromSelection. > > This applies to the whole cascade of functions, of course, not just this proxy wrapper. > > > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:-263 > > - if (!isValid()) > > - return false; > Removed by accident. Thanks for noticing. > Why is this not necessary any more? > > > Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:278 > > + return SharedBuffer::create(static_cast<unsigned char *>(sharedMemoryBuffer->data()), size); > > Style nit: star goes with the type. Sure.
Alexey Proskuryakov
Comment 9 2012-04-24 15:57:20 PDT
> I think I'll go for dataFromSelection and stringFromSelection. I'm still not sure if that would be clear enough later - when there is a SharedData buffer, it would be helpful to have names explain format for the buffer. > Removed by accident. Thanks for noticing. Please add it to the new function as well.
Enrica Casucci
Comment 10 2012-04-24 17:25:23 PDT
Note You need to log in before you can comment on or make changes to this bug.