WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(19.02 KB, patch)
2012-10-18 19:13 PDT
,
Yael
gyuyoung.kim
: review+
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Patch for landing.
(18.99 KB, patch)
2012-10-19 05:34 PDT
,
Yael
no flags
Details
Formatted Diff
Diff
Patch for landing.
(27.10 KB, patch)
2012-10-19 06:07 PDT
,
Yael
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug