Bug 145908

Created attachment 254782 [details] Patch

Comment on attachment 254782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254782&action=review This looks great! But I do not like duplicating the WebViewGroup code. Please move it to a common location (as I outline in the review comments). Otherwise, this looks great! r- simply because I don't want to duplicate that WebViewGroup implementation. Otherwise, this is perfect. > Source/WebKit/win/WebView.cpp:3703 > HRESULT STDMETHODCALLTYPE WebView::setGroupName( FYI, I've been trying to remove these STDMETHODCALLTYPE declarations in the implementation files. They don't serve any purpose (we only need them in the header). > Source/WebKit/win/WebCoreSupport/WebViewGroup.cpp:25 > + */ It's silly to duplicate this file -- we're just going to end up forgetting to update one of the copies of this content. Please do the following: 1. Create a new folder "Source/WebKit/WebCoreSupport" 2. Move the original file ("svn mv" so we don't lose history) "svn mv Source/WebKit/mac/WebCoreSupport/WebViewGroup.mm Source/WebKit/WebCoreSupport/WebViewGroup.cpp" 3. Ditto for the header. 4. Update the WebKit.xcodeproj/projects.pbxproj to point to the new location.

Comment on attachment 254782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254782&action=review >> Source/WebKit/win/WebCoreSupport/WebViewGroup.cpp:25 >> + */ > > It's silly to duplicate this file -- we're just going to end up forgetting to update one of the copies of this content. Please do the following: > > 1. Create a new folder "Source/WebKit/WebCoreSupport" > 2. Move the original file ("svn mv" so we don't lose history) "svn mv Source/WebKit/mac/WebCoreSupport/WebViewGroup.mm Source/WebKit/WebCoreSupport/WebViewGroup.cpp" > 3. Ditto for the header. > 4. Update the WebKit.xcodeproj/projects.pbxproj to point to the new location. I got your point. I will update it as your guide.

Created attachment 254854 [details] Patch

Created attachment 254855 [details] Patch

Attachment 254855 [details] did not pass style-queue: ERROR: Source/WebKit/WebCoreSupport/WebViewGroup.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 254855 [details] Patch r=me. Thank you! This looks great.

Comment on attachment 254855 [details] Patch Clearing flags on attachment: 254855 Committed r185636: <http://trac.webkit.org/changeset/185636>

All reviewed patches have been landed. Closing bug.

This has caused many crashes on Windows, rolling out. https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r185650%20(52456)/results.html

Re-opened since this is blocked by bug 146068

Sorry to break layout test :(. There are some cases that doesn't call WebView::initWithFrame() in Test Runner. ex) TestRunner::addUserStyleSheet() Current patch doesn't consider missing calling WebView::initWithFrame(). I will fix this issue soon.

Created attachment 255101 [details] Patch

Attachment 255101 [details] did not pass style-queue: ERROR: Source/WebKit/WebCoreSupport/WebViewGroup.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #14) > Attachment 255101 [details] did not pass style-queue: > > > ERROR: Source/WebKit/WebCoreSupport/WebViewGroup.cpp:26: Found header this > file implements before WebCore config.h. Should be: config.h, primary > header, blank line, and then alphabetically sorted. [build/include_order] > [4] > Total errors found: 1 in 13 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. This patch builds properly. I'm running tests locally to confirm it doesn't break anything.

Win port has no problem but Mac port makes compilation. In my investigation, WK1 Mac port doesn't uses config.h

Win port has no problem but Mac port makes compilation error. In my investigation, WK1 Mac port doesn't uses config.h

Mac port uses precompiled headers rather than config.h.

Hi, Brent Fulgham or Darin Adler Could you review my latest patch which fixed layout test issue?

Comment on attachment 255101 [details] Patch Looks good. Let's try it again!

Comment on attachment 255101 [details] Patch Clearing flags on attachment: 255101 Committed r185999: <http://trac.webkit.org/changeset/185999>

All reviewed patches have been landed. Closing bug.

The tests need to be reenabled (see Bug 140258).

Looks good! All tests continue to pass without any problems. I'll enable the addxxxToGroup tests soon in Bug 140258.

I'm not yet implement addUserScriptToGroup' (and related) API in WebView. I'm implementing those API now. Can I enable the addxxxToGroup tests after implement those API?

(In reply to comment #25) > I'm not yet implement addUserScriptToGroup' (and related) API in WebView. > I'm implementing those API now. > > Can I enable the addxxxToGroup tests after implement those API? Yes. We should start enabling tests as they start working. Did this patch allow us to begin running any tests? Maybe our tests were only about user scripts and so we cannot yet update the TestExpectations.

(In reply to comment #26) > (In reply to comment #25) > > I'm not yet implement addUserScriptToGroup' (and related) API in WebView. > > I'm implementing those API now. > > > > Can I enable the addxxxToGroup tests after implement those API? > > Yes. We should start enabling tests as they start working. Did this patch > allow us to begin running any tests? Maybe our tests were only about user > scripts and so we cannot yet update the TestExpectations. Yes, sure, you can start it. I'm implement not yet implemented API now such as following. HRESULT WebView::addUserScriptToGroup(BSTR groupName, IWebScriptWorld* iWorld, BSTR source, BSTR url, unsigned whitelistCount, BSTR* whitelist, unsigned blacklistCount, BSTR* blacklist, WebUserScriptInjectionTime injectionTime) { return E_NOTIMPL; }