Bug 89837 – [WK2][Qt][EFL] Extract common code from LayerTreeHostQt

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[YoungTaeck Song]

Extract common code from LayerTreeHostQt to be used by both Qt and Efl.
Because Extracted codes use WebGraphicsLayer, these are named 'LayerTreeHostWeb'.

[YoungTaeck Song]

Created attachment 149209 [details]
patch

[YoungTaeck Song]

Created attachment 149231 [details]
patch

rebase

[Noam Rosenthal]

Comment on attachment 149231 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149231&action=review

LayerTreeHostWeb doesn't fit with the rest of the names around.
How about we create a LayerTreeCoordinator directory under WebProcess/WebPage, and also move WebGraphicsLayer under that directory? We should rename LayerTreeHostProxy to LayerTreeCoordinatorProxy while we're at it.

> Source/WebKit2/WebProcess/WebPage/web/qt/LayerTreeHostQt.h:38
> +class LayerTreeHostQt : public LayerTreeHostWeb {
> +public:
> +    static PassRefPtr<LayerTreeHostQt> create(WebPage*);
> +    virtual ~LayerTreeHostQt();
> +
> +    virtual int64_t directlyCompositedImageKey(WebCore::Image*);
> +
> +private:
> +    explicit LayerTreeHostQt(WebPage*);
> +};

Virtualizing for this seems like an overkill... I'd rather have non-virtual functions, and implement directlyCompositedImageKey inside a platform-specific #ifdef.

[YoungTaeck Song]

Created attachment 149275 [details]
patch

[WebKit Review Bot]

Attachment 149275 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/WebGraphicsLayer.cpp:459:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/WebGraphicsLayer.h:78:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 149275 [details]
patch

Attachment 149275 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13097077

[YoungTaeck Song]

Created attachment 149276 [details]
patch

[Alexis Menard (darktears)]

Comment on attachment 149276 [details]
patch

To make this refactor more simple, I think you should do the renames of the classes first, then the move and then extract the code. It will be less horrible to review and easier to rollout if needed as it will be simple patches.

[Gyuyoung Kim]

Comment on attachment 149276 [details]
patch

Attachment 149276 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13088165

[YoungTaeck Song]

Created attachment 149277 [details]
patch

fix build error of EFL

[Gustavo Noronha (kov)]

Comment on attachment 149277 [details]
patch

Attachment 149277 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13090134

[Alexis Menard (darktears)]

(In reply to comment #11)
> (From update of attachment 149277 [details])
> Attachment 149277 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/13090134

And you would not fight that much with EWS :).

[YoungTaeck Song]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 149277 [details] [details])
> > Attachment 149277 [details] [details] did not pass gtk-ews (gtk):
> > Output: http://queues.webkit.org/results/13090134
> 
> And you would not fight that much with EWS :).

I'm very sorry. I thought My patch would not affect to a different port.

[Noam Rosenthal]

Comment on attachment 149277 [details]
patch

Looks great! Though the GTK build files need to be updated.

[Noam Rosenthal]

Comment on attachment 149277 [details]
patch

Need to fix build issues.

[YoungTaeck Song]

(In reply to comment #15)
> (From update of attachment 149277 [details])
> Need to fix build issues.

I'm sorry too late.
That's why I'm first time at building GTK.
I'll reupload ASAP.

[YoungTaeck Song]

Created attachment 149417 [details]
Patch

[Noam Rosenthal]

Comment on attachment 149417 [details]
Patch

LGTM!

[WebKit Review Bot]

Comment on attachment 149417 [details]
Patch

Clearing flags on attachment: 149417

Committed r121221: <http://trac.webkit.org/changeset/121221>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.