To support view-mode property in WebKit EFL port, I will add get/set view mode functions.
Created attachment 98708 [details] patch patch included
Comment on attachment 98708 [details] patch you don't add ChangeLog for this patch. Informal review- on my side.
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.
Created attachment 98710 [details] Patch Patch included
The gtk port just returns windowed mode on error in the getter. Wouldn't it make sense to do the same here?
(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.
(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 ?
Created attachment 98852 [details] patch
Agreed. I've attached new patch. Please review this patch.
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.
CC'ing kenneth and antonio.
Created attachment 98854 [details] patch
I've added detailed description on comments. Please refer to my latest patch.
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.
Created attachment 98858 [details] patch I've modified @param description on each function.
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.
Created attachment 98871 [details] patch Removed URL from comments.
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.
Mentioned comment was quoted from w3c document without any modification.
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?
Created attachment 98892 [details] patch I've modified change log and source code as guided.
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.
Created attachment 99022 [details] patch I've modified change log and source code comment as guided.
LGTM.
Comment on attachment 99022 [details] patch ok.
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
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.
Created attachment 99206 [details] patch I've updated patch based on latest source code.
Comment on attachment 99206 [details] patch Clearing flags on attachment: 99206 Committed r90080: <http://trac.webkit.org/changeset/90080>
All reviewed patches have been landed. Closing bug.