Bug 126626 – Stub for WebSession API

• Component: WebKit2
• Status: RESOLVED FIXED

[Martin Hock]

Stub for WebSession API

[Martin Hock]

Created attachment 220604 [details]
patch

[Martin Hock]

Created attachment 220605 [details]
rebase + style fixes

[Martin Hock]

Created attachment 220606 [details]
ChangeLog

[WebKit Commit Bot]

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.

[Martin Hock]

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.

[Alexey Proskuryakov]

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

[Sam Weinig]

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

[Martin Hock]

Created attachment 220642 [details]
address comments

[EFL EWS Bot]

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

[Martin Hock]

Created attachment 220644 [details]
rebase

[Martin Hock]

Created attachment 220645 [details]
add reviewer

[Anders Carlsson]

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.

[Sam Weinig]

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

[Martin Hock]

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

[Anders Carlsson]

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

[Martin Hock]

Created attachment 220652 [details]
move Session to API namespace

[Martin Hock]

Created attachment 220653 [details]
style

[WebKit Commit Bot]

Comment on attachment 220653 [details]
style

Clearing flags on attachment: 220653

Committed r161542: <http://trac.webkit.org/changeset/161542>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.