Bug 124617 – Fill out WebOriginDataManager, though it is still dormant

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit2

[Martin Hock]

124331 was too big, so this includes the changes to WebOriginDataManager without enabling it yet.

[Martin Hock]

Created attachment 217354 [details]
patch

[WebKit Commit Bot]

Attachment 217354 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/Shared/SecurityOriginDataHash.h', u'Source/WebKit2/Shared/StoredSecurityOriginData.cpp', u'Source/WebKit2/Shared/StoredSecurityOriginData.h', u'Source/WebKit2/Shared/WebStoredSecurityOrigin.h', u'Source/WebKit2/UIProcess/API/C/WKOriginDataManager.cpp', u'Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxy.h', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxy.messages.in', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxyChangeClient.cpp', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxyChangeClient.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Cookies/WebCookieManager.h', u'Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp', u'Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.h', u'Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.messages.in']" exit_code: 1
Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit2/Shared/StoredSecurityOriginData.cpp:96:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Hock]

Created attachment 217356 [details]
style

[WebKit Commit Bot]

Attachment 217356 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/Shared/SecurityOriginDataHash.h', u'Source/WebKit2/Shared/StoredSecurityOriginData.cpp', u'Source/WebKit2/Shared/StoredSecurityOriginData.h', u'Source/WebKit2/Shared/WebStoredSecurityOrigin.h', u'Source/WebKit2/UIProcess/API/C/WKOriginDataManager.cpp', u'Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxy.h', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxy.messages.in', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxyChangeClient.cpp', u'Source/WebKit2/UIProcess/WebOriginDataManagerProxyChangeClient.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Cookies/WebCookieManager.h', u'Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp', u'Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.h', u'Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.messages.in']" exit_code: 1
Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Hock]

*** Bug 124331 has been marked as a duplicate of this bug. ***

[Martin Hock]

Created attachment 217359 [details]
bug #

[Tim Horton]

Comment on attachment 217356 [details]
style

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

> Source/WebKit2/ChangeLog:92
> [phone]  Martin Hock  <[e-mail]>
> +
> +        Fill out WebOriginDataManager, though it is still dormant.
> +        Need the bug URL (OOPS!).

you seem to have two change log entries

> Source/WebKit2/Shared/WebStoredSecurityOrigin.h:56
> +            return 0;

nullptr

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:178
> +    for (auto &i : securityOrigins)

I think the & goes on the other side

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:388
> +    if (types & kWKCookieOriginData) {

early return?

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:402
> +    if (types & kWKCookieOriginData) {

early return?

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.h:89
> +    void processPluginData(const Vector<String> &);

no space before the &

> Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:69
> +    // FIXME this line is probabaly not needed: DatabaseManager::manager().setClient(this);

o...kay?

> Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:192
> +#if ENABLE(VIDEO)

If you shove this out of the 'if' you can remove the curly braces.

> Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:200
> +

extra space. there are a few of these dotted around your patch

> Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:229
> +    // kWKKeyValueStorageOriginData handled by WebOriginDataManagerProxy

These comments give me pause, though I understand what they're for.

> Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:232
> +#if ENABLE(VIDEO)

ditto on the hoisting and removing of braces

> Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.h:84
> +

extra space

[EFL EWS Bot]

Comment on attachment 217359 [details]
bug #

Attachment 217359 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/29738006

[Martin Hock]

Comment on attachment 217356 [details]
style

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

>> Source/WebKit2/ChangeLog:92
>> +        Need the bug URL (OOPS!).
> 
> you seem to have two change log entries

Yikes, if you look down you can actually see another change that shouldn't be in here as well.  Excised.

>> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:178
>> +    for (auto &i : securityOrigins)
> 
> I think the & goes on the other side

Yep.  I'll go through and clean up my autos to use const auto& whenever possible.

>> Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:69
>> +    // FIXME this line is probabaly not needed: DatabaseManager::manager().setClient(this);
> 
> o...kay?

Yeah, I removed support for client so I shouldn't set the client here.  This was just a note to myself that I forgot to delete, thanks for pointing it out!

>> Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:229
>> +    // kWKKeyValueStorageOriginData handled by WebOriginDataManagerProxy
> 
> These comments give me pause, though I understand what they're for.

Yeah, it's mainly because I am not going to get some kind of warning for leaving some enum values out in a switch statement, but I at least want to call out the fact that some of these are missing.  Actually, why don't I actually create the if (types & kUnusedThingy) and put ASSERT_NOT_REACHED in there?

[Martin Hock]

Created attachment 217456 [details]
address comments + build fix

[Martin Hock]

Created attachment 218334 [details]
rebase + switch to API::Array iterator + style fixes

[Tim Horton]

Comment on attachment 218334 [details]
rebase + switch to API::Array iterator + style fixes

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

with no change log comments and a lack of domain knowledge I can't be the final review. but I found some things!

> Source/WebKit2/ChangeLog:8
> +        * CMakeLists.txt:

Some words about the purpose of things would be nice. This is way too big of a patch for a barren changelog.

> Source/WebKit2/Shared/WebStoredSecurityOrigin.h:63
> +    WKOriginDataTypes types() const {return m_types; }

missing a space before the return.

> Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:80
> +static void addOr(HashMap<SecurityOriginData, WKOriginDataTypes>& map, const SecurityOriginData& key, WKOriginDataTypes val)

weird name

> Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:272
> +void WebOriginDataManager::getAcceptPolicy(WKOriginDataTypes types, uint64_t callbackID)

'get' is more or less reserved for functions with out parameters.

(but then I see that this is a message name, and there's already one like it, so maybe it's ok. I'm not really sure).

[Martin Hock]

(In reply to comment #12)
> (From update of attachment 218334 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218334&action=review
> 
> with no change log comments and a lack of domain knowledge I can't be the final review. but I found some things!

Thanks!

> 
> > Source/WebKit2/ChangeLog:8
> > +        * CMakeLists.txt:
> 
> Some words about the purpose of things would be nice. This is way too big of a patch for a barren change log.

OK, I'll add some more description.

> 
> > Source/WebKit2/Shared/WebStoredSecurityOrigin.h:63
> > +    WKOriginDataTypes types() const {return m_types; }
> 
> missing a space before the return.

Guess I spaced out there... (sorry...)

> 
> > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:80
> > +static void addOr(HashMap<SecurityOriginData, WKOriginDataTypes>& map, const SecurityOriginData& key, WKOriginDataTypes val)
> 
> weird name

I'll change it to "addTypeToMap", unless someone has a better suggestion.

> 
> > Source/WebKit2/WebProcess/OriginData/WebOriginDataManager.cpp:272
> > +void WebOriginDataManager::getAcceptPolicy(WKOriginDataTypes types, uint64_t callbackID)
> 
> 'get' is more or less reserved for functions with out parameters.
> 
> (but then I see that this is a message name, and there's already one like it, so maybe it's ok. I'm not really sure).

I was trying to follow the lead of the existing messages for the different Web*Managers.  They would typically take a callbackID and not an out parameter.  It's true that they usually wouldn't take multiple arguments, though I see that WebPage does do this for its Get messages, so it's not unprecedented?

[Martin Hock]

Created attachment 218444 [details]
ChangeLog + rebase + minor cleanup

[Martin Hock]

Created attachment 218464 [details]
cleanup

[Martin Hock]

(In reply to comment #15)
> Created an attachment (id=218464) [details]
> cleanup

The last patch had an extra newline to appease check-webkit-style, but that was fixed here: https://bugs.webkit.org/show_bug.cgi?id=125246

Also, the deprecated WKOriginDataManagerChangeClient struct was not in use, so I removed it.

[EFL EWS Bot]

Comment on attachment 218464 [details]
cleanup

Attachment 218464 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/42018084

[Martin Hock]

Created attachment 218924 [details]
rebase + allow !ENABLE(NETSCAPE_PLUGIN_API)

[Brady Eidson]

Comment on attachment 218924 [details]
rebase + allow !ENABLE(NETSCAPE_PLUGIN_API)

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

I didn't review all the way because I identified a few high level issues.

> Source/WebKit2/ChangeLog:20
> +        * Shared/StoredSecurityOriginData.cpp: Added.
> +        A combination of SecurityOriginData and WKOriginDataTypes to indicate how the SecurityOriginData is stored. Data is public (struct-like).

For internal use having this new object seems fine...  but instead of having 4 members, couldn't it just have 2?  The SecurityOrigin and the DataTypes?

> Source/WebKit2/ChangeLog:30
> +        * Shared/WebStoredSecurityOrigin.h: Added.
> +        The APIObject version of StoredSecurityOriginData. Data is accessed via methods.

Is this actually exposed as API?  I don't see where.

I'm not convinced we need a new API object for this.  A directory with the WKSecurityOrigin and the data types would work without expanding the API surface area.

> Source/WebKit2/Shared/SecurityOriginDataHash.h:36
> +inline bool operator==(const SecurityOriginData&, const SecurityOriginData&);

I think this should be in SecurityOriginData.h/cpp, not SecurityOriginDataHash.

> Source/WebKit2/Shared/SecurityOriginDataHash.h:98
> +inline bool operator==(const SecurityOriginData& a, const SecurityOriginData& b)
> +{
> +    if (WTF::HashTraits<SecurityOriginData>::isDeletedValue(a))
> +        return (WTF::HashTraits<SecurityOriginData>::isDeletedValue(b));
> +
> +    if (WTF::HashTraits<SecurityOriginData>::isDeletedValue(b))
> +        return false;
> +
> +    return a.host == b.host && a.protocol == b.protocol && a.port == b.port;
> +}

Maybe you put the == in the Hash header because you figured the deletedValue stuff was important for equality?

I'm not so sure it is.  If you have a SecurityOriginData object and it represents the deletedValue, and you compare it with any other SecurityOriginData based on host, protocol, and port...  that should still work.  All without any special knowledge of what a "deletedValue" is.

> Source/WebKit2/Shared/StoredSecurityOriginData.cpp:43
> +StoredSecurityOriginData StoredSecurityOriginData::fromSecurityOriginAndTypes(SecurityOrigin* securityOrigin, WKOriginDataTypes types)
> +{
> +    StoredSecurityOriginData storedSecurityOriginData;
> +
> +    storedSecurityOriginData.protocol = securityOrigin->protocol();

What guarantee's securityOrigin isn't null before dereferencing it?

If it can never be null, then the argument to the method should be SecurityOrigin&, not SecurityOrigin*.

> Source/WebKit2/Shared/StoredSecurityOriginData.cpp:95
> +    ASSERT(callback);
> +    if (!callback)
> +        return;

This is a weird pattern, ASSERTing not null and then immediately null checking.

An ASSERT means it should never ever  happen - why should it never ever happen, but also be important to null check?

(Yes, I know you can find this pattern elsewhere in the code, but we're trying to be much more mindful of it going forward)

> Source/WebKit2/Shared/WebStoredSecurityOrigin.h:41
> +    static PassRefPtr<WebStoredSecurityOrigin> createFromString(const String& string, WKOriginDataTypes types)
> +    {
> +        return create(WebCore::SecurityOrigin::createFromString(string), types);
> +    }

Is this ever used?  Let's try to avoid adding this.

> Source/WebKit2/Shared/WebStoredSecurityOrigin.h:46
> +    static PassRefPtr<WebStoredSecurityOrigin> createFromDatabaseIdentifier(const String& identifier, WKOriginDataTypes types)
> +    {
> +        return create(WebCore::SecurityOrigin::createFromDatabaseIdentifier(identifier), types);
> +    }

Ditto.

> Source/WebKit2/Shared/WebStoredSecurityOrigin.h:51
> +    static PassRefPtr<WebStoredSecurityOrigin> create(const String& protocol, const String& host, int port, WKOriginDataTypes types)
> +    {
> +        return create(WebCore::SecurityOrigin::create(protocol, host, port), types);
> +    }

This one is used...

> Source/WebKit2/Shared/WebStoredSecurityOrigin.h:58
> +    static PassRefPtr<WebStoredSecurityOrigin> create(PassRefPtr<WebCore::SecurityOrigin> securityOrigin, WKOriginDataTypes types)
> +    {
> +        if (!securityOrigin)
> +            return nullptr;
> +        return adoptRef(new WebStoredSecurityOrigin(securityOrigin, types));
> +    }

...but not this one.

Let's only add what's needed.  Especially since the string and databaseIdentifier versions are vile.

> Source/WebKit2/UIProcess/API/C/WKOriginDataManager.cpp:71
> +void WKOriginDataManagerStoredOriginsToOrigins(WKArrayRef arrayRef, WKErrorRef error, void* callback)

This isn't used in this patch...?

> Source/WebKit2/UIProcess/API/C/WKOriginDataManager.cpp:84
> +void WKOriginDataManagerStoredOriginsToHostnames(WKArrayRef arrayRef, WKErrorRef error, void* callback)

Ditto?

> Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h:80
> +// Used as callback adapters internally
> +void WKOriginDataManagerStoredOriginsToOrigins(WKArrayRef arrayRef, WKErrorRef error, void* callback);
> +void WKOriginDataManagerStoredOriginsToHostnames(WKArrayRef arrayRef, WKErrorRef error, void* callback);

If they're used internally (which I don't think they are) then they shouldn't be in public API headers.

[Martin Hock]

(In reply to comment #19)
> (From update of attachment 218924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218924&action=review
> 
> I didn't review all the way because I identified a few high level issues.
> 
> > Source/WebKit2/ChangeLog:20
> > +        * Shared/StoredSecurityOriginData.cpp: Added.
> > +        A combination of SecurityOriginData and WKOriginDataTypes to indicate how the SecurityOriginData is stored. Data is public (struct-like).
> 
> For internal use having this new object seems fine...  but instead of having 4 members, couldn't it just have 2?  The SecurityOrigin and the DataTypes?

I would have to make a special constructor for SecurityOrigin to use them because the normal constructor creates a URL out of the arguments and parses it, which leads to issues for my situation.  I could make it a pair of SecurityOriginData and the DataTypes.

> 
> > Source/WebKit2/ChangeLog:30
> > +        * Shared/WebStoredSecurityOrigin.h: Added.
> > +        The APIObject version of StoredSecurityOriginData. Data is accessed via methods.
> 
> Is this actually exposed as API?  I don't see where.
> 
> I'm not convinced we need a new API object for this.  A directory with the WKSecurityOrigin and the data types would work without expanding the API surface area.

Yes, see WKOriginDataManagerGetStoredOrigins.  The WKArrayRef contains WebSecurityOriginData.

I'm not sure what you mean by a directory.  I'm looking at APIObject::Type and I don't see Directory in there.  Do you mean Dictionary?  I didn't see an example of an API::Array of Dictionaries so I was a little wary of that, but I'm happy to try it out.

> 
> > Source/WebKit2/Shared/SecurityOriginDataHash.h:36
> > +inline bool operator==(const SecurityOriginData&, const SecurityOriginData&);
> 
> I think this should be in SecurityOriginData.h/cpp, not SecurityOriginDataHash.
> 
> > Source/WebKit2/Shared/SecurityOriginDataHash.h:98
> > +inline bool operator==(const SecurityOriginData& a, const SecurityOriginData& b)
> > +{
> > +    if (WTF::HashTraits<SecurityOriginData>::isDeletedValue(a))
> > +        return (WTF::HashTraits<SecurityOriginData>::isDeletedValue(b));
> > +
> > +    if (WTF::HashTraits<SecurityOriginData>::isDeletedValue(b))
> > +        return false;
> > +
> > +    return a.host == b.host && a.protocol == b.protocol && a.port == b.port;
> > +}
> 
> Maybe you put the == in the Hash header because you figured the deletedValue stuff was important for equality?
> 
> I'm not so sure it is.  If you have a SecurityOriginData object and it represents the deletedValue, and you compare it with any other SecurityOriginData based on host, protocol, and port...  that should still work.  All without any special knowledge of what a "deletedValue" is.

At one point, I definitely ran into issues with DeletedValue, but I can check to see if this is really necessary.  I could just delete it for now and restore the code if it turns out to be needed when we switch to using WebOriginDataManager.

> 
> > Source/WebKit2/Shared/StoredSecurityOriginData.cpp:43
> > +StoredSecurityOriginData StoredSecurityOriginData::fromSecurityOriginAndTypes(SecurityOrigin* securityOrigin, WKOriginDataTypes types)
> > +{
> > +    StoredSecurityOriginData storedSecurityOriginData;
> > +
> > +    storedSecurityOriginData.protocol = securityOrigin->protocol();
> 
> What guarantee's securityOrigin isn't null before dereferencing it?
> 
> If it can never be null, then the argument to the method should be SecurityOrigin&, not SecurityOrigin*.

I was just following the lead of SecurityOriginData.  I'll fix that one too.

> 
> > Source/WebKit2/Shared/StoredSecurityOriginData.cpp:95
> > +    ASSERT(callback);
> > +    if (!callback)
> > +        return;
> 
> This is a weird pattern, ASSERTing not null and then immediately null checking.
> 
> An ASSERT means it should never ever  happen - why should it never ever happen, but also be important to null check?
> 
> (Yes, I know you can find this pattern elsewhere in the code, but we're trying to be much more mindful of it going forward)

Yes, again, this was just following SecurityOriginData which included a FIXME.  If you'd like, I can leave that one alone.

> 
> > Source/WebKit2/Shared/WebStoredSecurityOrigin.h:41
> > +    static PassRefPtr<WebStoredSecurityOrigin> createFromString(const String& string, WKOriginDataTypes types)
> > +    {
> > +        return create(WebCore::SecurityOrigin::createFromString(string), types);
> > +    }
> 
> Is this ever used?  Let's try to avoid adding this.

I don't think I need this one, so I will delete it.

> 
> > Source/WebKit2/Shared/WebStoredSecurityOrigin.h:46
> > +    static PassRefPtr<WebStoredSecurityOrigin> createFromDatabaseIdentifier(const String& identifier, WKOriginDataTypes types)
> > +    {
> > +        return create(WebCore::SecurityOrigin::createFromDatabaseIdentifier(identifier), types);
> > +    }
> 
> Ditto.

Same.

> 
> > Source/WebKit2/Shared/WebStoredSecurityOrigin.h:51
> > +    static PassRefPtr<WebStoredSecurityOrigin> create(const String& protocol, const String& host, int port, WKOriginDataTypes types)
> > +    {
> > +        return create(WebCore::SecurityOrigin::create(protocol, host, port), types);
> > +    }
> 
> This one is used...
> 
> > Source/WebKit2/Shared/WebStoredSecurityOrigin.h:58
> > +    static PassRefPtr<WebStoredSecurityOrigin> create(PassRefPtr<WebCore::SecurityOrigin> securityOrigin, WKOriginDataTypes types)
> > +    {
> > +        if (!securityOrigin)
> > +            return nullptr;
> > +        return adoptRef(new WebStoredSecurityOrigin(securityOrigin, types));
> > +    }
> 
> ...but not this one.
> 
> Let's only add what's needed.  Especially since the string and databaseIdentifier versions are vile.

All right!

> 
> > Source/WebKit2/UIProcess/API/C/WKOriginDataManager.cpp:71
> > +void WKOriginDataManagerStoredOriginsToOrigins(WKArrayRef arrayRef, WKErrorRef error, void* callback)
> 
> This isn't used in this patch...?
> 
> > Source/WebKit2/UIProcess/API/C/WKOriginDataManager.cpp:84
> > +void WKOriginDataManagerStoredOriginsToHostnames(WKArrayRef arrayRef, WKErrorRef error, void* callback)
> 
> Ditto?

That's true, they'll be used in the subsequent patch.  I'll put them back for that one - they will make more sense in context.

> 
> > Source/WebKit2/UIProcess/API/C/WKOriginDataManager.h:80
> > +// Used as callback adapters internally
> > +void WKOriginDataManagerStoredOriginsToOrigins(WKArrayRef arrayRef, WKErrorRef error, void* callback);
> > +void WKOriginDataManagerStoredOriginsToHostnames(WKArrayRef arrayRef, WKErrorRef error, void* callback);
> 
> If they're used internally (which I don't think they are) then they shouldn't be in public API headers.

Right, "internal" was not the best way of putting it.  I just meant that they are used as callback adapters for the OriginDataManager-based implementation of the other WK* classes it's meant to replace.

[Brady Eidson]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 218924 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=218924&action=review
> > 
> > I didn't review all the way because I identified a few high level issues.
> > 
> > > Source/WebKit2/ChangeLog:20
> > > +        * Shared/StoredSecurityOriginData.cpp: Added.
> > > +        A combination of SecurityOriginData and WKOriginDataTypes to indicate how the SecurityOriginData is stored. Data is public (struct-like).
> > 
> > For internal use having this new object seems fine...  but instead of having 4 members, couldn't it just have 2?  The SecurityOrigin and the DataTypes?
> 
> I would have to make a special constructor for SecurityOrigin to use them because the normal constructor creates a URL out of the arguments and parses it, which leads to issues for my situation.  I could make it a pair of SecurityOriginData and the DataTypes.

SecurityOriginData - being a tuple of those 3 things - is fine!

> 
> > 
> > > Source/WebKit2/ChangeLog:30
> > > +        * Shared/WebStoredSecurityOrigin.h: Added.
> > > +        The APIObject version of StoredSecurityOriginData. Data is accessed via methods.
> > 
> > Is this actually exposed as API?  I don't see where.
> > 
> > I'm not convinced we need a new API object for this.  A directory with the WKSecurityOrigin and the data types would work without expanding the API surface area.
> 
> Yes, see WKOriginDataManagerGetStoredOrigins.  The WKArrayRef contains WebSecurityOriginData.
> 
> I'm not sure what you mean by a directory.  I'm looking at APIObject::Type and I don't see Directory in there.  Do you mean Dictionary?  I didn't see an example of an API::Array of Dictionaries so I was a little wary of that, but I'm happy to try it out.

Yes - dictionary, with a typo, and damn-you-autocorrected to directory  :)

I could have sworn we pass dictionaries around somewhere with preset keys...

> > 
> > > Source/WebKit2/Shared/SecurityOriginDataHash.h:36
> > > +inline bool operator==(const SecurityOriginData&, const SecurityOriginData&);
> > 
> > I think this should be in SecurityOriginData.h/cpp, not SecurityOriginDataHash.
> > 
> > > Source/WebKit2/Shared/SecurityOriginDataHash.h:98
> > > +inline bool operator==(const SecurityOriginData& a, const SecurityOriginData& b)
> > > +{
> > > +    if (WTF::HashTraits<SecurityOriginData>::isDeletedValue(a))
> > > +        return (WTF::HashTraits<SecurityOriginData>::isDeletedValue(b));
> > > +
> > > +    if (WTF::HashTraits<SecurityOriginData>::isDeletedValue(b))
> > > +        return false;
> > > +
> > > +    return a.host == b.host && a.protocol == b.protocol && a.port == b.port;
> > > +}
> > 
> > Maybe you put the == in the Hash header because you figured the deletedValue stuff was important for equality?
> > 
> > I'm not so sure it is.  If you have a SecurityOriginData object and it represents the deletedValue, and you compare it with any other SecurityOriginData based on host, protocol, and port...  that should still work.  All without any special knowledge of what a "deletedValue" is.
> 
> At one point, I definitely ran into issues with DeletedValue, but I can check to see if this is really necessary.  I could just delete it for now and restore the code if it turns out to be needed when we switch to using WebOriginDataManager.

I'd prefer that.

> > > Source/WebKit2/Shared/StoredSecurityOriginData.cpp:43
> > > +StoredSecurityOriginData StoredSecurityOriginData::fromSecurityOriginAndTypes(SecurityOrigin* securityOrigin, WKOriginDataTypes types)
> > > +{
> > > +    StoredSecurityOriginData storedSecurityOriginData;
> > > +
> > > +    storedSecurityOriginData.protocol = securityOrigin->protocol();
> > 
> > What guarantee's securityOrigin isn't null before dereferencing it?
> > 
> > If it can never be null, then the argument to the method should be SecurityOrigin&, not SecurityOrigin*.
> 
> I was just following the lead of SecurityOriginData.  I'll fix that one too.

Please don't expand the scope of this patch to fix pre-existing code, but feel free to cleanup the old case in a separate patch!

> > > Source/WebKit2/Shared/StoredSecurityOriginData.cpp:95
> > > +    ASSERT(callback);
> > > +    if (!callback)
> > > +        return;
> > 
> > This is a weird pattern, ASSERTing not null and then immediately null checking.
> > 
> > An ASSERT means it should never ever  happen - why should it never ever happen, but also be important to null check?
> > 
> > (Yes, I know you can find this pattern elsewhere in the code, but we're trying to be much more mindful of it going forward)
> 
> Yes, again, this was just following SecurityOriginData which included a FIXME.  If you'd like, I can leave that one alone.

You should also decide if it really can be null, in which case just the null check is good...  or if it really shouldn't be null, in which case the ASSERT is good.

> > 
> > > Source/WebKit2/UIProcess/API/C/WKOriginDataManager.cpp:71
> > > +void WKOriginDataManagerStoredOriginsToOrigins(WKArrayRef arrayRef, WKErrorRef error, void* callback)
> > 
> > This isn't used in this patch...?
> > 
> > > Source/WebKit2/UIProcess/API/C/WKOriginDataManager.cpp:84
> > > +void WKOriginDataManagerStoredOriginsToHostnames(WKArrayRef arrayRef, WKErrorRef error, void* callback)
> > 
> > Ditto?
> 
> That's true, they'll be used in the subsequent patch.  I'll put them back for that one - they will make more sense in context.

Indeed!  (We try not to land dead code)

[Martin Hock]

Created attachment 219202 [details]
address comments

[Martin Hock]

Created attachment 219344 [details]
eliminate unused include

[Brady Eidson]

Comment on attachment 219344 [details]
eliminate unused include

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

> Source/WebKit2/ChangeLog:11
> +        * Shared/APIObject.h: Add StoredSecurityOrigin object.

Thought we were getting rid of this?

> Source/WebKit2/Shared/APIObject.h:76
> +        StoredSecurityOrigin,

Thought we were getting rid of this?

> Source/WebKit2/Shared/StoredSecurityOriginData.cpp:41
> +    StoredSecurityOriginData storedSecurityOriginData = {{securityOrigin.protocol(), securityOrigin.host(), securityOrigin.port()}, types};

StoredSecurityOriginData already stores a SecurityOriginData.  Can't we create that SecurityOriginData directly from the securityOrigin, instead of breaking it down into component pieces?

This way if SecurityOriginData is ever extended in the future, StoredSecurityOriginData gets it for free, without having to revisit this call site.

> Source/WebKit2/Shared/StoredSecurityOriginData.cpp:48
> +    StoredSecurityOriginData storedSecurityOriginData = {{securityOrigin.protocol, securityOrigin.host, securityOrigin.port}, types};

Shouldn't the SecurityOriginData member just be constructed from the passed in SecurityOriginData directly?

This way if SecurityOriginData is ever extended in the future, StoredSecurityOriginData gets it for free, without having to revisit this call site.

> Source/WebKit2/Shared/StoredSecurityOriginData.cpp:55
> +    return SecurityOrigin::create(originData.protocol, originData.host, originData.port);

This site could create the SecurityOrigin from the stored SecurityOriginData.

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:44
> +class WebOriginDataManagerProxy::GetSitesWithDataState {

What is a "GetSitesWithDataState" object?  That name is confusing to me.

Does it need to be a member class of WebOriginDataManagerProxy?

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:84
> +class WebOriginDataManagerProxy::ClearSiteDataState {

What is a "ClearSiteDataState" object?  That name is confusing to me.

Does it need to be a member class of WebOriginDataManagerProxy?

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:219
> +    if (types & ~(kWKKeyValueStorageOriginData | kWKPluginDataOriginData)) {
> +        if (types & kWKCookieOriginData)
> +            context()->sendToNetworkingProcessRelaunchingIfNecessary(Messages::WebOriginDataManager::GetOrigins(kWKCookieOriginData, callbackID));
> +
> +        if (types & ~kWKCookieOriginData) {
> +            // FIXME (Multi-WebProcess): <rdar://problem/12239765> Make manipulating cache information work with per-tab WebProcess.
> +            context()->sendToAllProcessesRelaunchingThemIfNecessary(Messages::WebOriginDataManager::GetOrigins(types & ~(kWKKeyValueStorageOriginData | kWKPluginDataOriginData | kWKCookieOriginData), callbackID));
> +        }
> +    }

Uggh.  All of this bitwise operator stuff doesn't read easily.

I don't know if I have any particular suggestion.  I just feel it my duty to point out it's not easily readable.

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:341
> +    if (types & kWKCookieOriginData)
> +        context()->sendToNetworkingProcessRelaunchingIfNecessary(Messages::WebOriginDataManager::StartObservingChanges(kWKCookieOriginData));
> +    if (types & ~kWKCookieOriginData)
> +        context()->sendToAllProcessesRelaunchingThemIfNecessary(Messages::WebOriginDataManager::StartObservingChanges(types & ~kWKCookieOriginData));

Why the network/other divide here?

Couldn't all processes know which flags they are capable of monitoring, or required to monitor?

Doesn't the WebProcess still do some networking, and therefore still need to monitor cookie changes?

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:352
> +    if (types & kWKCookieOriginData)
> +        context()->sendToNetworkingProcessRelaunchingIfNecessary(Messages::WebOriginDataManager::StopObservingChanges(kWKCookieOriginData));
> +    if (types & ~kWKCookieOriginData)
> +        context()->sendToAllProcessesRelaunchingThemIfNecessary(Messages::WebOriginDataManager::StopObservingChanges(types & ~kWKCookieOriginData));

Why the network/other divide here?

Couldn't all processes know which flags they are capable of monitoring, or required to monitor?

Doesn't the WebProcess still do some networking, and therefore still need to monitor cookie changes?

[Martin Hock]

(In reply to comment #24)
> (From update of attachment 219344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219344&action=review
> 
> > Source/WebKit2/ChangeLog:11
> > +        * Shared/APIObject.h: Add StoredSecurityOrigin object.
> 
> Thought we were getting rid of this?
> 
> > Source/WebKit2/Shared/APIObject.h:76
> > +        StoredSecurityOrigin,
> 
> Thought we were getting rid of this?

Yes, thanks for pointing this out.

> > Source/WebKit2/Shared/StoredSecurityOriginData.cpp:41
> > +    StoredSecurityOriginData storedSecurityOriginData = {{securityOrigin.protocol(), securityOrigin.host(), securityOrigin.port()}, types};
> 
> StoredSecurityOriginData already stores a SecurityOriginData.  Can't we create that SecurityOriginData directly from the securityOrigin, instead of breaking it down into component pieces?
> 
> This way if SecurityOriginData is ever extended in the future, StoredSecurityOriginData gets it for free, without having to revisit this call site.
> 
> > Source/WebKit2/Shared/StoredSecurityOriginData.cpp:48
> > +    StoredSecurityOriginData storedSecurityOriginData = {{securityOrigin.protocol, securityOrigin.host, securityOrigin.port}, types};
> 
> Shouldn't the SecurityOriginData member just be constructed from the passed in SecurityOriginData directly?
> 
> This way if SecurityOriginData is ever extended in the future, StoredSecurityOriginData gets it for free, without having to revisit this call site.
> 
> > Source/WebKit2/Shared/StoredSecurityOriginData.cpp:55
> > +    return SecurityOrigin::create(originData.protocol, originData.host, originData.port);
> 
> This site could create the SecurityOrigin from the stored SecurityOriginData.

Yes, great suggestions!

> > Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:44
> > +class WebOriginDataManagerProxy::GetSitesWithDataState {
> 
> What is a "GetSitesWithDataState" object?  That name is confusing to me.
> 
> Does it need to be a member class of WebOriginDataManagerProxy?
> 
> > Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:84
> > +class WebOriginDataManagerProxy::ClearSiteDataState {
> 
> What is a "ClearSiteDataState" object?  That name is confusing to me.
> 
> Does it need to be a member class of WebOriginDataManagerProxy?

These are needed due to PluginProcessManager's implementation.  We need a callback mechanism with state because we have to manually iterate through one plugin at a time.  WebPluginSiteDataManager has similarly named classes.

> > Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:219
> > +    if (types & ~(kWKKeyValueStorageOriginData | kWKPluginDataOriginData)) {
> > +        if (types & kWKCookieOriginData)
> > +            context()->sendToNetworkingProcessRelaunchingIfNecessary(Messages::WebOriginDataManager::GetOrigins(kWKCookieOriginData, callbackID));
> > +
> > +        if (types & ~kWKCookieOriginData) {
> > +            // FIXME (Multi-WebProcess): <rdar://problem/12239765> Make manipulating cache information work with per-tab WebProcess.
> > +            context()->sendToAllProcessesRelaunchingThemIfNecessary(Messages::WebOriginDataManager::GetOrigins(types & ~(kWKKeyValueStorageOriginData | kWKPluginDataOriginData | kWKCookieOriginData), callbackID));
> > +        }
> > +    }
> 
> Uggh.  All of this bitwise operator stuff doesn't read easily.
> 
> I don't know if I have any particular suggestion.  I just feel it my duty to point out it's not easily readable.

I think it's more readable than if we had to call a bunch of methods to construct objects, though you may disagree.  At least logical operators have semantics and performance characteristics that are well-known to most programmers of C-based languages (ok, operator precedence is a bit weird but that's what parens are for).

> > Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:341
> > +    if (types & kWKCookieOriginData)
> > +        context()->sendToNetworkingProcessRelaunchingIfNecessary(Messages::WebOriginDataManager::StartObservingChanges(kWKCookieOriginData));
> > +    if (types & ~kWKCookieOriginData)
> > +        context()->sendToAllProcessesRelaunchingThemIfNecessary(Messages::WebOriginDataManager::StartObservingChanges(types & ~kWKCookieOriginData));
> 
> Why the network/other divide here?
> 
> Couldn't all processes know which flags they are capable of monitoring, or required to monitor?
> 
> Doesn't the WebProcess still do some networking, and therefore still need to monitor cookie changes?
> 
> > Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:352
> > +    if (types & kWKCookieOriginData)
> > +        context()->sendToNetworkingProcessRelaunchingIfNecessary(Messages::WebOriginDataManager::StopObservingChanges(kWKCookieOriginData));
> > +    if (types & ~kWKCookieOriginData)
> > +        context()->sendToAllProcessesRelaunchingThemIfNecessary(Messages::WebOriginDataManager::StopObservingChanges(types & ~kWKCookieOriginData));
> 
> Why the network/other divide here?
> 
> Couldn't all processes know which flags they are capable of monitoring, or required to monitor?
> 
> Doesn't the WebProcess still do some networking, and therefore still need to monitor cookie changes?

Regarding sending messages to all processes: we could do this, though it will result in additional messages being sent and discarded, though this code shouldn't be particularly performance sensitive.  I'll go ahead and simplify in the manner suggested.

Regarding WebProcess monitoring cookie changes, you could well be right that I wasn't picking them up.  After pushing this patch, I'll make sure that the cookie change monitoring works correctly before enabling WebOriginDataManager in the subsequent patch.

[Brady Eidson]

(In reply to comment #25)
> (In reply to comment #24)
> > > Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:219
> > > +    if (types & ~(kWKKeyValueStorageOriginData | kWKPluginDataOriginData)) {
> > > +        if (types & kWKCookieOriginData)
> > > +            context()->sendToNetworkingProcessRelaunchingIfNecessary(Messages::WebOriginDataManager::GetOrigins(kWKCookieOriginData, callbackID));
> > > +
> > > +        if (types & ~kWKCookieOriginData) {
> > > +            // FIXME (Multi-WebProcess): <rdar://problem/12239765> Make manipulating cache information work with per-tab WebProcess.
> > > +            context()->sendToAllProcessesRelaunchingThemIfNecessary(Messages::WebOriginDataManager::GetOrigins(types & ~(kWKKeyValueStorageOriginData | kWKPluginDataOriginData | kWKCookieOriginData), callbackID));
> > > +        }
> > > +    }
> > 
> > Uggh.  All of this bitwise operator stuff doesn't read easily.
> > 
> > I don't know if I have any particular suggestion.  I just feel it my duty to point out it's not easily readable.
> 
> I think it's more readable than if we had to call a bunch of methods to construct objects, though you may disagree.  At least logical operators have semantics and performance characteristics that are well-known to most programmers of C-based languages (ok, operator precedence is a bit weird but that's what parens are for).

I wasn't suggesting use objects.  And I wasn't saying bitwise operations are unreadable.

In the WebKit project we strive for readability/clarity - Well chosen names, good paragraphing, consistency in style, etc.  All of these things help make it so someone inexperienced with the code can learn it fairly quickly (without excessive commenting!), and somebody experienced with the code can read it like a story without having to stop and focus all too often.

I had to stop and *really* focus here.

I'm not saying the code is wrong, or should be rewritten with objects.  I'm suggesting to be aware of ways you can make things more obvious/readable to others who didn't write the code.

[Martin Hock]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > > Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:219
> > > > +    if (types & ~(kWKKeyValueStorageOriginData | kWKPluginDataOriginData)) {
> > > > +        if (types & kWKCookieOriginData)
> > > > +            context()->sendToNetworkingProcessRelaunchingIfNecessary(Messages::WebOriginDataManager::GetOrigins(kWKCookieOriginData, callbackID));
> > > > +
> > > > +        if (types & ~kWKCookieOriginData) {
> > > > +            // FIXME (Multi-WebProcess): <rdar://problem/12239765> Make manipulating cache information work with per-tab WebProcess.
> > > > +            context()->sendToAllProcessesRelaunchingThemIfNecessary(Messages::WebOriginDataManager::GetOrigins(types & ~(kWKKeyValueStorageOriginData | kWKPluginDataOriginData | kWKCookieOriginData), callbackID));
> > > > +        }
> > > > +    }
> > > 
> > > Uggh.  All of this bitwise operator stuff doesn't read easily.
> > > 
> > > I don't know if I have any particular suggestion.  I just feel it my duty to point out it's not easily readable.
> > 
> > I think it's more readable than if we had to call a bunch of methods to construct objects, though you may disagree.  At least logical operators have semantics and performance characteristics that are well-known to most programmers of C-based languages (ok, operator precedence is a bit weird but that's what parens are for).
> 
> I wasn't suggesting use objects.  And I wasn't saying bitwise operations are unreadable.
> 
> In the WebKit project we strive for readability/clarity - Well chosen names, good paragraphing, consistency in style, etc.  All of these things help make it so someone inexperienced with the code can learn it fairly quickly (without excessive commenting!), and somebody experienced with the code can read it like a story without having to stop and focus all too often.
> 
> I had to stop and *really* focus here.
> 
> I'm not saying the code is wrong, or should be rewritten with objects.  I'm suggesting to be aware of ways you can make things more obvious/readable to others who didn't write the code.

OK, thanks for providing some perspective.  It sounds like we don't want to create helper functions for this,  but I'll keep what you said in mind for the future.

[Martin Hock]

Created attachment 219691 [details]
address comments

I simplified the bit manipulation - we will send more set bits, but it's less obfuscated and fragile and shouldn't affect performance.

[EFL EWS Bot]

Comment on attachment 219691 [details]
address comments

Attachment 219691 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/50948116

[Martin Hock]

Created attachment 220272 [details]
rebase

[Martin Hock]

Created attachment 222365 [details]
rebase

[Martin Hock]

Comment on attachment 222365 [details]
rebase

Per discussion with Sam, I'm going to try merging this into Sessions.

[Anders Carlsson]

WebOriginDataManager is going away.