Bug 89837 - [WK2][Qt][EFL] Extract common code from LayerTreeHostQt
Summary: [WK2][Qt][EFL] Extract common code from LayerTreeHostQt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 89840
  Show dependency treegraph
 
Reported: 2012-06-24 17:07 PDT by YoungTaeck Song
Modified: 2012-06-25 20:41 PDT (History)
12 users (show)

See Also:


Attachments
patch (60.55 KB, patch)
2012-06-24 17:11 PDT, YoungTaeck Song
no flags Details | Formatted Diff | Diff
patch (60.59 KB, patch)
2012-06-24 20:39 PDT, YoungTaeck Song
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
patch (173.85 KB, patch)
2012-06-25 04:38 PDT, YoungTaeck Song
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
patch (173.84 KB, patch)
2012-06-25 04:49 PDT, YoungTaeck Song
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
patch (175.97 KB, patch)
2012-06-25 05:34 PDT, YoungTaeck Song
no flags Details | Formatted Diff | Diff
Patch (179.23 KB, patch)
2012-06-25 18:28 PDT, YoungTaeck Song
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description YoungTaeck Song 2012-06-24 17:07:30 PDT
Extract common code from LayerTreeHostQt to be used by both Qt and Efl.
Because Extracted codes use WebGraphicsLayer, these are named 'LayerTreeHostWeb'.
Comment 1 YoungTaeck Song 2012-06-24 17:11:06 PDT
Created attachment 149209 [details]
patch
Comment 2 YoungTaeck Song 2012-06-24 20:39:21 PDT
Created attachment 149231 [details]
patch

rebase
Comment 3 Noam Rosenthal 2012-06-24 21:23:31 PDT
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.
Comment 4 YoungTaeck Song 2012-06-25 04:38:44 PDT
Created attachment 149275 [details]
patch
Comment 5 WebKit Review Bot 2012-06-25 04:41:51 PDT
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.
Comment 6 Gyuyoung Kim 2012-06-25 04:42:05 PDT
Comment on attachment 149275 [details]
patch

Attachment 149275 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13097077
Comment 7 YoungTaeck Song 2012-06-25 04:49:06 PDT
Created attachment 149276 [details]
patch
Comment 8 Alexis Menard (darktears) 2012-06-25 04:52:44 PDT
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.
Comment 9 Gyuyoung Kim 2012-06-25 04:55:24 PDT
Comment on attachment 149276 [details]
patch

Attachment 149276 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13088165
Comment 10 YoungTaeck Song 2012-06-25 05:34:17 PDT
Created attachment 149277 [details]
patch

fix build error of EFL
Comment 11 Gustavo Noronha (kov) 2012-06-25 05:43:29 PDT
Comment on attachment 149277 [details]
patch

Attachment 149277 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13090134
Comment 12 Alexis Menard (darktears) 2012-06-25 05:55:00 PDT
(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 :).
Comment 13 YoungTaeck Song 2012-06-25 06:03:12 PDT
(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.
Comment 14 Noam Rosenthal 2012-06-25 06:24:41 PDT
Comment on attachment 149277 [details]
patch

Looks great! Though the GTK build files need to be updated.
Comment 15 Noam Rosenthal 2012-06-25 13:07:45 PDT
Comment on attachment 149277 [details]
patch

Need to fix build issues.
Comment 16 YoungTaeck Song 2012-06-25 16:43:11 PDT
(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.
Comment 17 YoungTaeck Song 2012-06-25 18:28:15 PDT
Created attachment 149417 [details]
Patch
Comment 18 Noam Rosenthal 2012-06-25 18:46:30 PDT
Comment on attachment 149417 [details]
Patch

LGTM!
Comment 19 WebKit Review Bot 2012-06-25 20:41:45 PDT
Comment on attachment 149417 [details]
Patch

Clearing flags on attachment: 149417

Committed r121221: <http://trac.webkit.org/changeset/121221>
Comment 20 WebKit Review Bot 2012-06-25 20:41:51 PDT
All reviewed patches have been landed.  Closing bug.