Bug 106633 - [EFL][WK2] Add APIs to set/get view source mode
Summary: [EFL][WK2] Add APIs to set/get view source mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jinwoo Song
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-10 23:26 PST by Jinwoo Song
Modified: 2013-01-21 18:04 PST (History)
8 users (show)

See Also:


Attachments
Patch (10.20 KB, patch)
2013-01-10 23:36 PST, Jinwoo Song
no flags Details | Formatted Diff | Diff
Patch (10.78 KB, patch)
2013-01-15 01:49 PST, Jinwoo Song
no flags Details | Formatted Diff | Diff
Patch (11.17 KB, patch)
2013-01-15 02:31 PST, Jinwoo Song
no flags Details | Formatted Diff | Diff
Patch (11.28 KB, patch)
2013-01-16 00:23 PST, Jinwoo Song
no flags Details | Formatted Diff | Diff
Patch (11.29 KB, patch)
2013-01-16 17:11 PST, Jinwoo Song
no flags Details | Formatted Diff | Diff
Patch (11.31 KB, patch)
2013-01-17 21:59 PST, Jinwoo Song
no flags Details | Formatted Diff | Diff
Patch (11.31 KB, patch)
2013-01-17 22:03 PST, Jinwoo Song
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jinwoo Song 2013-01-10 23:26:17 PST
Add APIs to set/get a view source mode for enabling to load the source code of the web page.
Comment 1 Jinwoo Song 2013-01-10 23:36:31 PST
Created attachment 182272 [details]
Patch
Comment 2 Benjamin Poulain 2013-01-14 17:28:02 PST
Comment on attachment 182272 [details]
Patch

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

Laszlo, feel free to review the EFL bits and then tell me in comments or on IRC when to rs+.

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:218
> +    void setInViewSourceMode(bool set) { m_inViewSourceMode = set; }
> +    bool inViewSourceMode() const { return m_inViewSourceMode; }

This may be better done on WebPageProxy?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:937
> +    impl->page()->setMainFrameInViewSourceMode(enabled);
> +    impl->setInViewSourceMode(enabled);

This is odd.
Comment 3 Gyuyoung Kim 2013-01-14 18:05:05 PST
(In reply to comment #2)
> (From update of attachment 182272 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182272&action=review
> 
> Laszlo, feel free to review the EFL bits and then tell me in comments or on IRC when to rs+.

Do you mean we should not set r+ even on EFL port patches ?
Comment 4 Benjamin Poulain 2013-01-14 18:16:18 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 182272 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=182272&action=review
> > 
> > Laszlo, feel free to review the EFL bits and then tell me in comments or on IRC when to rs+.
> 
> Do you mean we should not set r+ even on EFL port patches ?

Wasn't that the policy? I'll check my emails again.
Comment 5 Benjamin Poulain 2013-01-14 18:25:19 PST
> > Do you mean we should not set r+ even on EFL port patches ?
> 
> Wasn't that the policy? I'll check my emails again.

It looks like it (check the webkit-dev thread).
Comment 6 Gyuyoung Kim 2013-01-14 18:51:53 PST
(In reply to comment #5)
> > > Do you mean we should not set r+ even on EFL port patches ?
> > 
> > Wasn't that the policy? I'll check my emails again.
> 
> It looks like it (check the webkit-dev thread).

I don't know if the policy is decided on "[webkit-dev] Changes to the WebKit2 development process". It looks there are still some questions.
Comment 7 Jinwoo Song 2013-01-14 20:26:03 PST
Comment on attachment 182272 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.h:218
>> +    bool inViewSourceMode() const { return m_inViewSourceMode; }
> 
> This may be better done on WebPageProxy?

Yes, I totally agree with you. Then, I'll fix setMainFrameInViewSourceMode() to store the status and add mainFrameInViewSourceMode() to get the status. Is it plausible?
Comment 8 Jinwoo Song 2013-01-15 01:49:36 PST
Created attachment 182720 [details]
Patch
Comment 9 Gyuyoung Kim 2013-01-15 02:12:34 PST
Comment on attachment 182720 [details]
Patch

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

Looks good to me on EFL port side. But, it looks Apple reviewer might wanna take a final look.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:888
> + * Get the view source mode of the current web page.

Gets ?

> Tools/MiniBrowser/efl/main.c:300
> +    } else if  (!strcmp(ev->key, "F8")) {

Nit: Two spaces in "if ("
Comment 10 Jinwoo Song 2013-01-15 02:31:15 PST
Created attachment 182727 [details]
Patch

Fixed nits.
Comment 11 Gyuyoung Kim 2013-01-15 20:21:41 PST
Comment on attachment 182727 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:955
> +    EXPECT_FALSE(ewk_view_source_mode_get(webView()));

Can't you add a test for ewk_view_source_mode_set() at least ?
Comment 12 Jinwoo Song 2013-01-16 00:23:44 PST
Created attachment 182929 [details]
Patch

Applied Gyuyoung's comment.
Comment 13 Gyuyoung Kim 2013-01-16 01:43:29 PST
Comment on attachment 182929 [details]
Patch

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

LGTM except for trivial comment.

> Tools/MiniBrowser/efl/main.c:306
> +        Browser_Window *window = window_create(0, DEFAULT_URL, 0, 0, EINA_FALSE);

Shouldn't you use NULL instead of 0 for first parameter of window_create(Evas_Object* opener, const char *url, int width, int height) ?

> Tools/MiniBrowser/efl/main.c:1325
> +        window = window_create(0, url, 0, 0, EINA_FALSE);

ditto ?

> Tools/MiniBrowser/efl/main.c:1328
> +        window = window_create(0, DEFAULT_URL, 0, 0, EINA_FALSE);

ditto ?
Comment 14 Jinwoo Song 2013-01-16 17:11:12 PST
Created attachment 183066 [details]
Patch

Used NULL instead of 0 as Gyuyoung's comments.
Comment 15 Gyuyoung Kim 2013-01-16 18:21:48 PST
Comment on attachment 183066 [details]
Patch

Benjamin? If there is no comment, I would like to land this.
Comment 16 Benjamin Poulain 2013-01-17 17:45:02 PST
Comment on attachment 183066 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.h:1237
>  
> +    bool m_mainFrameInViewSourceMode;
> +

Please move the attribute close to the other mainFrame attributes. It is better to have related attributes grouped together.
Comment 17 Jinwoo Song 2013-01-17 21:59:23 PST
Created attachment 183371 [details]
Patch

Applied Benjamin's comment.
Comment 18 Jinwoo Song 2013-01-17 22:03:36 PST
Created attachment 183372 [details]
Patch
Comment 19 WebKit Review Bot 2013-01-21 17:54:16 PST
Comment on attachment 183372 [details]
Patch

Clearing flags on attachment: 183372

Committed r140376: <http://trac.webkit.org/changeset/140376>
Comment 20 WebKit Review Bot 2013-01-21 17:54:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Sam Weinig 2013-01-21 18:04:25 PST
In the future, please (In reply to comment #6)
> (In reply to comment #5)
> > > > Do you mean we should not set r+ even on EFL port patches ?
> > > 
> > > Wasn't that the policy? I'll check my emails again.
> > 
> > It looks like it (check the webkit-dev thread).
> 
> I don't know if the policy is decided on "[webkit-dev] Changes to the WebKit2 development process". It looks there are still some questions.

The policy was decided.  Please don't review patches that touch WebKit2 if you are not listed in the Owners file.