Bug 78282 - Refactor Mac platform implementation of the Pasteboard class to use Platform Strategies
Summary: Refactor Mac platform implementation of the Pasteboard class to use Platform ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Enrica Casucci
URL:
Keywords:
Depends on: 78410
Blocks: 77259
  Show dependency treegraph
 
Reported: 2012-02-09 14:19 PST by Enrica Casucci
Modified: 2012-02-13 14:45 PST (History)
4 users (show)

See Also:


Attachments
patch (70.95 KB, patch)
2012-02-09 15:43 PST, Enrica Casucci
andersca: review+
Details | Formatted Diff | Diff
Patch to verify the Qt build fix (76.82 KB, patch)
2012-02-13 13:39 PST, Enrica Casucci
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrica Casucci 2012-02-09 14:19:45 PST
This bug track another step in the direction of removing access to NSPasteboard from the WebProcess.
The Mac implementation of the Pasteboard class needs to be redesigned to use a platform strategy that can provide different implementations for WebKit and WebKit2.
Comment 1 Enrica Casucci 2012-02-09 15:43:06 PST
Created attachment 126392 [details]
patch
Comment 2 Csaba Osztrogonác 2012-02-11 00:48:02 PST
I rolled it out, because it broke the Qt build - http://trac.webkit.org/changeset/107475

../../../Source/WebKit/qt/WebCoreSupport/PlatformStrategiesQt.cpp: In static member function 'static void PlatformStrategiesQt::initialize()':
../../../Source/WebKit/qt/WebCoreSupport/PlatformStrategiesQt.cpp:51: error: cannot allocate an object of abstract type 'PlatformStrategiesQt'
../../../Source/WebKit/qt/WebCoreSupport/PlatformStrategiesQt.h:41: note:   because the following virtual functions are pure within 'PlatformStrategiesQt':
../../../Source/WebCore/platform/PlatformStrategies.h:95: note: 	virtual WebCore::PasteboardStrategy* WebCore::PlatformStrategies::createPasteboardStrategy()

Next time please use EWS bots or watch the bots after landing to ensure that your patch builds on all platform.

http://www.webkit.org/coding/contributing.html
"Your change must at least compile on all platforms."

Qt build was broken during ~70 revision because of this patch. :( It means that the test coverage was 0 in this time. And unfortunately somebody broke all JSC tests on Qt-debug during build breakage. It won't be a good game to find it manually ...
Comment 3 Enrica Casucci 2012-02-11 06:49:42 PST
(In reply to comment #2)
> I rolled it out, because it broke the Qt build - http://trac.webkit.org/changeset/107475
> 
> ../../../Source/WebKit/qt/WebCoreSupport/PlatformStrategiesQt.cpp: In static member function 'static void PlatformStrategiesQt::initialize()':
> ../../../Source/WebKit/qt/WebCoreSupport/PlatformStrategiesQt.cpp:51: error: cannot allocate an object of abstract type 'PlatformStrategiesQt'
> ../../../Source/WebKit/qt/WebCoreSupport/PlatformStrategiesQt.h:41: note:   because the following virtual functions are pure within 'PlatformStrategiesQt':
> ../../../Source/WebCore/platform/PlatformStrategies.h:95: note:     virtual WebCore::PasteboardStrategy* WebCore::PlatformStrategies::createPasteboardStrategy()
> 
> Next time please use EWS bots or watch the bots after landing to ensure that your patch builds on all platform.
> 
> http://www.webkit.org/coding/contributing.html
> "Your change must at least compile on all platforms."
> 
> Qt build was broken during ~70 revision because of this patch. :( It means that the test coverage was 0 in this time. And unfortunately somebody broke all JSC tests on Qt-debug during build breakage. It won't be a good game to find it manually ...

My apologies, I did not notice that the Qt port was using platform strategies. I will resubmit my patch with the build fix for Qt.
Comment 4 Enrica Casucci 2012-02-13 13:39:24 PST
Created attachment 126819 [details]
Patch to verify the Qt build fix
Comment 5 WebKit Review Bot 2012-02-13 13:41:09 PST
Attachment 126819 [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/WebCore/platform/PlatformPasteboard.h:53:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/PasteboardStrategy.h:50:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.h:67:  The parameter name "buffer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/mac/WebCoreSupport/WebPlatformStrategies.h:69:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/WebProcess/WebCoreSupport/WebPlatformStrategies.h:75:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/Pasteboard.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Enrica Casucci 2012-02-13 14:45:51 PST
http://trac.webkit.org/changeset/107606