Bug 63229 – [EFL] Add APIs to get/set view mode

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kangil Han]

To support view-mode property in WebKit EFL port, I will add get/set view mode functions.

[Kangil Han]

Created attachment 98708 [details]
patch

patch included

[Gyuyoung Kim]

Comment on attachment 98708 [details]
patch

you don't add ChangeLog for this patch. Informal review- on my side.

[Gyuyoung Kim]

Comment on attachment 98708 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:4549
> +        return EWK_VIEW_MODE_INVALID;

I'm not sure if this implementation is good.  Did you test "EWK_VIEW_SD_GET_OR_RETURN(o, sd, EWK_VIEW_MODE_INVALID)" ?
If there are no problems on this implementation, I think this is better.

Lucas, can we use this implementation? It looks there is no critical problems.

> Source/WebKit/efl/ewk/ewk_view.cpp:4552
> +        return EWK_VIEW_MODE_INVALID;

ditto.

[Kangil Han]

Created attachment 98710 [details]
Patch

Patch included

[Raphael Kubo da Costa (:rakuco)]

The gtk port just returns windowed mode on error in the getter. Wouldn't it make sense to do the same here?

[Kangil Han]

(In reply to comment #5)
> The gtk port just returns windowed mode on error in the getter. Wouldn't it make sense to do the same here?

I think program should catch exceptional case if there is problem on getting smart or private data in ewk view. Otherwise, it will invoke other issues.

[Gyuyoung Kim]

(In reply to comment #6)
> (In reply to comment #5)
> > The gtk port just returns windowed mode on error in the getter. Wouldn't it make sense to do the same here?
> 
> I think program should catch exceptional case if there is problem on getting smart or private data in ewk view. Otherwise, it will invoke other issues.

I don't know what WINDOWED mode mean well. But, it seems the WINDOWED mode is normal view mode. Thus, if there are any problems in view mode get function, I also think it is better to return WINDOWED mode as default. If the view mode get function returns EWK_VIEW_MODE_INVALID, how to set view mode in application ?

[Kangil Han]

Created attachment 98852 [details]
patch

[Kangil Han]

Agreed. I've attached new patch. Please review this patch.

[Gyuyoung Kim]

Comment on attachment 98852 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:4503
> + * Sets view mode.

Please add more description related to view mode. User can be confused with this comment.

> Source/WebKit/efl/ewk/ewk_view.cpp:4539
> + * Gets view mode.

ditto.

[Gyuyoung Kim]

CC'ing kenneth and antonio.

[Kangil Han]

Created attachment 98854 [details]
patch

[Kangil Han]

I've added detailed description on comments. Please refer to my latest patch.

[Gyuyoung Kim]

Comment on attachment 98854 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:4506
> + * @param o a Evas_Object webview

Add description more detail. Please see comment of other APIs.

For example,  @param o view object to change fixed layout.

> Source/WebKit/efl/ewk/ewk_view.cpp:4545
> + * @param o a Evas_Object webview

ditto.

[Kangil Han]

Created attachment 98858 [details]
patch

I've modified @param description on each function.

[Gyuyoung Kim]

Comment on attachment 98858 [details]
patch

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

> Source/WebKit/efl/ewk/ewk_view.cpp:4511
> + * See http://www.w3.org/TR/view-mode/

Hmm, it seems to me this is unnecessary comment.

> Source/WebKit/efl/ewk/ewk_view.cpp:4549
> + * See http://www.w3.org/TR/view-mode/

ditto.

[Kangil Han]

Created attachment 98871 [details]
patch

Removed URL from comments.

[Gyuyoung Kim]

Comment on attachment 98871 [details]
patch

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

LGTM except for comment I mention.

> Source/WebKit/efl/ewk/ewk_view.cpp:4504
> + * Web application is being shown as a running application on the platform.

I'm not sure if this comment is good.

[Kangil Han]

Mentioned comment was quoted from w3c document without any modification.

[Kenneth Rohde Christiansen]

Comment on attachment 98871 [details]
patch

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

> Source/WebKit/efl/ChangeLog:9
> +        This patch:
> +        - Implement view-mode property in EFL port.

Please describe it in a more natural way: Implement public API for getting/setting the view-mode media feature as specified by ...

> Source/WebKit/efl/ewk/ewk_view.cpp:4568
> +Ewk_View_Mode ewk_view_mode_get(Evas_Object* o)
> +{
> +    EWK_VIEW_SD_GET(o, sd);
> +    if (!sd)
> +        return EWK_VIEW_MODE_WINDOWED;
> +    EWK_VIEW_PRIV_GET(sd, priv);
> +    if (!priv)
> +        return EWK_VIEW_MODE_WINDOWED;
> +
> +    switch (priv->page->viewMode()) {
> +    case WebCore::Page::ViewModeFloating:
> +        return EWK_VIEW_MODE_FLOATING;
> +    case WebCore::Page::ViewModeFullscreen:
> +        return EWK_VIEW_MODE_FULLSCREEN;
> +    case WebCore::Page::ViewModeMaximized:
> +        return EWK_VIEW_MODE_MAXIMIZED;
> +    case WebCore::Page::ViewModeMinimized:
> +        return EWK_VIEW_MODE_MINIMIZED;
> +    default:
> +        return EWK_VIEW_MODE_WINDOWED;
> +    }
> +}

one is obvious default here. Why not do

Ewk_View_Mode mode = EWK_VIEW_MODE_WINDOWED;

then change all returns to "return mode" and make the switch modify mode?

[Kangil Han]

Created attachment 98892 [details]
patch

I've modified change log and source code as guided.

[Kenneth Rohde Christiansen]

Comment on attachment 98892 [details]
patch

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

Almost there.

> Source/WebKit/efl/ChangeLog:8
> +        This patch:

This line is useless

> Source/WebKit/efl/ChangeLog:9
> +        - Implement public API for getting/setting the view-mode media feature as specified in W3C.

No need for the -

> Source/WebKit/efl/ewk/ewk_view.cpp:4504
> + * Web application is being shown as a running application on the platform.

I don't think you need the "on the platform" part. It just confuses me.

[Kangil Han]

Created attachment 99022 [details]
patch

I've modified change log and source code comment as guided.

[Raphael Kubo da Costa (:rakuco)]

LGTM.

[Kenneth Rohde Christiansen]

Comment on attachment 99022 [details]
patch

ok.

[WebKit Review Bot]

Comment on attachment 99022 [details]
patch

Rejecting attachment 99022 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
er', u'Kenneth Rohde Christia..." exit_code: 1

Parsed 3 diffs from patch file(s).
patching file Source/WebKit/efl/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/efl/ewk/ewk_view.cpp
Hunk #1 FAILED at 4498.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/efl/ewk/ewk_view.cpp.rej
patching file Source/WebKit/efl/ewk/ewk_view.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christia..." exit_code: 1

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

[Gyuyoung Kim]

There was conflict while patch is landed. It looks this patch needs to be made based on latest webkit trunk again.

Kangil,

Could you make this patch again based on latest WebKit trunk ? When you make ChangeLog, please add "Reviewed by "Kenneth Rohde Christiansen" to reviewer field instead of "Reviewed by NOBODY (OOPS!)." Because, Kenneth already approved your patch officially. Then, please request only cq again.

[Kangil Han]

Created attachment 99206 [details]
patch

I've updated patch based on latest source code.

[WebKit Review Bot]

Comment on attachment 99206 [details]
patch

Clearing flags on attachment: 99206

Committed r90080: <http://trac.webkit.org/changeset/90080>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.