Bug 126626 - Stub for WebSession API
Summary: Stub for WebSession API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Martin Hock
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-08 00:38 PST by Martin Hock
Modified: 2014-01-08 19:57 PST (History)
9 users (show)

See Also:


Attachments
patch (28.31 KB, patch)
2014-01-08 00:40 PST, Martin Hock
no flags Details | Formatted Diff | Diff
rebase + style fixes (32.35 KB, patch)
2014-01-08 00:49 PST, Martin Hock
no flags Details | Formatted Diff | Diff
ChangeLog (27.06 KB, patch)
2014-01-08 01:03 PST, Martin Hock
sam: review+
Details | Formatted Diff | Diff
address comments (26.97 KB, patch)
2014-01-08 10:38 PST, Martin Hock
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
rebase (27.00 KB, patch)
2014-01-08 11:08 PST, Martin Hock
no flags Details | Formatted Diff | Diff
add reviewer (27.00 KB, patch)
2014-01-08 11:15 PST, Martin Hock
no flags Details | Formatted Diff | Diff
move Session to API namespace (27.08 KB, patch)
2014-01-08 12:32 PST, Martin Hock
no flags Details | Formatted Diff | Diff
style (27.08 KB, patch)
2014-01-08 12:34 PST, Martin Hock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Hock 2014-01-08 00:38:51 PST
Stub for WebSession API
Comment 1 Martin Hock 2014-01-08 00:40:12 PST
Created attachment 220604 [details]
patch
Comment 2 Martin Hock 2014-01-08 00:49:59 PST
Created attachment 220605 [details]
rebase + style fixes
Comment 3 Martin Hock 2014-01-08 01:03:35 PST
Created attachment 220606 [details]
ChangeLog
Comment 4 WebKit Commit Bot 2014-01-08 01:06:38 PST
Attachment 220606 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/API/c/WKBase.h', u'Source/WebKit2/Shared/API/c/WKSharedAPICast.h', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/UIProcess/API/C/WKSessionRef.cpp', u'Source/WebKit2/UIProcess/API/C/WKSessionRef.h', u'Source/WebKit2/UIProcess/API/Cocoa/WKSession.h', u'Source/WebKit2/UIProcess/API/Cocoa/WKSession.mm', u'Source/WebKit2/UIProcess/API/Cocoa/WKSessionInternal.h', u'Source/WebKit2/UIProcess/WebSession.cpp', u'Source/WebKit2/UIProcess/WebSession.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit2/UIProcess/WebSession.h:27:  WebSession_h is incorrect. #defined constants should use all uppercase names with words separated by underscores.  [readability/naming/define/constants] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Martin Hock 2014-01-08 01:14:36 PST
I don't understand this style error as it seems to follow the style guidelines for header guards.  Am I missing something?

(In reply to comment #4)
> Attachment 220606 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/API/c/WKBase.h', u'Source/WebKit2/Shared/API/c/WKSharedAPICast.h', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/UIProcess/API/C/WKSessionRef.cpp', u'Source/WebKit2/UIProcess/API/C/WKSessionRef.h', u'Source/WebKit2/UIProcess/API/Cocoa/WKSession.h', u'Source/WebKit2/UIProcess/API/Cocoa/WKSession.mm', u'Source/WebKit2/UIProcess/API/Cocoa/WKSessionInternal.h', u'Source/WebKit2/UIProcess/WebSession.cpp', u'Source/WebKit2/UIProcess/WebSession.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', '--commit-queue']" exit_code: 1
> ERROR: Source/WebKit2/UIProcess/WebSession.h:27:  WebSession_h is incorrect. #defined constants should use all uppercase names with words separated by underscores.  [readability/naming/define/constants] [4]
> Total errors found: 1 in 12 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Alexey Proskuryakov 2014-01-08 09:43:44 PST
> I don't understand this style error as it seems to follow the style guidelines for header guards.  Am I missing something?

This is a newly introduced mistake in style checker, will roll out in bug 126645.
Comment 7 Sam Weinig 2014-01-08 09:53:31 PST
Comment on attachment 220606 [details]
ChangeLog

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

With those changes, r=me.

> Source/WebKit2/UIProcess/API/Cocoa/WKSession.h:26
> +#include <WebKit2/WKSessionRef.h>

This should not be included. It is in some of the objective-c headers as a temporary renaming mitigation.

> Source/WebKit2/UIProcess/API/Cocoa/WKSession.h:28
> +#ifdef __OBJC__

This is also not needed.

> Source/WebKit2/UIProcess/WebSession.h:37
> +    bool getEphemeral();

This should be called either isEphemeral() or just ephemeral() (I prefer the isEphemeral()).
Comment 8 Martin Hock 2014-01-08 10:38:45 PST
Created attachment 220642 [details]
address comments
Comment 9 EFL EWS Bot 2014-01-08 10:58:56 PST
Comment on attachment 220642 [details]
address comments

Attachment 220642 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/5178054582206464
Comment 10 Martin Hock 2014-01-08 11:08:08 PST
Created attachment 220644 [details]
rebase
Comment 11 Martin Hock 2014-01-08 11:15:59 PST
Created attachment 220645 [details]
add reviewer
Comment 12 Anders Carlsson 2014-01-08 11:23:02 PST
Comment on attachment 220645 [details]
add reviewer

Here are some high-level comments I though of yesterday:

- WKSession is a very generic and non-descriptive name. I think it should be named to reflect what type of session it is.
- I had to look up what ephemeral meant, and I suspect other developers that aren't fluent in english will have to do so too. Maybe we can come up with a better name?
- I think the C++ object should be in the API namespace.
Comment 13 Sam Weinig 2014-01-08 11:31:40 PST
(In reply to comment #12)
> (From update of attachment 220645 [details])
> Here are some high-level comments I though of yesterday:
> 
> - WKSession is a very generic and non-descriptive name. I think it should be named to reflect what type of session it is.

In the end, I think the WKSession should have all the session specific information:
- Cookies
- Caches
- Local Storage
- Databases
- Etc.

> - I had to look up what ephemeral meant, and I suspect other developers that aren't fluent in english will have to do so too. Maybe we can come up with a better name?
- This is the name CFNetwork uses. I think it is quite good :(.

> - I think the C++ object should be in the API namespace.
Yeah, it should.
Comment 14 Martin Hock 2014-01-08 11:54:53 PST
(In reply to comment #13)
> (In reply to comment #12)
> > - I think the C++ object should be in the API namespace.
> Yeah, it should.

So WebSession should be in namespace API instead of namespace WebKit?  I'm not sure how this makes sense because API classes tend to be abstract concepts like Array, Number, Object, etc and the names all start with "API".  Should I give WebSession the same treatment?
Comment 15 Anders Carlsson 2014-01-08 11:59:27 PST
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > - I think the C++ object should be in the API namespace.
> > Yeah, it should.
> 
> So WebSession should be in namespace API instead of namespace WebKit?  I'm not sure how this makes sense because API classes tend to be abstract concepts like Array, Number, Object, etc and the names all start with "API".  

We've started using the API namespaces for objects that are exposed as API. Right now that means most of the abstract objects but we're moving more of the Web* classes over to API*.

> Should I give WebSession the same treatment?

Yes.
Comment 16 Martin Hock 2014-01-08 12:32:43 PST
Created attachment 220652 [details]
move Session to API namespace
Comment 17 Martin Hock 2014-01-08 12:34:10 PST
Created attachment 220653 [details]
style
Comment 18 WebKit Commit Bot 2014-01-08 19:57:50 PST
Comment on attachment 220653 [details]
style

Clearing flags on attachment: 220653

Committed r161542: <http://trac.webkit.org/changeset/161542>
Comment 19 WebKit Commit Bot 2014-01-08 19:57:53 PST
All reviewed patches have been landed.  Closing bug.