Bug 145908 - [Win] Implement WebViewGroup to support WebView::addxxxToGroup()
Summary: [Win] Implement WebViewGroup to support WebView::addxxxToGroup()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hyungwook Lee
URL:
Keywords:
Depends on: 146068
Blocks: 140258
  Show dependency treegraph
 
Reported: 2015-06-12 02:01 PDT by Hyungwook Lee
Modified: 2015-06-26 18:49 PDT (History)
4 users (show)

See Also:


Attachments
Patch (17.92 KB, patch)
2015-06-12 02:37 PDT, Hyungwook Lee
bfulgham: review-
Details | Formatted Diff | Diff
Patch (34.69 KB, patch)
2015-06-13 06:55 PDT, Hyungwook Lee
no flags Details | Formatted Diff | Diff
Patch (37.17 KB, patch)
2015-06-13 07:04 PDT, Hyungwook Lee
no flags Details | Formatted Diff | Diff
Patch (37.11 KB, patch)
2015-06-18 01:41 PDT, Hyungwook Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hyungwook Lee 2015-06-12 02:01:03 PDT
We need to implement WebViewGroup for supporting WebView::addUserScriptToGroup(), WebView::addUserStyleSheetToGroup() API.
Comment 1 Hyungwook Lee 2015-06-12 02:37:24 PDT
Created attachment 254782 [details]
Patch
Comment 2 Brent Fulgham 2015-06-12 09:17:08 PDT
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 3 Hyungwook Lee 2015-06-12 15:49:44 PDT
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.
Comment 4 Hyungwook Lee 2015-06-13 06:55:18 PDT
Created attachment 254854 [details]
Patch
Comment 5 Hyungwook Lee 2015-06-13 07:04:30 PDT
Created attachment 254855 [details]
Patch
Comment 6 WebKit Commit Bot 2015-06-13 07:06:33 PDT
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 7 Brent Fulgham 2015-06-16 10:09:32 PDT
Comment on attachment 254855 [details]
Patch

r=me. Thank you! This looks great.
Comment 8 WebKit Commit Bot 2015-06-16 20:31:33 PDT
Comment on attachment 254855 [details]
Patch

Clearing flags on attachment: 254855

Committed r185636: <http://trac.webkit.org/changeset/185636>
Comment 9 WebKit Commit Bot 2015-06-16 20:31:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Alexey Proskuryakov 2015-06-17 10:09:46 PDT
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
Comment 11 WebKit Commit Bot 2015-06-17 10:11:46 PDT
Re-opened since this is blocked by bug 146068
Comment 12 Hyungwook Lee 2015-06-18 00:53:01 PDT
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.
Comment 13 Hyungwook Lee 2015-06-18 01:41:25 PDT
Created attachment 255101 [details]
Patch
Comment 14 WebKit Commit Bot 2015-06-18 01:43:59 PDT
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.
Comment 15 Brent Fulgham 2015-06-19 10:45:49 PDT
(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.
Comment 16 Hyungwook Lee 2015-06-19 15:38:17 PDT
Win port has no problem but Mac port makes compilation.
In my investigation, WK1 Mac port doesn't uses config.h
Comment 17 Hyungwook Lee 2015-06-19 15:39:52 PDT
Win port has no problem but Mac port makes compilation error. 
In my investigation, WK1 Mac port doesn't uses config.h
Comment 18 Darin Adler 2015-06-19 16:09:50 PDT
Mac port uses precompiled headers rather than config.h.
Comment 19 Hyungwook Lee 2015-06-25 03:39:19 PDT
Hi, Brent Fulgham or Darin Adler
Could you review my latest patch which fixed layout test issue?
Comment 20 Brent Fulgham 2015-06-26 09:38:51 PDT
Comment on attachment 255101 [details]
Patch

Looks good. Let's try it again!
Comment 21 WebKit Commit Bot 2015-06-26 09:46:47 PDT
Comment on attachment 255101 [details]
Patch

Clearing flags on attachment: 255101

Committed r185999: <http://trac.webkit.org/changeset/185999>
Comment 22 WebKit Commit Bot 2015-06-26 09:46:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Brent Fulgham 2015-06-26 10:01:57 PDT
The tests need to be reenabled (see Bug 140258).
Comment 24 Brent Fulgham 2015-06-26 11:04:33 PDT
Looks good! All tests continue to pass without any problems. I'll enable the addxxxToGroup tests soon in Bug 140258.
Comment 25 Hyungwook Lee 2015-06-26 18:23:58 PDT
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?
Comment 26 Brent Fulgham 2015-06-26 18:31:03 PDT
(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.
Comment 27 Hyungwook Lee 2015-06-26 18:49:28 PDT
(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;
}