[webkit-reviews] review denied: [Bug 69919] [EFL] Remove ewk_frame APIs related to backward | forward. : [Attachment 110681] Patch

[bugzilla-daemon at webkit.org]

Raphael Kubo da Costa <kubo at profusion.mobi> has denied Gyuyoung Kim
<gyuyoung.kim at samsung.com>'s request for review:
Bug 69919: [EFL] Remove ewk_frame APIs related to backward | forward.
https://bugs.webkit.org/show_bug.cgi?id=69919

Attachment 110681: Patch
https://bugs.webkit.org/attachment.cgi?id=110681&action=review

------- Additional Comments from Raphael Kubo da Costa <kubo at profusion.mobi>
View in context: https://bugs.webkit.org/attachment.cgi?id=110681&action=review


Since we're changing the API here, I have a few suggestions:

 * "backwards" can stay as "back", even the WebCore method uses "back".
 * ewk_view_{backward,forward} are missing a verb to indicate which action they
perform. I suggest using "ewk_view_navigate_{back,forward}".
 * The boolean functions are also lacking a verb. I suggest either
"ewk_view_navigate_backward_possible" or "ewk_view_can_navigate_back".
 * "go" can be a nice, shorter substitute for the verb "navigate" itself.

> Source/WebKit/efl/ewk/ewk_view.cpp:1494
> +    WebCore::Frame* focusedFrame =
priv->page->focusController()->focusedOrMainFrame();
> +    WebCore::Page* page = focusedFrame->page();

Why not use sd->priv->page?

> Source/WebKit/efl/ewk/ewk_view.cpp:1516
> +    WebCore::Frame* focusedFrame =
priv->page->focusController()->focusedOrMainFrame();
> +    WebCore::Page* page = focusedFrame->page();

Ditto.