Bug 145908 – [Win] Implement WebViewGroup to support WebView::addxxxToGroup()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Hyungwook Lee]

We need to implement WebViewGroup for supporting WebView::addUserScriptToGroup(), WebView::addUserStyleSheetToGroup() API.

[Hyungwook Lee]

Created attachment 254782 [details]
Patch

[Brent Fulgham]

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.

[Hyungwook Lee]

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.

[Hyungwook Lee]

Created attachment 254854 [details]
Patch

[Hyungwook Lee]

Created attachment 254855 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

Comment on attachment 254855 [details]
Patch

r=me. Thank you! This looks great.

[WebKit Commit Bot]

Comment on attachment 254855 [details]
Patch

Clearing flags on attachment: 254855

Committed r185636: <http://trac.webkit.org/changeset/185636>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 146068

[Hyungwook Lee]

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.

[Hyungwook Lee]

Created attachment 255101 [details]
Patch

[WebKit Commit Bot]

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.

[Brent Fulgham]

(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.

[Hyungwook Lee]

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

[Hyungwook Lee]

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

[Darin Adler]

Mac port uses precompiled headers rather than config.h.

[Hyungwook Lee]

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

[Brent Fulgham]

Comment on attachment 255101 [details]
Patch

Looks good. Let's try it again!

[WebKit Commit Bot]

Comment on attachment 255101 [details]
Patch

Clearing flags on attachment: 255101

Committed r185999: <http://trac.webkit.org/changeset/185999>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

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

[Brent Fulgham]

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

[Hyungwook Lee]

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?

[Brent Fulgham]

(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.

[Hyungwook Lee]

(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;
}