EflViewportHandler should be renamed to PageViewportControllerClientEfl to be more inline with the Qt port. Subsequent patches should also update its functionality.
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.
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.
(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.
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.
(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.
(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.
Created attachment 169533 [details] Patch Address comments #4 and #6.
Comment on attachment 169533 [details] Patch LGTM.
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
Created attachment 169603 [details] Patch for landing. Rebased the patch.
Created attachment 169606 [details] Patch for landing. Another rebase.
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
Comment on attachment 169606 [details] Patch for landing. Clearing flags on attachment: 169606 Committed r131896: <http://trac.webkit.org/changeset/131896>
All reviewed patches have been landed. Closing bug.
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?
(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 .