Bug 99101 – [EFL][WK2] Rename EflViewportHandler to PageViewportControllerClientEfl

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Yael]

EflViewportHandler should be renamed to PageViewportControllerClientEfl to be more inline with the Qt port. Subsequent patches should also update its functionality.

[Yael]

Created attachment 169410 [details]
Patch

Rename EFlViewportHandler to PageViewportControllerClientEfl. Also, make it inherit from PageViewportControllerClient. The actual implementation of PageViewportControllerClient for Efl port will come in a separate patch.

[Gyuyoung Kim]

Comment on attachment 169410 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:47
> +EflViewportHandler::~EflViewportHandler()

I don't understand this file. Are you changing "EflViewportHandler.cpp | h" with "PageViewportControllerClientEfl.cpp | h" ? There are two PageViewportControllerClientEfl.cpp files in this patch.

[Yael]

(In reply to comment #2)
> (From update of attachment 169410 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169410&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:47
> > +EflViewportHandler::~EflViewportHandler()
> 
> I don't understand this file. Are you changing "EflViewportHandler.cpp | h" with "PageViewportControllerClientEfl.cpp | h" ? There are two PageViewportControllerClientEfl.cpp files in this patch.

There are 2 files because I did "svn rename".
One is a copy of the original file, and the other are changes I did inside the file, to rename the class and adjust the inheritance from PageViewportControllerClient.

[Gyuyoung Kim]

Comment on attachment 169410 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:119
> +    OwnPtr<PageViewportControllerClientEfl> viewportHandler;

Don't you need to change variable name according to new class name ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:120
> +    OwnPtr<PageViewportController> pageViewportController;

What is purpose of pageViewportController ? It looks this member variable is not used in this patch.

[Yael]

(In reply to comment #4)
Thanks for your review :)
> (From update of attachment 169410 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169410&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:119
> > +    OwnPtr<PageViewportControllerClientEfl> viewportHandler;
> 
> Don't you need to change variable name according to new class name ?
> 
Yes, I'll do that.
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:120
> > +    OwnPtr<PageViewportController> pageViewportController;
> 
> What is purpose of pageViewportController ? It looks this member variable is not used in this patch.
After renaming this class, it now becomes a client of PageViewportController, so I added pageViewportController, since it doesn't make sense to have a client without the class it is client of.
Subsequent patches will make use of pageViewportController. 
If you feel that pageViewportController should be added later, I can remove it from this patch and add it in a later patch.

[Gyuyoung Kim]

(In reply to comment #5)

> > What is purpose of pageViewportController ? It looks this member variable is not used in this patch.
> After renaming this class, it now becomes a client of PageViewportController, so I added pageViewportController, since it doesn't make sense to have a client without the class it is client of.
> Subsequent patches will make use of pageViewportController. 
> If you feel that pageViewportController should be added later, I can remove it from this patch and add it in a later patch.

I prefer to add it when using actually. It would be good if you add it when you prepare new patch.

[Yael]

Created attachment 169533 [details]
Patch

Address comments #4 and #6.

[Gyuyoung Kim]

Comment on attachment 169533 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 169533 [details]
Patch

Rejecting attachment 169533 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
D at 100.
4 out of 4 hunks FAILED -- saving rejects to file Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp.rej
patching file Source/WebKit2/UIProcess/API/efl/EflViewportHandler.h
rm 'Source/WebKit2/UIProcess/API/efl/EflViewportHandler.h'
patching file Source/WebKit2/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Gyuyoung K..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14424900

[Yael]

Created attachment 169603 [details]
Patch for landing.

Rebased the patch.

[Yael]

Created attachment 169606 [details]
Patch for landing.

Another rebase.

[WebKit Review Bot]

Comment on attachment 169606 [details]
Patch for landing.

Rejecting attachment 169606 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/JavaScriptCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14460464

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 169606 [details]
Patch for landing.

Clearing flags on attachment: 169606

Committed r131896: <http://trac.webkit.org/changeset/131896>

[Raphael Kubo da Costa (:rakuco)]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

Comment on attachment 169606 [details]
Patch for landing.

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

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:40
> +EflViewportHandler::EflViewportHandler(Evas_Object* viewWidget)

Shouldn't we rename these as well? :)

> Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.h:27
> +#ifndef EflViewportHandler_h
> +#define EflViewportHandler_h

And these?

[Yael]

(In reply to comment #15)
> (From update of attachment 169606 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169606&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.cpp:40
> > +EflViewportHandler::EflViewportHandler(Evas_Object* viewWidget)
> 
> Shouldn't we rename these as well? :)
> 
> > Source/WebKit2/UIProcess/API/efl/PageViewportControllerClientEfl.h:27
> > +#ifndef EflViewportHandler_h
> > +#define EflViewportHandler_h
> 
> And these?

The patch contains 2 versions of each file. One is an identical copy of the old file, and one is my modifications. You are looking at the copy which is identical to the old file, not at my modifications.
Please have a look at comment #2 and comment #3 .