Summary: | [EFL][WK2] Add APIs to set/get view source mode | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jinwoo Song <jinwoo7.song> | ||||||||||||||||
Component: | WebKit EFL | Assignee: | Jinwoo Song <jinwoo7.song> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | benjamin, gyuyoung.kim, laszlo.gombos, lucas.de.marchi, rakuco, sam, webkitbottester, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Jinwoo Song
2013-01-10 23:26:17 PST
Created attachment 182272 [details]
Patch
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. (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 ? (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. > > 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).
(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 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? Created attachment 182720 [details]
Patch
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 (" Created attachment 182727 [details]
Patch
Fixed nits.
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 ? Created attachment 182929 [details]
Patch
Applied Gyuyoung's comment.
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 ? Created attachment 183066 [details]
Patch
Used NULL instead of 0 as Gyuyoung's comments.
Comment on attachment 183066 [details]
Patch
Benjamin? If there is no comment, I would like to land this.
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. Created attachment 183371 [details]
Patch
Applied Benjamin's comment.
Created attachment 183372 [details]
Patch
Comment on attachment 183372 [details] Patch Clearing flags on attachment: 183372 Committed r140376: <http://trac.webkit.org/changeset/140376> All reviewed patches have been landed. Closing bug. 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. |