Bug 106633 – [EFL][WK2] Add APIs to set/get view source mode

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Jinwoo Song]

Add APIs to set/get a view source mode for enabling to load the source code of the web page.

[Jinwoo Song]

Created attachment 182272 [details]
Patch

[Benjamin Poulain]

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.

[Gyuyoung Kim]

(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 ?

[Benjamin Poulain]

(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.

[Benjamin Poulain]

> > 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).

[Gyuyoung Kim]

(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.

[Jinwoo Song]

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?

[Jinwoo Song]

Created attachment 182720 [details]
Patch

[Gyuyoung Kim]

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 ("

[Jinwoo Song]

Created attachment 182727 [details]
Patch

Fixed nits.

[Gyuyoung Kim]

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 ?

[Jinwoo Song]

Created attachment 182929 [details]
Patch

Applied Gyuyoung's comment.

[Gyuyoung Kim]

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 ?

[Jinwoo Song]

Created attachment 183066 [details]
Patch

Used NULL instead of 0 as Gyuyoung's comments.

[Gyuyoung Kim]

Comment on attachment 183066 [details]
Patch

Benjamin? If there is no comment, I would like to land this.

[Benjamin Poulain]

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.

[Jinwoo Song]

Created attachment 183371 [details]
Patch

Applied Benjamin's comment.

[Jinwoo Song]

Created attachment 183372 [details]
Patch

[WebKit Review Bot]

Comment on attachment 183372 [details]
Patch

Clearing flags on attachment: 183372

Committed r140376: <http://trac.webkit.org/changeset/140376>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

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.