RESOLVED FIXED 99101
[EFL][WK2] Rename EflViewportHandler to PageViewportControllerClientEfl
https://bugs.webkit.org/show_bug.cgi?id=99101
Summary [EFL][WK2] Rename EflViewportHandler to PageViewportControllerClientEfl
Yael
Reported 2012-10-11 14:48:39 PDT
EflViewportHandler should be renamed to PageViewportControllerClientEfl to be more inline with the Qt port. Subsequent patches should also update its functionality.
Attachments
Patch (27.94 KB, patch)
2012-10-18 07:46 PDT, Yael
no flags
Patch (19.02 KB, patch)
2012-10-18 19:13 PDT, Yael
gyuyoung.kim: review+
webkit.review.bot: commit-queue-
Patch for landing. (18.99 KB, patch)
2012-10-19 05:34 PDT, Yael
no flags
Patch for landing. (27.10 KB, patch)
2012-10-19 06:07 PDT, Yael
no flags
Yael
Comment 1 2012-10-18 07:46:21 PDT
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 2 2012-10-18 17:53:11 PDT
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
Comment 3 2012-10-18 17:58:47 PDT
(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 4 2012-10-18 18:34:17 PDT
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
Comment 5 2012-10-18 18:40:37 PDT
(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
Comment 6 2012-10-18 18:44:45 PDT
(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
Comment 7 2012-10-18 19:13:28 PDT
Created attachment 169533 [details] Patch Address comments #4 and #6.
Gyuyoung Kim
Comment 8 2012-10-18 19:15:02 PDT
Comment on attachment 169533 [details] Patch LGTM.
WebKit Review Bot
Comment 9 2012-10-19 02:38:01 PDT
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
Comment 10 2012-10-19 05:34:32 PDT
Created attachment 169603 [details] Patch for landing. Rebased the patch.
Yael
Comment 11 2012-10-19 06:07:22 PDT
Created attachment 169606 [details] Patch for landing. Another rebase.
WebKit Review Bot
Comment 12 2012-10-19 06:48:04 PDT
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 13 2012-10-19 06:58:09 PDT
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)
Comment 14 2012-10-19 06:58:18 PDT
All reviewed patches have been landed. Closing bug.
Kenneth Rohde Christiansen
Comment 15 2012-10-21 03:14:07 PDT
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
Comment 16 2012-10-21 05:06:10 PDT
(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 .
Note You need to log in before you can comment on or make changes to this bug.