Bug 63229

Summary: [EFL] Add APIs to get/set view mode
Product: WebKit Reporter: Kangil Han <kangil.han>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
gyuyoung.kim: review-
Patch
none
patch
none
patch
none
patch
none
patch
none
patch
none
patch
kenneth: review+, webkit.review.bot: commit-queue-
patch none

Description Kangil Han 2011-06-22 22:47:48 PDT
To support view-mode property in WebKit EFL port, I will add get/set view mode functions.
Comment 1 Kangil Han 2011-06-27 05:38:57 PDT
Created attachment 98708 [details]
patch

patch included
Comment 2 Gyuyoung Kim 2011-06-27 05:42:41 PDT
Comment on attachment 98708 [details]
patch

you don't add ChangeLog for this patch. Informal review- on my side.
Comment 3 Gyuyoung Kim 2011-06-27 05:57:20 PDT
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.
Comment 4 Kangil Han 2011-06-27 06:01:17 PDT
Created attachment 98710 [details]
Patch

Patch included
Comment 5 Raphael Kubo da Costa (:rakuco) 2011-06-27 06:42:23 PDT
The gtk port just returns windowed mode on error in the getter. Wouldn't it make sense to do the same here?
Comment 6 Kangil Han 2011-06-27 19:54:56 PDT
(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.
Comment 7 Gyuyoung Kim 2011-06-27 20:07:19 PDT
(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 ?
Comment 8 Kangil Han 2011-06-27 21:15:11 PDT
Created attachment 98852 [details]
patch
Comment 9 Kangil Han 2011-06-27 21:17:25 PDT
Agreed. I've attached new patch. Please review this patch.
Comment 10 Gyuyoung Kim 2011-06-27 21:21:47 PDT
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.
Comment 11 Gyuyoung Kim 2011-06-27 21:23:09 PDT
CC'ing kenneth and antonio.
Comment 12 Kangil Han 2011-06-27 21:35:38 PDT
Created attachment 98854 [details]
patch
Comment 13 Kangil Han 2011-06-27 21:37:28 PDT
I've added detailed description on comments. Please refer to my latest patch.
Comment 14 Gyuyoung Kim 2011-06-27 21:53:41 PDT
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.
Comment 15 Kangil Han 2011-06-27 22:01:30 PDT
Created attachment 98858 [details]
patch

I've modified @param description on each function.
Comment 16 Gyuyoung Kim 2011-06-27 23:47:07 PDT
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.
Comment 17 Kangil Han 2011-06-28 00:12:30 PDT
Created attachment 98871 [details]
patch

Removed URL from comments.
Comment 18 Gyuyoung Kim 2011-06-28 00:23:58 PDT
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.
Comment 19 Kangil Han 2011-06-28 00:30:19 PDT
Mentioned comment was quoted from w3c document without any modification.
Comment 20 Kenneth Rohde Christiansen 2011-06-28 01:23:47 PDT
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?
Comment 21 Kangil Han 2011-06-28 03:13:37 PDT
Created attachment 98892 [details]
patch

I've modified change log and source code as guided.
Comment 22 Kenneth Rohde Christiansen 2011-06-28 03:55:19 PDT
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.
Comment 23 Kangil Han 2011-06-28 18:31:59 PDT
Created attachment 99022 [details]
patch

I've modified change log and source code comment as guided.
Comment 24 Raphael Kubo da Costa (:rakuco) 2011-06-29 06:06:58 PDT
LGTM.
Comment 25 Kenneth Rohde Christiansen 2011-06-29 06:41:24 PDT
Comment on attachment 99022 [details]
patch

ok.
Comment 26 WebKit Review Bot 2011-06-29 06:43:17 PDT
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
Comment 27 Gyuyoung Kim 2011-06-29 18:21:05 PDT
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.
Comment 28 Kangil Han 2011-06-29 18:49:17 PDT
Created attachment 99206 [details]
patch

I've updated patch based on latest source code.
Comment 29 WebKit Review Bot 2011-06-29 19:06:51 PDT
Comment on attachment 99206 [details]
patch

Clearing flags on attachment: 99206

Committed r90080: <http://trac.webkit.org/changeset/90080>
Comment 30 WebKit Review Bot 2011-06-29 19:06:58 PDT
All reviewed patches have been landed.  Closing bug.